Re: [PATCH 09/12] xfs: refactor xfs_inode_verify_forks

[Brian Foster <bfoster@redhat.com>]

On Fri, May 08, 2020 at 08:34:20AM +0200, Christoph Hellwig wrote:
> The split between xfs_inode_verify_forks and the two helpers
> implementing the actual functionality is a little strange.  Reshuffle
> it so that xfs_inode_verify_forks verifies if the data and attr forks
> are actually in local format and only call the low-level helpers if
> that is the case.  Handle the actual error reporting in the low-level
> handlers to streamline the caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_inode_fork.c | 51 ++++++++++++++++++++++------------
>  fs/xfs/libxfs/xfs_inode_fork.h |  4 +--
>  fs/xfs/xfs_inode.c             | 21 +++-----------
>  3 files changed, 40 insertions(+), 36 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index e346e143f1053..401921975d75b 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -674,34 +674,51 @@ xfs_ifork_init_cow(
>  }
>  
>  /* Verify the inline contents of the data fork of an inode. */
> -xfs_failaddr_t
> -xfs_ifork_verify_data(
> +int
> +xfs_ifork_verify_local_data(
>  	struct xfs_inode	*ip)
>  {
> -	/* Non-local data fork, we're done. */
> -	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
> -		return NULL;
> +	xfs_failaddr_t		fa = NULL;
>  
> -	/* Check the inline data fork if there is one. */
>  	switch (VFS_I(ip)->i_mode & S_IFMT) {
>  	case S_IFDIR:
> -		return xfs_dir2_sf_verify(ip);
> +		fa = xfs_dir2_sf_verify(ip);
> +		break;
>  	case S_IFLNK:
> -		return xfs_symlink_shortform_verify(ip);
> +		fa = xfs_symlink_shortform_verify(ip);
> +		break;
>  	default:
> -		return NULL;
> +		break;
>  	}
> +
> +	if (fa) {
> +		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
> +			ip->i_df.if_u1.if_data, ip->i_df.if_bytes, fa);
> +		return -EFSCORRUPTED;
> +	}
> +
> +	return 0;
>  }
>  
>  /* Verify the inline contents of the attr fork of an inode. */
> -xfs_failaddr_t
> -xfs_ifork_verify_attr(
> +int
> +xfs_ifork_verify_local_attr(
>  	struct xfs_inode	*ip)
>  {
> -	/* There has to be an attr fork allocated if aformat is local. */
> -	if (ip->i_d.di_aformat != XFS_DINODE_FMT_LOCAL)
> -		return NULL;
> -	if (!XFS_IFORK_PTR(ip, XFS_ATTR_FORK))
> -		return __this_address;
> -	return xfs_attr_shortform_verify(ip);
> +	struct xfs_ifork	*ifp = ip->i_afp;
> +	xfs_failaddr_t		fa;
> +
> +	if (!ifp)
> +		fa = __this_address;
> +	else
> +		fa = xfs_attr_shortform_verify(ip);
> +
> +	if (fa) {
> +		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> +			ifp ? ifp->if_u1.if_data : NULL,
> +			ifp ? ifp->if_bytes : 0, fa);
> +		return -EFSCORRUPTED;
> +	}
> +
> +	return 0;
>  }
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 3f84d33abd3b7..f46a8c1db5964 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -176,7 +176,7 @@ extern struct kmem_zone	*xfs_ifork_zone;
>  
>  extern void xfs_ifork_init_cow(struct xfs_inode *ip);
>  
> -xfs_failaddr_t xfs_ifork_verify_data(struct xfs_inode *ip);
> -xfs_failaddr_t xfs_ifork_verify_attr(struct xfs_inode *ip);
> +int xfs_ifork_verify_local_data(struct xfs_inode *ip);
> +int xfs_ifork_verify_local_attr(struct xfs_inode *ip);
>  
>  #endif	/* __XFS_INODE_FORK_H__ */
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 25c00ffe18409..c8abdefe00377 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3715,25 +3715,12 @@ bool
>  xfs_inode_verify_forks(
>  	struct xfs_inode	*ip)
>  {
> -	struct xfs_ifork	*ifp;
> -	xfs_failaddr_t		fa;
> -
> -	fa = xfs_ifork_verify_data(ip);
> -	if (fa) {
> -		ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> -		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
> -				ifp->if_u1.if_data, ifp->if_bytes, fa);
> +	if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
> +	    xfs_ifork_verify_local_data(ip))
>  		return false;
> -	}
> -
> -	fa = xfs_ifork_verify_attr(ip);
> -	if (fa) {
> -		ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
> -		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> -				ifp ? ifp->if_u1.if_data : NULL,
> -				ifp ? ifp->if_bytes : 0, fa);
> +	if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL &&
> +	    xfs_ifork_verify_local_attr(ip))
>  		return false;
> -	}
>  	return true;
>  }
>  
> -- 
> 2.26.2
>