cleanup transaction allocation

cleanup transaction allocation

[Christoph Hellwig]

Hi all,

this series cleans up the xfs_trans_alloc* and xfs_trans_reserve*
interfaces by keeping different code paths more separate and thus
removing redundant arguments and error returns.

A git tree is also available here:

    git://git.infradead.org/users/hch/xfs.git xfs-trans-cleanups

Gitweb:

    https://git.infradead.org/?p=users/hch/xfs.git;a=shortlog;h=refs/heads/xfs-trans-cleanups

Diffstat:
 libxfs/xfs_refcount.c |    4 -
 scrub/common.c        |    7 +
 scrub/common.h        |    2 
 scrub/dir_repair.c    |    8 --
 scrub/fscounters.c    |    3 
 scrub/metapath.c      |    4 -
 scrub/nlinks.c        |    8 --
 scrub/nlinks_repair.c |    4 -
 scrub/parent_repair.c |   12 ---
 scrub/quotacheck.c    |    4 -
 scrub/repair.c        |   16 ----
 scrub/repair.h        |    4 -
 scrub/rmap_repair.c   |    9 --
 scrub/rtrmap_repair.c |    9 --
 scrub/scrub.c         |    5 -
 xfs_attr_item.c       |    5 -
 xfs_discard.c         |   12 ---
 xfs_fsmap.c           |    4 -
 xfs_icache.c          |    5 -
 xfs_inode.c           |    5 -
 xfs_itable.c          |   18 ----
 xfs_iwalk.c           |   11 --
 xfs_log.c             |    6 -
 xfs_log_priv.h        |    4 -
 xfs_notify_failure.c  |    5 -
 xfs_qm.c              |   10 --
 xfs_rtalloc.c         |   13 ---
 xfs_trans.c           |  196 +++++++++++++++++++++++---------------------------
 xfs_trans.h           |    3 
 xfs_zone_gc.c         |    5 -
 30 files changed, 147 insertions(+), 254 deletions(-)

[PATCH 1/8] xfs: use xfs_trans_reserve_more in xfs_trans_reserve_more_inode

[Christoph Hellwig]

Instead of duplicating the empty transacaction reservation
definition.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_trans.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index b4a07af513ba..8b15bfe68774 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -1163,14 +1163,13 @@ xfs_trans_reserve_more_inode(
 	unsigned int		rblocks,
 	bool			force_quota)
 {
-	struct xfs_trans_res	resv = { };
 	struct xfs_mount	*mp = ip->i_mount;
 	unsigned int		rtx = xfs_extlen_to_rtxlen(mp, rblocks);
 	int			error;
 
 	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
 
-	error = xfs_trans_reserve(tp, &resv, dblocks, rtx);
+	error = xfs_trans_reserve_more(tp, dblocks, rtx);
 	if (error)
 		return error;
 
-- 
2.47.2

Re: [PATCH 1/8] xfs: use xfs_trans_reserve_more in xfs_trans_reserve_more_inode

[Darrick J. Wong]

On Tue, Jul 15, 2025 at 02:25:34PM +0200, Christoph Hellwig wrote:
> Instead of duplicating the empty transacaction reservation
> definition.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Nice cleanup
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_trans.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index b4a07af513ba..8b15bfe68774 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -1163,14 +1163,13 @@ xfs_trans_reserve_more_inode(
>  	unsigned int		rblocks,
>  	bool			force_quota)
>  {
> -	struct xfs_trans_res	resv = { };
>  	struct xfs_mount	*mp = ip->i_mount;
>  	unsigned int		rtx = xfs_extlen_to_rtxlen(mp, rblocks);
>  	int			error;
>  
>  	xfs_assert_ilocked(ip, XFS_ILOCK_EXCL);
>  
> -	error = xfs_trans_reserve(tp, &resv, dblocks, rtx);
> +	error = xfs_trans_reserve_more(tp, dblocks, rtx);
>  	if (error)
>  		return error;
>  
> -- 
> 2.47.2
> 
> 

[PATCH 2/8] xfs: don't use xfs_trans_reserve in xfs_trans_reserve_more

[Christoph Hellwig]

xfs_trans_reserve_more just tries to allocate additional blocks and/or
rtextents and is otherwise unrelated to the transaction reservation
logic.  Open code the block and rtextent reservation in
xfs_trans_reserve_more to prepare for simplifying xfs_trans_reserve.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_trans.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 8b15bfe68774..1fddea8d761a 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -1146,9 +1146,18 @@ xfs_trans_reserve_more(
 	unsigned int		blocks,
 	unsigned int		rtextents)
 {
-	struct xfs_trans_res	resv = { };
-
-	return xfs_trans_reserve(tp, &resv, blocks, rtextents);
+	bool			rsvd = tp->t_flags & XFS_TRANS_RESERVE;
+
+	if (blocks && xfs_dec_fdblocks(tp->t_mountp, blocks, rsvd))
+		return -ENOSPC;
+	if (rtextents && xfs_dec_frextents(tp->t_mountp, rtextents) < 0) {
+		if (blocks)
+			xfs_add_fdblocks(tp->t_mountp, blocks);
+		return -ENOSPC;
+	}
+	tp->t_blk_res += blocks;
+	tp->t_rtx_res += rtextents;
+	return 0;
 }
 
 /*
-- 
2.47.2

Re: [PATCH 2/8] xfs: don't use xfs_trans_reserve in xfs_trans_reserve_more

[Darrick J. Wong]

On Tue, Jul 15, 2025 at 02:25:35PM +0200, Christoph Hellwig wrote:
> xfs_trans_reserve_more just tries to allocate additional blocks and/or
> rtextents and is otherwise unrelated to the transaction reservation
> logic.  Open code the block and rtextent reservation in
> xfs_trans_reserve_more to prepare for simplifying xfs_trans_reserve.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_trans.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 8b15bfe68774..1fddea8d761a 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -1146,9 +1146,18 @@ xfs_trans_reserve_more(
>  	unsigned int		blocks,
>  	unsigned int		rtextents)
>  {
> -	struct xfs_trans_res	resv = { };
> -
> -	return xfs_trans_reserve(tp, &resv, blocks, rtextents);
> +	bool			rsvd = tp->t_flags & XFS_TRANS_RESERVE;
> +
> +	if (blocks && xfs_dec_fdblocks(tp->t_mountp, blocks, rsvd))
> +		return -ENOSPC;
> +	if (rtextents && xfs_dec_frextents(tp->t_mountp, rtextents) < 0) {

xfs_dec_frextents is checked for a negative return value, then why isn't
xfs_dec_fdblocks given the same treatment?  Or, since IIRC both return 0
or -ENOSPC you could omit the "< 0" ?

I like how this eliminates the pointless empty resv object though.

--D

> +		if (blocks)
> +			xfs_add_fdblocks(tp->t_mountp, blocks);
> +		return -ENOSPC;
> +	}
> +	tp->t_blk_res += blocks;
> +	tp->t_rtx_res += rtextents;
> +	return 0;
>  }
>  
>  /*
> -- 
> 2.47.2
> 
> 

Re: [PATCH 2/8] xfs: don't use xfs_trans_reserve in xfs_trans_reserve_more

[Christoph Hellwig]

On Tue, Jul 15, 2025 at 07:49:44AM -0700, Darrick J. Wong wrote:
> > +	if (blocks && xfs_dec_fdblocks(tp->t_mountp, blocks, rsvd))
> > +		return -ENOSPC;
> > +	if (rtextents && xfs_dec_frextents(tp->t_mountp, rtextents) < 0) {
> 
> xfs_dec_frextents is checked for a negative return value, then why isn't
> xfs_dec_fdblocks given the same treatment?  Or, since IIRC both return 0
> or -ENOSPC you could omit the "< 0" ?

No good reason, I'll make sure they use the same scheme.  I don't care
which one, so if you prefer the shorter one I'll go for that.

Re: [PATCH 2/8] xfs: don't use xfs_trans_reserve in xfs_trans_reserve_more

[Darrick J. Wong]

On Tue, Jul 15, 2025 at 05:35:00PM +0200, Christoph Hellwig wrote:
> On Tue, Jul 15, 2025 at 07:49:44AM -0700, Darrick J. Wong wrote:
> > > +	if (blocks && xfs_dec_fdblocks(tp->t_mountp, blocks, rsvd))
> > > +		return -ENOSPC;
> > > +	if (rtextents && xfs_dec_frextents(tp->t_mountp, rtextents) < 0) {
> > 
> > xfs_dec_frextents is checked for a negative return value, then why isn't
> > xfs_dec_fdblocks given the same treatment?  Or, since IIRC both return 0
> > or -ENOSPC you could omit the "< 0" ?
> 
> No good reason, I'll make sure they use the same scheme.  I don't care
> which one, so if you prefer the shorter one I'll go for that.

I have no particular preference either way.

--D

[PATCH 3/8] xfs: decouple xfs_trans_alloc_empty from xfs_trans_alloc

[Christoph Hellwig]

xfs_trans_alloc_empty only shares the very basic transaction structure
allocation and initialization with xfs_trans_alloc.

Split out a new __xfs_trans_alloc helper for that and otherwise decouple
xfs_trans_alloc_empty from xfs_trans_alloc.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_trans.c | 52 +++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 1fddea8d761a..e43f44f62c5f 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -241,6 +241,28 @@ xfs_trans_reserve(
 	return error;
 }
 
+static struct xfs_trans *
+__xfs_trans_alloc(
+	struct xfs_mount	*mp,
+	uint			flags)
+{
+	struct xfs_trans	*tp;
+
+	ASSERT(!(flags & XFS_TRANS_RES_FDBLKS) || xfs_has_lazysbcount(mp));
+
+	tp = kmem_cache_zalloc(xfs_trans_cache, GFP_KERNEL | __GFP_NOFAIL);
+	if (!(flags & XFS_TRANS_NO_WRITECOUNT))
+		sb_start_intwrite(mp->m_super);
+	xfs_trans_set_context(tp);
+	tp->t_flags = flags;
+	tp->t_mountp = mp;
+	INIT_LIST_HEAD(&tp->t_items);
+	INIT_LIST_HEAD(&tp->t_busy);
+	INIT_LIST_HEAD(&tp->t_dfops);
+	tp->t_highest_agno = NULLAGNUMBER;
+	return tp;
+}
+
 int
 xfs_trans_alloc(
 	struct xfs_mount	*mp,
@@ -254,33 +276,16 @@ xfs_trans_alloc(
 	bool			want_retry = true;
 	int			error;
 
+	ASSERT(resp->tr_logres > 0);
+
 	/*
 	 * Allocate the handle before we do our freeze accounting and setting up
 	 * GFP_NOFS allocation context so that we avoid lockdep false positives
 	 * by doing GFP_KERNEL allocations inside sb_start_intwrite().
 	 */
 retry:
-	tp = kmem_cache_zalloc(xfs_trans_cache, GFP_KERNEL | __GFP_NOFAIL);
-	if (!(flags & XFS_TRANS_NO_WRITECOUNT))
-		sb_start_intwrite(mp->m_super);
-	xfs_trans_set_context(tp);
-
-	/*
-	 * Zero-reservation ("empty") transactions can't modify anything, so
-	 * they're allowed to run while we're frozen.
-	 */
-	WARN_ON(resp->tr_logres > 0 &&
-		mp->m_super->s_writers.frozen == SB_FREEZE_COMPLETE);
-	ASSERT(!(flags & XFS_TRANS_RES_FDBLKS) ||
-	       xfs_has_lazysbcount(mp));
-
-	tp->t_flags = flags;
-	tp->t_mountp = mp;
-	INIT_LIST_HEAD(&tp->t_items);
-	INIT_LIST_HEAD(&tp->t_busy);
-	INIT_LIST_HEAD(&tp->t_dfops);
-	tp->t_highest_agno = NULLAGNUMBER;
-
+	WARN_ON(mp->m_super->s_writers.frozen == SB_FREEZE_COMPLETE);
+	tp = __xfs_trans_alloc(mp, flags);
 	error = xfs_trans_reserve(tp, resp, blocks, rtextents);
 	if (error == -ENOSPC && want_retry) {
 		xfs_trans_cancel(tp);
@@ -329,9 +334,8 @@ xfs_trans_alloc_empty(
 	struct xfs_mount		*mp,
 	struct xfs_trans		**tpp)
 {
-	struct xfs_trans_res		resv = {0};
-
-	return xfs_trans_alloc(mp, &resv, 0, 0, XFS_TRANS_NO_WRITECOUNT, tpp);
+	*tpp = __xfs_trans_alloc(mp, XFS_TRANS_NO_WRITECOUNT);
+	return 0;
 }
 
 /*
-- 
2.47.2

Re: [PATCH 3/8] xfs: decouple xfs_trans_alloc_empty from xfs_trans_alloc

[Darrick J. Wong]

On Tue, Jul 15, 2025 at 02:25:36PM +0200, Christoph Hellwig wrote:
> xfs_trans_alloc_empty only shares the very basic transaction structure
> allocation and initialization with xfs_trans_alloc.
> 
> Split out a new __xfs_trans_alloc helper for that and otherwise decouple
> xfs_trans_alloc_empty from xfs_trans_alloc.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

That makes sense, especially because all the _empty callsites become so
much cleaner later on...

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_trans.c | 52 +++++++++++++++++++++++++---------------------
>  1 file changed, 28 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 1fddea8d761a..e43f44f62c5f 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -241,6 +241,28 @@ xfs_trans_reserve(
>  	return error;
>  }
>  
> +static struct xfs_trans *
> +__xfs_trans_alloc(
> +	struct xfs_mount	*mp,
> +	uint			flags)
> +{
> +	struct xfs_trans	*tp;
> +
> +	ASSERT(!(flags & XFS_TRANS_RES_FDBLKS) || xfs_has_lazysbcount(mp));
> +
> +	tp = kmem_cache_zalloc(xfs_trans_cache, GFP_KERNEL | __GFP_NOFAIL);
> +	if (!(flags & XFS_TRANS_NO_WRITECOUNT))
> +		sb_start_intwrite(mp->m_super);
> +	xfs_trans_set_context(tp);
> +	tp->t_flags = flags;
> +	tp->t_mountp = mp;
> +	INIT_LIST_HEAD(&tp->t_items);
> +	INIT_LIST_HEAD(&tp->t_busy);
> +	INIT_LIST_HEAD(&tp->t_dfops);
> +	tp->t_highest_agno = NULLAGNUMBER;
> +	return tp;
> +}
> +
>  int
>  xfs_trans_alloc(
>  	struct xfs_mount	*mp,
> @@ -254,33 +276,16 @@ xfs_trans_alloc(
>  	bool			want_retry = true;
>  	int			error;
>  
> +	ASSERT(resp->tr_logres > 0);
> +
>  	/*
>  	 * Allocate the handle before we do our freeze accounting and setting up
>  	 * GFP_NOFS allocation context so that we avoid lockdep false positives
>  	 * by doing GFP_KERNEL allocations inside sb_start_intwrite().
>  	 */
>  retry:
> -	tp = kmem_cache_zalloc(xfs_trans_cache, GFP_KERNEL | __GFP_NOFAIL);
> -	if (!(flags & XFS_TRANS_NO_WRITECOUNT))
> -		sb_start_intwrite(mp->m_super);
> -	xfs_trans_set_context(tp);
> -
> -	/*
> -	 * Zero-reservation ("empty") transactions can't modify anything, so
> -	 * they're allowed to run while we're frozen.
> -	 */
> -	WARN_ON(resp->tr_logres > 0 &&
> -		mp->m_super->s_writers.frozen == SB_FREEZE_COMPLETE);
> -	ASSERT(!(flags & XFS_TRANS_RES_FDBLKS) ||
> -	       xfs_has_lazysbcount(mp));
> -
> -	tp->t_flags = flags;
> -	tp->t_mountp = mp;
> -	INIT_LIST_HEAD(&tp->t_items);
> -	INIT_LIST_HEAD(&tp->t_busy);
> -	INIT_LIST_HEAD(&tp->t_dfops);
> -	tp->t_highest_agno = NULLAGNUMBER;
> -
> +	WARN_ON(mp->m_super->s_writers.frozen == SB_FREEZE_COMPLETE);
> +	tp = __xfs_trans_alloc(mp, flags);
>  	error = xfs_trans_reserve(tp, resp, blocks, rtextents);
>  	if (error == -ENOSPC && want_retry) {
>  		xfs_trans_cancel(tp);
> @@ -329,9 +334,8 @@ xfs_trans_alloc_empty(
>  	struct xfs_mount		*mp,
>  	struct xfs_trans		**tpp)
>  {
> -	struct xfs_trans_res		resv = {0};
> -
> -	return xfs_trans_alloc(mp, &resv, 0, 0, XFS_TRANS_NO_WRITECOUNT, tpp);
> +	*tpp = __xfs_trans_alloc(mp, XFS_TRANS_NO_WRITECOUNT);
> +	return 0;
>  }
>  
>  /*
> -- 
> 2.47.2
> 
> 

[PATCH 4/8] xfs: don't use xfs_trans_reserve in xfs_trans_roll

[Christoph Hellwig]

xfs_trans_roll uses xfs_trans_reserve to basically just call into
xfs_log_regrant while bypassing the reset of xfs_trans_reserve.

Open code the call to xfs_log_regrant in xfs_trans_roll and simplify
xfs_trans_reserve now that it never regrants and always asks for a log
reservation.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_trans.c | 122 +++++++++++++++++++--------------------------
 1 file changed, 51 insertions(+), 71 deletions(-)

diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index e43f44f62c5f..553128b7f86a 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -134,18 +134,14 @@ xfs_trans_dup(
 }
 
 /*
- * This is called to reserve free disk blocks and log space for the
- * given transaction.  This must be done before allocating any resources
- * within the transaction.
+ * This is called to reserve free disk blocks and log space for the given
+ * transaction before allocating any resources within the transaction.
  *
  * This will return ENOSPC if there are not enough blocks available.
  * It will sleep waiting for available log space.
- * The only valid value for the flags parameter is XFS_RES_LOG_PERM, which
- * is used by long running transactions.  If any one of the reservations
- * fails then they will all be backed out.
  *
- * This does not do quota reservations. That typically is done by the
- * caller afterwards.
+ * This does not do quota reservations. That typically is done by the caller
+ * afterwards.
  */
 static int
 xfs_trans_reserve(
@@ -158,10 +154,12 @@ xfs_trans_reserve(
 	int			error = 0;
 	bool			rsvd = (tp->t_flags & XFS_TRANS_RESERVE) != 0;
 
+	ASSERT(resp->tr_logres > 0);
+
 	/*
-	 * Attempt to reserve the needed disk blocks by decrementing
-	 * the number needed from the number available.  This will
-	 * fail if the count would go below zero.
+	 * Attempt to reserve the needed disk blocks by decrementing the number
+	 * needed from the number available.  This will fail if the count would
+	 * go below zero.
 	 */
 	if (blocks > 0) {
 		error = xfs_dec_fdblocks(mp, blocks, rsvd);
@@ -173,42 +171,20 @@ xfs_trans_reserve(
 	/*
 	 * Reserve the log space needed for this transaction.
 	 */
-	if (resp->tr_logres > 0) {
-		bool	permanent = false;
-
-		ASSERT(tp->t_log_res == 0 ||
-		       tp->t_log_res == resp->tr_logres);
-		ASSERT(tp->t_log_count == 0 ||
-		       tp->t_log_count == resp->tr_logcount);
-
-		if (resp->tr_logflags & XFS_TRANS_PERM_LOG_RES) {
-			tp->t_flags |= XFS_TRANS_PERM_LOG_RES;
-			permanent = true;
-		} else {
-			ASSERT(tp->t_ticket == NULL);
-			ASSERT(!(tp->t_flags & XFS_TRANS_PERM_LOG_RES));
-		}
-
-		if (tp->t_ticket != NULL) {
-			ASSERT(resp->tr_logflags & XFS_TRANS_PERM_LOG_RES);
-			error = xfs_log_regrant(mp, tp->t_ticket);
-		} else {
-			error = xfs_log_reserve(mp, resp->tr_logres,
-						resp->tr_logcount,
-						&tp->t_ticket, permanent);
-		}
-
-		if (error)
-			goto undo_blocks;
+	if (resp->tr_logflags & XFS_TRANS_PERM_LOG_RES)
+		tp->t_flags |= XFS_TRANS_PERM_LOG_RES;
+	error = xfs_log_reserve(mp, resp->tr_logres, resp->tr_logcount,
+			&tp->t_ticket, (tp->t_flags & XFS_TRANS_PERM_LOG_RES));
+	if (error)
+		goto undo_blocks;
 
-		tp->t_log_res = resp->tr_logres;
-		tp->t_log_count = resp->tr_logcount;
-	}
+	tp->t_log_res = resp->tr_logres;
+	tp->t_log_count = resp->tr_logcount;
 
 	/*
-	 * Attempt to reserve the needed realtime extents by decrementing
-	 * the number needed from the number available.  This will
-	 * fail if the count would go below zero.
+	 * Attempt to reserve the needed realtime extents by decrementing the
+	 * number needed from the number available.  This will fail if the
+	 * count would go below zero.
 	 */
 	if (rtextents > 0) {
 		error = xfs_dec_frextents(mp, rtextents);
@@ -221,18 +197,11 @@ xfs_trans_reserve(
 
 	return 0;
 
-	/*
-	 * Error cases jump to one of these labels to undo any
-	 * reservations which have already been performed.
-	 */
 undo_log:
-	if (resp->tr_logres > 0) {
-		xfs_log_ticket_ungrant(mp->m_log, tp->t_ticket);
-		tp->t_ticket = NULL;
-		tp->t_log_res = 0;
-		tp->t_flags &= ~XFS_TRANS_PERM_LOG_RES;
-	}
-
+	xfs_log_ticket_ungrant(mp->m_log, tp->t_ticket);
+	tp->t_ticket = NULL;
+	tp->t_log_res = 0;
+	tp->t_flags &= ~XFS_TRANS_PERM_LOG_RES;
 undo_blocks:
 	if (blocks > 0) {
 		xfs_add_fdblocks(mp, blocks);
@@ -1030,36 +999,41 @@ xfs_trans_cancel(
 }
 
 /*
- * Roll from one trans in the sequence of PERMANENT transactions to
- * the next: permanent transactions are only flushed out when
- * committed with xfs_trans_commit(), but we still want as soon
- * as possible to let chunks of it go to the log. So we commit the
- * chunk we've been working on and get a new transaction to continue.
+ * Roll from one trans in the sequence of PERMANENT transactions to the next:
+ * permanent transactions are only flushed out when committed with
+ * xfs_trans_commit(), but we still want as soon as possible to let chunks of it
+ * go to the log.  So we commit the chunk we've been working on and get a new
+ * transaction to continue.
  */
 int
 xfs_trans_roll(
 	struct xfs_trans	**tpp)
 {
 	struct xfs_trans	*trans = *tpp;
+	struct xfs_mount	*mp = trans->t_mountp;
 	struct xfs_trans_res	tres;
 	int			error;
 
 	trace_xfs_trans_roll(trans, _RET_IP_);
 
+	ASSERT(trans->t_log_res > 0);
+
 	/*
 	 * Copy the critical parameters from one trans to the next.
 	 */
 	tres.tr_logres = trans->t_log_res;
 	tres.tr_logcount = trans->t_log_count;
+	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
 
 	*tpp = xfs_trans_dup(trans);
 
 	/*
 	 * Commit the current transaction.
-	 * If this commit failed, then it'd just unlock those items that
-	 * are not marked ihold. That also means that a filesystem shutdown
-	 * is in progress. The caller takes the responsibility to cancel
-	 * the duplicate transaction that gets returned.
+	 *
+	 * If this commit failed, then it'd just unlock those items that are not
+	 * marked ihold. That also means that a filesystem shutdown is in
+	 * progress.  The caller takes the responsibility to cancel the
+	 * duplicate transaction that gets returned.
 	 */
 	error = __xfs_trans_commit(trans, true);
 	if (error)
@@ -1067,14 +1041,20 @@ xfs_trans_roll(
 
 	/*
 	 * Reserve space in the log for the next transaction.
-	 * This also pushes items in the "AIL", the list of logged items,
-	 * out to disk if they are taking up space at the tail of the log
-	 * that we want to use.  This requires that either nothing be locked
-	 * across this call, or that anything that is locked be logged in
-	 * the prior and the next transactions.
+	 *
+	 * This also pushes items in the AIL out to disk if they are taking up
+	 * space at the tail of the log that we want to use.  This requires that
+	 * either nothing be locked across this call, or that anything that is
+	 * locked be logged in the prior and the next transactions.
 	 */
-	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
-	return xfs_trans_reserve(*tpp, &tres, 0, 0);
+	trans = *tpp;
+	trans->t_flags |= XFS_TRANS_PERM_LOG_RES;
+	error = xfs_log_regrant(mp, trans->t_ticket);
+	if (error)
+		return error;
+	trans->t_log_res = tres.tr_logres;
+	trans->t_log_count = tres.tr_logcount;
+	return 0;
 }
 
 /*
-- 
2.47.2

Re: [PATCH 4/8] xfs: don't use xfs_trans_reserve in xfs_trans_roll

[Darrick J. Wong]

On Tue, Jul 15, 2025 at 02:25:37PM +0200, Christoph Hellwig wrote:
> xfs_trans_roll uses xfs_trans_reserve to basically just call into
> xfs_log_regrant while bypassing the reset of xfs_trans_reserve.
> 
> Open code the call to xfs_log_regrant in xfs_trans_roll and simplify
> xfs_trans_reserve now that it never regrants and always asks for a log
> reservation.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_trans.c | 122 +++++++++++++++++++--------------------------
>  1 file changed, 51 insertions(+), 71 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index e43f44f62c5f..553128b7f86a 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -134,18 +134,14 @@ xfs_trans_dup(
>  }
>  
>  /*
> - * This is called to reserve free disk blocks and log space for the
> - * given transaction.  This must be done before allocating any resources
> - * within the transaction.
> + * This is called to reserve free disk blocks and log space for the given
> + * transaction before allocating any resources within the transaction.
>   *
>   * This will return ENOSPC if there are not enough blocks available.
>   * It will sleep waiting for available log space.
> - * The only valid value for the flags parameter is XFS_RES_LOG_PERM, which
> - * is used by long running transactions.  If any one of the reservations
> - * fails then they will all be backed out.
>   *
> - * This does not do quota reservations. That typically is done by the
> - * caller afterwards.
> + * This does not do quota reservations. That typically is done by the caller
> + * afterwards.
>   */
>  static int
>  xfs_trans_reserve(
> @@ -158,10 +154,12 @@ xfs_trans_reserve(
>  	int			error = 0;
>  	bool			rsvd = (tp->t_flags & XFS_TRANS_RESERVE) != 0;
>  
> +	ASSERT(resp->tr_logres > 0);
> +
>  	/*
> -	 * Attempt to reserve the needed disk blocks by decrementing
> -	 * the number needed from the number available.  This will
> -	 * fail if the count would go below zero.
> +	 * Attempt to reserve the needed disk blocks by decrementing the number
> +	 * needed from the number available.  This will fail if the count would
> +	 * go below zero.
>  	 */
>  	if (blocks > 0) {
>  		error = xfs_dec_fdblocks(mp, blocks, rsvd);
> @@ -173,42 +171,20 @@ xfs_trans_reserve(
>  	/*
>  	 * Reserve the log space needed for this transaction.
>  	 */
> -	if (resp->tr_logres > 0) {
> -		bool	permanent = false;
> -
> -		ASSERT(tp->t_log_res == 0 ||
> -		       tp->t_log_res == resp->tr_logres);
> -		ASSERT(tp->t_log_count == 0 ||
> -		       tp->t_log_count == resp->tr_logcount);
> -
> -		if (resp->tr_logflags & XFS_TRANS_PERM_LOG_RES) {
> -			tp->t_flags |= XFS_TRANS_PERM_LOG_RES;
> -			permanent = true;
> -		} else {
> -			ASSERT(tp->t_ticket == NULL);
> -			ASSERT(!(tp->t_flags & XFS_TRANS_PERM_LOG_RES));
> -		}
> -
> -		if (tp->t_ticket != NULL) {
> -			ASSERT(resp->tr_logflags & XFS_TRANS_PERM_LOG_RES);
> -			error = xfs_log_regrant(mp, tp->t_ticket);
> -		} else {
> -			error = xfs_log_reserve(mp, resp->tr_logres,
> -						resp->tr_logcount,
> -						&tp->t_ticket, permanent);
> -		}
> -
> -		if (error)
> -			goto undo_blocks;
> +	if (resp->tr_logflags & XFS_TRANS_PERM_LOG_RES)
> +		tp->t_flags |= XFS_TRANS_PERM_LOG_RES;
> +	error = xfs_log_reserve(mp, resp->tr_logres, resp->tr_logcount,
> +			&tp->t_ticket, (tp->t_flags & XFS_TRANS_PERM_LOG_RES));
> +	if (error)
> +		goto undo_blocks;
>  
> -		tp->t_log_res = resp->tr_logres;
> -		tp->t_log_count = resp->tr_logcount;
> -	}
> +	tp->t_log_res = resp->tr_logres;
> +	tp->t_log_count = resp->tr_logcount;
>  
>  	/*
> -	 * Attempt to reserve the needed realtime extents by decrementing
> -	 * the number needed from the number available.  This will
> -	 * fail if the count would go below zero.
> +	 * Attempt to reserve the needed realtime extents by decrementing the
> +	 * number needed from the number available.  This will fail if the
> +	 * count would go below zero.
>  	 */
>  	if (rtextents > 0) {
>  		error = xfs_dec_frextents(mp, rtextents);
> @@ -221,18 +197,11 @@ xfs_trans_reserve(
>  
>  	return 0;
>  
> -	/*
> -	 * Error cases jump to one of these labels to undo any
> -	 * reservations which have already been performed.
> -	 */
>  undo_log:
> -	if (resp->tr_logres > 0) {
> -		xfs_log_ticket_ungrant(mp->m_log, tp->t_ticket);
> -		tp->t_ticket = NULL;
> -		tp->t_log_res = 0;
> -		tp->t_flags &= ~XFS_TRANS_PERM_LOG_RES;
> -	}
> -
> +	xfs_log_ticket_ungrant(mp->m_log, tp->t_ticket);
> +	tp->t_ticket = NULL;
> +	tp->t_log_res = 0;
> +	tp->t_flags &= ~XFS_TRANS_PERM_LOG_RES;
>  undo_blocks:
>  	if (blocks > 0) {
>  		xfs_add_fdblocks(mp, blocks);
> @@ -1030,36 +999,41 @@ xfs_trans_cancel(
>  }
>  
>  /*
> - * Roll from one trans in the sequence of PERMANENT transactions to
> - * the next: permanent transactions are only flushed out when
> - * committed with xfs_trans_commit(), but we still want as soon
> - * as possible to let chunks of it go to the log. So we commit the
> - * chunk we've been working on and get a new transaction to continue.
> + * Roll from one trans in the sequence of PERMANENT transactions to the next:
> + * permanent transactions are only flushed out when committed with
> + * xfs_trans_commit(), but we still want as soon as possible to let chunks of it
> + * go to the log.  So we commit the chunk we've been working on and get a new
> + * transaction to continue.
>   */
>  int
>  xfs_trans_roll(
>  	struct xfs_trans	**tpp)
>  {
>  	struct xfs_trans	*trans = *tpp;
> +	struct xfs_mount	*mp = trans->t_mountp;
>  	struct xfs_trans_res	tres;
>  	int			error;
>  
>  	trace_xfs_trans_roll(trans, _RET_IP_);
>  
> +	ASSERT(trans->t_log_res > 0);
> +
>  	/*
>  	 * Copy the critical parameters from one trans to the next.
>  	 */
>  	tres.tr_logres = trans->t_log_res;
>  	tres.tr_logcount = trans->t_log_count;
> +	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;

As a dumb optimization, you could initialize tres at declaration time
instead of down here.

Other than that minor nitpicking, this does make it easier for me to
figure out what xfs_trans_roll does because now I don't have to go
picking through the _reserve logic so on those grounds

Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

>  
>  	*tpp = xfs_trans_dup(trans);
>  
>  	/*
>  	 * Commit the current transaction.
> -	 * If this commit failed, then it'd just unlock those items that
> -	 * are not marked ihold. That also means that a filesystem shutdown
> -	 * is in progress. The caller takes the responsibility to cancel
> -	 * the duplicate transaction that gets returned.
> +	 *
> +	 * If this commit failed, then it'd just unlock those items that are not
> +	 * marked ihold. That also means that a filesystem shutdown is in
> +	 * progress.  The caller takes the responsibility to cancel the
> +	 * duplicate transaction that gets returned.
>  	 */
>  	error = __xfs_trans_commit(trans, true);
>  	if (error)
> @@ -1067,14 +1041,20 @@ xfs_trans_roll(
>  
>  	/*
>  	 * Reserve space in the log for the next transaction.
> -	 * This also pushes items in the "AIL", the list of logged items,
> -	 * out to disk if they are taking up space at the tail of the log
> -	 * that we want to use.  This requires that either nothing be locked
> -	 * across this call, or that anything that is locked be logged in
> -	 * the prior and the next transactions.
> +	 *
> +	 * This also pushes items in the AIL out to disk if they are taking up
> +	 * space at the tail of the log that we want to use.  This requires that
> +	 * either nothing be locked across this call, or that anything that is
> +	 * locked be logged in the prior and the next transactions.
>  	 */
> -	tres.tr_logflags = XFS_TRANS_PERM_LOG_RES;
> -	return xfs_trans_reserve(*tpp, &tres, 0, 0);
> +	trans = *tpp;
> +	trans->t_flags |= XFS_TRANS_PERM_LOG_RES;
> +	error = xfs_log_regrant(mp, trans->t_ticket);
> +	if (error)
> +		return error;
> +	trans->t_log_res = tres.tr_logres;
> +	trans->t_log_count = tres.tr_logcount;
> +	return 0;
>  }
>  
>  /*
> -- 
> 2.47.2
> 
> 

[PATCH 5/8] xfs: return the allocated transaction from xfs_trans_alloc_empty

[Christoph Hellwig]

xfs_trans_alloc_empty can't return errors, so return the allocated
transaction directly instead of an output double pointer argument.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_refcount.c |  4 +---
 fs/xfs/scrub/common.c        |  3 ++-
 fs/xfs/scrub/repair.c        | 12 ++----------
 fs/xfs/scrub/scrub.c         |  5 +----
 fs/xfs/xfs_attr_item.c       |  5 +----
 fs/xfs/xfs_discard.c         | 12 +++---------
 fs/xfs/xfs_fsmap.c           |  4 +---
 fs/xfs/xfs_icache.c          |  5 +----
 fs/xfs/xfs_inode.c           |  5 +----
 fs/xfs/xfs_itable.c          | 18 +++---------------
 fs/xfs/xfs_iwalk.c           | 11 +++--------
 fs/xfs/xfs_notify_failure.c  |  5 +----
 fs/xfs/xfs_qm.c              | 10 ++--------
 fs/xfs/xfs_rtalloc.c         | 13 +++----------
 fs/xfs/xfs_trans.c           |  8 +++-----
 fs/xfs/xfs_trans.h           |  3 +--
 fs/xfs/xfs_zone_gc.c         |  5 +----
 17 files changed, 30 insertions(+), 98 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
index cebe83f7842a..897784037483 100644
--- a/fs/xfs/libxfs/xfs_refcount.c
+++ b/fs/xfs/libxfs/xfs_refcount.c
@@ -2099,9 +2099,7 @@ xfs_refcount_recover_cow_leftovers(
 	 * recording the CoW debris we cancel the (empty) transaction
 	 * and everything goes away cleanly.
 	 */
-	error = xfs_trans_alloc_empty(mp, &tp);
-	if (error)
-		return error;
+	tp = xfs_trans_alloc_empty(mp);
 
 	if (isrt) {
 		xfs_rtgroup_lock(to_rtg(xg), XFS_RTGLOCK_REFCOUNT);
diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
index 28ad341df8ee..d080f4e6e9d8 100644
--- a/fs/xfs/scrub/common.c
+++ b/fs/xfs/scrub/common.c
@@ -870,7 +870,8 @@ int
 xchk_trans_alloc_empty(
 	struct xfs_scrub	*sc)
 {
-	return xfs_trans_alloc_empty(sc->mp, &sc->tp);
+	sc->tp = xfs_trans_alloc_empty(sc->mp);
+	return 0;
 }
 
 /*
diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index f8f9ed30f56b..f7f80ff32afc 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -1279,18 +1279,10 @@ xrep_trans_alloc_hook_dummy(
 	void			**cookiep,
 	struct xfs_trans	**tpp)
 {
-	int			error;
-
 	*cookiep = current->journal_info;
 	current->journal_info = NULL;
-
-	error = xfs_trans_alloc_empty(mp, tpp);
-	if (!error)
-		return 0;
-
-	current->journal_info = *cookiep;
-	*cookiep = NULL;
-	return error;
+	*tpp = xfs_trans_alloc_empty(mp);
+	return 0;
 }
 
 /* Cancel a dummy transaction used by a live update hook function. */
diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
index 76e24032e99a..3c3b0d25006f 100644
--- a/fs/xfs/scrub/scrub.c
+++ b/fs/xfs/scrub/scrub.c
@@ -876,10 +876,7 @@ xchk_scrubv_open_by_handle(
 	struct xfs_inode		*ip;
 	int				error;
 
-	error = xfs_trans_alloc_empty(mp, &tp);
-	if (error)
-		return NULL;
-
+	tp = xfs_trans_alloc_empty(mp);
 	error = xfs_iget(mp, tp, head->svh_ino, XCHK_IGET_FLAGS, 0, &ip);
 	xfs_trans_cancel(tp);
 	if (error)
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index f683b7a9323f..da1e11f38eb0 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -616,10 +616,7 @@ xfs_attri_iread_extents(
 	struct xfs_trans		*tp;
 	int				error;
 
-	error = xfs_trans_alloc_empty(ip->i_mount, &tp);
-	if (error)
-		return error;
-
+	tp = xfs_trans_alloc_empty(ip->i_mount);
 	xfs_ilock(ip, XFS_ILOCK_EXCL);
 	error = xfs_iread_extents(tp, ip, XFS_ATTR_FORK);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index 603d51365645..ee49f20875af 100644
--- a/fs/xfs/xfs_discard.c
+++ b/fs/xfs/xfs_discard.c
@@ -189,9 +189,7 @@ xfs_trim_gather_extents(
 	 */
 	xfs_log_force(mp, XFS_LOG_SYNC);
 
-	error = xfs_trans_alloc_empty(mp, &tp);
-	if (error)
-		return error;
+	tp = xfs_trans_alloc_empty(mp);
 
 	error = xfs_alloc_read_agf(pag, tp, 0, &agbp);
 	if (error)
@@ -583,9 +581,7 @@ xfs_trim_rtextents(
 	struct xfs_trans	*tp;
 	int			error;
 
-	error = xfs_trans_alloc_empty(mp, &tp);
-	if (error)
-		return error;
+	tp = xfs_trans_alloc_empty(mp);
 
 	/*
 	 * Walk the free ranges between low and high.  The query_range function
@@ -701,9 +697,7 @@ xfs_trim_rtgroup_extents(
 	struct xfs_trans	*tp;
 	int			error;
 
-	error = xfs_trans_alloc_empty(mp, &tp);
-	if (error)
-		return error;
+	tp = xfs_trans_alloc_empty(mp);
 
 	/*
 	 * Walk the free ranges between low and high.  The query_range function
diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index 414b27a86458..af68c7de8ee8 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -1270,9 +1270,7 @@ xfs_getfsmap(
 		 * buffer locking abilities to detect cycles in the rmapbt
 		 * without deadlocking.
 		 */
-		error = xfs_trans_alloc_empty(mp, &tp);
-		if (error)
-			break;
+		tp = xfs_trans_alloc_empty(mp);
 
 		info.dev = handlers[i].dev;
 		info.last = false;
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index bbc2f2973dcc..4cf7abe50143 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -893,10 +893,7 @@ xfs_metafile_iget(
 	struct xfs_trans	*tp;
 	int			error;
 
-	error = xfs_trans_alloc_empty(mp, &tp);
-	if (error)
-		return error;
-
+	tp = xfs_trans_alloc_empty(mp);
 	error = xfs_trans_metafile_iget(tp, ino, metafile_type, ipp);
 	xfs_trans_cancel(tp);
 	return error;
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 761a996a857c..4264f8c92e53 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2934,10 +2934,7 @@ xfs_inode_reload_unlinked(
 	struct xfs_trans	*tp;
 	int			error;
 
-	error = xfs_trans_alloc_empty(ip->i_mount, &tp);
-	if (error)
-		return error;
-
+	tp = xfs_trans_alloc_empty(ip->i_mount);
 	xfs_ilock(ip, XFS_ILOCK_SHARED);
 	if (xfs_inode_unlinked_incomplete(ip))
 		error = xfs_inode_reload_unlinked_bucket(tp, ip);
diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index 1fa1c0564b0c..c8c9b8d8309f 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -239,14 +239,10 @@ xfs_bulkstat_one(
 	 * Grab an empty transaction so that we can use its recursive buffer
 	 * locking abilities to detect cycles in the inobt without deadlocking.
 	 */
-	error = xfs_trans_alloc_empty(breq->mp, &tp);
-	if (error)
-		goto out;
-
+	tp = xfs_trans_alloc_empty(breq->mp);
 	error = xfs_bulkstat_one_int(breq->mp, breq->idmap, tp,
 			breq->startino, &bc);
 	xfs_trans_cancel(tp);
-out:
 	kfree(bc.buf);
 
 	/*
@@ -331,17 +327,13 @@ xfs_bulkstat(
 	 * Grab an empty transaction so that we can use its recursive buffer
 	 * locking abilities to detect cycles in the inobt without deadlocking.
 	 */
-	error = xfs_trans_alloc_empty(breq->mp, &tp);
-	if (error)
-		goto out;
-
+	tp = xfs_trans_alloc_empty(breq->mp);
 	if (breq->flags & XFS_IBULK_SAME_AG)
 		iwalk_flags |= XFS_IWALK_SAME_AG;
 
 	error = xfs_iwalk(breq->mp, tp, breq->startino, iwalk_flags,
 			xfs_bulkstat_iwalk, breq->icount, &bc);
 	xfs_trans_cancel(tp);
-out:
 	kfree(bc.buf);
 
 	/*
@@ -464,14 +456,10 @@ xfs_inumbers(
 	 * Grab an empty transaction so that we can use its recursive buffer
 	 * locking abilities to detect cycles in the inobt without deadlocking.
 	 */
-	error = xfs_trans_alloc_empty(breq->mp, &tp);
-	if (error)
-		goto out;
-
+	tp = xfs_trans_alloc_empty(breq->mp);
 	error = xfs_inobt_walk(breq->mp, tp, breq->startino, breq->flags,
 			xfs_inumbers_walk, breq->icount, &ic);
 	xfs_trans_cancel(tp);
-out:
 
 	/*
 	 * We found some inode groups, so clear the error status and return
diff --git a/fs/xfs/xfs_iwalk.c b/fs/xfs/xfs_iwalk.c
index 7db3ece370b1..c1c31d1a8e21 100644
--- a/fs/xfs/xfs_iwalk.c
+++ b/fs/xfs/xfs_iwalk.c
@@ -377,11 +377,8 @@ xfs_iwalk_run_callbacks(
 	if (!has_more)
 		return 0;
 
-	if (iwag->drop_trans) {
-		error = xfs_trans_alloc_empty(mp, &iwag->tp);
-		if (error)
-			return error;
-	}
+	if (iwag->drop_trans)
+		iwag->tp = xfs_trans_alloc_empty(mp);
 
 	/* ...and recreate the cursor just past where we left off. */
 	error = xfs_ialloc_read_agi(iwag->pag, iwag->tp, 0, agi_bpp);
@@ -617,9 +614,7 @@ xfs_iwalk_ag_work(
 	 * Grab an empty transaction so that we can use its recursive buffer
 	 * locking abilities to detect cycles in the inobt without deadlocking.
 	 */
-	error = xfs_trans_alloc_empty(mp, &iwag->tp);
-	if (error)
-		goto out;
+	iwag->tp = xfs_trans_alloc_empty(mp);
 	iwag->drop_trans = 1;
 
 	error = xfs_iwalk_ag(iwag);
diff --git a/fs/xfs/xfs_notify_failure.c b/fs/xfs/xfs_notify_failure.c
index 42e9c72b85c0..fbd521f89874 100644
--- a/fs/xfs/xfs_notify_failure.c
+++ b/fs/xfs/xfs_notify_failure.c
@@ -279,10 +279,7 @@ xfs_dax_notify_dev_failure(
 		kernel_frozen = xfs_dax_notify_failure_freeze(mp) == 0;
 	}
 
-	error = xfs_trans_alloc_empty(mp, &tp);
-	if (error)
-		goto out;
-
+	tp = xfs_trans_alloc_empty(mp);
 	start_gno = xfs_fsb_to_gno(mp, start_bno, type);
 	end_gno = xfs_fsb_to_gno(mp, end_bno, type);
 	while ((xg = xfs_group_next_range(mp, xg, start_gno, end_gno, type))) {
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index fa135ac26471..23ba84ec919a 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -660,10 +660,7 @@ xfs_qm_load_metadir_qinos(
 	struct xfs_trans	*tp;
 	int			error;
 
-	error = xfs_trans_alloc_empty(mp, &tp);
-	if (error)
-		return error;
-
+	tp = xfs_trans_alloc_empty(mp);
 	error = xfs_dqinode_load_parent(tp, &qi->qi_dirip);
 	if (error == -ENOENT) {
 		/* no quota dir directory, but we'll create one later */
@@ -1755,10 +1752,7 @@ xfs_qm_qino_load(
 	struct xfs_inode	*dp = NULL;
 	int			error;
 
-	error = xfs_trans_alloc_empty(mp, &tp);
-	if (error)
-		return error;
-
+	tp = xfs_trans_alloc_empty(mp);
 	if (xfs_has_metadir(mp)) {
 		error = xfs_dqinode_load_parent(tp, &dp);
 		if (error)
diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 736eb0924573..6907e871fa15 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -729,9 +729,7 @@ xfs_rtginode_ensure(
 	if (rtg->rtg_inodes[type])
 		return 0;
 
-	error = xfs_trans_alloc_empty(rtg_mount(rtg), &tp);
-	if (error)
-		return error;
+	tp = xfs_trans_alloc_empty(rtg_mount(rtg));
 	error = xfs_rtginode_load(rtg, type, tp);
 	xfs_trans_cancel(tp);
 
@@ -1305,9 +1303,7 @@ xfs_growfs_rt_prep_groups(
 	if (!mp->m_rtdirip) {
 		struct xfs_trans	*tp;
 
-		error = xfs_trans_alloc_empty(mp, &tp);
-		if (error)
-			return error;
+		tp = xfs_trans_alloc_empty(mp);
 		error = xfs_rtginode_load_parent(tp);
 		xfs_trans_cancel(tp);
 
@@ -1674,10 +1670,7 @@ xfs_rtmount_inodes(
 	struct xfs_rtgroup	*rtg = NULL;
 	int			error;
 
-	error = xfs_trans_alloc_empty(mp, &tp);
-	if (error)
-		return error;
-
+	tp = xfs_trans_alloc_empty(mp);
 	if (xfs_has_rtgroups(mp) && mp->m_sb.sb_rgcount > 0) {
 		error = xfs_rtginode_load_parent(tp);
 		if (error)
diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
index 553128b7f86a..dc7199c81a78 100644
--- a/fs/xfs/xfs_trans.c
+++ b/fs/xfs/xfs_trans.c
@@ -298,13 +298,11 @@ xfs_trans_alloc(
  * where we can be grabbing buffers at the same time that freeze is trying to
  * drain the buffer LRU list.
  */
-int
+struct xfs_trans *
 xfs_trans_alloc_empty(
-	struct xfs_mount		*mp,
-	struct xfs_trans		**tpp)
+	struct xfs_mount		*mp)
 {
-	*tpp = __xfs_trans_alloc(mp, XFS_TRANS_NO_WRITECOUNT);
-	return 0;
+	return __xfs_trans_alloc(mp, XFS_TRANS_NO_WRITECOUNT);
 }
 
 /*
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 2b366851e9a4..a6b10aaeb1f1 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -168,8 +168,7 @@ int		xfs_trans_alloc(struct xfs_mount *mp, struct xfs_trans_res *resp,
 			struct xfs_trans **tpp);
 int		xfs_trans_reserve_more(struct xfs_trans *tp,
 			unsigned int blocks, unsigned int rtextents);
-int		xfs_trans_alloc_empty(struct xfs_mount *mp,
-			struct xfs_trans **tpp);
+struct xfs_trans *xfs_trans_alloc_empty(struct xfs_mount *mp);
 void		xfs_trans_mod_sb(xfs_trans_t *, uint, int64_t);
 
 int xfs_trans_get_buf_map(struct xfs_trans *tp, struct xfs_buftarg *target,
diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
index 9c00fc5baa30..e1954b0e6021 100644
--- a/fs/xfs/xfs_zone_gc.c
+++ b/fs/xfs/xfs_zone_gc.c
@@ -328,10 +328,7 @@ xfs_zone_gc_query(
 	iter->rec_idx = 0;
 	iter->rec_count = 0;
 
-	error = xfs_trans_alloc_empty(mp, &tp);
-	if (error)
-		return error;
-
+	tp = xfs_trans_alloc_empty(mp);
 	xfs_rtgroup_lock(rtg, XFS_RTGLOCK_RMAP);
 	cur = xfs_rtrmapbt_init_cursor(tp, rtg);
 	error = xfs_rmap_query_range(cur, &ri_low, &ri_high,
-- 
2.47.2

Re: [PATCH 5/8] xfs: return the allocated transaction from xfs_trans_alloc_empty

[Darrick J. Wong]

On Tue, Jul 15, 2025 at 02:25:38PM +0200, Christoph Hellwig wrote:
> xfs_trans_alloc_empty can't return errors, so return the allocated
> transaction directly instead of an output double pointer argument.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_refcount.c |  4 +---
>  fs/xfs/scrub/common.c        |  3 ++-
>  fs/xfs/scrub/repair.c        | 12 ++----------
>  fs/xfs/scrub/scrub.c         |  5 +----
>  fs/xfs/xfs_attr_item.c       |  5 +----
>  fs/xfs/xfs_discard.c         | 12 +++---------
>  fs/xfs/xfs_fsmap.c           |  4 +---
>  fs/xfs/xfs_icache.c          |  5 +----
>  fs/xfs/xfs_inode.c           |  5 +----
>  fs/xfs/xfs_itable.c          | 18 +++---------------
>  fs/xfs/xfs_iwalk.c           | 11 +++--------
>  fs/xfs/xfs_notify_failure.c  |  5 +----
>  fs/xfs/xfs_qm.c              | 10 ++--------
>  fs/xfs/xfs_rtalloc.c         | 13 +++----------
>  fs/xfs/xfs_trans.c           |  8 +++-----
>  fs/xfs/xfs_trans.h           |  3 +--
>  fs/xfs/xfs_zone_gc.c         |  5 +----
>  17 files changed, 30 insertions(+), 98 deletions(-)

Yaaaaaay,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> 
> diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> index cebe83f7842a..897784037483 100644
> --- a/fs/xfs/libxfs/xfs_refcount.c
> +++ b/fs/xfs/libxfs/xfs_refcount.c
> @@ -2099,9 +2099,7 @@ xfs_refcount_recover_cow_leftovers(
>  	 * recording the CoW debris we cancel the (empty) transaction
>  	 * and everything goes away cleanly.
>  	 */
> -	error = xfs_trans_alloc_empty(mp, &tp);
> -	if (error)
> -		return error;
> +	tp = xfs_trans_alloc_empty(mp);
>  
>  	if (isrt) {
>  		xfs_rtgroup_lock(to_rtg(xg), XFS_RTGLOCK_REFCOUNT);
> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
> index 28ad341df8ee..d080f4e6e9d8 100644
> --- a/fs/xfs/scrub/common.c
> +++ b/fs/xfs/scrub/common.c
> @@ -870,7 +870,8 @@ int
>  xchk_trans_alloc_empty(
>  	struct xfs_scrub	*sc)
>  {
> -	return xfs_trans_alloc_empty(sc->mp, &sc->tp);
> +	sc->tp = xfs_trans_alloc_empty(sc->mp);
> +	return 0;
>  }
>  
>  /*
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index f8f9ed30f56b..f7f80ff32afc 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -1279,18 +1279,10 @@ xrep_trans_alloc_hook_dummy(
>  	void			**cookiep,
>  	struct xfs_trans	**tpp)
>  {
> -	int			error;
> -
>  	*cookiep = current->journal_info;
>  	current->journal_info = NULL;
> -
> -	error = xfs_trans_alloc_empty(mp, tpp);
> -	if (!error)
> -		return 0;
> -
> -	current->journal_info = *cookiep;
> -	*cookiep = NULL;
> -	return error;
> +	*tpp = xfs_trans_alloc_empty(mp);
> +	return 0;
>  }
>  
>  /* Cancel a dummy transaction used by a live update hook function. */
> diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
> index 76e24032e99a..3c3b0d25006f 100644
> --- a/fs/xfs/scrub/scrub.c
> +++ b/fs/xfs/scrub/scrub.c
> @@ -876,10 +876,7 @@ xchk_scrubv_open_by_handle(
>  	struct xfs_inode		*ip;
>  	int				error;
>  
> -	error = xfs_trans_alloc_empty(mp, &tp);
> -	if (error)
> -		return NULL;
> -
> +	tp = xfs_trans_alloc_empty(mp);
>  	error = xfs_iget(mp, tp, head->svh_ino, XCHK_IGET_FLAGS, 0, &ip);
>  	xfs_trans_cancel(tp);
>  	if (error)
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index f683b7a9323f..da1e11f38eb0 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -616,10 +616,7 @@ xfs_attri_iread_extents(
>  	struct xfs_trans		*tp;
>  	int				error;
>  
> -	error = xfs_trans_alloc_empty(ip->i_mount, &tp);
> -	if (error)
> -		return error;
> -
> +	tp = xfs_trans_alloc_empty(ip->i_mount);
>  	xfs_ilock(ip, XFS_ILOCK_EXCL);
>  	error = xfs_iread_extents(tp, ip, XFS_ATTR_FORK);
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> index 603d51365645..ee49f20875af 100644
> --- a/fs/xfs/xfs_discard.c
> +++ b/fs/xfs/xfs_discard.c
> @@ -189,9 +189,7 @@ xfs_trim_gather_extents(
>  	 */
>  	xfs_log_force(mp, XFS_LOG_SYNC);
>  
> -	error = xfs_trans_alloc_empty(mp, &tp);
> -	if (error)
> -		return error;
> +	tp = xfs_trans_alloc_empty(mp);
>  
>  	error = xfs_alloc_read_agf(pag, tp, 0, &agbp);
>  	if (error)
> @@ -583,9 +581,7 @@ xfs_trim_rtextents(
>  	struct xfs_trans	*tp;
>  	int			error;
>  
> -	error = xfs_trans_alloc_empty(mp, &tp);
> -	if (error)
> -		return error;
> +	tp = xfs_trans_alloc_empty(mp);
>  
>  	/*
>  	 * Walk the free ranges between low and high.  The query_range function
> @@ -701,9 +697,7 @@ xfs_trim_rtgroup_extents(
>  	struct xfs_trans	*tp;
>  	int			error;
>  
> -	error = xfs_trans_alloc_empty(mp, &tp);
> -	if (error)
> -		return error;
> +	tp = xfs_trans_alloc_empty(mp);
>  
>  	/*
>  	 * Walk the free ranges between low and high.  The query_range function
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index 414b27a86458..af68c7de8ee8 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -1270,9 +1270,7 @@ xfs_getfsmap(
>  		 * buffer locking abilities to detect cycles in the rmapbt
>  		 * without deadlocking.
>  		 */
> -		error = xfs_trans_alloc_empty(mp, &tp);
> -		if (error)
> -			break;
> +		tp = xfs_trans_alloc_empty(mp);
>  
>  		info.dev = handlers[i].dev;
>  		info.last = false;
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index bbc2f2973dcc..4cf7abe50143 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -893,10 +893,7 @@ xfs_metafile_iget(
>  	struct xfs_trans	*tp;
>  	int			error;
>  
> -	error = xfs_trans_alloc_empty(mp, &tp);
> -	if (error)
> -		return error;
> -
> +	tp = xfs_trans_alloc_empty(mp);
>  	error = xfs_trans_metafile_iget(tp, ino, metafile_type, ipp);
>  	xfs_trans_cancel(tp);
>  	return error;
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 761a996a857c..4264f8c92e53 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2934,10 +2934,7 @@ xfs_inode_reload_unlinked(
>  	struct xfs_trans	*tp;
>  	int			error;
>  
> -	error = xfs_trans_alloc_empty(ip->i_mount, &tp);
> -	if (error)
> -		return error;
> -
> +	tp = xfs_trans_alloc_empty(ip->i_mount);
>  	xfs_ilock(ip, XFS_ILOCK_SHARED);
>  	if (xfs_inode_unlinked_incomplete(ip))
>  		error = xfs_inode_reload_unlinked_bucket(tp, ip);
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 1fa1c0564b0c..c8c9b8d8309f 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -239,14 +239,10 @@ xfs_bulkstat_one(
>  	 * Grab an empty transaction so that we can use its recursive buffer
>  	 * locking abilities to detect cycles in the inobt without deadlocking.
>  	 */
> -	error = xfs_trans_alloc_empty(breq->mp, &tp);
> -	if (error)
> -		goto out;
> -
> +	tp = xfs_trans_alloc_empty(breq->mp);
>  	error = xfs_bulkstat_one_int(breq->mp, breq->idmap, tp,
>  			breq->startino, &bc);
>  	xfs_trans_cancel(tp);
> -out:
>  	kfree(bc.buf);
>  
>  	/*
> @@ -331,17 +327,13 @@ xfs_bulkstat(
>  	 * Grab an empty transaction so that we can use its recursive buffer
>  	 * locking abilities to detect cycles in the inobt without deadlocking.
>  	 */
> -	error = xfs_trans_alloc_empty(breq->mp, &tp);
> -	if (error)
> -		goto out;
> -
> +	tp = xfs_trans_alloc_empty(breq->mp);
>  	if (breq->flags & XFS_IBULK_SAME_AG)
>  		iwalk_flags |= XFS_IWALK_SAME_AG;
>  
>  	error = xfs_iwalk(breq->mp, tp, breq->startino, iwalk_flags,
>  			xfs_bulkstat_iwalk, breq->icount, &bc);
>  	xfs_trans_cancel(tp);
> -out:
>  	kfree(bc.buf);
>  
>  	/*
> @@ -464,14 +456,10 @@ xfs_inumbers(
>  	 * Grab an empty transaction so that we can use its recursive buffer
>  	 * locking abilities to detect cycles in the inobt without deadlocking.
>  	 */
> -	error = xfs_trans_alloc_empty(breq->mp, &tp);
> -	if (error)
> -		goto out;
> -
> +	tp = xfs_trans_alloc_empty(breq->mp);
>  	error = xfs_inobt_walk(breq->mp, tp, breq->startino, breq->flags,
>  			xfs_inumbers_walk, breq->icount, &ic);
>  	xfs_trans_cancel(tp);
> -out:
>  
>  	/*
>  	 * We found some inode groups, so clear the error status and return
> diff --git a/fs/xfs/xfs_iwalk.c b/fs/xfs/xfs_iwalk.c
> index 7db3ece370b1..c1c31d1a8e21 100644
> --- a/fs/xfs/xfs_iwalk.c
> +++ b/fs/xfs/xfs_iwalk.c
> @@ -377,11 +377,8 @@ xfs_iwalk_run_callbacks(
>  	if (!has_more)
>  		return 0;
>  
> -	if (iwag->drop_trans) {
> -		error = xfs_trans_alloc_empty(mp, &iwag->tp);
> -		if (error)
> -			return error;
> -	}
> +	if (iwag->drop_trans)
> +		iwag->tp = xfs_trans_alloc_empty(mp);
>  
>  	/* ...and recreate the cursor just past where we left off. */
>  	error = xfs_ialloc_read_agi(iwag->pag, iwag->tp, 0, agi_bpp);
> @@ -617,9 +614,7 @@ xfs_iwalk_ag_work(
>  	 * Grab an empty transaction so that we can use its recursive buffer
>  	 * locking abilities to detect cycles in the inobt without deadlocking.
>  	 */
> -	error = xfs_trans_alloc_empty(mp, &iwag->tp);
> -	if (error)
> -		goto out;
> +	iwag->tp = xfs_trans_alloc_empty(mp);
>  	iwag->drop_trans = 1;
>  
>  	error = xfs_iwalk_ag(iwag);
> diff --git a/fs/xfs/xfs_notify_failure.c b/fs/xfs/xfs_notify_failure.c
> index 42e9c72b85c0..fbd521f89874 100644
> --- a/fs/xfs/xfs_notify_failure.c
> +++ b/fs/xfs/xfs_notify_failure.c
> @@ -279,10 +279,7 @@ xfs_dax_notify_dev_failure(
>  		kernel_frozen = xfs_dax_notify_failure_freeze(mp) == 0;
>  	}
>  
> -	error = xfs_trans_alloc_empty(mp, &tp);
> -	if (error)
> -		goto out;
> -
> +	tp = xfs_trans_alloc_empty(mp);
>  	start_gno = xfs_fsb_to_gno(mp, start_bno, type);
>  	end_gno = xfs_fsb_to_gno(mp, end_bno, type);
>  	while ((xg = xfs_group_next_range(mp, xg, start_gno, end_gno, type))) {
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index fa135ac26471..23ba84ec919a 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -660,10 +660,7 @@ xfs_qm_load_metadir_qinos(
>  	struct xfs_trans	*tp;
>  	int			error;
>  
> -	error = xfs_trans_alloc_empty(mp, &tp);
> -	if (error)
> -		return error;
> -
> +	tp = xfs_trans_alloc_empty(mp);
>  	error = xfs_dqinode_load_parent(tp, &qi->qi_dirip);
>  	if (error == -ENOENT) {
>  		/* no quota dir directory, but we'll create one later */
> @@ -1755,10 +1752,7 @@ xfs_qm_qino_load(
>  	struct xfs_inode	*dp = NULL;
>  	int			error;
>  
> -	error = xfs_trans_alloc_empty(mp, &tp);
> -	if (error)
> -		return error;
> -
> +	tp = xfs_trans_alloc_empty(mp);
>  	if (xfs_has_metadir(mp)) {
>  		error = xfs_dqinode_load_parent(tp, &dp);
>  		if (error)
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 736eb0924573..6907e871fa15 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -729,9 +729,7 @@ xfs_rtginode_ensure(
>  	if (rtg->rtg_inodes[type])
>  		return 0;
>  
> -	error = xfs_trans_alloc_empty(rtg_mount(rtg), &tp);
> -	if (error)
> -		return error;
> +	tp = xfs_trans_alloc_empty(rtg_mount(rtg));
>  	error = xfs_rtginode_load(rtg, type, tp);
>  	xfs_trans_cancel(tp);
>  
> @@ -1305,9 +1303,7 @@ xfs_growfs_rt_prep_groups(
>  	if (!mp->m_rtdirip) {
>  		struct xfs_trans	*tp;
>  
> -		error = xfs_trans_alloc_empty(mp, &tp);
> -		if (error)
> -			return error;
> +		tp = xfs_trans_alloc_empty(mp);
>  		error = xfs_rtginode_load_parent(tp);
>  		xfs_trans_cancel(tp);
>  
> @@ -1674,10 +1670,7 @@ xfs_rtmount_inodes(
>  	struct xfs_rtgroup	*rtg = NULL;
>  	int			error;
>  
> -	error = xfs_trans_alloc_empty(mp, &tp);
> -	if (error)
> -		return error;
> -
> +	tp = xfs_trans_alloc_empty(mp);
>  	if (xfs_has_rtgroups(mp) && mp->m_sb.sb_rgcount > 0) {
>  		error = xfs_rtginode_load_parent(tp);
>  		if (error)
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 553128b7f86a..dc7199c81a78 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -298,13 +298,11 @@ xfs_trans_alloc(
>   * where we can be grabbing buffers at the same time that freeze is trying to
>   * drain the buffer LRU list.
>   */
> -int
> +struct xfs_trans *
>  xfs_trans_alloc_empty(
> -	struct xfs_mount		*mp,
> -	struct xfs_trans		**tpp)
> +	struct xfs_mount		*mp)
>  {
> -	*tpp = __xfs_trans_alloc(mp, XFS_TRANS_NO_WRITECOUNT);
> -	return 0;
> +	return __xfs_trans_alloc(mp, XFS_TRANS_NO_WRITECOUNT);
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 2b366851e9a4..a6b10aaeb1f1 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -168,8 +168,7 @@ int		xfs_trans_alloc(struct xfs_mount *mp, struct xfs_trans_res *resp,
>  			struct xfs_trans **tpp);
>  int		xfs_trans_reserve_more(struct xfs_trans *tp,
>  			unsigned int blocks, unsigned int rtextents);
> -int		xfs_trans_alloc_empty(struct xfs_mount *mp,
> -			struct xfs_trans **tpp);
> +struct xfs_trans *xfs_trans_alloc_empty(struct xfs_mount *mp);
>  void		xfs_trans_mod_sb(xfs_trans_t *, uint, int64_t);
>  
>  int xfs_trans_get_buf_map(struct xfs_trans *tp, struct xfs_buftarg *target,
> diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
> index 9c00fc5baa30..e1954b0e6021 100644
> --- a/fs/xfs/xfs_zone_gc.c
> +++ b/fs/xfs/xfs_zone_gc.c
> @@ -328,10 +328,7 @@ xfs_zone_gc_query(
>  	iter->rec_idx = 0;
>  	iter->rec_count = 0;
>  
> -	error = xfs_trans_alloc_empty(mp, &tp);
> -	if (error)
> -		return error;
> -
> +	tp = xfs_trans_alloc_empty(mp);
>  	xfs_rtgroup_lock(rtg, XFS_RTGLOCK_RMAP);
>  	cur = xfs_rtrmapbt_init_cursor(tp, rtg);
>  	error = xfs_rmap_query_range(cur, &ri_low, &ri_high,
> -- 
> 2.47.2
> 
> 

[PATCH 6/8] xfs: return the allocated transaction from xchk_trans_alloc_empty

[Christoph Hellwig]

xchk_trans_alloc_empty can't return errors, so return the allocated
transaction directly instead of an output double pointer argument.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/scrub/common.c        |  6 +++---
 fs/xfs/scrub/common.h        |  2 +-
 fs/xfs/scrub/dir_repair.c    |  8 ++------
 fs/xfs/scrub/fscounters.c    |  3 ++-
 fs/xfs/scrub/metapath.c      |  4 +---
 fs/xfs/scrub/nlinks.c        |  8 ++------
 fs/xfs/scrub/nlinks_repair.c |  4 +---
 fs/xfs/scrub/parent_repair.c | 12 +++---------
 fs/xfs/scrub/quotacheck.c    |  4 +---
 fs/xfs/scrub/rmap_repair.c   |  4 +---
 fs/xfs/scrub/rtrmap_repair.c |  4 +---
 11 files changed, 18 insertions(+), 41 deletions(-)

diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
index d080f4e6e9d8..2ef7742be7d3 100644
--- a/fs/xfs/scrub/common.c
+++ b/fs/xfs/scrub/common.c
@@ -866,12 +866,11 @@ xchk_trans_cancel(
 	sc->tp = NULL;
 }
 
-int
+void
 xchk_trans_alloc_empty(
 	struct xfs_scrub	*sc)
 {
 	sc->tp = xfs_trans_alloc_empty(sc->mp);
-	return 0;
 }
 
 /*
@@ -893,7 +892,8 @@ xchk_trans_alloc(
 		return xfs_trans_alloc(sc->mp, &M_RES(sc->mp)->tr_itruncate,
 				resblks, 0, 0, &sc->tp);
 
-	return xchk_trans_alloc_empty(sc);
+	xchk_trans_alloc_empty(sc);
+	return 0;
 }
 
 /* Set us up with a transaction and an empty context. */
diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h
index 19877d99f255..ddbc065c798c 100644
--- a/fs/xfs/scrub/common.h
+++ b/fs/xfs/scrub/common.h
@@ -7,7 +7,7 @@
 #define __XFS_SCRUB_COMMON_H__
 
 int xchk_trans_alloc(struct xfs_scrub *sc, uint resblks);
-int xchk_trans_alloc_empty(struct xfs_scrub *sc);
+void xchk_trans_alloc_empty(struct xfs_scrub *sc);
 void xchk_trans_cancel(struct xfs_scrub *sc);
 
 bool xchk_process_error(struct xfs_scrub *sc, xfs_agnumber_t agno,
diff --git a/fs/xfs/scrub/dir_repair.c b/fs/xfs/scrub/dir_repair.c
index 249313882108..8d3b550990b5 100644
--- a/fs/xfs/scrub/dir_repair.c
+++ b/fs/xfs/scrub/dir_repair.c
@@ -1289,9 +1289,7 @@ xrep_dir_scan_dirtree(
 	if (sc->ilock_flags & (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL))
 		xchk_iunlock(sc, sc->ilock_flags & (XFS_ILOCK_SHARED |
 						    XFS_ILOCK_EXCL));
-	error = xchk_trans_alloc_empty(sc);
-	if (error)
-		return error;
+	xchk_trans_alloc_empty(sc);
 
 	while ((error = xchk_iscan_iter(&rd->pscan.iscan, &ip)) == 1) {
 		bool		flush;
@@ -1317,9 +1315,7 @@ xrep_dir_scan_dirtree(
 			if (error)
 				break;
 
-			error = xchk_trans_alloc_empty(sc);
-			if (error)
-				break;
+			xchk_trans_alloc_empty(sc);
 		}
 
 		if (xchk_should_terminate(sc, &error))
diff --git a/fs/xfs/scrub/fscounters.c b/fs/xfs/scrub/fscounters.c
index 9b598c5790ad..cebd0d526926 100644
--- a/fs/xfs/scrub/fscounters.c
+++ b/fs/xfs/scrub/fscounters.c
@@ -237,7 +237,8 @@ xchk_setup_fscounters(
 			return error;
 	}
 
-	return xchk_trans_alloc_empty(sc);
+	xchk_trans_alloc_empty(sc);
+	return 0;
 }
 
 /*
diff --git a/fs/xfs/scrub/metapath.c b/fs/xfs/scrub/metapath.c
index e21c16fbd15d..14939d7de349 100644
--- a/fs/xfs/scrub/metapath.c
+++ b/fs/xfs/scrub/metapath.c
@@ -318,9 +318,7 @@ xchk_metapath(
 		return 0;
 	}
 
-	error = xchk_trans_alloc_empty(sc);
-	if (error)
-		return error;
+	xchk_trans_alloc_empty(sc);
 
 	error = xchk_metapath_ilock_both(mpath);
 	if (error)
diff --git a/fs/xfs/scrub/nlinks.c b/fs/xfs/scrub/nlinks.c
index 4a47d0aabf73..26721fab5cab 100644
--- a/fs/xfs/scrub/nlinks.c
+++ b/fs/xfs/scrub/nlinks.c
@@ -555,9 +555,7 @@ xchk_nlinks_collect(
 	 * do not take sb_internal.
 	 */
 	xchk_trans_cancel(sc);
-	error = xchk_trans_alloc_empty(sc);
-	if (error)
-		return error;
+	xchk_trans_alloc_empty(sc);
 
 	while ((error = xchk_iscan_iter(&xnc->collect_iscan, &ip)) == 1) {
 		if (S_ISDIR(VFS_I(ip)->i_mode))
@@ -880,9 +878,7 @@ xchk_nlinks_compare(
 	 * inactivation workqueue.
 	 */
 	xchk_trans_cancel(sc);
-	error = xchk_trans_alloc_empty(sc);
-	if (error)
-		return error;
+	xchk_trans_alloc_empty(sc);
 
 	/*
 	 * Use the inobt to walk all allocated inodes to compare the link
diff --git a/fs/xfs/scrub/nlinks_repair.c b/fs/xfs/scrub/nlinks_repair.c
index 4ebdee095428..6ef2ee9c3814 100644
--- a/fs/xfs/scrub/nlinks_repair.c
+++ b/fs/xfs/scrub/nlinks_repair.c
@@ -340,9 +340,7 @@ xrep_nlinks(
 		 * We can only push the inactivation workqueues with an empty
 		 * transaction.
 		 */
-		error = xchk_trans_alloc_empty(sc);
-		if (error)
-			break;
+		xchk_trans_alloc_empty(sc);
 	}
 	xchk_iscan_iter_finish(&xnc->compare_iscan);
 	xchk_iscan_teardown(&xnc->compare_iscan);
diff --git a/fs/xfs/scrub/parent_repair.c b/fs/xfs/scrub/parent_repair.c
index 31bfe10be22a..2949feda6271 100644
--- a/fs/xfs/scrub/parent_repair.c
+++ b/fs/xfs/scrub/parent_repair.c
@@ -569,9 +569,7 @@ xrep_parent_scan_dirtree(
 	if (sc->ilock_flags & (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL))
 		xchk_iunlock(sc, sc->ilock_flags & (XFS_ILOCK_SHARED |
 						    XFS_ILOCK_EXCL));
-	error = xchk_trans_alloc_empty(sc);
-	if (error)
-		return error;
+	xchk_trans_alloc_empty(sc);
 
 	while ((error = xchk_iscan_iter(&rp->pscan.iscan, &ip)) == 1) {
 		bool		flush;
@@ -597,9 +595,7 @@ xrep_parent_scan_dirtree(
 			if (error)
 				break;
 
-			error = xchk_trans_alloc_empty(sc);
-			if (error)
-				break;
+			xchk_trans_alloc_empty(sc);
 		}
 
 		if (xchk_should_terminate(sc, &error))
@@ -1099,9 +1095,7 @@ xrep_parent_flush_xattrs(
 	xrep_tempfile_iounlock(rp->sc);
 
 	/* Recreate the empty transaction and relock the inode. */
-	error = xchk_trans_alloc_empty(rp->sc);
-	if (error)
-		return error;
+	xchk_trans_alloc_empty(rp->sc);
 	xchk_ilock(rp->sc, XFS_ILOCK_EXCL);
 	return 0;
 }
diff --git a/fs/xfs/scrub/quotacheck.c b/fs/xfs/scrub/quotacheck.c
index dc4033b91e44..e4105aaafe84 100644
--- a/fs/xfs/scrub/quotacheck.c
+++ b/fs/xfs/scrub/quotacheck.c
@@ -505,9 +505,7 @@ xqcheck_collect_counts(
 	 * transactions do not take sb_internal.
 	 */
 	xchk_trans_cancel(sc);
-	error = xchk_trans_alloc_empty(sc);
-	if (error)
-		return error;
+	xchk_trans_alloc_empty(sc);
 
 	while ((error = xchk_iscan_iter(&xqc->iscan, &ip)) == 1) {
 		error = xqcheck_collect_inode(xqc, ip);
diff --git a/fs/xfs/scrub/rmap_repair.c b/fs/xfs/scrub/rmap_repair.c
index f5f73078ffe2..bf1e632b449a 100644
--- a/fs/xfs/scrub/rmap_repair.c
+++ b/fs/xfs/scrub/rmap_repair.c
@@ -951,9 +951,7 @@ xrep_rmap_find_rmaps(
 	sa->agf_bp = NULL;
 	sa->agi_bp = NULL;
 	xchk_trans_cancel(sc);
-	error = xchk_trans_alloc_empty(sc);
-	if (error)
-		return error;
+	xchk_trans_alloc_empty(sc);
 
 	/* Iterate all AGs for inodes rmaps. */
 	while ((error = xchk_iscan_iter(&rr->iscan, &ip)) == 1) {
diff --git a/fs/xfs/scrub/rtrmap_repair.c b/fs/xfs/scrub/rtrmap_repair.c
index fc2592c53af5..4a56726d9952 100644
--- a/fs/xfs/scrub/rtrmap_repair.c
+++ b/fs/xfs/scrub/rtrmap_repair.c
@@ -580,9 +580,7 @@ xrep_rtrmap_find_rmaps(
 	 */
 	xchk_trans_cancel(sc);
 	xchk_rtgroup_unlock(&sc->sr);
-	error = xchk_trans_alloc_empty(sc);
-	if (error)
-		return error;
+	xchk_trans_alloc_empty(sc);
 
 	while ((error = xchk_iscan_iter(&rr->iscan, &ip)) == 1) {
 		error = xrep_rtrmap_scan_inode(rr, ip);
-- 
2.47.2

Re: [PATCH 6/8] xfs: return the allocated transaction from xchk_trans_alloc_empty

[Darrick J. Wong]

On Tue, Jul 15, 2025 at 02:25:39PM +0200, Christoph Hellwig wrote:
> xchk_trans_alloc_empty can't return errors, so return the allocated
> transaction directly instead of an output double pointer argument.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/scrub/common.c        |  6 +++---
>  fs/xfs/scrub/common.h        |  2 +-
>  fs/xfs/scrub/dir_repair.c    |  8 ++------
>  fs/xfs/scrub/fscounters.c    |  3 ++-
>  fs/xfs/scrub/metapath.c      |  4 +---
>  fs/xfs/scrub/nlinks.c        |  8 ++------
>  fs/xfs/scrub/nlinks_repair.c |  4 +---
>  fs/xfs/scrub/parent_repair.c | 12 +++---------
>  fs/xfs/scrub/quotacheck.c    |  4 +---
>  fs/xfs/scrub/rmap_repair.c   |  4 +---
>  fs/xfs/scrub/rtrmap_repair.c |  4 +---
>  11 files changed, 18 insertions(+), 41 deletions(-)

Moar yaaaay,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> 
> diff --git a/fs/xfs/scrub/common.c b/fs/xfs/scrub/common.c
> index d080f4e6e9d8..2ef7742be7d3 100644
> --- a/fs/xfs/scrub/common.c
> +++ b/fs/xfs/scrub/common.c
> @@ -866,12 +866,11 @@ xchk_trans_cancel(
>  	sc->tp = NULL;
>  }
>  
> -int
> +void
>  xchk_trans_alloc_empty(
>  	struct xfs_scrub	*sc)
>  {
>  	sc->tp = xfs_trans_alloc_empty(sc->mp);
> -	return 0;
>  }
>  
>  /*
> @@ -893,7 +892,8 @@ xchk_trans_alloc(
>  		return xfs_trans_alloc(sc->mp, &M_RES(sc->mp)->tr_itruncate,
>  				resblks, 0, 0, &sc->tp);
>  
> -	return xchk_trans_alloc_empty(sc);
> +	xchk_trans_alloc_empty(sc);
> +	return 0;
>  }
>  
>  /* Set us up with a transaction and an empty context. */
> diff --git a/fs/xfs/scrub/common.h b/fs/xfs/scrub/common.h
> index 19877d99f255..ddbc065c798c 100644
> --- a/fs/xfs/scrub/common.h
> +++ b/fs/xfs/scrub/common.h
> @@ -7,7 +7,7 @@
>  #define __XFS_SCRUB_COMMON_H__
>  
>  int xchk_trans_alloc(struct xfs_scrub *sc, uint resblks);
> -int xchk_trans_alloc_empty(struct xfs_scrub *sc);
> +void xchk_trans_alloc_empty(struct xfs_scrub *sc);
>  void xchk_trans_cancel(struct xfs_scrub *sc);
>  
>  bool xchk_process_error(struct xfs_scrub *sc, xfs_agnumber_t agno,
> diff --git a/fs/xfs/scrub/dir_repair.c b/fs/xfs/scrub/dir_repair.c
> index 249313882108..8d3b550990b5 100644
> --- a/fs/xfs/scrub/dir_repair.c
> +++ b/fs/xfs/scrub/dir_repair.c
> @@ -1289,9 +1289,7 @@ xrep_dir_scan_dirtree(
>  	if (sc->ilock_flags & (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL))
>  		xchk_iunlock(sc, sc->ilock_flags & (XFS_ILOCK_SHARED |
>  						    XFS_ILOCK_EXCL));
> -	error = xchk_trans_alloc_empty(sc);
> -	if (error)
> -		return error;
> +	xchk_trans_alloc_empty(sc);
>  
>  	while ((error = xchk_iscan_iter(&rd->pscan.iscan, &ip)) == 1) {
>  		bool		flush;
> @@ -1317,9 +1315,7 @@ xrep_dir_scan_dirtree(
>  			if (error)
>  				break;
>  
> -			error = xchk_trans_alloc_empty(sc);
> -			if (error)
> -				break;
> +			xchk_trans_alloc_empty(sc);
>  		}
>  
>  		if (xchk_should_terminate(sc, &error))
> diff --git a/fs/xfs/scrub/fscounters.c b/fs/xfs/scrub/fscounters.c
> index 9b598c5790ad..cebd0d526926 100644
> --- a/fs/xfs/scrub/fscounters.c
> +++ b/fs/xfs/scrub/fscounters.c
> @@ -237,7 +237,8 @@ xchk_setup_fscounters(
>  			return error;
>  	}
>  
> -	return xchk_trans_alloc_empty(sc);
> +	xchk_trans_alloc_empty(sc);
> +	return 0;
>  }
>  
>  /*
> diff --git a/fs/xfs/scrub/metapath.c b/fs/xfs/scrub/metapath.c
> index e21c16fbd15d..14939d7de349 100644
> --- a/fs/xfs/scrub/metapath.c
> +++ b/fs/xfs/scrub/metapath.c
> @@ -318,9 +318,7 @@ xchk_metapath(
>  		return 0;
>  	}
>  
> -	error = xchk_trans_alloc_empty(sc);
> -	if (error)
> -		return error;
> +	xchk_trans_alloc_empty(sc);
>  
>  	error = xchk_metapath_ilock_both(mpath);
>  	if (error)
> diff --git a/fs/xfs/scrub/nlinks.c b/fs/xfs/scrub/nlinks.c
> index 4a47d0aabf73..26721fab5cab 100644
> --- a/fs/xfs/scrub/nlinks.c
> +++ b/fs/xfs/scrub/nlinks.c
> @@ -555,9 +555,7 @@ xchk_nlinks_collect(
>  	 * do not take sb_internal.
>  	 */
>  	xchk_trans_cancel(sc);
> -	error = xchk_trans_alloc_empty(sc);
> -	if (error)
> -		return error;
> +	xchk_trans_alloc_empty(sc);
>  
>  	while ((error = xchk_iscan_iter(&xnc->collect_iscan, &ip)) == 1) {
>  		if (S_ISDIR(VFS_I(ip)->i_mode))
> @@ -880,9 +878,7 @@ xchk_nlinks_compare(
>  	 * inactivation workqueue.
>  	 */
>  	xchk_trans_cancel(sc);
> -	error = xchk_trans_alloc_empty(sc);
> -	if (error)
> -		return error;
> +	xchk_trans_alloc_empty(sc);
>  
>  	/*
>  	 * Use the inobt to walk all allocated inodes to compare the link
> diff --git a/fs/xfs/scrub/nlinks_repair.c b/fs/xfs/scrub/nlinks_repair.c
> index 4ebdee095428..6ef2ee9c3814 100644
> --- a/fs/xfs/scrub/nlinks_repair.c
> +++ b/fs/xfs/scrub/nlinks_repair.c
> @@ -340,9 +340,7 @@ xrep_nlinks(
>  		 * We can only push the inactivation workqueues with an empty
>  		 * transaction.
>  		 */
> -		error = xchk_trans_alloc_empty(sc);
> -		if (error)
> -			break;
> +		xchk_trans_alloc_empty(sc);
>  	}
>  	xchk_iscan_iter_finish(&xnc->compare_iscan);
>  	xchk_iscan_teardown(&xnc->compare_iscan);
> diff --git a/fs/xfs/scrub/parent_repair.c b/fs/xfs/scrub/parent_repair.c
> index 31bfe10be22a..2949feda6271 100644
> --- a/fs/xfs/scrub/parent_repair.c
> +++ b/fs/xfs/scrub/parent_repair.c
> @@ -569,9 +569,7 @@ xrep_parent_scan_dirtree(
>  	if (sc->ilock_flags & (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL))
>  		xchk_iunlock(sc, sc->ilock_flags & (XFS_ILOCK_SHARED |
>  						    XFS_ILOCK_EXCL));
> -	error = xchk_trans_alloc_empty(sc);
> -	if (error)
> -		return error;
> +	xchk_trans_alloc_empty(sc);
>  
>  	while ((error = xchk_iscan_iter(&rp->pscan.iscan, &ip)) == 1) {
>  		bool		flush;
> @@ -597,9 +595,7 @@ xrep_parent_scan_dirtree(
>  			if (error)
>  				break;
>  
> -			error = xchk_trans_alloc_empty(sc);
> -			if (error)
> -				break;
> +			xchk_trans_alloc_empty(sc);
>  		}
>  
>  		if (xchk_should_terminate(sc, &error))
> @@ -1099,9 +1095,7 @@ xrep_parent_flush_xattrs(
>  	xrep_tempfile_iounlock(rp->sc);
>  
>  	/* Recreate the empty transaction and relock the inode. */
> -	error = xchk_trans_alloc_empty(rp->sc);
> -	if (error)
> -		return error;
> +	xchk_trans_alloc_empty(rp->sc);
>  	xchk_ilock(rp->sc, XFS_ILOCK_EXCL);
>  	return 0;
>  }
> diff --git a/fs/xfs/scrub/quotacheck.c b/fs/xfs/scrub/quotacheck.c
> index dc4033b91e44..e4105aaafe84 100644
> --- a/fs/xfs/scrub/quotacheck.c
> +++ b/fs/xfs/scrub/quotacheck.c
> @@ -505,9 +505,7 @@ xqcheck_collect_counts(
>  	 * transactions do not take sb_internal.
>  	 */
>  	xchk_trans_cancel(sc);
> -	error = xchk_trans_alloc_empty(sc);
> -	if (error)
> -		return error;
> +	xchk_trans_alloc_empty(sc);
>  
>  	while ((error = xchk_iscan_iter(&xqc->iscan, &ip)) == 1) {
>  		error = xqcheck_collect_inode(xqc, ip);
> diff --git a/fs/xfs/scrub/rmap_repair.c b/fs/xfs/scrub/rmap_repair.c
> index f5f73078ffe2..bf1e632b449a 100644
> --- a/fs/xfs/scrub/rmap_repair.c
> +++ b/fs/xfs/scrub/rmap_repair.c
> @@ -951,9 +951,7 @@ xrep_rmap_find_rmaps(
>  	sa->agf_bp = NULL;
>  	sa->agi_bp = NULL;
>  	xchk_trans_cancel(sc);
> -	error = xchk_trans_alloc_empty(sc);
> -	if (error)
> -		return error;
> +	xchk_trans_alloc_empty(sc);
>  
>  	/* Iterate all AGs for inodes rmaps. */
>  	while ((error = xchk_iscan_iter(&rr->iscan, &ip)) == 1) {
> diff --git a/fs/xfs/scrub/rtrmap_repair.c b/fs/xfs/scrub/rtrmap_repair.c
> index fc2592c53af5..4a56726d9952 100644
> --- a/fs/xfs/scrub/rtrmap_repair.c
> +++ b/fs/xfs/scrub/rtrmap_repair.c
> @@ -580,9 +580,7 @@ xrep_rtrmap_find_rmaps(
>  	 */
>  	xchk_trans_cancel(sc);
>  	xchk_rtgroup_unlock(&sc->sr);
> -	error = xchk_trans_alloc_empty(sc);
> -	if (error)
> -		return error;
> +	xchk_trans_alloc_empty(sc);
>  
>  	while ((error = xchk_iscan_iter(&rr->iscan, &ip)) == 1) {
>  		error = xrep_rtrmap_scan_inode(rr, ip);
> -- 
> 2.47.2
> 
> 

[PATCH 7/8] xfs: return the allocated transaction from xrep_trans_alloc_hook_dummy

[Christoph Hellwig]

xrep_trans_alloc_hook_dummy can't return errors, so return the allocated
transaction directly instead of an output double pointer argument.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/scrub/repair.c        | 8 +++-----
 fs/xfs/scrub/repair.h        | 4 ++--
 fs/xfs/scrub/rmap_repair.c   | 5 +----
 fs/xfs/scrub/rtrmap_repair.c | 5 +----
 4 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index f7f80ff32afc..79251c595e18 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -1273,16 +1273,14 @@ xrep_setup_xfbtree(
  * function MUST NOT be called from regular repair code because the current
  * process' transaction is saved via the cookie.
  */
-int
+struct xfs_trans *
 xrep_trans_alloc_hook_dummy(
 	struct xfs_mount	*mp,
-	void			**cookiep,
-	struct xfs_trans	**tpp)
+	void			**cookiep)
 {
 	*cookiep = current->journal_info;
 	current->journal_info = NULL;
-	*tpp = xfs_trans_alloc_empty(mp);
-	return 0;
+	return xfs_trans_alloc_empty(mp);
 }
 
 /* Cancel a dummy transaction used by a live update hook function. */
diff --git a/fs/xfs/scrub/repair.h b/fs/xfs/scrub/repair.h
index af0a3a9e5ed9..0a808e903cf5 100644
--- a/fs/xfs/scrub/repair.h
+++ b/fs/xfs/scrub/repair.h
@@ -180,8 +180,8 @@ int xrep_quotacheck(struct xfs_scrub *sc);
 int xrep_reinit_pagf(struct xfs_scrub *sc);
 int xrep_reinit_pagi(struct xfs_scrub *sc);
 
-int xrep_trans_alloc_hook_dummy(struct xfs_mount *mp, void **cookiep,
-		struct xfs_trans **tpp);
+struct xfs_trans *xrep_trans_alloc_hook_dummy(struct xfs_mount *mp,
+		void **cookiep);
 void xrep_trans_cancel_hook_dummy(void **cookiep, struct xfs_trans *tp);
 
 bool xrep_buf_verify_struct(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
diff --git a/fs/xfs/scrub/rmap_repair.c b/fs/xfs/scrub/rmap_repair.c
index bf1e632b449a..6024872a17e5 100644
--- a/fs/xfs/scrub/rmap_repair.c
+++ b/fs/xfs/scrub/rmap_repair.c
@@ -1621,9 +1621,7 @@ xrep_rmapbt_live_update(
 
 	trace_xrep_rmap_live_update(pag_group(rr->sc->sa.pag), action, p);
 
-	error = xrep_trans_alloc_hook_dummy(mp, &txcookie, &tp);
-	if (error)
-		goto out_abort;
+	tp = xrep_trans_alloc_hook_dummy(mp, &txcookie);
 
 	mutex_lock(&rr->lock);
 	mcur = xfs_rmapbt_mem_cursor(rr->sc->sa.pag, tp, &rr->rmap_btree);
@@ -1644,7 +1642,6 @@ xrep_rmapbt_live_update(
 out_cancel:
 	xfbtree_trans_cancel(&rr->rmap_btree, tp);
 	xrep_trans_cancel_hook_dummy(&txcookie, tp);
-out_abort:
 	mutex_unlock(&rr->lock);
 	xchk_iscan_abort(&rr->iscan);
 out_unlock:
diff --git a/fs/xfs/scrub/rtrmap_repair.c b/fs/xfs/scrub/rtrmap_repair.c
index 4a56726d9952..5b8155c87873 100644
--- a/fs/xfs/scrub/rtrmap_repair.c
+++ b/fs/xfs/scrub/rtrmap_repair.c
@@ -855,9 +855,7 @@ xrep_rtrmapbt_live_update(
 
 	trace_xrep_rmap_live_update(rtg_group(rr->sc->sr.rtg), action, p);
 
-	error = xrep_trans_alloc_hook_dummy(mp, &txcookie, &tp);
-	if (error)
-		goto out_abort;
+	tp = xrep_trans_alloc_hook_dummy(mp, &txcookie);
 
 	mutex_lock(&rr->lock);
 	mcur = xfs_rtrmapbt_mem_cursor(rr->sc->sr.rtg, tp, &rr->rtrmap_btree);
@@ -878,7 +876,6 @@ xrep_rtrmapbt_live_update(
 out_cancel:
 	xfbtree_trans_cancel(&rr->rtrmap_btree, tp);
 	xrep_trans_cancel_hook_dummy(&txcookie, tp);
-out_abort:
 	xchk_iscan_abort(&rr->iscan);
 	mutex_unlock(&rr->lock);
 out_unlock:
-- 
2.47.2

Re: [PATCH 7/8] xfs: return the allocated transaction from xrep_trans_alloc_hook_dummy

[Darrick J. Wong]

On Tue, Jul 15, 2025 at 02:25:40PM +0200, Christoph Hellwig wrote:
> xrep_trans_alloc_hook_dummy can't return errors, so return the allocated
> transaction directly instead of an output double pointer argument.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/scrub/repair.c        | 8 +++-----
>  fs/xfs/scrub/repair.h        | 4 ++--
>  fs/xfs/scrub/rmap_repair.c   | 5 +----
>  fs/xfs/scrub/rtrmap_repair.c | 5 +----
>  4 files changed, 7 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index f7f80ff32afc..79251c595e18 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -1273,16 +1273,14 @@ xrep_setup_xfbtree(
>   * function MUST NOT be called from regular repair code because the current
>   * process' transaction is saved via the cookie.
>   */
> -int
> +struct xfs_trans *
>  xrep_trans_alloc_hook_dummy(
>  	struct xfs_mount	*mp,
> -	void			**cookiep,
> -	struct xfs_trans	**tpp)
> +	void			**cookiep)
>  {
>  	*cookiep = current->journal_info;
>  	current->journal_info = NULL;

You could get rid of all this journal_info manipulation because xfs no
longer uses that to track the current transaction.

--D

> -	*tpp = xfs_trans_alloc_empty(mp);
> -	return 0;
> +	return xfs_trans_alloc_empty(mp);
>  }
>  
>  /* Cancel a dummy transaction used by a live update hook function. */
> diff --git a/fs/xfs/scrub/repair.h b/fs/xfs/scrub/repair.h
> index af0a3a9e5ed9..0a808e903cf5 100644
> --- a/fs/xfs/scrub/repair.h
> +++ b/fs/xfs/scrub/repair.h
> @@ -180,8 +180,8 @@ int xrep_quotacheck(struct xfs_scrub *sc);
>  int xrep_reinit_pagf(struct xfs_scrub *sc);
>  int xrep_reinit_pagi(struct xfs_scrub *sc);
>  
> -int xrep_trans_alloc_hook_dummy(struct xfs_mount *mp, void **cookiep,
> -		struct xfs_trans **tpp);
> +struct xfs_trans *xrep_trans_alloc_hook_dummy(struct xfs_mount *mp,
> +		void **cookiep);
>  void xrep_trans_cancel_hook_dummy(void **cookiep, struct xfs_trans *tp);
>  
>  bool xrep_buf_verify_struct(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
> diff --git a/fs/xfs/scrub/rmap_repair.c b/fs/xfs/scrub/rmap_repair.c
> index bf1e632b449a..6024872a17e5 100644
> --- a/fs/xfs/scrub/rmap_repair.c
> +++ b/fs/xfs/scrub/rmap_repair.c
> @@ -1621,9 +1621,7 @@ xrep_rmapbt_live_update(
>  
>  	trace_xrep_rmap_live_update(pag_group(rr->sc->sa.pag), action, p);
>  
> -	error = xrep_trans_alloc_hook_dummy(mp, &txcookie, &tp);
> -	if (error)
> -		goto out_abort;
> +	tp = xrep_trans_alloc_hook_dummy(mp, &txcookie);
>  
>  	mutex_lock(&rr->lock);
>  	mcur = xfs_rmapbt_mem_cursor(rr->sc->sa.pag, tp, &rr->rmap_btree);
> @@ -1644,7 +1642,6 @@ xrep_rmapbt_live_update(
>  out_cancel:
>  	xfbtree_trans_cancel(&rr->rmap_btree, tp);
>  	xrep_trans_cancel_hook_dummy(&txcookie, tp);
> -out_abort:
>  	mutex_unlock(&rr->lock);
>  	xchk_iscan_abort(&rr->iscan);
>  out_unlock:
> diff --git a/fs/xfs/scrub/rtrmap_repair.c b/fs/xfs/scrub/rtrmap_repair.c
> index 4a56726d9952..5b8155c87873 100644
> --- a/fs/xfs/scrub/rtrmap_repair.c
> +++ b/fs/xfs/scrub/rtrmap_repair.c
> @@ -855,9 +855,7 @@ xrep_rtrmapbt_live_update(
>  
>  	trace_xrep_rmap_live_update(rtg_group(rr->sc->sr.rtg), action, p);
>  
> -	error = xrep_trans_alloc_hook_dummy(mp, &txcookie, &tp);
> -	if (error)
> -		goto out_abort;
> +	tp = xrep_trans_alloc_hook_dummy(mp, &txcookie);
>  
>  	mutex_lock(&rr->lock);
>  	mcur = xfs_rtrmapbt_mem_cursor(rr->sc->sr.rtg, tp, &rr->rtrmap_btree);
> @@ -878,7 +876,6 @@ xrep_rtrmapbt_live_update(
>  out_cancel:
>  	xfbtree_trans_cancel(&rr->rtrmap_btree, tp);
>  	xrep_trans_cancel_hook_dummy(&txcookie, tp);
> -out_abort:
>  	xchk_iscan_abort(&rr->iscan);
>  	mutex_unlock(&rr->lock);
>  out_unlock:
> -- 
> 2.47.2
> 
> 

Re: [PATCH 7/8] xfs: return the allocated transaction from xrep_trans_alloc_hook_dummy

[Christoph Hellwig]

On Tue, Jul 15, 2025 at 08:18:11AM -0700, Darrick J. Wong wrote:
> >  	*cookiep = current->journal_info;
> >  	current->journal_info = NULL;
> 
> You could get rid of all this journal_info manipulation because xfs no
> longer uses that to track the current transaction.

I'll take a look.  Probably separately from this series to avoid
scope creep.

[PATCH 8/8] xfs: remove the xlog_ticket_t typedef

[Christoph Hellwig]

Almost no users of the typedef left, kill it and switch the remaining
users to use the underlying struct.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c      | 6 +++---
 fs/xfs/xfs_log_priv.h | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 793468b4d30d..bc3297da2143 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3092,16 +3092,16 @@ xfs_log_force_seq(
  */
 void
 xfs_log_ticket_put(
-	xlog_ticket_t	*ticket)
+	struct xlog_ticket	*ticket)
 {
 	ASSERT(atomic_read(&ticket->t_ref) > 0);
 	if (atomic_dec_and_test(&ticket->t_ref))
 		kmem_cache_free(xfs_log_ticket_cache, ticket);
 }
 
-xlog_ticket_t *
+struct xlog_ticket *
 xfs_log_ticket_get(
-	xlog_ticket_t	*ticket)
+	struct xlog_ticket	*ticket)
 {
 	ASSERT(atomic_read(&ticket->t_ref) > 0);
 	atomic_inc(&ticket->t_ref);
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index 39a102cc1b43..a9a7a271c15b 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -144,7 +144,7 @@ enum xlog_iclog_state {
 
 #define XLOG_COVER_OPS		5
 
-typedef struct xlog_ticket {
+struct xlog_ticket {
 	struct list_head	t_queue;	/* reserve/write queue */
 	struct task_struct	*t_task;	/* task that owns this ticket */
 	xlog_tid_t		t_tid;		/* transaction identifier */
@@ -155,7 +155,7 @@ typedef struct xlog_ticket {
 	char			t_cnt;		/* current unit count */
 	uint8_t			t_flags;	/* properties of reservation */
 	int			t_iclog_hdrs;	/* iclog hdrs in t_curr_res */
-} xlog_ticket_t;
+};
 
 /*
  * - A log record header is 512 bytes.  There is plenty of room to grow the
-- 
2.47.2

Re: [PATCH 8/8] xfs: remove the xlog_ticket_t typedef

[Darrick J. Wong]

On Tue, Jul 15, 2025 at 02:25:41PM +0200, Christoph Hellwig wrote:
> Almost no users of the typedef left, kill it and switch the remaining
> users to use the underlying struct.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_log.c      | 6 +++---
>  fs/xfs/xfs_log_priv.h | 4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 793468b4d30d..bc3297da2143 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3092,16 +3092,16 @@ xfs_log_force_seq(
>   */
>  void
>  xfs_log_ticket_put(
> -	xlog_ticket_t	*ticket)
> +	struct xlog_ticket	*ticket)
>  {
>  	ASSERT(atomic_read(&ticket->t_ref) > 0);
>  	if (atomic_dec_and_test(&ticket->t_ref))
>  		kmem_cache_free(xfs_log_ticket_cache, ticket);
>  }
>  
> -xlog_ticket_t *
> +struct xlog_ticket *
>  xfs_log_ticket_get(
> -	xlog_ticket_t	*ticket)
> +	struct xlog_ticket	*ticket)
>  {
>  	ASSERT(atomic_read(&ticket->t_ref) > 0);
>  	atomic_inc(&ticket->t_ref);
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index 39a102cc1b43..a9a7a271c15b 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -144,7 +144,7 @@ enum xlog_iclog_state {
>  
>  #define XLOG_COVER_OPS		5
>  
> -typedef struct xlog_ticket {
> +struct xlog_ticket {
>  	struct list_head	t_queue;	/* reserve/write queue */
>  	struct task_struct	*t_task;	/* task that owns this ticket */
>  	xlog_tid_t		t_tid;		/* transaction identifier */
> @@ -155,7 +155,7 @@ typedef struct xlog_ticket {
>  	char			t_cnt;		/* current unit count */
>  	uint8_t			t_flags;	/* properties of reservation */
>  	int			t_iclog_hdrs;	/* iclog hdrs in t_curr_res */
> -} xlog_ticket_t;
> +};
>  
>  /*
>   * - A log record header is 512 bytes.  There is plenty of room to grow the
> -- 
> 2.47.2
> 
> 

cleanup transaction allocation v2

[Christoph Hellwig]

Hi all,

this series cleans up the xfs_trans_alloc* and xfs_trans_reserve*
interfaces by keeping different code paths more separate and thus
removing redundant arguments and error returns.

A git tree is also available here:

    git://git.infradead.org/users/hch/xfs.git xfs-trans-cleanups

Gitweb:

    https://git.infradead.org/?p=users/hch/xfs.git;a=shortlog;h=refs/heads/xfs-trans-cleanups

Changes since v1:
 - drop the scrub handling for ->journal_info
 - clean up xfs_trans_roll a bit more
 - clean up xfs_trans_reserve_more a bit more
 - fix an potentially uninitialized error variable

Diffstat:
 libxfs/xfs_refcount.c |    4 
 scrub/common.c        |    7 -
 scrub/common.h        |    2 
 scrub/dir_repair.c    |    8 -
 scrub/fscounters.c    |    3 
 scrub/metapath.c      |    4 
 scrub/nlinks.c        |    8 -
 scrub/nlinks_repair.c |    4 
 scrub/parent_repair.c |   12 --
 scrub/quotacheck.c    |    4 
 scrub/repair.c        |   36 --------
 scrub/repair.h        |    4 
 scrub/rmap_repair.c   |   14 ---
 scrub/rtrmap_repair.c |   14 ---
 scrub/scrub.c         |    5 -
 xfs_attr_item.c       |    5 -
 xfs_discard.c         |   12 --
 xfs_fsmap.c           |    4 
 xfs_icache.c          |    5 -
 xfs_inode.c           |    7 -
 xfs_itable.c          |   18 ----
 xfs_iwalk.c           |   11 --
 xfs_log.c             |    6 -
 xfs_log_priv.h        |    4 
 xfs_notify_failure.c  |    5 -
 xfs_qm.c              |   10 --
 xfs_rtalloc.c         |   13 ---
 xfs_trans.c           |  208 +++++++++++++++++++++++---------------------------
 xfs_trans.h           |    3 
 xfs_zone_gc.c         |    5 -
 30 files changed, 151 insertions(+), 294 deletions(-)

[PATCH 8/8] xfs: remove the xlog_ticket_t typedef

[Christoph Hellwig]

Almost no users of the typedef left, kill it and switch the remaining
users to use the underlying struct.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/xfs_log.c      | 6 +++---
 fs/xfs/xfs_log_priv.h | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 793468b4d30d..bc3297da2143 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3092,16 +3092,16 @@ xfs_log_force_seq(
  */
 void
 xfs_log_ticket_put(
-	xlog_ticket_t	*ticket)
+	struct xlog_ticket	*ticket)
 {
 	ASSERT(atomic_read(&ticket->t_ref) > 0);
 	if (atomic_dec_and_test(&ticket->t_ref))
 		kmem_cache_free(xfs_log_ticket_cache, ticket);
 }
 
-xlog_ticket_t *
+struct xlog_ticket *
 xfs_log_ticket_get(
-	xlog_ticket_t	*ticket)
+	struct xlog_ticket	*ticket)
 {
 	ASSERT(atomic_read(&ticket->t_ref) > 0);
 	atomic_inc(&ticket->t_ref);
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index 39a102cc1b43..a9a7a271c15b 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -144,7 +144,7 @@ enum xlog_iclog_state {
 
 #define XLOG_COVER_OPS		5
 
-typedef struct xlog_ticket {
+struct xlog_ticket {
 	struct list_head	t_queue;	/* reserve/write queue */
 	struct task_struct	*t_task;	/* task that owns this ticket */
 	xlog_tid_t		t_tid;		/* transaction identifier */
@@ -155,7 +155,7 @@ typedef struct xlog_ticket {
 	char			t_cnt;		/* current unit count */
 	uint8_t			t_flags;	/* properties of reservation */
 	int			t_iclog_hdrs;	/* iclog hdrs in t_curr_res */
-} xlog_ticket_t;
+};
 
 /*
  * - A log record header is 512 bytes.  There is plenty of room to grow the
-- 
2.47.2