Re: [PATCH] writeback: bound cleanup_offline_cgwb() rescans by rotating b_attached

[Jan Kara]

On Thu 10-09-26 23:47:44, Patrick Lu (Anthropic) wrote:
> On Thu, Sep 10, 2026 at 01:24:52PM +0200, Jan Kara wrote:
> > On Wed 09-09-26 18:50:27, Patrick Lu (Anthropic) wrote:
> > > Move every scanned inode to the tail of b_attached, so the next pass
> > > starts where the previous one stopped and the drain becomes linear.
> > > b_attached is unordered and isw_prepare_wbs_switch() is its only
> > > walker, so nobody else sees the reorder. b_dirty_time is ordered by
> > > expiry for move_expired_inodes() and keeps its current scan.
> >
> > OK, but isn't there the very same quadratic behavior problem with
> > b_dirty_time scan which you don't touch (and where your trick cannot work)?
> 
> Yes, the same thing happens there. We never saw it because none of our
> filesystems are mounted with lazytime, so b_dirty_time was always empty
> on the hosts we looked at.
> 
> We realized the rotation works for b_dirty_time too if the walk starts
> from the oldest end instead of the newest. sync takes the whole list no
> matter the order, and move_expired_inodes() picks from the oldest end,
> so walking with list_for_each_entry_safe_reverse() and moving scanned
> inodes to the newest end keeps the oldest unscanned inode right where
> the expiry looks. Prepared inodes leave the list as soon as the switch
> work runs and get a new dirtied_time_when on the new wb anyway
> (9a6ebbdbd412), so the only inodes left out of order are the ones that
> can never switch (DAX), and only on the dying wb.
> 
> I tried it in qemu with 100k lazytime inodes on a dying cgwb. With v1
> the b_dirty_time scan under list_lock still grows from 11 to 115 ms per
> pass across the drain, same as unpatched. Walking both lists from the
> oldest end keeps b_attached and b_dirty_time flat at ~0.6 ms per pass,
> with one loop and no flag. Does that make sense? Something like this,
> which I can send as v2:

Yes, I was also thinking that since we are going to clobber
dirtied_time_when anyway, we can as well do it a bit earlier to ease
iteration.

I can see in your patch you don't touch dirtied_time_when at all which will
make the b_dirtied_time list not ordered by the timestamps. That makes me a
bit uneasy but since we are going to move everything out from that list
eventually I think that is fine.

								Honza


> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index e744f9f9d43f..ea3eb40bf828 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -727,19 +727,34 @@ static bool isw_prepare_wbs_switch(struct bdi_writeback *new_wb,
>  				   struct inode_switch_wbs_context *isw,
>  				   struct list_head *list, int *nr)
>  {
> -	struct inode *inode;
> +	struct inode *inode, *tmp;
> +	LIST_HEAD(scanned);
> +	bool full = false;
> +
> +	/*
> +	 * Walk from the oldest end and move scanned inodes to the newest
> +	 * end, so the next scan resumes at unscanned inodes instead of
> +	 * re-walking an ever-growing run of prepared and skipped ones.
> +	 * For b_dirty_time this keeps the oldest unscanned inode at the
> +	 * end move_expired_inodes() picks from; b_attached is unordered.
> +	 */
> +	list_for_each_entry_safe_reverse(inode, tmp, list, i_io_list) {
> +		list_move(&inode->i_io_list, &scanned);
>  
> -	list_for_each_entry(inode, list, i_io_list) {
>  		if (!inode_prepare_wbs_switch(inode, new_wb))
>  			continue;
>  
>  		isw->inodes[*nr] = inode;
>  		(*nr)++;
>  
> -		if (*nr >= WB_MAX_INODES_PER_ISW - 1)
> -			return true;
> +		if (*nr >= WB_MAX_INODES_PER_ISW - 1) {
> +			full = true;
> +			break;
> +		}
>  	}
> -	return false;
> +	list_splice(&scanned, list);
> +
> +	return full;
>  }
>  
>  /**
> 
> Thanks,
> Patrick
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR