Re: [PATCH 07/10] xfs: refactor creation of bmap btree roots

[Dave Chinner <david@fromorbit.com>]

On Tue, Aug 27, 2024 at 04:35:33PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Now that we've created inode fork helpers to allocate and free btree
> roots, create a new bmap btree helper to create a new bmbt root, and
> refactor the extents <-> btree conversion functions to use our new
> helpers.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_bmap.c       |   20 ++++++--------------
>  fs/xfs/libxfs/xfs_bmap_btree.c |   13 +++++++++++++
>  fs/xfs/libxfs/xfs_bmap_btree.h |    2 ++
>  3 files changed, 21 insertions(+), 14 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 00cac756c9566..e3922cf75381c 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -614,7 +614,7 @@ xfs_bmap_btree_to_extents(
>  	xfs_trans_binval(tp, cbp);
>  	if (cur->bc_levels[0].bp == cbp)
>  		cur->bc_levels[0].bp = NULL;
> -	xfs_iroot_realloc(ip, -1, whichfork);
> +	xfs_iroot_free(ip, whichfork);

I feel like the "whichfork" interface is unnecessary here. We
already have the ifp in all cases here, and so

	xfs_iroot_free(ifp);

avoids the need to look up the ifp again in xfs_iroot_free().

The same happens with xfs_iroot_alloc() - the callers already have
the ifp in a local variable, so...

>  	ASSERT(ifp->if_broot == NULL);
>  	ifp->if_format = XFS_DINODE_FMT_EXTENTS;
>  	*logflagsp |= XFS_ILOG_CORE | xfs_ilog_fext(whichfork);
> @@ -655,19 +655,10 @@ xfs_bmap_extents_to_btree(
>  	ASSERT(ifp->if_format == XFS_DINODE_FMT_EXTENTS);
>  
>  	/*
> -	 * Make space in the inode incore. This needs to be undone if we fail
> -	 * to expand the root.
> -	 */
> -	xfs_iroot_realloc(ip, 1, whichfork);
> -
> -	/*
> -	 * Fill in the root.
> -	 */
> -	block = ifp->if_broot;
> -	xfs_bmbt_init_block(ip, block, NULL, 1, 1);
> -	/*
> -	 * Need a cursor.  Can't allocate until bb_level is filled in.
> +	 * Fill in the root, create a cursor.  Can't allocate until bb_level is
> +	 * filled in.
>  	 */
> +	xfs_bmbt_iroot_alloc(ip, whichfork);

.... this becomes xfs_bmbt_iroot_alloc(ip, ifp);

i.e. once we already have an ifp resolved for the fork, it makes no
sense to pass whichfork down the stack instead of the ifp...

-Dave.
-- 
Dave Chinner
david@fromorbit.com