Page MenuHomeFreeBSD
Differential D27624

Add aio_writev and aio_readv
AbandonedPublic
Actions

Authored by asomers on Dec 15 2020, 6:36 PM.
Tags
None
Referenced Files
F167484680: D27624.id80882.diff
Sat, Aug 22, 4:10 AM
Unknown Object (File)
Sun, Aug 9, 5:58 AM
Unknown Object (File)
Sat, Aug 8, 4:39 PM
Unknown Object (File)
Tue, Aug 4, 9:56 PM
Unknown Object (File)
Thu, Jul 30, 4:44 PM
Unknown Object (File)
Wed, Jul 29, 7:35 PM
Unknown Object (File)
Wed, Jul 29, 7:30 AM
Unknown Object (File)
Mon, Jul 27, 3:47 PM
View All Files
Subscribers
afedorov
bcr
freqlabs
imp
kib
mav
mmacy

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 Passed
Unit
No Test Coverage
Build Status
Buildable 35421
Build 32335: arc lint + arc unit

Event Timeline

asomers created this revision.Dec 15 2020, 6:36 PM
Herald added a reviewer: brooks. · View Herald TranscriptDec 15 2020, 6:36 PM
Herald added a reviewer: manpages. · View Herald Transcript
asomers requested review of this revision.Dec 15 2020, 6:36 PM
Harbormaster completed remote builds in B35421: Diff 80726.Dec 15 2020, 6:36 PM
asomers added inline comments.Dec 15 2020, 6:58 PM
sys/kern/vfs_aio.c
1230

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.

tmunro added a comment.Dec 16 2020, 2:32 AM

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 a subscriber: bcr.Dec 16 2020, 6:38 PM
bcr added inline comments.
lib/libc/sys/aio_write.2
88

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

tests/sys/aio/aio_test.c
1369

Is that s/DOS/DoS/ here?

asomers updated this revision to Diff 80801.Dec 16 2020, 10:54 PM

Formatting changes suggested by bcr

Harbormaster completed remote builds in B35464: Diff 80801.Dec 16 2020, 10:54 PM
asomers mentioned this in D26726: Add support for asynchrous file system operations to aio.Dec 17 2020, 3:35 AM
bcr accepted this revision as: manpages.Dec 17 2020, 8:40 AM

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

jhb added a comment.Dec 17 2020, 6:02 PM

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
225

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

lib/libc/sys/aio_write.2
69

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.

96

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

sys/kern/vfs_aio.c
841

Style: buddy the { up to the previous line.

1230

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?

2462

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

tests/sys/aio/aio_test.c
1331

Style: // comments vs /*.

asomers updated this revision to Diff 80882.Dec 18 2020, 5:08 AM
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).

Harbormaster completed remote builds in B35505: Diff 80882.Dec 18 2020, 5:08 AM
asomers updated this revision to Diff 80884.Dec 18 2020, 5:20 AM

Delete some errant commented code

Harbormaster completed remote builds in B35507: Diff 80884.Dec 18 2020, 5:20 AM
kib added a subscriber: kib.Dec 18 2020, 10:55 PM
kib added inline comments.
lib/libc/sys/Symbol.map
417

Should be alphabetically ordered.

sys/kern/vfs_aio.c
820

I believe the continuation line is wrongly indented.

836

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

840

Extra () again.

1256

You can fill lines much more compact.

1269

style: != 0

1323

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.

1360

void * casts are not needed.

1640

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.

219

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

asomers updated this revision to Diff 80953.Dec 19 2020, 4:06 PM
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
1323

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.

Harbormaster completed remote builds in B35539: Diff 80953.Dec 19 2020, 4:07 PM
asomers mentioned this in D27682: AIO: remove the kaiocb->bio linkage.Dec 21 2020, 5:26 PM
asomers abandoned this revision.Dec 23 2020, 7:04 PM
asomers mentioned this in D27743: Add aio_writev and aio_readv.

Replaced by D27743

Revision Contents
Changeset List

PathSize
lib/
libc/
sys/
2 lines
2 lines
6 lines
51 lines
2 lines
72 lines
share/
man/
man4/
4 lines
sys/
bsm/
2 lines
compat/
freebsd32/
6 lines
kern/
2 lines
87 lines
12 lines
350 lines
sys/
21 lines
tests/
sys/
aio/
662 lines

Diff 80726

View Options

lib/libc/sys/Makefile.inc

Loading...
View Options

lib/libc/sys/Symbol.map

Loading...
View Options

lib/libc/sys/aio_error.2

Loading...
View Options

lib/libc/sys/aio_read.2

Loading...
View Options

lib/libc/sys/aio_return.2

Loading...
View Options

lib/libc/sys/aio_write.2

Loading...
View Options

share/man/man4/aio.4

Loading...
View Options

sys/bsm/audit_kevents.h

Loading...
View Options

sys/compat/freebsd32/syscalls.master

Loading...
View Options

sys/kern/capabilities.conf

Loading...
View Options

sys/kern/sys_socket.c

Loading...
View Options

sys/kern/syscalls.master

Loading...
View Options

sys/kern/vfs_aio.c

Loading...
View Options

sys/sys/aio.h

Loading...
View Options

tests/sys/aio/aio_test.c

Loading...