On Tue, Mar 21, 2017 at 5:34 PM, Dave Chinner <david@xxxxxxxxxxxxx> wrote: > On Tue, Mar 21, 2017 at 06:55:31AM -0400, Amir Goldstein wrote: >> On Tue, Mar 21, 2017 at 5:18 AM, Zorro Lang <zlang@xxxxxxxxxx> wrote: >> > The output of x/068 will be perturbed by new fsstress operations. >> > For keep away breaking x/068's golden image, skip mread and >> > mwrite which are brought in by a new fsstress patch. >> > >> > Reported-by: Eryu Guan <eguan@xxxxxxxxxx> >> > Signed-off-by: Zorro Lang <zlang@xxxxxxxxxx> >> > --- >> > tests/xfs/068 | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/tests/xfs/068 b/tests/xfs/068 >> > index 4dac95e..568ba60 100755 >> > --- a/tests/xfs/068 >> > +++ b/tests/xfs/068 >> > @@ -44,7 +44,7 @@ _supported_fs xfs >> > _supported_os Linux >> > >> > # need to ensure new fsstress operations don't perturb expected output >> > -FSSTRESS_AVOID="-f insert=0 $FSSTRESS_AVOID" >> > +FSSTRESS_AVOID="-f insert=0 -f mwrite=0 -f mread=0 $FSSTRESS_AVOID" >> >> What?? This is so fragile! > > Yet, it is is the existing method for telling fsstress not to run > certain optionsi on an "as needed" basis: > > $ git grep ^FSSTRESS_AVOID > tests/ext4/307:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0 -ffdatasync=0" > tests/generic/019:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0 -ffdatasync=0 -f setattr=1" > tests/generic/269:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0 -ffdatasync=0" > tests/generic/270:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0 -ffdatasync=0" > tests/xfs/068:FSSTRESS_AVOID="-f insert=0 $FSSTRESS_AVOID" > $ git grep -B 1 ^FSSTRESS_AVOID tests/ext4/307-# Disable all sync operations to get higher load tests/ext4/307:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0 -ffdatasync=0" -- tests/generic/019-# Disable all sync operations to get higher load tests/generic/019:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0 -ffdatasync=0 -f setattr=1" -- tests/generic/269-# Disable all sync operations to get higher load tests/generic/269:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0 -ffdatasync=0" -- tests/generic/270-# Disable all sync operations to get higher load tests/generic/270:FSSTRESS_AVOID="$FSSTRESS_AVOID -ffsync=0 -fsync=0 -ffdatasync=0" -- tests/xfs/068-# need to ensure new fsstress operations don't perturb expected output tests/xfs/068:FSSTRESS_AVOID="-f insert=0 $FSSTRESS_AVOID" There is a pattern here and xfs/086 doesn't match it. It is perfectly understandable to want to exclude certain ops that don't mix well with the test, but to avoid an op instead of update the output? I am must be missing the difficulty of the alternative solution. >> If the test wanted to restrict fsstress to a pre-defined set of operations, then >> _create_dumpdir_stress_num() should have prefixed '_param' with -z and >> mention all pre-defined ops (and not only the more frequent ones). > > Reality: _create_dumpdir_stress_num(), along with all the other dump > tests and infrastructure, was ported from the Irix version of > xfstests way back in: > > commit 27fba05e66981c239c3be7a7e5a3aa0d8dc59247 > Author: Nathan Scott <nathans@xxxxxxx> > Date: Mon Jan 15 05:01:19 2001 +0000 > > cmd/xfs/stress/001 1.6 Renamed to cmd/xfstests/001 > > The dump infrastructure is muddy and obtuse, and rather than risk > breaking other dump tests by changing infrastructure, I simply > applied the known, accepted method for making fsstress avoid certain > operations. > >> I am very surprised that Dave was the one that made this bandaid. > > Reality: Maintainers consider more than just the code when it comes > to merging things. Encouraging community participation and making > forwards progress is rather more important than having any single > change be perfect. IOWs, rather than have things drag on endlessly > by sending the patchset back to the authors for yet another round of > review, I simply said "fuck it, no more bikeshedding, we need to > merge this" and made the obvious, one line change that fixed the > only regression my testing uncovered. > Music to my ears :-) > >> Eric, >> >> Can you say if adding -z to _create_dumpdir_stress_num() params >> would be fine with the 2 tests that use it - xfs/022 and xfs/068? > > That means dump tests will /never/ exercise new fsstress operations > which, IMO, is much worse than having to keep a test up to date with > new operations. > > i.e. The whole point of adding new operations to fsstress and have > tests run them is that we automatically get coverage of new > functionality. Neutering this for all xfsdump tests by using "-z" is > - philosophically speaking - the wrong thing to do. > Fair enough. So why exclude the new fsstress operation instead of accomodating 086.out? philosophically speaking - that would be the right thing to do. No? >> 068 output would have to be fixed to accommodate this change, >> but then we can remove the bandaid above and never think of >> this ever again. > > And so never get coverage of new functionality fsstress exposes that > may affect xfsdump behaviour, and hence we may very well miss > regressions those new operations cause.... Operations like insert and mmap write.... I think we both agree that Zorro should try to remove the existing bandaid and fix the expected output. Right? -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
