Message ID | 20200106070654.13249-2-yangx.jy@cn.fujitsu.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2,1/2] ltp/fsx.c: Add FALLOC_FL_KEEP_SIZE flag and '-K' option | expand |
On Mon, Jan 6, 2020 at 9:12 AM Xiao Yang <yangx.jy@cn.fujitsu.com> wrote: > > 1) Use '-a' option to skip unsupported keep size automatically even if > it's defined by --replay-ops. > 2) Add $FSX_AVOID to tests which defines keep size by --replay-ops. > > Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> > --- > ltp/fsx.c | 9 +++++++-- > tests/generic/456 | 2 +- > tests/generic/469 | 2 +- > tests/generic/499 | 2 +- > tests/generic/511 | 2 +- > 5 files changed, 11 insertions(+), 6 deletions(-) > 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. Thanks, Amir.
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? 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). 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. Best Regards, Xiao Yang
On Tue, Jan 7, 2020 at 4:10 AM Xiao Yang <yangx.jy@cn.fujitsu.com> 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.
On 2020/1/7 14:47, Amir Goldstein wrote: > 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. Hi Amir, If keep_size is not supported, ops log can also includes the keyword "skip ..." by Eryu's patch: https://lore.kernel.org/fstests/20191022123115.12250-1-eguan@linux.alibaba.com/ Do you want to add require support for keep_size/punch/zero/collapse and accept Eryu's patch as well? Thanks, Xiao Yang > > 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.
On Tue, Jan 7, 2020 at 9:45 AM Xiao Yang <yangx.jy@cn.fujitsu.com> wrote: > > On 2020/1/7 14:47, Amir Goldstein wrote: > > 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. > Hi Amir, > > If keep_size is not supported, ops log can also includes the keyword > "skip ..." by Eryu's patch: > https://lore.kernel.org/fstests/20191022123115.12250-1-eguan@linux.alibaba.com/ > > Do you want to add require support for keep_size/punch/zero/collapse and > accept Eryu's patch as well? > I am perfectly fine with Eryu's patch and I think it is correct to merge it, but it is completely independent to fixing the 4 tests. Eryu's patch is changing the way the ops log is recorded (for the better), but it doesn't change (and I think it is wrong to change) the way that ops log is replayed. The hardcoded ops logs in those 4 tests are not going to change because you modify fsx. The hardcoded ops logs in those 4 tests intentionally include the keep_size directive, so those tests should not be run on NFS. Thanks, Amir.
On 2020/1/7 16:49, Amir Goldstein wrote: >> > If keep_size is not supported, ops log can also includes the keyword >> > "skip ..." by Eryu's patch: >> > https://lore.kernel.org/fstests/20191022123115.12250-1-eguan@linux.alibaba.com/ >> > >> > Do you want to add require support for keep_size/punch/zero/collapse and >> > accept Eryu's patch as well? >> > > I am perfectly fine with Eryu's patch and I think it is correct to merge it, > but it is completely independent to fixing the 4 tests. Hi Eryu, These 4 tests need the exact operations to reproduce some issues by --replay-ops so Amir perfer to skip tests rather than one operation if a required operation/flag in tests is not supported. I think it is reasonable to skip these tests in the case so I have sent v3 patch set[1][2]: https://www.spinics.net/lists/fstests/msg13290.html https://www.spinics.net/lists/fstests/msg13291.html With my v3 patch set, it seems that these 4 tests have no chance to fall into the condition described by your patch: https://lore.kernel.org/fstests/20191022123115.12250-1-eguan@linux.alibaba.com/ Best Regards, Xiao Yang
diff --git a/ltp/fsx.c b/ltp/fsx.c index 11465e62..cde7df5c 100644 --- a/ltp/fsx.c +++ b/ltp/fsx.c @@ -163,6 +163,7 @@ int seed = 1; /* -S flag */ int mapped_writes = 1; /* -W flag disables */ int fallocate_calls = 1; /* -F flag disables */ int keep_size_calls = 1; /* -K flag disables */ +int skip_keep_size = 0; /* -a flag skips unsupported keep_size automatically */ int punch_hole_calls = 1; /* -H flag disables */ int zero_range_calls = 1; /* -z flag disables */ int collapse_range_calls = 1; /* -C flag disables */ @@ -2215,7 +2216,7 @@ void usage(void) { fprintf(stdout, "usage: %s", - "fsx [-dknqxABEFJKLOWZ] [-b opnum] [-c Prob] [-g filldata] [-i logdev] [-j logid] [-l flen] [-m start:end] [-o oplen] [-p progressinterval] [-r readbdy] [-s style] [-t truncbdy] [-w writebdy] [-D startingop] [-N numops] [-P dirpath] [-S seed] fname\n\ + "fsx [-adknqxABEFJKLOWZ] [-b opnum] [-c Prob] [-g filldata] [-i logdev] [-j logid] [-l flen] [-m start:end] [-o oplen] [-p progressinterval] [-r readbdy] [-s style] [-t truncbdy] [-w writebdy] [-D startingop] [-N numops] [-P dirpath] [-S seed] fname\n\ -b opnum: beginning operation number (default 1)\n\ -c P: 1 in P chance of file close+open at each op (default infinity)\n\ -d: debug output for all operations\n\ @@ -2268,6 +2269,7 @@ usage(void) #ifdef FALLOC_FL_KEEP_SIZE " -K: Do not use keep size calls\n" #endif +" -a: Skip unsupported keep size automatically even if it's defined by --replay-ops\n" " -L: fsxLite - no file creations & no file size changes\n\ -N numops: total # operations to do (default infinity)\n\ -O: use oplen (see -o flag) for every op (default random)\n\ @@ -2472,7 +2474,7 @@ main(int argc, char **argv) setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */ while ((ch = getopt_long(argc, argv, - "b:c:dfg:i:j:kl:m:no:p:qr:s:t:w:xyABD:EFJKHzCILN:OP:RS:WXZ", + "ab:c:dfg:i:j:kl:m:no:p:qr:s:t:w:xyABD:EFJKHzCILN:OP:RS:WXZ", longopts, NULL)) != EOF) switch (ch) { case 'b': @@ -2590,6 +2592,9 @@ main(int argc, char **argv) case 'K': keep_size_calls = 0; break; + case 'a': + skip_keep_size = 1; + break; case 'H': punch_hole_calls = 0; break; diff --git a/tests/generic/456 b/tests/generic/456 index 6124f0bb..0298c789 100755 --- a/tests/generic/456 +++ b/tests/generic/456 @@ -56,7 +56,7 @@ write 0x3e5ec 0x1a14 0x21446 zero_range 0x20fac 0x6d9c 0x40000 keep_size mapwrite 0x216ad 0x274f 0x40000 EOF -run_check $here/ltp/fsx -d --replay-ops $fsxops $SCRATCH_MNT/testfile +run_check $here/ltp/fsx -d --replay-ops $fsxops $FSX_AVOID $SCRATCH_MNT/testfile _flakey_drop_and_remount _unmount_flakey diff --git a/tests/generic/469 b/tests/generic/469 index 47fdf0cf..ba13e022 100755 --- a/tests/generic/469 +++ b/tests/generic/469 @@ -43,7 +43,7 @@ _require_test run_fsx() { - $here/ltp/fsx $2 --replay-ops $1 $file 2>&1 | tee -a $seqres.full >$tmp.fsx + $here/ltp/fsx $2 --replay-ops $1 $FSX_AVOID $file 2>&1 | tee -a $seqres.full >$tmp.fsx if [ ${PIPESTATUS[0]} -ne 0 ]; then cat $tmp.fsx exit 1 diff --git a/tests/generic/499 b/tests/generic/499 index 773eab2e..61346832 100755 --- a/tests/generic/499 +++ b/tests/generic/499 @@ -50,7 +50,7 @@ ENDL victim=$SCRATCH_MNT/a touch $victim -$here/ltp/fsx --replay-ops $tmp.fsxops $victim > $tmp.output 2>&1 || cat $tmp.output +$here/ltp/fsx --replay-ops $tmp.fsxops $FSX_AVOID $victim > $tmp.output 2>&1 || cat $tmp.output echo "Silence is golden" status=0 diff --git a/tests/generic/511 b/tests/generic/511 index 4d133f49..346e291d 100755 --- a/tests/generic/511 +++ b/tests/generic/511 @@ -47,7 +47,7 @@ ENDL victim=$SCRATCH_MNT/a touch $victim -$here/ltp/fsx --replay-ops $tmp.fsxops $victim > $tmp.output 2>&1 || cat $tmp.output +$here/ltp/fsx --replay-ops $tmp.fsxops $FSX_AVOID $victim > $tmp.output 2>&1 || cat $tmp.output echo "Silence is golden" status=0
1) Use '-a' option to skip unsupported keep size automatically even if it's defined by --replay-ops. 2) Add $FSX_AVOID to tests which defines keep size by --replay-ops. Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> --- ltp/fsx.c | 9 +++++++-- tests/generic/456 | 2 +- tests/generic/469 | 2 +- tests/generic/499 | 2 +- tests/generic/511 | 2 +- 5 files changed, 11 insertions(+), 6 deletions(-)