cgwb_kill() takes an offline memcg's wbs out of bdi->cgwb_tree, but inodes that are still being dirtied stay attached to them. When a replacement container keeps appending to those files, its foreign flushes fail with -ENOENT in wb_get_lookup(), and it sleeps in balance_dirty_pages() for seconds to minutes. 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 a live 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.0s 1.4s Suggested-by: Tejun Heo Assisted-by: Claude:claude-opus-5-5 checkpatch sparse Signed-off-by: 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. Based on vfs-7.4.misc, next to Xin Yin's 168a8c13159c, which fixes the sizing but not the lookup of a removed cgroup's wb. Each helps without the other; this one also applies cleanly to mainline. On mainline (fd179f8a05be), without 168a8c13159c, the test from the changelog gave 52.8s, 73.3s and 5.2s worst lag before this patch and 3.9s, 1.0s and 1.0s after. Trigger: a cgroup dirties files and is removed while they are still dirty; a sibling keeps appending to the same files under a parent memory limit. Symptom: cgroup_writeback_by_id() returns -ENOENT and the sibling stalls in balance_dirty_pages(). Minimal recipe below; the harness that produced the numbers (paced writer, lag per second, MODE=alive control) is at https://gist.github.com/lizthegrey/2209d831930588f63076bdc0ac7b78e2. 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 patch one syncfs lagged at most 1.5s in 3 of 3. 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.0-1.1s with this patch). 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; 1.1-1.2s with this patch); an empty file did not. Harness 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. 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 (160 takeovers, checked with temporary trace_printk()s). W=1 and checkpatch --strict clean, no new sparse warnings. All changes are under CONFIG_CGROUP_WRITEBACK; only arm64 builds with it enabled were done for v2. Not tested: x86 runtime, this patch in production, linux-next. 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). Not covered: when a live memcg's blkcg association changes, the new wb takes over the slot, and a killed wb with dirty inodes is unreachable again, as before this patch. 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. 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 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 --- include/linux/backing-dev-defs.h | 13 +++++- include/linux/backing-dev.h | 2 +- mm/backing-dev.c | 86 ++++++++++++++++++++++++++++++++-------- 3 files changed, 83 insertions(+), 18 deletions(-) diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h index 4f1084937315..035d25f5b81b 100644 --- a/include/linux/backing-dev-defs.h +++ b/include/linux/backing-dev-defs.h @@ -196,7 +196,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 +229,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 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..a74caa5c348a 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -614,6 +614,11 @@ static void cgwb_release_workfn(struct work_struct *work) release_work); struct backing_dev_info *bdi = wb->bdi; + /* a newer wb may have taken the slot, see cgwb_create() */ + spin_lock_irq(&cgwb_lock); + radix_tree_delete_item(&bdi->cgwb_tree, wb->memcg_css->id, wb); + spin_unlock_irq(&cgwb_lock); + mutex_lock(&wb->bdi->cgwb_release_mutex); wb_shutdown(wb); @@ -645,11 +650,23 @@ static void cgwb_release(struct percpu_ref *refcnt) queue_work(cgwb_release_wq, &wb->release_work); } +static bool cgwb_dying(struct bdi_writeback *wb) +{ + lockdep_assert_held(&cgwb_lock); + + return percpu_ref_is_dying(&wb->refcnt); +} + +/* + * A killed wb stays in bdi->cgwb_tree until it is released, 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); @@ -670,6 +687,7 @@ static int cgwb_create(struct backing_dev_info *bdi, struct cgroup_subsys_state *blkcg_css; struct list_head *memcg_cgwb_list, *blkcg_cgwb_list; struct bdi_writeback *wb; + void __rcu **slot; unsigned long flags; int ret = 0; @@ -681,10 +699,10 @@ 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->blkcg_css != blkcg_css) { + if (wb && !cgwb_dying(wb) && wb->blkcg_css != blkcg_css) cgwb_kill(wb); + if (wb && cgwb_dying(wb)) wb = NULL; - } spin_unlock_irqrestore(&cgwb_lock, flags); if (wb) goto out_put; @@ -727,8 +745,20 @@ 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 if (cgwb_dying(radix_tree_deref_slot_protected(slot, + &cgwb_lock))) { + 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); @@ -768,10 +798,29 @@ static int cgwb_create(struct backing_dev_info *bdi, * 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. - * + * The wb may have been killed and its blkcg association may be stale. This + * is what foreign flushes want: they target the wb that owns the dirty + * inodes, which can be a killed one. Use wb_get_create() to get a wb to + * attach inodes to. + */ +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 @@ -782,9 +831,13 @@ static int cgwb_create(struct backing_dev_info *bdi, * both the memcg and blkcg associated with it and verifies the blkcg on * each lookup. On mismatch, the existing wb is discarded and a new one is * created. + * + * 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_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 +851,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 +866,8 @@ 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. */ struct bdi_writeback *wb_get_create(struct backing_dev_info *bdi, struct cgroup_subsys_state *memcg_css, @@ -825,7 +878,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; @@ -859,7 +912,8 @@ static void cgwb_bdi_unregister(struct backing_dev_info *bdi) spin_lock_irq(&cgwb_lock); radix_tree_for_each_slot(slot, &bdi->cgwb_tree, &iter, 0) - cgwb_kill(*slot); + if (!cgwb_dying(*slot)) + cgwb_kill(*slot); spin_unlock_irq(&cgwb_lock); mutex_lock(&bdi->cgwb_release_mutex); --- base-commit: 168a8c13159c6e3f0f08da6f8fa2f633a91ba9fd change-id: 20260926-wb-dying-cgwb-flush-06e260e2abad Best regards, -- Liz Fong-Jones