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

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

On 05 Jan 2022 at 06:17, Darrick J. Wong wrote:
> On Tue, Dec 14, 2021 at 02:15:19PM +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_BMBT_EXTLEN.
>> 
>> Suggested-by: Darrick J. Wong <djwong@kernel.org>
>> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
>> ---
>>  fs/xfs/libxfs/xfs_alloc.c      |  2 +-
>>  fs/xfs/libxfs/xfs_bmap.c       | 57 +++++++++++++++++-----------------
>>  fs/xfs/libxfs/xfs_format.h     | 21 +++++++------
>>  fs/xfs/libxfs/xfs_inode_buf.c  |  4 +--
>>  fs/xfs/libxfs/xfs_trans_resv.c | 11 ++++---
>>  fs/xfs/scrub/bmap.c            |  2 +-
>>  fs/xfs/xfs_bmap_util.c         | 14 +++++----
>>  fs/xfs/xfs_iomap.c             | 28 ++++++++---------
>>  8 files changed, 72 insertions(+), 67 deletions(-)
>> 
>> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
>> index 353e53b892e6..3f9b9cbfef43 100644
>> --- a/fs/xfs/libxfs/xfs_alloc.c
>> +++ b/fs/xfs/libxfs/xfs_alloc.c
>> @@ -2493,7 +2493,7 @@ __xfs_free_extent_later(
>>  
>>  	ASSERT(bno != NULLFSBLOCK);
>>  	ASSERT(len > 0);
>> -	ASSERT(len <= MAXEXTLEN);
>> +	ASSERT(len <= XFS_MAX_BMBT_EXTLEN);
>>  	ASSERT(!isnullstartblock(bno));
>>  	agno = XFS_FSB_TO_AGNO(mp, bno);
>>  	agbno = XFS_FSB_TO_AGBNO(mp, bno);
>
> Yessss another  unprefixed constant goes away.
>
> <snip>
>
>> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
>> index 3183f78fe7a3..dd5cffe63be3 100644
>> --- a/fs/xfs/libxfs/xfs_format.h
>> +++ b/fs/xfs/libxfs/xfs_format.h
>> @@ -885,15 +885,6 @@ enum xfs_dinode_fmt {
>>  	{ XFS_DINODE_FMT_BTREE,		"btree" }, \
>>  	{ XFS_DINODE_FMT_UUID,		"uuid" }
>>  
>> -/*
>> - * Max values for extlen, extnum, aextnum.
>> - */
>> -#define	MAXEXTLEN			((xfs_extlen_t)0x1fffff)	/* 21 bits */
>> -#define XFS_MAX_EXTCNT_DATA_FORK	((xfs_extnum_t)0xffffffffffff)	/* Unsigned 48-bits */
>> -#define XFS_MAX_EXTCNT_ATTR_FORK	((xfs_aextnum_t)0xffffffff)	/* Unsigned 32-bits */
>> -#define XFS_MAX_EXTCNT_DATA_FORK_OLD	((xfs_extnum_t)0x7fffffff)	/* Signed 32-bits */
>> -#define XFS_MAX_EXTCNT_ATTR_FORK_OLD	((xfs_aextnum_t)0x7fff)		/* Signed 16-bits */
>> -
>>  /*
>>   * Inode minimum and maximum sizes.
>>   */
>> @@ -1623,7 +1614,17 @@ typedef struct xfs_bmdr_block {
>>  #define BMBT_BLOCKCOUNT_BITLEN	21
>>  
>>  #define BMBT_STARTOFF_MASK	((1ULL << BMBT_STARTOFF_BITLEN) - 1)
>> -#define BMBT_BLOCKCOUNT_MASK	((1ULL << BMBT_BLOCKCOUNT_BITLEN) - 1)
>> +
>> +/*
>> + * Max values for extlen and disk inode's extent counters.
>
> Nit: 'ondisk inode'
>
>
>> + */
>> +#define XFS_MAX_BMBT_EXTLEN		((xfs_extlen_t)(1ULL << BMBT_BLOCKCOUNT_BITLEN) - 1)
>> +#define XFS_MAX_EXTCNT_DATA_FORK	((xfs_extnum_t)0xffffffffffff)	/* Unsigned 48-bits */
>> +#define XFS_MAX_EXTCNT_ATTR_FORK	((xfs_aextnum_t)0xffffffff)	/* Unsigned 32-bits */
>> +#define XFS_MAX_EXTCNT_DATA_FORK_OLD	((xfs_extnum_t)0x7fffffff)	/* Signed 32-bits */
>> +#define XFS_MAX_EXTCNT_ATTR_FORK_OLD	((xfs_aextnum_t)0x7fff)		/* Signed 16-bits */
>> +
>> +#define BMBT_BLOCKCOUNT_MASK	XFS_MAX_BMBT_EXTLEN
>
> Would this be simpler if XFS_MAX_EXTCNT* stay where they are, and only
> XFS_MAX_BMBT_EXTLEN moves down to be defined as an alias of
> BMBT_BLOCKCOUNT_MASK?
>

Yes, I think so. Also, all the *BMBT* macros defined around the same place
will probably help make the organization better.

-- 
chandan