[PATCH v6 0/3] writeback: let foreign flushes reach dying cgwbs

[PATCH v6 0/3] writeback: let foreign flushes reach dying cgwbs

[Liz Fong-Jones]

At Honeycomb, a container that reads from Kafka and writes columnar
files to a host volume stalls for 30-60s after each deploy replaces it
(v6.18). We're increasingly confident this is the cause: the stall
looks the same as in the reproducer (little CPU use, lag growing
linearly and then recovering), and the mitigation it predicts, moving
our final syncfs after the last write, worked in production (below).
We haven't caught it with probes in production yet.

Patch 1 is a prep fix: switching a dirty inode away left the old wb
counted in bdi->tot_write_bandwidth. Patch 2 keeps killed wbs in
bdi->cgwb_tree until they are released, so foreign flushes can still
reach them. Patch 3 handles the other way a wb leaves the tree: when a
memcg's blkcg association changes and a new wb takes over the slot, it
switches the old wb's inodes, dirty or not, to the new one.

Applies to mainline (5d144c294ae2) and vfs-7.4.misc. Tested on
vfs-7.4.misc and on mainline (d24e8ac715de, 7.3-rc6) with 168a8c13159c,
where patch 2's test gave 1.0s, 2.2s, 1.1s, 1.0s and 1.0s worst lag in
five runs. Patch 3's loop follows 407a5d205179, and f6988c90671e bounds
its rescans.

Syncing before the removal isn't enough: cleanup_offline_cgwbs_workfn()
skips a dying wb with any dirty inode and only retries on the next memcg
offline. On ext4, syncfs can leave inodes I_DIRTY_SYNC: at 5000 files
one syncfs still stalled in 7 of 9 runs (19-26s worst lag), a second
syncfs avoided it in 5 of 5, and with this series one syncfs lagged at
most 1.5s in 2 of 2. On XFS one syncfs left the wb clean in 5 of 5 runs,
but a single 4KiB append from the old cgroup after it stranded the wb
again (15-28s worst lag, 5 of 5; 1.2-1.6s in 2 of 2 with this series).
In production, our writer synced after closing its data files but before
writing its final Kafka offset to a small shutdown file, and moving the
syncfs after that write mitigated the stall for us on XFS. In the
reproducer, a 14-byte file written after the syncfs stranded the wb the
same way, whether created directly or via a temp file and rename (16-29s
worst lag, 7 of 7; with this series, 1.0-1.4s in 2 of 2 created
directly); an empty file did not.

The harness that produced the numbers (paced writer, lag per second,
MODE=alive control) is at
https://gist.github.com/lizthegrey/2209d831930588f63076bdc0ac7b78e2, and
a minimal recipe is below. Settings for all numbers: MEM_MAX=max
POD_MAX=4G RATE_MBPS=250, plus NFILES=5000 SYNC_OLD=1 (or 2) and
DIRTY_AFTER=1 or SHUTDOWN_FILE=offset|offset_rename|empty for the syncfs
runs and NFILES=500000 OLD_SECS=120 SYNC_OLD=1 POD_HOP=10 for the
500k-file runs.

Also reproduced unpatched on bare-metal arm64 with Ubuntu's 7.0 kernel
(ext4 on a loop device): 12.9s worst lag with the old cgroup removed,
2.5s with it kept.

Tested: arm64 KVM guests (virtme-ng), ext4 and XFS on virtio, on
vfs-7.4.misc and on mainline with 168a8c13159c. No KASAN, lockdep,
PROVE_RCU, DEBUG_LIST, DEBUG_OBJECTS_WORK, DEBUG_ATOMIC_SLEEP or
DEBUG_PAGEALLOC reports, including a stress run toggling io on the
parent of two writers 40 times so their wbs are killed and replaced in
the same slot (240 calls of patch 3's switch, 16200 inode switches,
counted with bpftrace), and the patch 1 and 3 tests in their changelogs
and the race test below. W=1 and checkpatch --strict clean, no new
sparse warnings. All changes are under CONFIG_CGROUP_WRITEBACK, and only
builds with it enabled were done for v6.

Not tested: x86 runtime, this series in production, linux-next, and
patch 3's fallback for a successor that is already gone (not reachable
on demand; it is cleanup_offline_cgwb()'s loop).

Workaround until this lands: syncfs after the old container's last
write (twice on ext4). Lowering vm.dirty_expire_centisecs from 3000 to
500 cut the worst lag on unpatched mainline to 4.8s, 4.6s and 14.3s, but
did not remove the stall.

Related: without 168a8c13159c the flush is sized from the dead memcg's
own dirty pages, so this only helps while it has some. f6988c90671e
(mainline) also matters: on XFS with 500k synced inodes and the pod
cgroup removed 10s later, the handover was slow enough without it that
the replacement fell 21-22s behind (vfs-7.4.misc), against 1.0-1.2s with
it (mainline), both with v1 of this series.

In patch 3's test all 5001 inodes switched within 27-39ms, and on
mainline the sibling lagged at most 1.1s. As with today's per-inode
switches, switched inodes land at the newest end of the new wb's b_dirty
with their old dirtied_when, so periodic writeback may reach them up to
dirty_expire_centisecs late; background and foreign flushes are
unaffected.

To test the re-kick in patch 3, a debug-only delay in
inode_switch_wbs_work_fn() held back foreign switches to a memcg's wb
until after io was disabled on its parent, a new wb took over its slot
and the memcg was removed. With v5, the 61, 62 and 19 inodes that then
landed on the replaced wb stayed there until the next memcg offline ran
cleanup_offline_cgwbs_workfn(); with v6, all 5, 35 and 82 that landed
were queued to the successor as they landed, checked by wb pointer with
bpftrace. Switches queued after the takeover went to the successor
directly. The inodes were clean by the time they landed, so this shows
where they end up, not a stall. On the lockdep kernel, the re-kick moved
165, 82 and 198 inodes from the replaced wb to the successor in three
runs.

Developed with Claude Opus 5.5, which wrote the code, the reproducers
and first drafts of this text, and ran the builds and VM tests. Claude
Fable 5.1 reviewed the code and the claims in this text before v2 to v6
were posted. I drove the investigation from the production symptoms,
designed the experiments and controls (repeated runs, parent-commit
baselines, testing this patch on its own), and reviewed the analysis,
code and results.

Minimal recipe:
	#!/bin/bash
	# As root, cgroup v2, in a directory on ext4/xfs/btrfs ($DIR).
	cg=/sys/fs/cgroup/wbmini
	mkdir $cg && echo +memory > $cg/cgroup.subtree_control
	echo 4G > $cg/memory.max
	mkdir $cg/old $cg/new
	append() {  # append $1 blocks of $2 to each of 1000 files
		for i in $(seq 1000); do
			dd if=/dev/zero of=$DIR/f$i bs=$2 count=$1 \
			   oflag=append conv=notrunc status=none
		done
	}
	(echo $BASHPID > $cg/old/cgroup.procs
	 append 8 1M            # old owner writes the files...
	 append 1 64k)          # ...and exits with all of them dirty
	rmdir $cg/old           # its cgroup goes away while they are dirty
	bpftrace -e 'kretprobe:cgroup_writeback_by_id {
		@ret[(int32)retval] = count(); }' &
	sleep 3
	time (echo $BASHPID > $cg/new/cgroup.procs
	      append 4 1M)      # replacement keeps appending to the same files
	kill -INT $!; wait
	rmdir $cg/new $cg
	# Unpatched: many @ret[-2] (-ENOENT) and the append stalls.
	# Patched: @ret[0] only.

---
Changes in v6:
- Patch 1: trim the test paragraph (Tejun)
- Patch 2: comment updates for wb_get_lookup(), wb_get_create(),
  struct bdi_writeback and wb_dying() (Tejun)
- Patch 3: queue replaced_work again when a switch lands on a wb that
  was replaced after the switch was queued; share the tryget, queue
  and put on failure in a helper (Tejun)
- Link to v5: https://patch.msgid.link/20261001-wb-dying-cgwb-flush-v5-0-8361eb8c65c6@honeycomb.io

Changes in v5:
- New patch 1: clear the old wb's dirty IO state after switching
  inodes (Tejun)
- Patch 2: drop the css_is_dying() check, not needed with patch 3
  (Tejun)
- Patch 3: if the successor is gone, fall back like
  cleanup_offline_cgwb(); report a Tasks-RCU quiescent state per
  pass, as 407a5d205179 does (Tejun)
- Also tested on mainline (7.3-rc6) with 168a8c13159c
- Link to v4: https://patch.msgid.link/20260930-wb-dying-cgwb-flush-v4-0-bde637803a96@honeycomb.io

Changes in v4:
- Don't link a new wb for a dying memcg in cgwb_create() (Tejun)
- Patch 2: switch the replaced wb's inodes to its successor instead
  of kicking writeback (Tejun, Jan)
- Link to v3: https://patch.msgid.link/20260928-wb-dying-cgwb-flush-v3-0-e35374884667@honeycomb.io

Changes in v3:
- Use wb_dying(), scoped_guard() with the offline_node list_del moved
  into it, and {}; filter dying wbs right after the lookup in
  cgwb_create(); comment wording (Tejun)
- New patch 2: kick writeback on a wb replaced in cgwb_create() (Tejun)
- Lead patch 1's changelog with the problem and its trigger (Andrew)
- Link to v2: https://patch.msgid.link/20260928-wb-dying-cgwb-flush-v2-1-56b54cda74f2@honeycomb.io

Changes in v2:
- Keep killed wbs in bdi->cgwb_tree until release instead of walking
  bdi->wb_list (Tejun)
- Drop Fixes: and Cc: stable (Tejun)
- Link to v1: https://patch.msgid.link/20260926-wb-dying-cgwb-flush-v1-1-a8d898085a3a@honeycomb.io

---
Liz Fong-Jones (3):
      writeback: clear the old wb's dirty IO state after switching inodes
      writeback: let foreign flushes reach dying cgwbs
      writeback: switch a replaced cgwb's inodes to its successor

 fs/fs-writeback.c                |  67 +++++++++++++++++++
 include/linux/backing-dev-defs.h |  23 +++++--
 include/linux/backing-dev.h      |   3 +-
 include/linux/writeback.h        |   1 +
 mm/backing-dev.c                 | 139 ++++++++++++++++++++++++++++++++-------
 5 files changed, 205 insertions(+), 28 deletions(-)
---
base-commit: 168a8c13159c6e3f0f08da6f8fa2f633a91ba9fd
change-id: 20260926-wb-dying-cgwb-flush-06e260e2abad

Best regards,
--  
Liz Fong-Jones <lizf@honeycomb.io>

[PATCH v6 1/3] writeback: clear the old wb's dirty IO state after switching inodes

[Liz Fong-Jones]

Switching a dirty inode to another wb can leave WB_has_dirty_io set on
the old one. inode_do_switch_wbs() moves the inode onto new_wb->b_dirty
through inode_io_list_move_locked(), which only updates new_wb, and
nothing calls wb_io_lists_depopulated() on old_wb. A live wb clears the
bit on its next dirty to clean transition, but a wb that gets no more
inodes, such as a dying one, is freed with its avg_write_bandwidth still
in bdi->tot_write_bandwidth, which wb_split_bdi_pages() and
wb_min_max_ratio() divide by.

Call wb_io_lists_depopulated(old_wb) after the switch loop in
process_inode_switch_wbs(), while old_wb->list_lock is still held.

Test: after the last patch in this series switched a replaced wb's dirty
inodes away, BdiWriteBandwidth stayed at 411700 and 407820 kBps with no
writers and b_dirty, b_io and b_more_io all empty; with this patch it
read 0, in 3 of 3 runs.

Suggested-by: Tejun Heo <tj@kernel.org>
Assisted-by: Claude:claude-opus-5-5 checkpatch sparse
Assisted-by: Claude:claude-fable-5-1
Acked-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Liz Fong-Jones <lizf@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 d26b2cf05283..050d507aa554 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -566,6 +566,12 @@ static void process_inode_switch_wbs(struct bdi_writeback *new_wb,
 		}
 	}
 
+	/*
+	 * inode_do_switch_wbs() only updates @new_wb's dirty IO state.  Clear
+	 * @old_wb's if its IO lists are now empty.
+	 */
+	wb_io_lists_depopulated(old_wb);
+
 	spin_unlock(&new_wb->list_lock);
 	spin_unlock(&old_wb->list_lock);
 

-- 
2.53.0

[PATCH v6 2/3] writeback: let foreign flushes reach dying cgwbs

[Liz Fong-Jones]

When a container is replaced and the new one keeps appending to files
the old one left dirty, the new one can stall in balance_dirty_pages()
for seconds to minutes, with little CPU use and its write lag growing
linearly before it recovers. This matches a 30-60s stall after each
deploy of a container at Honeycomb that reads from Kafka and writes
columnar files to a host volume.

Trigger: a cgroup dirties files and is removed while they are still
dirty, and a sibling keeps appending to the same files under a parent
memory limit. cgwb_kill() takes the removed memcg's wbs out of
bdi->cgwb_tree, but the inodes stay attached to them, so the sibling's
foreign flushes fail with -ENOENT in wb_get_lookup().

Keep killed wbs in bdi->cgwb_tree until they are released, so that
cgroup_writeback_by_id() finds them through the regular lookup. The
creation paths skip dying wbs, and a new wb takes over the slot of a
dying one when the memcg's blkcg association changes; release only
removes a wb that still owns its slot. The blkcg association check moves
from wb_get_lookup() to the creation side: once a memcg with io enabled
is removed, cgroup_get_e_css() returns an ancestor's io css and its
dying wb would not match.

With commit 168a8c13159c ("writeback: size foreign flushes by target
wb dirty pages"), the flush is then sized from the wb's own dirty
pages and writes out what the replacement dirtied.

Test, on top of that commit: a cgroup appends to 1000 files at
250MiB/s and is removed, then a second cgroup keeps appending to the
same files under a 4G parent limit. Worst lag of the second writer
behind schedule over 120s, in three runs:

  without this patch   85.3s  46.9s  52.1s
  with this patch       1.0s   1.2s   1.2s

Suggested-by: Tejun Heo <tj@kernel.org>
Assisted-by: Claude:claude-opus-5-5 checkpatch sparse
Assisted-by: Claude:claude-fable-5-1
Acked-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Liz Fong-Jones <lizf@honeycomb.io>
---
 include/linux/backing-dev-defs.h | 22 ++++++++--
 include/linux/backing-dev.h      |  2 +-
 mm/backing-dev.c                 | 90 ++++++++++++++++++++++++++++++----------
 3 files changed, 86 insertions(+), 28 deletions(-)

diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
index 4f1084937315..33e06b1b1e1c 100644
--- a/include/linux/backing-dev-defs.h
+++ b/include/linux/backing-dev-defs.h
@@ -100,8 +100,10 @@ struct wb_completion {
  * refcounted with the number of inodes attached to it, and pins the memcg
  * and the corresponding blkcg.  As the corresponding blkcg for a memcg may
  * change as blkcg is disabled and enabled higher up in the hierarchy, a wb
- * is tested for blkcg after lookup and removed from index on mismatch so
- * that a new wb for the combination can be created.
+ * is tested for blkcg after lookup and killed on mismatch so that a new wb
+ * for the combination can be created.  A killed wb stays in the index until
+ * it is released or a new wb takes over its slot, so that foreign flushes
+ * can still reach it.
  *
  * Each bdi_writeback that is not embedded into the backing_dev_info must hold
  * a reference to the parent backing_dev_info.  See cgwb_create() for details.
@@ -196,7 +198,7 @@ struct backing_dev_info {
 	struct bdi_writeback wb;  /* the root writeback info for this bdi */
 	struct list_head wb_list; /* list of all wbs */
 #ifdef CONFIG_CGROUP_WRITEBACK
-	struct radix_tree_root cgwb_tree; /* radix tree of active cgroup wbs */
+	struct radix_tree_root cgwb_tree; /* radix tree of cgroup wbs, incl. killed */
 	struct mutex cgwb_release_mutex;  /* protect shutdown of wb structs */
 	struct rw_semaphore wb_switch_rwsem; /* no cgwb switch while syncing */
 #endif
@@ -229,6 +231,17 @@ static inline bool wb_tryget(struct bdi_writeback *wb)
 	return true;
 }
 
+/**
+ * wb_tryget_live - try to increment a wb's refcount if it hasn't been killed
+ * @wb: bdi_writeback to get
+ */
+static inline bool wb_tryget_live(struct bdi_writeback *wb)
+{
+	if (wb != &wb->bdi->wb)
+		return percpu_ref_tryget_live(&wb->refcnt);
+	return true;
+}
+
 /**
  * wb_get - increment a wb's refcount
  * @wb: bdi_writeback to get
@@ -271,7 +284,8 @@ static inline void wb_put(struct bdi_writeback *wb)
  * wb_dying - is a wb dying?
  * @wb: bdi_writeback of interest
  *
- * Returns whether @wb is unlinked and being drained.
+ * Returns whether @wb has been killed and is being drained.  A dying wb may
+ * still be in bdi->cgwb_tree, see cgwb_kill().
  */
 static inline bool wb_dying(struct bdi_writeback *wb)
 {
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index c2284466e7aa..f7ef5895625a 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -222,7 +222,7 @@ wb_get_create_current(struct backing_dev_info *bdi, gfp_t gfp)
 
 	rcu_read_lock();
 	wb = wb_find_current(bdi);
-	if (wb && unlikely(!wb_tryget(wb)))
+	if (wb && unlikely(!wb_tryget_live(wb)))
 		wb = NULL;
 	rcu_read_unlock();
 
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index cecbcf9060a6..85b2c95a920c 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -614,6 +614,12 @@ static void cgwb_release_workfn(struct work_struct *work)
 						release_work);
 	struct backing_dev_info *bdi = wb->bdi;
 
+	scoped_guard(spinlock_irq, &cgwb_lock) {
+		/* a newer wb may have taken the slot, see cgwb_create() */
+		radix_tree_delete_item(&bdi->cgwb_tree, wb->memcg_css->id, wb);
+		list_del(&wb->offline_node);
+	}
+
 	mutex_lock(&wb->bdi->cgwb_release_mutex);
 	wb_shutdown(wb);
 
@@ -627,10 +633,6 @@ static void cgwb_release_workfn(struct work_struct *work)
 
 	fprop_local_destroy_percpu(&wb->memcg_completions);
 
-	spin_lock_irq(&cgwb_lock);
-	list_del(&wb->offline_node);
-	spin_unlock_irq(&cgwb_lock);
-
 	wb_exit(wb);
 	bdi_put(bdi);
 	WARN_ON_ONCE(!list_empty(&wb->b_attached));
@@ -645,11 +647,16 @@ static void cgwb_release(struct percpu_ref *refcnt)
 	queue_work(cgwb_release_wq, &wb->release_work);
 }
 
+/*
+ * A killed wb stays in bdi->cgwb_tree until it is released or replaced in
+ * cgwb_create(), so that foreign flushes can still find it through
+ * wb_get_lookup().  Inodes attached to it can keep collecting dirty pages
+ * from other memcgs until they are written back and switched away.
+ */
 static void cgwb_kill(struct bdi_writeback *wb)
 {
 	lockdep_assert_held(&cgwb_lock);
 
-	WARN_ON(!radix_tree_delete(&wb->bdi->cgwb_tree, wb->memcg_css->id));
 	list_del(&wb->memcg_node);
 	list_del(&wb->blkcg_node);
 	list_add(&wb->offline_node, &offline_cgwbs);
@@ -669,7 +676,8 @@ static int cgwb_create(struct backing_dev_info *bdi,
 	struct mem_cgroup *memcg;
 	struct cgroup_subsys_state *blkcg_css;
 	struct list_head *memcg_cgwb_list, *blkcg_cgwb_list;
-	struct bdi_writeback *wb;
+	struct bdi_writeback *wb, *old_wb;
+	void __rcu **slot;
 	unsigned long flags;
 	int ret = 0;
 
@@ -681,6 +689,8 @@ static int cgwb_create(struct backing_dev_info *bdi,
 	/* look up again under lock and discard on blkcg mismatch */
 	spin_lock_irqsave(&cgwb_lock, flags);
 	wb = radix_tree_lookup(&bdi->cgwb_tree, memcg_css->id);
+	if (wb && wb_dying(wb))
+		wb = NULL;
 	if (wb && wb->blkcg_css != blkcg_css) {
 		cgwb_kill(wb);
 		wb = NULL;
@@ -727,8 +737,22 @@ static int cgwb_create(struct backing_dev_info *bdi,
 	spin_lock_irqsave(&cgwb_lock, flags);
 	if (test_bit(WB_registered, &bdi->wb.state) &&
 	    blkcg_cgwb_list->next && memcg_cgwb_list->next) {
-		/* we might have raced another instance of this function */
-		ret = radix_tree_insert(&bdi->cgwb_tree, memcg_css->id, wb);
+		/*
+		 * We might have raced another instance of this function.  A
+		 * dying wb keeps its slot until released; take it over.
+		 */
+		slot = radix_tree_lookup_slot(&bdi->cgwb_tree, memcg_css->id);
+		if (!slot) {
+			ret = radix_tree_insert(&bdi->cgwb_tree, memcg_css->id, wb);
+		} else {
+			old_wb = radix_tree_deref_slot_protected(slot, &cgwb_lock);
+			if (wb_dying(old_wb)) {
+				radix_tree_replace_slot(&bdi->cgwb_tree, slot, wb);
+				ret = 0;
+			} else {
+				ret = -EEXIST;
+			}
+		}
 		if (!ret) {
 			list_add_tail_rcu(&wb->bdi_node, &bdi->wb_list);
 			list_add(&wb->memcg_node, memcg_cgwb_list);
@@ -765,13 +789,27 @@ static int cgwb_create(struct backing_dev_info *bdi,
  * @bdi: target bdi
  * @memcg_css: cgroup_subsys_state of the target memcg (must have positive ref)
  *
- * Try to get the wb for @memcg_css on @bdi.  The returned wb has its
- * refcount incremented.
- *
- * This function uses css_get() on @memcg_css and thus expects its refcnt
- * to be positive on invocation.  IOW, rcu_read_lock() protection on
- * @memcg_css isn't enough.  try_get it before calling this function.
- *
+ * Try to get the wb in @memcg_css's slot on @bdi, killed or not.  The
+ * returned wb has its refcount incremented.
+ */
+struct bdi_writeback *wb_get_lookup(struct backing_dev_info *bdi,
+				    struct cgroup_subsys_state *memcg_css)
+{
+	struct bdi_writeback *wb;
+
+	if (!memcg_css->parent)
+		return &bdi->wb;
+
+	rcu_read_lock();
+	wb = radix_tree_lookup(&bdi->cgwb_tree, memcg_css->id);
+	if (wb && !wb_tryget(wb))
+		wb = NULL;
+	rcu_read_unlock();
+
+	return wb;
+}
+
+/*
  * A wb is keyed by its associated memcg.  As blkcg implicitly enables
  * memcg on the default hierarchy, memcg association is guaranteed to be
  * more specific (equal or descendant to the associated blkcg) and thus can
@@ -783,8 +821,8 @@ static int cgwb_create(struct backing_dev_info *bdi,
  * each lookup.  On mismatch, the existing wb is discarded and a new one is
  * created.
  */
-struct bdi_writeback *wb_get_lookup(struct backing_dev_info *bdi,
-				    struct cgroup_subsys_state *memcg_css)
+static struct bdi_writeback *cgwb_get_live(struct backing_dev_info *bdi,
+					   struct cgroup_subsys_state *memcg_css)
 {
 	struct bdi_writeback *wb;
 
@@ -798,7 +836,7 @@ struct bdi_writeback *wb_get_lookup(struct backing_dev_info *bdi,
 
 		/* see whether the blkcg association has changed */
 		blkcg_css = cgroup_get_e_css(memcg_css->cgroup, &io_cgrp_subsys);
-		if (unlikely(wb->blkcg_css != blkcg_css || !wb_tryget(wb)))
+		if (unlikely(wb->blkcg_css != blkcg_css || !wb_tryget_live(wb)))
 			wb = NULL;
 		css_put(blkcg_css);
 	}
@@ -813,8 +851,12 @@ struct bdi_writeback *wb_get_lookup(struct backing_dev_info *bdi,
  * @memcg_css: cgroup_subsys_state of the target memcg (must have positive ref)
  * @gfp: allocation mask to use
  *
- * Try to get the wb for @memcg_css on @bdi.  If it doesn't exist, try to
- * create one.  See wb_get_lookup() for more details.
+ * Try to get the live wb for @memcg_css on @bdi.  If it doesn't exist, try
+ * to create one.  See cgwb_get_live() for more details.
+ *
+ * This function uses css_get() on @memcg_css and thus expects its refcnt
+ * to be positive on invocation.  IOW, rcu_read_lock() protection on
+ * @memcg_css isn't enough.  try_get it before calling this function.
  */
 struct bdi_writeback *wb_get_create(struct backing_dev_info *bdi,
 				    struct cgroup_subsys_state *memcg_css,
@@ -825,7 +867,7 @@ struct bdi_writeback *wb_get_create(struct backing_dev_info *bdi,
 	might_alloc(gfp);
 
 	do {
-		wb = wb_get_lookup(bdi, memcg_css);
+		wb = cgwb_get_live(bdi, memcg_css);
 	} while (!wb && !cgwb_create(bdi, memcg_css, gfp));
 
 	return wb;
@@ -858,8 +900,10 @@ static void cgwb_bdi_unregister(struct backing_dev_info *bdi)
 	WARN_ON(test_bit(WB_registered, &bdi->wb.state));
 
 	spin_lock_irq(&cgwb_lock);
-	radix_tree_for_each_slot(slot, &bdi->cgwb_tree, &iter, 0)
-		cgwb_kill(*slot);
+	radix_tree_for_each_slot(slot, &bdi->cgwb_tree, &iter, 0) {
+		if (!wb_dying(*slot))
+			cgwb_kill(*slot);
+	}
 	spin_unlock_irq(&cgwb_lock);
 
 	mutex_lock(&bdi->cgwb_release_mutex);

-- 
2.53.0

[PATCH v6 3/3] writeback: switch a replaced cgwb's inodes to its successor

[Liz Fong-Jones]

When a live memcg's blkcg association changes, cgwb_create() replaces
its killed wb in bdi->cgwb_tree. Foreign flushes for the memcg then
find the new wb, but the inodes, dirty or not, are still attached to the
old one, and they only move over as they are written back (see
wbc_attach_and_unlock_inode()). Until then, other memcgs appending to
those inodes stall in balance_dirty_pages().

After the takeover, queue a work item that switches all of the old wb's
inodes to the wb foreign flushes now find, like cleanup_offline_cgwb()
does for b_attached and b_dirty_time, but including b_dirty, b_io and
b_more_io. The switch carries the dirty and writeback page counts, so
nothing needs to be written back first. cgwb_create() can run with
interrupts disabled (folio_account_dirtied() -> inode_attach_wb()), so
it only queues the work. A switch to the old wb that was queued before
it was replaced can land after the work ran, so
inode_switch_wbs_work_fn() queues the work again when the wb it switched
inodes to is dying and no longer owns its slot. An inode attached to the
old wb as it is replaced (see __inode_attach_wb()) still moves over only
when written back. If the successor is already gone, fall back like
cleanup_offline_cgwb() does, to the nearest live ancestor's wb or the
root wb.

Test: a live cgroup appends to 5000 files on XFS at 250MiB/s, io is
disabled on its parent so its wb is killed, it creates a file (the
takeover), and a sibling keeps appending to its files under a 4G parent
limit. Worst lag of the sibling behind schedule over 30s, in three runs:

  without this patch    9.1s   4.0s   1.0s
  with this patch       1.0s   1.1s   1.1s

Without this patch, moving the old wb's inodes over took up to 11s,
first switch to last; with it, all 5001 were switched in a 27-39ms
window, with up to 311MiB of the old cgroup's pages dirty.

Suggested-by: Tejun Heo <tj@kernel.org>
Assisted-by: Claude:claude-opus-5-5 checkpatch sparse
Assisted-by: Claude:claude-fable-5-1
Signed-off-by: Liz Fong-Jones <lizf@honeycomb.io>
---
 fs/fs-writeback.c                | 61 ++++++++++++++++++++++++++++++++++++++++
 include/linux/backing-dev-defs.h |  1 +
 include/linux/backing-dev.h      |  1 +
 include/linux/writeback.h        |  1 +
 mm/backing-dev.c                 | 49 ++++++++++++++++++++++++++++++++
 5 files changed, 113 insertions(+)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 050d507aa554..7e80f3935dce 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -624,6 +624,8 @@ void inode_switch_wbs_work_fn(struct work_struct *work)
 
 	llist_for_each_entry_safe(isw, next_isw, list, list)
 		process_inode_switch_wbs(new_wb, isw);
+	/* @new_wb may have been replaced while the switches were queued */
+	cgwb_kick_replaced(new_wb);
 	wb_put(new_wb);
 }
 
@@ -809,6 +811,65 @@ bool cleanup_offline_cgwb(struct bdi_writeback *wb)
 	return restart;
 }
 
+/**
+ * switch_replaced_cgwb - switch a replaced wb's inodes to its successor
+ * @wb: target wb, replaced in bdi->cgwb_tree by another wb of its memcg
+ *
+ * Switch all inodes attached to @wb, dirty or not, to the wb that foreign
+ * flushes now find for @wb's memcg.  If that one is already gone, fall back
+ * like cleanup_offline_cgwb() does, to the nearest live ancestor's wb or the
+ * root wb.  The switch carries the dirty and writeback page counts, so
+ * nothing needs to be written back first.  Returns %true if not all inodes
+ * were switched and the function has to be restarted.
+ */
+bool switch_replaced_cgwb(struct bdi_writeback *wb)
+{
+	struct cgroup_subsys_state *memcg_css;
+	struct inode_switch_wbs_context *isw;
+	struct bdi_writeback *new_wb;
+	bool restart;
+	int nr = 0;
+
+	new_wb = wb_get_lookup(wb->bdi, wb->memcg_css);
+	for (memcg_css = wb->memcg_css->parent; !new_wb && memcg_css;
+	     memcg_css = memcg_css->parent)
+		new_wb = wb_get_create(wb->bdi, memcg_css, GFP_KERNEL);
+	if (!new_wb)
+		new_wb = &wb->bdi->wb; /* wb_get() is noop for bdi's wb */
+	if (WARN_ON_ONCE(new_wb == wb)) {
+		wb_put(new_wb);
+		return false;
+	}
+
+	isw = kzalloc_flex(*isw, inodes, WB_MAX_INODES_PER_ISW);
+	if (!isw) {
+		wb_put(new_wb);
+		return false;
+	}
+
+	atomic_inc(&isw_nr_in_flight);
+
+	spin_lock(&wb->list_lock);
+	restart = isw_prepare_wbs_switch(new_wb, isw, &wb->b_attached, &nr) ||
+		  isw_prepare_wbs_switch(new_wb, isw, &wb->b_dirty, &nr) ||
+		  isw_prepare_wbs_switch(new_wb, isw, &wb->b_io, &nr) ||
+		  isw_prepare_wbs_switch(new_wb, isw, &wb->b_more_io, &nr) ||
+		  isw_prepare_wbs_switch(new_wb, isw, &wb->b_dirty_time, &nr);
+	spin_unlock(&wb->list_lock);
+
+	if (nr == 0) {
+		atomic_dec(&isw_nr_in_flight);
+		wb_put(new_wb);
+		kfree(isw);
+		return restart;
+	}
+
+	trace_inode_switch_wbs_queue(wb, new_wb, nr);
+	wb_queue_isw(new_wb, isw);
+
+	return restart;
+}
+
 /**
  * wbc_attach_and_unlock_inode - associate wbc with target inode and unlock it
  * @wbc: writeback_control of interest
diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
index 33e06b1b1e1c..7713fd60c985 100644
--- a/include/linux/backing-dev-defs.h
+++ b/include/linux/backing-dev-defs.h
@@ -162,6 +162,7 @@ struct bdi_writeback {
 					 * to this wb */
 	struct llist_head switch_wbs_ctxs;	/* queued contexts for
 						 * writeback switching */
+	struct work_struct replaced_work;	/* see switch_replaced_cgwb() */
 
 	union {
 		struct work_struct release_work;
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index f7ef5895625a..84057fa92f24 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -151,6 +151,7 @@ static inline void bdi_wb_stat_mod(struct inode *inode, enum wb_stat_item item,
 
 struct bdi_writeback *wb_get_lookup(struct backing_dev_info *bdi,
 				    struct cgroup_subsys_state *memcg_css);
+void cgwb_kick_replaced(struct bdi_writeback *wb);
 struct bdi_writeback *wb_get_create(struct backing_dev_info *bdi,
 				    struct cgroup_subsys_state *memcg_css,
 				    gfp_t gfp);
diff --git a/include/linux/writeback.h b/include/linux/writeback.h
index b749a9a5a5ee..88e4b5049698 100644
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -209,6 +209,7 @@ int cgroup_writeback_by_id(u64 bdi_id, int memcg_id,
 			   enum wb_reason reason, struct wb_completion *done);
 void cgroup_writeback_umount(struct super_block *sb);
 bool cleanup_offline_cgwb(struct bdi_writeback *wb);
+bool switch_replaced_cgwb(struct bdi_writeback *wb);
 
 /**
  * inode_attach_wb - associate an inode with its wb
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 85b2c95a920c..e401293c260a 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -637,9 +637,51 @@ static void cgwb_release_workfn(struct work_struct *work)
 	bdi_put(bdi);
 	WARN_ON_ONCE(!list_empty(&wb->b_attached));
 	WARN_ON_ONCE(work_pending(&wb->switch_work));
+	WARN_ON_ONCE(work_pending(&wb->replaced_work));
 	call_rcu(&wb->rcu, cgwb_free_rcu);
 }
 
+static void cgwb_replaced_workfn(struct work_struct *work)
+{
+	struct bdi_writeback *wb = container_of(work, struct bdi_writeback,
+						replaced_work);
+
+	do {
+		cond_resched_tasks_rcu_qs();
+	} while (switch_replaced_cgwb(wb));
+	wb_put(wb);
+}
+
+/* called with cgwb_lock held, possibly with interrupts disabled */
+static void cgwb_queue_replaced_work(struct bdi_writeback *wb)
+{
+	lockdep_assert_held(&cgwb_lock);
+
+	if (wb_tryget(wb) && !queue_work(system_dfl_wq, &wb->replaced_work))
+		wb_put(wb);
+}
+
+/**
+ * cgwb_kick_replaced - switch inodes away from @wb again if it was replaced
+ * @wb: wb that inodes were just switched to
+ *
+ * An inode switch pins its target wb before the switch lands, and the wb
+ * may be replaced in cgwb_create() and have its replaced_work run in
+ * between.  Kick the work again so that those inodes reach the successor
+ * too.  A dying wb that still owns its slot is left alone: foreign flushes
+ * can reach it, and the work would look up @wb itself.
+ */
+void cgwb_kick_replaced(struct bdi_writeback *wb)
+{
+	if (!wb_dying(wb))
+		return;
+
+	spin_lock_irq(&cgwb_lock);
+	if (radix_tree_lookup(&wb->bdi->cgwb_tree, wb->memcg_css->id) != wb)
+		cgwb_queue_replaced_work(wb);
+	spin_unlock_irq(&cgwb_lock);
+}
+
 static void cgwb_release(struct percpu_ref *refcnt)
 {
 	struct bdi_writeback *wb = container_of(refcnt, struct bdi_writeback,
@@ -724,6 +766,7 @@ static int cgwb_create(struct backing_dev_info *bdi,
 	INIT_WORK(&wb->switch_work, inode_switch_wbs_work_fn);
 	init_llist_head(&wb->switch_wbs_ctxs);
 	INIT_WORK(&wb->release_work, cgwb_release_workfn);
+	INIT_WORK(&wb->replaced_work, cgwb_replaced_workfn);
 	set_bit(WB_registered, &wb->state);
 	bdi_get(bdi);
 
@@ -748,6 +791,12 @@ static int cgwb_create(struct backing_dev_info *bdi,
 			old_wb = radix_tree_deref_slot_protected(slot, &cgwb_lock);
 			if (wb_dying(old_wb)) {
 				radix_tree_replace_slot(&bdi->cgwb_tree, slot, wb);
+				/*
+				 * @old_wb is out of foreign flushes' reach
+				 * but may still have inodes attached, dirty
+				 * or not.  Switch them over to @wb.
+				 */
+				cgwb_queue_replaced_work(old_wb);
 				ret = 0;
 			} else {
 				ret = -EEXIST;

-- 
2.53.0

Re: [PATCH v6 0/3] writeback: let foreign flushes reach dying cgwbs

[Liz Fong-Jones]

Tejun and Jan,

Gentle ping on v6. Patches 1 and 2 carry Tejun's acks. Patch 3 changed
in v6 to queue replaced_work again when a switch lands on a wb that was
replaced after the switch was queued, as Tejun suggested on v5, and it
hasn't had review yet. Jan, you confirmed on v3 that switching all of a
replaced wb's inodes, dirty ones included, is safe; would you have time
to look at patch 3?

Christian, if they're happy with it, could the series go into
vfs-7.4.misc next to Xin's 168a8c13159c? It still applies cleanly there
and to 7.3-rc7. If that's too late for this cycle, I'm happy to repost
on whatever base you'd like to queue it on.

Thanks,
Liz

Re: [PATCH v6 1/3] writeback: clear the old wb's dirty IO state after switching inodes

[Jan Kara]

On Fri 02-10-26 21:40:45, Liz Fong-Jones wrote:
> Switching a dirty inode to another wb can leave WB_has_dirty_io set on
> the old one. inode_do_switch_wbs() moves the inode onto new_wb->b_dirty
> through inode_io_list_move_locked(), which only updates new_wb, and
> nothing calls wb_io_lists_depopulated() on old_wb. A live wb clears the
> bit on its next dirty to clean transition, but a wb that gets no more
> inodes, such as a dying one, is freed with its avg_write_bandwidth still
> in bdi->tot_write_bandwidth, which wb_split_bdi_pages() and
> wb_min_max_ratio() divide by.
> 
> Call wb_io_lists_depopulated(old_wb) after the switch loop in
> process_inode_switch_wbs(), while old_wb->list_lock is still held.
> 
> Test: after the last patch in this series switched a replaced wb's dirty
> inodes away, BdiWriteBandwidth stayed at 411700 and 407820 kBps with no
> writers and b_dirty, b_io and b_more_io all empty; with this patch it
> read 0, in 3 of 3 runs.
> 
> Suggested-by: Tejun Heo <tj@kernel.org>
> Assisted-by: Claude:claude-opus-5-5 checkpatch sparse
> Assisted-by: Claude:claude-fable-5-1
> Acked-by: Tejun Heo <tj@kernel.org>
> Signed-off-by: Liz Fong-Jones <lizf@honeycomb.io>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/fs-writeback.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index d26b2cf05283..050d507aa554 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -566,6 +566,12 @@ static void process_inode_switch_wbs(struct bdi_writeback *new_wb,
>  		}
>  	}
>  
> +	/*
> +	 * inode_do_switch_wbs() only updates @new_wb's dirty IO state.  Clear
> +	 * @old_wb's if its IO lists are now empty.
> +	 */
> +	wb_io_lists_depopulated(old_wb);
> +
>  	spin_unlock(&new_wb->list_lock);
>  	spin_unlock(&old_wb->list_lock);
>  
> 
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v6 2/3] writeback: let foreign flushes reach dying cgwbs

[Jan Kara]

On Fri 02-10-26 21:40:46, Liz Fong-Jones wrote:
> When a container is replaced and the new one keeps appending to files
> the old one left dirty, the new one can stall in balance_dirty_pages()
> for seconds to minutes, with little CPU use and its write lag growing
> linearly before it recovers. This matches a 30-60s stall after each
> deploy of a container at Honeycomb that reads from Kafka and writes
> columnar files to a host volume.
> 
> Trigger: a cgroup dirties files and is removed while they are still
> dirty, and a sibling keeps appending to the same files under a parent
> memory limit. cgwb_kill() takes the removed memcg's wbs out of
> bdi->cgwb_tree, but the inodes stay attached to them, so the sibling's
> foreign flushes fail with -ENOENT in wb_get_lookup().
> 
> Keep killed wbs in bdi->cgwb_tree until they are released, so that
> cgroup_writeback_by_id() finds them through the regular lookup. The
> creation paths skip dying wbs, and a new wb takes over the slot of a
> dying one when the memcg's blkcg association changes; release only
> removes a wb that still owns its slot. The blkcg association check moves
> from wb_get_lookup() to the creation side: once a memcg with io enabled
> is removed, cgroup_get_e_css() returns an ancestor's io css and its
> dying wb would not match.
> 
> With commit 168a8c13159c ("writeback: size foreign flushes by target
> wb dirty pages"), the flush is then sized from the wb's own dirty
> pages and writes out what the replacement dirtied.
> 
> Test, on top of that commit: a cgroup appends to 1000 files at
> 250MiB/s and is removed, then a second cgroup keeps appending to the
> same files under a 4G parent limit. Worst lag of the second writer
> behind schedule over 120s, in three runs:
> 
>   without this patch   85.3s  46.9s  52.1s
>   with this patch       1.0s   1.2s   1.2s
> 
> Suggested-by: Tejun Heo <tj@kernel.org>
> Assisted-by: Claude:claude-opus-5-5 checkpatch sparse
> Assisted-by: Claude:claude-fable-5-1
> Acked-by: Tejun Heo <tj@kernel.org>
> Signed-off-by: Liz Fong-Jones <lizf@honeycomb.io>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  include/linux/backing-dev-defs.h | 22 ++++++++--
>  include/linux/backing-dev.h      |  2 +-
>  mm/backing-dev.c                 | 90 ++++++++++++++++++++++++++++++----------
>  3 files changed, 86 insertions(+), 28 deletions(-)
> 
> diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
> index 4f1084937315..33e06b1b1e1c 100644
> --- a/include/linux/backing-dev-defs.h
> +++ b/include/linux/backing-dev-defs.h
> @@ -100,8 +100,10 @@ struct wb_completion {
>   * refcounted with the number of inodes attached to it, and pins the memcg
>   * and the corresponding blkcg.  As the corresponding blkcg for a memcg may
>   * change as blkcg is disabled and enabled higher up in the hierarchy, a wb
> - * is tested for blkcg after lookup and removed from index on mismatch so
> - * that a new wb for the combination can be created.
> + * is tested for blkcg after lookup and killed on mismatch so that a new wb
> + * for the combination can be created.  A killed wb stays in the index until
> + * it is released or a new wb takes over its slot, so that foreign flushes
> + * can still reach it.
>   *
>   * Each bdi_writeback that is not embedded into the backing_dev_info must hold
>   * a reference to the parent backing_dev_info.  See cgwb_create() for details.
> @@ -196,7 +198,7 @@ struct backing_dev_info {
>  	struct bdi_writeback wb;  /* the root writeback info for this bdi */
>  	struct list_head wb_list; /* list of all wbs */
>  #ifdef CONFIG_CGROUP_WRITEBACK
> -	struct radix_tree_root cgwb_tree; /* radix tree of active cgroup wbs */
> +	struct radix_tree_root cgwb_tree; /* radix tree of cgroup wbs, incl. killed */
>  	struct mutex cgwb_release_mutex;  /* protect shutdown of wb structs */
>  	struct rw_semaphore wb_switch_rwsem; /* no cgwb switch while syncing */
>  #endif
> @@ -229,6 +231,17 @@ static inline bool wb_tryget(struct bdi_writeback *wb)
>  	return true;
>  }
>  
> +/**
> + * wb_tryget_live - try to increment a wb's refcount if it hasn't been killed
> + * @wb: bdi_writeback to get
> + */
> +static inline bool wb_tryget_live(struct bdi_writeback *wb)
> +{
> +	if (wb != &wb->bdi->wb)
> +		return percpu_ref_tryget_live(&wb->refcnt);
> +	return true;
> +}
> +
>  /**
>   * wb_get - increment a wb's refcount
>   * @wb: bdi_writeback to get
> @@ -271,7 +284,8 @@ static inline void wb_put(struct bdi_writeback *wb)
>   * wb_dying - is a wb dying?
>   * @wb: bdi_writeback of interest
>   *
> - * Returns whether @wb is unlinked and being drained.
> + * Returns whether @wb has been killed and is being drained.  A dying wb may
> + * still be in bdi->cgwb_tree, see cgwb_kill().
>   */
>  static inline bool wb_dying(struct bdi_writeback *wb)
>  {
> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index c2284466e7aa..f7ef5895625a 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -222,7 +222,7 @@ wb_get_create_current(struct backing_dev_info *bdi, gfp_t gfp)
>  
>  	rcu_read_lock();
>  	wb = wb_find_current(bdi);
> -	if (wb && unlikely(!wb_tryget(wb)))
> +	if (wb && unlikely(!wb_tryget_live(wb)))
>  		wb = NULL;
>  	rcu_read_unlock();
>  
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index cecbcf9060a6..85b2c95a920c 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -614,6 +614,12 @@ static void cgwb_release_workfn(struct work_struct *work)
>  						release_work);
>  	struct backing_dev_info *bdi = wb->bdi;
>  
> +	scoped_guard(spinlock_irq, &cgwb_lock) {
> +		/* a newer wb may have taken the slot, see cgwb_create() */
> +		radix_tree_delete_item(&bdi->cgwb_tree, wb->memcg_css->id, wb);
> +		list_del(&wb->offline_node);
> +	}
> +
>  	mutex_lock(&wb->bdi->cgwb_release_mutex);
>  	wb_shutdown(wb);
>  
> @@ -627,10 +633,6 @@ static void cgwb_release_workfn(struct work_struct *work)
>  
>  	fprop_local_destroy_percpu(&wb->memcg_completions);
>  
> -	spin_lock_irq(&cgwb_lock);
> -	list_del(&wb->offline_node);
> -	spin_unlock_irq(&cgwb_lock);
> -
>  	wb_exit(wb);
>  	bdi_put(bdi);
>  	WARN_ON_ONCE(!list_empty(&wb->b_attached));
> @@ -645,11 +647,16 @@ static void cgwb_release(struct percpu_ref *refcnt)
>  	queue_work(cgwb_release_wq, &wb->release_work);
>  }
>  
> +/*
> + * A killed wb stays in bdi->cgwb_tree until it is released or replaced in
> + * cgwb_create(), so that foreign flushes can still find it through
> + * wb_get_lookup().  Inodes attached to it can keep collecting dirty pages
> + * from other memcgs until they are written back and switched away.
> + */
>  static void cgwb_kill(struct bdi_writeback *wb)
>  {
>  	lockdep_assert_held(&cgwb_lock);
>  
> -	WARN_ON(!radix_tree_delete(&wb->bdi->cgwb_tree, wb->memcg_css->id));
>  	list_del(&wb->memcg_node);
>  	list_del(&wb->blkcg_node);
>  	list_add(&wb->offline_node, &offline_cgwbs);
> @@ -669,7 +676,8 @@ static int cgwb_create(struct backing_dev_info *bdi,
>  	struct mem_cgroup *memcg;
>  	struct cgroup_subsys_state *blkcg_css;
>  	struct list_head *memcg_cgwb_list, *blkcg_cgwb_list;
> -	struct bdi_writeback *wb;
> +	struct bdi_writeback *wb, *old_wb;
> +	void __rcu **slot;
>  	unsigned long flags;
>  	int ret = 0;
>  
> @@ -681,6 +689,8 @@ static int cgwb_create(struct backing_dev_info *bdi,
>  	/* look up again under lock and discard on blkcg mismatch */
>  	spin_lock_irqsave(&cgwb_lock, flags);
>  	wb = radix_tree_lookup(&bdi->cgwb_tree, memcg_css->id);
> +	if (wb && wb_dying(wb))
> +		wb = NULL;
>  	if (wb && wb->blkcg_css != blkcg_css) {
>  		cgwb_kill(wb);
>  		wb = NULL;
> @@ -727,8 +737,22 @@ static int cgwb_create(struct backing_dev_info *bdi,
>  	spin_lock_irqsave(&cgwb_lock, flags);
>  	if (test_bit(WB_registered, &bdi->wb.state) &&
>  	    blkcg_cgwb_list->next && memcg_cgwb_list->next) {
> -		/* we might have raced another instance of this function */
> -		ret = radix_tree_insert(&bdi->cgwb_tree, memcg_css->id, wb);
> +		/*
> +		 * We might have raced another instance of this function.  A
> +		 * dying wb keeps its slot until released; take it over.
> +		 */
> +		slot = radix_tree_lookup_slot(&bdi->cgwb_tree, memcg_css->id);
> +		if (!slot) {
> +			ret = radix_tree_insert(&bdi->cgwb_tree, memcg_css->id, wb);
> +		} else {
> +			old_wb = radix_tree_deref_slot_protected(slot, &cgwb_lock);
> +			if (wb_dying(old_wb)) {
> +				radix_tree_replace_slot(&bdi->cgwb_tree, slot, wb);
> +				ret = 0;
> +			} else {
> +				ret = -EEXIST;
> +			}
> +		}
>  		if (!ret) {
>  			list_add_tail_rcu(&wb->bdi_node, &bdi->wb_list);
>  			list_add(&wb->memcg_node, memcg_cgwb_list);
> @@ -765,13 +789,27 @@ static int cgwb_create(struct backing_dev_info *bdi,
>   * @bdi: target bdi
>   * @memcg_css: cgroup_subsys_state of the target memcg (must have positive ref)
>   *
> - * Try to get the wb for @memcg_css on @bdi.  The returned wb has its
> - * refcount incremented.
> - *
> - * This function uses css_get() on @memcg_css and thus expects its refcnt
> - * to be positive on invocation.  IOW, rcu_read_lock() protection on
> - * @memcg_css isn't enough.  try_get it before calling this function.
> - *
> + * Try to get the wb in @memcg_css's slot on @bdi, killed or not.  The
> + * returned wb has its refcount incremented.
> + */
> +struct bdi_writeback *wb_get_lookup(struct backing_dev_info *bdi,
> +				    struct cgroup_subsys_state *memcg_css)
> +{
> +	struct bdi_writeback *wb;
> +
> +	if (!memcg_css->parent)
> +		return &bdi->wb;
> +
> +	rcu_read_lock();
> +	wb = radix_tree_lookup(&bdi->cgwb_tree, memcg_css->id);
> +	if (wb && !wb_tryget(wb))
> +		wb = NULL;
> +	rcu_read_unlock();
> +
> +	return wb;
> +}
> +
> +/*
>   * A wb is keyed by its associated memcg.  As blkcg implicitly enables
>   * memcg on the default hierarchy, memcg association is guaranteed to be
>   * more specific (equal or descendant to the associated blkcg) and thus can
> @@ -783,8 +821,8 @@ static int cgwb_create(struct backing_dev_info *bdi,
>   * each lookup.  On mismatch, the existing wb is discarded and a new one is
>   * created.
>   */
> -struct bdi_writeback *wb_get_lookup(struct backing_dev_info *bdi,
> -				    struct cgroup_subsys_state *memcg_css)
> +static struct bdi_writeback *cgwb_get_live(struct backing_dev_info *bdi,
> +					   struct cgroup_subsys_state *memcg_css)
>  {
>  	struct bdi_writeback *wb;
>  
> @@ -798,7 +836,7 @@ struct bdi_writeback *wb_get_lookup(struct backing_dev_info *bdi,
>  
>  		/* see whether the blkcg association has changed */
>  		blkcg_css = cgroup_get_e_css(memcg_css->cgroup, &io_cgrp_subsys);
> -		if (unlikely(wb->blkcg_css != blkcg_css || !wb_tryget(wb)))
> +		if (unlikely(wb->blkcg_css != blkcg_css || !wb_tryget_live(wb)))
>  			wb = NULL;
>  		css_put(blkcg_css);
>  	}
> @@ -813,8 +851,12 @@ struct bdi_writeback *wb_get_lookup(struct backing_dev_info *bdi,
>   * @memcg_css: cgroup_subsys_state of the target memcg (must have positive ref)
>   * @gfp: allocation mask to use
>   *
> - * Try to get the wb for @memcg_css on @bdi.  If it doesn't exist, try to
> - * create one.  See wb_get_lookup() for more details.
> + * Try to get the live wb for @memcg_css on @bdi.  If it doesn't exist, try
> + * to create one.  See cgwb_get_live() for more details.
> + *
> + * This function uses css_get() on @memcg_css and thus expects its refcnt
> + * to be positive on invocation.  IOW, rcu_read_lock() protection on
> + * @memcg_css isn't enough.  try_get it before calling this function.
>   */
>  struct bdi_writeback *wb_get_create(struct backing_dev_info *bdi,
>  				    struct cgroup_subsys_state *memcg_css,
> @@ -825,7 +867,7 @@ struct bdi_writeback *wb_get_create(struct backing_dev_info *bdi,
>  	might_alloc(gfp);
>  
>  	do {
> -		wb = wb_get_lookup(bdi, memcg_css);
> +		wb = cgwb_get_live(bdi, memcg_css);
>  	} while (!wb && !cgwb_create(bdi, memcg_css, gfp));
>  
>  	return wb;
> @@ -858,8 +900,10 @@ static void cgwb_bdi_unregister(struct backing_dev_info *bdi)
>  	WARN_ON(test_bit(WB_registered, &bdi->wb.state));
>  
>  	spin_lock_irq(&cgwb_lock);
> -	radix_tree_for_each_slot(slot, &bdi->cgwb_tree, &iter, 0)
> -		cgwb_kill(*slot);
> +	radix_tree_for_each_slot(slot, &bdi->cgwb_tree, &iter, 0) {
> +		if (!wb_dying(*slot))
> +			cgwb_kill(*slot);
> +	}
>  	spin_unlock_irq(&cgwb_lock);
>  
>  	mutex_lock(&bdi->cgwb_release_mutex);
> 
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v6 3/3] writeback: switch a replaced cgwb's inodes to its successor

[Jan Kara]

On Fri 02-10-26 21:40:48, Liz Fong-Jones wrote:
> When a live memcg's blkcg association changes, cgwb_create() replaces
> its killed wb in bdi->cgwb_tree. Foreign flushes for the memcg then
> find the new wb, but the inodes, dirty or not, are still attached to the
> old one, and they only move over as they are written back (see
> wbc_attach_and_unlock_inode()). Until then, other memcgs appending to
> those inodes stall in balance_dirty_pages().
> 
> After the takeover, queue a work item that switches all of the old wb's
> inodes to the wb foreign flushes now find, like cleanup_offline_cgwb()
> does for b_attached and b_dirty_time, but including b_dirty, b_io and
> b_more_io. The switch carries the dirty and writeback page counts, so
> nothing needs to be written back first. cgwb_create() can run with
> interrupts disabled (folio_account_dirtied() -> inode_attach_wb()), so
> it only queues the work. A switch to the old wb that was queued before
> it was replaced can land after the work ran, so
> inode_switch_wbs_work_fn() queues the work again when the wb it switched
> inodes to is dying and no longer owns its slot. An inode attached to the
> old wb as it is replaced (see __inode_attach_wb()) still moves over only
> when written back. If the successor is already gone, fall back like
> cleanup_offline_cgwb() does, to the nearest live ancestor's wb or the
> root wb.
> 
> Test: a live cgroup appends to 5000 files on XFS at 250MiB/s, io is
> disabled on its parent so its wb is killed, it creates a file (the
> takeover), and a sibling keeps appending to its files under a 4G parent
> limit. Worst lag of the sibling behind schedule over 30s, in three runs:
> 
>   without this patch    9.1s   4.0s   1.0s
>   with this patch       1.0s   1.1s   1.1s
> 
> Without this patch, moving the old wb's inodes over took up to 11s,
> first switch to last; with it, all 5001 were switched in a 27-39ms
> window, with up to 311MiB of the old cgroup's pages dirty.
> 
> Suggested-by: Tejun Heo <tj@kernel.org>
> Assisted-by: Claude:claude-opus-5-5 checkpatch sparse
> Assisted-by: Claude:claude-fable-5-1
> Signed-off-by: Liz Fong-Jones <lizf@honeycomb.io>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/fs-writeback.c                | 61 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/backing-dev-defs.h |  1 +
>  include/linux/backing-dev.h      |  1 +
>  include/linux/writeback.h        |  1 +
>  mm/backing-dev.c                 | 49 ++++++++++++++++++++++++++++++++
>  5 files changed, 113 insertions(+)
> 
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 050d507aa554..7e80f3935dce 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -624,6 +624,8 @@ void inode_switch_wbs_work_fn(struct work_struct *work)
>  
>  	llist_for_each_entry_safe(isw, next_isw, list, list)
>  		process_inode_switch_wbs(new_wb, isw);
> +	/* @new_wb may have been replaced while the switches were queued */
> +	cgwb_kick_replaced(new_wb);
>  	wb_put(new_wb);
>  }
>  
> @@ -809,6 +811,65 @@ bool cleanup_offline_cgwb(struct bdi_writeback *wb)
>  	return restart;
>  }
>  
> +/**
> + * switch_replaced_cgwb - switch a replaced wb's inodes to its successor
> + * @wb: target wb, replaced in bdi->cgwb_tree by another wb of its memcg
> + *
> + * Switch all inodes attached to @wb, dirty or not, to the wb that foreign
> + * flushes now find for @wb's memcg.  If that one is already gone, fall back
> + * like cleanup_offline_cgwb() does, to the nearest live ancestor's wb or the
> + * root wb.  The switch carries the dirty and writeback page counts, so
> + * nothing needs to be written back first.  Returns %true if not all inodes
> + * were switched and the function has to be restarted.
> + */
> +bool switch_replaced_cgwb(struct bdi_writeback *wb)
> +{
> +	struct cgroup_subsys_state *memcg_css;
> +	struct inode_switch_wbs_context *isw;
> +	struct bdi_writeback *new_wb;
> +	bool restart;
> +	int nr = 0;
> +
> +	new_wb = wb_get_lookup(wb->bdi, wb->memcg_css);
> +	for (memcg_css = wb->memcg_css->parent; !new_wb && memcg_css;
> +	     memcg_css = memcg_css->parent)
> +		new_wb = wb_get_create(wb->bdi, memcg_css, GFP_KERNEL);
> +	if (!new_wb)
> +		new_wb = &wb->bdi->wb; /* wb_get() is noop for bdi's wb */
> +	if (WARN_ON_ONCE(new_wb == wb)) {
> +		wb_put(new_wb);
> +		return false;
> +	}
> +
> +	isw = kzalloc_flex(*isw, inodes, WB_MAX_INODES_PER_ISW);
> +	if (!isw) {
> +		wb_put(new_wb);
> +		return false;
> +	}
> +
> +	atomic_inc(&isw_nr_in_flight);
> +
> +	spin_lock(&wb->list_lock);
> +	restart = isw_prepare_wbs_switch(new_wb, isw, &wb->b_attached, &nr) ||
> +		  isw_prepare_wbs_switch(new_wb, isw, &wb->b_dirty, &nr) ||
> +		  isw_prepare_wbs_switch(new_wb, isw, &wb->b_io, &nr) ||
> +		  isw_prepare_wbs_switch(new_wb, isw, &wb->b_more_io, &nr) ||
> +		  isw_prepare_wbs_switch(new_wb, isw, &wb->b_dirty_time, &nr);
> +	spin_unlock(&wb->list_lock);
> +
> +	if (nr == 0) {
> +		atomic_dec(&isw_nr_in_flight);
> +		wb_put(new_wb);
> +		kfree(isw);
> +		return restart;
> +	}
> +
> +	trace_inode_switch_wbs_queue(wb, new_wb, nr);
> +	wb_queue_isw(new_wb, isw);
> +
> +	return restart;
> +}
> +
>  /**
>   * wbc_attach_and_unlock_inode - associate wbc with target inode and unlock it
>   * @wbc: writeback_control of interest
> diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
> index 33e06b1b1e1c..7713fd60c985 100644
> --- a/include/linux/backing-dev-defs.h
> +++ b/include/linux/backing-dev-defs.h
> @@ -162,6 +162,7 @@ struct bdi_writeback {
>  					 * to this wb */
>  	struct llist_head switch_wbs_ctxs;	/* queued contexts for
>  						 * writeback switching */
> +	struct work_struct replaced_work;	/* see switch_replaced_cgwb() */
>  
>  	union {
>  		struct work_struct release_work;
> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index f7ef5895625a..84057fa92f24 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -151,6 +151,7 @@ static inline void bdi_wb_stat_mod(struct inode *inode, enum wb_stat_item item,
>  
>  struct bdi_writeback *wb_get_lookup(struct backing_dev_info *bdi,
>  				    struct cgroup_subsys_state *memcg_css);
> +void cgwb_kick_replaced(struct bdi_writeback *wb);
>  struct bdi_writeback *wb_get_create(struct backing_dev_info *bdi,
>  				    struct cgroup_subsys_state *memcg_css,
>  				    gfp_t gfp);
> diff --git a/include/linux/writeback.h b/include/linux/writeback.h
> index b749a9a5a5ee..88e4b5049698 100644
> --- a/include/linux/writeback.h
> +++ b/include/linux/writeback.h
> @@ -209,6 +209,7 @@ int cgroup_writeback_by_id(u64 bdi_id, int memcg_id,
>  			   enum wb_reason reason, struct wb_completion *done);
>  void cgroup_writeback_umount(struct super_block *sb);
>  bool cleanup_offline_cgwb(struct bdi_writeback *wb);
> +bool switch_replaced_cgwb(struct bdi_writeback *wb);
>  
>  /**
>   * inode_attach_wb - associate an inode with its wb
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 85b2c95a920c..e401293c260a 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -637,9 +637,51 @@ static void cgwb_release_workfn(struct work_struct *work)
>  	bdi_put(bdi);
>  	WARN_ON_ONCE(!list_empty(&wb->b_attached));
>  	WARN_ON_ONCE(work_pending(&wb->switch_work));
> +	WARN_ON_ONCE(work_pending(&wb->replaced_work));
>  	call_rcu(&wb->rcu, cgwb_free_rcu);
>  }
>  
> +static void cgwb_replaced_workfn(struct work_struct *work)
> +{
> +	struct bdi_writeback *wb = container_of(work, struct bdi_writeback,
> +						replaced_work);
> +
> +	do {
> +		cond_resched_tasks_rcu_qs();
> +	} while (switch_replaced_cgwb(wb));
> +	wb_put(wb);
> +}
> +
> +/* called with cgwb_lock held, possibly with interrupts disabled */
> +static void cgwb_queue_replaced_work(struct bdi_writeback *wb)
> +{
> +	lockdep_assert_held(&cgwb_lock);
> +
> +	if (wb_tryget(wb) && !queue_work(system_dfl_wq, &wb->replaced_work))
> +		wb_put(wb);
> +}
> +
> +/**
> + * cgwb_kick_replaced - switch inodes away from @wb again if it was replaced
> + * @wb: wb that inodes were just switched to
> + *
> + * An inode switch pins its target wb before the switch lands, and the wb
> + * may be replaced in cgwb_create() and have its replaced_work run in
> + * between.  Kick the work again so that those inodes reach the successor
> + * too.  A dying wb that still owns its slot is left alone: foreign flushes
> + * can reach it, and the work would look up @wb itself.
> + */
> +void cgwb_kick_replaced(struct bdi_writeback *wb)
> +{
> +	if (!wb_dying(wb))
> +		return;
> +
> +	spin_lock_irq(&cgwb_lock);
> +	if (radix_tree_lookup(&wb->bdi->cgwb_tree, wb->memcg_css->id) != wb)
> +		cgwb_queue_replaced_work(wb);
> +	spin_unlock_irq(&cgwb_lock);
> +}
> +
>  static void cgwb_release(struct percpu_ref *refcnt)
>  {
>  	struct bdi_writeback *wb = container_of(refcnt, struct bdi_writeback,
> @@ -724,6 +766,7 @@ static int cgwb_create(struct backing_dev_info *bdi,
>  	INIT_WORK(&wb->switch_work, inode_switch_wbs_work_fn);
>  	init_llist_head(&wb->switch_wbs_ctxs);
>  	INIT_WORK(&wb->release_work, cgwb_release_workfn);
> +	INIT_WORK(&wb->replaced_work, cgwb_replaced_workfn);
>  	set_bit(WB_registered, &wb->state);
>  	bdi_get(bdi);
>  
> @@ -748,6 +791,12 @@ static int cgwb_create(struct backing_dev_info *bdi,
>  			old_wb = radix_tree_deref_slot_protected(slot, &cgwb_lock);
>  			if (wb_dying(old_wb)) {
>  				radix_tree_replace_slot(&bdi->cgwb_tree, slot, wb);
> +				/*
> +				 * @old_wb is out of foreign flushes' reach
> +				 * but may still have inodes attached, dirty
> +				 * or not.  Switch them over to @wb.
> +				 */
> +				cgwb_queue_replaced_work(old_wb);
>  				ret = 0;
>  			} else {
>  				ret = -EEXIST;
> 
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v6 3/3] writeback: switch a replaced cgwb's inodes to its successor

[Tejun Heo]

Hello, Liz.

The following is a Claude-generated review.

On Fri, Oct 02, 2026 at 09:40:48PM +0000, Liz Fong-Jones wrote:
> + * Switch all inodes attached to @wb, dirty or not, to the wb that foreign
> + * flushes now find for @wb's memcg.  If that one is already gone, fall back

The scan only reaches inodes on @wb's lists, so any inode attached to @wb
but on none of them stays behind, not just one from the
__inode_attach_wb() race. A clean inode switched over from the root wb,
which keeps clean inodes off its lists, is one example. Maybe say "all
inodes on @wb's lists" here and broaden that sentence in the description?

> +	spin_lock(&wb->list_lock);
> +	restart = isw_prepare_wbs_switch(new_wb, isw, &wb->b_attached, &nr) ||
> +		  isw_prepare_wbs_switch(new_wb, isw, &wb->b_dirty, &nr) ||
> +		  isw_prepare_wbs_switch(new_wb, isw, &wb->b_io, &nr) ||
> +		  isw_prepare_wbs_switch(new_wb, isw, &wb->b_more_io, &nr) ||
> +		  isw_prepare_wbs_switch(new_wb, isw, &wb->b_dirty_time, &nr);
> +	spin_unlock(&wb->list_lock);

With f6988c90671e ("writeback: bound cleanup_offline_cgwb() rescans by
rotating scanned inodes") on mainline, isw_prepare_wbs_switch() moves
every scanned inode to the newest end of the list, and wb_switch_rwsem
keeps switches from landing during a sync but doesn't cover this scan.
On b_dirty, that can make a sync miss data:

1. A background work on the replaced wb yields to a sync_inodes_sb()
   work and leaves inodes on b_io.

2. The sync work fixes dirtied_before at entry but doesn't queue_io()
   until b_io drains.

3. Meanwhile, the memcg dirties a clean inode, which goes to the head of
   b_dirty.

4. This scan fills an isw from the tail of b_dirty and moves those older
   inodes in front of the new one.

5. Their switches block on wb_switch_rwsem, which the sync holds for
   write.

6. queue_io() stops at the new inode, so the older ones, dirtied before
   the sync started, are still dirty when it returns.

Maybe hold wb_switch_rwsem for read across the scan?

> +/* called with cgwb_lock held, possibly with interrupts disabled */

cgwb_lock is always taken with irqs disabled and lockdep_assert_held()
already says the rest. Maybe drop the comment?

> + * cgwb_kick_replaced - switch inodes away from @wb again if it was replaced

This queues replaced_work rather than switching anything. Maybe "queue
@wb's replaced_work if @wb has been replaced"?

Thanks.

-- 
tejun

Re: [PATCH v6 3/3] writeback: switch a replaced cgwb's inodes to its successor

[Liz Fong-Jones]

Tejun,

Thanks, confirmed. On mainline, with a debug delay holding a sync's
work between its cutoff and queue_io(), v6 left 378 of 400 files empty
after a crash and v7 none. v7 holds wb_switch_rwsem across the scan and
takes your other three points:
https://lore.kernel.org/all/20261015-wb-dying-cgwb-flush-v7-0-9278d9ebaf5d@honeycomb.io/

Since f6988c90671e, mainline's own cleanup_offline_cgwb() rotates
b_dirty_time the same way, so a racing syncfs can drop lazytime
timestamps. Separate fix:
https://lore.kernel.org/all/20261015114307.1041273-1-lizf@honeycomb.io/

Liz