Re: [Qemu-devel] [PATCH] block: don't probe zeroes in bs->file by default on block_status

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3852 bytes --]

On 1/10/19 7:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> drv_co_block_status digs bs->file for additional, more accurate search
> for hole inside region, reported as DATA by bs since 5daa74a6ebc.

s/region, reported/regions reported/

> 
> This accuracy is not free: assume we have qcow2 disk. Actually, qcow2
> knows, where are holes and where is data. But every block_status

Not quite true. qcow2 knows where some holes are, but does not know if
there are even more holes hiding in the sections marked data (such as
because of how the disk was pre-allocated).

> request calls lseek additionally. Assume a big disk, full of
> data, in any iterative copying block job (or img convert) we'll call
> lseek(HOLE) on every iteration, and each of these lseeks will have to
> iterate through all metadata up to the end of file. It's obviously
> ineffective behavior. And for many scenarios we don't need this lseek
> at all.

How much performance can we buy back without any knobs at all, if we
just taught posix-file.c to cache lseek() results?  That is, when
visiting a file sequentially, if lseek(fd, 0, SEEK_HOLE) returns EOF on
our first block status query, then all subsequent block status queries
fall within what we know to be data, and we can skip the lseek() calls.

> 
> So, let's "5daa74a6ebc" by default, leaving an option to return

s/let's/let's undo/

> previous behavior, which is needed for scenarios with preallocated
> images.
> 
> Add iotest illustrating new option semantics.

And none of the existing iotests fail due to changed output?  Does that
mean our testsuite does not have good coverage of pre-allocation modes
where the zero probe mattered?

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 

> +++ b/qapi/block-core.json
> @@ -3673,6 +3673,8 @@
>  #                 (default: off)
>  # @force-share:   force share all permission on added nodes.
>  #                 Requires read-only=true. (Since 2.10)
> +# @probe-zeroes:  Probe zeroes on protocol layer if format reports data
> +#                 (default: false) (since 4.0)

Why do we need a new bool?  Can we instead...

>  #
>  # Remaining options are determined by the block driver.
>  #
> @@ -3686,7 +3688,8 @@
>              '*read-only': 'bool',
>              '*auto-read-only': 'bool',
>              '*force-share': 'bool',
> -            '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
> +            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
> +            '*probe-zeroes': 'bool' },

...add another enum value to 'detect-zeroes', since after all, what we
are really doing is fine-tuning what level of zero probing we are
willing to do?


> +++ b/qemu-options.hx
> @@ -615,6 +615,7 @@ DEF("blockdev", HAS_ARG, QEMU_OPTION_blockdev,
>      "-blockdev [driver=]driver[,node-name=N][,discard=ignore|unmap]\n"
>      "          [,cache.direct=on|off][,cache.no-flush=on|off]\n"
>      "          [,read-only=on|off][,detect-zeroes=on|off|unmap]\n"
> +    "          [,probe-zeroes=on|off]\n"
>      "          [,driver specific parameters...]\n"
>      "                configure a block backend\n", QEMU_ARCH_ALL)
>  STEXI
> @@ -670,6 +671,8 @@ discard requests.
>  conversion of plain zero writes by the OS to driver specific optimized
>  zero write commands. You may even choose "unmap" if @var{discard} is set
>  to "unmap" to allow a zero write to be converted to an @code{unmap} operation.
> +@item probe-zeroes=@var{probe-zeroes}
> +Probe zeroes on protocol layer if format reports data. Default is "off".

Again, fitting this into detect-zeroes seems better than inventing a new
knob.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]