Page MenuHomeFreeBSD

Add aio_writev and aio_readv
AbandonedPublic

Authored by asomers on Dec 15 2020, 6:36 PM.

Details

Reviewers
jhb
tmunro
brooks
Group Reviewers
manpages
Summary

POSIX AIO is great, but it lacks vectored I/O functions. This commit fixes
that shortcoming by adding aio_writev and aio_readv. They aren't part of
the standard, but they're an obvious extension. They work just like their
synchronous equivalents writev and readv.

It isn't yet possible to use vectored aiocbs with lio_listio, but that could
be added in the future.

Test Plan

ATF test cases added

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 35539
Build 32440: arc lint + arc unit

Event Timeline

sys/kern/vfs_aio.c
1202

I'm not sure about these SDT probes. They were useful for me, but they might not be useful for anybody else. Think I should delete them?

sys/sys/aio.h
99

AFAICT, the only reason why the standard mandates volatile here is so the user can manipulate the aiocb's data while it's being processed by the kernel or vice versa. But that's dumb. I don't think it's necessary, so I didn't include it on aio_iov.

103

Should I add some kind of padding field for LP64 arches?

141

It turns out that the bp, pbuf, and pages fields were redundant. pbuf and pages were already stored in the bio, and bp is available during aio_biowakeup. So I could just delete those fields.

Great feature. +1 for allowing it in lio_listio() for batched submissions in later work (I'd also like to see LIO_FSYNC allowed in there too). I wonder if it is OK to use C11 anon union syntax in a system header; I see that sys/ucred.h does that.

bcr added inline comments.
lib/libc/sys/aio_write.2
87

No need for an extra line break here, continue to write after "system call".

tests/sys/aio/aio_test.c
1368

Is that s/DOS/DoS/ here?

Formatting changes suggested by bcr

The man page is OK now, thanks.
Remember to bump the .Dd for this content change when you do the commit.

I didn't look at the tests in detail.

The one larger suggestion I have is that I would not deal with vectored vs non-vectored in all the backends, instead I would do what the kernel does for regular read/write which is to always allocate a uio (it can live in struct kaiocb, and perhaps struct kaiocb should also contain a single struct iovec to minimize allocations for the non-vectored case) and have the I/O always be vectored in the backends. This would reduce some duplicated code to handle this in the socket and process_rw cases. I can take care of updating the Chelsio bits to cope (I think those are the only aio handlers you haven't already touched).

lib/libc/sys/aio_read.2
232

Maybe Xr readv(2) here for the description of iovec?

lib/libc/sys/aio_write.2
68

I think you can Xr writev(2) here for the description of struct iovec instead of duplicating it. Regardless of which approach, aio_readv(2) and aio_writev(2) should be consistent.

80

Maybe "write operations append to the file..."?

sys/kern/vfs_aio.c
817

Style: buddy the { up to the previous line.

1202

I'm not sure about these SDT probes. They were useful for me, but they might not be useful for anybody else. Think I should delete them?

I think they are fine, but perhaps a blank line before the static int?

2440

We don't use // comments in the kernel generally, and add a blank line before comments.

tests/sys/aio/aio_test.c
1330

Style: // comments vs /*.

asomers marked 8 inline comments as done.
  • Fix some whitespace in aio.h
  • Fix a memory leak in an error path

Respond to jhb's comments:

  • style
  • Create the struct uio in aio_aqueue, instead of in each separate backend.

Note that the Chelsio implementation for sockets will probably still work (I
haven't tested it), but it isn't taking advantage of the already-created uio).

Delete some errant commented code

kib added inline comments.
lib/libc/sys/Symbol.map
418

Should be alphabetically ordered.

sys/kern/vfs_aio.c
796

I believe the continuation line is wrongly indented.

813

Fix style while there:
error != 0
remove unneded ()

816

Extra () again.

1223

You can fill lines much more compact.

1238

style: != 0

1269

This check together with the last arg of vm_fault_quick_hold_pages() still left as atop(maxphys) + 1 makes me somewhat worried. I think that it would be better/safer to track number of slots in pages array and pass the right number to fault_quick_hold.

1303

void * casts are not needed.

1583

Style: we split lines after binary op, not before.

sys/sys/aio.h
141

I think this change should be extracted and reviewed/committed alone in advance.

222

Should be under BSD_VISIBLE guard, the functions are not POSIX.

asomers marked 9 inline comments as done.

Respond to kib's comments:

  • style
  • Guard the aio_writev and aio_readv declarations by BSD_VISIBLE
sys/kern/vfs_aio.c
1269

But the final argument to vm_fault_quick_hold_pages doesn't really do anything. It's just there for a sanity check. Given that, I think that passing a constant makes sense.

sys/sys/aio.h
141

Ok, I'll open another review for that.