Re: [PATCH v12 3/4] xfs: refactor the usage around xfs_trans_context_{set,clear}

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

On Tue, Dec 15, 2020 at 5:08 AM Dave Chinner <david@fromorbit.com> wrote:
>
> On Sun, Dec 13, 2020 at 05:09:02PM +0800, Yafang Shao wrote:
> > On Thu, Dec 10, 2020 at 3:52 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > >
> > > On Wed, Dec 09, 2020 at 09:11:45PM +0800, Yafang Shao wrote:
> > > > The xfs_trans context should be active after it is allocated, and
> > > > deactive when it is freed.
> > > >
> > > > So these two helpers are refactored as,
> > > > - xfs_trans_context_set()
> > > >   Used in xfs_trans_alloc()
> > > > - xfs_trans_context_clear()
> > > >   Used in xfs_trans_free()
> > > >
> > > > This patch is based on Darrick's work to fix the issue in xfs/141 in the
> > > > earlier version. [1]
> > > >
> > > > 1. https://lore.kernel.org/linux-xfs/20201104001649.GN7123@magnolia
> > > >
> > > > Cc: Darrick J. Wong <darrick.wong@oracle.com>
> > > > Cc: Matthew Wilcox (Oracle) <willy@infradead.org>
> > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > Cc: Dave Chinner <david@fromorbit.com>
> > > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > > > ---
> > > >  fs/xfs/xfs_trans.c | 28 +++++++++++++++-------------
> > > >  1 file changed, 15 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> > > > index 11d390f0d3f2..4f4645329bb2 100644
> > > > --- a/fs/xfs/xfs_trans.c
> > > > +++ b/fs/xfs/xfs_trans.c
> > > > @@ -67,6 +67,17 @@ xfs_trans_free(
> > > >       xfs_extent_busy_sort(&tp->t_busy);
> > > >       xfs_extent_busy_clear(tp->t_mountp, &tp->t_busy, false);
> > > >
> > > > +
> > > > +     /* Detach the transaction from this thread. */
> > > > +     ASSERT(current->journal_info != NULL);
> > > > +     /*
> > > > +      * The PF_MEMALLOC_NOFS is bound to the transaction itself instead
> > > > +      * of the reservation, so we need to check if tp is still the
> > > > +      * current transaction before clearing the flag.
> > > > +      */
> > > > +     if (current->journal_info == tp)
> > >
> > > Um, you don't start setting journal_info until the next patch, so this
> > > means that someone who lands on this commit with git bisect will have a
> > > xfs with broken logic.
> > >
> > > Because this is the patch that changes where we set and restore NOFS
> > > context, I think you have to introduce xfs_trans_context_swap here,
> > > and not in the next patch.
> > >
> >
> > Thanks for the review. I will change it in the next version.
> >
> > > I also think the _swap routine has to move the old NOFS state to the
> > > new transaction's t_pflags,
> >
> > Sure
> >
> > > and then set NOFS in the old transaction's
> > > t_pflags so that when we clear the context on the old transaction we
> > > don't actually change the thread's NOFS state.
> > >
> >
> > Both thread's NOFS state and thead's journal_info state can't be
> > changed in that case, right ?
> > So should it better be,
> >
> >     __xfs_trans_commit(tp, regrant)
> >         xfs_trans_free(tp, regrant)
> >             if (!regrant). // don't clear the xfs_trans_context if
> > regrant is true.
> >                 xfs_trans_context_clear()
>
> No. You are trying to make this way more complex than it needs to be.
> The logic in the core XFS code is *already correct* and all we need
> to do is move that logic to wrapper functions, then slightly modify
> the implementation inside the wrapper functions.
>

Thanks for the explanation.

>
> That is, xfs_trans_context_clear() should end up like this:
>

Agreed.

> static inline void
> xfs_trans_context_clear(struct xfs_trans *tp)
> {
>         /*
>          * If xfs_trans_context_swap() handed the NOFS context to a
>          * new transaction we do not clear the context here.
>          */
>         if (current->journal_info != tp)

current->journal_info hasn't been used in patch #3, that will make
patch #3 a little more complex.
We have to do some workaround in patch #3. I will think about it.

>                 return;
>         current->journal_info = NULL;
>         memalloc_nofs_restore(tp->t_pflags);
> }
>
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com



-- 
Thanks
Yafang