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

[Dave Chinner <david@fromorbit.com>]

On Thu, Dec 17, 2020 at 03:06:27PM -0800, Darrick J. Wong wrote:
> On Fri, Dec 18, 2020 at 09:15:09AM +1100, Dave Chinner wrote:
> > 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.

Minor thinko.

tp->t_pflags only stores a single bit of information w.r.t
PF_MEMALLOC_NOFS state, not the entire set of current task flags:
whether it should be cleared or not on a call to
memalloc_nofs_restore(). See memalloc_nofs_save():

static inline unsigned int memalloc_nofs_save(void)
{
        unsigned int flags = current->flags & PF_MEMALLOC_NOFS;
        current->flags |= PF_MEMALLOC_NOFS;
        return flags;
}

So, yeah, I got the t_pflags logic the wrong way around here - zero
means clear it, non-zero means don't clear it. IOWs, swap:

	ntp->t_pflags = tp->t_pflags;
	tp->t_flags = -1;

and clear:

	if (!tp->t_flags)
		memalloc_nofs_restore(tp->t_pflags);

This should only be temporary code, anyway. The next patch should
change this state clearing check to use current->journal_info, then
we aren't dependent on process flags state at all.

> 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.

That's pretty much what the current logic guarantees. Once the
wrappers provide this same guarantee, we can greatly simply the the
transaction context state management (i.e. set on alloc, swap on
dup, clear on free). And thread handoffs can just use clear/set
appropriately.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com