Re: [PATCH v1 3/8] qcow2: add zstd cluster compression

[Denis Plotnikov <dplotnikov@virtuozzo.com>]

On 27.02.2020 17:01, Eric Blake wrote:
> On 2/27/20 1:29 AM, Denis Plotnikov wrote:
>> zstd significantly reduces cluster compression time.
>> It provides better compression performance maintaining
>> the same level of the compression ratio in comparison with
>> zlib, which, at the moment, is the only compression
>> method available.
>>
>
>> +static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
>> +                                   const void *src, size_t src_size)
>> +{
>> +    size_t ret;
>> +
>> +    /*
>> +     * steal ZSTD_LEN_BUF bytes in the very beginng of the buffer
>
> beginning
>
>> +     * to store compressed chunk size
>> +     */
>> +    char *d_buf = ((char *) dest) + ZSTD_LEN_BUF;
>> +
>> +    /*
>> +     * sanity check that we can store the compressed data length,
>> +     * and there is some space left for the compressor buffer
>> +     */
>> +    if (dest_size <= ZSTD_LEN_BUF) {
>> +        return -ENOMEM;
>> +    }
>> +
>> +    dest_size -= ZSTD_LEN_BUF;
>> +
>> +    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;
>> +        }
>> +    }
>> +
>> +    /* paraniod sanity check that we can store the commpressed size */
>
> paranoid, compressed
>
>> +    if (ret > UINT_MAX) {
>> +        return -ENOMEM;
>> +    }
>
> This is pointless.  Better is to ensure that we actually compressed 
> data (the pigeonhole principle states that there are some inputs that 
> MUST result in inflation, in order for most other inputs to result in 
> compression).  But that check was satisfied by checking for 
> ZSTD_error_dstSize_tooSmall, which is what happens for one of those 
> uncompressible inputs.  Namely, zstd will never return a result larger 
> than dest_size, and since dest_size is smaller than UINT_MAX on entry, 
> this check is pointless.  But if you want something, I'd be okay with: 
> assert(ret <= dest_size).
Taking into account that this is "just in case" and I'm trying to 
protect the first 4 bytes of the buffer from the overflow and
I can't imagine the situation when we deal with cluster sizes greater 
than UINT32_MAX but the input size is size_t which can be > UINT32_MAX 
on 64bit archs.
I'd rather stick with
     if (ret > UINT32_MAX) {
         return -ENOMEM;
     }
as Vladimir suggested.

I'm not sure that the assert is good here, since it stops the system 
operating and this isn't potential error but a limitation
Does it work for you?

Denis
>
>> +++ b/docs/interop/qcow2.txt
>> @@ -208,6 +208,7 @@ version 2.
>>                         Available compression type values:
>>                           0: zlib <https://www.zlib.net/>
>> +                        1: zstd <http://github.com/facebook/zstd>
>>       === Header padding ===
>> @@ -575,11 +576,28 @@ 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
>>   zeros for all parts that are not covered by the backing file.
>>   +=== Compressed Cluster Layout ===
>> +
>> +The compressed cluster data has a layout depending on the compression
>> +type used for the image, as follows:
>> +
>> +Compressed data layout for the available compression types:
>> +(x = data_space_length - 1)
>> +
>> +    0:  (default)  zlib <http://zlib.net/>:
>> +            Byte  0 -  x:     the compressed data content
>> +                              all the space provided used for 
>> compressed data
>> +    1:  zstd <http://github.com/facebook/zstd>:
>> +            Byte  0 -  3:     the length of compressed data in bytes
>> +                  4 -  x:     the compressed data content
>>     == Snapshots ==
>>   diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 873fbef3b5..4b6e576c44 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -4401,11 +4401,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: 5.0
>>   ##
>>   { 'enum': 'Qcow2CompressionType',
>> -  'data': [ 'zlib' ] }
>> +  'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } 
>> ] }
>
> The spec and UI changes are okay.
>