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

[Philippe Mathieu-Daudé]

On 2/11/21 11:40 PM, 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>

Thanks for your help. I'll address your comments and respin.

Regards,

Phil.