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

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

BR,
Yun