ufs: make fdatasync(2), O_SYNC and O_DSYNC writes durable

[sobomax]

Summary

Summary

On UFS, fdatasync(2) and writes to a descriptor opened with O_DSYNC could return success while the new file size and the block pointers to the new data were still only in memory. On soft updates file systems the same held for O_SYNC writes. A crash before the syncer caught up lost data the application had been told was on stable storage. Appending and then calling fdatasync(), as log writers and database WALs do, hits every one of these cases.

The series fixes four separate problems and adds a test. Git branch: https://github.com/sobomax/freebsd/tree/ufs-fdatasync-iflag

1. ufs: make fdatasync(2) and O_DSYNC writes update the inode

ffs_syncvnode() (the DATA_ONLY case) and ffs_write() (IO_DATASYNC) decide whether the inode must be written by testing IN_SIZEMOD | IN_IBLKDATA against ip->i_flags, the chflags(2) word, instead of ip->i_flag. Those bits alias UF_READONLY and UF_ARCHIVE:

• For ordinary files the test was always false, so the inode was never written.
• Files with urdonly or uarchive set got a synchronous inode write on every call instead.

journal_mount() has the same mix-up and sets IN_MODIFIED in i_flags. That only left UF_OPAQUE on the in-core .sujournal inode. The ffs_update() right after it is synchronous anyway.

The fix is one letter in each of the three places.

Fixes: 7428630b757, 52488b51489, e7347be9e34, 113db2dddb7

2. ufs: make fdatasync(2) write dirty indirect blocks

A data-only ffs_syncvnode() skipped dirty indirect blocks without soft updates. With soft updates, it stopped making passes once only indirect blocks were left dirty.

Indirect blocks only become dirty when the block pointers in them change, and those pointers are what makes a new block reachable. So an append or hole fill beyond the direct blocks had its data and size on disk, but not the pointer to the new block.

The fix treats indirect blocks the same way in data-only and full syncs, as the loop did before 2f514f92cf0. Plain overwrites, the case data-only sync is meant to speed up, don't dirty indirect blocks, so they still write neither indirect blocks nor the inode.

Fixes: 2f514f92cf0

3. ufs: flush soft updates dependencies for O_SYNC and O_DSYNC writes

For an IO_SYNC write, ffs_write() writes the data with bwrite() and then the inode with ffs_update(vp, 1). With soft updates, a new block pointer (and, for an extending write, the size) is rolled back in the buffer being written until the block's dependencies, such as its cylinder group map, are on disk. ffs_update() still clears IN_SIZEMOD and IN_IBLKDATA. So the correct inode was only written later, by the syncer. The same happened to the pointer in an indirect block, which ffs_balloc() writes synchronously but rolled back.

With soft updates, a synchronous write that allocated blocks now finishes with ffs_syncvnode(), as fsync(2) and fdatasync(2) do: DATA_ONLY for O_DSYNC, a full sync for O_SYNC. Whether the write allocated anything is decided by comparing the inode's block count before and after the write. The inode flags can't be used, because allocating a block below an existing indirect block sets neither of them. A write that allocated nothing, such as an overwrite of existing blocks, keeps writing only the inode as before, so overwrite workloads don't pay for a sync of the whole vnode. ffs_syncvnode() only drops the vnode lock for directory dependencies, and a KASSERT checks that it never returns ERELOOKUP here.

The same commit adds a sync on open. ffs_syncvnode() also writes whatever other descriptors left dirty on the vnode, so the first synchronous write after a lot of buffered writes would pay for all of them. A new ffs_open() syncs the vnode when a regular file on a soft updates file system is opened for writing with O_SYNC (full sync) or O_DSYNC (data-only sync), so this cost is mostly paid once, by the open.

• It calls ffs_syncvnode() rather than VOP_FSYNC(), because softdep_fsync() may drop the vnode lock in the middle of the open. A writable open holds the vnode lock exclusively.
• It only moves work the writes would otherwise do. So it skips the sync rather than wait when writes are suspended (vn_start_write() with V_NOWAIT), and does not fail the open on sync errors.
• It does nothing without soft updates. There the writes sync only their own blocks and the inode, so there is no cost to move, and a sync on open would only add one: opening a large file that another process is writing would block until all of it was on disk.

4. ufs: do not let a rolled back inode write end a data-only sync

With soft updates, ffs_syncvnode() writes the inode between passes while buffers are still dirty. That write can go out with new block pointers rolled back, yet it clears IN_SIZEMOD and IN_IBLKDATA.

This is exposed by change 2 on an append that allocates a file's first indirect block. The inode goes out while the new indirect block is still unwritten, so it is written with the new pointer rolled back. The indirect block is written in a later pass. The final check then sees no flags set and skips the inode, which is never written with the real pointer.

The fix: note on entry whether a data-only sync has to write the inode, and base the final write on that alone. Nothing else can set the flags meanwhile, because the vnode is locked.

5. tests: add UFS synchronous write durability tests

tests/sys/fs/ufs/sync_test (ATF, 72 cases) checks that once fsync() or fdatasync() returns, or a write with O_SYNC or O_DSYNC returns, the new data, the file size and every block pointer leading to the data are on disk.

The cases cover six scenarios, each with the four sync methods, on UFS without soft updates, with soft updates and with SU+J:

• appending a block
• filling a hole
• appending the first block that needs an indirect block
• appending below an existing indirect block
• appending a direct block after another write left a new direct block unsynced
• the same, with the synced append allocating the first indirect block

The last two model a descriptor without O_SYNC writing next to one with it. With soft updates, the unsynced direct block cuts the size and the later pointers of any inode written meanwhile. For O_SYNC and O_DSYNC on soft updates and SU+J, these cases also check right after the open, before any write, that the sync on open already made the unsynced block, its pointer and the size durable.

Each case runs on an md(4)-backed file system. Right after the call returns, it reads the inode, the indirect block and the data block straight from the device with libufs. There is no cache in front of the disk device, so this shows what would survive a crash at that moment without crashing anything.

Files: sys/ufs/ffs/ffs_vnops.c, sys/ufs/ffs/ffs_softdep.c (2 lines), tests/sys/fs/ufs/ (new), tests/sys/fs/Makefile, etc/mtree/BSD.tests.dist.

Test Plan

All kernel testing was on releng/14.3, amd64 (AWS EC2, 4 vCPUs, 16 GB RAM), with the series cherry-picked. The first revisions were tested with INVARIANTS, INVARIANT_SUPPORT, WITNESS and DEBUG_VFS_LOCKS; the last two, including this one, on non-debug kernels. The changes apply to main without conflicts. On main, the UFS module was only compiled, not booted.

This revision goes back to ffs_syncvnode() for synchronous writes (change 3 as in the revision before last) and adds change 4. It was tested on non-debug 14.3 kernels. The UFS module also builds with -Werror on main.

• sync_test: 72/72 with this revision. The new after-open check fails on a kernel without change 4, on all 12 O_SYNC/O_DSYNC unsynced cases and all three file system types: the size and the unsynced block are not on disk after the open, and the rest of each case passes. With change 4 they pass. In those 12 cases, DTrace showed ffs_open() calling ffs_syncvnode() once per case, plus 8 calls from ffs_write() for the allocating writes on soft updates and SU+J.
• stress2: the whole list below passed with this revision. In 20-second samples of the -o sync loads, ffs_write() called ffs_syncvnode() 19,776 times on soft updates, 136 on SU+J and 102 on soft updates with snapshots.

sync_test

With change 3 as in this revision, sync_test passed all 72 cases, before the after-open check was added. With the targeted flush of the last revision, it also passed all 72. Adding the fixes one at a time shows each one's effect on the 48 original cases:

Kernel	Passed	Still failing
change 1 only	26/48	O_SYNC/O_DSYNC on soft updates and SU+J (16); fdatasync() past the direct blocks on all three file system types (6)
changes 1 + 3	38/48	fdatasync() and O_DSYNC past the direct blocks (10)
changes 1 + 3 + 2	46/48	fdatasync() on soft updates and SU+J when the append allocates the first indirect block (2)
full series	48/48	none

• Before the series: the original version of the test failed every fdatasync() and O_DSYNC case. The on-disk inode still had the old size and a zero pointer for the new block.
• Test controls: in every case the test first checks what it reads after a plain fsync(), so it can tell a written inode from an unwritten one. A build of the test with fdatasync() replaced by fsync() passed on the unfixed kernel.
• Repeats: the two cases change 5 fixes failed 5 out of 5 times before it and passed 5 out of 5 times after it.
• DTrace: it confirmed the mechanism behind change 3, the rolled-back O_DSYNC inode write (buf_start() in ffs_geom_strategy() replaces the new di_size in the outgoing buffer), and behind change 5, the intermediate inode write that clears the flags before the indirect block is written.

stress2

These all passed with this revision, and earlier with change 3 alone. The ones that end with a file system check reported it clean, and there were no panics, KASSERT failures or new WITNESS warnings attributable to the series:

• fdatasync.sh and fdatasync2.sh, each on UFS without soft updates, with soft updates and with SU+J, each followed by a clean fsck_ffs -fn
• ftruncate2.sh
• fsync2.sh (soft updates and SU+J) and fsync3.sh
• gnop7.sh (soft updates and SU+J), gnop9.sh, gnop10.sh and fsck6.sh: forced unmounts and injected I/O errors, each followed by fsck
• 10-minute marcus.cfg loads on file systems mounted -o sync, where every write has IO_SYNC and takes the new path from change 3: soft updates, SU+J, and soft updates with a snapshot taken every 20 seconds. DTrace counted about 14,000 ffs_syncvnode() calls per second from ffs_write() on soft updates, which confirms that path was exercised.

The last revision, with the targeted flush, also passed the whole list.

During testing of this revision, one gnop9.sh run hung its forced unmount. That was a separate, pre-existing bug: ufs_rename() leaks the journal record for .. when the rewrite of .. fails. It is fixed in a separate review. With that fix, gnop9.sh passed 4 times in a row.

WITNESS reported one snaplk -> bufwait lock order reversal during the snapshot run. Both stacks (ffs_snapshot() -> softdep_sync_metadata(), and a full fsync -> ffs_copyonwrite()) go through code this series does not change, so it appears to predate the series. The same reversal came back in the rerun, again in the snapshot run. It is a false positive: the order is established when a snapshot file is unlinked (the .snap directory fsync writes a directory buffer, and ffs_copyonwrite() takes snaplk), and reversed by the final ffs_syncvnode() in ffs_snapshot() waiting on a cylinder group buffer. Cylinder group blocks are in the snapshot's block list, so their writes never wait for snaplk, and there is no cycle.

Performance

Not benchmarked against a kernel without the series. Changes 2 to 4 add I/O by design:

• O_SYNC and O_DSYNC on soft updates: each write that allocates blocks now syncs the vnode. Writes that allocate nothing are unchanged.
• Opening for writing with O_SYNC or O_DSYNC: syncs the vnode, on all UFS.
• fdatasync() of a large file after an append: it now writes the dirty indirect blocks.

With change 3 on SU+J under the -o sync load, most synchronous writes took 8-64 us in ffs_syncvnode(), and about 8% took 4 ms or more, up to about 1 s, waiting for journal and cylinder group writes.

Not covered

• UFS1. The test's on-disk reader handles it, but no case creates a UFS1 file system.
• Double and triple indirect blocks.
• ffs_extwrite(), which has the same IO_SYNC -> ffs_update() pattern for extended attributes. It is not changed here.

Side effect on the PR 297976 reproducer

The mkdir_blkreuse.sh reproducer for PR 297976 relied on fdatasync() not writing the inode, which is the bug change 1 fixes. It has been reworked in that review to write blocks with O_DIRECT instead.

[kib]

sys/ufs/ffs/ffs_softdep.c

713–714

I think this change is not incorrect, but it is also not clear why is it needed. ffs_update() is called immediately after setting mtime and the IN_MODIDIFED flag, so there is no pending updates that need to be handled by the syncer.

Still it is probably not too harmful, only causing useless locking and more unneeded CPU use in syncer.

sys/ufs/ffs/ffs_vnops.c

462

What is the reason to recheck IN_SIZEMOD|IN_IBLKDATA again there? Isn't inoupdt value enough? We have the vnode locked.

1057

This is huge perf hit for some workloads.

[sobomax]

@kib thanks, updated.

ffs_softdep.c:2962 (journal_mount)
Agreed. Setting the flag is not needed at all: ffs_update(vp, 1) only looks at IN_MODIFIED to skip a write when waitfor is 0, so the inode is written either way and the flag is cleared right away. The line is removed instead of fixed. That also means it never had any effect beyond setting UF_OPAQUE in the chflags word.

ffs_vnops.c:449 (IN_SIZEMOD|IN_IBLKDATA recheck)
Right, the recheck is redundant. The only code that sets those flags is in allocation and truncation (ffs_balloc(), ffs_truncate() and the softdep freeblocks paths it calls), and nothing reached from ffs_syncvnode() runs them while we hold the vnode lock. process_truncates() writes the inode itself with ffs_update(vp, 1). The final check is now just inoupdt.

ffs_vnops.c:1044 (perf)
Agreed, that sent every synchronous write on soft updates through ffs_syncvnode(), including plain overwrites, which can never hit the rollback: it only affects newly allocated blocks.
It is now limited to writes that allocated blocks. ffs_write() compares DIP(ip, i_blocks) before and after the write. That catches every allocation, including a block below an existing indirect block, which neither IN_SIZEMOD nor IN_IBLKDATA flags. A write that allocated nothing goes back to the old ffs_update() path, so overwrite-heavy workloads (database pages written with O_SYNC/O_DSYNC) are unaffected. Appending workloads pay for the dependency flush, which is what makes their data durable.

One corner is still not covered, as before this change: an O_DSYNC write into a block that an earlier asynchronous write allocated and whose dependencies are still incomplete. That write allocates nothing, so it takes the cheap path, but the block's pointer can still be rolled back. Covering it would need a cheap soft updates check for an inode with incomplete allocation dependencies. I can add that as a follow-up if you think it is worth it.

[kib]

sys/ufs/ffs/ffs_softdep.c

713–714

I do not understand the removal of IN_MODIFIED. I said that the change to set the flag using UFS_INODE_SET_FLAG() added vlazy() call which is redundant. I do not mean that setting of IN_MODIFIED setting is not needed.

sys/ufs/ffs/ffs_vnops.c

1057

Doing fsync() even under the additional condition is not . Imagine large file where one file descriptor was opened without O_SYNC and doing a lot of writes, and another file descriptor opened with O_SYNC and appending a single byte. The io hit is not proportional.

If going this route, I might propose to do fsync() on open with O_SYNC/O_DSYNC too.

[sobomax]

@kib agreed, that was not proportional. The new revision replaces the ffs_syncvnode() call with a targeted flush, ffs_sync_alloc(), that only makes the pointers to the written range durable:

• softdep_sync_metadata() for the dependencies of the blocks the inode points to.
• Deepest level first, softdep_sync_buf() and a write of each indirect block on the path to the written range. The walk is bounded by the size of the write.
• The dirty direct blocks that still carry allocation dependencies, at most UFS_NDADDR of them.

Then the inode is written as before (for O_DSYNC, only if IN_SIZEMOD or IN_IBLKDATA is set).

The direct-block pass is the one place where another descriptor's writes matter. While any direct block's allocation is incomplete, initiate_write_inodeblock_ufs2() writes the inode with di_size cut back to that block and every later pointer cleared, including di_ib[]. So in your example, a block the non-O_SYNC descriptor appended just before would hide the byte the O_SYNC descriptor appended after it. Pointers to other writers' blocks in the same indirect blocks just stay rolled back, and nothing else of theirs is written.

Fsync on open with O_SYNC/O_DSYNC would not cover your example. The other descriptor keeps allocating blocks after the open, so each O_SYNC append can again find a direct block with an incomplete allocation in front of it. It would also make the open itself pay for everything already dirty on the vnode.

I added two scenarios to sync_test for this (24 more cases, 72 total). In each, another write leaves a new direct block unsynced, and then the synced append follows, in one case into the direct blocks and in the other allocating the first indirect block.

[kib]

I generally do not like this version, it adds a lot of complexity for something that is hard to test.

After lots of hesitation, I suggest to return back to the previous version that does syncvnode on IO_SYNC write, but also, as suggested, does fsync on open with any of O_SYNC/DSYNC flag. It is much simpler and in most situations would put the cost on the sync open call, which is perhaps not too unexpected for consumers.

sys/ufs/ffs/ffs_vnops.c

861

Why all direct blocks are written unconditionally?

[sobomax]

@kib OK, done. ffs_sync_alloc() is gone, and an allocating O_SYNC/O_DSYNC write finishes with ffs_syncvnode() again, as in the revision before last (DATA_ONLY for O_DSYNC, still only when the write changed i_blocks).

The sync on open is a new commit, "ufs: sync the vnode when it is opened with O_SYNC or O_DSYNC", right after that one. It adds an ffs_open() that calls ufs_open(). Then, for a regular file opened for writing with O_SYNC or O_DSYNC, it does what the writes would do: ffs_syncvnode(), full or DATA_ONLY. A few choices in it you may want to check:

• It calls ffs_syncvnode() and not VOP_FSYNC(). ffs_fsync() goes on to softdep_fsync(), which can drop the vnode lock, and I did not want that in the middle of VOP_OPEN(). ffs_syncvnode() only drops it for directories. Opens without FWRITE are skipped. vn_open_cred() only takes LOCKSHARED for those, so the vnode is exclusively locked here, and there is an ASSERT_VOP_ELOCKED().
• It takes vn_start_write() with V_NOWAIT and skips the sync if writes are suspended. The open has no write reference otherwise, and the writes will sync anyway.
• Sync errors are ignored rather than failing the open, for the same reason.
• It applies with and without soft updates, as you suggested: any O_SYNC/O_DSYNC open.

sync_test checks it directly. In the cases where another write left a block unsynced, the O_SYNC/O_DSYNC variants now verify right after the open, before writing, that the block, its pointer and the size are on disk. On a kernel without the new commit, those 12 cases fail at that check on all three file system types. With it they pass, and DTrace showed ffs_open() calling ffs_syncvnode() once in each of them.

The revision passes all 72 sync_test cases and the full stress2 set I ran before: fdatasync, fsync and ftruncate tests, the gnop forced-unmount tests and the -o sync marcus.cfg loads. The test plan in the summary has the details. One gnop9.sh run hung its forced unmount. That turned out to be a pre-existing leak of the .. journal record in ufs_rename() when the .. rewrite fails, which I put up as a separate review. With that fix, gnop9.sh passed 4 times in a row.

On line 848, why all direct blocks were written: it only wrote dirty direct blocks that still had allocation dependencies (B_DELWRI with a non-empty b_dep), at most UFS_NDADDR of them. While the allocation of any direct block is incomplete, initiate_write_inodeblock_ufs2() writes the inode with di_size cut back to that block and every later pointer cleared, di_ib[] included. So another descriptor's unwritten direct block could hide the data the sync write had just made durable. That code is gone now along with ffs_sync_alloc().

[kib]

Please ask Peter to do the full stress2 run

[pho]

I completet a full stress2 test without observing any issues.

[sobomax]

@kib thanks for the approval. While organizing the patch set into logically independent commits, I noticed a potential minor issue with the version you approved, though.

There are two changes in this update, neither to the write path:

The two commits are squashed. "ufs: sync the vnode when it is opened with O_SYNC or O_DSYNC" is folded into "ufs: flush soft updates dependencies for O_SYNC and O_DSYNC writes", with one description for both. The sync on open only exists to offset the cost of the write-side change, so it does not stand on its own.

The sync on open is now limited to soft updates (!DOINGSOFTDEP(vp) returns early in ffs_open()). Writing the merged description made the mismatch obvious: I had described it as only moving work the writes would do anyway, and that is true only with soft updates. Without them, an IO_SYNC write never calls ffs_syncvnode(). It writes its own blocks and the inode, and nothing is rolled back. So there the open had no cost to take over. It only added one: opening a large file that another process is writing with O_SYNC blocked until all of it was on disk.

I had read your "fsync on open with any of O_SYNC/DSYNC flag" as "on any UFS". If you did mean it for file systems without soft updates too, tell me and I will put it back.

sync_test follows: the after-open check now applies to the soft updates and SU+J cases only (8 of the 12). On a kernel without ffs_open() those 8 fail and the 4 without soft updates pass. On a kernel with this update all 72 cases pass, and DTrace shows ffs_open() calling ffs_syncvnode() for each of the 8 opens on soft updates and SU+J, and for none of the 4 without soft updates.

On re-running stress2 for this update. I did not repeat the stress2 set, and I do not think repeating it would tell us anything:

• No stress2 test opens a file with O_SYNC, O_DSYNC or O_FSYNC, in testcases/, lib/ or misc/. So under every one of those loads ffs_open() already returned before its sync, on every file system type. The new DOINGSOFTDEP() test only adds one more way to return early from a branch those loads never took. What runs is the same as before.
• The -o sync marcus.cfg loads do not change that. A sync mount sets IO_SYNC on the writes, which exercises the ffs_write() half heavily, but the open still has no FFSYNC or FDSYNC in its mode.
• The ffs_write() half, which those runs did exercise, is not touched by this update.

The same fact cuts the other way, and I should say so: the earlier stress2 runs never exercised the sync in ffs_open() either, in either version. Its only coverage is sync_test. That runs one open at a time on a quiet file system, plus the DTrace check that it fires on soft updates and SU+J and not without them. What has not been tested is the sync on open under load: opens with O_SYNC racing buffered writers on the same file, a snapshot being taken, or a forced unmount. ffs_open() takes its write reference with V_NOWAIT and syncs with ffs_syncvnode(), which does not drop the vnode lock for regular files, so I do not expect a problem there. But that is reasoning, not a test result.

The code change since the previous version (the squash itself changes no code):

diff --git a/sys/ufs/ffs/ffs_vnops.c b/sys/ufs/ffs/ffs_vnops.c
index 368b0a1fcbb1..8cdad58a3096 100644
--- a/sys/ufs/ffs/ffs_vnops.c
+++ b/sys/ufs/ffs/ffs_vnops.c
@@ -475,7 +475,9 @@ ffs_fdatasync(struct vop_fdatasync_args *ap)
  *
  * This only moves work that the writes would otherwise do, so skip it
  * rather than wait when writes are suspended, and do not fail the open on
- * its errors.  A writable open locks the vnode exclusively.
+ * its errors.  Without soft updates the writes sync nothing but their own
+ * blocks and the inode, so there is nothing to move and the open does not
+ * sync.  A writable open locks the vnode exclusively.
  */
 static int
 ffs_open(struct vop_open_args *ap)
@@ -486,7 +488,7 @@ ffs_open(struct vop_open_args *ap)

        error = VOP_OPEN_APV(&ufs_vnodeops, ap);
        vp = ap->a_vp;
-       if (error != 0 || vp->v_type != VREG ||
+       if (error != 0 || vp->v_type != VREG || !DOINGSOFTDEP(vp) ||
            (ap->a_mode & FWRITE) == 0 ||
            (ap->a_mode & (FFSYNC | FDSYNC)) == 0)
                return (error);
diff --git a/tests/sys/fs/ufs/sync_test.c b/tests/sys/fs/ufs/sync_test.c
index c5c3dfbf524e..9adc853affd7 100644
--- a/tests/sys/fs/ufs/sync_test.c
+++ b/tests/sys/fs/ufs/sync_test.c
@@ -79,7 +79,8 @@ enum sync_method {
  * written with one of the synchronous methods.  If "unsynced" is not -1,
  * that block is written first without syncing, as another descriptor of a
  * process that does not care about durability would.  With O_SYNC and
- * O_DSYNC, the open that follows must already have made it durable.
+ * O_DSYNC on soft updates, the open that follows must already have made it
+ * durable.
  */
 struct scenario {
        int     prefill;
@@ -297,8 +298,11 @@ test_sync(const char *newfs_opts, const struct scenario *sc,
                ATF_REQUIRE_EQ(0, close(fd));
                fd = open(TFILE, O_RDWR | oflags);
                ATF_REQUIRE(fd >= 0);
-               if (sc->unsynced != -1) {
-                       /* The open syncs what was written before it. */
+               if (sc->unsynced != -1 && newfs_opts != NULL) {
+                       /*
+                        * With soft updates, the open syncs what was
+                        * written before it.
+                        */
                        ondisk_read(sb.st_ino, sc->unsynced, &od);
                        ATF_CHECK_MSG(od.size >= (sc->unsynced + 1) * bsize,
                            "on-disk size is %jd after open",