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

[Denis Plotnikov]

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

-- 
Best,
Denis