Re: [PATCH] xfs: restore nofs context unconditionally in xfs_trans_roll

On Tue, Jul 14, 2026 at 10:55:28AM -0700, Darrick J. Wong wrote:
> [add linux-mm since we're talking about memalloc_nofs_save]

Thanks!

> On Tue, Jul 14, 2026 at 10:15:53AM +0800, Zhou, Yun wrote:
> > On 7/14/26 07:04, Darrick J. Wong wrote:
> > > On Mon, Jul 13, 2026 at 06:28:38AM -0700, Christoph Hellwig wrote:
> > > > On Mon, Jul 13, 2026 at 06:06:38PM +0800, Zhou, Yun wrote:
> > > > > On 7/13/26 17:09, Christoph Hellwig wrote:
> > > > > > On Mon, Jul 13, 2026 at 11:55:05AM +0800, Yun Zhou wrote:
> > > > > > > diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> > > > > > > index 7bfbd9f6f0df..1b36cf12d4e3 100644
> > > > > > > --- a/fs/xfs/xfs_trans.c
> > > > > > > +++ b/fs/xfs/xfs_trans.c
> > > > > > > @@ -1029,6 +1029,15 @@ xfs_trans_roll(
> > > > > > >          * duplicate transaction that gets returned.
> > > > > > >          */
> > > > > > >         error = __xfs_trans_commit(tp, true);
> > > > > > > +
> > > > > > > +     tp = *tpp;
> > > > > > > +     /*
> > > > > > > +      * __xfs_trans_commit cleared the NOFS flag by calling into
> > > > > > > +      * xfs_trans_free.  Set it again here before doing memory
> > > > > > > +      * allocations.
> > > > > > > +      */
> > > > > > > +     xfs_trans_set_context(tp);
> > > > > > 
> > > > > > The tp assignment above now returns the incorrect transaction when
> > > > > > __xfs_trans_commit fails, so you can't do this.
> > > > > > 
> > > > > > Otherwise yes, this call should move up.  I don't really see how
> > > > > > it fixes the syzbot report, though.
> > > > > 
> > > > > Thank you very much for your reply. The tp here is a local variable only
> > > > > used for convenience within the function. The caller always gets the new
> > > > > transaction through *tpp, which was set by xfs_trans_dup() before the commit
> > > > > call. Moving tp = *tpp before the error check doesn't change what the caller
> > > > > sees - *tpp still points to the new (dup'd) transaction regardless.
> > > > 
> > > > Ah, right.  Tis should be fine:
> > > > 
> > > > Reviewed-by: Christoph Hellwig <[email protected]>
> > > 
> > > Why not move tp_pflags to the new transaction in xfs_trans_dup like we
> > > do for the deferred item list:
> > > 
> > >          /* move deferred ops over to the new tp */
> > >          xfs_defer_move(ntp, tp);
> > > 
> > >          ntp->t_pflags = tp->t_pflags;
> > >          tp->t_pflags = 0;
> > 
> > That's what the old xfs_trans_switch_context() did before a1ca658d649a
> > removed it. The problem is that setting tp->t_pflags = 0 means
> > xfs_trans_free() calls memalloc_nofs_restore(0), which relies on that being
> > a  no-op — an mm implementation detail. A fresh memalloc_nofs_save() on the
> > new tp keeps the save/restore pairing correct unconditionally.
> 
> So add a new helper.
> 
> /**
>  * memalloc_flags_take - move an implicit __GFP_MEMALLOC scope from one
>  * tracking structure to another.
>  */
> static inline unsigned int
> memalloc_flags_take(unsigned int *old_flags)
> {
> 	unsigned int ret = *old_flags;
> 
> 	*old_flags = 0;
> 	return ret;
> }
> 
> and then:
> 
> 	/* move deferred ops over to the new tp */
> 	xfs_defer_move(ntp, tp);
> 
> 	ntp->t_pflags = memalloc_flags_take(&tp->t_pflags);

I might stick with the 'move' wording?  ie memalloc_flags_move().

And I think you should bury the call to memalloc_flags_move() inside
xfs_defer_move().  I don't think there's a case where you'd want to move
from one transaction to another without preserving the nofs state, is
there?  Bit hard to tell since there's only one caller of xfs_defer_move()
in the XFS code base.