Re: [PATCH v13 3/4] qcow2: add zstd cluster compression

[Vladimir Sementsov-Ogievskiy]

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



--
Best regards,
Vladimir