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

Darrick J. Wong
2024-08-29T22:46:40Z

On Thu, Aug 29, 2024 at 12:13:09PM +1000, Dave Chinner wrote:
> 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().

Yeah, that makes sense.

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

Makes sense here too, will change both.

--D

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