[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: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH v2 1/2] block: Explicit null-co uses 'read-zeroes=false' |
Date: |
Fri, 12 Feb 2021 00:49:38 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 |
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.