Hello,
On Fri, Sep 12, 2025 at 12:38:35PM +0200, Jan Kara wrote:
> There can be multiple inode switch works that are trying to switch
> inodes to / from the same wb. This can happen in particular if some
> cgroup exits which owns many (thousands) inodes and we need to switch
> them all. In this case several inode_switch_wbs_work_fn() instances will
> be just spinning on the same wb->list_lock while only one of them makes
> forward progress. This wastes CPU cycles and quickly leads to softlockup
> reports and unusable system.
>
> Instead of running several inode_switch_wbs_work_fn() instances in
> parallel switching to the same wb and contending on wb->list_lock, run
> just one work item per wb and manage a queue of isw items switching to
> this wb.
>
> Signed-off-by: Jan Kara <jack@xxxxxxx>
Generally looks great to me, but
> +void inode_switch_wbs_work_fn(struct work_struct *work)
> +{
> + struct bdi_writeback *new_wb = container_of(work, struct bdi_writeback,
> + switch_work);
> + struct inode_switch_wbs_context *isw, *next_isw;
> + struct llist_node *list;
> +
> + /*
> + * Grab out reference to wb so that it cannot get freed under us
> + * after we process all the isw items.
> + */
> + wb_get(new_wb);
Shouldn't this ref put at the end of the function?
> + while (1) {
> + list = llist_del_all(&new_wb->switch_wbs_ctxs);
> + /* Nothing to do? */
> + if (!list) {
> + wb_put(new_wb);
> + return;
> + }
> + /*
> + * In addition to synchronizing among switchers, I_WB_SWITCH
> + * tells the RCU protected stat update paths to grab the i_page
> + * lock so that stat transfer can synchronize against them.
> + * Let's continue after I_WB_SWITCH is guaranteed to be
> + * visible.
> + */
> + synchronize_rcu();
> +
> + llist_for_each_entry_safe(isw, next_isw, list, list)
> + process_inode_switch_wbs_work(new_wb, isw);
> + }
> +}
Thanks.
--
tejun
