Page MenuHomeFreeBSD

aio_fsync(O_DSYNC)
ClosedPublic

Authored by tmunro on May 30 2020, 2:56 PM.

Details

Reviewers
kib
mjg
asomers
jhb
Group Reviewers
manpages
Summary

I would like to be able to initiate the asynchronous equivalent of fdatasync(2) using aio_fsync(O_DSYNC, ...), as allowed by POSIX. Here is a patch to do that.

Builds on D25090 and D25160 which will go in first.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Please upload with context.

I believe that my main objection against defining O_DSYNC as O_SYNC was that it de-facto provides some intentionally wrong ABI. Defining O_DSYNC as you did, and converting it to IO_DSYNC or better IO_DATASYNC is fine even if no filesystem handle the flag differently from IO_SYNC ATM.

You can look ffs_syncvnode() to see what our fdatasync() does. It is not quite correct for two reasons:

  • we do not flush metadata mostly
  • we do not wait for async writes in-flight to finish.

For metadata we are only wrong on direct/ext block allocations pending writes, and I suspect this is easy to fix.

bcr added a subscriber: bcr.

OK from manpages (don't forget to bump .Dd when committing).
Thanks for implementing this feature.

asomers requested changes to this revision.May 30 2020, 7:01 PM

This change is a good one. And BTW, there is already one file system that can take immediate advantage of the new flag: fusefs. Once this change merges, ping me and I'll add a testcase for fusefs.

One other thing: in aio_test.c could you please create a variant of aio_fsync_test that uses the new flag?

sys/kern/vfs_aio.c
1729

A few lines below here is a check job2->uaiocb.aio_lio_opcode != LIO_SYNC. Why didn't you modify that to check for LIO_DSYNC too?

Pro tip: if you generate the diff with svn diff -D9999 Phabricator will intelligently show more context. Or, install arcanist-php72 and create the review with arc diff --create.

This revision now requires changes to proceed.May 30 2020, 7:01 PM

Handled feedback from asomers@: added a test, added a missing check in aio_queue_file(), used -U999999 for more context. I also created a separate review ticket D25090 to add O_DSYNC for open(2) and fcntl(2). The present patch is rebased on top of that one.

tmunro added inline comments.
sys/kern/vfs_aio.c
1729

Yeah, without that check it would set the KAIOCB_CHECKSYNC flag on other LIO_DSYNC requests queued earlier, which wasn't intentional. Fixed.

Thanks for the feedback!

In D25071#552096, @kib wrote:

I believe that my main objection against defining O_DSYNC as O_SYNC was that it de-facto provides some intentionally wrong ABI. Defining O_DSYNC as you did, and converting it to IO_DSYNC or better IO_DATASYNC is fine even if no filesystem handle the flag differently from IO_SYNC ATM.

Got it. I created D25090 to do that.

You can look ffs_syncvnode() to see what our fdatasync() does. It is not quite correct for two reasons:

  • we do not flush metadata mostly
  • we do not wait for async writes in-flight to finish.

Right, so synchronous fsync(2) and fdatasync(2) should read jobseqno on entry, and wait until everything in the process's kaio_jobqueue with matching fd and lower job number to complete. I found your note in kern_fsync():

#if 0

if (!fullsync)
        /* XXXKIB: compete outstanding aio writes */;

#endif

I this it should do that unconditionally.

For metadata we are only wrong on direct/ext block allocations pending writes, and I suspect this is easy to fix.

I saw D25072. Thanks!

asomers requested changes to this revision.Jun 1 2020, 1:20 PM
asomers added inline comments.
tests/sys/aio/aio_test.c
1145

This test is doing way too much stuff now. You should split it into two test cases. And it should be possible to share most of the code, rather than copy it.

This revision now requires changes to proceed.Jun 1 2020, 1:20 PM
tests/sys/aio/aio_test.c
1145

What do you think about this refactoring?

kib added inline comments.
sys/kern/vfs_aio.c
847

This line should have 4-spaces indent.

asomers requested changes to this revision.Jun 2 2020, 2:02 PM
asomers added inline comments.
tests/sys/aio/aio_test.c
1145

It certainly reduces line count. But my bigger point was that there's too much stuff happening in that one ATF test case. I think you should split the test case into two.

This revision now requires changes to proceed.Jun 2 2020, 2:02 PM
jhb added a subscriber: jhb.
jhb added inline comments.
sys/kern/vfs_aio.c
853

You could just pass the opcode through. It might be slightly easier to read to do the 'opcode == LIO_SYNC' comparison in aio_fsync_vnode() directly rather than here in the caller, but that's really minor.

2478

I would perhaps consolidate the checks:

 switch (op) {
 case O_SYNC:
     listop = LIO_SYNC;
     break;
 case O_DYSNC:
     listop = LIO_DSYNC;
     break;
 default:
     return (EINVAL);
}
tests/sys/aio/aio_test.c
1145

I agree that the refactoring is good, and that it would be better to use two test cases: 1 for O_SYNC and 1 for O_DSYNC.

tmunro edited the summary of this revision. (Show Details)

Rebased.

Updated to address review feedback and fix oversights in man page.

LGTM!

lib/libc/sys/aio_fsync.2
59

I would say "for the behavior of" or "as if by a call to".

sys/sys/aio.h
51

Note that this line will conflict with D27942 .

This revision is now accepted and ready to land.Jan 6 2021, 3:20 PM