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