Re: [PATCH] loop: add WQ_MEM_RECLAIM flag to per device workqueue

[Dave Chinner <david@fromorbit.com>]

On Tue, Mar 22, 2022 at 06:52:14AM -1000, Tejun Heo wrote:
> Hello,
> 
> On Tue, Mar 22, 2022 at 09:09:53AM +0900, Tetsuo Handa wrote:
> > > The legacy flushing warning is telling us that those workqueues can be
> > 
> > s/can be/must be/ ?
> 
> Well, one thing that we can but don't want to do is converting all
> create_workqueue() users to alloc_workqueue() with MEM_RECLAIM, which is
> wasteful but won't break anything. We know for sure that the workqueues
> which trigger the legacy warning are participating in memory reclaim and
> thus need MEM_RECLAIM. So, yeah, the warning tells us that they need
> MEM_RECLAIM and should be converted.
> 
> > ? Current /* internal: create*_workqueue() */ tells me nothing.
> 
> It's trying to say that it shouldn't be used outside workqueue proper and
> the warning message is supposed to trigger the conversion. But, yeah, a
> stronger comment can help.
> 
> > My question is: I want to add WQ_MEM_RECLAIM flag to the WQ used by loop module
> > because this WQ is involved upon writeback operation. But unless I add both
> > __WQ_LEGACY | WQ_MEM_RECLAIM flags to the WQ used by loop module, we will hit
> > 
> > 	WARN_ONCE(worker && ((worker->current_pwq->wq->flags &
> > 			      (WQ_MEM_RECLAIM | __WQ_LEGACY)) == WQ_MEM_RECLAIM),
> > 
> > warning because e.g. xfs-sync WQ used by xfs module is not using WQ_MEM_RECLAIM flag.
> > 
> > 	mp->m_sync_workqueue = alloc_workqueue("xfs-sync/%s",
> > 				XFS_WQFLAGS(WQ_FREEZABLE), 0, mp->m_super->s_id);
> > 
> > You are suggesting that the correct approach is to add WQ_MEM_RECLAIM flag to WQs
> > used by filesystems when adding WQ_MEM_RECLAIM flag to the WQ used by loop module
> > introduces possibility of hitting
> > 
> > 	WARN_ONCE(worker && ((worker->current_pwq->wq->flags &
> > 			      (WQ_MEM_RECLAIM | __WQ_LEGACY)) == WQ_MEM_RECLAIM),
> > 
> > warning (instead of either adding __WQ_LEGACY | WQ_MEM_RECLAIM flags to the WQ used
> > by loop module or giving up WQ_MEM_RECLAIM flag for the WQ used by loop module),
> > correct?
> 
> Yeah, you detected multiple issues at the same time. xfs sync is
> participating in memory reclaim

No it isn't. What makes you think it is part of memory reclaim?

The xfs-sync workqueue exists solely to perform async flushes of
dirty data at ENOSPC via sync_inodes_sb() because we can't call
sync_inodes_sb directly in the context that hit ENOSPC due to locks
and transaction contexts held. The paths that need this are
buffered writes and file create (on disk inode allocation), neither
of which are in the the memory reclaim path, either.

So this work has nothing to do with memory reclaim, and as such it's
not tagged with WQ_MEM_RECLAIM.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com