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

["Darrick J. Wong" <djwong@kernel.org>]

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
> 
>