Re: [PATCH] xfs: restore nofs context unconditionally in xfs_trans_roll
From: Zhou, Yun
Date: Mon Jul 13 2026 - 22:16:31 EST
On 7/14/26 07:04, Darrick J. Wong wrote:
CAUTION: This email comes from a non Wind River email account!
Do not click links or open attachments unless you recognize the sender and know the content is safe.
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 <hch@xxxxxx>
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.
BR,
Yun