Message ID | 20200331131743.17448-4-dplotnikov@virtuozzo.com |
---|---|
State | New |
Headers | show |
Series | None | expand |
31.03.2020 16:17, 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 <dplotnikov@virtuozzo.com> > QAPI part: > Acked-by: Markus Armbruster <armbru@redhat.com> > --- Hi! I have three suggestions: 1. return type of qcow2 compression functions is ssize_t, and return type of zstd is size_t. I think better is not to mix them. 2. safe check for ENOMEM in compression part - avoid overflow ( maximum of paranoia :) 3. slightly more tolerate sanity check in decompression part: allow zstd to make progress only on one of the buffers Here is it, and if you like it, take it together with my Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c index 0b04f30bd8..208218c8f3 100644 --- a/block/qcow2-threads.c +++ b/block/qcow2-threads.c @@ -188,7 +188,7 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t dest_size, static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size, const void *src, size_t src_size) { - size_t ret; + ssize_t ret; ZSTD_outBuffer output = { dest, dest_size, 0 }; ZSTD_inBuffer input = { src, src_size, 0 }; ZSTD_CCtx *cctx; @@ -214,13 +214,14 @@ static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size, * We want to use zstd streamed interface on decompression, * as we won't know the exact size of the compressed data. */ - ret = ZSTD_compressStream2(cctx, &output, &input, ZSTD_e_end); - if (ZSTD_isError(ret)) { + size_t zstd_ret = ZSTD_compressStream2(cctx, &output, &input, + ZSTD_e_end); + if (ZSTD_isError(zstd_ret)) { ret = -EIO; goto out; } /* Dest buffer isn't big enough to store compressed content */ - if (output.pos + ret > output.size) { + if (zstd_ret > output.size - output.pos) { ret = -ENOMEM; goto out; } @@ -248,7 +249,8 @@ out: static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size, const void *src, size_t src_size) { - size_t ret = 0; + ssize_t ret = 0; + size_t zstd_ret = 0; ZSTD_outBuffer output = { dest, dest_size, 0 }; ZSTD_inBuffer input = { src, src_size, 0 }; ZSTD_DCtx *dctx = ZSTD_createDCtx(); @@ -269,8 +271,9 @@ static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size, * ZSTD_decompressStream reads another ONE full frame. */ while (output.pos < output.size) { - size_t last_input_pos = input.pos; - ret = ZSTD_decompressStream(dctx, &output, &input); + size_t last_progress = input.pos + output.pos; + + zstd_ret = ZSTD_decompressStream(dctx, &output, &input); /* * zstd manual doesn't explicitly states what happens, * if the read the frame partially. But, based on the @@ -278,7 +281,7 @@ static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size, * and have read all the frames from the input, * we end up with error here. */ - if (ZSTD_isError(ret)) { + if (ZSTD_isError(zstd_ret)) { ret = -EIO; break; } @@ -286,7 +289,7 @@ static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size, /* * Sanity check that each time we do some progress */ - if (last_input_pos >= input.pos) { + if (last_progress >= input.pos + output.pos) { ret = -EIO; break; } @@ -297,7 +300,7 @@ static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size, * if not, we somehow managed to get uncompressed cluster * greater then the cluster size. */ - if (ret > 0) { + if (zstd_ret > 0) { ret = -EIO; }
On 31.03.2020 17:49, Vladimir Sementsov-Ogievskiy wrote: > 31.03.2020 16:17, 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 <dplotnikov@virtuozzo.com> >> QAPI part: >> Acked-by: Markus Armbruster <armbru@redhat.com> >> --- > > Hi! > > I have three suggestions: > > 1. return type of qcow2 compression functions is ssize_t, and return > type of zstd is size_t. I think better is not to mix them. > 2. safe check for ENOMEM in compression part - avoid overflow ( > maximum of paranoia :) > 3. slightly more tolerate sanity check in decompression part: allow > zstd to make progress only on one of the buffers > > Here is it, and if you like it, take it together with my > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c > index 0b04f30bd8..208218c8f3 100644 > --- a/block/qcow2-threads.c > +++ b/block/qcow2-threads.c > @@ -188,7 +188,7 @@ static ssize_t qcow2_zlib_decompress(void *dest, > size_t dest_size, > static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size, > const void *src, size_t src_size) > { > - size_t ret; > + ssize_t ret; > ZSTD_outBuffer output = { dest, dest_size, 0 }; > ZSTD_inBuffer input = { src, src_size, 0 }; > ZSTD_CCtx *cctx; > @@ -214,13 +214,14 @@ static ssize_t qcow2_zstd_compress(void *dest, > size_t dest_size, > * We want to use zstd streamed interface on decompression, > * as we won't know the exact size of the compressed data. > */ > - ret = ZSTD_compressStream2(cctx, &output, &input, ZSTD_e_end); > - if (ZSTD_isError(ret)) { > + size_t zstd_ret = ZSTD_compressStream2(cctx, &output, &input, > + ZSTD_e_end); > + if (ZSTD_isError(zstd_ret)) { > ret = -EIO; > goto out; > } > /* Dest buffer isn't big enough to store compressed content */ > - if (output.pos + ret > output.size) { > + if (zstd_ret > output.size - output.pos) { > ret = -ENOMEM; > goto out; > } I'll apply this part as you suggested > @@ -248,7 +249,8 @@ out: > static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size, > const void *src, size_t src_size) > { > - size_t ret = 0; > + ssize_t ret = 0; > + size_t zstd_ret = 0; > ZSTD_outBuffer output = { dest, dest_size, 0 }; > ZSTD_inBuffer input = { src, src_size, 0 }; > ZSTD_DCtx *dctx = ZSTD_createDCtx(); > @@ -269,8 +271,9 @@ static ssize_t qcow2_zstd_decompress(void *dest, > size_t dest_size, > * ZSTD_decompressStream reads another ONE full frame. > */ > while (output.pos < output.size) { > - size_t last_input_pos = input.pos; > - ret = ZSTD_decompressStream(dctx, &output, &input); > + size_t last_progress = input.pos + output.pos; These two may overflow last_progress. In that case we have to check them in separate > + > + zstd_ret = ZSTD_decompressStream(dctx, &output, &input); > /* > * zstd manual doesn't explicitly states what happens, > * if the read the frame partially. But, based on the > @@ -278,7 +281,7 @@ static ssize_t qcow2_zstd_decompress(void *dest, > size_t dest_size, > * and have read all the frames from the input, > * we end up with error here. > */ > - if (ZSTD_isError(ret)) { > + if (ZSTD_isError(zstd_ret)) { > ret = -EIO; > break; > } > @@ -286,7 +289,7 @@ static ssize_t qcow2_zstd_decompress(void *dest, > size_t dest_size, > /* > * Sanity check that each time we do some progress > */ > - if (last_input_pos >= input.pos) { > + if (last_progress >= input.pos + output.pos) { > ret = -EIO; > break; > } > @@ -297,7 +300,7 @@ static ssize_t qcow2_zstd_decompress(void *dest, > size_t dest_size, > * if not, we somehow managed to get uncompressed cluster > * greater then the cluster size. > */ > - if (ret > 0) { > + if (zstd_ret > 0) { > ret = -EIO; > } I'll apply the changes and resend the series. Thanks! > > > >
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt index 640e0eca40..18a77f737e 100644 --- a/docs/interop/qcow2.txt +++ b/docs/interop/qcow2.txt @@ -209,6 +209,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 e225a1e3ff..fdc991b010 100755 --- a/configure +++ b/configure @@ -1861,7 +1861,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 d669ec0965..44690ed266 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -4293,11 +4293,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..0b04f30bd8 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,141 @@ 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; + + /* make sure we can safely return compressed buffer size with ssize_t */ + assert(dest_size <= SSIZE_MAX); + + cctx = ZSTD_createCCtx(); + if (!cctx) { + return -EIO; + } + /* + * ZSTD spec: "You must continue calling ZSTD_compressStream2() + * with ZSTD_e_end until it returns 0, at which point you are + * free to start a new frame". + * In the loop, we try to compress all the data into one zstd frame. + * ZSTD_compressStream2 potentially can finish a frame earlier + * than the full input data is consumed. That's why we are looping + * until all the input data is consumed. + */ + do { + /* + * We want to use zstd streamed interface on decompression, + * as we won't know the exact size of the compressed data. + */ + ret = ZSTD_compressStream2(cctx, &output, &input, ZSTD_e_end); + 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 || input.pos < input.size); + + 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; + } + + /* + * The compressed stream from input buffer may consist from more + * than one zstd frames. So we iterate until we get a fully + * uncompressed cluster. + * from zstd docs: acording to ZSTD_decompressStream: + * "return : 0 when a frame is completely decoded and fully flushed" + * We suppose that this means: each time ZSTD_decompressStream reads + * only ONE full frame and return 0 if and only if that frame + * is completely decoded and flushed. Only after returning 0, + * ZSTD_decompressStream reads another ONE full frame. + */ + while (output.pos < output.size) { + size_t last_input_pos = input.pos; + ret = ZSTD_decompressStream(dctx, &output, &input); + /* + * zstd manual doesn't explicitly states what happens, + * if the read the frame partially. But, based on the + * our tests, if we don't fully populate the output + * and have read all the frames from the input, + * we end up with error here. + */ + if (ZSTD_isError(ret)) { + ret = -EIO; + break; + } + + /* + * Sanity check that each time we do some progress + */ + if (last_input_pos >= input.pos) { + ret = -EIO; + break; + } + } + + /* + * Make sure that we have the frame fully flushed here + * if not, we somehow managed to get uncompressed cluster + * greater then the cluster size. + */ + if (ret > 0) { + ret = -EIO; + } + + ZSTD_freeDCtx(dctx); + return ret; +} +#endif + static int qcow2_compress_pool_func(void *opaque) { Qcow2CompressData *data = opaque; @@ -217,6 +357,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 +394,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 6cb000be19..3ae9624ba3 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: @@ -3478,6 +3481,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;