Paths

Table of Contentst

Add support for asynchrous file system operations to aio
Needs RevisionPublic
Actions

Authored by mmacy on Oct 9 2020, 8:53 PM.

Details

Reviewers

asomers
jhb
scottl
imp

Group Reviewers

manpages

Summary

There is a WIP branch of openzfs that supports fully asynchronous reads and largely asynchronous writes (tx assignment and prefaulting indirect blocks might block). Leveraging this outside of zvol requires VFS support and changes to aio. This is fairly simplistic:

- limited to 32MB per operation and will fall back to synchronous for larger requests
uses a uio_bio which passes an array of held pages to the file system. The file system calls uiobiomove to do the actual data movement.
currently doesn't expose async fsync to aio

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 34965

Event Timeline

mmacy created this revision.Oct 9 2020, 8:53 PM

Herald added a subscriber: imp. · View Herald TranscriptOct 9 2020, 8:53 PM

mmacy requested review of this revision.Oct 9 2020, 8:53 PM

Harbormaster completed remote builds in B34076: Diff 78064.Oct 9 2020, 8:53 PM

Generally looks fine to me.

sys/kern/vfs_aio.c
1815	Maybe call it `aio_queue_vfs`? qasync is a pretty generic name but the function is VFS specific.

afedorov added a subscriber: afedorov.Oct 9 2020, 9:03 PM

mmacy added reviewers: asomers, jhb.Oct 9 2020, 9:04 PM

Could we get some documentation for the uiobiomove, aio_qasync, and VOP_UBOP ?

In D26726#595827, @asomers wrote:

Could we get some documentation for the uiobiomove, aio_qasync, and VOP_UBOP ?

It depends on how loosely you define 'documentation'. think the sanest thing is to follow precedent. uiobiomove should be documented in a manpage. aio_qasync is an implementation detail that perhaps justifies a few sentence comment on top of it. VOP_UBOP could stand a comment on top of it that describes the significance of the uio_bio parameters and expected return values. I don't envision it being used outside of aio.

It depends on how loosely you define 'documentation'. think the sanest thing is to follow precedent. uiobiomove should be documented in a manpage. aio_qasync is an implementation detail that perhaps justifies a few sentence comment on top of it. VOP_UBOP could stand a comment on top of it that describes the significance of the uio_bio parameters and expected return values. I don't envision it being used outside of aio.

+1 to comments for aio_qasync and man page for uiobiomove. But I think VOP_UBOP deserves a manpage, too. Most VOPs have them. And in addition to being used by aio, VIO_UBOP might be useful for md(4), and it might have many implementors. fusefs could profitably implement it as well as ZFS.

scottl added a subscriber: scottl.Oct 9 2020, 11:12 PM

scottl added inline comments.

sys/kern/vfs_aio.c
1402	Just say "Reading from disk means writing into memory"

scottl accepted this revision.Oct 10 2020, 12:00 AM

This revision is now accepted and ready to land.Oct 10 2020, 12:00 AM

comment new functions in vfs_aio.c
add man page for uio_bio (includes uiobiomove) and VOP_UBOP.

This revision now requires review to proceed.Oct 13 2020, 8:10 PM

Herald added a reviewer: manpages. · View Herald TranscriptOct 13 2020, 8:10 PM

Harbormaster completed remote builds in B34140: Diff 78187.Oct 13 2020, 8:10 PM

@asomers can you let me what more you'd like to see in the documentation.

add sparse page array support to uiobiomove

Harbormaster completed remote builds in B34150: Diff 78200.Oct 13 2020, 11:48 PM

Some man pages nits.

share/man/man9/VOP_UBOP.9
2	Is this required?
70	It would be nice if you could also add a HISTORY section to the man page.
share/man/man9/uio_bio.9
67	Whitespace at the end of the sentence.
120	This .Pp can be deleted since the next paragraph is a new section.
224	The comma is needed since the .Xr reference is the last one.

asomers added inline comments.Oct 17 2020, 2:09 AM

share/man/man9/VOP_UBOP.9
64	If the operation is not completed immediately, how will completion notification be delivered? The man page should mention the asynchronous nature of UIO_UBOP, since that's its whole point.
share/man/man9/uio_bio.9
29	This date needs fixing.
58	I think you should remove the trailing comma
228	It would be nice to say "and first appeared in FreeBSD 13.0."

imp added inline comments.Oct 17 2020, 2:44 AM

sys/sys/_uio.h
47	DELETE needed as well?

asomers added inline comments.Oct 17 2020, 1:42 PM

sys/sys/_uio.h
47	That would be really sweet. But I don't think we need a UIO_BIO_DELETE until somebody writes aio_delete().

imp accepted this revision.Oct 18 2020, 3:47 PM

imp added inline comments.

sys/sys/_uio.h
47	Fair point....

This revision is now accepted and ready to land.Oct 18 2020, 3:47 PM

update uio_bio structure to support scatter gather as well as improve interop with linux

This revision now requires review to proceed.Nov 23 2020, 1:37 AM

Harbormaster completed remote builds in B34965: Diff 79875.Nov 23 2020, 1:38 AM

asomers requested changes to this revision.Dec 17 2020, 3:35 AM

asomers added inline comments.

share/man/man9/uio_bio.9
60	This should be `uiobiomove`, right?
85	uio_bv_offset ?
sys/kern/vfs_aio.c
1388	Why not use `<< PAGE_SHIFT`?
1389	Is io_pages the same thing as `btoc(cb->aio_nbytes + cb->aio_buf) - btoc(cb->aio_buf) + 1`?
1391	pages is a field from the bio backend's union member. But you're adding a new backend. I think you should create a new union member in struct kaiocb, and put vfs_pages or something in there. Especially because I'm eliminating the pages field in D27624. Also, what initializes pages at this point? I thought it was only used by aio_qbio, which hasn't been called yet, right?
1424	This is dead code. Since io_pages is unsigned, you'll always return -1 in the previous block before you get here. Or maybe this check should come before the previous one?
sys/sys/uio.h
65	Where is the matching #endif ? And what defines HAVE_UBOP?
69	What are the UIO_BIO_USER, UIO_BIO_PREEMPTED, and UIO_BIO_SG flags for?

This revision now requires changes to proceed.Dec 17 2020, 3:35 AM

khng added a subscriber: khng.Jan 30 2021, 5:03 PM

khng added inline comments.Feb 8 2021, 8:27 AM

sys/sys/uio.h
87	Would it be preferrable to make the field off_t if we want to support 64bit operation length with DELETE operation on 32bit platform potentially?

khng added inline comments.Feb 8 2021, 9:00 AM

sys/sys/uio.h
87	Would it be preferrable to make the field off_t if we want to support 64bit operation length with DELETE operation on 32bit platform potentially? Unfortunately, what I said above sounds quite impossible, since our current aio interface use ssize_t for length also.