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

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

On Fri, Dec 18, 2020 at 8:07 AM Dave Chinner <david@fromorbit.com> wrote:
>
> 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.
>

Make sense to me.


-- 
Thanks
Yafang