[PATCH trivial] tests/qemu-iotests/tests/mirror-sparse: skip if O_DIRECT is not supported

[PATCH trivial] tests/qemu-iotests/tests/mirror-sparse: skip if O_DIRECT is not supported

[Michael Tokarev]

This test uses cache.direct=true, but does not check if O_DIRECT
is supported by the underlying filesystem, and fails, for example,
on a tmpfs (which is rather common on various auto-builders, in CI,
etc).

Fix this by using _require_o_direct.

This example shows where our testing framework is significantly
lacking.  In this test, qemu produces an error message on stderr
at startup, because it can't use O_DIRECT mode.  But this error
message is not shown anywhere at all, even when running this test
separately outside of meson framework, - stderr is completely
hidden, and the only error we're getting is

 +Timeout waiting for capabilities on handle 0

so it's rather painful to find what the actual error is.  I think
that besides this change, we should also change the testing framework
to show stderr at least in case of test failure, and especially when
the failure occurs at the very beginning when we're checking for
sanity.

Fixes: c0ddcb2cbc146e "tests: Add iotest mirror-sparse for recent patches"
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 tests/qemu-iotests/tests/mirror-sparse | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/tests/mirror-sparse b/tests/qemu-iotests/tests/mirror-sparse
index cfcaa600ab..19843a622c 100755
--- a/tests/qemu-iotests/tests/mirror-sparse
+++ b/tests/qemu-iotests/tests/mirror-sparse
@@ -41,6 +41,7 @@ _supported_fmt qcow2 raw  # Format of the source. dst is always raw file
 _supported_proto file
 _supported_os Linux
 _require_disk_usage
+_require_o_direct
 
 echo
 echo "=== Initial image setup ==="
-- 
2.47.2

Re: [PATCH-for-10.1] tests/qemu-iotests/tests/mirror-sparse: skip if O_DIRECT is not supported

[Philippe Mathieu-Daudé]

On 1/8/25 14:28, Michael Tokarev wrote:
> This test uses cache.direct=true, but does not check if O_DIRECT
> is supported by the underlying filesystem, and fails, for example,
> on a tmpfs (which is rather common on various auto-builders, in CI,
> etc).
> 
> Fix this by using _require_o_direct.
> 
> This example shows where our testing framework is significantly
> lacking.  In this test, qemu produces an error message on stderr
> at startup, because it can't use O_DIRECT mode.  But this error
> message is not shown anywhere at all, even when running this test
> separately outside of meson framework, - stderr is completely
> hidden, and the only error we're getting is
> 
>   +Timeout waiting for capabilities on handle 0
> 
> so it's rather painful to find what the actual error is.  I think
> that besides this change, we should also change the testing framework
> to show stderr at least in case of test failure, and especially when
> the failure occurs at the very beginning when we're checking for
> sanity.
> 
> Fixes: c0ddcb2cbc146e "tests: Add iotest mirror-sparse for recent patches"
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>   tests/qemu-iotests/tests/mirror-sparse | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/tests/qemu-iotests/tests/mirror-sparse b/tests/qemu-iotests/tests/mirror-sparse
> index cfcaa600ab..19843a622c 100755
> --- a/tests/qemu-iotests/tests/mirror-sparse
> +++ b/tests/qemu-iotests/tests/mirror-sparse
> @@ -41,6 +41,7 @@ _supported_fmt qcow2 raw  # Format of the source. dst is always raw file
>   _supported_proto file
>   _supported_os Linux
>   _require_disk_usage
> +_require_o_direct

Could the correct use be:

   _supported_cache_modes none directsync

?

>   
>   echo
>   echo "=== Initial image setup ==="

Re: [PATCH-for-10.1] tests/qemu-iotests/tests/mirror-sparse: skip if O_DIRECT is not supported

[Michael Tokarev]

On 05.08.2025 20:23, Philippe Mathieu-Daudé wrote:

>> diff --git a/tests/qemu-iotests/tests/mirror-sparse b/tests/qemu- 
>> iotests/tests/mirror-sparse
>> index cfcaa600ab..19843a622c 100755
>> --- a/tests/qemu-iotests/tests/mirror-sparse
>> +++ b/tests/qemu-iotests/tests/mirror-sparse
>> @@ -41,6 +41,7 @@ _supported_fmt qcow2 raw  # Format of the source. 
>> dst is always raw file
>>   _supported_proto file
>>   _supported_os Linux
>>   _require_disk_usage
>> +_require_o_direct
> 
> Could the correct use be:
> 
>    _supported_cache_modes none directsync

Yes that works too.  I've no idea which is "better" - we've
a bit too many options here, I think.  I'll change it to your
suggestion.

Thanks,

/mjt

Re: [PATCH-for-10.1] tests/qemu-iotests/tests/mirror-sparse: skip if O_DIRECT is not supported

[Kevin Wolf]

Am 05.08.2025 um 19:56 hat Michael Tokarev geschrieben:
> On 05.08.2025 20:23, Philippe Mathieu-Daudé wrote:
> 
> > > diff --git a/tests/qemu-iotests/tests/mirror-sparse b/tests/qemu-
> > > iotests/tests/mirror-sparse
> > > index cfcaa600ab..19843a622c 100755
> > > --- a/tests/qemu-iotests/tests/mirror-sparse
> > > +++ b/tests/qemu-iotests/tests/mirror-sparse
> > > @@ -41,6 +41,7 @@ _supported_fmt qcow2 raw  # Format of the source.
> > > dst is always raw file
> > >   _supported_proto file
> > >   _supported_os Linux
> > >   _require_disk_usage
> > > +_require_o_direct
> > 
> > Could the correct use be:
> > 
> >    _supported_cache_modes none directsync
> 
> Yes that works too.  I've no idea which is "better" - we've
> a bit too many options here, I think.  I'll change it to your
> suggestion.

No, _require_o_direct is better because it directly checks if files in
the scratch directory support O_DIRECT, which is what we need here
because the test unconditionally opens the file this way:

    -blockdev '{"driver":"file", "cache":{"direct":true, "no-flush":false},
                "filename":"'"$TEST_IMG.base"'", "node-name":"src-file"}' \

_supported_cache_modes is about the cache mode requested on the command
line when running qemu-iotests, which is not what we're interested in.
The relevant call doesn't even consider the command line option. It
still "fixes" the failure because requesting "none" or "directsync"
makes it do the O_DIRECT check, too.

But the effect is different: With "_supported_cache_modes none
directsync", the test will always be skipped if on the command line
"writeback" was requested (it's the default, so we'll skip the test by
default - that's a bad idea). With _require_o_direct it will only be
skipped if the filesystem really doesn't support O_DIRECT.

Kevin

Re: [PATCH-for-10.1] tests/qemu-iotests/tests/mirror-sparse: skip if O_DIRECT is not supported

[Philippe Mathieu-Daudé]

On 6/8/25 09:30, Kevin Wolf wrote:
> Am 05.08.2025 um 19:56 hat Michael Tokarev geschrieben:
>> On 05.08.2025 20:23, Philippe Mathieu-Daudé wrote:
>>
>>>> diff --git a/tests/qemu-iotests/tests/mirror-sparse b/tests/qemu-
>>>> iotests/tests/mirror-sparse
>>>> index cfcaa600ab..19843a622c 100755
>>>> --- a/tests/qemu-iotests/tests/mirror-sparse
>>>> +++ b/tests/qemu-iotests/tests/mirror-sparse
>>>> @@ -41,6 +41,7 @@ _supported_fmt qcow2 raw  # Format of the source.
>>>> dst is always raw file
>>>>    _supported_proto file
>>>>    _supported_os Linux
>>>>    _require_disk_usage
>>>> +_require_o_direct
>>>
>>> Could the correct use be:
>>>
>>>     _supported_cache_modes none directsync
>>
>> Yes that works too.  I've no idea which is "better" - we've
>> a bit too many options here, I think.  I'll change it to your
>> suggestion.
> 
> No, _require_o_direct is better because it directly checks if files in
> the scratch directory support O_DIRECT, which is what we need here
> because the test unconditionally opens the file this way:
> 
>      -blockdev '{"driver":"file", "cache":{"direct":true, "no-flush":false},
>                  "filename":"'"$TEST_IMG.base"'", "node-name":"src-file"}' \
> 
> _supported_cache_modes is about the cache mode requested on the command
> line when running qemu-iotests, which is not what we're interested in.
> The relevant call doesn't even consider the command line option. It
> still "fixes" the failure because requesting "none" or "directsync"
> makes it do the O_DIRECT check, too.
> 
> But the effect is different: With "_supported_cache_modes none
> directsync", the test will always be skipped if on the command line
> "writeback" was requested (it's the default, so we'll skip the test by
> default - that's a bad idea). With _require_o_direct it will only be
> skipped if the filesystem really doesn't support O_DIRECT.

Oops, sorry for the bad suggestion :/