Re: [PATCH 2/2] xfs: fix xfs_rmap_has_other_keys usage of ECANCELED

[Dave Chinner <david@fromorbit.com>]

On Tue, Mar 10, 2020 at 05:47:10PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In e7ee96dfb8c26, we converted all ITER_ABORT users to use ECANCELED
> instead, but we forgot to teach xfs_rmap_has_other_keys not to return
> that magic value to callers.  Fix it now.
> 
> Fixes: e7ee96dfb8c26 ("xfs: remove all *_ITER_ABORT values")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

/me had to go look at that commit to see what this was fixing.

> ---
>  fs/xfs/libxfs/xfs_rmap.c |   13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
> index ff9412f113c4..dae1a2bf28eb 100644
> --- a/fs/xfs/libxfs/xfs_rmap.c
> +++ b/fs/xfs/libxfs/xfs_rmap.c
> @@ -2694,7 +2694,6 @@ struct xfs_rmap_key_state {
>  	uint64_t			owner;
>  	uint64_t			offset;
>  	unsigned int			flags;
> -	bool				has_rmap;
>  };
>  
>  /* For each rmap given, figure out if it doesn't match the key we want. */
> @@ -2709,7 +2708,6 @@ xfs_rmap_has_other_keys_helper(
>  	if (rks->owner == rec->rm_owner && rks->offset == rec->rm_offset &&
>  	    ((rks->flags & rec->rm_flags) & XFS_RMAP_KEY_FLAGS) == rks->flags)
>  		return 0;
> -	rks->has_rmap = true;
>  	return -ECANCELED;
>  }
>  
> @@ -2731,7 +2729,7 @@ xfs_rmap_has_other_keys(
>  	int				error;
>  
>  	xfs_owner_info_unpack(oinfo, &rks.owner, &rks.offset, &rks.flags);
> -	rks.has_rmap = false;
> +	*has_rmap = false;
>  
>  	low.rm_startblock = bno;
>  	memset(&high, 0xFF, sizeof(high));
> @@ -2739,11 +2737,12 @@ xfs_rmap_has_other_keys(
>  
>  	error = xfs_rmap_query_range(cur, &low, &high,
>  			xfs_rmap_has_other_keys_helper, &rks);
> -	if (error < 0)
> -		return error;
> +	if (error == -ECANCELED) {
> +		*has_rmap = true;
> +		return 0;
> +	}
>  
> -	*has_rmap = rks.has_rmap;
> -	return 0;
> +	return error;
>  }

Ok, so there's two things here. The first is catching ECANCELED and
returning the correct value(0). The second is we no longer need the
rks.has_rmap member to be passed to the helper, because -ECANCELED
indicates that we found an rmap.

Ok, makes sense.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com