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@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. > */ > If there are no objections, I will include the above comment as part of this patch. -- chandan
