[PATCH] ltp/fsx: really skip fallocate keep_size calls when replaying ops

[PATCH] ltp/fsx: really skip fallocate keep_size calls when replaying ops

[Eryu Guan]

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@redhat.com>
---

Note that generic/469 hasn't been pushed to upstream yet. Will do in
this week's update.

 ltp/fsx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ltp/fsx.c b/ltp/fsx.c
index 9c358f27bd92..fc1381e60f09 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -1469,7 +1469,7 @@ test(void)
 			offset = log_entry.args[0];
 			size = log_entry.args[1];
 			closeopen = !!(log_entry.flags & FL_CLOSE_OPEN);
-			keep_size = !!(log_entry.flags & FL_KEEP_SIZE);
+			keep_size = !!((log_entry.flags & FL_KEEP_SIZE) && keep_size_calls);
 			goto have_op;
 		}
 		return 0;
-- 
2.14.3

Re: [PATCH] ltp/fsx: really skip fallocate keep_size calls when replaying ops

[Eryu Guan]

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@redhat.com>
> ---
> 
> 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.

Thanks,
Eryu

> 
>  ltp/fsx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ltp/fsx.c b/ltp/fsx.c
> index 9c358f27bd92..fc1381e60f09 100644
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
> @@ -1469,7 +1469,7 @@ test(void)
>  			offset = log_entry.args[0];
>  			size = log_entry.args[1];
>  			closeopen = !!(log_entry.flags & FL_CLOSE_OPEN);
> -			keep_size = !!(log_entry.flags & FL_KEEP_SIZE);
> +			keep_size = !!((log_entry.flags & FL_KEEP_SIZE) && keep_size_calls);
>  			goto have_op;
>  		}
>  		return 0;
> -- 
> 2.14.3
> 

Re: [PATCH] ltp/fsx: really skip fallocate keep_size calls when replaying ops

[Amir Goldstein]

On Thu, Nov 23, 2017 at 9:08 AM, Eryu Guan <eguan@redhat.com> 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@redhat.com>
>> ---
>>
>> 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.

Because the sub-test in question really wants to test KEEP_SIZE,
IMO we should explicitly:

_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.

Re: [PATCH] ltp/fsx: really skip fallocate keep_size calls when replaying ops

[Eryu Guan]

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@redhat.com> 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@redhat.com>
> >> ---
> >>
> >> 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.

Re: [PATCH] ltp/fsx: really skip fallocate keep_size calls when replaying ops

[Amir Goldstein]

On Thu, Nov 23, 2017 at 10:28 AM, Eryu Guan <eguan@redhat.com> wrote:
> 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@redhat.com> 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@redhat.com>
>> >> ---
>> >>
>> >> 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?

OK, but I still don't like the patch.

Here is what I think you should do:

Test for KEEP_SIZE support and conditionally set
keep_size=keep_size

Use $keep_size in fsxops generation.

Explain is comment why.

The reason I do not like the patch is because I don't like the fact
that:
fsx --replay-ops=ops.in  --record-ops=ops.out

Results in ops.out different than opts.in.
I know it is already the case for the "skipped" operations, but
at least that is well annotated, whereas your patch silently
drops the keep_size argument from ops script.

I would even go further and suggest a command line option
to fsx that will fail unsupported scripted ops instead of
skipping them, because one might imagine that was the intention
of the author of an ops script. IMO that should be the default
behavior of fsx --replay-ops.

Amir.