[PATCH v2 1/2] ltp/fsx.c: Add FALLOC_FL_KEEP_SIZE flag and '-K' option

[PATCH v2 1/2] ltp/fsx.c: Add FALLOC_FL_KEEP_SIZE flag and '-K' option

[Xiao Yang]

1) Add FALLOC_FL_KEEP_SIZE flag for do_zero_range().
2) Add '-K' option to the usage of fsx.

Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com>
---
 ltp/fsx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/ltp/fsx.c b/ltp/fsx.c
index 06d08e4e..11465e62 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -1197,7 +1197,7 @@ void
 do_zero_range(unsigned offset, unsigned length, int keep_size)
 {
 	unsigned end_offset;
-	int mode = FALLOC_FL_ZERO_RANGE;
+	int mode = keep_size ? FALLOC_FL_ZERO_RANGE | FALLOC_FL_KEEP_SIZE : FALLOC_FL_ZERO_RANGE;
 
 	if (length == 0) {
 		if (!quiet && testcalls > simulatedopcount)
@@ -2215,7 +2215,7 @@ void
 usage(void)
 {
 	fprintf(stdout, "usage: %s",
-		"fsx [-dknqxABEFJLOWZ] [-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 [-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\
 	-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\
@@ -2265,6 +2265,9 @@ usage(void)
 #ifdef HAVE_COPY_FILE_RANGE
 "	-E: Do not use copy range calls\n"
 #endif
+#ifdef FALLOC_FL_KEEP_SIZE
+"	-K: Do not use keep size calls\n"
+#endif
 "	-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\
-- 
2.21.0

[PATCH v2 2/2] fsx: Add '-a' option to skip unsupported keep size automatically

[Xiao Yang]

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(-)

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
-- 
2.21.0

Re: [PATCH v2 2/2] fsx: Add '-a' option to skip unsupported keep size automatically

[Amir Goldstein]

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.

Re: [PATCH v2 2/2] fsx: Add '-a' option to skip unsupported keep size automatically

[Xiao Yang]

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

Re: [PATCH v2 2/2] fsx: Add '-a' option to skip unsupported keep size automatically

[Amir Goldstein]

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.

Re: [PATCH v2 2/2] fsx: Add '-a' option to skip unsupported keep size automatically

[Xiao Yang]

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.

Re: [PATCH v2 2/2] fsx: Add '-a' option to skip unsupported keep size automatically

[Amir Goldstein]

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.

Re: [PATCH v2 2/2] fsx: Add '-a' option to skip unsupported keep size automatically

[Xiao Yang]

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