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

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Dec 18, 2020 at 09:15:09AM +1100, Dave Chinner 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*.
> 
> 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.

Er... correct me if I'm wrong, but I thought t_pflags stores the old
state of current->flags from before we call xfs_trans_alloc?  So if we
call into xfs_trans_alloc with current->flags==0 and we commit the
transaction having not rolled, we won't unset the _NOFS state, and exit
back to userspace with _NOFS set.

I think the logic is correct here -- we transfer the old pflags value
from @tp to @ntp, which effectively puts @ntp in charge of restoring the
old pflags value; and then we set tp->t_pflags to current->flags, which
means that @tp will "restore" the current pflags value into pflags, which
is a nop.

--D

> 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