Re: [PATCH v2] writeback: bound cleanup_offline_cgwb() rescans by rotating scanned inodes

[Jan Kara]

On Fri 11-09-26 18:49:49, Patrick Lu (Anthropic) wrote:
> cleanup_offline_cgwb() prepares at most WB_MAX_INODES_PER_ISW inodes
> per call and is called again until the dying wb is drained, but every
> call walks wb->b_attached and then wb->b_dirty_time from the same end.
> Inodes already prepared (they stay on the list with I_WB_SWITCH set
> until the switch worker runs) and inodes that cannot be switched
> (I_FREEING, I_WILL_FREE, !SB_ACTIVE, DAX, already on the target wb)
> stay where they are, so each pass rescans a growing run of them under
> wb->list_lock and a full drain is quadratic in the number of inodes on
> the list. With ~17M inodes attached to one dying cgwb we saw this end
> in soft lockups, with CPUs reported stuck for 21-48s.
> 
> Walk both lists from the oldest end and move every scanned inode to
> the newest end, so the next pass starts where the previous one stopped
> and the drain becomes linear. b_attached is unordered, so nobody sees
> the reorder there. b_dirty_time is ordered by dirtied_when, but the
> oldest unscanned inode stays at the end move_expired_inodes() picks
> from, sync takes the whole list regardless of order, and prepared
> inodes leave the list as soon as the switch work runs and get a new
> dirtied_time_when on the new wb anyway, so the only inodes left out of
> order are the ones that can never switch (DAX), and only on the dying
> wb.
> 
> Fixes: c22d70a162d3 ("writeback, cgroup: release dying cgwbs by switching attached inodes")
> Cc: stable@xxxxxxxxxxxxxxx
> Acked-by: Tejun Heo <tj@xxxxxxxxxx>
> Acked-by: Roman Gushchin <roman.gushchin@xxxxxxxxx>
> Assisted-by: LLM
> Signed-off-by: Patrick Lu (Anthropic) <perf.patrick.lu@xxxxxxxxx>

Looks good to me now. Thanks! Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
> Seen in production on a 6.18-based kernel: with ~17M inodes attached
> to one dying cgwb, a node spent 36 minutes in back-to-back
> wb->list_lock holds by the cleanup scanner (~6ms each, ~46% of wall
> time, starving writeback on that wb); with v1 of this patch the same
> workload drains in ~30 seconds. Also seen on stock Amazon Linux 2023
> 6.12.68 as isw workers spinning on the list_lock in
> inode_switch_wbs_work_fn() while cleanup_offline_cgwbs_workfn() runs.
> 
> Tested v2 with a QEMU A/B at 100k inodes on b_attached and 100k
> lazytime inodes on b_dirty_time: the per-pass scan under list_lock is
> flat on both lists where unpatched (and v1 on b_dirty_time) grows
> across the drain, all inodes switch, and on-disk timestamps match after
> sync. v1 was also run patched vs unpatched on production-class hardware
> at ~17M attached inodes.
> 
> Josef Bacik's patch making the drain loop report a Tasks-RCU quiescent
> state [1] fixes BPF/ftrace detach stalls behind the same drain; this
> patch bounds the walk itself. The two are independent.
> 
> [1] https://lore.kernel.org/linux-mm/20260909-cgwb-tasks-rcu-qs-v1-1-967a7754771f@xxxxxxxxxxxxxx/
> ---
> Changes in v2:
> - Rotate b_dirty_time as well, walking both lists from the oldest end
>   so the expiry still sees the oldest unscanned inode first (Jan)
> - Drop the unrelated comment updates
> - Kept acks from Tejun and Roman since the b_attached side did not
>   change
> - Link to v1: https://patch.msgid.link/20260909-wb-cgwb-rotate-v1-1-f2eb994d2a46@xxxxxxxxx
> ---
>  fs/fs-writeback.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> 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;
>  }
>  
>  /**
> 
> ---
> base-commit: e14d4302cbd0de773960bec33c2281508c8d8855
> change-id: 20260909-wb-cgwb-rotate-f17a75facfdc
> 
> Best regards,
> --  
> Patrick Lu (Anthropic) <perf.patrick.lu@xxxxxxxxx>
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR