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:

quoted

On Tue, Oct 12, 2021 at 04:33:01PM -0700, Darrick J. Wong wrote:

quoted

From: Darrick J. Wong <djwong@kernel.org>

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@kernel.org>
Reviewed-by: Chandan Babu R <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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*

quoted

quoted

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

quoted

quoted

@@ -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!

quoted

quoted

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@fromorbit.com