On Tue, Jan 7, 2020 at 4:10 AM Xiao Yang <yangx.jy@xxxxxxxxxxxxxx> wrote: > > On 2020/1/6 19:24, Amir Goldstein wrote: > > A very strong NACK! > > > > Why are you making this change? > > Did you read the description of the tests and try to understand the sequence > > prescribed in the replay-ops file? > > > > Those are reproducers to specific issues that require a very specific sequence > > of operations and it seems to me that 'keep_size' is there for a reason in every > > one of those tests. > > > > For example, take the test generic/456, which I wrote, it has this link in the > > comment above fsxops to a very elaborate email from Ted explaining the > > problem:https://marc.info/?l=linux-ext4&m=151137380830381&w=2 > > > > You cannot just remove 'keep_size' from the test because then the test > > doesn't do what it is intended to do. > > > > Did you read my reply to Eryu's patch which he referred you to? > > https://spinics.net/lists/fstests/msg08007.html > > > > Instead of allowing test generic/456 to run on fs which doesn't support > > FL_KEEP_SIZE, you should change the test to*require* support for > > FL_KEEP_SIZE as well as require support for punch/zero/collapse: > > > > _require_xfs_io_command "falloc" "-k" > > _require_xfs_io_command "fpunch" > > _require_xfs_io_command "fzero" > > _require_xfs_io_command "fcollapse" > > > > Same for the other tests that you changed to ignore keep_size. > > > Hi Amir, > > Thanks for your comment. > > I had a doubt and asked Eryu after sending the second patch: > https://www.spinics.net/lists/fstests/msg13278.html > > Current fsx skips all unsupported ops(e.g. punch_hole, zero_range, > collapse_range) automatically even if they are specified by > --replay-ops. Is this existing logic wrong? No it is not wrong, but this is different than skipping keep_size. My point is that when fsx is run with some parameters and produces a replay-ops log. If that replay-ops log is replayed with exact same parameters it should produce the exact same sequence. In the case of punch/zero/collapse, if those are skipped when recording the ops log, then ops log includes the keyword "skip ..." explicitly, but but for skipped keep_size, keep_size will simply not be in the log. IOW, the fsxops logs that are hardcoded in the 4 tests that you changed have all been created *with* keep_size support and *with* punch/zero/collapse support, so should also be replayed the same way, because otherwise the test is simply not doing what it was intended to do. > > I read your suggestion before, but i just have a worry: > xfs_io commands cannot detect the supported flags of fallocate() > correctly in one case(i.e. xfs_io commands are not supported but > fallocate(2) supports flags). > That is really not a problem. Why do you think we need to invest energy on this scenario? Simply update xfsprogs to latest version if you want to run all the tests in xfstests. That is anyway required for many other tests regardless. > fsx has many test_ functions to check these flags, so we can make fsx > call only these test_ functions by adding a option and then report "not > run" by analyzing the generated output. > I think that would be an overkill. You already have fine _require statements that you can use, so better use them. The _require statements as I wrote them above also serve as clear human readable documentation of what this test requires. Thanks, Amir.
