Re: [PATCH V3 05/12] xfs: Introduce xfs_dfork_nextents() helper

From: Chandan Babu R <chandan.babu@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org, djwong@kernel.org
Subject: Re: [PATCH V3 05/12] xfs: Introduce xfs_dfork_nextents() helper
Date: Tue, 28 Sep 2021 15:16:59 +0530	[thread overview]
Message-ID: <87a6jx117w.fsf@debian-BULLSEYE-live-builder-AMD64> (raw)
In-Reply-To: <20210927224649.GK1756565@dread.disaster.area>

On 28 Sep 2021 at 04:16, Dave Chinner wrote:
> On Thu, Sep 16, 2021 at 03:36:40PM +0530, Chandan Babu R wrote:
>> This commit replaces the macro XFS_DFORK_NEXTENTS() with the helper function
>> xfs_dfork_nextents(). As of this commit, xfs_dfork_nextents() returns the same
>> value as XFS_DFORK_NEXTENTS(). A future commit which extends inode's extent
>> counter fields will add more logic to this helper.
>> 
>> This commit also replaces direct accesses to xfs_dinode->di_[a]nextents
>> with calls to xfs_dfork_nextents().
>> 
>> No functional changes have been made.
>> 
>> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
>> ---
>>  fs/xfs/libxfs/xfs_format.h     | 28 +++++++++++++++++++++----
>>  fs/xfs/libxfs/xfs_inode_buf.c  | 16 +++++++++-----
>>  fs/xfs/libxfs/xfs_inode_fork.c | 10 +++++----
>>  fs/xfs/scrub/inode.c           | 18 +++++++++-------
>>  fs/xfs/scrub/inode_repair.c    | 38 +++++++++++++++++++++-------------
>>  5 files changed, 75 insertions(+), 35 deletions(-)
>> 
>> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
>> index ed8a5354bcbf..b4638052801f 100644
>> --- a/fs/xfs/libxfs/xfs_format.h
>> +++ b/fs/xfs/libxfs/xfs_format.h
>> @@ -930,10 +930,30 @@ enum xfs_dinode_fmt {
>>  	((w) == XFS_DATA_FORK ? \
>>  		(dip)->di_format : \
>>  		(dip)->di_aformat)
>> -#define XFS_DFORK_NEXTENTS(dip,w) \
>> -	((w) == XFS_DATA_FORK ? \
>> -		be32_to_cpu((dip)->di_nextents) : \
>> -		be16_to_cpu((dip)->di_anextents))
>> +
>> +static inline xfs_extnum_t
>> +xfs_dfork_nextents(
>> +	struct xfs_dinode	*dip,
>> +	int			whichfork)
>> +{
>> +	xfs_extnum_t		nextents = 0;
>> +
>> +	switch (whichfork) {
>> +	case XFS_DATA_FORK:
>> +		nextents = be32_to_cpu(dip->di_nextents);
>> +		break;
>> +
>
> No need for whitespace line after the break, and this could just
> return the value directly.
>

Ok. I will fix this.

>> +	case XFS_ATTR_FORK:
>> +		nextents = be16_to_cpu(dip->di_anextents);
>> +		break;
>> +
>> +	default:
>> +		ASSERT(0);
>> +		break;
>> +	}
>> +
>> +	return nextents;
>> +}
>
> I think that all the conditional inode fork macros
> should be moved to libxfs/xfs_inode_fork.h as they are converted.
>
> These macros are not acutally part of the on-disk format definition
> (which is what xfs_format.h is supposed to contain) - it's code that
> parses the on-disk format and that is supposed to be in
> libxfs/xfs_inode_fork.[ch]....
>
> Next thing: the caller almost always knows what fork it wants
> the extents for - only 3 callers have a whichfork variable. So,
> perhaps:
>
> static inline xfs_extnum_t
> xfs_dfork_data_extents(
> 	struct xfs_dinode	*dip)
> {
> 	return be32_to_cpu(dip->di_nextents);
> }
>
> static inline xfs_extnum_t
> xfs_dfork_attr_extents(
> 	struct xfs_dinode	*dip)
> {
> 	return be16_to_cpu(dip->di_anextents);
> }
>
> static inline xfs_extnum_t
> xfs_dfork_extents(
> 	struct xfs_dinode	*dip,
> 	int			whichfork)
> {
> 	switch (whichfork) {
> 	case XFS_DATA_FORK:
> 		return xfs_dfork_data_extents(dip);
> 	case XFS_ATTR_FORK:
> 		return xfs_dfork_attr_extents(dip);
> 	default:
> 		ASSERT(0);
> 		break;
> 	}
> 	return 0;
> }
>
> So we don't have to rely on the compiler optimising away the switch
> statement correctly to produce optimal code.
>

I will fix this too.

>> --- a/fs/xfs/libxfs/xfs_inode_buf.c
>> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
>> @@ -342,9 +342,11 @@ xfs_dinode_verify_fork(
>>  	struct xfs_mount	*mp,
>>  	int			whichfork)
>>  {
>> -	xfs_extnum_t		di_nextents = XFS_DFORK_NEXTENTS(dip, whichfork);
>> +	xfs_extnum_t		di_nextents;
>>  	xfs_extnum_t		max_extents;
>>  
>> +	di_nextents = xfs_dfork_nextents(dip, whichfork);
>> +
>>  	switch (XFS_DFORK_FORMAT(dip, whichfork)) {
>>  	case XFS_DINODE_FMT_LOCAL:
>>  		/*
>> @@ -474,6 +476,8 @@ xfs_dinode_verify(
>>  	uint16_t		flags;
>>  	uint64_t		flags2;
>>  	uint64_t		di_size;
>> +	xfs_extnum_t            nextents;
>> +	xfs_rfsblock_t		nblocks;
>
> That's a block number type, not a block count:
>
> typedef uint64_t        xfs_rfsblock_t; /* blockno in filesystem (raw) */
> ....
> typedef uint64_t        xfs_filblks_t;  /* number of blocks in a file */
>
> The latter is the appropriate type to use here.
>
> Oh, the struct xfs_inode and the struct xfs_log_dinode makes
> this same type mistake. Ok, that's a cleanup for another day....
>

I will add this cleanup to my todo list. 

-- 
chandan