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

[Vladimir Sementsov-Ogievskiy]

12.02.2021 01:40, Eric Blake wrote:
> 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>

I'd keep tests/qemu-iotests as is..

Benefits of keeping old behavior (by adding read-zeroes=false)

 - saving several milliseconds. I doubt that it would be even noticeable. Could you give 
have much time of "make check-block" it saves, if we update all iotests to use 
read-zeroes=false, instead of transparently move to new default?

Drawbacks of adding read-zeroes=false:

 - it's generally better to test only default behavior than only non-default 
behavior
 - this patch creates extra conflicts on any patch porting between 
upstream/downstream
 - for new tests, nobady will care about read-zeroes and will not add it.. (or 
will add when copy existing test no thinking about it), so it will be a kind of 
mess

--
Best regards,
Vladimir