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