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

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Oct 28, 2022 at 08:15:31AM +1100, Dave Chinner wrote:
> 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...

Done.

> >  
> > @@ -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>

Cool, thanks!

--D

> -- 
> Dave Chinner
> david@fromorbit.com