On Fri 12-09-25 06:15:49, Tejun Heo wrote:
> 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?
It is put:
> > + while (1) {
> > + list = llist_del_all(&new_wb->switch_wbs_ctxs);
> > + /* Nothing to do? */
> > + if (!list) {
> > + wb_put(new_wb);
^^^^ here
There's no other way how to exit the function... But I can put 'break' here
and do wb_put() at the end of the function. That will likely be less
subtle.
Honza
> > + 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
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
