Re: [PATCH v2] xfs: don't hold buffer locks across sync transaction commit in xfs_sync_sb_buf

On 7/30/2026 7:30 PM, Christoph Hellwig wrote:
> On Wed, Jul 22, 2026 at 09:38:44PM +0800, Yun Zhou wrote:
>> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
>> index 47322adb7690..929677ad95b4 100644
>> --- a/fs/xfs/libxfs/xfs_sb.c
>> +++ b/fs/xfs/libxfs/xfs_sb.c
>> @@ -1470,36 +1470,35 @@ xfs_sync_sb_buf(
>>        bool                    update_rtsb)
>>   {
>>        struct xfs_trans        *tp;
>>        int                     error;
>>
>>        error = xfs_trans_alloc(mp, &M_RES(mp)->tr_sb, 0, 0, 0, &tp);
>>        if (error)
>>                return error;
>>
>>        xfs_log_sb(tp);
>> +     if (update_rtsb)
>> +             xfs_log_rtsb(tp, xfs_trans_getsb(tp));
>>        xfs_trans_set_sync(tp);
> 
> No new here, but I don't think the transaction reservation is correct
> here.  If we're writing both the sb and rtsb buffers, we need a log
> reservation for two buffers, not just for one.  Separate patch,
> though.
> 

Good catch. I will leave that for a separate fix later.

>>
>>        error = xfs_trans_commit(tp);
>>        if (error)
>> +             return error;
>> +
>> +     /* Re-acquire and write the sb and rtsb to disk. */
>> +     xfs_buf_lock(mp->m_sb_bp);
>> +     xfs_buf_hold(mp->m_sb_bp);
>> +     error = xfs_bwrite(mp->m_sb_bp);
>> +     xfs_buf_relse(mp->m_sb_bp);
>> +     if (error)
>> +             return error;
>> +
>> +     if (update_rtsb && mp->m_rtsb_bp) {
>> +             xfs_buf_lock(mp->m_rtsb_bp);
>> +             xfs_buf_hold(mp->m_rtsb_bp);
>> +             error = xfs_bwrite(mp->m_rtsb_bp);
>> +             xfs_buf_relse(mp->m_rtsb_bp);
>> +     }
> 
> I don't think we need an extra hold here for both buffers, just
> a lock/unlock, or am I missing something?
> 

You are right. I will change it in v3.

Thanks,
Yun