Re: [Qemu-devel] [PATCH v0 1/3] qcow2: introduce compression type feature

[Denis Plotnikov]

On 28.06.2019 12:54, Kevin Wolf wrote:
> Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
>> The patch adds some preparation parts for incompatible compression type
>> feature to QCOW2 header that indicates that *all* compressed clusters
>> must be (de)compressed using a certain compression type.
>>
>> It is implied that the compression type is set on the image creation and
>> can be changed only later by image conversion, thus compression type
>> defines the only compression algorithm used for the image.
>>
>> The goal of the feature is to add support of other compression algorithms
>> to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
>> It works roughly x2 faster than ZLIB providing a comparable compression ratio
>> and therefore provide a performance advantage in backup scenarios.
>>
>> The default compression is ZLIB. Images created with ZLIB compression type
>> is backward compatible with older qemu versions.
>>
>> Signed-off-by: Denis Plotnikov <address@hidden>
> 
>> @@ -3119,6 +3123,10 @@
>>   #                         an image, the data file name is loaded from the 
>> image
>>   #                         file. (since 4.0)
>>   #
>> +# @compression-type:      compression method to use for image clusters 
>> compression
>> +#                         The comression method is set on image creation 
>> and can
>> +#                         be changed via image converting only. (since 4.1)
>> +#
>>   # Since: 2.9
>>   ##
>>   { 'struct': 'BlockdevOptionsQcow2',
>> @@ -3134,7 +3142,8 @@
>>               '*refcount-cache-size': 'int',
>>               '*cache-clean-interval': 'int',
>>               '*encrypt': 'BlockdevQcow2Encryption',
>> -            '*data-file': 'BlockdevRef' } }
>> +            '*data-file': 'BlockdevRef',
>> +            '*compression-type': 'Qcow2CompressionType' } }
> 
> qcow2_open() doesn't actually parse this option (and it couldn't do
> anything useful with it because the image is fixed to a single
> compression type), so this shouldn't be added.

Yes, that's true. Will remove it.
> 
>>   ##
>>   # @SshHostKeyCheckMode:
>> @@ -4206,6 +4215,19 @@
>>     'data': [ 'v2', 'v3' ] }
>>   
>>   
>> +##
>> +# @Qcow2CompressionType:
>> +#
>> +# Compression type used in qcow2 image file
>> +#
>> +# @zlib     : gzip compressor
>> +# @zstd     : zstd compression
>> +#
>> +# Since: 4.1
>> +##
>> +{ 'enum': 'Qcow2CompressionType',
>> +  'data': [ 'zlib', 'zstd' ] }
> 
> I think it would be cleaner to start with only 'zlib' here, like your C
> code that doesn't implement anything for non-zlib compression types yet.
> 
> 'zstd' can be added to the enum when it's actually implemented. This
> will also make schema introspection provide the right information with a
> build that includes this patch, but not the zstd compression type
> patch.
Make sense. Will be fixed
> 
> Kevin
> 

-- 
Best,
Denis