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@xxxxxxxxxxxxxxxxxxx/ 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@xxxxxxxxxxxxx
