Re: [RFC PATCH] xfs/179: modify test to trigger refcount update bugs

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

On Wed, Nov 30, 2022 at 09:42:27AM +1100, Dave Chinner wrote:
> On Tue, Nov 29, 2022 at 02:06:39PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Upon enabling fsdax + reflink for XFS, this test began to report
> > refcount metadata corruptions after being run.  Specifically, xfs_repair
> > noticed single-block refcount records that could be combined but had not
> > been.
> > 
> > The root cause of this is improper MAXREFCOUNT edge case handling in
> > xfs_refcount_merge_extents.  When we're trying to find candidates for a
> > record merge, we compute the refcount of the merged record, but without
> > accounting for the fact that once a record hits rc_refcount ==
> > MAXREFCOUNT, it is pinned that way forever.
> > 
> > Adjust this test to use a sub-filesize write for one of the COW writes,
> > because this is how we force the extent merge code to run.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> 
> Seems like a reasonable modification to the test....
> 
> > ---
> >  tests/xfs/179 |   28 +++++++++++++++++++++++++---
> >  1 file changed, 25 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tests/xfs/179 b/tests/xfs/179
> > index ec0cb7e5b4..214558f694 100755
> > --- a/tests/xfs/179
> > +++ b/tests/xfs/179
> > @@ -21,17 +21,28 @@ _require_scratch_nocheck
> >  _require_cp_reflink
> >  _require_test_program "punch-alternating"
> >  
> > +_fixed_by_kernel_commit XXXXXXXXXXXX "xfs: estimate post-merge refcounts correctly"
> 
> Though I really don't like these annotation because when the test
> fails in future as I'm developing new code it's going to tell me I
> need a fix I already have in the kernel. This is just extra noise
> that I have to filter out of the results output. IMO a comment for
> this information or a line in the commit message is fine - it
> just doesn't belong in the test output....

I'll turn that into a comment, since this originally was a functional
test, not a regression test.

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

Ok thanks!

--D

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