Re: [PATCH V3 06/12] xfs: xfs_dfork_nextents: Return extent count via an out argument

[Dave Chinner <david@fromorbit.com>]

On Thu, Sep 16, 2021 at 03:36:41PM +0530, Chandan Babu R wrote:
> This commit changes xfs_dfork_nextents() to return an error code. The extent
> count itself is now returned through an out argument. This facility will be
> used by a future commit to indicate an inconsistent ondisk extent count.
> 
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_format.h     | 14 ++---
>  fs/xfs/libxfs/xfs_inode_buf.c  | 16 ++++--
>  fs/xfs/libxfs/xfs_inode_fork.c | 21 ++++++--
>  fs/xfs/scrub/inode.c           | 94 +++++++++++++++++++++-------------
>  fs/xfs/scrub/inode_repair.c    | 34 ++++++++----
>  5 files changed, 118 insertions(+), 61 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index b4638052801f..dba868f2c3e3 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -931,28 +931,30 @@ enum xfs_dinode_fmt {
>  		(dip)->di_format : \
>  		(dip)->di_aformat)
>  
> -static inline xfs_extnum_t
> +static inline int
>  xfs_dfork_nextents(
>  	struct xfs_dinode	*dip,
> -	int			whichfork)
> +	int			whichfork,
> +	xfs_extnum_t		*nextents)
>  {
> -	xfs_extnum_t		nextents = 0;
> +	int			error = 0;
>  
>  	switch (whichfork) {
>  	case XFS_DATA_FORK:
> -		nextents = be32_to_cpu(dip->di_nextents);
> +		*nextents = be32_to_cpu(dip->di_nextents);
>  		break;
>  
>  	case XFS_ATTR_FORK:
> -		nextents = be16_to_cpu(dip->di_anextents);
> +		*nextents = be16_to_cpu(dip->di_anextents);
>  		break;
>  
>  	default:
>  		ASSERT(0);
> +		error = -EFSCORRUPTED;
>  		break;
>  	}
>  
> -	return nextents;
> +	return error;
>  }

So why do we need to do this? AFAICT, the only check that can return
errors that is added by the ned of the patch series is a
on-disk-format check that does:

	if (inode_has_nrext64 && dip->di_nextents16 != 0)
		return -EFSCORRUPTED;

This doesn't belong here - it is conflating verification with
extraction. Verfication of the on-disk format belongs in the
verifiers or verification code, not in the function that extracts

>  /*
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 176c98798aa4..dc511630cc7a 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -345,7 +345,8 @@ xfs_dinode_verify_fork(
>  	xfs_extnum_t		di_nextents;
>  	xfs_extnum_t		max_extents;
>  
> -	di_nextents = xfs_dfork_nextents(dip, whichfork);
> +	if (xfs_dfork_nextents(dip, whichfork, &di_nextents))
> +		return __this_address;
>  
>  	switch (XFS_DFORK_FORMAT(dip, whichfork)) {
>  	case XFS_DINODE_FMT_LOCAL:
> @@ -477,6 +478,7 @@ xfs_dinode_verify(
>  	uint64_t		flags2;
>  	uint64_t		di_size;
>  	xfs_extnum_t            nextents;
> +	xfs_extnum_t            naextents;
>  	xfs_rfsblock_t		nblocks;
>  
>  	if (dip->di_magic != cpu_to_be16(XFS_DINODE_MAGIC))
> @@ -508,8 +510,13 @@ xfs_dinode_verify(
>  	if ((S_ISLNK(mode) || S_ISDIR(mode)) && di_size == 0)
>  		return __this_address;
>  
> -	nextents = xfs_dfork_nextents(dip, XFS_DATA_FORK);
> -	nextents += xfs_dfork_nextents(dip, XFS_ATTR_FORK);
> +	if (xfs_dfork_nextents(dip, XFS_DATA_FORK, &nextents))
> +		return __this_address;
> +
> +	if (xfs_dfork_nextents(dip, XFS_ATTR_FORK, &naextents))
> +		return __this_address;

Yeah, so this should end up being:

xfs_failaddr_t
xfs_dfork_nextents_verify(
	... )
{
	if (ip->di_flags2 & NREXT64) {
		if (!xfs_has_nrext64(mp))
			return __this_address;
		if (dip->di_nextents16 != 0)
			return __this_address;
	} else if (dip->di_nextents64 != 0)
		return __this_address;
	}
	return NULL;
}

and
	faddr = xfs_dfork_nextents_verify(dip, mp);
	if (faddr)
		return faddr;
	nextents = xfs_dfork_nextents(dip, XFS_DATA_FORK);
	naextents = xfs_dfork_nextents(dip, XFS_ATTR_FORK);

Now all the verification can be removed from xfs_dfork_nextents(),
and anything that needs to verify that the extent counts are in a
valid format can call xfs_dfork_nextents_verify() to perform this
check (i.e. the dinode verifiers and scrub checking code).

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com