Re: [PATCH V7 11/17] xfs: Introduce macros to represent new maximum extent counts for data/attr forks

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

On 04 Mar 2022 at 08:02, Dave Chinner wrote:
> On Tue, Mar 01, 2022 at 04:09:32PM +0530, Chandan Babu R wrote:
>> This commit defines new macros to represent maximum extent counts allowed by
>> filesystems which have support for large per-inode extent counters.
>> 
>> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
>> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
>> ---
>>  fs/xfs/libxfs/xfs_bmap.c       |  8 +++-----
>>  fs/xfs/libxfs/xfs_bmap_btree.c |  2 +-
>>  fs/xfs/libxfs/xfs_format.h     | 20 ++++++++++++++++----
>>  fs/xfs/libxfs/xfs_inode_buf.c  |  3 ++-
>>  fs/xfs/libxfs/xfs_inode_fork.c |  2 +-
>>  fs/xfs/libxfs/xfs_inode_fork.h | 19 +++++++++++++++----
>>  6 files changed, 38 insertions(+), 16 deletions(-)
>> 
>> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
>> index a01d9a9225ae..be7f8ebe3cd5 100644
>> --- a/fs/xfs/libxfs/xfs_bmap.c
>> +++ b/fs/xfs/libxfs/xfs_bmap.c
>> @@ -61,10 +61,8 @@ xfs_bmap_compute_maxlevels(
>>  	int		sz;		/* root block size */
>>  
>>  	/*
>> -	 * The maximum number of extents in a file, hence the maximum number of
>> -	 * leaf entries, is controlled by the size of the on-disk extent count,
>> -	 * either a signed 32-bit number for the data fork, or a signed 16-bit
>> -	 * number for the attr fork.
>> +	 * The maximum number of extents in a fork, hence the maximum number of
>> +	 * leaf entries, is controlled by the size of the on-disk extent count.
>>  	 *
>>  	 * Note that we can no longer assume that if we are in ATTR1 that the
>>  	 * fork offset of all the inodes will be
>> @@ -74,7 +72,7 @@ xfs_bmap_compute_maxlevels(
>>  	 * ATTR2 we have to assume the worst case scenario of a minimum size
>>  	 * available.
>>  	 */
>> -	maxleafents = xfs_iext_max_nextents(whichfork);
>> +	maxleafents = xfs_iext_max_nextents(xfs_has_nrext64(mp), whichfork);
>>  	if (whichfork == XFS_DATA_FORK)
>>  		sz = XFS_BMDR_SPACE_CALC(MINDBTPTRS);
>>  	else
>> diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
>> index 453309fc85f2..e8d21d69b9ff 100644
>> --- a/fs/xfs/libxfs/xfs_bmap_btree.c
>> +++ b/fs/xfs/libxfs/xfs_bmap_btree.c
>> @@ -611,7 +611,7 @@ xfs_bmbt_maxlevels_ondisk(void)
>>  	minrecs[1] = xfs_bmbt_block_maxrecs(blocklen, false) / 2;
>>  
>>  	/* One extra level for the inode root. */
>> -	return xfs_btree_compute_maxlevels(minrecs, MAXEXTNUM) + 1;
>> +	return xfs_btree_compute_maxlevels(minrecs, XFS_MAX_EXTCNT_DATA_FORK) + 1;
>>  }
>>  
>>  /*
>> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
>> index 9934c320bf01..d3dfd45c39e0 100644
>> --- a/fs/xfs/libxfs/xfs_format.h
>> +++ b/fs/xfs/libxfs/xfs_format.h
>> @@ -872,10 +872,22 @@ enum xfs_dinode_fmt {
>>  
>>  /*
>>   * Max values for extlen, extnum, aextnum.
>> - */
>> -#define	MAXEXTLEN	((xfs_extlen_t)0x001fffff)	/* 21 bits */
>> -#define	MAXEXTNUM	((xfs_extnum_t)0x7fffffff)	/* signed int */
>> -#define	MAXAEXTNUM	((xfs_aextnum_t)0x7fff)		/* signed short */
>> + *
>> + * The newly introduced data fork extent counter is a 64-bit field. However, the
>> + * maximum number of extents in a file is limited to 2^54 extents (assuming one
>> + * blocks per extent) by the 54-bit wide startoff field of an extent record.
>> + *
>> + * A further limitation applies as shown below,
>> + * 2^63 (max file size) / 64k (max block size) = 2^47
>> + *
>> + * Rounding up 47 to the nearest multiple of bits-per-byte results in 48. Hence
>> + * 2^48 was chosen as the maximum data fork extent count.
>> + */
>> +#define	MAXEXTLEN			((xfs_extlen_t)((1ULL << 21) - 1)) /* 21 bits */
>> +#define XFS_MAX_EXTCNT_DATA_FORK	((xfs_extnum_t)((1ULL << 48) - 1)) /* Unsigned 48-bits */
>> +#define XFS_MAX_EXTCNT_ATTR_FORK	((xfs_extnum_t)((1ULL << 32) - 1)) /* Unsigned 32-bits */
>> +#define XFS_MAX_EXTCNT_DATA_FORK_OLD	((xfs_extnum_t)((1ULL << 31) - 1)) /* Signed 32-bits */
>> +#define XFS_MAX_EXTCNT_ATTR_FORK_OLD	((xfs_extnum_t)((1ULL << 15) - 1)) /* Signed 16-bits */
>
> These go way beyond 80 columns. You do not need the trailing comment
> saying how many bits are supported - that's obvious from numbers.
> If you need to describe the actual supported limits, then do it
> in the head comment:
>
> /*
>  * Max values for extent sizes and counts
>  *
>  * The original on-disk extent counts were held in signed fields,
>  * resulting in maximum extent counts of 2^31 and 2^15 for the data
>  * and attr forks respectively. Similarly the maximum extent length
>  * is limited to 2^21 blocks by the 21-bit wide blockcount field of
>  * a BMBT extent record.
>  *
>  * The newly introduced data fork extent counter can hold a 64-bit
>  * value, however the  maximum number of extents in a file is also
>  * limited to 2^54 extents by the 54-bit wide startoff field of a BMBT
>  * extent record.
>  *
>  * It is further limited by the maximum supported file size
>  * of 2^63 *bytes*. This leads to a maximum extent count for maximally sized
>  * filesystem blocks (64kB) of:
>  *
>  * 2^63 bytes / 2^16 bytes per block = 2^47 blocks
>  *
>  * Rounding up 47 to the nearest multiple of bits-per-byte
>  * results in 48. Hence 2^48 was chosen as the maximum data fork
>  * extent count.
>  */
> #define	MAXEXTLEN			((xfs_extlen_t)((1ULL << 21) - 1))
> #define XFS_MAX_EXTCNT_DATA_FORK	((xfs_extnum_t)((1ULL << 48) - 1))
> #define XFS_MAX_EXTCNT_ATTR_FORK	((xfs_extnum_t)((1ULL << 32) - 1))
> #define XFS_MAX_EXTCNT_DATA_FORK_OLD	((xfs_extnum_t)((1ULL << 31) - 1))
> #define XFS_MAX_EXTCNT_ATTR_FORK_OLD	((xfs_extnum_t)((1ULL << 15) - 1))
>

Ok. I will make the change suggested above.

>
> Hmmm. On reading that back and looking at the code below, maybe the
> names should be _LARGE and _SMALL, not (blank) and _OLD....
>

Ok. I will make this change.

>>  /*
>>   * Inode minimum and maximum sizes.
>> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
>> index 860d32816909..34f360a38603 100644
>> --- a/fs/xfs/libxfs/xfs_inode_buf.c
>> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
>> @@ -361,7 +361,8 @@ xfs_dinode_verify_fork(
>>  			return __this_address;
>>  		break;
>>  	case XFS_DINODE_FMT_BTREE:
>> -		max_extents = xfs_iext_max_nextents(whichfork);
>> +		max_extents = xfs_iext_max_nextents(xfs_dinode_has_nrext64(dip),
>> +					whichfork);
>
>>  		if (di_nextents > max_extents)
>>  			return __this_address;
>>  		break;
>> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
>> index ce690abe5dce..a3a3b54f9c55 100644
>> --- a/fs/xfs/libxfs/xfs_inode_fork.c
>> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
>> @@ -746,7 +746,7 @@ xfs_iext_count_may_overflow(
>>  	if (whichfork == XFS_COW_FORK)
>>  		return 0;
>>  
>> -	max_exts = xfs_iext_max_nextents(whichfork);
>> +	max_exts = xfs_iext_max_nextents(xfs_inode_has_nrext64(ip), whichfork);
>>  
>>  	if (XFS_TEST_ERROR(false, ip->i_mount, XFS_ERRTAG_REDUCE_MAX_IEXTENTS))
>>  		max_exts = 10;
>> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
>> index 4a8b77d425df..e56803436c61 100644
>> --- a/fs/xfs/libxfs/xfs_inode_fork.h
>> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
>> @@ -133,12 +133,23 @@ static inline int8_t xfs_ifork_format(struct xfs_ifork *ifp)
>>  	return ifp->if_format;
>>  }
>>  
>> -static inline xfs_extnum_t xfs_iext_max_nextents(int whichfork)
>> +static inline xfs_extnum_t xfs_iext_max_nextents(bool has_nrext64,
> 							has_large_extent_counts
>> +				int whichfork)
>>  {
>> -	if (whichfork == XFS_DATA_FORK || whichfork == XFS_COW_FORK)
>> -		return MAXEXTNUM;
>> +	switch (whichfork) {
>> +	case XFS_DATA_FORK:
>> +	case XFS_COW_FORK:
>> +		return has_nrext64 ? XFS_MAX_EXTCNT_DATA_FORK
>> +			: XFS_MAX_EXTCNT_DATA_FORK_OLD;
>
> 		if (has_large_extent_counts)
> 			return XFS_MAX_EXTCNT_DATA_FORK_LARGE;
> 		return XFS_MAX_EXTCNT_DATA_FORK_SMALL;
>
> That reads much better to me...
>

Ok. 

-- 
chandan