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