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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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



-- 
Best regards,
Vladimir