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

[Peter Lieven <pl@kamp.de>]

On 07.05.2014 04:52, Eric Blake wrote:
> 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?

Actually I have no idea. As you pointed out in the qapi patch I sent
it was quite hard for me to crawl through the whole stuff as one who is not
familiar with it. Can somebody advise here? Anyhow, I wonder
how this would work since qapi doesn't know the C Macros.

>
>> +++ 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).

I tried to be consistent here. All "setters" use a dash while all "getters"
have an underscore. Look e.g. at all the I/O thortteling parameters.
One reason might be that a dash is not allowed as a member name in a struct.

 From this perspective the only inconsistent one is node-name.

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

I tried to, however it did not compile for me when I tried this.

>
> 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".

The reason I choosed zeroEs is that the function in the background
is named bdrv_write_zeroEs.

>
>>               '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.

Ok

>
>> +#
>> +# @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.

Will do.

Peter

>
>> +# @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.
>