[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false'
Date: Fri, 12 Feb 2021 14:34:12 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1

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 =

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

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 
 - this patch creates extra conflicts on any patch porting between 
 - 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 

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]