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

Hi Christoph,

I've addressed your feedback in v2 (converted the stub to inline 
function). Could you take a look when you get a chance?

Thanks,
Yun

On 7/22/26 21:38, Yun Zhou wrote:
> xfs_sync_sb_buf() holds sb/rtsb buffer locks across a synchronous
> xfs_trans_commit(), which flushes the CIL push workqueue internally.
> If shutdown occurs during the CIL push, xfs_buf_item_unpin() needs to
> lock these buffers to fail them, causing a deadlock:
> 
>    setlabel:          holds buf lock -> flush_workqueue(xfs-cil)
>    CIL push worker:   xfs_buf_item_unpin -> xfs_buf_lock(same buf)
> 
> Remove the xfs_trans_bhold() calls so that commit releases the buffer
> locks normally.  After the sync commit, re-acquire the buffers via
> mp->m_sb_bp / mp->m_rtsb_bp for the on-disk writeback.
> 
> Fixes: f7664b31975b ("xfs: implement online get/set fs label")
> Reported-by: [email protected]
> Closes: https://syzkaller.appspot.com/bug?extid=837bcd54843dd6262f2f
> Signed-off-by: Yun Zhou <[email protected]>
> ---
> Changes in v2:
> - Remove the bp variable and pass xfs_trans_getsb(tp) directly to
>    xfs_log_rtsb() to fix compilation warnings when CONFIG_XFS_RT=n.
> - Convert xfs_log_rtsb() stub from macro to inline function to avoid
>    the need for (void) casting (Christoph).
> ---
>   fs/xfs/libxfs/xfs_rtgroup.h |  6 +++++-
>   fs/xfs/libxfs/xfs_sb.c      | 39 ++++++++++++++++++-------------------
>   2 files changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_rtgroup.h b/fs/xfs/libxfs/xfs_rtgroup.h
> index c0b9f9f2c413..fca2eb74908c 100644
> --- a/fs/xfs/libxfs/xfs_rtgroup.h
> +++ b/fs/xfs/libxfs/xfs_rtgroup.h
> @@ -359,7 +359,11 @@ static inline int xfs_initialize_rtgroups(struct xfs_mount *mp,
>   # define xfs_rtgroup_unlock(rtg, gf)		((void)0)
>   # define xfs_rtgroup_trans_join(tp, rtg, gf)	((void)0)
>   # define xfs_update_rtsb(bp, sb_bp)	((void)0)
> -# define xfs_log_rtsb(tp, sb_bp)	(NULL)
> +static inline struct xfs_buf *xfs_log_rtsb(struct xfs_trans *tp,
> +		const struct xfs_buf *sb_bp)
> +{
> +	return NULL;
> +}
>   # define xfs_rtgroup_get_geometry(rtg, rgeo)	(-EOPNOTSUPP)
>   #endif /* CONFIG_XFS_RT */
>   
> 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;
> -	struct xfs_buf		*bp;
> -	struct xfs_buf		*rtsb_bp = NULL;
>   	int			error;
>   
>   	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_sb, 0, 0, 0, &tp);
>   	if (error)
>   		return error;
>   
> -	bp = xfs_trans_getsb(tp);
>   	xfs_log_sb(tp);
> -	xfs_trans_bhold(tp, bp);
> -	if (update_rtsb) {
> -		rtsb_bp = xfs_log_rtsb(tp, bp);
> -		if (rtsb_bp)
> -			xfs_trans_bhold(tp, rtsb_bp);
> -	}
> +	if (update_rtsb)
> +		xfs_log_rtsb(tp, xfs_trans_getsb(tp));
>   	xfs_trans_set_sync(tp);
>   	error = xfs_trans_commit(tp);
>   	if (error)
> -		goto out;
> -	/*
> -	 * write out the sb buffer to get the changes to disk
> -	 */
> -	error = xfs_bwrite(bp);
> -	if (!error && rtsb_bp)
> -		error = xfs_bwrite(rtsb_bp);
> -out:
> -	if (rtsb_bp)
> -		xfs_buf_relse(rtsb_bp);
> -	xfs_buf_relse(bp);
> +		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);
> +	}
> +
>   	return error;
>   }
>