Re: [Qemu-devel] [PATCH 08/11] drive-backup: added support for data compression

[Pavel Butsykin <pbutsykin@virtuozzo.com>]

On 13.06.2016 23:18, Eric Blake wrote:
> On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>
>> The idea is simple - backup is "written-once" data. It is written block
>> by block and it is large enough. It would be nice to save storage
>> space and compress it.
>>
>> The patch adds a flag to the qmp/hmp drive-backup command which enables
>> block compression. Compression should be implemented in the format driver
>> to enable this feature.
>>
>> There are some limitations of the format driver to allow compressed writes.
>> We can write data only once. Though for backup this is perfectly fine.
>> These limitations are maintained by the driver and the error will be
>> reported if we are doing something wrong.
>>
>
>> @@ -3309,6 +3315,7 @@ void qmp_drive_backup(const char *device, const char *target,
>>                         bool has_mode, enum NewImageMode mode,
>>                         bool has_speed, int64_t speed,
>>                         bool has_bitmap, const char *bitmap,
>> +                      bool has_compress, bool compress,
>>                         bool has_on_source_error, BlockdevOnError on_source_error,
>>                         bool has_on_target_error, BlockdevOnError on_target_error,
>>                         Error **errp)
>
> Might be nice to simplify this signature once my qapi 'box' patches land.
>

Yes, it would certainly be better. But your patches are not applied on
the current, maybe you have your own branch?

>
>> +++ b/qapi/block-core.json
>> @@ -888,6 +888,9 @@
>>   #          Must be present if sync is "incremental", must NOT be present
>>   #          otherwise. (Since 2.4)
>>   #
>> +# @compress: #optional the compression data blocks (if the target format
>> +#            supports it; default: false).
>
> Missing a '(since 2.7)' notation.
>
>> +++ b/qmp-commands.hx
>> @@ -1186,7 +1186,8 @@ EQMP
>>       {
>>           .name       = "drive-backup",
>>           .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
>> -                      "bitmap:s?,on-source-error:s?,on-target-error:s?",
>> +                      "bitmap:s?,compress:b?,"
>> +                      "on-source-error:s?,on-target-error:s?",
>>           .mhandler.cmd_new = qmp_marshal_drive_backup,
>>       },
>>
>> @@ -1220,6 +1221,8 @@ Arguments:
>>   - "mode": whether and how QEMU should create a new image
>>             (NewImageMode, optional, default 'absolute-paths')
>>   - "speed": the maximum speed, in bytes per second (json-int, optional)
>> +- "compress": the compression data blocks (if the target format supports it).
>> +              (json-bool, optional, default false)
>
> Reads awkwardly; maybe "compress": true to compress data, if the target
> format supports it. (json-bool, optional, default false)
>
>