[Qemu-devel] [PATCH 0/7] qcow2 decompress in threads

[Qemu-devel] [PATCH 0/7] qcow2 decompress in threads

[Vladimir Sementsov-Ogievskiy]

Hi all!

The series brings threads to qcow2 decompression path, like it is
already done for compression.

Performance gain is illustrated by the following test:

[]# cat gen.sh
#!/bin/bash

echo 'create pattern-file /ssd/t_pat'

./qemu-img create -f raw /ssd/t_pat 10g
for i in {0..9}; do
    ./qemu-io -f raw -c "write -P 0xab ${i}g 1g" /ssd/t_pat
done

echo 'convert it to compressed /ssd/t_pat.compressed.qcow2'

./qemu-img convert -W -f raw -O qcow2 -c /ssd/t_pat /ssd/t_pat.compressed.qcow2
rm -f /ssd/t_pat

test:
[]# time ./qemu-img convert -f qcow2 --target-image-opts -n /ssd/t_pat.compressed.qcow2  'driver=null-co,size=10G'



result before the series:

real    0m16.585s
user    0m14.899s
sys     0m2.219s



result after the series:

real    0m6.528s
user    0m19.343s
sys     0m3.081s


Note: my cpu is 4-cores 8-threads i7-4790

Vladimir Sementsov-Ogievskiy (7):
  qcow2: use Z_OK instead of 0 for deflateInit2 return code check
  qcow2: make more generic interface for qcow2_compress
  qcow2: move decompression from qcow2-cluster.c to qcow2.c
  qcow2: refactor decompress_buffer
  qcow2: use byte-based read in qcow2_decompress_cluster
  qcow2: aio support for compressed cluster read
  qcow2: do decompression in threads

 block/qcow2.h         |   4 -
 block/qcow2-cluster.c |  70 -----------------
 block/qcow2.c         | 169 +++++++++++++++++++++++++++++++++++-------
 3 files changed, 143 insertions(+), 100 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH 1/7] qcow2: use Z_OK instead of 0 for deflateInit2 return code check

[Vladimir Sementsov-Ogievskiy]

Use appropriate macro, corresponding to deflateInit2 spec.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 30689b7688..3e9367c449 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3734,7 +3734,7 @@ static ssize_t qcow2_compress(void *dest, const void *src, size_t size)
     memset(&strm, 0, sizeof(strm));
     ret = deflateInit2(&strm, Z_DEFAULT_COMPRESSION, Z_DEFLATED,
                        -12, 9, Z_DEFAULT_STRATEGY);
-    if (ret != 0) {
+    if (ret != Z_OK) {
         return -2;
     }
 
-- 
2.18.0

Re: [Qemu-devel] [Qemu-block] [PATCH 1/7] qcow2: use Z_OK instead of 0 for deflateInit2 return code check

[Alberto Garcia]

On Thu 01 Nov 2018 07:27:32 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> Use appropriate macro, corresponding to deflateInit2 spec.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[Qemu-devel] [PATCH 2/7] qcow2: make more generic interface for qcow2_compress

[Vladimir Sementsov-Ogievskiy]

Give explicit size both for source and destination buffers, to make it
similar with decompression path and than cleanly reuse parameter
structure for decompression threads.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 3e9367c449..9eab2dbfb6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3718,14 +3718,15 @@ fail:
 /*
  * qcow2_compress()
  *
- * @dest - destination buffer, at least of @size-1 bytes
- * @src - source buffer, @size bytes
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
  *
  * Returns: compressed size on success
- *          -1 if compression is inefficient
+ *          -1 destination buffer is not enough to store compressed data
  *          -2 on any other error
  */
-static ssize_t qcow2_compress(void *dest, const void *src, size_t size)
+static ssize_t qcow2_compress(void *dest, size_t dest_size,
+                              const void *src, size_t src_size)
 {
     ssize_t ret;
     z_stream strm;
@@ -3740,14 +3741,14 @@ static ssize_t qcow2_compress(void *dest, const void *src, size_t size)
 
     /* strm.next_in is not const in old zlib versions, such as those used on
      * OpenBSD/NetBSD, so cast the const away */
-    strm.avail_in = size;
+    strm.avail_in = src_size;
     strm.next_in = (void *) src;
-    strm.avail_out = size - 1;
+    strm.avail_out = dest_size;
     strm.next_out = dest;
 
     ret = deflate(&strm, Z_FINISH);
     if (ret == Z_STREAM_END) {
-        ret = size - 1 - strm.avail_out;
+        ret = dest_size - strm.avail_out;
     } else {
         ret = (ret == Z_OK ? -1 : -2);
     }
@@ -3761,8 +3762,9 @@ static ssize_t qcow2_compress(void *dest, const void *src, size_t size)
 
 typedef struct Qcow2CompressData {
     void *dest;
+    size_t dest_size;
     const void *src;
-    size_t size;
+    size_t src_size;
     ssize_t ret;
 } Qcow2CompressData;
 
@@ -3770,7 +3772,8 @@ static int qcow2_compress_pool_func(void *opaque)
 {
     Qcow2CompressData *data = opaque;
 
-    data->ret = qcow2_compress(data->dest, data->src, data->size);
+    data->ret = qcow2_compress(data->dest, data->dest_size,
+                               data->src, data->src_size);
 
     return 0;
 }
@@ -3782,15 +3785,17 @@ static void qcow2_compress_complete(void *opaque, int ret)
 
 /* See qcow2_compress definition for parameters description */
 static ssize_t qcow2_co_compress(BlockDriverState *bs,
-                                 void *dest, const void *src, size_t size)
+                                 void *dest, size_t dest_size,
+                                 const void *src, size_t src_size)
 {
     BDRVQcow2State *s = bs->opaque;
     BlockAIOCB *acb;
     ThreadPool *pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
     Qcow2CompressData arg = {
         .dest = dest,
+        .dest_size = dest_size,
         .src = src,
-        .size = size,
+        .src_size = src_size,
     };
 
     while (s->nb_compress_threads >= MAX_COMPRESS_THREADS) {
@@ -3857,7 +3862,8 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
 
     out_buf = g_malloc(s->cluster_size);
 
-    out_len = qcow2_co_compress(bs, out_buf, buf, s->cluster_size);
+    out_len = qcow2_co_compress(bs, out_buf, s->cluster_size - 1,
+                                buf, s->cluster_size);
     if (out_len == -2) {
         ret = -EINVAL;
         goto fail;
-- 
2.18.0

Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] qcow2: make more generic interface for qcow2_compress

[Alberto Garcia]

On Thu 01 Nov 2018 07:27:33 PM CET, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> Give explicit size both for source and destination buffers, to make it
> similar with decompression path and than cleanly reuse parameter
> structure for decompression threads.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[Qemu-devel] [PATCH 3/7] qcow2: move decompression from qcow2-cluster.c to qcow2.c

[Vladimir Sementsov-Ogievskiy]

Compression is done in threads in qcow2.c. We want to do decompression
in the same way, so, firstly, move it to the same file.

The only change is braces around if-body in decompress_buffer, to
satisfy checkpatch.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-cluster.c | 70 ------------------------------------------
 block/qcow2.c         | 71 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 70 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index d37fe08b3d..e2737429f5 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1571,76 +1571,6 @@ again:
     return 0;
 }
 
-static int decompress_buffer(uint8_t *out_buf, int out_buf_size,
-                             const uint8_t *buf, int buf_size)
-{
-    z_stream strm1, *strm = &strm1;
-    int ret, out_len;
-
-    memset(strm, 0, sizeof(*strm));
-
-    strm->next_in = (uint8_t *)buf;
-    strm->avail_in = buf_size;
-    strm->next_out = out_buf;
-    strm->avail_out = out_buf_size;
-
-    ret = inflateInit2(strm, -12);
-    if (ret != Z_OK)
-        return -1;
-    ret = inflate(strm, Z_FINISH);
-    out_len = strm->next_out - out_buf;
-    if ((ret != Z_STREAM_END && ret != Z_BUF_ERROR) ||
-        out_len != out_buf_size) {
-        inflateEnd(strm);
-        return -1;
-    }
-    inflateEnd(strm);
-    return 0;
-}
-
-int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
-{
-    BDRVQcow2State *s = bs->opaque;
-    int ret, csize, nb_csectors, sector_offset;
-    uint64_t coffset;
-
-    coffset = cluster_offset & s->cluster_offset_mask;
-    if (s->cluster_cache_offset != coffset) {
-        nb_csectors = ((cluster_offset >> s->csize_shift) & s->csize_mask) + 1;
-        sector_offset = coffset & 511;
-        csize = nb_csectors * 512 - sector_offset;
-
-        /* Allocate buffers on first decompress operation, most images are
-         * uncompressed and the memory overhead can be avoided.  The buffers
-         * are freed in .bdrv_close().
-         */
-        if (!s->cluster_data) {
-            /* one more sector for decompressed data alignment */
-            s->cluster_data = qemu_try_blockalign(bs->file->bs,
-                    QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size + 512);
-            if (!s->cluster_data) {
-                return -ENOMEM;
-            }
-        }
-        if (!s->cluster_cache) {
-            s->cluster_cache = g_malloc(s->cluster_size);
-        }
-
-        BLKDBG_EVENT(bs->file, BLKDBG_READ_COMPRESSED);
-        ret = bdrv_read(bs->file, coffset >> 9, s->cluster_data,
-                        nb_csectors);
-        if (ret < 0) {
-            return ret;
-        }
-        if (decompress_buffer(s->cluster_cache, s->cluster_size,
-                              s->cluster_data + sector_offset, csize) < 0) {
-            return -EIO;
-        }
-        s->cluster_cache_offset = coffset;
-    }
-    return 0;
-}
-
 /*
  * This discards as many clusters of nb_clusters as possible at once (i.e.
  * all clusters in the same L2 slice) and returns the number of discarded
diff --git a/block/qcow2.c b/block/qcow2.c
index 9eab2dbfb6..b45a6c4135 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3758,6 +3758,34 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
     return ret;
 }
 
+static int decompress_buffer(uint8_t *out_buf, int out_buf_size,
+                             const uint8_t *buf, int buf_size)
+{
+    z_stream strm1, *strm = &strm1;
+    int ret, out_len;
+
+    memset(strm, 0, sizeof(*strm));
+
+    strm->next_in = (uint8_t *)buf;
+    strm->avail_in = buf_size;
+    strm->next_out = out_buf;
+    strm->avail_out = out_buf_size;
+
+    ret = inflateInit2(strm, -12);
+    if (ret != Z_OK) {
+        return -1;
+    }
+    ret = inflate(strm, Z_FINISH);
+    out_len = strm->next_out - out_buf;
+    if ((ret != Z_STREAM_END && ret != Z_BUF_ERROR) ||
+        out_len != out_buf_size) {
+        inflateEnd(strm);
+        return -1;
+    }
+    inflateEnd(strm);
+    return 0;
+}
+
 #define MAX_COMPRESS_THREADS 4
 
 typedef struct Qcow2CompressData {
@@ -3911,6 +3939,49 @@ fail:
     return ret;
 }
 
+int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
+{
+    BDRVQcow2State *s = bs->opaque;
+    int ret, csize, nb_csectors, sector_offset;
+    uint64_t coffset;
+
+    coffset = cluster_offset & s->cluster_offset_mask;
+    if (s->cluster_cache_offset != coffset) {
+        nb_csectors = ((cluster_offset >> s->csize_shift) & s->csize_mask) + 1;
+        sector_offset = coffset & 511;
+        csize = nb_csectors * 512 - sector_offset;
+
+        /* Allocate buffers on first decompress operation, most images are
+         * uncompressed and the memory overhead can be avoided.  The buffers
+         * are freed in .bdrv_close().
+         */
+        if (!s->cluster_data) {
+            /* one more sector for decompressed data alignment */
+            s->cluster_data = qemu_try_blockalign(bs->file->bs,
+                    QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size + 512);
+            if (!s->cluster_data) {
+                return -ENOMEM;
+            }
+        }
+        if (!s->cluster_cache) {
+            s->cluster_cache = g_malloc(s->cluster_size);
+        }
+
+        BLKDBG_EVENT(bs->file, BLKDBG_READ_COMPRESSED);
+        ret = bdrv_read(bs->file, coffset >> 9, s->cluster_data,
+                        nb_csectors);
+        if (ret < 0) {
+            return ret;
+        }
+        if (decompress_buffer(s->cluster_cache, s->cluster_size,
+                              s->cluster_data + sector_offset, csize) < 0) {
+            return -EIO;
+        }
+        s->cluster_cache_offset = coffset;
+    }
+    return 0;
+}
+
 static int make_completely_empty(BlockDriverState *bs)
 {
     BDRVQcow2State *s = bs->opaque;
-- 
2.18.0

Re: [Qemu-devel] [Qemu-block] [PATCH 3/7] qcow2: move decompression from qcow2-cluster.c to qcow2.c

[Alberto Garcia]

On Thu 01 Nov 2018 07:27:34 PM CET, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> Compression is done in threads in qcow2.c. We want to do decompression
> in the same way, so, firstly, move it to the same file.
>
> The only change is braces around if-body in decompress_buffer, to
> satisfy checkpatch.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[Qemu-devel] [PATCH 4/7] qcow2: refactor decompress_buffer

[Vladimir Sementsov-Ogievskiy]

- make it look more like a pair of qcow2_compress - rename the function
  and its parameters
- drop extra out_len variable, check filling of output buffer by strm
  structure itself
- fix code style
- add some documentation

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.c | 56 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 21 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index b45a6c4135..e9d24b801e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3758,32 +3758,46 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
     return ret;
 }
 
-static int decompress_buffer(uint8_t *out_buf, int out_buf_size,
-                             const uint8_t *buf, int buf_size)
+/* qcow2_decompress()
+ *
+ * Decompress some data (not more than @src_size bytes) to produce exactly
+ * @dest_size bytes.
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ *          -1 on fail
+ */
+static ssize_t qcow2_decompress(void *dest, size_t dest_size,
+                                const void *src, size_t src_size)
 {
-    z_stream strm1, *strm = &strm1;
-    int ret, out_len;
-
-    memset(strm, 0, sizeof(*strm));
+    int ret = 0;
+    z_stream strm;
 
-    strm->next_in = (uint8_t *)buf;
-    strm->avail_in = buf_size;
-    strm->next_out = out_buf;
-    strm->avail_out = out_buf_size;
+    memset(&strm, 0, sizeof(strm));
+    strm.avail_in = src_size;
+    strm.next_in = (void *) src;
+    strm.avail_out = dest_size;
+    strm.next_out = dest;
 
-    ret = inflateInit2(strm, -12);
+    ret = inflateInit2(&strm, -12);
     if (ret != Z_OK) {
         return -1;
     }
-    ret = inflate(strm, Z_FINISH);
-    out_len = strm->next_out - out_buf;
-    if ((ret != Z_STREAM_END && ret != Z_BUF_ERROR) ||
-        out_len != out_buf_size) {
-        inflateEnd(strm);
-        return -1;
+
+    ret = inflate(&strm, Z_FINISH);
+    if ((ret != Z_STREAM_END && ret != Z_BUF_ERROR) || strm.avail_out != 0) {
+        /* We approve Z_BUF_ERROR because we need @dest buffer to be filled, but
+         * @src buffer may be processed partly (because in qcow2 we know size of
+         * compressed data with precision of one sector)
+         */
+        ret = -1;
     }
-    inflateEnd(strm);
-    return 0;
+
+    inflateEnd(&strm);
+
+    return ret;
 }
 
 #define MAX_COMPRESS_THREADS 4
@@ -3973,8 +3987,8 @@ int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
         if (ret < 0) {
             return ret;
         }
-        if (decompress_buffer(s->cluster_cache, s->cluster_size,
-                              s->cluster_data + sector_offset, csize) < 0) {
+        if (qcow2_decompress(s->cluster_cache, s->cluster_size,
+                             s->cluster_data + sector_offset, csize) < 0) {
             return -EIO;
         }
         s->cluster_cache_offset = coffset;
-- 
2.18.0

Re: [Qemu-devel] [Qemu-block] [PATCH 4/7] qcow2: refactor decompress_buffer

[Alberto Garcia]

On Thu 01 Nov 2018 07:27:35 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> - make it look more like a pair of qcow2_compress - rename the function
>   and its parameters
> - drop extra out_len variable, check filling of output buffer by strm
>   structure itself
> - fix code style
> - add some documentation
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[Qemu-devel] [PATCH 5/7] qcow2: use byte-based read in qcow2_decompress_cluster

[Vladimir Sementsov-Ogievskiy]

We are gradually moving away from sector-based interfaces, towards
byte-based.  Get rid of it here too.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index e9d24b801e..950b9f7ec6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3956,14 +3956,15 @@ fail:
 int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
 {
     BDRVQcow2State *s = bs->opaque;
-    int ret, csize, nb_csectors, sector_offset;
+    int ret, csize, nb_csectors;
     uint64_t coffset;
+    struct iovec iov;
+    QEMUIOVector local_qiov;
 
     coffset = cluster_offset & s->cluster_offset_mask;
     if (s->cluster_cache_offset != coffset) {
         nb_csectors = ((cluster_offset >> s->csize_shift) & s->csize_mask) + 1;
-        sector_offset = coffset & 511;
-        csize = nb_csectors * 512 - sector_offset;
+        csize = nb_csectors * 512 - (coffset & 511);
 
         /* Allocate buffers on first decompress operation, most images are
          * uncompressed and the memory overhead can be avoided.  The buffers
@@ -3981,14 +3982,17 @@ int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
             s->cluster_cache = g_malloc(s->cluster_size);
         }
 
+        iov.iov_base = s->cluster_data;
+        iov.iov_len = csize;
+        qemu_iovec_init_external(&local_qiov, &iov, 1);
+
         BLKDBG_EVENT(bs->file, BLKDBG_READ_COMPRESSED);
-        ret = bdrv_read(bs->file, coffset >> 9, s->cluster_data,
-                        nb_csectors);
+        ret = bdrv_co_preadv(bs->file, coffset, csize, &local_qiov, 0);
         if (ret < 0) {
             return ret;
         }
         if (qcow2_decompress(s->cluster_cache, s->cluster_size,
-                             s->cluster_data + sector_offset, csize) < 0) {
+                             s->cluster_data, csize) < 0) {
             return -EIO;
         }
         s->cluster_cache_offset = coffset;
-- 
2.18.0

Re: [Qemu-devel] [Qemu-block] [PATCH 5/7] qcow2: use byte-based read in qcow2_decompress_cluster

[Alberto Garcia]

On Thu 01 Nov 2018 07:27:36 PM CET, Vladimir Sementsov-Ogievskiy wrote:

> diff --git a/block/qcow2.c b/block/qcow2.c
> index e9d24b801e..950b9f7ec6 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3956,14 +3956,15 @@ fail:
>  int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
>  {
>      BDRVQcow2State *s = bs->opaque;
> -    int ret, csize, nb_csectors, sector_offset;
> +    int ret, csize, nb_csectors;
>      uint64_t coffset;
> +    struct iovec iov;
> +    QEMUIOVector local_qiov;
>  
>      coffset = cluster_offset & s->cluster_offset_mask;
>      if (s->cluster_cache_offset != coffset) {
>          nb_csectors = ((cluster_offset >> s->csize_shift) & s->csize_mask) + 1;
> -        sector_offset = coffset & 511;
> -        csize = nb_csectors * 512 - sector_offset;
> +        csize = nb_csectors * 512 - (coffset & 511);
>  
>          /* Allocate buffers on first decompress operation, most images are
>           * uncompressed and the memory overhead can be avoided.  The buffers
> @@ -3981,14 +3982,17 @@ int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
>              s->cluster_cache = g_malloc(s->cluster_size);
>          }
>  
> +        iov.iov_base = s->cluster_data;
> +        iov.iov_len = csize;
> +        qemu_iovec_init_external(&local_qiov, &iov, 1);
> +
>          BLKDBG_EVENT(bs->file, BLKDBG_READ_COMPRESSED);
> -        ret = bdrv_read(bs->file, coffset >> 9, s->cluster_data,
> -                        nb_csectors);
> +        ret = bdrv_co_preadv(bs->file, coffset, csize, &local_qiov,
> 0);

I think you should annotate the function with coroutine_fn or use
bdrv_pread() instead.

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH 5/7] qcow2: use byte-based read in qcow2_decompress_cluster

[Vladimir Sementsov-Ogievskiy]

06.11.2018 16:53, Alberto Garcia wrote:
> On Thu 01 Nov 2018 07:27:36 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index e9d24b801e..950b9f7ec6 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -3956,14 +3956,15 @@ fail:
>>   int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
>>   {
>>       BDRVQcow2State *s = bs->opaque;
>> -    int ret, csize, nb_csectors, sector_offset;
>> +    int ret, csize, nb_csectors;
>>       uint64_t coffset;
>> +    struct iovec iov;
>> +    QEMUIOVector local_qiov;
>>   
>>       coffset = cluster_offset & s->cluster_offset_mask;
>>       if (s->cluster_cache_offset != coffset) {
>>           nb_csectors = ((cluster_offset >> s->csize_shift) & s->csize_mask) + 1;
>> -        sector_offset = coffset & 511;
>> -        csize = nb_csectors * 512 - sector_offset;
>> +        csize = nb_csectors * 512 - (coffset & 511);
>>   
>>           /* Allocate buffers on first decompress operation, most images are
>>            * uncompressed and the memory overhead can be avoided.  The buffers
>> @@ -3981,14 +3982,17 @@ int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
>>               s->cluster_cache = g_malloc(s->cluster_size);
>>           }
>>   
>> +        iov.iov_base = s->cluster_data;
>> +        iov.iov_len = csize;
>> +        qemu_iovec_init_external(&local_qiov, &iov, 1);
>> +
>>           BLKDBG_EVENT(bs->file, BLKDBG_READ_COMPRESSED);
>> -        ret = bdrv_read(bs->file, coffset >> 9, s->cluster_data,
>> -                        nb_csectors);
>> +        ret = bdrv_co_preadv(bs->file, coffset, csize, &local_qiov,
>> 0);
> I think you should annotate the function with coroutine_fn or use
> bdrv_pread() instead.
>
> Berto

it is called only from qcow2_co_preadv, so it's ok to move to 
already-in-coroutine behaviour. I'll add coroutine_fn if new version is 
needed, or it can be added inflight.

-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH 6/7] qcow2: aio support for compressed cluster read

[Vladimir Sementsov-Ogievskiy]

Allocate buffers locally and release qcow2 lock. Than, reads inside
qcow2_co_preadv_compressed may be done in parallel, however all
decompression is still done synchronously. Let's improve it in the
following commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.h |  4 ---
 block/qcow2.c | 93 ++++++++++++++++++++++++++-------------------------
 2 files changed, 47 insertions(+), 50 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 29c98d87a0..8495d2efbe 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -272,9 +272,6 @@ typedef struct BDRVQcow2State {
     QEMUTimer *cache_clean_timer;
     unsigned cache_clean_interval;
 
-    uint8_t *cluster_cache;
-    uint8_t *cluster_data;
-    uint64_t cluster_cache_offset;
     QLIST_HEAD(QCowClusterAlloc, QCowL2Meta) cluster_allocs;
 
     uint64_t *refcount_table;
@@ -610,7 +607,6 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
                         bool exact_size);
 int qcow2_shrink_l1_table(BlockDriverState *bs, uint64_t max_size);
 int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index);
-int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset);
 int qcow2_encrypt_sectors(BDRVQcow2State *s, int64_t sector_num,
                           uint8_t *buf, int nb_sectors, bool enc, Error **errp);
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 950b9f7ec6..678d737044 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -74,6 +74,12 @@ typedef struct {
 #define  QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x0537be77
 #define  QCOW2_EXT_MAGIC_BITMAPS 0x23852875
 
+static int qcow2_co_preadv_compressed(BlockDriverState *bs,
+                                      uint64_t file_cluster_offset,
+                                      uint64_t offset,
+                                      uint64_t bytes,
+                                      QEMUIOVector *qiov);
+
 static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
     const QCowHeader *cow_header = (const void *)buf;
@@ -1410,7 +1416,6 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
         goto fail;
     }
 
-    s->cluster_cache_offset = -1;
     s->flags = flags;
 
     ret = qcow2_refcount_init(bs);
@@ -1910,15 +1915,15 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
             break;
 
         case QCOW2_CLUSTER_COMPRESSED:
-            /* add AIO support for compressed blocks ? */
-            ret = qcow2_decompress_cluster(bs, cluster_offset);
+            qemu_co_mutex_unlock(&s->lock);
+            ret = qcow2_co_preadv_compressed(bs, cluster_offset,
+                                             offset, cur_bytes,
+                                             &hd_qiov);
+            qemu_co_mutex_lock(&s->lock);
             if (ret < 0) {
                 goto fail;
             }
 
-            qemu_iovec_from_buf(&hd_qiov, 0,
-                                s->cluster_cache + offset_in_cluster,
-                                cur_bytes);
             break;
 
         case QCOW2_CLUSTER_NORMAL:
@@ -2054,8 +2059,6 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
 
     qemu_iovec_init(&hd_qiov, qiov->niov);
 
-    s->cluster_cache_offset = -1; /* disable compressed cache */
-
     qemu_co_mutex_lock(&s->lock);
 
     while (bytes != 0) {
@@ -2219,8 +2222,6 @@ static void qcow2_close(BlockDriverState *bs)
     g_free(s->image_backing_file);
     g_free(s->image_backing_format);
 
-    g_free(s->cluster_cache);
-    qemu_vfree(s->cluster_data);
     qcow2_refcount_close(bs);
     qcow2_free_snapshots(bs);
 }
@@ -3397,7 +3398,6 @@ qcow2_co_copy_range_to(BlockDriverState *bs,
     QCowL2Meta *l2meta = NULL;
 
     assert(!bs->encrypted);
-    s->cluster_cache_offset = -1; /* disable compressed cache */
 
     qemu_co_mutex_lock(&s->lock);
 
@@ -3953,51 +3953,52 @@ fail:
     return ret;
 }
 
-int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
+static int qcow2_co_preadv_compressed(BlockDriverState *bs,
+                                      uint64_t file_cluster_offset,
+                                      uint64_t offset,
+                                      uint64_t bytes,
+                                      QEMUIOVector *qiov)
 {
     BDRVQcow2State *s = bs->opaque;
-    int ret, csize, nb_csectors;
+    int ret = 0, csize, nb_csectors;
     uint64_t coffset;
+    uint8_t *buf, *out_buf;
     struct iovec iov;
     QEMUIOVector local_qiov;
+    int offset_in_cluster = offset_into_cluster(s, offset);
 
-    coffset = cluster_offset & s->cluster_offset_mask;
-    if (s->cluster_cache_offset != coffset) {
-        nb_csectors = ((cluster_offset >> s->csize_shift) & s->csize_mask) + 1;
-        csize = nb_csectors * 512 - (coffset & 511);
+    coffset = file_cluster_offset & s->cluster_offset_mask;
+    nb_csectors = ((file_cluster_offset >> s->csize_shift) & s->csize_mask) + 1;
+    csize = nb_csectors * 512 - (coffset & 511);
 
-        /* Allocate buffers on first decompress operation, most images are
-         * uncompressed and the memory overhead can be avoided.  The buffers
-         * are freed in .bdrv_close().
-         */
-        if (!s->cluster_data) {
-            /* one more sector for decompressed data alignment */
-            s->cluster_data = qemu_try_blockalign(bs->file->bs,
-                    QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size + 512);
-            if (!s->cluster_data) {
-                return -ENOMEM;
-            }
-        }
-        if (!s->cluster_cache) {
-            s->cluster_cache = g_malloc(s->cluster_size);
-        }
+    buf = g_try_malloc(csize);
+    if (!buf) {
+        return -ENOMEM;
+    }
+    iov.iov_base = buf;
+    iov.iov_len = csize;
+    qemu_iovec_init_external(&local_qiov, &iov, 1);
 
-        iov.iov_base = s->cluster_data;
-        iov.iov_len = csize;
-        qemu_iovec_init_external(&local_qiov, &iov, 1);
+    out_buf = qemu_blockalign(bs, s->cluster_size);
 
-        BLKDBG_EVENT(bs->file, BLKDBG_READ_COMPRESSED);
-        ret = bdrv_co_preadv(bs->file, coffset, csize, &local_qiov, 0);
-        if (ret < 0) {
-            return ret;
-        }
-        if (qcow2_decompress(s->cluster_cache, s->cluster_size,
-                             s->cluster_data, csize) < 0) {
-            return -EIO;
-        }
-        s->cluster_cache_offset = coffset;
+    BLKDBG_EVENT(bs->file, BLKDBG_READ_COMPRESSED);
+    ret = bdrv_co_preadv(bs->file, coffset, csize, &local_qiov, 0);
+    if (ret < 0) {
+        goto fail;
     }
-    return 0;
+
+    if (qcow2_decompress(out_buf, s->cluster_size, buf, csize) < 0) {
+        ret = -EIO;
+        goto fail;
+    }
+
+    qemu_iovec_from_buf(qiov, 0, out_buf + offset_in_cluster, bytes);
+
+fail:
+    qemu_vfree(out_buf);
+    g_free(buf);
+
+    return ret;
 }
 
 static int make_completely_empty(BlockDriverState *bs)
-- 
2.18.0

Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] qcow2: aio support for compressed cluster read

[Alberto Garcia]

On Thu 01 Nov 2018 07:27:37 PM CET, Vladimir Sementsov-Ogievskiy wrote:

> +    buf = g_try_malloc(csize);
> +    if (!buf) {
> +        return -ENOMEM;
> +    }
> +    iov.iov_base = buf;
> +    iov.iov_len = csize;
> +    qemu_iovec_init_external(&local_qiov, &iov, 1);
>  
> -        iov.iov_base = s->cluster_data;
> -        iov.iov_len = csize;
> -        qemu_iovec_init_external(&local_qiov, &iov, 1);
> +    out_buf = qemu_blockalign(bs, s->cluster_size);

You should also check whether out_buf is NULL, shouldn't you?

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] qcow2: aio support for compressed cluster read

[Vladimir Sementsov-Ogievskiy]

06.11.2018 18:06, Alberto Garcia wrote:
> On Thu 01 Nov 2018 07:27:37 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>
>> +    buf = g_try_malloc(csize);
>> +    if (!buf) {
>> +        return -ENOMEM;
>> +    }
>> +    iov.iov_base = buf;
>> +    iov.iov_len = csize;
>> +    qemu_iovec_init_external(&local_qiov, &iov, 1);
>>   
>> -        iov.iov_base = s->cluster_data;
>> -        iov.iov_len = csize;
>> -        qemu_iovec_init_external(&local_qiov, &iov, 1);
>> +    out_buf = qemu_blockalign(bs, s->cluster_size);
> You should also check whether out_buf is NULL, shouldn't you?

No, it will abort on fail. qemu_try_blockalign result should be checked.

>
> Berto


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] qcow2: aio support for compressed cluster read

[Alberto Garcia]

On Tue 06 Nov 2018 04:13:58 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> 06.11.2018 18:06, Alberto Garcia wrote:
>> On Thu 01 Nov 2018 07:27:37 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>>
>>> +    buf = g_try_malloc(csize);
>>> +    if (!buf) {
>>> +        return -ENOMEM;
>>> +    }
>>> +    iov.iov_base = buf;
>>> +    iov.iov_len = csize;
>>> +    qemu_iovec_init_external(&local_qiov, &iov, 1);
>>>   
>>> -        iov.iov_base = s->cluster_data;
>>> -        iov.iov_len = csize;
>>> -        qemu_iovec_init_external(&local_qiov, &iov, 1);
>>> +    out_buf = qemu_blockalign(bs, s->cluster_size);
>> You should also check whether out_buf is NULL, shouldn't you?
>
> No, it will abort on fail. qemu_try_blockalign result should be
> checked.

Is there any reason why some parts of the QEMU code use qemu_blockalign
and others qemu_try_blockalign() ? From what I can see it seems to be up
to whoever wrote it...

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] qcow2: aio support for compressed cluster read

[Vladimir Sementsov-Ogievskiy]

06.11.2018 18:30, Alberto Garcia wrote:
> On Tue 06 Nov 2018 04:13:58 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>> 06.11.2018 18:06, Alberto Garcia wrote:
>>> On Thu 01 Nov 2018 07:27:37 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>>>
>>>> +    buf = g_try_malloc(csize);
>>>> +    if (!buf) {
>>>> +        return -ENOMEM;
>>>> +    }
>>>> +    iov.iov_base = buf;
>>>> +    iov.iov_len = csize;
>>>> +    qemu_iovec_init_external(&local_qiov, &iov, 1);
>>>>    
>>>> -        iov.iov_base = s->cluster_data;
>>>> -        iov.iov_len = csize;
>>>> -        qemu_iovec_init_external(&local_qiov, &iov, 1);
>>>> +    out_buf = qemu_blockalign(bs, s->cluster_size);
>>> You should also check whether out_buf is NULL, shouldn't you?
>> No, it will abort on fail. qemu_try_blockalign result should be
>> checked.
> Is there any reason why some parts of the QEMU code use qemu_blockalign
> and others qemu_try_blockalign() ? From what I can see it seems to be up
> to whoever wrote it...
>
> Berto

As I understand, the good reason to use _try_ versions, is when we are 
allocating some size, taken from user input, so it may be unpredictable 
large (hm, or just any really large allocation), so, I use try_malloc 
for compressed size, which may be very large in somehow corrupted image. 
And it looks a common practice to use not-failing (aborting) allocation 
functions for cluster_size allocations in qcow2.c

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] qcow2: aio support for compressed cluster read

[Alberto Garcia]

On Tue 06 Nov 2018 05:24:36 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>> Is there any reason why some parts of the QEMU code use qemu_blockalign
>> and others qemu_try_blockalign() ? From what I can see it seems to be up
>> to whoever wrote it...
>
> As I understand, the good reason to use _try_ versions, is when we are
> allocating some size, taken from user input, so it may be
> unpredictable large (hm, or just any really large allocation), so, I
> use try_malloc for compressed size, which may be very large in somehow
> corrupted image.

Ah right, the value of the compressed size field can be up to 8192
sectors, or 4MB.

I realized that you should add the coroutine_fn annotation to
qcow2_co_preadv_compressed(). Other than that the patch looks good so,
with that changed,

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[Qemu-devel] [PATCH 7/7] qcow2: do decompression in threads

[Vladimir Sementsov-Ogievskiy]

Do decompression in threads, like it is already done for compression.
This improves asynchronous compressed reads performance.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.c | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 678d737044..3f1c773b13 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3802,20 +3802,24 @@ static ssize_t qcow2_decompress(void *dest, size_t dest_size,
 
 #define MAX_COMPRESS_THREADS 4
 
+typedef ssize_t (*Qcow2CompressFunc)(void *dest, size_t dest_size,
+                                     const void *src, size_t src_size);
 typedef struct Qcow2CompressData {
     void *dest;
     size_t dest_size;
     const void *src;
     size_t src_size;
     ssize_t ret;
+
+    Qcow2CompressFunc func;
 } Qcow2CompressData;
 
 static int qcow2_compress_pool_func(void *opaque)
 {
     Qcow2CompressData *data = opaque;
 
-    data->ret = qcow2_compress(data->dest, data->dest_size,
-                               data->src, data->src_size);
+    data->ret = data->func(data->dest, data->dest_size,
+                           data->src, data->src_size);
 
     return 0;
 }
@@ -3825,10 +3829,10 @@ static void qcow2_compress_complete(void *opaque, int ret)
     qemu_coroutine_enter(opaque);
 }
 
-/* See qcow2_compress definition for parameters description */
-static ssize_t qcow2_co_compress(BlockDriverState *bs,
-                                 void *dest, size_t dest_size,
-                                 const void *src, size_t src_size)
+static ssize_t qcow2_co_do_compress(BlockDriverState *bs,
+                                    void *dest, size_t dest_size,
+                                    const void *src, size_t src_size,
+                                    Qcow2CompressFunc func)
 {
     BDRVQcow2State *s = bs->opaque;
     BlockAIOCB *acb;
@@ -3838,6 +3842,7 @@ static ssize_t qcow2_co_compress(BlockDriverState *bs,
         .dest_size = dest_size,
         .src = src,
         .src_size = src_size,
+        .func = func,
     };
 
     while (s->nb_compress_threads >= MAX_COMPRESS_THREADS) {
@@ -3860,6 +3865,22 @@ static ssize_t qcow2_co_compress(BlockDriverState *bs,
     return arg.ret;
 }
 
+static ssize_t qcow2_co_compress(BlockDriverState *bs,
+                                 void *dest, size_t dest_size,
+                                 const void *src, size_t src_size)
+{
+    return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
+                                qcow2_compress);
+}
+
+static ssize_t qcow2_co_decompress(BlockDriverState *bs,
+                                   void *dest, size_t dest_size,
+                                   const void *src, size_t src_size)
+{
+    return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
+                                qcow2_decompress);
+}
+
 /* XXX: put compressed sectors first, then all the cluster aligned
    tables to avoid losing bytes in alignment */
 static coroutine_fn int
@@ -3987,7 +4008,7 @@ static int qcow2_co_preadv_compressed(BlockDriverState *bs,
         goto fail;
     }
 
-    if (qcow2_decompress(out_buf, s->cluster_size, buf, csize) < 0) {
+    if (qcow2_co_decompress(bs, out_buf, s->cluster_size, buf, csize) < 0) {
         ret = -EIO;
         goto fail;
     }
-- 
2.18.0

Re: [Qemu-devel] [Qemu-block] [PATCH 7/7] qcow2: do decompression in threads

[Alberto Garcia]

On Thu 01 Nov 2018 07:27:38 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> Do decompression in threads, like it is already done for compression.
> This improves asynchronous compressed reads performance.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [PATCH 7/7] qcow2: do decompression in threads

[Paolo Bonzini]

On 01/11/2018 19:27, Vladimir Sementsov-Ogievskiy wrote:
> -/* See qcow2_compress definition for parameters description */
> -static ssize_t qcow2_co_compress(BlockDriverState *bs,
> -                                 void *dest, size_t dest_size,
> -                                 const void *src, size_t src_size)
> +static ssize_t qcow2_co_do_compress(BlockDriverState *bs,
> +                                    void *dest, size_t dest_size,
> +                                    const void *src, size_t src_size,
> +                                    Qcow2CompressFunc func)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      BlockAIOCB *acb;
> @@ -3838,6 +3842,7 @@ static ssize_t qcow2_co_compress(BlockDriverState *bs,
>          .dest_size = dest_size,
>          .src = src,
>          .src_size = src_size,
> +        .func = func,
>      };
>  
>      while (s->nb_compress_threads >= MAX_COMPRESS_THREADS) {
> @@ -3860,6 +3865,22 @@ static ssize_t qcow2_co_compress(BlockDriverState *bs,
>      return arg.ret;
>  }
>  

Not a blocker for this patch, but I'll note that qcow2_co_do_compress
could use thread_pool_submit_co.  Also, qcow2_co_do_compress should be a
"coroutine_fn".

Paolo

Re: [Qemu-devel] [PATCH 0/7] qcow2 decompress in threads

[Kevin Wolf]

Am 01.11.2018 um 19:27 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi all!
> 
> The series brings threads to qcow2 decompression path, like it is
> already done for compression.

Thanks! Nice series, I like it a lot.

I fixed some non-function stuff like the coroutine_fn tags and comment
formatting and applied it to block-next (for 3.2).

Kevin

Re: [Qemu-devel] [PATCH 0/7] qcow2 decompress in threads

[Vladimir Sementsov-Ogievskiy]

12.11.2018 18:32, Kevin Wolf wrote:
> Am 01.11.2018 um 19:27 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Hi all!
>>
>> The series brings threads to qcow2 decompression path, like it is
>> already done for compression.
> 
> Thanks! Nice series, I like it a lot.

Thank you!

> 
> I fixed some non-function stuff like the coroutine_fn tags and comment
> formatting and applied it to block-next (for 3.2).
> 
> Kevin
> 


-- 
Best regards,
Vladimir