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.
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...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
