Re: [PATCH v6 2/3] memcg,writeback: flush foreign bdev mappings separately from owner wbs

[Tejun Heo <tj@kernel.org>]

Hello, Julian.

On Thu, Sep 17, 2026 at 03:01:18PM +0800, Julian Sun wrote:
> Bdev foreign flushes can be triggered frequently. Use a dedicated
> workqueue to avoid interfering with tasks on existing workqueues. If
> allocation fails, retain the existing foreign-wb path.

Work items on an unbound workqueue don't interfere with each other beyond
max_active, so this isn't a reason for a dedicated workqueue. Wanting a
separate max_active pool for up to four long-blocking items per memcg is,
if that's the intent. Can you update the rationale?

> Use fixed per-memcg slots, with a shared lock protecting device records
> and in-flight state. Tracking is best effort: record only dev_t without
> holding device or inode references. Skip closed devices or devices whose
> open_mutex is busy. Device removal and device-number reuse may race with
> lookup.

Andrew asked why dev_t rather than a reference. The reasons you gave in the
thread belong here: the record is written under i_pages with IRQs off,
dropping a bdev reference can sleep, and a memcg that never throttles would
pin the device indefinitely.

It'd also help to say who this affects. Only filesystems that keep metadata
in the bdev page cache, so ext4 and the other buffer_head users but not
xfs, btrfs or f2fs. And that the decision is still blind to how much of the
memcg's dirty memory is actually in the bdev, so one dirty bitmap block
still flushes the whole mapping each time the memcg throttles.

> Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing")

This is new development rather than a fix. Maybe drop the tag? Patch 1
doesn't carry one, so a stable pick of this patch alone wouldn't build
anyway.

> +static void mem_cgroup_track_foreign_bdev(struct mem_cgroup *memcg, dev_t dev)

This lands above the "Foreign dirty flushing" overview, which now says
nothing about the bdev path. Can you move it below the overview and add a
sentence there?

> +	/*
> +	 * Tracking is best effort, so losing hints when all slots are occupied
> +	 * is expected.
> +	 */
> +	for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) {
> +		ctx = &memcg->bdev_frn[i];
> +		if (ctx->inflight)
> +			continue;
> +		if (slot < 0)
> +			slot = i;
> +		if (!ctx->dev) {
> +			slot = i;
> +			break;
> +		}
> +	}

When all four are occupied this always evicts slot 0, and the comment says
the new hint is dropped, which isn't what happens.

Why not follow the cgwb_frn style? A timestamp per slot gives oldest-first
replacement here, expiry in mem_cgroup_flush_foreign() so a record from
long ago doesn't fire a flush when the memcg finally throttles, and
re-arming when the mapping is dirtied while a flush is in flight, which the
worker currently forgets when it clears the slot. It could also drop
frn_lock. An atomic in-flight flag on the work gives the same guarantee
done.cnt does, and a torn read of dev or the stamp only costs a wrong
best-effort flush, which is already the deal.

> +	if (memcg_bdev_frn_wq && bdev_inode &&
> +	    sb_is_blkdev_sb(bdev_inode->i_sb)) {
> +		mem_cgroup_track_foreign_bdev(memcg, bdev_inode->i_rdev);
> +		return;
> +	}

Patch 3 moves the trace call below this. Might as well put it there here.

> +	for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) {
> +		struct bdev_frn_flush_ctx *ctx = &memcg->bdev_frn[i];
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&memcg->frn_lock, flags);
> +		if (!ctx->inflight && ctx->dev) {
> +			ctx->inflight = true;
> +			queue_work(memcg_bdev_frn_wq, &ctx->work);
> +		}
> +		spin_unlock_irqrestore(&memcg->frn_lock, flags);
> +	}

If the lock stays, one acquisition around the loop is enough.

> +#ifdef CONFIG_CGROUP_WRITEBACK
> +	memcg_bdev_frn_wq = alloc_workqueue("memcg_bdev_frn_flusher",
> +					    WQ_UNBOUND | WQ_MEM_RECLAIM, 0);
> +	WARN_ON(!memcg_bdev_frn_wq);
> +#endif

WQ_MEM_RECLAIM is for work items that memory reclaim depends on to make
forward progress. Nothing waits on these flushes. The throttled task just
sleeps and re-evaluates, and the bdi flusher remains the path that's
guaranteed to make progress. Can you drop the flag?

Thanks.

--
tejun