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

Chandan Babu R
2022-04-07T08:18:40Z

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.

--
chandan