Thanks for reviewing, I'll take into account the suggestions below and
send the next version of the series soon.
On 28.06.2019 14:57, Kevin Wolf wrote:
> 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
>
--
Best,
Denis
