ufs: undo the new parent's link when ufs_rename() cannot rewrite ".."

[sobomax]

Summary

Summary

When ufs_rename() moves a directory to a new parent, it first adds a link to the new parent for the directory's ... With SU+J it also sets up a journal record (jaddref) for that link with softdep_setup_dotdot_link(). Then it rewrites .. with ufs_dirrewrite().

If that rewrite fails, for example on an I/O error reading the directory block, ufs_dirrewrite() puts the old parent's link back, so .. still consistently names the old parent. But two things are left behind:

• the extra link on the new parent
• the journal record, which is neither written nor cancelled

The same happens if the update of the new parent fails before the rewrite is tried. That path also leaves through unlockout with the link and the record in place.

Without the journal, fsck later finds the new parent's link count one too high.

With SU+J the result is a livelock:

• initiate_write_inodeblock_ufs2() rolls the link count back to that of the first unwritten journal record on the inodedep in every write of the inode. handle_written_inodeblock() then redirties the block. The leaked record is never written, so this never ends.
• An unmount cannot suspend writes. A plain umount fails with EAGAIN once vn_fsync_buf() gives up on the block. The deferred forced unmount after I/O errors keeps retrying in vfs_write_suspend(), which leaves every writer stuck in suspfs, and every vfs_busy() of the mount stuck as well (mount -v, df, ...).

The fix: on either failure, cancel the journal record with softdep_revert_link() and take the extra link off the new parent. Both paths go through a small helper, ufs_rename_dotdot_undo(). How the link comes off depends on whether the move replaced a directory:

• The target name was free (tip == NULL). A link was added to the new parent for the new ... It is taken back.
• The move replaced an existing directory (tip != NULL). No link was added, because the new .. was to take over the link of the replaced directory's ... For that, the replaced directory's removal is set up with DIRCHG, which tells handle_workitem_remove() to leave the parent's link count alone. With the rewrite failed, nothing takes that link over, and the new parent kept it. The fix lowers the parent's i_effnlink and clears DIRCHG on the pending dirrem with a new softdep_revert_dirchg(). The link is then dropped when the removal is on disk, as it is for a directory replaced within the same parent. The replaced directory stays locked for the whole rename, so the dirrem cannot have been processed yet. Without soft updates the link count is lowered directly.

Before a179e72489f8, a failed rewrite was reported as "rename: missing .. entry" through ufs_dirbad(), which only prints. The rename then went on the same way. Those messages, often seen at forced unmounts in the gnop stress2 tests, were this leak.

How it was found

gnop9.sh (SU+J, forced unmounts with injected I/O errors) hung. The deferred unmount task spun in vfs_write_suspend() -> ffs_sync() -> vn_fsync_buf() for hours, rewriting the same 4 inode blocks about 475 times a second each. A live minidump (savecore -L) examined with kgdb showed:

• 6 directories with 235 to 1028 links, each with a non-empty id_inoreflst. Their inodedeps were otherwise ALLCOMPLETE.
• 111 D_JADDREF records on those lists, all in state ATTACHED only, with if_diroff 12 (..), no jseg and not on the journal pending list (sd_on_journal 0). if_ino was the new parent and if_parent the moved directory.

Test

New stress2 test rename19.sh, run on SU and on SU+J, each with a free target name and with an existing empty target directory:

1. It creates a/d and b (and b/d for the existing-target pass) on a gnop(8) device, remounts, and caches everything the rename needs except a/d's directory block.
2. It makes all reads fail for the duration of rename("a/d", "b/d"), so the only read that fails is ufs_dirrewrite()'s.
3. It unmounts, with a watchdog, and runs fsck_ffs -fn.
4. The test requires the stale .. to be reported, which proves the rewrite failed, and nothing else to be reported.

Test Plan

Testing was on amd64 (AWS EC2, 4 vCPUs, 16 GB RAM), non-debug kernel.

Without the fix

With a free target name, rename19.sh fails on both file system types:

• SU: fsck reports the stale .. (CURRENTLY POINTS TO I=... (/a), SHOULD POINT TO I=... (/b)) and LINK COUNT DIR I=... (/b) COUNT 3 SHOULD BE 2.
• SU+J: umount fails with "Resource temporarily unavailable" (EAGAIN), and the file system cannot be unmounted at all.

With an existing target directory, the previous revision of this fix (which only cancelled the journal record in that case) let the unmount succeed, but fsck still reported LINK COUNT DIR I=... (/b) COUNT 3 SHOULD BE 2 on both SU and SU+J.

With the fix

• rename19.sh: all four passes pass: SU and SU+J, each with a free and with an existing target. The stale .. is the only problem fsck reports, and the unmount succeeds. DTrace showed ufs_dirrewrite() failing with EIO in each of the four passes, softdep_revert_link() called in each, and softdep_revert_dirchg() called in the two existing-target passes.
• gnop9.sh: passed 4 times in a row with this revision, and 4 times with the previous one, which handled only the free-target case. No forced unmount got stuck, and the fsck runs in the tests reported no link count or .. repairs.
• Other stress2 tests: these passed on the same kernel, with no panics and only the expected kernel messages from the injected errors and forced unmounts:
  • gnop10.sh, gnop7.sh (SU and SU+J) and fsck6.sh
  • fdatasync.sh and fdatasync2.sh (without soft updates, SU and SU+J) and ftruncate2.sh
  • fsync2.sh (SU and SU+J) and fsync3.sh
  • 10-minute marcus.cfg loads on -o sync mounts: SU, SU+J, and SU with snapshots
  • nlink6.sh

The tested kernel also carried the fdatasync/O_SYNC durability and ufs_sync_nlink() changes that are now on main. They do not touch ufs_rename().

Not covered

• The failure of the new parent's update before the rewrite (the UFS_UPDATE() path) is not exercised by the test. It takes the same undo as the rewrite failure.
• A file system without soft updates. The existing-target case lowers the link count directly there, and rename19.sh only runs on SU and SU+J.

[kib]

sys/ufs/ufs/ufs_vnops.c

1251

Why is the call to softdep_revert_link() only done in case of SU+J, but not for SU?

1783–1784

When tip != NULL, where was the tdp' link count increase that we are undoing there?

[sobomax]

@kib both comments were right, and the second one pointed at a real gap.

Line 1257, softdep_revert_link() only for SU+J. No reason. It is now called whenever soft updates are enabled, like its other callers in ufs_rename(), ufs_link() and ufs_mkdir(). With plain SU it creates no journal record to cancel, but it still brings the inodedep's id_nlinkdelta up to date through inodedep_lookup_ip().

Line 1776, the link count when tip != NULL. There was no increase to undo there. In the previous version the helper did not touch the count in that case (addedlink was false). It only cancelled the jaddref that softdep_setup_dotdot_link() sets up under SU+J. But looking for the answer showed that leaving the count alone was wrong too:

• When the move replaces directory tip, the new .. is meant to take over the link that tip's .. holds on tdp. So ufs_dirrewrite(tdp, tip, ...) is called with newparent, and softdep_setup_directory_change() sets DIRCHG on the dirrem. That makes handle_workitem_remove() skip the parent's decrement.
• When the rewrite of .. then fails, nothing takes that link over, and tdp keeps one link too many.

I confirmed it with the test changed to rename onto an existing empty directory. With the previous version the unmount succeeded, but fsck reported LINK COUNT DIR (/b) COUNT 3 SHOULD BE 2 on both SU and SU+J.

The new version handles it:

• ufs_rename_dotdot_undo() now takes tip instead of a flag, and always lowers tdp->i_effnlink.
• If the target name was free, or without soft updates, it lowers i_nlink directly, as before.
• If the move replaced a directory under soft updates, it calls a new softdep_revert_dirchg(tdp, tip), which clears DIRCHG on the pending dirrem of tip. The removal then continues exactly as for a directory replaced within the same parent, and drops the link of tdp once the removal is on disk. tip stays locked for the whole rename, so handle_workitem_remove() cannot have processed the dirrem yet. The dotdot jremref that newdirrem() creates for an rmdir is already there in the DIRCHG case, so the journal side matches as well.

rename19.sh now runs each file system type with a free target and with an existing one. On a kernel with the previous version, the two existing-target passes fail on the link count. With the new version all four pass: the stale .. is the only thing fsck reports, and the unmount succeeds. DTrace shows softdep_revert_link() called in each pass and softdep_revert_dirchg() in the two existing-target ones.

One thing I would like your opinion on: softdep_revert_dirchg() finds the dirrem by walking id_dirremhd of tip for the one with DIRCHG set. Is that acceptable, or would you rather have softdep_setup_directory_change() hand the dirrem back?

[kib]

sys/ufs/ffs/ffs_softdep.c

5163

So suppose that this situation happen. Isn't the metadata of the filesystem is broken, even if we fix the link count somehow?
How could we continue with such filesystem mounted?

I suspect the right action is to ffs_fsfail_cleanup() and let fsck clean the mess.