Re: [PATCH 10/12] xfs: improve local fork verification

[Brian Foster <bfoster@redhat.com>]

On Fri, May 08, 2020 at 08:34:21AM +0200, Christoph Hellwig wrote:
> Call the data/attr local fork verifies as soon as we are ready for them.
> This keeps them close to the code setting up the forks, and avoids a
> few branches later on.  Also open code xfs_inode_verify_forks in the
> only remaining caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

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

>  fs/xfs/libxfs/xfs_inode_fork.c |  8 +++++++-
>  fs/xfs/xfs_icache.c            |  6 ------
>  fs/xfs/xfs_inode.c             | 28 +++++++++-------------------
>  fs/xfs/xfs_inode.h             |  2 --
>  fs/xfs/xfs_log_recover.c       |  5 -----
>  5 files changed, 16 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 401921975d75b..2fe325e38fd88 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -227,6 +227,7 @@ xfs_iformat_data_fork(
>  	struct xfs_dinode	*dip)
>  {
>  	struct inode		*inode = VFS_I(ip);
> +	int			error;
>  
>  	switch (inode->i_mode & S_IFMT) {
>  	case S_IFIFO:
> @@ -241,8 +242,11 @@ xfs_iformat_data_fork(
>  	case S_IFDIR:
>  		switch (dip->di_format) {
>  		case XFS_DINODE_FMT_LOCAL:
> -			return xfs_iformat_local(ip, dip, XFS_DATA_FORK,
> +			error = xfs_iformat_local(ip, dip, XFS_DATA_FORK,
>  					be64_to_cpu(dip->di_size));
> +			if (!error)
> +				error = xfs_ifork_verify_local_data(ip);
> +			return error;
>  		case XFS_DINODE_FMT_EXTENTS:
>  			return xfs_iformat_extents(ip, dip, XFS_DATA_FORK);
>  		case XFS_DINODE_FMT_BTREE:
> @@ -282,6 +286,8 @@ xfs_iformat_attr_fork(
>  	case XFS_DINODE_FMT_LOCAL:
>  		error = xfs_iformat_local(ip, dip, XFS_ATTR_FORK,
>  				xfs_dfork_attr_shortform_size(dip));
> +		if (!error)
> +			error = xfs_ifork_verify_local_attr(ip);
>  		break;
>  	case XFS_DINODE_FMT_EXTENTS:
>  		error = xfs_iformat_extents(ip, dip, XFS_ATTR_FORK);
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index af5748f5d9271..5a3a520b95288 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -543,14 +543,8 @@ xfs_iget_cache_miss(
>  			goto out_destroy;
>  	}
>  
> -	if (!xfs_inode_verify_forks(ip)) {
> -		error = -EFSCORRUPTED;
> -		goto out_destroy;
> -	}
> -
>  	trace_xfs_iget_miss(ip);
>  
> -
>  	/*
>  	 * Check the inode free state is valid. This also detects lookup
>  	 * racing with unlinks.
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c8abdefe00377..549ff468b7b60 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3707,23 +3707,6 @@ xfs_iflush(
>  	return error;
>  }
>  
> -/*
> - * If there are inline format data / attr forks attached to this inode,
> - * make sure they're not corrupt.
> - */
> -bool
> -xfs_inode_verify_forks(
> -	struct xfs_inode	*ip)
> -{
> -	if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
> -	    xfs_ifork_verify_local_data(ip))
> -		return false;
> -	if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL &&
> -	    xfs_ifork_verify_local_attr(ip))
> -		return false;
> -	return true;
> -}
> -
>  STATIC int
>  xfs_iflush_int(
>  	struct xfs_inode	*ip,
> @@ -3808,8 +3791,15 @@ xfs_iflush_int(
>  	if (!xfs_sb_version_has_v3inode(&mp->m_sb))
>  		ip->i_d.di_flushiter++;
>  
> -	/* Check the inline fork data before we write out. */
> -	if (!xfs_inode_verify_forks(ip))
> +	/*
> +	 * If there are inline format data / attr forks attached to this inode,
> +	 * make sure they are not corrupt.
> +	 */
> +	if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
> +	    xfs_ifork_verify_local_data(ip))
> +		goto flush_out;
> +	if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL &&
> +	    xfs_ifork_verify_local_attr(ip))
>  		goto flush_out;
>  
>  	/*
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 83073c883fbf9..ff846197941e4 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -498,8 +498,6 @@ extern struct kmem_zone	*xfs_inode_zone;
>  /* The default CoW extent size hint. */
>  #define XFS_DEFAULT_COWEXTSZ_HINT 32
>  
> -bool xfs_inode_verify_forks(struct xfs_inode *ip);
> -
>  int xfs_iunlink_init(struct xfs_perag *pag);
>  void xfs_iunlink_destroy(struct xfs_perag *pag);
>  
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 3960caf51c9f7..87b940cb760db 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2878,11 +2878,6 @@ xfs_recover_inode_owner_change(
>  	if (error)
>  		goto out_free_ip;
>  
> -	if (!xfs_inode_verify_forks(ip)) {
> -		error = -EFSCORRUPTED;
> -		goto out_free_ip;
> -	}
> -
>  	if (in_f->ilf_fields & XFS_ILOG_DOWNER) {
>  		ASSERT(in_f->ilf_fields & XFS_ILOG_DBROOT);
>  		error = xfs_bmbt_change_owner(NULL, ip, XFS_DATA_FORK,
> -- 
> 2.26.2
>