| Message ID | 172480131627.2291268.8798821424165754100.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [01/10] xfs: fix C++ compilation errors in xfs_fs.h | expand |
Loks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
If xfs_bmbt_iroot_alloc return the if_rot value,
xfs_bmap_extents_to_btree could make use of that, though.
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.
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 >
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); 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); cur = xfs_bmbt_init_cursor(mp, tp, ip, whichfork); if (wasdel) cur->bc_flags |= XFS_BTREE_BMBT_WASDEL; @@ -724,6 +715,7 @@ xfs_bmap_extents_to_btree( /* * Fill in the root key and pointer. */ + block = ifp->if_broot; kp = xfs_bmbt_key_addr(mp, block, 1); arp = xfs_bmbt_rec_addr(mp, ablock, 1); kp->br_startoff = cpu_to_be64(xfs_bmbt_disk_get_startoff(arp)); @@ -745,7 +737,7 @@ xfs_bmap_extents_to_btree( out_unreserve_dquot: xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_BCOUNT, -1L); out_root_realloc: - xfs_iroot_realloc(ip, -1, whichfork); + xfs_iroot_free(ip, whichfork); ifp->if_format = XFS_DINODE_FMT_EXTENTS; ASSERT(ifp->if_broot == NULL); xfs_btree_del_cursor(cur, XFS_BTREE_ERROR); diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c index 3695b3ad07d4d..0769644d30412 100644 --- a/fs/xfs/libxfs/xfs_bmap_btree.c +++ b/fs/xfs/libxfs/xfs_bmap_btree.c @@ -759,3 +759,16 @@ xfs_bmbt_destroy_cur_cache(void) kmem_cache_destroy(xfs_bmbt_cur_cache); xfs_bmbt_cur_cache = NULL; } + +/* Create an incore bmbt btree root block. */ +void +xfs_bmbt_iroot_alloc( + struct xfs_inode *ip, + int whichfork) +{ + struct xfs_mount *mp = ip->i_mount; + struct xfs_ifork *ifp = xfs_ifork_ptr(ip, whichfork); + + xfs_iroot_alloc(ip, whichfork, xfs_bmap_broot_space_calc(mp, 1)); + xfs_bmbt_init_block(ip, ifp->if_broot, NULL, 1, 1); +} diff --git a/fs/xfs/libxfs/xfs_bmap_btree.h b/fs/xfs/libxfs/xfs_bmap_btree.h index b7842c3420f04..a187f4b120ea1 100644 --- a/fs/xfs/libxfs/xfs_bmap_btree.h +++ b/fs/xfs/libxfs/xfs_bmap_btree.h @@ -204,4 +204,6 @@ xfs_bmap_bmdr_space(struct xfs_btree_block *bb) return xfs_bmdr_space_calc(be16_to_cpu(bb->bb_numrecs)); } +void xfs_bmbt_iroot_alloc(struct xfs_inode *ip, int whichfork); + #endif /* __XFS_BMAP_BTREE_H__ */