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

[Vladimir Sementsov-Ogievskiy]

19.08.2019 18:00, 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
> 
> The test was conducted with brd disk to reduce the influence
> of disk subsystem to the test results.
> The results is given in seconds.
> 
> compress cmd:
>    time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
>                    src.img [zlib|zstd]_compressed.img
> decompress cmd
>    time ./qemu-img convert -O qcow2
>                    [zlib|zstd]_compressed.img uncompressed.img
> 
>             compression               decompression
>           zlib       zstd           zlib         zstd
> ------------------------------------------------------------
> real     65.5       16.3 (-75 %)    1.9          1.6 (-16 %)
> user     65.0       15.8            5.3          2.5
> sys       3.3        0.2            2.0          2.0
> 
> Both ZLIB and ZSTD gave the same compression ratio: 1.57
> compressed image size in both cases: 1.4G
> 
> Signed-off-by: Denis Plotnikov <address@hidden>
> ---
>   block/qcow2-threads.c  | 94 ++++++++++++++++++++++++++++++++++++++++++
>   block/qcow2.c          |  6 +++
>   configure              | 34 +++++++++++++++
>   docs/interop/qcow2.txt | 20 +++++++++
>   qapi/block-core.json   |  3 +-
>   5 files changed, 156 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> index 14b5bd76fb..85d04e6c2e 100644
> --- a/block/qcow2-threads.c
> +++ b/block/qcow2-threads.c
> @@ -28,6 +28,11 @@
>   #define ZLIB_CONST
>   #include <zlib.h>
>   
> +#ifdef CONFIG_ZSTD
> +#include <zstd.h>
> +#include <zstd_errors.h>
> +#endif
> +
>   #include "qcow2.h"
>   #include "block/thread-pool.h"
>   #include "crypto.h"
> @@ -165,6 +170,85 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t 
> dest_size,
>       return ret;
>   }
>   
> +#ifdef CONFIG_ZSTD
> +/*
> + * qcow2_zstd_compress()
> + *
> + * Compress @src_size bytes of data using zstd compression method
> + *
> + * @dest - destination buffer, @dest_size bytes
> + * @src - source buffer, @src_size bytes
> + *
> + * Returns: compressed size on success
> + *          -ENOMEM destination buffer is not enough to store compressed data
> + *          -EIO    on any other error
> + */
> +
> +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);
> +
> +    if (dest_size < sizeof(*c_size)) {
> +        return -ENOMEM;
> +    }
> +
> +    dest_size -= sizeof(*c_size);
> +
> +    ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);

Let's call this "5" to be something like QCOW2_DEFAUTLT_ZSTD_COMPRESSION_LEVEL, 
we'll
possibly want to add an option for this in future.

> +
> +    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 = ret;
> +
> +    return ret + sizeof(*c_size);
> +}
> +
> +/*
> + * qcow2_zstd_decompress()
> + *
> + * Decompress some data (not more than @src_size bytes) to produce exactly
> + * @dest_size bytes using zstd compression method
> + *
> + * @dest - destination buffer, @dest_size bytes
> + * @src - source buffer, @src_size bytes
> + *
> + * Returns: 0 on success
> + *          -EIO on any error
> + */
> +
> +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
> +     * the very beginning of the compressed buffer
> +     */
> +    const uint32_t *s_size = src;
> +    const char *s_buf = ((const char *) src) + sizeof(*s_size);

but what if *s_size (or even src_size) is corrupted? I think we need to check 
it before calling
ZSTD_decompress:

if (src_size < sizeof(*s_size) || src_size - sizeof(*s_size) < *s_size) {
    return -EIO;
}

with this (and optionally with QCOW2_DEFAUTLT_ZSTD_COMPRESSION_LEVEL):

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>


> +
> +    ret = ZSTD_decompress(dest, dest_size, s_buf, *s_size);
> +
> +    if (ZSTD_isError(ret)) {
> +        return -EIO;
> +    }
> +
> +    return 0;
> +}
> +#endif
> +

[...]




-- 
Best regards,
Vladimir