Re: [PATCH V3 12/12] xfs: Define max extent length based on on-disk format definition

[Chandan Babu R <chandan.babu@oracle.com>]

On 28 Sep 2021 at 06:03, Dave Chinner wrote:
> On Thu, Sep 16, 2021 at 03:36:47PM +0530, Chandan Babu R wrote:
>> The maximum extent length depends on maximum block count that can be stored in
>> a BMBT record. Hence this commit defines MAXEXTLEN based on
>> BMBT_BLOCKCOUNT_BITLEN.
>> 
>> While at it, the commit also renames MAXEXTLEN to XFS_MAX_EXTLEN.
>
> hmmmm. So you reimplemented:
>
> #define BMBT_BLOCKCOUNT_MASK    ((1ULL << BMBT_BLOCKCOUNT_BITLEN) - 1)
>
> and defined it as XFS_MAX_EXTLEN?
>
> One of these two defines needs to go away. :)
>
> Also, this macro really defines the maximum extent length a BMBT
> record can hold, not the maximum XFS extent length supported. I
> think it should be  named XFS_BMBT_MAX_EXTLEN and also used to
> replace BMBT_BLOCKCOUNT_MASK.

Thanks for the suggestion. I will incorporate this before posting the next
version of the patchset.

>
> The counter example are free space btree records - they can hold
> extents lengths up to 2^31 blocks long:
>
> typedef struct xfs_alloc_rec {
>         __be32          ar_startblock;  /* starting block number */
>         __be32          ar_blockcount;  /* count of free blocks */
> } xfs_alloc_rec_t, xfs_alloc_key_t;
>
> So, yes, I think MAXEXTLEN needs cleaning up, but it needs some more
> work to make it explicit in what it refers to.
>
> Also:
>
>> -/*
>> - * Max values for extlen and disk inode's extent counters.
>> - */
>> -#define	MAXEXTLEN		((xfs_extlen_t)0x1fffff)	/* 21 bits */
>> -#define XFS_IFORK_EXTCNT_MAXU48	((xfs_extnum_t)0xffffffffffff)	/* Unsigned 48-bits */
>> -#define XFS_IFORK_EXTCNT_MAXU32	((xfs_aextnum_t)0xffffffff)	/* Unsigned 32-bits */
>> -#define XFS_IFORK_EXTCNT_MAXS32 ((xfs_extnum_t)0x7fffffff)	/* Signed 32-bits */
>> -#define XFS_IFORK_EXTCNT_MAXS16 ((xfs_aextnum_t)0x7fff)		/* Signed 16-bits */
>> -
>> -
>>  /*
>>   * Inode minimum and maximum sizes.
>>   */
>> @@ -1701,6 +1691,16 @@ typedef struct xfs_bmbt_rec {
>>  typedef uint64_t	xfs_bmbt_rec_base_t;	/* use this for casts */
>>  typedef xfs_bmbt_rec_t xfs_bmdr_rec_t;
>>  
>> +/*
>> + * Max values for extlen and disk inode's extent counters.
>> + */
>> +#define XFS_MAX_EXTLEN		((xfs_extlen_t)(1 << BMBT_BLOCKCOUNT_BITLEN) - 1)
>> +#define XFS_IFORK_EXTCNT_MAXU48	((xfs_extnum_t)0xffffffffffff)	/* Unsigned 48-bits */
>> +#define XFS_IFORK_EXTCNT_MAXU32	((xfs_aextnum_t)0xffffffff)	/* Unsigned 32-bits */
>> +#define XFS_IFORK_EXTCNT_MAXS32 ((xfs_extnum_t)0x7fffffff)	/* Signed 32-bits */
>> +#define XFS_IFORK_EXTCNT_MAXS16 ((xfs_aextnum_t)0x7fff)		/* Signed 16-bits */
>
> At the end of the patch series, I still really don't like these
> names. Hungarian notation is ugly, and they don't tell me what type
> they apply to. Hence I don't know what limit is the correct one to
> apply to which fork and which format....
>
> These would be much better as
>
> #define XFS_MAX_EXTCNT_DATA_FORK	((1ULL < 48) - 1)
> #define XFS_MAX_EXTCNT_ATTR_FORK	((1ULL < 32) - 1)
>
> #define XFS_MAX_EXTCNT_DATA_FORK_OLD	((1ULL < 31) - 1)
> #define XFS_MAX_EXTCNT_ATTR_FORK_OLD	((1ULL < 15) - 1)
>
> The name tells me what object/format they apply to, and the
> implementation tells me the exact size without needing a comment
> to make it readable. And it doesn't need casts that just add noise
> to the implementation...

I agree. I will include this change in the next version of the patchset.

-- 
chandan