Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false'

[Eric Blake <eblake@redhat.com>]

On 2/11/21 8:26 AM, Philippe Mathieu-Daudé wrote:
> We are going to switch the 'null-co' default 'read-zeroes' value
> from FALSE to TRUE in the next commit. First explicit the FALSE
> value when it is not set.

Grammar suggestion, along with a suggestion for an additional sentence
to make the intent of this commit clearer:

As a first step, request an explicit FALSE value rather than relying on
the defaults.  This is intended to be a purely mechanical adjustment for
no performance behavior in the tests; later patches may then flip or
elide the explicit choice for tests where performance does not matter.

> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> - Missing: 056 & 155. I couldn't figure out the proper syntax,
>   any help welcomed...

056 - looks like just one line:
        self.vm =
iotests.VM().add_drive_raw("file=blkdebug::null-co://,id=drive0,align=65536,driver=blkdebug")

the best way to add it here would be rewriting that line to use blockdev
syntax rather than blkdebug: URI syntax.  The other question is whether
it is a noticeable time difference when the default is flipped in 2/2.

155 - looks like several uses such as:

class TestBlockdevMirrorForcedBacking(MirrorBaseClass):
    cmd = 'blockdev-mirror'
    existing = True
    target_backing = None
    target_blockdev_backing = { 'driver': 'null-co' }
    target_real_backing = 'null-co://'


> - I'm unsure about 162, this doesn't seem to use the null-co
>   driver but rather testing global syntax.

Concur; I don't see any reason to worry about this one (but mentioning
it in the commit message can't hurt in case someone asks later)

# blkdebug expects all of its arguments to be strings, but its
# bdrv_refresh_filename() implementation should not assume that they
have been
# passed as strings in the original options QDict.
# So this should emit blkdebug:42:null-co:// as the filename:
touch 42
$QEMU_IMG info 'json:{"driver": "blkdebug", "config": 42,
                      "image.driver": "null-co"}' \


> ---
>  docs/devel/testing.rst                     | 14 +++++++-------
>  tests/qtest/fuzz/generic_fuzz_configs.h    | 11 ++++++-----
>  tests/test-bdrv-drain.c                    | 10 ++++++++--
>  tests/acceptance/virtio_check_params.py    |  2 +-
>  tests/perf/block/qcow2/convert-blockstatus |  6 +++---
>  tests/qemu-iotests/040                     |  2 +-

You did a pretty good hunt for culprits!

>  tests/qemu-iotests/041                     | 12 ++++++++----
>  tests/qemu-iotests/051                     |  2 +-
>  tests/qemu-iotests/051.out                 |  2 +-
>  tests/qemu-iotests/051.pc.out              |  4 ++--

and for the fallout to the iotests.

I did not audit for which tests are easy candidates for dropping the
explicit read-zeroes=false (that is, where the extra time in allowing
the flipped default doesn't penalize the test), but am okay giving this
patch:

Reviewed-by: Eric Blake <eblake@redhat.com>


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org