[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v0 3/3] qcow2: add zstd cluster compression
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [PATCH v0 3/3] qcow2: add zstd cluster compression |
Date: |
Fri, 28 Jun 2019 13:57:20 +0200 |
User-agent: |
Mutt/1.11.3 (2019-02-01) |
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 <address@hidden>
> ---
> 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
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-block] [PATCH v0 3/3] qcow2: add zstd cluster compression,
Kevin Wolf <=