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 <address@hidden>
QAPI part:
Acked-by: Markus Armbruster <address@hidden>
---
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 <address@hidden>
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;
}