On Wed, Oct 13, 2021 at 09:52:18AM -0700, Darrick J. Wong wrote:
> On Wed, Oct 13, 2021 at 04:31:22PM +1100, Dave Chinner wrote:
> > On Tue, Oct 12, 2021 at 04:33:01PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> > >
> > > Split out the btree level information into a separate struct and put it
> > > at the end of the cursor structure as a VLA. The realtime rmap btree
> > > (which is rooted in an inode) will require the ability to support many
> > > more levels than a per-AG btree cursor, which means that we're going to
> > > create two btree cursor caches to conserve memory for the more common
> > > case.
> > >
> > > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > > Reviewed-by: Chandan Babu R <chandan.babu@xxxxxxxxxx>
> > > Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> > > ---
> > > fs/xfs/libxfs/xfs_alloc.c | 6 +-
> > > fs/xfs/libxfs/xfs_bmap.c | 10 +--
> > > fs/xfs/libxfs/xfs_btree.c | 168 +++++++++++++++++++++++----------------------
> > > fs/xfs/libxfs/xfs_btree.h | 28 ++++++--
> > > fs/xfs/scrub/bitmap.c | 22 +++---
> > > fs/xfs/scrub/bmap.c | 2 -
> > > fs/xfs/scrub/btree.c | 47 +++++++------
> > > fs/xfs/scrub/trace.c | 7 +-
> > > fs/xfs/scrub/trace.h | 10 +--
> > > fs/xfs/xfs_super.c | 2 -
> > > fs/xfs/xfs_trace.h | 2 -
> > > 11 files changed, 164 insertions(+), 140 deletions(-)
> >
> > Hmmm - subject of the patch doesn't really match the changes being
> > made - there's nothing here that makes the btree cursor heights
> > dynamic. It's just a structure layout change...
>
> "xfs: prepare xfs_btree_cur for dynamic cursor heights" ?
*nod*
> > > @@ -922,11 +922,11 @@ xfs_btree_readahead(
> > > (lev == cur->bc_nlevels - 1))
> > > return 0;
> > >
> > > - if ((cur->bc_ra[lev] | lr) == cur->bc_ra[lev])
> > > + if ((cur->bc_levels[lev].ra | lr) == cur->bc_levels[lev].ra)
> > > return 0;
> >
> > That's whacky logic. Surely that's just:
> >
> > if (cur->bc_levels[lev].ra & lr)
> > return 0;
>
> This is an early-exit test, which means the careful check is necessary.
>
> If (some day) someone calls this function with (LEFTRA|RIGHTRA) to
> readahead both siblings on a btree level where one sibling has been ra'd
> but not the other, we must avoid taking the branch.
Which I didn't see any callers do, so I ignored that possibility.
Regardless, it's the use of "|" to do an additive mask match that
makes it look wierd. i.e. the normal way of writing a multi-biti
mask match is to apply the mask and check that the returned value
matches the mask, like so:
if ((cur->bc_levels[lev].ra & lr) == lr)
return 0;
Really, though, this was just another "ObHuh" comment, and you don't
need to "fix" it now...
> > > @@ -242,8 +250,17 @@ struct xfs_btree_cur
> > > struct xfs_btree_cur_ag bc_ag;
> > > struct xfs_btree_cur_ino bc_ino;
> > > };
> > > +
> > > + /* Must be at the end of the struct! */
> > > + struct xfs_btree_level bc_levels[];
> > > };
> > >
> > > +static inline size_t
> > > +xfs_btree_cur_sizeof(unsigned int nlevels)
> > > +{
> > > + return struct_size((struct xfs_btree_cur *)NULL, bc_levels, nlevels);
> > > +}
> >
> > Ooooh, yeah, we really need comments explaining how many btree
> > levels these VLAs are tracking, because this one doesn't have a "-
> > 1" in it like the previous one I commented on....
>
> /*
> * Compute the size of a btree cursor that can handle a btree of a given
> * height. The bc_levels array handles node and leaf blocks, so its
> * size is exactly nlevels.
> */
Nice. Thanks!
> > > diff --git a/fs/xfs/scrub/trace.c b/fs/xfs/scrub/trace.c
> > > index c0ef53fe6611..816dfc8e5a80 100644
> > > --- a/fs/xfs/scrub/trace.c
> > > +++ b/fs/xfs/scrub/trace.c
> > > @@ -21,10 +21,11 @@ xchk_btree_cur_fsbno(
> > > struct xfs_btree_cur *cur,
> > > int level)
> > > {
> > > - if (level < cur->bc_nlevels && cur->bc_bufs[level])
> > > + if (level < cur->bc_nlevels && cur->bc_levels[level].bp)
> > > return XFS_DADDR_TO_FSB(cur->bc_mp,
> > > - xfs_buf_daddr(cur->bc_bufs[level]));
> > > - if (level == cur->bc_nlevels - 1 && cur->bc_flags & XFS_BTREE_LONG_PTRS)
> > > + xfs_buf_daddr(cur->bc_levels[level].bp));
> > > + else if (level == cur->bc_nlevels - 1 &&
> > > + cur->bc_flags & XFS_BTREE_LONG_PTRS)
> >
> > No need for an else there as the first if () clause returns.
> > Also, needs more () around that "a & b" second line.
>
> TBH I think we check the wrong flag, and that last bit should be:
>
> if (level == cur->bc_nlevels - 1 &&
> (cur->bc_flags & XFS_BTREE_ROOT_IN_INODE))
> return XFS_INO_TO_FSB(cur->bc_mp, cur->bc_ino.ip->i_ino);
>
> return NULLFSBLOCK;
Yup, true, long ptrs and inodes are currently interchangable so it
works, but that's a landmine waiting to pounce....
> But for now I'll stick to the straight replacement and tack on another
> patch to fix that.
*nod*.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
