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

Dave Chinner
2024-08-29T02:13:13Z

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