Re: [PATCH v2 8/8] xfs/068: fix clonerange problems in file/dir count output

[Eryu Guan <eguan@redhat.com>]

On Thu, Dec 14, 2017 at 08:52:32AM +0200, Amir Goldstein wrote:
> On Thu, Dec 14, 2017 at 1:44 AM, Dave Chinner <david@fromorbit.com> wrote:
> > On Wed, Dec 13, 2017 at 03:28:05PM -0800, Darrick J. Wong wrote:
> >> From: Darrick J. Wong <darrick.wong@oracle.com>
> >>
> >> In this test we use a fixed sequence of operations in fsstress to create
> >> some number of files and dirs and then exercise xfsdump/xfsrestore on
> >> them.  Since clonerange/deduperange are not supported on all xfs
> >> configurations, detect if they're in fsstress and disable them so that
> >> we always execute exactly the same sequence of operations no matter how
> >> the filesystem is configured.
> >>
> >> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> >> ---
> >>  tests/xfs/068 |    8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/tests/xfs/068 b/tests/xfs/068
> >> index 7151e28..f95a539 100755
> >> --- a/tests/xfs/068
> >> +++ b/tests/xfs/068
> >> @@ -43,6 +43,14 @@ trap "rm -rf $tmp.*; exit \$status" 0 1 2 3 15
> >>  _supported_fs xfs
> >>  _supported_os Linux
> >>
> >> +# Remove fsstress commands that aren't supported on all xfs configs
> >> +if $FSSTRESS_PROG | grep -q clonerange; then
> >> +     FSSTRESS_AVOID="-f clonerange=0 $FSSTRESS_AVOID"
> >> +fi
> >> +if $FSSTRESS_PROG | grep -q deduperange; then
> >> +     FSSTRESS_AVOID="-f deduperange=0 $FSSTRESS_AVOID"
> >> +fi
> >> +
> >
> > I'd put this inside _create_dumpdir_stress_num as it's supposed to
> > DTRT for the dump/restore that follows. Otherwise looks fine.
> >
> 
> Guys,
> 
> Please take a look at the only 2 changes in the history of this test.
> I would like to make sure we are not in a loop:
> 
> 5d36d85 xfs/068: update golden output due to new operations in fsstress
> 6e5194d fsstress: Add fallocate insert range operation
> 
> The first change excludes the new insert op (by dchinner on commit)
> The second change re-includes insert op, does not exclude new
> mread/mwrite ops and updates golden output, following this discussion:
> https://marc.info/?l=fstests&m=149014697111838&w=2
> (the referenced thread ends with a ? to Dave, but was followed by v6..v8
>  that were "silently acked" by Dave).
> 
> I personally argued that the blacklist approach to xfs/068 is fragile and indeed
> this is the third time the test breaks in the history I know of,
> because of added
> fsstress ops. Fine. As long as we at least stay consistent with a decision about
> update golden output vs. exclude ops and document the decision in a comment
> with the reasoning, so we won't have to repeat this discussion next time.

I think the fundamental problem of xfs/068 is the hardcoded file numbers
in .out file, perhaps we should calculate the expected number of
files/dirs to be dumped/restored before the dump test and extract the
actual restored number of files/dirs from xfsrestore output and do a
comparison. (or save the whole tree structure for comparison? I haven't
done any test yet, just some random thoughts for now.)

Currently, xfs/068 will easily break if there's user-defined
FSSTRESS_AVOID, e.g. FSSTRESS_AVOID="-ffallocate=0", and that's totally
legal test configuration.

IHMO we really should fix xfs/068 first to avoid hitting the same
problem again and again.

Thanks,
Eryu

> 
> Darrick,
> 
> IMO, we should follow the path of updating golden output and instead of
> dropping clone/dedupe from ops table in runtime, you should make them
> a noop or ignore the error, keeping the random sequence unchanged.
> This is more or less what happens with insert/collapse (error is ignored)
> already, so it would be weird to make exceptions.
> 
> For reference, fsx does disable insert/collapse/zero/punch at runtime
> and that does change the random sequence of fsx.
> 
> Cheers,
> Amir.