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

[Zhou, Yun]

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