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
