* [PATCH v2] xfs: don't hold buffer locks across sync transaction commit in xfs_sync_sb_buf
@ 2026-07-22 13:38 Yun Zhou
2026-07-30 9:17 ` Zhou, Yun
2026-07-30 11:30 ` Christoph Hellwig
0 siblings, 2 replies; 4+ messages in thread
From: Yun Zhou @ 2026-07-22 13:38 UTC (permalink / raw)
To: cem; +Cc: djwong, hch, linux-xfs, linux-kernel
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: syzbot+837bcd54843dd6262f2f@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=837bcd54843dd6262f2f
Signed-off-by: Yun Zhou <yun.zhou@windriver.com>
---
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;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] xfs: don't hold buffer locks across sync transaction commit in xfs_sync_sb_buf
2026-07-22 13:38 [PATCH v2] xfs: don't hold buffer locks across sync transaction commit in xfs_sync_sb_buf Yun Zhou
@ 2026-07-30 9:17 ` Zhou, Yun
2026-07-30 11:30 ` Christoph Hellwig
1 sibling, 0 replies; 4+ messages in thread
From: Zhou, Yun @ 2026-07-30 9:17 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: djwong, linux-xfs, linux-kernel, Carlos Maiolino
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: syzbot+837bcd54843dd6262f2f@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=837bcd54843dd6262f2f
> Signed-off-by: Yun Zhou <yun.zhou@windriver.com>
> ---
> 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;
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] xfs: don't hold buffer locks across sync transaction commit in xfs_sync_sb_buf
2026-07-22 13:38 [PATCH v2] xfs: don't hold buffer locks across sync transaction commit in xfs_sync_sb_buf Yun Zhou
2026-07-30 9:17 ` Zhou, Yun
@ 2026-07-30 11:30 ` Christoph Hellwig
2026-07-30 13:43 ` Zhou, Yun
1 sibling, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2026-07-30 11:30 UTC (permalink / raw)
To: Yun Zhou; +Cc: cem, djwong, hch, linux-xfs, linux-kernel
On Wed, Jul 22, 2026 at 09:38:44PM +0800, 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: syzbot+837bcd54843dd6262f2f@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=837bcd54843dd6262f2f
> Signed-off-by: Yun Zhou <yun.zhou@windriver.com>
> ---
> 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;
> 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.
>
> 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?
Anyway, the fix itself looks good, so:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] xfs: don't hold buffer locks across sync transaction commit in xfs_sync_sb_buf
2026-07-30 11:30 ` Christoph Hellwig
@ 2026-07-30 13:43 ` Zhou, Yun
0 siblings, 0 replies; 4+ messages in thread
From: Zhou, Yun @ 2026-07-30 13:43 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: cem, djwong, linux-xfs, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-07-30 13:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-22 13:38 [PATCH v2] xfs: don't hold buffer locks across sync transaction commit in xfs_sync_sb_buf Yun Zhou
2026-07-30 9:17 ` Zhou, Yun
2026-07-30 11:30 ` Christoph Hellwig
2026-07-30 13:43 ` Zhou, Yun
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox