Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - 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.
OK from manpages (don't forget to bump .Dd when committing).
Thanks for implementing this feature.
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. | |
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.
| 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. | |
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!
| tests/sys/aio/aio_test.c | ||
|---|---|---|
| 1179–1180 | 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. | |
| tests/sys/aio/aio_test.c | ||
|---|---|---|
| 1179–1180 | What do you think about this refactoring? | |
| sys/kern/vfs_aio.c | ||
|---|---|---|
| 847 | This line should have 4-spaces indent. | |
| tests/sys/aio/aio_test.c | ||
|---|---|---|
| 1179–1180 | 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. | |
| 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 | ||
| 1179–1180 | 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. | |