From: "Darrick J. Wong" <djwong@kernel.org>
To: Dave Chinner <david@fromorbit.com>
Cc: chandan.babu@oracle.com, chandanrlinux@gmail.com,
linux-xfs@vger.kernel.org
Subject: Re: [PATCH 3/4] xfs: check absolute maximum nlevels for each btree type
Date: Mon, 27 Sep 2021 13:29:10 -0700 [thread overview]
Message-ID: <20210927202910.GU570615@magnolia> (raw)
In-Reply-To: <20210927181751.GS570615@magnolia>
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
next prev parent reply other threads:[~2021-09-27 20:29 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-24 1:27 [PATCHSET RFC v2 chandan 0/4] xfs: separate btree cursor slab zones Darrick J. Wong
2021-09-24 1:27 ` [PATCH 1/4] xfs: remove xfs_btree_cur.bc_blocklog Darrick J. Wong
2021-09-24 1:27 ` [PATCH 2/4] xfs: reduce the size of nr_ops for refcount btree cursors Darrick J. Wong
2021-09-24 1:27 ` [PATCH 3/4] xfs: check absolute maximum nlevels for each btree type Darrick J. Wong
2021-09-26 0:43 ` Dave Chinner
2021-09-27 18:17 ` Darrick J. Wong
2021-09-27 20:29 ` Darrick J. Wong [this message]
2021-09-27 21:57 ` Dave Chinner
2021-09-24 1:27 ` [PATCH 4/4] xfs: use separate btree cursor slab " Darrick J. Wong
2021-09-26 0:47 ` Dave Chinner
2021-09-27 18:21 ` Darrick J. Wong
2021-09-27 22:01 ` Dave Chinner
2021-10-12 18:49 ` Darrick J. Wong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210927202910.GU570615@magnolia \
--to=djwong@kernel.org \
--cc=chandan.babu@oracle.com \
--cc=chandanrlinux@gmail.com \
--cc=david@fromorbit.com \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).