Re: [PATCH v3] xfs: avoid deadlock when trigger memory reclaim in ->writepages

[Matthew Wilcox <willy@infradead.org>]

On Mon, Jun 15, 2020 at 04:53:31PM +0200, Michal Hocko wrote:
> On Mon 15-06-20 16:25:52, Holger Hoffstätte wrote:
> > On 2020-06-15 13:56, Yafang Shao wrote:
> [...]
> > > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> > > index b356118..1ccfbf2 100644
> > > --- a/fs/xfs/xfs_aops.c
> > > +++ b/fs/xfs/xfs_aops.c
> > > @@ -573,9 +573,21 @@ static inline bool xfs_ioend_needs_workqueue(struct iomap_ioend *ioend)
> > >   	struct writeback_control *wbc)
> > >   {
> > >   	struct xfs_writepage_ctx wpc = { };
> > > +	unsigned int nofs_flag;
> > > +	int ret;
> > >   	xfs_iflags_clear(XFS_I(mapping->host), XFS_ITRUNCATED);
> > > -	return iomap_writepages(mapping, wbc, &wpc.ctx, &xfs_writeback_ops);
> > > +
> > > +	/*
> > > +	 * We can allocate memory here while doing writeback on behalf of
> > > +	 * memory reclaim.  To avoid memory allocation deadlocks set the
> > > +	 * task-wide nofs context for the following operations.
> > > +	 */
> > > +	nofs_flag = memalloc_nofs_save();
> > > +	ret = iomap_writepages(mapping, wbc, &wpc.ctx, &xfs_writeback_ops);
> > > +	memalloc_nofs_restore(nofs_flag);
> > > +
> > > +	return ret;
> > >   }
> > >   STATIC int
> > > 
> > 
> > Not sure if I did something wrong, but while the previous version of this patch
> > worked fine, this one gave me (with v2 removed obviously):
> > 
> > [  +0.000004] WARNING: CPU: 0 PID: 2811 at fs/iomap/buffered-io.c:1544 iomap_do_writepage+0x6b4/0x780
> 
> This corresponds to
>         /*
>          * Given that we do not allow direct reclaim to call us, we should
>          * never be called in a recursive filesystem reclaim context.
>          */
>         if (WARN_ON_ONCE(current->flags & PF_MEMALLOC_NOFS))
>                 goto redirty;
> 
> which effectivelly says that memalloc_nofs_save/restore cannot be used
> for that code path. Your stack trace doesn't point to a reclaim path
> which shows that this path is shared and also underlines that this is
> not really an intended use of the api. Please refer to
> Documentation/core-api/gfp_mask-from-fs-io.rst for more details but
> shortly the API should be used at the layer which defines a context
> which shouldn't allow to recurse. E.g. a lock which would be problematic
> in the reclaim recursion path.

I'm concerned that this might not be the correct approach.  Generally we
have the rule that freeing memory should not require allocating memory
to succeed.  Since XFS obviously does need to allocate memory to start
a transaction, this allocation should normally be protected by a mempool
or similar.

XFS is subtle and has its own rules around memory allocation and
writeback, so I don't want to say this is definitely wrong.  I'm just
saying that I have concerns it might not be right.