Re: [PATCH V9 12/19] xfs: Introduce macros to represent new maximum extent counts for data/attr forks

Dave Chinner
2022-04-07T02:44:09Z

On Wed, Apr 06, 2022 at 06:58:55PM -0700, Darrick J. Wong wrote:
> On Thu, Apr 07, 2022 at 11:05:44AM +1000, Dave Chinner wrote:
> > On Wed, Apr 06, 2022 at 11:48:56AM +0530, Chandan Babu R wrote:
> > > diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
> > > index 453309fc85f2..7aabeccea9ab 100644
> > > --- a/fs/xfs/libxfs/xfs_bmap_btree.c
> > > +++ b/fs/xfs/libxfs/xfs_bmap_btree.c
> > > @@ -611,7 +611,8 @@ xfs_bmbt_maxlevels_ondisk(void)
> > > minrecs[1] = xfs_bmbt_block_maxrecs(blocklen, false) / 2;
> > >
> > > /* One extra level for the inode root. */
> > > - return xfs_btree_compute_maxlevels(minrecs, MAXEXTNUM) + 1;
> > > + return xfs_btree_compute_maxlevels(minrecs,
> > > + XFS_MAX_EXTCNT_DATA_FORK_LARGE) + 1;
> > > }
> >
> > Why is this set to XFS_MAX_EXTCNT_DATA_FORK_LARGE rather than being
> > conditional xfs_has_large_extent_counts(mp)? i.e. if the feature bit
> > is not set, the maximum on-disk levels in the bmbt is determined by
> > XFS_MAX_EXTCNT_DATA_FORK_SMALL, not XFS_MAX_EXTCNT_DATA_FORK_LARGE.
>
> This function (and all the other _maxlevels_ondisk functions) compute
> the maximum possible btree height for any filesystem that we'd care to
> mount. This value is then passed to the functions that create the btree
> cursor caches, which is why this is independent of any xfs_mount.
>
> That said ... depending on how much this inflates the size of the bmbt
> cursor cache, I think we could create multiple slabs.
>
> > The "_ondisk" suffix implies that it has something to do with the
> > on-disk format of the filesystem, but AFAICT what we are calculating
> > here is a constant used for in-memory structure allocation? There
> > needs to be something explained/changed here, because this is
> > confusing...
>
> You suggested it. ;)
>
> https://lore.kernel.org/linux-xfs/20211013075743.GG2361455@dread.disaster.area/

That doesn't mean it's perfect and can't be changed, nor that I
remember the exact details of something that happened 6 months ago.
Indeed, if I'm confused by it 6 months later, that tends to say it
wasn't a very good name... :)

.... or that the missing context needs explaining so the reader is
reminded what the _ondisk() name means.

i.e. the problem goes away with a simple comment:

/*
* Calculate the maximum possible height of the btree that the
* on-disk format supports. This is used for sizing structures large
* enough to support every possible configuration of a filesystem
* that might get mounted.
*/

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com