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

[Dave Chinner <david@fromorbit.com>]

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@fromorbit.com