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

[Tejun Heo <tj@kernel.org>]

Hello, Julian.

This is an AI review. The series was built here, and the bdev reference
across a racing last close and disk removal, the slot races, and the
css_free ordering were traced and look correct. Two behavioral notes and
some description and comment fixups.

On Mon, Sep 21, 2026 at 08:06:57PM +0800, Julian Sun wrote:
> Use fixed per-memcg slots. Tracking is best effort: record only dev_t

The slot policy this version adopted, oldest-first replacement, expiry
after dirty_expire_interval, and re-arming when the mapping is dirtied
while a flush is in flight, isn't stated anywhere. Can you add a sentence?

> If allocation fails, retain the existing foreign-wb path.

Andrew found this unclear on v6 and the subject is still implied. Something
like "If the workqueue can't be allocated at boot, bdev inodes keep using
the existing foreign-wb path."?

> This primarily affects filesystems that keep metadata in the bdev page
> cache, such as ext4 and other buffer_head users, rather than the usual

The branch keys on sb_is_blkdev_sb(), so buffered writers of raw block
devices are covered too whenever the bdev inode's wb belongs to another
cgroup. Worth a mention.

> +struct bdev_frn_flush_ctx {
> +	struct work_struct work;
> +	dev_t dev;			/* for bdev inode, device hint */
> +	u64 at;				/* last recorded dirtying time in jiffies */
> +	atomic_t inflight;
> +};

It sits next to memcg_cgwb_frn and is memcg's, so maybe memcg_bdev_frn?
"for bdev inode, device hint" doesn't say what the value is. Following the
sibling's comments, "dev_t of the foreign bdev inode", and @inflight could
use one too.

> + * For non-bdev inodes, when a foreign page - a page whose memcg and
> + * writeback ownerships don't match - is dirtied,
> + * mem_cgroup_track_foreign_dirty() records the inode owning bdi_writeback
> + * on the page owning memcg. When balance_dirty_pages() decides that the
> + * memcg needs to sleep due to high dirty ratio, it calls
>   * mem_cgroup_flush_foreign() which queues writeback on the recorded
>   * foreign bdi_writebacks which haven't expired.  Both the numbers of
>   * recorded bdi_writebacks and concurrent in-flight foreign writebacks are
>   * limited to MEMCG_CGWB_FRN_CNT.

The flush sentence and the slot limit apply to both paths now, so the "For
non-bdev inodes" scoping reads wrong. Maybe keep the original paragraph and
let the new one state only the difference: separate MEMCG_CGWB_FRN_CNT
slots keyed by dev_t, flushing only the bdev mapping, same expiry.

> + * These wb/bdev_inodes records only remember IDs and don't hold any object
> + * references. As being wrong occasionally doesn't matter, updates and
> + * accesses to the records are lockless and racy.

The paragraph above already says the bdev records hold no references, and
"wb/bdev_inodes records" doesn't parse. "Both kinds of records only
remember IDs ..." would do.

> +	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;
> +	}
>
>  	trace_track_foreign_dirty(folio, wb);

With this patch alone, foreign dirtying of a bdev folio no longer hits
track_foreign_dirty at all and patch 3 adds the call back inside the
branch. Can you keep the tracepoint firing on the bdev path here so that
patch 3 only adds the argument? Also, @bdev_inode can't be NULL:
__folio_mark_dirty() checked folio->mapping under the same lock and
folio_account_dirtied() already dereferenced mapping->host.

> +static void bdev_frn_flush_work(struct work_struct *work)
> +{
> +	struct bdev_frn_flush_ctx *ctx =
> +		container_of(work, struct bdev_frn_flush_ctx, work);
> +
> +	bdev_flush_by_dev(ctx->dev);
> +
> +	atomic_set(&ctx->inflight, 0);
> +}

The in-flight window is now just the submission of the bdev mapping, and
mem_cgroup_flush_foreign() runs on every throttle-loop iteration, so a
memcg that keeps dirtying and throttling requeues the flush back to back.
A folio the flush redirties, e.g. a buffer locked by a jbd2 checkpoint
write, re-stamps the slot on its own. The old record stayed in flight for
the whole owner-wb work. With a journal the write cadence of hot metadata
is still bounded by the commit interval, but for nojournal ext4 and other
buffer_head users it's bounded only by the throttle pause, for every
tenant of the filesystem while any tenant throttles. Not measured. The
description covers the target size but not the cadence.

Each pass also goes through wbc_attach_fdatawrite_inode() and
wbc_detach_inode(), so it votes in the bdev inode's wb-switch detection
like the owner-wb flush did, at the higher pass rate. Whether the bdev
inode ends up switching wbs more often is an open question.

> +		if (memcg_bdev_frn_wq && time_after64(ctx->at, now - intv) &&
> +		    atomic_cmpxchg(&ctx->inflight, 0, 1) == 0) {

Records are only created when the workqueue exists and @at stays 0
otherwise, so the wq test can't be false with a live record. Fine as
documentation, but the cover lists it as a fix.

> +		INIT_WORK(&ctx->work, bdev_frn_flush_work);
> +		atomic_set(&ctx->inflight, 0);

memcg is zero-allocated, so the atomic_set() isn't needed.

On patch 1, the kerneldoc's "does not wait for all I/O to complete"
understates it, the flush waits for nothing.

Thanks.

--
tejun