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

[Eric Blake]

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).

> +++ 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.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org