Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libc: add support for AIO #5302

Open
atopia opened this issue Jul 18, 2024 · 12 comments
Open

libc: add support for AIO #5302

atopia opened this issue Jul 18, 2024 · 12 comments
Assignees
Labels

Comments

@atopia
Copy link
Contributor

atopia commented Jul 18, 2024

Add support for the AIO(4) asynchronous IO facility.

@atopia atopia self-assigned this Jul 18, 2024
@atopia atopia added the feature label Jul 18, 2024
atopia added a commit to atopia/genode that referenced this issue Jul 18, 2024
atopia added a commit to atopia/genode that referenced this issue Jul 18, 2024
atopia added a commit to atopia/genode that referenced this issue Jul 18, 2024
atopia added a commit to atopia/genode that referenced this issue Jul 18, 2024
atopia added a commit to atopia/genode that referenced this issue Jul 29, 2024
atopia added a commit to atopia/genode that referenced this issue Jul 29, 2024
atopia added a commit to atopia/genode that referenced this issue Jul 29, 2024
atopia added a commit to atopia/genode that referenced this issue Jul 29, 2024
atopia added a commit to atopia/genode that referenced this issue Jul 31, 2024
atopia added a commit to atopia/genode that referenced this issue Jul 31, 2024
atopia added a commit to atopia/genode that referenced this issue Jul 31, 2024
atopia added a commit to atopia/genode that referenced this issue Aug 7, 2024
atopia added a commit to atopia/genode that referenced this issue Aug 7, 2024
atopia added a commit to atopia/genode that referenced this issue Aug 7, 2024
atopia added a commit to atopia/genode that referenced this issue Aug 7, 2024
atopia added a commit to atopia/genode that referenced this issue Aug 7, 2024
atopia added a commit to atopia/genode that referenced this issue Aug 7, 2024
atopia added a commit to atopia/genode that referenced this issue Aug 12, 2024
atopia added a commit to atopia/genode that referenced this issue Aug 12, 2024
atopia added a commit to atopia/genode that referenced this issue Aug 12, 2024
atopia added a commit to atopia/genode that referenced this issue Aug 12, 2024
chelmuth added a commit to chelmuth/genode that referenced this issue Sep 2, 2024
@chelmuth
Copy link
Member

chelmuth commented Sep 2, 2024

I replaced the commit for async jobs by my initial proposal commit a6aced7 that keeps the semantic as tight as possible: onl one new API function monitor_async() in Monitor::Pool. This requires a rebase of other topic commits after addressing the upcoming review.

@chelmuth
Copy link
Member

chelmuth commented Sep 2, 2024

I finished the first review - please address my inline comments. Note, I made remarks about coding style issues but only once. Please lookout for further similar errors by yourself.

chelmuth added a commit to chelmuth/genode that referenced this issue Sep 2, 2024
chelmuth added a commit to chelmuth/genode that referenced this issue Sep 3, 2024
@chelmuth
Copy link
Member

chelmuth commented Sep 3, 2024

chelmuth added a commit that referenced this issue Sep 6, 2024
chelmuth added a commit that referenced this issue Sep 6, 2024
chelmuth pushed a commit to chelmuth/genode that referenced this issue Sep 6, 2024
chelmuth pushed a commit to chelmuth/genode that referenced this issue Sep 6, 2024
Make the Sync object accessible outside of Vfs_plugin for use in AIO.

Issue genodelabs#5302
chelmuth pushed a commit to chelmuth/genode that referenced this issue Sep 6, 2024
chelmuth pushed a commit to chelmuth/genode that referenced this issue Sep 6, 2024
chelmuth pushed a commit to chelmuth/genode that referenced this issue Sep 6, 2024
chelmuth pushed a commit to chelmuth/genode that referenced this issue Sep 6, 2024
Make the Sync object accessible outside of Vfs_plugin implementation,
e.g., for use in AIO.

Issue genodelabs#5302
chelmuth pushed a commit to chelmuth/genode that referenced this issue Sep 6, 2024
chelmuth pushed a commit to chelmuth/genode that referenced this issue Sep 6, 2024
atopia added a commit to atopia/genode that referenced this issue Sep 9, 2024
atopia added a commit to atopia/genode that referenced this issue Sep 9, 2024
atopia added a commit to atopia/genode that referenced this issue Sep 9, 2024
atopia added a commit to atopia/genode that referenced this issue Sep 11, 2024
@atopia
Copy link
Contributor Author

atopia commented Sep 11, 2024

@chelmuth please give my atopia/genode/tree/aio-rebased branch a spin

chelmuth pushed a commit to chelmuth/genode that referenced this issue Sep 16, 2024
chelmuth pushed a commit to chelmuth/genode that referenced this issue Sep 16, 2024
Make the Sync object accessible outside of Vfs_plugin implementation,
e.g., for use in AIO.

Issue genodelabs#5302
chelmuth pushed a commit to chelmuth/genode that referenced this issue Sep 16, 2024
chelmuth pushed a commit to chelmuth/genode that referenced this issue Sep 16, 2024
@chelmuth
Copy link
Member

Please see my new aio branch that includes all your changes plus some minor adjustments (libc port-hash update, indentation fixes).

Commit 4ed972c is special as it proposes to change the signatures of async_read and async_write from non-const reference parameters to result type. I suggest to move the seek-position handling into Vfs_plugin::write() as aio has no notion of seek pos, which removes the need for first_run.

I also noticed aio.cc suffers from indentation levels above 4, which could be addressed in a fixup in my opinion.

@atopia
Copy link
Contributor Author

atopia commented Sep 16, 2024

Unless I have a fundamental misunderstanding about the monitor mechanism, each Job will remain in the Job registry and will be run over and over again until it returns a Function_result::COMPLETE. Therefore it's crucial that bytes_written is also passed into async_write() calls and not only returned one-way as part of Async_result, because on a second call after a partial write, this state will be used to continue where async_write left off when returning false (i.e. incomplete) on the previous job run. Similarly, async_read() needs to learn about complete_read (there might be a better name for that one) in order to skip setting up an offset and queuing a read on a second run, after the first call to complete_read() returned READ_QUEUED.

As for the second idea: Why do you think that AIO does not have a seek pos?
Quoting aio_write(2):

If O_APPEND is not set for the file descriptor, the write operation will occur at the absolute position from the beginning of the file plus iocb->aio_offset.

I read that as meaning that an aio write needs to seek to offset and because the Job is run asynchronously inside the monitor, I found it best to directly include it into the async_write logic and Async_write_state. It is certainly possible to move the initial seek outside of async_(read|write) and let the execute() method in the AIO Function do the initial seeking and record the fact that it has been completed if that is preferred. That does not change either async method's need for having persistent state passed in though.

@atopia
Copy link
Contributor Author

atopia commented Sep 16, 2024

Now that I quoted it, it seems that I would need to check fd->flags for O_APPEND before honoring the offset parameter. I'll fix that when we have an agreement about where to seek.

@atopia
Copy link
Contributor Author

atopia commented Sep 16, 2024

Thanks for the little fixes!

@chelmuth
Copy link
Member

As for the second idea: Why do you think that AIO does not have a seek pos? Quoting aio_write(2):

If O_APPEND is not set for the file descriptor, the write operation will occur at the absolute position from the beginning of the file plus iocb->aio_offset.

The last sentence of the quoted paragraph from https://pubs.opengroup.org/onlinepubs/9799919799/functions/aio_write.html reads like follows.

After a successful call to enqueue an asynchronous I/O operation, the value of the file offset for the file is unspecified.

Blending this with O_APPEND does not re-enable a seek position, but an append mode. Do you already ensure the order of operations as stated in the spec?

Unfortunately, I entirely missed the fact that the VFS API still depends on the seek position stored in the VFS handle for implementing a pread/pwrite like semantic. Let's discuss offline tomorrow after I had some time to ponder on this.

@atopia
Copy link
Contributor Author

atopia commented Sep 17, 2024

Yes, I remember that the file offset is unspecified, but honestly I've just relied on the seek mechanism internally (basically thinking that just because it is unspecified after queueing an async op does not mean that I can't use it internally from within the subsystem). However - you are raising a valid point w.r.t. the order of operations. Although my understanding is that the monitor will run the Jobs in the order they were queued, for example async writes could still be interleaved if there is a partial write, then the job returns as incomplete, then the next async job on the same file runs with another partial write etc. I'll also think about what the requirements and guarantees really should be and then we can discuss offline!

atopia added a commit to atopia/genode that referenced this issue Sep 19, 2024
…nc_write"

See
genodelabs#5302 (comment)
for a discussion of the proposal.

This reverts commit 4ed972c.
atopia added a commit to atopia/genode that referenced this issue Sep 19, 2024
This tries to solve the problems around multiple queued async writes to
the same file for the non-append case, but still needs confirmation and
testing.

The append-case will work fine when async_write() is called from a
synchronous write operation (because that blocks until the write is
completed) but will likely end up with reordering or even intertwined
writes for async writes to files opened with O_APPEND>

POSIX.1-2024 2.8.2 Asynchronous I/O states:
"If O_APPEND is set for the file descriptor, or if aio_fildes is
associated with a device that is incapable of seeking, write operations
append to the file in the same order as the calls were made, with the
following exception: under implementation-defined circumstances, such as
operation on a multi-processor or when requests of differing priorities
are submitted at the same time, the ordering restriction may be
relaxed."

We could try to do a full write in the append case (just like with
O_NONBLOCK), but in that case we would change the normal, synchronous
case for a very slight improvment:
Since under the hood the Job Registry reverses the order of all
incomplete jobs after each run, such a reordering is even likely and
by Posix' "relaxed" ordering, doing to do async writes to O_APPEND files
doesn't seem like such a great idea anyway.

To recap: the current code might even end up with interleaved writes for
async writes to a file opened with O_APPEND. This could be mitigated by
attempting to write out the write request in full, which would lead to
possibly reordered but at least not interleaved writes to O_APPEND
files. We currently don't do this, because it would change the write
approach for synchronous writes to O_APPEND files as well, which seems
like the much saner case, and thus it does not seem like a good
tradeoff.

Issue genodelabs#5302
nfeske pushed a commit that referenced this issue Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants