Re: [PATCH 3/4] xfs: check absolute maximum nlevels for each btree type

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Sep 27, 2021 at 11:17:51AM -0700, Darrick J. Wong wrote:
> On Sun, Sep 26, 2021 at 10:43:43AM +1000, Dave Chinner wrote:
> > On Thu, Sep 23, 2021 at 06:27:54PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Add code for all five btree types so that we can compute the absolute
> > > maximum possible btree height for each btree type, and then check that
> > > none of them exceed XFS_BTREE_CUR_ZONE_MAXLEVELS.  The code to do the
> > > actual checking is a little excessive, but it sets us up for per-type
> > > cursor zones in the next patch.
> > 
> > Ok, I think the cursor "zone" array is the wrong approach here.
> > 
> > First of all - can we stop using the term "zone" for new code?
> > That's the old irix terminolgy for slab caches, and we have been
> > moving away from that to the Linux "kmem_cache" terminology and
> > types for quite some time.
> > 
> > AFAICT, the only reason for having the zone array is so that
> > xfs_btree_alloc_cursor() can do a lookup via btnum into the array to
> > get the maxlevels and kmem cache pointer to allocate from.
> > 
> > Given that we've just called into xfs_btree_alloc_cursor() from the
> > specific btree type we are allocating the cursor for (that's where
> > we got btnum from!), we should just be passing these type specific
> > variables directly from the caller like we do for btnum. That gets
> > rid of the need for the zone array completely....
> > 
> > i.e. I don't see why the per-type cache information needs to be
> > global information. The individual max-level calculations could just
> > be individual kmem_cache_alloc() calls to set locally defined (i.e.
> > static global) cache pointers and max size variables.
> 
> If the cache is a static variable inside xfs_fubar_btree.c, how do you
> know which cache to pass to kmem_cache_free in xfs_btree_del_cursor?
> Does this imply adding per-btree del_cursor functions and refactoring
> the entire codebase to use them?
> 
> I was /trying/ to get a dependent patchset ready so that Chandan could
> submit the extent counters patchset for 5.16, not trigger a refactoring
> of a whole ton of btree code.  If you want to hide the information that
> badly, please take over this patchset and solve both the above problem
> and then one below.

So of course 5 minutes after sending this grouchy message I notice that
everthing you asked for can be done pretty easily by having each btree
type call a generic btree function that does:

int __init
xfs_btree_create_cursor_cache(xfs_btnum_t btnum, const char *name,
		unsigned int maxlevels)
{
	struct xfs_btree_cur_cache		*cc;

	cc = &xfs_btree_cur_caches[btnum];
	cc->name = name;
	cc->maxlevels = maxlevels;
	cc->alias = false;

	return 0;
}

and a similar destructor function to null all that out.

Then the xfs_db callout becomes:

unsigned int
xfs_btree_absolute_maxlevels(xfs_btnum_t btnum)
{
	return xfs_btree_cur_caches[btnum].maxlevels;
}

printf("rmap maxlevels %u\n",
		libxfs_btree_absolute_maxlevels(XFS_BTNUM_RMAP));

So yeah, I'll do that.

--D

> > > diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.c b/fs/xfs/libxfs/xfs_ialloc_btree.c
> > > index c8fea6a464d5..ce428c98e7c4 100644
> > > --- a/fs/xfs/libxfs/xfs_ialloc_btree.c
> > > +++ b/fs/xfs/libxfs/xfs_ialloc_btree.c
> > > @@ -541,6 +541,17 @@ xfs_inobt_maxrecs(
> > >  	return blocklen / (sizeof(xfs_inobt_key_t) + sizeof(xfs_inobt_ptr_t));
> > >  }
> > >  
> > > +unsigned int
> > > +xfs_inobt_absolute_maxlevels(void)
> > > +{
> > > +	unsigned int		minrecs[2];
> > > +
> > > +	xfs_btree_absolute_minrecs(minrecs, 0, sizeof(xfs_inobt_rec_t),
> > > +			sizeof(xfs_inobt_key_t) + sizeof(xfs_inobt_ptr_t));
> > > +
> > > +	return xfs_btree_compute_maxlevels(minrecs, XFS_MAX_AG_INODES);
> > > +}
> > 
> > i.e. rather than returning the size here, we do:
> > 
> > static int xfs_inobt_maxlevels;
> > static struct kmem_cache xfs_inobt_cursor_cache;
> > 
> > int __init
> > xfs_inobt_create_cursor_cache(void)
> > {
> > 	unsigned int		minrecs[2];
> > 
> > 	xfs_btree_absolute_minrecs(minrecs, 0, sizeof(xfs_inobt_rec_t),
> > 			sizeof(xfs_inobt_key_t) + sizeof(xfs_inobt_ptr_t));
> > 	xfs_inobt_maxlevels = xfs_btree_compute_maxlevels(minrecs,
> > 			XFS_MAX_AG_INODES);
> 
> Something you couldn't have seen here is that the xfsprogs port contains
> an addition to the xfs_db btheight switch to print these absolute maxima
> so that we won't have to compute them by hand anymore.
> 
> Maybe I should have noted both of these points in the commit message?
> Though I've also been chided for submitting excessive comments in the
> past, which is why I didn't.
> 
> --D
> 
> > 	xfs_inobt_cursor_cache = kmem_cache_alloc("xfs_inobt_cur",
> > 			xfs_btree_cur_sizeof(xfs_inobt_maxlevels),
> > 			0, 0, NULL);
> > 	if (!xfs_inobt_cursor_cache)
> > 		return -ENOMEM;
> > 	return 0;
> > }
> > 
> > void
> > xfs_inobt_destroy_cursor_cache(void)
> > {
> > 	kmem_cache_destroy(xfs_inobt_cursor_cache);
> > }
> > 
> > and nothing outside fs/xfs/libxfs/xfs_ialloc_btree.c ever needs to
> > know about these variables as they only ever feed into
> > xfs_btree_alloc_cursor() from xfs_inobt_init_common().
> > 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com