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

[Dave Chinner <david@fromorbit.com>]

On Thu, Apr 07, 2022 at 01:48:17PM +0530, Chandan Babu R wrote:
> On 07 Apr 2022 at 08:14, Dave Chinner wrote:
> > 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.
> >  */
> >
> 
> If there are no objections, I will include the above comment as part of this
> patch.

Yes, that's fine, and with that added, you can add:

Reviewed-by: Dave Chinner <dchinner@redhat.com>

as well.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com