Re: [PATCH] writeback: report a Tasks-RCU quiescent state per cgwb drain pass

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Unbounded iteration under a spinlock in a list filtering loop.
- [High] O(N^2) list traversal and spinlock acquisitions when draining cgwbs.
- [High] Unbounded queueing of work items ignoring WB_FRN_MAX_IN_FLIGHT limit.
--

commit 6a4c1184ed4e555add5aa8d7af747c8f54a2713f
Author: Josef Bacik <josef@toxicpanda.com>

writeback: report a Tasks-RCU quiescent state per cgwb drain pass

This commit addresses an issue where draining a dying cgwb with many attached
inodes can run for minutes without reporting a Tasks-RCU quiescent state,
causing RCU stalls and watchdog panics. It fixes this by replacing a plain
cond_resched() with cond_resched_tasks_rcu_qs() in a loop as long as
cleanup_offline_cgwb() returns true.

Link: https://lore.kernel.org/bpf/9d444098-7c03-4163-af12-bd0a79a51443@paulmck-laptop/

> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index cecbcf9060a65..18e999053bae0 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -910,8 +910,9 @@ static void cleanup_offline_cgwbs_workfn(struct work_struct *work)
>  			continue;

[Severity: High]
This isn't a bug introduced by this patch, but doesn't this continue bypass
the lock-drop at the bottom of the loop?

If a user creates and destroys many cgroups, the offline_cgwbs list can grow
arbitrarily. Iterating over all of them under spin_lock_irq(&cgwb_lock) and
hitting this continue creates an unbounded IRQ-disabled critical section,
which could cause latency spikes and trigger the hard lockup watchdog.

>  
>  		spin_unlock_irq(&cgwb_lock);
> -		while (cleanup_offline_cgwb(wb))
> -			cond_resched();
> +		do {
> +			cond_resched_tasks_rcu_qs();
> +		} while (cleanup_offline_cgwb(wb));

[Severity: High]
This is a pre-existing issue, but does cleanup_offline_cgwb() suffer from an
algorithmic regression that causes the CPU livelocks this patch observes?

When cleanup_offline_cgwb() calls isw_prepare_wbs_switch():

fs/fs-writeback.c:isw_prepare_wbs_switch() {
	...
	list_for_each_entry(inode, list, i_io_list) {
		if (!inode_prepare_wbs_switch(inode, new_wb))
			continue;
	...
}

It processes inodes, sets I_WB_SWITCH, and queues them, but they are not
removed from the list synchronously. They are removed by an async worker
after a slow synchronize_rcu(). Because this worker is slow and we
tightly loop here, the next iteration traverses the same list from the
head, encountering all previously processed inodes again.

For each one, inode_prepare_wbs_switch() does this:

fs/fs-writeback.c:inode_prepare_wbs_switch() {
	spin_lock(&inode->i_lock);
	if (!(inode->i_sb->s_flags & SB_ACTIVE) ||
	    inode_state_read(inode) & (I_WB_SWITCH | I_FREEING | I_WILL_FREE) ||
	    inode_to_wb(inode) == new_wb) {
		spin_unlock(&inode->i_lock);
		goto out_unpin;
	}
	...
}

For N inodes, doesn't this result in an O(N^2) list traversal and O(N^2)
lock acquisitions, wasting CPU time during the drain?


[Severity: High]
This is also a pre-existing issue, but does this loop queue work items
without enforcing concurrency limits?

Inside cleanup_offline_cgwb():

fs/fs-writeback.c:cleanup_offline_cgwb() {
	...
	atomic_inc(&isw_nr_in_flight);
	...
	wb_queue_isw(new_wb, isw);
	...
}

This queues a new isw work item for every 125 inodes without checking if
isw_nr_in_flight exceeds WB_FRN_MAX_IN_FLIGHT (1024). For a large cgroup
with millions of inodes, doesn't this unconditionally queue thousands of
work items concurrently, bypassing the limit and potentially flooding the
workqueue?

>  		spin_lock_irq(&cgwb_lock);
>  
>  		wb_put(wb);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-cgwb-tasks-rcu-qs-v1-1-967a7754771f@toxicpanda.com?part=1