[PATCH] writeback: keep cleanup_offline_cgwb()'s scan out of syncs

[Liz Fong-Jones]

On a filesystem mounted with lazytime, sync() and syncfs() can return
without persisting timestamp updates (atime, mtime, ctime) of inodes
attached to a cgroup writeback that is being cleaned up after its
memory cgroup was removed. A crash after the sync loses them, contrary
to the lazytime guarantee that sync persists them. File data and sizes
are not affected.

Trigger: a wb has lazytime-dirty inodes on b_dirty_time when a sync
starts, an inode on it gets a timestamp update after the sync's cutoff,
and then its memcg (or, if the wb is already dying, any memcg) is
removed, which runs cleanup_offline_cgwb() on it before the sync work
queues inodes.

Since commit f6988c90671e ("writeback: bound cleanup_offline_cgwb()
rescans by rotating scanned inodes"), isw_prepare_wbs_switch() moves
every inode it walks, prepared or not, to the newest end of the list.
Once an isw fills before the scan reaches a newer inode, or a later pass
walks past it, older inodes land in front of it on b_dirty_time. A
for_sync queue_io() expires b_dirty_time with the sync's fixed cutoff
and stops at the first newer inode, so it never reaches them. The
switches themselves already wait for the sync on wb_switch_rwsem; the
scan does not.

Take wb_switch_rwsem for read across the scan, as
process_inode_switch_wbs() does for the switches.

Tejun spotted this reordering racing with a sync on b_dirty in a patch
under review and suggested holding wb_switch_rwsem for read across that
scan [1]. cleanup_offline_cgwb()'s scan of b_dirty_time needs the same.

Test, with a debug-only delay holding a syncfs's work on the wb between
its cutoff and queue_io(): 300 files on a cgroup's wb with lazytime
atime updates, a syncfs held after its cutoff, one more read, then the
cgroup removed so the cleanup ran. Without this patch, syncfs() queued
48 of the 300 and after a crash 252 atime updates were lost, on ext4
(-I 4096, so neighbours' timestamps are not written along) in 3 of 3
runs and on XFS in 1 of 1. With it, the scan waited for the sync, which
queued all 300, and none were lost.

Fixes: f6988c90671e ("writeback: bound cleanup_offline_cgwb() rescans by rotating scanned inodes")
Cc: stable@xxxxxxxxxxxxxxx
Suggested-by: Tejun Heo <tj@xxxxxxxxxx>
Link: https://lore.kernel.org/all/0bc5d58533f10b4841807be3025daa11@xxxxxxxxxx/ [1]
Assisted-by: Claude:claude-opus-5-5 checkpatch
Assisted-by: Claude:claude-fable-5-1
Signed-off-by: Liz Fong-Jones <lizf@xxxxxxxxxxxx>
---
This came up while answering Tejun's review of my series "writeback: let
foreign flushes reach dying cgwbs" [1], whose patch 3 had the same
problem on b_dirty in its own scan; v7 of that series [2] takes the same
read lock there. This patch is independent of the series and applies to
7.3-rc7 on its own; both together apply cleanly.

Impact is limited to lazytime timestamps on inodes attached to a cgroup
wb while it is cleaned up after its memcg is removed. On ext4 with the
default inode size, a write to any inode in the same inode-table block
also writes the neighbours' timestamps, so fewer are lost there; the
test used -I 4096 to rule that out. XFS has no such rescue.

The debug knob used for the test, applied to wb_writeback() and not part
of this patch: when /sys/module/kernel/parameters/wb_debug_sync_delay_ms
is set, a WB_SYNC_ALL work on a non-root wb sleeps that long once, after
dirtied_before is fixed and before queue_io(); it also skips sync's
best-effort WB_SYNC_NONE pass on cgroup wbs, which had nothing to write
here. Steps: mount -o lazytime,strictatime; a cgroup writes 300 small
files and one more, syncfs twice; read the 300 in order (each goes to
b_dirty_time); set the knob to 10s and start syncfs; 1s later read the
extra file; 1s later remove the cgroup; after syncfs returns, crash with
sysrq-b and compare atimes on disk in a second boot. The scripts are
available if useful.

Tested on arm64 KVM guests (virtme-ng) with ext4 and XFS on virtio, on
7.3-rc7. Not tested: x86, production.

Developed with Claude Opus 5.5 and Claude Fable 5.1. Fable spotted that
cleanup_offline_cgwb()'s scan of b_dirty_time was exposed the same way
while checking Tejun's analysis, and reviewed the fix and the claims
here; Opus wrote the fix, the reproducer and this text, and ran the
tests. I directed the investigation and reviewed the analysis, code and
results.

[1] https://lore.kernel.org/all/0bc5d58533f10b4841807be3025daa11@xxxxxxxxxx/
[2] https://lore.kernel.org/all/20261015-wb-dying-cgwb-flush-v7-0-9278d9ebaf5d@xxxxxxxxxxxx/

 fs/fs-writeback.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index ea3eb40bf828..99570aacdd18 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -789,6 +789,11 @@ bool cleanup_offline_cgwb(struct bdi_writeback *wb)
 		new_wb = &wb->bdi->wb; /* wb_get() is noop for bdi's wb */
 
 	nr = 0;
+	/*
+	 * The scan reorders b_dirty_time.  Keep it out of a sync's way, which
+	 * could otherwise miss timestamps moved past its cutoff.
+	 */
+	down_read(&wb->bdi->wb_switch_rwsem);
 	spin_lock(&wb->list_lock);
 	/*
 	 * In addition to the inodes that have completed writeback, also switch
@@ -803,6 +808,7 @@ bool cleanup_offline_cgwb(struct bdi_writeback *wb)
 		restart = isw_prepare_wbs_switch(new_wb, isw, &wb->b_dirty_time,
 						 &nr);
 	spin_unlock(&wb->list_lock);
+	up_read(&wb->bdi->wb_switch_rwsem);
 
 	/* no attached inodes? bail out */
 	if (nr == 0) {

base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.53.0