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

[Denis Plotnikov]

On 04.09.2019 19:07, Eric Blake wrote:
> On 9/4/19 10:29 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, at the moment, has been the only compression
>> method available.
>>
>> The performance test results:
>> Test compresses and decompresses qemu qcow2 image with just
>> installed rhel-7.6 guest.
>> Image cluster size: 64K. Image on disk size: 2.2G
>>
>> Signed-off-by: Denis Plotnikov <address@hidden>
>> ---
>> +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;
> Potentially unaligned pointer...
>
>> +    /* steal some bytes to store compressed chunk size */
>> +    char *d_buf = ((char *) dest) + sizeof(*c_size);
>> +
>> +    /* sanity check that we can store the compressed data length */
>> +    if (dest_size < sizeof(*c_size)) {
>> +        return -ENOMEM;
>> +    }
>> +
>> +    dest_size -= sizeof(*c_size);
>> +
>> +    ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
>> +
>> +    if (ZSTD_isError(ret)) {
>> +        if (ZSTD_getErrorCode(ret) == ZSTD_error_dstSize_tooSmall) {
>> +            return -ENOMEM;
>> +        } else {
>> +            return -EIO;
>> +        }
>> +    }
>> +
>> +    /* store the compressed chunk size in the very beginning of the buffer 
>> */
>> +    *c_size = cpu_to_be32(ret);
> ...and you are storing into it.  You are using the wrong conversion
> function; you want stl_be_p(dest, ret) or similar.
>
>> +static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size,
>> +                                     const void *src, size_t src_size)
>> +{
>> +    ssize_t ret;
>> +    /*
>> +     * zstd decompress wants to know the exact length of the data
>> +     * for that purpose, on the compression the length is stored in
> s/data for/data. For/
> s/on the compression/on compression,/
>
>> +     * the very beginning of the compressed buffer
>> +     */
>> +    uint32_t s_size;
>> +    const char *s_buf = ((const char *) src) + sizeof(s_size);
>> +
>> +    /* sanity check that we can read the content length */
>> +    if (src_size < sizeof(s_size)) {
> Should this use <=?  After all, I seriously doubt you can get a 0-byte
> compression stream.
>
>> +        return -EIO;
>> +    }
>> +
>> +    s_size = be32_to_cpu(*(const uint32_t *) src);
> As written, this looks like you may be dereferencing an unaligned
> pointer.  It so happens that be32_to_cpu() applies & to your * to get
> back at the raw pointer, and then is careful to handle unaligned
> pointers, so it works; but it would look a lot nicer as merely:
>
> s_size = be32_to_cpu(src);
>
>> +
>> +    /* sanity check that the buffer is big enough to read the content */
>> +    if (src_size - sizeof(s_size) < s_size) {
> Why < and not !=?  As written, you are silently ignoring trailing
> garbage, instead of treating it as a client that did not write the data
> correctly.

but src_size (512) might be bigger than the content of the compressed 
data, don't it?

Denis

>
>> +++ b/docs/interop/qcow2.txt
>> @@ -181,6 +181,7 @@ in the description of a field.
>>                       must be set.
>>                       Available compression type values:
>>                           0: zlib <https://www.zlib.net/> (default)
>> +                        1: zstd <http://github.com/facebook/zstd>
> At this point, this listing is almost redundant with the more-detailed
> header below.  Maybe it is worth just forward referencing that section
> for a listing of valid values, and then mentioning the values 0 and 1 in
> that section?
>
>>   
>>   Directly after the image header, optional sections called header 
>> extensions can
>>   be stored. Each extension has a structure like the following:
>> @@ -536,6 +537,9 @@ Compressed Clusters Descriptor (x = 62 - (cluster_bits - 
>> 8)):
>>                       Another compressed cluster may map to the tail of the 
>> final
>>                       sector used by this compressed cluster.
>>   
>> +                    The layout of the compressed data depends on the 
>> compression
>> +                    type used for the image (see compressed cluster layout).
>> +
>>   If a cluster is unallocated, read requests shall read the data from the 
>> backing
>>   file (except if bit 0 in the Standard Cluster Descriptor is set). If there 
>> is
>>   no backing file or the backing file is smaller than the image, they shall 
>> read
>> @@ -788,3 +792,19 @@ In the image file the 'enabled' state is reflected by 
>> the 'auto' flag. If this
>>   flag is set, the software must consider the bitmap as 'enabled' and start
>>   tracking virtual disk changes to this bitmap from the first write to the
>>   virtual disk. If this flag is not set then the bitmap is disabled.
>> +
>> +=== Compressed cluster layout ===
> As written, you have made this a child to '== Bitmaps ==' (and sibling
> to '=== Dirty tracking bitmaps ==='); that feels wrong.  I would place
> this subsection belong under '== Cluster mapping ==' right after the
> 'Compressed Clusters Descriptor'.
>
>> +
>> +The compressed cluster data may have a different layout depending on the
>> +compression type used for the image, and store specific data for the 
>> particular
>> +compression type.
>> +
>> +Compressed data layout for the available compression types:
> Wordy; maybe:
>
> The compressed cluster data has a layout depending on the compression
> type used for the image, as follows:
>
>> +(x = data_space_length - 1)
>> +
>> +    zlib <http://zlib.net/>:
>> +        Byte  0 -  x:     the compressed data content
>> +                          all the space provided used for compressed data
> Worth a mention that this is compression type 0?
>
>> +    zstd <http://github.com/facebook/zstd>:
>> +        Byte  0 -  3:     the length of compressed data in bytes
>> +              4 -  x:     the compressed data content
> Worth a mention that this is compression type 1?
>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 2c002ca6a9..9e458d5b40 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -4283,11 +4283,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.2
>>   ##
>>   { 'enum': 'Qcow2CompressionType',
>> -  'data': [ 'zlib' ] }
>> +  'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
>>   
>>   ##
>>   # @BlockdevCreateOptionsQcow2:
>>