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

[Kevin Wolf <kwolf@redhat.com>]

Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
> zstd significantly reduces cluster compression time.
> It provides better compression performance maintaining
> the same level of compression ratio in comparison with
> zlib, which, by 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 <dplotnikov@virtuozzo.com>
> ---
>  block/qcow2.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  configure     | 26 ++++++++++++++++
>  2 files changed, 108 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 90f15cc3c9..58901f9f79 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -26,6 +26,7 @@
>  
>  #define ZLIB_CONST
>  #include <zlib.h>
> +#include <zstd.h>
>  
>  #include "block/block_int.h"
>  #include "block/qdict.h"
> @@ -1553,6 +1554,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>      case QCOW2_COMPRESSION_TYPE_ZLIB:
>          break;
>  
> +    case QCOW2_COMPRESSION_TYPE_ZSTD:
> +        break;

If we don't intend to add any code here, why not just add another case
label to the existing break?

>      default:
>          error_setg(errp, "Unknown compression type");
>          ret = -EINVAL;
> @@ -3286,6 +3290,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
>           *  ZLIB shouldn't be here since it's the default
>           */
>          switch (qcow2_opts->compression_type) {
> +        case QCOW2_COMPRESSION_TYPE_ZSTD:
> +            break;
> +
>          default:
>              error_setg_errno(errp, -EINVAL, "Unknown compression type");
>              goto out;
> @@ -4113,6 +4120,73 @@ static ssize_t zlib_decompress(void *dest, size_t dest_size,
>      return ret;
>  }
>  
> +/*
> + * zstd_compress()
> + *
> + * @dest - destination buffer, @dest_size bytes
> + * @src - source buffer, @src_size bytes
> + *
> + * Returns: compressed size on success
> + *          -1 on any error
> + */
> +
> +static ssize_t zstd_compress(void *dest, size_t dest_size,
> +                             const void *src, size_t src_size)
> +{
> +    /* steal some bytes to store compressed chunk size */
> +    size_t ret;

This ends up in a file, so this needs to be a fixed number of bytes.
size_t varies between different platforms, so it is not acceptable.

If I understand correctly, the maximum for this is the cluster size, so
uint32_t should be right.

This isn't plain the zstd compression format any more, so it needs to be
described in the qcow2 spec.

> +    size_t *c_size = dest;
> +    char *d_buf = dest;
> +    d_buf += sizeof(ret);

char *d_bug = dest + sizeof(ret);

> +    dest_size -= sizeof(ret);

We don't want to end up with an integer overflow, so before this:

    if (dest_size < sizeof(ret)) {
        return -ENOMEM;
    }

> +    ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
> +
> +    if (ZSTD_isError(ret)) {
> +        return -1;
> +    }

Need an error code here, not just -1. In particular, we need to
distinguish cases where the buffer was too small and uncompressed data
should be written instead (ENOMEM) from real errors that should be
returned to the caller (EIO).

> +
> +    /* store the compressed chunk size in the very beginning of the buffer */
> +    *c_size = ret;
> +
> +    return ret + sizeof(ret);
> +}
> +
> +/*
> + * zstd_decompress()
> + *
> + * Decompress some data (not more than @src_size bytes) to produce exactly
> + * @dest_size bytes.
> + *
> + * @dest - destination buffer, @dest_size bytes
> + * @src - source buffer, @src_size bytes
> + *
> + * Returns: 0 on success
> + *          -1 on fail
> + */
> +
> +static ssize_t zstd_decompress(void *dest, size_t dest_size,
> +                             const void *src, size_t src_size)

Indentation is off.

> +{
> +    size_t ret;
> +    /*
> +     * zstd decompress wants to know the exact lenght of the data
> +     * for that purpose, zstd_compress stores the length in the
> +     * very beginning of the compressed buffer
> +     */
> +    const size_t *s_size = src;
> +    const char *s_buf = src;
> +    s_buf += sizeof(size_t);

Single line: const char *s_buf = src + sizeof(size_t);

Of course, size_t is wrong here, too. And above you used sizeof() on a
variable and here it's on the type. I think we should stay consistent.

You're lacking a check against src_size. A malicious image could make
use read beyond the end of the buffer. (Also consider that src_size
could be smaller than sizeof(size_t).)

> +    ret = ZSTD_decompress(dest, dest_size, s_buf, *s_size);
> +
> +    if (ZSTD_isError(ret)) {
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  #define MAX_COMPRESS_THREADS 4
>  
>  typedef ssize_t (*Qcow2CompressFunc)(void *dest, size_t dest_size,
> @@ -4189,6 +4263,10 @@ qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
>          fn = zlib_compress;
>          break;
>  
> +    case QCOW2_COMPRESSION_TYPE_ZSTD:
> +        fn = zstd_compress;
> +        break;
> +
>      default:
>          return -ENOTSUP;
>      }
> @@ -4208,6 +4286,10 @@ qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
>          fn = zlib_decompress;
>          break;
>  
> +    case QCOW2_COMPRESSION_TYPE_ZSTD:
> +        fn = zstd_decompress;
> +        break;
> +
>      default:
>          return -ENOTSUP;
>      }
> diff --git a/configure b/configure
> index 1c563a7027..c90716189c 100755
> --- a/configure
> +++ b/configure
> @@ -433,6 +433,7 @@ opengl_dmabuf="no"
>  cpuid_h="no"
>  avx2_opt=""
>  zlib="yes"
> +zstd="yes"

This should be zstd="" so that a missing library will automatically
disable it instead of producing an error. (Building QEMU without zlib is
impossible, but building it without ZSTD should work.)

>  capstone=""
>  lzo=""
>  snappy=""
> @@ -1317,6 +1318,8 @@ for opt do
>    ;;
>    --disable-zlib-test) zlib="no"
>    ;;
> +  --disable-zstd-test) zstd="no"
> +  ;;

Instead of this one, after making the above change, options
--disable-zstd and --enable-zstd should be introduced that set
zstd="yes" (that actually does produce an error if it's not available)
or zstd="no".

>    --disable-lzo) lzo="no"
>    ;;
>    --enable-lzo) lzo="yes"
> @@ -3702,6 +3705,29 @@ EOF
>      fi
>  fi
>  
> +#########################################
> +# zstd check
> +
> +if test "$zstd" != "no" ; then
> +    if $pkg_config --exists libzstd; then
> +        zstd_cflags=$($pkg_config --cflags libzstd)
> +        zstd_libs=$($pkg_config --libs libzstd)
> +        QEMU_CFLAGS="$zstd_cflags $QEMU_CFLAGS"
> +        LIBS="$zstd_libs $LIBS"
> +    else
> +        cat > $TMPC << EOF
> +#include <zstd.h>
> +int main(void) { ZSTD_versionNumber(); return 0; }
> +EOF
> +        if compile_prog "" "-lzstd" ; then
> +            LIBS="$LIBS -lzstd"
> +        else
> +            error_exit "zstd check failed" \
> +                "Make sure to have the zstd libs and headers installed."
> +        fi

This needs to be changed, too, to get the desired behaviour. Model it
after bzip2 or lzo support checks:

    if compile_prog "" "-lbz2" ; then
        bzip2="yes"
    else
        if test "$bzip2" = "yes"; then
            feature_not_found "libbzip2" "Install libbzip2 devel"
        fi
        bzip2="no"
    fi

> +    fi
> +fi
> +
>  ##########################################
>  # SHA command probe for modules
>  if test "$modules" = yes; then

Kevin