Re: [PATCH v2] xfs: reintroduce PF_FSTRANS for transaction reservation recursion protection

[Yafang Shao <laoar.shao@gmail.com>]

On Fri, Jun 26, 2020 at 5:02 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Wed, Jun 24, 2020 at 11:05:42AM -0400, Yafang Shao wrote:
> > PF_FSTRANS which is used to avoid transaction reservation recursion, is
> > dropped since commit 9070733b4efa ("xfs: abstract PF_FSTRANS to
> > PF_MEMALLOC_NOFS") and commit 7dea19f9ee63 ("mm: introduce
> > memalloc_nofs_{save,restore} API") and replaced by PF_MEMALLOC_NOFS which
> > means to avoid filesystem reclaim recursion. That change is subtle.
> > Let's take the exmple of the check of WARN_ON_ONCE(current->flags &
> > PF_MEMALLOC_NOFS)) to explain why this abstraction from PF_FSTRANS to
> > PF_MEMALLOC_NOFS is not proper.
> >
> > Bellow comment is quoted from Dave,
> > > It wasn't for memory allocation recursion protection in XFS - it was for
> > > transaction reservation recursion protection by something trying to flush
> > > data pages while holding a transaction reservation. Doing
> > > this could deadlock the journal because the existing reservation
> > > could prevent the nested reservation for being able to reserve space
> > > in the journal and that is a self-deadlock vector.
> > > IOWs, this check is not protecting against memory reclaim recursion
> > > bugs at all (that's the previous check [1]). This check is
> > > protecting against the filesystem calling writepages directly from a
> > > context where it can self-deadlock.
> > > So what we are seeing here is that the PF_FSTRANS ->
> > > PF_MEMALLOC_NOFS abstraction lost all the actual useful information
> > > about what type of error this check was protecting against.
> >
> > [1]. Bellow check is to avoid memory reclaim recursion.
> > if (WARN_ON_ONCE((current->flags & (PF_MEMALLOC|PF_KSWAPD)) ==
> >       PF_MEMALLOC))
> >       goto redirty;
> >
> > Suggested-by: Dave Chinner <david@fromorbit.com>
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Cc: Michal Hocko <mhocko@kernel.org>
>
> This generally looks sane, but:
>
>  - adds a bunch of overly long lines for no good reason
>  - doesn't really hide this behind a useful informatin, e.g. a
>    xfs_trans_context_start/end helpers for the normal case, plus
>    an extra helper with kswapd in the name for that case.
>

Good point. I will try to think about it.

> The latter should also help to isolate a bit against the mm-area
> changes to the memalloc flags proposed.

I have read the patchset from Matthew.  Agree with you that we should
do it the same way.

[adding  Matthew to cc]

-- 
Thanks
Yafang