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@xxxxxxxxxxxxx
