On Thu, Nov 23, 2017 at 10:05:00AM +0200, Amir Goldstein wrote: > On Thu, Nov 23, 2017 at 9:08 AM, Eryu Guan <eguan@xxxxxxxxxx> wrote: > > On Sat, Nov 18, 2017 at 02:19:06PM +0800, Eryu Guan wrote: > >> On start up, fsx checks for various fallocate(2) operation support > >> status and disables unsupported operations. But when replaying > >> operations from a log, fsx failed to skip KEEP_SIZE fallocate(2) > >> calls if underlying filesystem doesn't support it. For example, > >> NFSv4.2 supports fallocate(2) but not KEEP_SIZE, and this causes > >> generic/469 fails on NFSv4.2. > >> > >> Fix it by taking 'keep_size_calls' into consideration when replaying > >> ops from log file. > >> > >> Signed-off-by: Eryu Guan <eguan@xxxxxxxxxx> > >> --- > >> > >> Note that generic/469 hasn't been pushed to upstream yet. Will do in > >> this week's update. > > > > generic/469 is upstream now, still need some reviews on this patch. > > > > I don't like the path we have taken starting with generic/456 (so partly > my own blame) that the test passes instead of "not run" for fs that > don't support the replayed fsx operations, because fsx skips them. Firstly thanks a lot for the review! IMHO, there's no harm to run more tests even if that's not the primary/original test goal, as long as test doesn't fail due to the unsupported operations. I've seen quite a few bugs found by totally unexpected test cases, you'll never know when and where bug happens :) > > Because the sub-test in question really wants to test KEEP_SIZE, > IMO we should explicitly: generic/469 also tests the non-KEEP_SIZE path for better test coverage, though that won't reproduce the original bug. But I agreed that it can be a bit confusing to test on filesystems without KEEP_SIZE support. So how about I adding some comments to generic/456 and 469 to state that it's intentional, for better test coverage, to run tests on filesystems that don't support the replayed operations? Thanks, Eryu > > _require_xfs_io_command "falloc" "-k" > _require_xfs_io_command "fpunch" > > And for generic/456, we should also explicitly: > > _require_xfs_io_command "fpunch" > _require_xfs_io_command "fzero" > _require_xfs_io_command "fcollapse" > > Amir. -- 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
