On Wed, Jan 23, 2019 at 09:35:47AM +0200, Amir Goldstein wrote: > > > > 3) Either change common/dump or xfs/068.out doesn't take much time/code, so I > > > > decided to change 068.out to make x/068 to do operations. > > > > > > Did you validate that the file count is actually correct? > > > > > > The file count in these dump/restore tests is how we tell we've > > > broken bulkstat iteration - any mismatch on the file count indicates > > > bulkstat didn't find something it should have or it counted things > > > twice. Hence I'm wary making changes to the file set size the > > > dump/restore tests use. > > > > > > And, really, changing the file set size somewhat invalidates the > > > long regression history we have with these existing tests. i.e. > > > don't change tests that work correctly unless it's absolutely > > > necessary. It only adds to the risk of introducing new bugs. > > > > Thanks so much for your detailed description :) I'll turn to change > > FSSTRESS_AVOID in next V4 patch. > > > > On the same discussion when Zorro added "insert" op, I asked > why not convert test to whitelist of ops instead of having to update > the blacklist every time fsstress is extended (which happened 3 times > since we had this discussion). > > To my question, Dave answered: > (https://marc.info/?l=fstests&m=149013211607727&w=2) > > > 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. > > For the life of me, I cannot follow the logic of how this argument ends > up with a decision to update the blacklist. It is just me? > > Yes, if we use a whitelist we will /never/ exercise new fsstress operations for > xfs/068. It may be the wrong thing to do "philosophically speaking" but in > practice, we are doing this anyway, just manually every time an op is added. > > This is just madness. > > Eryu, can you please step in and end this ordeal once and for all. > > If we still want to exercise new fsstress ops for xfs/022 and future xfsdump > tests (do we?) the solution is quite simple: > factor out _create_dumpdir_stress_num_param and pass in whitelist > of ops only from xfs/068 and be done with that. > > Zorro, be the final decision as it may, blacklist or whitelist, the test 068 is > currently verifying that the file count is what happened to be the file count > after addition of "insert" and "mread/mwrite" ops, which to the best of my > knowledge, no one verified is the correct file count. > > So if we want to leave this test "unchanged", we'd better revert your last two > changes to 068.out and resort to Eric's original test output and also > exclude (or not include in whitelist) insert/mread/mwrite ops. :) I don't mind re-adding mread/mwrite operations into FSSTRESS_AVOID. At that time, we haven't decided either "change 068.out" or "avoid new operations" is better. So maybe both are fine (my personal point). But Darrick *had to* brought in FSSTRESS_AVOID into common/dump (commit ID: 451504891), he can't change 068.out directly due to dedupe/clonerange/copyrange aren't supported by all xfs configurations. So my original point is: change 068.out if add common&stable operations into fsstress, otherwise add into FSSTRESS_AVOID in common/dump. As Dave specified so many problems of splice(), I don't mind avoiding it. But mread/mwrite are common functions, as we've changed 068.out for them, how about keep the current code until we find something wrong in the future :) Thanks, Zorro > > Thanks, > Amir.
