Re: [Qemu-devel] [PATCH v1 3/3] qcow2: add zstd cluster compression

[Denis Plotnikov]

On 03.07.2019 17:36, Eric Blake wrote:
> On 7/3/19 6:00 AM, Denis Plotnikov wrote:
>> zstd significantly reduces cluster compression time.
>> It provides better compression performance maintaining
>> the same level of compression ratio in comparison with
>> zlib, which, by the moment, has been the only compression
>> method available.
>>
> 
>> ---
>>   block/qcow2.c        | 96 ++++++++++++++++++++++++++++++++++++++++++++
>>   configure            | 32 +++++++++++++++
>>   qapi/block-core.json |  3 +-
>>   3 files changed, 130 insertions(+), 1 deletion(-)
> 
> Where is the change to docs/interop/qcow2.txt to describe this new
> compression format?
> 
ok
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 37a563a671..caa04b0beb 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -27,6 +27,11 @@
>>   #define ZLIB_CONST
>>   #include <zlib.h>
>>   
> 
>> +static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
>> +                                   const void *src, size_t src_size)
>> +{
>> +    ssize_t ret;
>> +    uint32_t *c_size = dest;
>> +    /* steal some bytes to store compressed chunk size */
>> +    char *d_buf = ((char *) dest) + sizeof(*c_size);
>> +
> 
> Do you always want exactly 4 bytes for the compressed size? Or is it
> worth some sort of variable-length encoding, since we're already dealing
> with non-cacheline-aligned data? You can represent all sizes up to 4M
> using a maximum of 3 bytes (set the high bit if the integer continues,
> then sizes 0-127 take 1 byte [7 bits], 128-32767 take 2 bytes [15 bits],
> and 32768-4194303 take 3 bytes [22 bits]).
Is it really worth to do that? I liked Kevin's comment that the size 
should complain with some variable size
> 
>> +    if (dest_size < sizeof(*c_size)) {
>> +        return -ENOMEM;
>> +    }
>> +
>> +    dest_size -= sizeof(*c_size);
>> +
>> +    ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
> 
> The fact that you are storing the size separate from the data passed to
> zstd MUST be documented in the qcow2 spec, for the next person to
> produce/consume the same data.
Should I add a separate chapter there?
> 
> 
>> +++ b/qapi/block-core.json
>> @@ -4215,11 +4215,12 @@
>>   # Compression type used in qcow2 image file
>>   #
>>   # @zlib:  zlib compression, see <http://zlib.net/>
>> +# @zstd:  zstd compression, see <http://github.com/facebook/zstd>
>>   #
>>   # Since: 4.1
>>   ##
>>   { 'enum': 'Qcow2CompressionType',
>> -  'data': [ 'zlib' ] }
>> +  'data': [ 'zlib', 'zstd' ] }
> 
> Since you patched configure so that linking against zstd is optional,
> this should use { 'name':'zstd', 'if':'CONDITIONAL' } so that during
> introspection, the enum only advertises zstd on a build that linked
> against the library.
Sure, will change it
> 

-- 
Best,
Denis