On 28 Sep 2021 at 06:03, Dave Chinner wrote:

On Thu, Sep 16, 2021 at 03:36:47PM +0530, Chandan Babu R wrote:

quoted

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:

quoted

-/*
- * 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