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@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. Yes, that's fine, and with that added, you can add: Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx> as well. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
