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

[Denis Plotnikov]

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 <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.)
But if we add zstd for clusters compression we have to build with zstd 
each time. If we want to chose whether we want to build zstd we need to
enclose all zstd related code with "ifdef"-s.
I don't think it's good because we can end up with mess of version 
supporting and not supporting zstd compression.

Another point is what the benefit of building qemu without zstd support 
is since it's available and provides better performance than zlib (i.e. 
the replacement for zlib) ?

Denis
> 
>>   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