Re: [Qemu-devel] [PATCHv3] block: optimize zero writes with bdrv_write_zeroes

[Eric Blake <eblake@redhat.com>]

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

On 05/06/2014 06:23 PM, Peter Lieven wrote:
> this patch tries to optimize zero write requests
> by automatically using bdrv_write_zeroes if it is
> supported by the format.
> 
> This significantly speeds up file system initialization and
> should speed zero write test used to test backend storage
> performance.
> 

> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
> v2->v3: - moved parameter parsing to blockdev_init
>         - added per device detect_zeroes status to 
>           hmp (info block -v) and qmp (query-block) [Eric]
>         - added support to enable detect-zeroes also
>           for hot added devices [Eric]
>         - added missing entry to qemu_common_drive_opts
>         - fixed description of qemu_iovec_is_zero [Fam]
> 

>  
> +static BdrvDetectZeroes parse_detect_zeroes(const char *buf, Error **errp)
> +{
> +    if (!buf || !strcmp(buf, "off")) {
> +        return BDRV_DETECT_ZEROES_OFF;
> +    } else if (!strcmp(buf, "on")) {
> +        return BDRV_DETECT_ZEROES_ON;
> +    } else if (!strcmp(buf, "unmap")) {
> +        return BDRV_DETECT_ZEROES_UNMAP;
> +    } else {
> +        error_setg(errp, "invalid value for detect-zeroes: %s",
> +                   buf);
> +    }
> +    return BDRV_DETECT_ZEROES_OFF;
> +}

Isn't there QAPI generated code that you can use instead of open-coding
this conversion between string and enum values?

> +++ b/qapi-schema.json
> @@ -937,6 +937,8 @@
>  # @encryption_key_missing: true if the backing device is encrypted but an
>  #                          valid encryption key is missing
>  #
> +# @detect_zeroes: detect and optimize zero writes (Since 2.1)
> +#
>  # @bps: total throughput limit in bytes per second is specified
>  #
>  # @bps_rd: read throughput limit in bytes per second is specified
> @@ -972,6 +974,7 @@
>    'data': { 'file': 'str', '*node-name': 'str', 'ro': 'bool', 'drv': 'str',
>              '*backing_file': 'str', 'backing_file_depth': 'int',
>              'encrypted': 'bool', 'encryption_key_missing': 'bool',
> +            'detect_zeroes': 'str',

For new additions, we try to prefer dash over underscore.  Eww - we've
already been inconsistent in this struct, with backing_file vs. node-name.

Maybe s/detect_zeroes/detect-zeroes/.  I obviously can't argue too
strongly in light of the rest of the struct in isolation.  However, you
DID use detect-zeroes on the input side later in the patch, so being
consistent between the two sites would be nice (given that node-name was
named consistently).

On the other hand, I _can_ argue strongly that open-coding this as 'str'
is wrong.  You already added an enum type, so use it:

'detect_zeroes': 'BlockdevDetectZeroesOptions',

(hmm, in this context, it's not really an option, so maybe there is some
other name we can bikeshed about; but beyond 'BlockdevDetectZeroes', I
don't have any other good ideas)

zero is one of those odd words with two different pluralized spellings
both in common use.  Code base currently has a 1:2 ratio between 'zeros'
vs. 'zeroes', so I guess you're okay; on the other hand, 'man qemu-img'
documents that 'convert -S' detects "zeros".

>              'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
>              'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
>              'image': 'ImageInfo',
> @@ -4250,6 +4253,20 @@
>    'data': [ 'ignore', 'unmap' ] }
>  
>  ##
> +# @BlockdevDetectZeroesOptions
> +#
> +# Selects the operation mode for zero write detection.

s/Selects/Describes/ since you are also going to use this enum on the
output field.

> +#
> +# @off:      Disabled
> +# @on:       Enabled

Maybe more details?  Seeing this doesn't tell me the tradeoffs involved
in tweaking the parameter (one is faster, while the other uses less
storage resources - so maybe mention those benefits).  I see the
documentation later on for the command line, so maybe repeating some of
that here would help someone going by just the QMP interface.

> +# @unmap:    Enabled and even try to unmap blocks if possible
> +#
> +# Since: 2.1
> +##
> +{ 'enum': 'BlockdevDetectZeroesOptions',
> +  'data': [ 'off', 'on', 'unmap' ] }
> +
> +##
>  # @BlockdevAioOptions
>  #
>  # Selects the AIO backend to handle I/O requests

> @@ -4327,7 +4346,8 @@
>              '*aio': 'BlockdevAioOptions',
>              '*rerror': 'BlockdevOnError',
>              '*werror': 'BlockdevOnError',
> -            '*read-only': 'bool' } }
> +            '*read-only': 'bool',
> +            '*detect-zeroes': 'BlockdevDetectZeroesOptions' } }

This part is fine.

>  @var{copy-on-read} is "on" or "off" and enables whether to copy read backing
>  file sectors into the image file.
> +@item detect-zeroes=@var{detect-zeroes}
> +@var{detect-zeroes} is "off", "on" or "unmap" and enables the automatic
> +conversion of plain zero writes by the OS to driver specific optimized
> +zero write commands. If "unmap" is chosen and @var{discard} is "on"
> +a zero write may even be converted to an UNMAP operation.

This looks good.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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