Re: [PATCH 09/12] xfs: check record domain when accessing refcount records

[Dave Chinner <david@fromorbit.com>]

On Thu, Oct 27, 2022 at 10:14:53AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Now that we've separated the startblock and CoW/shared extent domain in
> the incore refcount record structure, check the domain whenever we
> retrieve a record to ensure that it's still in the domain that we want.
> Depending on the circumstances, a change in domain either means we're
> done processing or that we've found a corruption and need to fail out.
> 
> The refcount check in xchk_xref_is_cow_staging is redundant since
> _get_rec has done that for a long time now, so we can get rid of it.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_refcount.c |   53 ++++++++++++++++++++++++++++++++----------
>  fs/xfs/scrub/refcount.c      |    4 ++-
>  2 files changed, 43 insertions(+), 14 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> index 3b1cb0578770..608a122eef16 100644
> --- a/fs/xfs/libxfs/xfs_refcount.c
> +++ b/fs/xfs/libxfs/xfs_refcount.c
> @@ -386,6 +386,8 @@ xfs_refcount_split_extent(
>  		goto out_error;
>  	}
>  
> +	if (rcext.rc_domain != domain)
> +		return 0;
>  	if (rcext.rc_startblock == agbno || xfs_refc_next(&rcext) <= agbno)
>  		return 0;
>  
> @@ -434,6 +436,9 @@ xfs_refcount_merge_center_extents(
>  	int				error;
>  	int				found_rec;
>  
> +	ASSERT(left->rc_domain == center->rc_domain);
> +	ASSERT(right->rc_domain == center->rc_domain);
> +
>  	trace_xfs_refcount_merge_center_extents(cur->bc_mp,
>  			cur->bc_ag.pag->pag_agno, left, center, right);

Can you move the asserts to after the trace points? That way we if
need to debug the assert, we'll have a tracepoint with the record
information that triggered the assert emitted immediately before it
fires...

>  
> @@ -510,6 +515,8 @@ xfs_refcount_merge_left_extent(
>  	int				error;
>  	int				found_rec;
>  
> +	ASSERT(left->rc_domain == cleft->rc_domain);
> +
>  	trace_xfs_refcount_merge_left_extent(cur->bc_mp,
>  			cur->bc_ag.pag->pag_agno, left, cleft);
>  
> @@ -571,6 +578,8 @@ xfs_refcount_merge_right_extent(
>  	int				error;
>  	int				found_rec;
>  
> +	ASSERT(right->rc_domain == cright->rc_domain);
> +
>  	trace_xfs_refcount_merge_right_extent(cur->bc_mp,
>  			cur->bc_ag.pag->pag_agno, cright, right);
>  
> @@ -654,12 +663,10 @@ xfs_refcount_find_left_extents(
>  		goto out_error;
>  	}
>  
> +	if (tmp.rc_domain != domain)
> +		return 0;
>  	if (xfs_refc_next(&tmp) != agbno)
>  		return 0;
> -	if (domain == XFS_REFC_DOMAIN_SHARED && tmp.rc_refcount < 2)
> -		return 0;
> -	if (domain == XFS_REFC_DOMAIN_COW && tmp.rc_refcount > 1)
> -		return 0;

Ah, as these go away, you can ignore my comment about them in the
previous patches... :)

Otherwise, looks ok.

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

-- 
Dave Chinner
david@fromorbit.com