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 <dplotni...@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