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

[Denis Plotnikov]

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.