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

Yafang Shao
2020-12-19T00:28:40Z

On Fri, Dec 18, 2020 at 6:15 AM Dave Chinner <david@fromorbit.com> wrote:
>
> On Thu, Dec 17, 2020 at 09:11:56AM +0800, Yafang Shao wrote:
> > The xfs_trans context should be active after it is allocated, and
> > deactive when it is freed.
> > The rolling transaction should be specially considered, because in the
> > case when we clear the old transaction the thread's NOFS state shouldn't
> > be changed, as a result we have to set NOFS in the old transaction's
> > t_pflags in xfs_trans_context_swap().
> >
> > So these helpers are refactored as,
> > - xfs_trans_context_set()
> > Used in xfs_trans_alloc()
> > - xfs_trans_context_clear()
> > Used in xfs_trans_free()
> >
> > And a new helper is instroduced to handle the rolling transaction,
> > - xfs_trans_context_swap()
> > Used in rolling transaction
> >
> > 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
>
> As I said in my last comments, this change of logic is not
> necessary. All we need to do is transfer the NOFS state to the new
> transactions and *remove it from the old one*.
>

Thanks for the explanation, I will change it.

> IOWs, all this patch should do is:
>
> > @@ -119,7 +123,9 @@ xfs_trans_dup(
> >
> > ntp->t_rtx_res = tp->t_rtx_res - tp->t_rtx_res_used;
> > tp->t_rtx_res = tp->t_rtx_res_used;
> > - ntp->t_pflags = tp->t_pflags;
> > +
> > + /* Associate the new transaction with this thread. */
> > + xfs_trans_context_swap(tp, ntp);
> >
> > /* move deferred ops over to the new tp */
> > xfs_defer_move(ntp, tp);
>
> This, and
>
> > diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> > index 44b11c64a15e..12380eaaf7ce 100644
> > --- a/fs/xfs/xfs_trans.h
> > +++ b/fs/xfs/xfs_trans.h
> > @@ -280,4 +280,17 @@ xfs_trans_context_clear(struct xfs_trans *tp)
> > memalloc_nofs_restore(tp->t_pflags);
> > }
> >
> > +static inline void
> > +xfs_trans_context_swap(struct xfs_trans *tp, struct xfs_trans *ntp)
> > +{
>
> introduce this wrapper.
>
> > + ntp->t_pflags = tp->t_pflags;
> > + /*
> > + * For the rolling transaction, we have to 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.
> > + */
> > + tp->t_pflags = current->flags | PF_MEMALLOC_NOFS;
> > +}
>
> But not with this implementation.
>
> Think for a minute, please. All we want to do is avoid clearing
> the nofs state when we call xfs_trans_context_clear(tp) if the state
> has been handed to another transaction.
>
> Your current implementation hands the state to ntp, but *then leaves
> it on tp* as well. So then you hack a PF_MEMALLOC_NOFS flag into
> tp->t_pflags so that it doesn't clear that flag (abusing the masking
> done during clearing). That's just nasty because it relies on
> internal memalloc_nofs_restore() details for correct functionality.
>
> The obvious solution: we've moved the saved process state to a
> different context, so it is no longer needed for the current
> transaction we are about to commit. So How about just clearing the
> saved state from the original transaction when swappingi like so:
>
> static inline void
> xfs_trans_context_swap(struct xfs_trans *tp, struct xfs_trans *ntp)
> {
> ntp->t_pflags = tp->t_pflags;
> tp->t_flags = 0;
> }
>
> And now, when we go to clear the transaction context, we can simply
> do this:
>
> static inline void
> xfs_trans_context_clear(struct xfs_trans *tp)
> {
> if (tp->t_pflags)
> memalloc_nofs_restore(tp->t_pflags);
> }
>
> and the problem is solved. The NOFS state will follow the active
> transaction and not be reset until the entire transaction chain is
> completed.
>
> In the next patch you can go and introduce current->journal_info
> into just the wrapper functions, maintaining the same overall
> logic.
>
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com

--
Thanks
Yafang