On 23.03.2020 11:44, Vladimir Sementsov-Ogievskiy wrote:
21.03.2020 17:34, Denis Plotnikov wrote:zstd significantly reduces cluster compression time. It provides better compression performance maintaining the same level of the compression ratio in comparison with zlib, which, at the moment, is 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> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> Reviewed-by: Alberto Garcia <be...@igalia.com> QAPI part: Acked-by: Markus Armbruster <arm...@redhat.com>You forget to drop signs, patch is changed significantly, including algorithm.
ok, my fault
Should I remove it? Doesn't worth to express that explicitly? This line removes the question what compression level is used. And gives a hint where to change it ,if we decide to implement compression ratio changing.--- docs/interop/qcow2.txt | 1 + configure | 2 +- qapi/block-core.json | 3 +- block/qcow2-threads.c | 129 +++++++++++++++++++++++++++++++++++++++++ block/qcow2.c | 7 +++ 5 files changed, 140 insertions(+), 2 deletions(-) diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt index 5597e24474..795dbb21dd 100644 --- a/docs/interop/qcow2.txt +++ b/docs/interop/qcow2.txt @@ -208,6 +208,7 @@ version 2. Available compression type values: 0: zlib <https://www.zlib.net/> + 1: zstd <http://github.com/facebook/zstd> === Header padding === diff --git a/configure b/configure index caa65f5883..b2a0aa241a 100755 --- a/configure +++ b/configure@@ -1835,7 +1835,7 @@ disabled with --disable-FEATURE, default is enabled if available:lzfse support of lzfse compression library (for reading lzfse-compressed dmg images) zstd support for zstd compression library - (for migration compression)+ (for migration compression and qcow2 cluster compression)seccomp seccomp support coroutine-pool coroutine freelist (better performance) glusterfs GlusterFS backend diff --git a/qapi/block-core.json b/qapi/block-core.json index a306484973..8953451818 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -4401,11 +4401,12 @@ # Compression type used in qcow2 image file # # @zlib: zlib compression, see <http://zlib.net/> +# @zstd: zstd compression, see <http://github.com/facebook/zstd> # # Since: 5.0 ## { 'enum': 'Qcow2CompressionType', - 'data': [ 'zlib' ] }+ 'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }## # @BlockdevCreateOptionsQcow2: diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c index 7dbaf53489..ee4bad8d5b 100644 --- a/block/qcow2-threads.c +++ b/block/qcow2-threads.c @@ -28,6 +28,11 @@ #define ZLIB_CONST #include <zlib.h> +#ifdef CONFIG_ZSTD +#include <zstd.h> +#include <zstd_errors.h> +#endif + #include "qcow2.h" #include "block/thread-pool.h" #include "crypto.h"@@ -166,6 +171,120 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t dest_size,return ret; } +#ifdef CONFIG_ZSTD + +/* + * qcow2_zstd_compress() + * + * Compress @src_size bytes of data using zstd compression method + * + * @dest - destination buffer, @dest_size bytes + * @src - source buffer, @src_size bytes + * + * Returns: compressed size on success+ * -ENOMEM destination buffer is not enough to store compressed data+ * -EIO on any other error + */ +static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size, + const void *src, size_t src_size) +{ + size_t ret; + ZSTD_outBuffer output = { dest, dest_size, 0 }; + ZSTD_inBuffer input = { src, src_size, 0 }; + ZSTD_CCtx *cctx = ZSTD_createCCtx(); + + if (!cctx) { + return -EIO; + } + + ret = ZSTD_CCtx_setParameter(cctx, ZSTD_c_compressionLevel, + ZSTD_CLEVEL_DEFAULT);Hmm, looks a bit strange.. Isn't it already default by default?)
+ if (ZSTD_isError(ret)) { + ret = -EIO; + goto out; + } +Hmm, strange that we need a loop, but zstd spec directly requires it, possibly we need to make a comment from it:"ZSTD spec: You must continue calling ZSTD_compressStream2() with ZSTD_e_flush until it returns 0, at which point you can change the operation."
ok
+ { + /* + * Instruct zstd to compress the whole buffer and write + * the frame epilogue. This allows as to use zstd streaming + * semantics and don't store the compressed size for the + * zstd decompression.May be not "to don't store", but to "decompress without knowing exact compressed data length" or something like this. I mean, make the comment for people who read the final code, not for those who review changes from version to version.
I'd better write * zstd simple interface requires exact compressed size.* zstd stream interface holds all the data in the compressed frame.
* Instruct zstd to compress the whole buffer and write * the frame epilogue. This allows as to use zstd streaming * semantics and don't store the compressed size for the * zstd decompression.
If the image is corrupted we can't continue anyway. If we return -EIO on this condition, we need to do some work investigating what has happened. Assert explicitly points out what happened. I though we should return -EIO if we know what to do with the error. But if the invariant doesn't hold, do we really know what to do rather then stop the execution and report an error?+ */ + ret = ZSTD_compressStream2(cctx, &output , &input, ZSTD_e_end);extra whitespace+ if (ZSTD_isError(ret)) { + ret = -EIO; + goto out; + } + /* Dest buffer isn't big enough to store compressed content */ + if (output.pos + ret > output.size) { + ret = -ENOMEM; + goto out; + } + } while (ret); + + /* if no error, the input data must be fully consumed */ + assert(input.pos == input.size);+ /* make sure we can safely return compressed buffer size with ssize_t */+ assert(output.pos <= SSIZE_MAX); + ret = output.pos; + +out: + ZSTD_freeCCtx(cctx); + return ret; +} + +/* + * qcow2_zstd_decompress() + *+ * Decompress some data (not more than @src_size bytes) to produce exactly+ * @dest_size bytes using zstd compression method + * + * @dest - destination buffer, @dest_size bytes + * @src - source buffer, @src_size bytes + * + * Returns: 0 on success + * -EIO on any error + */ +static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size, + const void *src, size_t src_size) +{ + size_t ret = 0; + ZSTD_outBuffer output = { dest, dest_size, 0 }; + ZSTD_inBuffer input = { src, src_size, 0 }; + ZSTD_DCtx *dctx = ZSTD_createDCtx(); + + if (!dctx) { + return -EIO; + } + + { + ret = ZSTD_decompressStream(dctx, &output, &input); + if (ZSTD_isError(ret)) { + ret = -EIO; + goto out; + } + /* + * Dest buffer size is the image cluster size. + * It should be big enough to store uncompressed content. + * There shouldn't be any cases when the decompressed content + * size is greater then the cluster size. + */But consider corrupted image: it may contain any data. And we should not crash because of it. So, we should return error here.
+ assert(output.pos + ret <= output.size); + } while (ret); + + /* + * If decompression went fine we must have the compressed + * cluser fully consumed and flushedcluster+ */ + assert(output.pos == output.size);again, it should be EIO, not crash.+out: + ZSTD_freeDCtx(dctx); + return ret; + +} +#endif + static int qcow2_compress_pool_func(void *opaque) { Qcow2CompressData *data = opaque;@@ -217,6 +336,11 @@ qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,fn = qcow2_zlib_compress; break; +#ifdef CONFIG_ZSTD + case QCOW2_COMPRESSION_TYPE_ZSTD: + fn = qcow2_zstd_compress; + break; +#endif default: abort(); }@@ -249,6 +373,11 @@ qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,fn = qcow2_zlib_decompress; break; +#ifdef CONFIG_ZSTD + case QCOW2_COMPRESSION_TYPE_ZSTD: + fn = qcow2_zstd_decompress; + break; +#endif default: abort(); } diff --git a/block/qcow2.c b/block/qcow2.c index 899b5541f5..6420d27a6d 100644 --- a/block/qcow2.c +++ b/block/qcow2.c@@ -1246,6 +1246,9 @@ static int validate_compression_type(BDRVQcow2State *s, Error **errp){ switch (s->compression_type) { case QCOW2_COMPRESSION_TYPE_ZLIB: +#ifdef CONFIG_ZSTD + case QCOW2_COMPRESSION_TYPE_ZSTD: +#endif break; default:@@ -3454,6 +3457,10 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)} switch (qcow2_opts->compression_type) { +#ifdef CONFIG_ZSTD + case QCOW2_COMPRESSION_TYPE_ZSTD: + break; +#endif default: error_setg(errp, "Unknown compression type"); goto out;
