Re: [Qemu-devel] [PATCH] file-posix: add rough-block-status parameter

[Eric Blake <eblake@redhat.com>]

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

On 1/8/19 1:45 PM, Vladimir Sementsov-Ogievskiy wrote:
> bdrv_co_block_status digs bs->file for additional, more accurate search
> for hole inside region, reported as DATA by bs since 5daa74a6ebc.
> 
> This accuracy is not free: assume we have qcow2 disk. Actually, qcow2
> knows, where are holes and where is data.

Not when it is has been preallocated (at which point the qcow2 metadata
says the entire file is data, even though much of it may still be zeroes).

> But every block_status
> 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.

This part is true, and we have had several complaints from various
places about the time spent doing lseek()s where the block layer fails
to cache what was learned on the previous iteration, coupled with
inefficient kernel/filesystem implementations where lseek is not as
cheap as it should be.

> 
> So, add an option to omit calling lseek.

The commit message would do well with an example showing the usage of
the parameter via a typical qemu-img command line that is currently
negatively impacted.  Or even better, where is the iotests change that
serves as a unit test of the new knob?

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> Hi all!
> 
> This is a continuation of "do not lseek in qcow2 block_status" thread
> (https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg05749.html)
> 
>  qapi/block-core.json |  6 +++++-
>  block/file-posix.c   | 13 ++++++++++++-
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 762000f31f..ec5549e5c2 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2645,6 +2645,9 @@
>  #                         migration.  May cause noticeable delays if the image
>  #                         file is large, do not use in production.
>  #                         (default: off) (since: 3.0)

Pre-existing: "off" isn't a valid QAPI bool; better would be fixing all
instances of this to "default: false".

> +# @rough-block-status: If set, on block-status querying assume that the whole
> +#                      file is data and do not try to find holes through system
> +#                      calls (default: off) (since 4.0)

You merely copied and pasted bad wording for the default section, but
you might as well fix it, whether or not we also fix the other instances
in a separate patch.

Possible better wording:
@rough-block-status: True to skip system calls that probe for holes when
querying for block status (default: false)

Bike-shedding: Since the point of this knob is to skip zero probing,
could we instead name it '@probe-zeroes: True to use lseek to probe for
holes (default: true)'?

>  #
>  # Since: 2.9
>  ##
> @@ -2653,7 +2656,8 @@
>              '*pr-manager': 'str',
>              '*locking': 'OnOffAuto',
>              '*aio': 'BlockdevAioOptions',
> -            '*x-check-cache-dropped': 'bool' } }
> +            '*x-check-cache-dropped': 'bool',
> +            '*rough-block-status': 'bool' } }
>  
>  ##
>  # @BlockdevOptionsNull:
> diff --git a/block/file-posix.c b/block/file-posix.c
> index d8f0b93752..3f6d76a5dc 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -167,6 +167,7 @@ typedef struct BDRVRawState {
>      bool has_fallocate;
>      bool needs_alignment;
>      bool check_cache_dropped;
> +    bool rough_block_status;
>  
>      PRManager *pr_mgr;
>  } BDRVRawState;
> @@ -439,6 +440,13 @@ static QemuOptsList raw_runtime_opts = {
>              .type = QEMU_OPT_BOOL,
>              .help = "check that page cache was dropped on live migration (default: off)"
>          },
> +        {
> +            .name = "rough-block-status",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "If set, on block-status querying assume that the whole "
> +                    "file is data and do not try to find holes through system "
> +                    "calls (default: off)"

Whatever wording changes we make to QAPI should be copied here.

The idea makes sense, but without unit test coverage, I'm reluctant to
leave R-b yet.

-- 
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 --]