From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-yb0-f194.google.com ([209.85.213.194]:36258 "EHLO mail-yb0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750873AbdLNGwd (ORCPT ); Thu, 14 Dec 2017 01:52:33 -0500 MIME-Version: 1.0 In-Reply-To: <20171213234404.GF5858@dastard> References: <151314499003.18893.8687182548758898133.stgit@magnolia> <151314505158.18893.11894289091110903029.stgit@magnolia> <20171213232805.GI6896@magnolia> <20171213234404.GF5858@dastard> From: Amir Goldstein Date: Thu, 14 Dec 2017 08:52:32 +0200 Message-ID: Subject: Re: [PATCH v2 8/8] xfs/068: fix clonerange problems in file/dir count output Content-Type: text/plain; charset="UTF-8" Sender: fstests-owner@vger.kernel.org To: Dave Chinner Cc: "Darrick J. Wong" , Eryu Guan , linux-xfs , fstests List-ID: On Thu, Dec 14, 2017 at 1:44 AM, Dave Chinner wrote: > On Wed, Dec 13, 2017 at 03:28:05PM -0800, Darrick J. Wong wrote: >> From: Darrick J. Wong >> >> 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 >> --- >> 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. 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.