Skip to contentSkip to conversation

qcow2 decompress in threads

PATCH In review Vladimir Sementsov-Ogievskiy 4 participants 23 messages Nov 1, 2018 – Nov 12, 2018 Block layer

lore
Share & tools
lore

Vladimir Sementsov-Ogievskiy

author
#0 lore raw

cover qcow2 decompress in threads

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(-)
Signature · 1 line
2.18.0
author
#1 lore raw

PATCH 1/7 qcow2: use Z_OK instead of 0 for deflateInit2 return code check

Use appropriate macro, corresponding to deflateInit2 spec.

Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>
---
 block/qcow2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
block/qcow2.c +1 -1
index 30689b7688..3e9367c449 100644
@@ -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;
     }
Signature · 1 line
2.18.0
author
#3 lore raw

PATCH 2/7 qcow2: make more generic interface for qcow2_compress

In reply to 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 <[email protected]>
---
 block/qcow2.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)
block/qcow2.c +18 -12
index 3e9367c449..9eab2dbfb6 100644
@@ -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;
Signature · 1 line
2.18.0
Reviewed-by on v1 2/7
#4 lore raw

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

Quoted 5 lines · 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 <[email protected]>
Reviewed-by Alberto Garcia <[email protected]>

Berto

author
#5 lore raw

PATCH 3/7 qcow2: move decompression from qcow2-cluster.c to qcow2.c

In reply to 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 <[email protected]>
---
 block/qcow2-cluster.c | 70 ------------------------------------------
 block/qcow2.c         | 71 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 70 deletions(-)
block/qcow2-cluster.c +0 -70
index d37fe08b3d..e2737429f5 100644
@@ -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
block/qcow2.c +71 -0

83 lines not on this page · raw message

Signature · 1 line
2.18.0
Reviewed-by on v1 3/7
#6 lore raw

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

Quoted 7 lines · 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 <[email protected]>
Reviewed-by Alberto Garcia <[email protected]>

Berto

author
#7 lore raw

PATCH 4/7 qcow2: refactor decompress_buffer

In reply to 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 <[email protected]>
---
 block/qcow2.c | 56 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 21 deletions(-)
block/qcow2.c +35 -21
index b45a6c4135..e9d24b801e 100644
@@ -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;
Signature · 1 line
2.18.0
Reviewed-by on v1 4/7
#8 lore raw

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

Quoted 8 lines · 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 <[email protected]>
Reviewed-by Alberto Garcia <[email protected]>

Berto

author
#9 lore raw

PATCH 5/7 qcow2: use byte-based read in qcow2_decompress_cluster

In reply to 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 <[email protected]>
---
 block/qcow2.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)
block/qcow2.c +10 -6
index e9d24b801e..950b9f7ec6 100644
@@ -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;
Signature · 1 line
2.18.0
#10 lore raw

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

Quoted 26 lines · Vladimir Sementsov-Ogievskiy
block/qcow2.c +4 -3
index e9d24b801e..950b9f7ec6 100644
@@ -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);
         }

block/qcow2.c int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset) · line 3991

Quoted 10 lines
block/qcow2.c +5 -2
 
+        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

author
#11 lore raw

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

Quoted 41 lines · Alberto Garcia
Quoted 26 lines · Vladimir Sementsov-Ogievskiy
block/qcow2.c +4 -3
index e9d24b801e..950b9f7ec6 100644
@@ -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);
          }

block/qcow2.c int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset) · line 3991

Quoted 10 lines
block/qcow2.c +5 -2
 
+        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.

Quoted 1 line

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.

Signature · 2 lines
Best regards,
Vladimir
author
#12 lore raw

PATCH 6/7 qcow2: aio support for compressed cluster read

In reply to 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 <[email protected]>
---
 block/qcow2.h |  4 ---
 block/qcow2.c | 93 ++++++++++++++++++++++++++-------------------------
 2 files changed, 47 insertions(+), 50 deletions(-)
block/qcow2.h +0 -4
index 29c98d87a0..8495d2efbe 100644
@@ -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);
block/qcow2.c +47 -46

149 lines not on this page · raw message

Signature · 1 line
2.18.0
#13 lore raw

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

block/qcow2.c fail: · line 3982

Quoted 12 lines · Vladimir Sementsov-Ogievskiy
block/qcow2.c +8 -3
+    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

author
#14 lore raw

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

Quoted 10 lines · Alberto Garcia
Quoted 8 lines · Vladimir Sementsov-Ogievskiy
block/qcow2.c +7 -0
+    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);
Quoted 5 lines

block/qcow2.c fail: · line 3982

Quoted 4 lines
block/qcow2.c +1 -3
-        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.

Quoted 2 lines

Berto

Signature · 2 lines
Best regards,
Vladimir
#15 lore raw

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

Quoted 11 lines · Vladimir Sementsov-Ogievskiy
Quoted 10 lines · Alberto Garcia
Quoted 8 lines · Vladimir Sementsov-Ogievskiy
block/qcow2.c +7 -0
+    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);
Quoted 8 lines
Quoted 5 lines

block/qcow2.c fail: · line 3982

Quoted 4 lines
block/qcow2.c +1 -3
-        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

author
#16 lore raw

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

Quoted 16 lines · Alberto Garcia
Quoted 15 lines · Vladimir Sementsov-Ogievskiy
Quoted 14 lines · Alberto Garcia
Quoted 12 lines · Vladimir Sementsov-Ogievskiy
block/qcow2.c +8 -3
+    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);
Quoted 8 lines
Quoted 3 lines
Quoted 1 line

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

Signature · 2 lines
Best regards,
Vladimir
Reviewed-by
#17 lore raw

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

Quoted 9 lines · Vladimir Sementsov-Ogievskiy
Quoted 3 lines

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 <[email protected]>

Berto

author
#18 lore raw

PATCH 7/7 qcow2: do decompression in threads

In reply to 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 <[email protected]>
---
 block/qcow2.c | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)
block/qcow2.c +28 -7
index 678d737044..3f1c773b13 100644
@@ -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;
     }
Signature · 1 line
2.18.0
#20 lore raw

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

In reply to Vladimir Sementsov-Ogievskiy
Quoted 20 lines · Vladimir Sementsov-Ogievskiy
block/qcow2.c +5 -4
-/* 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) {

block/qcow2.c static ssize_t qcow2_co_compress(BlockDriverState *bs, · line 3867

Quoted 3 lines
block/qcow2.c +0 -0
@@ -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

#21 lore raw
In reply to Vladimir Sementsov-Ogievskiy

Am 01.11.2018 um 19:27 hat Vladimir Sementsov-Ogievskiy geschrieben:

Quoted 4 lines

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

author
#22 lore raw
Quoted 7 lines · Kevin Wolf

Am 01.11.2018 um 19:27 hat Vladimir Sementsov-Ogievskiy geschrieben:

Quoted 4 lines

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!

Quoted 6 lines

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

Kevin

Signature · 2 lines
Best regards,
Vladimir