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

[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@vger.kernel.org
Suggested-by: Tejun Heo <tj@kernel.org>
Link: https://lore.kernel.org/all/0bc5d58533f10b4841807be3025daa11@kernel.org/ [1]
Assisted-by: Claude:claude-opus-5-5 checkpatch
Assisted-by: Claude:claude-fable-5-1
Signed-off-by: Liz Fong-Jones <lizf@honeycomb.io>
---
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@kernel.org/
[2] https://lore.kernel.org/all/20261015-wb-dying-cgwb-flush-v7-0-9278d9ebaf5d@honeycomb.io/

 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

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

[Tejun Heo]

Hello, Liz.

On Thu, Oct 15, 2026 at 11:43:09AM +0000, Liz Fong-Jones wrote:
> 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.

The late inode has to be one without a pending lazytime update. One
already on b_dirty_time keeps its position and dirtied_when.

cleanup_offline_cgwbs_workfn() skips wbs with anything on b_dirty, b_io
or b_more_io. So the scan has to land between wb_writeback() fixing
dirtied_before and taking list_lock for the first queue_io(), which is a
few instructions. The exception is a multi-pass drain that started
before the wb got dirty IO, as the loop doesn't recheck. Maybe say that
the window is narrow so that stable can weigh it?

> 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 pass keeps the inodes it walks in order, so the only reordering is
between the walked inodes and the ones a filled pass didn't reach. "or a
later pass walks past it" doesn't seem to be a separate case.

> 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.

The description is pretty long for a six-line fix. Maybe drop this
paragraph, which Suggested-by and Link already cover, move the test
details below ---, and tighten the rest?

> +	/*
> +	 * The scan reorders b_dirty_time.  Keep it out of a sync's way, which
> +	 * could otherwise miss timestamps moved past its cutoff.
> +	 */

The inodes' dirtied_when doesn't change. What moves is their list
position. Maybe something like:

	/*
	 * The scan moves walked inodes to the newest end of b_dirty_time,
	 * past ones it didn't reach. If one of those was dirtied after a
	 * sync's cutoff, the sync's queue_io() stops there and misses the
	 * moved ones. Exclude syncs as process_inode_switch_wbs() does.
	 */

None of the above is blocking.

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

[PATCH v2] 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 of inodes attached to a dying
cgroup wb, so a crash after the sync loses them. File data and sizes are
not affected.

Since commit f6988c90671e ("writeback: bound cleanup_offline_cgwb()
rescans by rotating scanned inodes"), isw_prepare_wbs_switch() moves the
inodes it walks to the newest end of the list. When a pass fills its isw
before reaching an inode that got its lazytime update after a sync's
cutoff, the walked inodes land past it on b_dirty_time, and the sync's
queue_io(), which stops at the first inode dirtied after the cutoff,
misses them. The late inode has to be one without a pending lazytime
update; one already on b_dirty_time keeps its position.

The window is narrow. cleanup_offline_cgwbs_workfn() skips wbs with
anything on b_dirty, b_io or b_more_io, so the scan has to land between
wb_writeback() fixing dirtied_before and taking list_lock for its first
queue_io(), unless a multi-pass drain started before the wb got dirty
IO.

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

Fixes: f6988c90671e ("writeback: bound cleanup_offline_cgwb() rescans by rotating scanned inodes")
Cc: stable@vger.kernel.org
Suggested-by: Tejun Heo <tj@kernel.org>
Link: https://lore.kernel.org/all/0bc5d58533f10b4841807be3025daa11@kernel.org/
Assisted-by: Claude:claude-opus-5-5 checkpatch
Assisted-by: Claude:claude-fable-5-1
Acked-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Liz Fong-Jones <lizf@honeycomb.io>
---
Changes in v2, all from Tejun's review:
- Correct the trigger and the reordering description, and note how
  narrow the window is
- Reword the code comment
- Move the test details here and trim the description

Test, with a debug-only delay holding a syncfs's work on the wb between
its cutoff and queue_io() to widen the window: 300 files on a cgroup's
wb with lazytime atime updates, one more read after the cutoff, 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, none were lost. arm64 KVM guests,
7.3-rc7.

v1: https://lore.kernel.org/all/20261015114307.1041273-1-lizf@honeycomb.io/

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

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index ea3eb40bf828..372ac9785e03 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -789,6 +789,13 @@ 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 moves walked inodes to the newest end of b_dirty_time,
+	 * past ones it didn't reach. If one of those was dirtied after a
+	 * sync's cutoff, the sync's queue_io() stops there and misses the
+	 * moved ones. Exclude syncs as process_inode_switch_wbs() does.
+	 */
+	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 +810,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