[Qemu-devel] [PATCH v3 0/9] qcow2: encryption threads

[Qemu-devel] [PATCH v3 0/9] qcow2: encryption threads

[Vladimir Sementsov-Ogievskiy]

Hi all!

The series brings threads to qcow2 encryption/decryption path,
like it is already done for compression.
Now, based on master

v3:
01: drop 'include block_int.h' lines not needed more [Alberto]
02: fix comments style (open/close on separate line), add Alberto's r-b
03: improve commit message, add Alberto's r-b
04: new, follows Paolo's comment


v2: (not very careful change list)
    - multiple cipher inside QCryptoBlock instead of multiple
      blocks inside qcow2, as suggested by Daniel, and it is
      done in separate series
    - use threaded encryption in do_perform_cow_encrypt() too
    - some renaming and refactoring and simplifications


== Performance testing ==

Hmm, do we have something like tests/qemu-iotests, but for performance?

[root@kvm up-qcow2-encrypt-threads]# cat test.sh 
    #!/bin/bash

    size=1G
    src=/ssd/src.raw
    dst=/ssd/dst.enc.qcow2

    # create source for tests
    ./qemu-img create -f raw "$src" $size > /dev/null
    ./qemu-io -f raw -c "write -P 0xa 0 $size" "$src" > /dev/null

    for w in "" "-W"; do
        echo -e "Test with additional paramter for qemu-img: '$w'"

        # create target
        ./qemu-img create -f qcow2 --object secret,id=sec0,data=test -o encrypt.format=luks,encrypt.key-secret=sec0 "$dst" $size > /dev/null

        time ./qemu-img convert $w -f raw --object secret,id=sec0,data=test --target-image-opts -n "$src" "driver=qcow2,file.filename=$dst,encrypt.key-secret=sec0"
        echo
    done


before the series:
    Test with additional paramter for qemu-img: ''

    real    0m14.318s
    user    0m13.637s
    sys     0m0.881s

    Test with additional paramter for qemu-img: '-W'

    real    0m13.962s
    user    0m13.681s
    sys     0m1.156s


after the series:
    Test with additional paramter for qemu-img: ''

    real    0m14.382s
    user    0m13.735s
    sys     0m0.835s

    Test with additional paramter for qemu-img: '-W'

    real    0m5.696s
    user    0m15.931s
    sys     0m1.144s

Vladimir Sementsov-Ogievskiy (9):
  qcow2.h: add missing include
  qcow2: add separate file for threaded data processing functions
  qcow2-threads: use thread_pool_submit_co
  qcow2-threads: qcow2_co_do_compress: protect queuing by mutex
  qcow2-threads: split out generic path
  qcow2: qcow2_co_preadv: improve locking
  qcow2: qcow2_co_preadv: skip using hd_qiov when possible
  qcow2: bdrv_co_pwritev: move encryption code out of the lock
  qcow2: do encryption in threads

 block/qcow2.h          |  20 ++-
 block/qcow2-bitmap.c   |   1 -
 block/qcow2-cache.c    |   1 -
 block/qcow2-cluster.c  |   8 +-
 block/qcow2-refcount.c |   1 -
 block/qcow2-snapshot.c |   1 -
 block/qcow2-threads.c  | 268 +++++++++++++++++++++++++++++++++++++++
 block/qcow2.c          | 277 ++++++++---------------------------------
 block/Makefile.objs    |   2 +-
 9 files changed, 343 insertions(+), 236 deletions(-)
 create mode 100644 block/qcow2-threads.c

-- 
2.18.0

[Qemu-devel] [PATCH v3 1/9] qcow2.h: add missing include

[Vladimir Sementsov-Ogievskiy]

qcow2.h depends on block_int.h. Compilation isn't broken currently only
due to block_int.h always included before qcow2.h. Though, it seems
better to directly include block_int.h in qcow2.h.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.h          | 1 +
 block/qcow2-bitmap.c   | 1 -
 block/qcow2-cache.c    | 1 -
 block/qcow2-cluster.c  | 1 -
 block/qcow2-refcount.c | 1 -
 block/qcow2-snapshot.c | 1 -
 block/qcow2.c          | 1 -
 7 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index a98d24500b..5095f893a0 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -28,6 +28,7 @@
 #include "crypto/block.h"
 #include "qemu/coroutine.h"
 #include "qemu/units.h"
+#include "block/block_int.h"
 
 //#define DEBUG_ALLOC
 //#define DEBUG_ALLOC2
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index accebef4cf..a12d458d7d 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -29,7 +29,6 @@
 #include "qapi/error.h"
 #include "qemu/cutils.h"
 
-#include "block/block_int.h"
 #include "qcow2.h"
 
 /* NOTICE: BME here means Bitmaps Extension and used as a namespace for
diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index d9dafa31e5..00bbc4ce14 100644
--- a/block/qcow2-cache.c
+++ b/block/qcow2-cache.c
@@ -23,7 +23,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "block/block_int.h"
 #include "qemu-common.h"
 #include "qcow2.h"
 #include "trace.h"
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index e2737429f5..7815772116 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -27,7 +27,6 @@
 
 #include "qapi/error.h"
 #include "qemu-common.h"
-#include "block/block_int.h"
 #include "qcow2.h"
 #include "qemu/bswap.h"
 #include "trace.h"
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 1c63ac244a..d4eee6d350 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -25,7 +25,6 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu-common.h"
-#include "block/block_int.h"
 #include "qcow2.h"
 #include "qemu/range.h"
 #include "qemu/bswap.h"
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index bb6a5b7516..733656fa00 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -24,7 +24,6 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
-#include "block/block_int.h"
 #include "qcow2.h"
 #include "qemu/bswap.h"
 #include "qemu/error-report.h"
diff --git a/block/qcow2.c b/block/qcow2.c
index 4897abae5e..01789de3ef 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -27,7 +27,6 @@
 #define ZLIB_CONST
 #include <zlib.h>
 
-#include "block/block_int.h"
 #include "block/qdict.h"
 #include "sysemu/block-backend.h"
 #include "qemu/module.h"
-- 
2.18.0

Re: [Qemu-devel] [PATCH v3 1/9] qcow2.h: add missing include

[Alberto Garcia]

On Tue 08 Jan 2019 06:06:47 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> qcow2.h depends on block_int.h. Compilation isn't broken currently only
> due to block_int.h always included before qcow2.h. Though, it seems
> better to directly include block_int.h in qcow2.h.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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

Berto

[Qemu-devel] [PATCH v3 2/9] qcow2: add separate file for threaded data processing functions

[Vladimir Sementsov-Ogievskiy]

Move compression-on-threads to separate file. Encryption will be in it
too.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.h         |   7 ++
 block/qcow2-threads.c | 201 ++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c         | 169 -----------------------------------
 block/Makefile.objs   |   2 +-
 4 files changed, 209 insertions(+), 170 deletions(-)
 create mode 100644 block/qcow2-threads.c

diff --git a/block/qcow2.h b/block/qcow2.h
index 5095f893a0..be84d7c96a 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -695,4 +695,11 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
                                           const char *name,
                                           Error **errp);
 
+ssize_t coroutine_fn
+qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
+                  const void *src, size_t src_size);
+ssize_t coroutine_fn
+qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
+                    const void *src, size_t src_size);
+
 #endif
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
new file mode 100644
index 0000000000..dea9ac431c
--- /dev/null
+++ b/block/qcow2-threads.c
@@ -0,0 +1,201 @@
+/*
+ * Threaded data processing for Qcow2: compression, encryption
+ *
+ * Copyright (c) 2004-2006 Fabrice Bellard
+ * Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+
+#define ZLIB_CONST
+#include <zlib.h>
+
+#include "qcow2.h"
+#include "block/thread-pool.h"
+
+#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;
+
+/*
+ * qcow2_compress()
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ *          -1 destination buffer is not enough to store compressed data
+ *          -2 on any other error
+ */
+static ssize_t qcow2_compress(void *dest, size_t dest_size,
+                              const void *src, size_t src_size)
+{
+    ssize_t ret;
+    z_stream strm;
+
+    /* best compression, small window, no zlib header */
+    memset(&strm, 0, sizeof(strm));
+    ret = deflateInit2(&strm, Z_DEFAULT_COMPRESSION, Z_DEFLATED,
+                       -12, 9, Z_DEFAULT_STRATEGY);
+    if (ret != Z_OK) {
+        return -2;
+    }
+
+    /*
+     * 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 = src_size;
+    strm.next_in = (void *) src;
+    strm.avail_out = dest_size;
+    strm.next_out = dest;
+
+    ret = deflate(&strm, Z_FINISH);
+    if (ret == Z_STREAM_END) {
+        ret = dest_size - strm.avail_out;
+    } else {
+        ret = (ret == Z_OK ? -1 : -2);
+    }
+
+    deflateEnd(&strm);
+
+    return ret;
+}
+
+/*
+ * 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)
+{
+    int ret = 0;
+    z_stream strm;
+
+    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);
+    if (ret != Z_OK) {
+        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 ret;
+}
+
+static int qcow2_compress_pool_func(void *opaque)
+{
+    Qcow2CompressData *data = opaque;
+
+    data->ret = data->func(data->dest, data->dest_size,
+                           data->src, data->src_size);
+
+    return 0;
+}
+
+static void qcow2_compress_complete(void *opaque, int ret)
+{
+    qemu_coroutine_enter(opaque);
+}
+
+static ssize_t coroutine_fn
+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;
+    ThreadPool *pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
+    Qcow2CompressData arg = {
+        .dest = dest,
+        .dest_size = dest_size,
+        .src = src,
+        .src_size = src_size,
+        .func = func,
+    };
+
+    while (s->nb_compress_threads >= MAX_COMPRESS_THREADS) {
+        qemu_co_queue_wait(&s->compress_wait_queue, NULL);
+    }
+
+    s->nb_compress_threads++;
+    acb = thread_pool_submit_aio(pool, qcow2_compress_pool_func, &arg,
+                                 qcow2_compress_complete,
+                                 qemu_coroutine_self());
+
+    if (!acb) {
+        s->nb_compress_threads--;
+        return -EINVAL;
+    }
+    qemu_coroutine_yield();
+    s->nb_compress_threads--;
+    qemu_co_queue_next(&s->compress_wait_queue);
+
+    return arg.ret;
+}
+
+ssize_t coroutine_fn
+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);
+}
+
+ssize_t coroutine_fn
+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);
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 01789de3ef..4a1265dc42 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -24,9 +24,6 @@
 
 #include "qemu/osdep.h"
 
-#define ZLIB_CONST
-#include <zlib.h>
-
 #include "block/qdict.h"
 #include "sysemu/block-backend.h"
 #include "qemu/module.h"
@@ -43,7 +40,6 @@
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qapi-visit-block-core.h"
 #include "crypto.h"
-#include "block/thread-pool.h"
 
 /*
   Differences with QCOW:
@@ -3719,171 +3715,6 @@ fail:
     return ret;
 }
 
-/*
- * qcow2_compress()
- *
- * @dest - destination buffer, @dest_size bytes
- * @src - source buffer, @src_size bytes
- *
- * Returns: compressed size on success
- *          -1 destination buffer is not enough to store compressed data
- *          -2 on any other error
- */
-static ssize_t qcow2_compress(void *dest, size_t dest_size,
-                              const void *src, size_t src_size)
-{
-    ssize_t ret;
-    z_stream strm;
-
-    /* best compression, small window, no zlib header */
-    memset(&strm, 0, sizeof(strm));
-    ret = deflateInit2(&strm, Z_DEFAULT_COMPRESSION, Z_DEFLATED,
-                       -12, 9, Z_DEFAULT_STRATEGY);
-    if (ret != Z_OK) {
-        return -2;
-    }
-
-    /* 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 = src_size;
-    strm.next_in = (void *) src;
-    strm.avail_out = dest_size;
-    strm.next_out = dest;
-
-    ret = deflate(&strm, Z_FINISH);
-    if (ret == Z_STREAM_END) {
-        ret = dest_size - strm.avail_out;
-    } else {
-        ret = (ret == Z_OK ? -1 : -2);
-    }
-
-    deflateEnd(&strm);
-
-    return ret;
-}
-
-/*
- * 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)
-{
-    int ret = 0;
-    z_stream strm;
-
-    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);
-    if (ret != Z_OK) {
-        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 ret;
-}
-
-#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 = data->func(data->dest, data->dest_size,
-                           data->src, data->src_size);
-
-    return 0;
-}
-
-static void qcow2_compress_complete(void *opaque, int ret)
-{
-    qemu_coroutine_enter(opaque);
-}
-
-static ssize_t coroutine_fn
-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;
-    ThreadPool *pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
-    Qcow2CompressData arg = {
-        .dest = dest,
-        .dest_size = dest_size,
-        .src = src,
-        .src_size = src_size,
-        .func = func,
-    };
-
-    while (s->nb_compress_threads >= MAX_COMPRESS_THREADS) {
-        qemu_co_queue_wait(&s->compress_wait_queue, NULL);
-    }
-
-    s->nb_compress_threads++;
-    acb = thread_pool_submit_aio(pool, qcow2_compress_pool_func, &arg,
-                                 qcow2_compress_complete,
-                                 qemu_coroutine_self());
-
-    if (!acb) {
-        s->nb_compress_threads--;
-        return -EINVAL;
-    }
-    qemu_coroutine_yield();
-    s->nb_compress_threads--;
-    qemu_co_queue_next(&s->compress_wait_queue);
-
-    return arg.ret;
-}
-
-static ssize_t coroutine_fn
-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 coroutine_fn
-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
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 7a81892a52..ae11605c9f 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -6,7 +6,7 @@ block-obj-$(CONFIG_BOCHS) += bochs.o
 block-obj-$(CONFIG_VVFAT) += vvfat.o
 block-obj-$(CONFIG_DMG) += dmg.o
 
-block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o qcow2-bitmap.o
+block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o qcow2-bitmap.o qcow2-threads.o
 block-obj-$(CONFIG_QED) += qed.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-obj-$(CONFIG_QED) += qed-check.o
 block-obj-y += vhdx.o vhdx-endian.o vhdx-log.o
-- 
2.18.0

[Qemu-devel] [PATCH v3 3/9] qcow2-threads: use thread_pool_submit_co

[Vladimir Sementsov-Ogievskiy]

Use thread_pool_submit_co, instead of reinventing it here. Note, that
thread_pool_submit_aio() never returns NULL, so checking it was an
extra thing.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2-threads.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index dea9ac431c..20b2616529 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -144,17 +144,11 @@ static int qcow2_compress_pool_func(void *opaque)
     return 0;
 }
 
-static void qcow2_compress_complete(void *opaque, int ret)
-{
-    qemu_coroutine_enter(opaque);
-}
-
 static ssize_t coroutine_fn
 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;
     ThreadPool *pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
     Qcow2CompressData arg = {
         .dest = dest,
@@ -169,16 +163,9 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, size_t dest_size,
     }
 
     s->nb_compress_threads++;
-    acb = thread_pool_submit_aio(pool, qcow2_compress_pool_func, &arg,
-                                 qcow2_compress_complete,
-                                 qemu_coroutine_self());
-
-    if (!acb) {
-        s->nb_compress_threads--;
-        return -EINVAL;
-    }
-    qemu_coroutine_yield();
+    thread_pool_submit_co(pool, qcow2_compress_pool_func, &arg);
     s->nb_compress_threads--;
+
     qemu_co_queue_next(&s->compress_wait_queue);
 
     return arg.ret;
-- 
2.18.0

[Qemu-devel] [PATCH v3 4/9] qcow2-threads: qcow2_co_do_compress: protect queuing by mutex

[Vladimir Sementsov-Ogievskiy]

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

diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 20b2616529..156e0667be 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -158,15 +158,19 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, size_t dest_size,
         .func = func,
     };
 
+    qemu_co_mutex_lock(&s->lock);
     while (s->nb_compress_threads >= MAX_COMPRESS_THREADS) {
-        qemu_co_queue_wait(&s->compress_wait_queue, NULL);
+        qemu_co_queue_wait(&s->compress_wait_queue, &s->lock);
     }
-
     s->nb_compress_threads++;
+    qemu_co_mutex_unlock(&s->lock);
+
     thread_pool_submit_co(pool, qcow2_compress_pool_func, &arg);
-    s->nb_compress_threads--;
 
+    qemu_co_mutex_lock(&s->lock);
+    s->nb_compress_threads--;
     qemu_co_queue_next(&s->compress_wait_queue);
+    qemu_co_mutex_unlock(&s->lock);
 
     return arg.ret;
 }
-- 
2.18.0

Re: [Qemu-devel] [PATCH v3 4/9] qcow2-threads: qcow2_co_do_compress: protect queuing by mutex

[Paolo Bonzini]

On 08/01/19 18:06, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2-threads.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> index 20b2616529..156e0667be 100644
> --- a/block/qcow2-threads.c
> +++ b/block/qcow2-threads.c
> @@ -158,15 +158,19 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, size_t dest_size,
>          .func = func,
>      };
>  
> +    qemu_co_mutex_lock(&s->lock);
>      while (s->nb_compress_threads >= MAX_COMPRESS_THREADS) {
> -        qemu_co_queue_wait(&s->compress_wait_queue, NULL);
> +        qemu_co_queue_wait(&s->compress_wait_queue, &s->lock);
>      }
> -
>      s->nb_compress_threads++;
> +    qemu_co_mutex_unlock(&s->lock);
> +
>      thread_pool_submit_co(pool, qcow2_compress_pool_func, &arg);
> -    s->nb_compress_threads--;
>  
> +    qemu_co_mutex_lock(&s->lock);
> +    s->nb_compress_threads--;
>      qemu_co_queue_next(&s->compress_wait_queue);
> +    qemu_co_mutex_unlock(&s->lock);
>  
>      return arg.ret;
>  }
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

but, some information would be nice to have in the commit message.

Paolo

Re: [Qemu-devel] [PATCH v3 4/9] qcow2-threads: qcow2_co_do_compress: protect queuing by mutex

[Vladimir Sementsov-Ogievskiy]

16.01.2019 2:29, Paolo Bonzini wrote:
> On 08/01/19 18:06, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2-threads.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
>> index 20b2616529..156e0667be 100644
>> --- a/block/qcow2-threads.c
>> +++ b/block/qcow2-threads.c
>> @@ -158,15 +158,19 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, size_t dest_size,
>>           .func = func,
>>       };
>>   
>> +    qemu_co_mutex_lock(&s->lock);
>>       while (s->nb_compress_threads >= MAX_COMPRESS_THREADS) {
>> -        qemu_co_queue_wait(&s->compress_wait_queue, NULL);
>> +        qemu_co_queue_wait(&s->compress_wait_queue, &s->lock);
>>       }
>> -
>>       s->nb_compress_threads++;
>> +    qemu_co_mutex_unlock(&s->lock);
>> +
>>       thread_pool_submit_co(pool, qcow2_compress_pool_func, &arg);
>> -    s->nb_compress_threads--;
>>   
>> +    qemu_co_mutex_lock(&s->lock);
>> +    s->nb_compress_threads--;
>>       qemu_co_queue_next(&s->compress_wait_queue);
>> +    qemu_co_mutex_unlock(&s->lock);
>>   
>>       return arg.ret;
>>   }
>>
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> but, some information would be nice to have in the commit message.

Could you suggest one?

Just, "To make it thread-safe."? I don't have good understanding
of moving from "AioContext lock usage" to other locks, as well as
I don't know how BlockDriverState is going to be used in parallel
threads, and how much of other code is already prepared to work
in multiple-threads environment.

I looked through your commit messages on patches which added protection
to similar places about co-queues, but didn't find detailed description.

> 
> Paolo
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 4/9] qcow2-threads: qcow2_co_do_compress: protect queuing by mutex

[Alberto Garcia]

On Tue 08 Jan 2019 06:06:50 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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

Berto

[Qemu-devel] [PATCH v3 5/9] qcow2-threads: split out generic path

[Vladimir Sementsov-Ogievskiy]

Move generic part out of qcow2_co_do_compress, to reuse it for
encryption and rename things that would be shared with encryption path.

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

diff --git a/block/qcow2.h b/block/qcow2.h
index be84d7c96a..9c2f6749ba 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -336,8 +336,8 @@ typedef struct BDRVQcow2State {
     char *image_backing_file;
     char *image_backing_format;
 
-    CoQueue compress_wait_queue;
-    int nb_compress_threads;
+    CoQueue thread_task_queue;
+    int nb_threads;
 } BDRVQcow2State;
 
 typedef struct Qcow2COWRegion {
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 156e0667be..e3066075da 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -31,7 +31,36 @@
 #include "qcow2.h"
 #include "block/thread-pool.h"
 
-#define MAX_COMPRESS_THREADS 4
+#define QCOW2_MAX_THREADS 4
+
+static int coroutine_fn
+qcow2_co_process(BlockDriverState *bs, ThreadPoolFunc *func, void *arg)
+{
+    int ret;
+    BDRVQcow2State *s = bs->opaque;
+    ThreadPool *pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
+
+    qemu_co_mutex_lock(&s->lock);
+    while (s->nb_threads >= QCOW2_MAX_THREADS) {
+        qemu_co_queue_wait(&s->thread_task_queue, &s->lock);
+    }
+    s->nb_threads++;
+    qemu_co_mutex_unlock(&s->lock);
+
+    ret = thread_pool_submit_co(pool, func, arg);
+
+    qemu_co_mutex_lock(&s->lock);
+    s->nb_threads--;
+    qemu_co_queue_next(&s->thread_task_queue);
+    qemu_co_mutex_unlock(&s->lock);
+
+    return ret;
+}
+
+
+/*
+ * Compression
+ */
 
 typedef ssize_t (*Qcow2CompressFunc)(void *dest, size_t dest_size,
                                      const void *src, size_t src_size);
@@ -148,8 +177,6 @@ static ssize_t coroutine_fn
 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;
-    ThreadPool *pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
     Qcow2CompressData arg = {
         .dest = dest,
         .dest_size = dest_size,
@@ -158,19 +185,7 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, size_t dest_size,
         .func = func,
     };
 
-    qemu_co_mutex_lock(&s->lock);
-    while (s->nb_compress_threads >= MAX_COMPRESS_THREADS) {
-        qemu_co_queue_wait(&s->compress_wait_queue, &s->lock);
-    }
-    s->nb_compress_threads++;
-    qemu_co_mutex_unlock(&s->lock);
-
-    thread_pool_submit_co(pool, qcow2_compress_pool_func, &arg);
-
-    qemu_co_mutex_lock(&s->lock);
-    s->nb_compress_threads--;
-    qemu_co_queue_next(&s->compress_wait_queue);
-    qemu_co_mutex_unlock(&s->lock);
+    qcow2_co_process(bs, qcow2_compress_pool_func, &arg);
 
     return arg.ret;
 }
diff --git a/block/qcow2.c b/block/qcow2.c
index 4a1265dc42..2865903b0e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1599,7 +1599,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
     }
 #endif
 
-    qemu_co_queue_init(&s->compress_wait_queue);
+    qemu_co_queue_init(&s->thread_task_queue);
 
     return ret;
 
-- 
2.18.0

Re: [Qemu-devel] [PATCH v3 5/9] qcow2-threads: split out generic path

[Alberto Garcia]

On Tue 08 Jan 2019 06:06:51 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> Move generic part out of qcow2_co_do_compress, to reuse it for
> encryption and rename things that would be shared with encryption path.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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

Berto

[Qemu-devel] [PATCH v3 6/9] qcow2: qcow2_co_preadv: improve locking

[Vladimir Sementsov-Ogievskiy]

Background: decryption will be done in threads, to take benefit of it,
we should move it out of the lock first.

But let's go further: it turns out, that for locking around switch
cases we have only two variants: when we just do memset(0) not
releasing the lock (it is useless) and when we actually can handle the
whole case out of the lock. So, refactor the whole thing to reduce
locked code region and make it clean.

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

diff --git a/block/qcow2.c b/block/qcow2.c
index 2865903b0e..e650528af7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1883,6 +1883,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
 
         ret = qcow2_get_cluster_offset(bs, offset, &cur_bytes, &cluster_offset);
         if (ret < 0) {
+            qemu_co_mutex_unlock(&s->lock);
             goto fail;
         }
 
@@ -1891,39 +1892,38 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
         qemu_iovec_reset(&hd_qiov);
         qemu_iovec_concat(&hd_qiov, qiov, bytes_done, cur_bytes);
 
+        if (ret == QCOW2_CLUSTER_ZERO_PLAIN ||
+            ret == QCOW2_CLUSTER_ZERO_ALLOC ||
+            (ret == QCOW2_CLUSTER_UNALLOCATED && !bs->backing))
+        {
+            /* No sense in releasing the lock */
+
+            qemu_iovec_memset(&hd_qiov, 0, 0, cur_bytes);
+
+            bytes -= cur_bytes;
+            offset += cur_bytes;
+            bytes_done += cur_bytes;
+            continue;
+        }
+
+        qemu_co_mutex_unlock(&s->lock);
+
         switch (ret) {
         case QCOW2_CLUSTER_UNALLOCATED:
-
-            if (bs->backing) {
-                BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
-                qemu_co_mutex_unlock(&s->lock);
-                ret = bdrv_co_preadv(bs->backing, offset, cur_bytes,
-                                     &hd_qiov, 0);
-                qemu_co_mutex_lock(&s->lock);
-                if (ret < 0) {
-                    goto fail;
-                }
-            } else {
-                /* Note: in this case, no need to wait */
-                qemu_iovec_memset(&hd_qiov, 0, 0, cur_bytes);
+            BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
+            ret = bdrv_co_preadv(bs->backing, offset, cur_bytes, &hd_qiov, 0);
+            if (ret < 0) {
+                goto fail;
             }
             break;
 
-        case QCOW2_CLUSTER_ZERO_PLAIN:
-        case QCOW2_CLUSTER_ZERO_ALLOC:
-            qemu_iovec_memset(&hd_qiov, 0, 0, cur_bytes);
-            break;
-
         case QCOW2_CLUSTER_COMPRESSED:
-            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;
             }
-
             break;
 
         case QCOW2_CLUSTER_NORMAL:
@@ -1956,11 +1956,9 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
             }
 
             BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
-            qemu_co_mutex_unlock(&s->lock);
             ret = bdrv_co_preadv(bs->file,
                                  cluster_offset + offset_in_cluster,
                                  cur_bytes, &hd_qiov, 0);
-            qemu_co_mutex_lock(&s->lock);
             if (ret < 0) {
                 goto fail;
             }
@@ -1991,12 +1989,14 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
         bytes -= cur_bytes;
         offset += cur_bytes;
         bytes_done += cur_bytes;
+
+        qemu_co_mutex_lock(&s->lock);
     }
     ret = 0;
 
-fail:
     qemu_co_mutex_unlock(&s->lock);
 
+fail:
     qemu_iovec_destroy(&hd_qiov);
     qemu_vfree(cluster_data);
 
-- 
2.18.0

Re: [Qemu-devel] [PATCH v3 6/9] qcow2: qcow2_co_preadv: improve locking

[Alberto Garcia]

On Tue 08 Jan 2019 06:06:52 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> @@ -1891,39 +1892,38 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>          qemu_iovec_reset(&hd_qiov);
>          qemu_iovec_concat(&hd_qiov, qiov, bytes_done, cur_bytes);
>  
> +        if (ret == QCOW2_CLUSTER_ZERO_PLAIN ||
> +            ret == QCOW2_CLUSTER_ZERO_ALLOC ||
> +            (ret == QCOW2_CLUSTER_UNALLOCATED && !bs->backing))
> +        {
> +            /* No sense in releasing the lock */
> +
> +            qemu_iovec_memset(&hd_qiov, 0, 0, cur_bytes);
> +
> +            bytes -= cur_bytes;
> +            offset += cur_bytes;
> +            bytes_done += cur_bytes;
> +            continue;
> +        }

Maybe that was just me, but I got confused by the "No sense in releasing
the lock" thinking that it referred to the memset() call.

What you're trying to say is that it doesn't make sense to release the
lock while you only get zero / unallocated clusters only to lock it
again at the next iteration of the loop.

I think that comment may make sense when you think about the code
changes that you are doing, but when you read the actual resulting code
it's fairly obvious that you don't need to touch the lock there.

So I'd rather remove that comment.

Else the patch looks good.

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

Berto

[Qemu-devel] [PATCH v3 7/9] qcow2: qcow2_co_preadv: skip using hd_qiov when possible

[Vladimir Sementsov-Ogievskiy]

qemu_iovec_memset has @offset parameter, so using hd_qiov for it is not
needed.

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

diff --git a/block/qcow2.c b/block/qcow2.c
index e650528af7..d6ef606d89 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1887,18 +1887,13 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
             goto fail;
         }
 
-        offset_in_cluster = offset_into_cluster(s, offset);
-
-        qemu_iovec_reset(&hd_qiov);
-        qemu_iovec_concat(&hd_qiov, qiov, bytes_done, cur_bytes);
-
         if (ret == QCOW2_CLUSTER_ZERO_PLAIN ||
             ret == QCOW2_CLUSTER_ZERO_ALLOC ||
             (ret == QCOW2_CLUSTER_UNALLOCATED && !bs->backing))
         {
             /* No sense in releasing the lock */
 
-            qemu_iovec_memset(&hd_qiov, 0, 0, cur_bytes);
+            qemu_iovec_memset(qiov, bytes_done, 0, cur_bytes);
 
             bytes -= cur_bytes;
             offset += cur_bytes;
@@ -1906,6 +1901,11 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
             continue;
         }
 
+        offset_in_cluster = offset_into_cluster(s, offset);
+
+        qemu_iovec_reset(&hd_qiov);
+        qemu_iovec_concat(&hd_qiov, qiov, bytes_done, cur_bytes);
+
         qemu_co_mutex_unlock(&s->lock);
 
         switch (ret) {
-- 
2.18.0

Re: [Qemu-devel] [PATCH v3 7/9] qcow2: qcow2_co_preadv: skip using hd_qiov when possible

[Alberto Garcia]

On Tue 08 Jan 2019 06:06:53 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> qemu_iovec_memset has @offset parameter, so using hd_qiov for it is not
> needed.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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

Berto

[Qemu-devel] [PATCH v3 8/9] qcow2: bdrv_co_pwritev: move encryption code out of the lock

[Vladimir Sementsov-Ogievskiy]

Encryption will be done in threads, to take benefit of it, we should
move it out of the lock first.

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

diff --git a/block/qcow2.c b/block/qcow2.c
index d6ef606d89..76d3715350 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2077,11 +2077,20 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
         ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
                                          &cluster_offset, &l2meta);
         if (ret < 0) {
-            goto fail;
+            goto out_locked;
         }
 
         assert((cluster_offset & 511) == 0);
 
+        ret = qcow2_pre_write_overlap_check(bs, 0,
+                                            cluster_offset + offset_in_cluster,
+                                            cur_bytes);
+        if (ret < 0) {
+            goto out_locked;
+        }
+
+        qemu_co_mutex_unlock(&s->lock);
+
         qemu_iovec_reset(&hd_qiov);
         qemu_iovec_concat(&hd_qiov, qiov, bytes_done, cur_bytes);
 
@@ -2093,7 +2102,7 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
                                                    * s->cluster_size);
                 if (cluster_data == NULL) {
                     ret = -ENOMEM;
-                    goto fail;
+                    goto out_unlocked;
                 }
             }
 
@@ -2108,40 +2117,34 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
                                       cluster_data,
                                       cur_bytes, NULL) < 0) {
                 ret = -EIO;
-                goto fail;
+                goto out_unlocked;
             }
 
             qemu_iovec_reset(&hd_qiov);
             qemu_iovec_add(&hd_qiov, cluster_data, cur_bytes);
         }
 
-        ret = qcow2_pre_write_overlap_check(bs, 0,
-                cluster_offset + offset_in_cluster, cur_bytes);
-        if (ret < 0) {
-            goto fail;
-        }
-
         /* If we need to do COW, check if it's possible to merge the
          * writing of the guest data together with that of the COW regions.
          * If it's not possible (or not necessary) then write the
          * guest data now. */
         if (!merge_cow(offset, cur_bytes, &hd_qiov, l2meta)) {
-            qemu_co_mutex_unlock(&s->lock);
             BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
             trace_qcow2_writev_data(qemu_coroutine_self(),
                                     cluster_offset + offset_in_cluster);
             ret = bdrv_co_pwritev(bs->file,
                                   cluster_offset + offset_in_cluster,
                                   cur_bytes, &hd_qiov, 0);
-            qemu_co_mutex_lock(&s->lock);
             if (ret < 0) {
-                goto fail;
+                goto out_unlocked;
             }
         }
 
+        qemu_co_mutex_lock(&s->lock);
+
         ret = qcow2_handle_l2meta(bs, &l2meta, true);
         if (ret) {
-            goto fail;
+            goto out_locked;
         }
 
         bytes -= cur_bytes;
@@ -2150,8 +2153,12 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
         trace_qcow2_writev_done_part(qemu_coroutine_self(), cur_bytes);
     }
     ret = 0;
+    goto out_locked;
 
-fail:
+out_unlocked:
+    qemu_co_mutex_lock(&s->lock);
+
+out_locked:
     qcow2_handle_l2meta(bs, &l2meta, false);
 
     qemu_co_mutex_unlock(&s->lock);
-- 
2.18.0

Re: [Qemu-devel] [PATCH v3 8/9] qcow2: bdrv_co_pwritev: move encryption code out of the lock

[Alberto Garcia]

On Tue 08 Jan 2019 06:06:54 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> Encryption will be done in threads, to take benefit of it, we should
> move it out of the lock first.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index d6ef606d89..76d3715350 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2077,11 +2077,20 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
>          ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
>                                           &cluster_offset, &l2meta);
>          if (ret < 0) {
> -            goto fail;
> +            goto out_locked;
>          }
>  
>          assert((cluster_offset & 511) == 0);
>  
> +        ret = qcow2_pre_write_overlap_check(bs, 0,
> +                                            cluster_offset + offset_in_cluster,
> +                                            cur_bytes);
> +        if (ret < 0) {
> +            goto out_locked;
> +        }
> +
> +        qemu_co_mutex_unlock(&s->lock);

The usage of lock() and unlock() functions inside and outside of the
loop plus the two 'locked' and 'unlocked' exit paths is starting to make
things a bit more messy.

Having a look at the code it seems that there's only these three parts
in the whole function that need to have the lock held:

one:
   ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
                                    &cluster_offset, &l2meta);
   /* ... */
   ret = qcow2_pre_write_overlap_check(bs, 0,
                                       cluster_offset +
                                       offset_in_cluster,
                                       cur_bytes);

two:

   ret = qcow2_handle_l2meta(bs, &l2meta, true);


three:
   qcow2_handle_l2meta(bs, &l2meta, false);


So I wonder if it's perhaps simpler to add lock/unlock calls around
those blocks?

Berto

Re: [Qemu-devel] [PATCH v3 8/9] qcow2: bdrv_co_pwritev: move encryption code out of the lock

[Vladimir Sementsov-Ogievskiy]

18.01.2019 12:51, Alberto Garcia wrote:
> On Tue 08 Jan 2019 06:06:54 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>> Encryption will be done in threads, to take benefit of it, we should
>> move it out of the lock first.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2.c | 35 +++++++++++++++++++++--------------
>>   1 file changed, 21 insertions(+), 14 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index d6ef606d89..76d3715350 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2077,11 +2077,20 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
>>           ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
>>                                            &cluster_offset, &l2meta);
>>           if (ret < 0) {
>> -            goto fail;
>> +            goto out_locked;
>>           }
>>   
>>           assert((cluster_offset & 511) == 0);
>>   
>> +        ret = qcow2_pre_write_overlap_check(bs, 0,
>> +                                            cluster_offset + offset_in_cluster,
>> +                                            cur_bytes);
>> +        if (ret < 0) {
>> +            goto out_locked;
>> +        }
>> +
>> +        qemu_co_mutex_unlock(&s->lock);
> 
> The usage of lock() and unlock() functions inside and outside of the
> loop plus the two 'locked' and 'unlocked' exit paths is starting to make
> things a bit more messy.
> 
> Having a look at the code it seems that there's only these three parts
> in the whole function that need to have the lock held:
> 
> one:
>     ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
>                                      &cluster_offset, &l2meta);
>     /* ... */
>     ret = qcow2_pre_write_overlap_check(bs, 0,
>                                         cluster_offset +
>                                         offset_in_cluster,
>                                         cur_bytes);
> 
> two:
> 
>     ret = qcow2_handle_l2meta(bs, &l2meta, true);
> 
> 
> three:
>     qcow2_handle_l2meta(bs, &l2meta, false);
> 
> 
> So I wonder if it's perhaps simpler to add lock/unlock calls around
> those blocks?

this means, that we'll unlock after qcow2_handle_l2meta and then immediately
lock on next iteration before qcow2_alloc_cluster_offset, so current code
avoids this extra unlock/lock..


> 
> Berto
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 8/9] qcow2: bdrv_co_pwritev: move encryption code out of the lock

[Alberto Garcia]

On Fri 18 Jan 2019 12:37:42 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> 18.01.2019 12:51, Alberto Garcia wrote:
>> On Tue 08 Jan 2019 06:06:54 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>>> Encryption will be done in threads, to take benefit of it, we should
>>> move it out of the lock first.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/qcow2.c | 35 +++++++++++++++++++++--------------
>>>   1 file changed, 21 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index d6ef606d89..76d3715350 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -2077,11 +2077,20 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
>>>           ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
>>>                                            &cluster_offset, &l2meta);
>>>           if (ret < 0) {
>>> -            goto fail;
>>> +            goto out_locked;
>>>           }
>>>   
>>>           assert((cluster_offset & 511) == 0);
>>>   
>>> +        ret = qcow2_pre_write_overlap_check(bs, 0,
>>> +                                            cluster_offset + offset_in_cluster,
>>> +                                            cur_bytes);
>>> +        if (ret < 0) {
>>> +            goto out_locked;
>>> +        }
>>> +
>>> +        qemu_co_mutex_unlock(&s->lock);
>> 
>> The usage of lock() and unlock() functions inside and outside of the
>> loop plus the two 'locked' and 'unlocked' exit paths is starting to make
>> things a bit more messy.
>> 
>> Having a look at the code it seems that there's only these three parts
>> in the whole function that need to have the lock held:
>> 
>> one:
>>     ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
>>                                      &cluster_offset, &l2meta);
>>     /* ... */
>>     ret = qcow2_pre_write_overlap_check(bs, 0,
>>                                         cluster_offset +
>>                                         offset_in_cluster,
>>                                         cur_bytes);
>> 
>> two:
>> 
>>     ret = qcow2_handle_l2meta(bs, &l2meta, true);
>> 
>> 
>> three:
>>     qcow2_handle_l2meta(bs, &l2meta, false);
>> 
>> 
>> So I wonder if it's perhaps simpler to add lock/unlock calls around
>> those blocks?
>
> this means, that we'll unlock after qcow2_handle_l2meta and then
> immediately lock on next iteration before qcow2_alloc_cluster_offset,
> so current code avoids this extra unlock/lock..

I don't have a very strong opinion, but maybe it's worth having if it
makes the code easier to read and maintain.

Berto

Re: [Qemu-devel] [PATCH v3 8/9] qcow2: bdrv_co_pwritev: move encryption code out of the lock

[Vladimir Sementsov-Ogievskiy]

18.01.2019 17:39, Alberto Garcia wrote:
> On Fri 18 Jan 2019 12:37:42 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>> 18.01.2019 12:51, Alberto Garcia wrote:
>>> On Tue 08 Jan 2019 06:06:54 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>>>> Encryption will be done in threads, to take benefit of it, we should
>>>> move it out of the lock first.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    block/qcow2.c | 35 +++++++++++++++++++++--------------
>>>>    1 file changed, 21 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>> index d6ef606d89..76d3715350 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>>> @@ -2077,11 +2077,20 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
>>>>            ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
>>>>                                             &cluster_offset, &l2meta);
>>>>            if (ret < 0) {
>>>> -            goto fail;
>>>> +            goto out_locked;
>>>>            }
>>>>    
>>>>            assert((cluster_offset & 511) == 0);
>>>>    
>>>> +        ret = qcow2_pre_write_overlap_check(bs, 0,
>>>> +                                            cluster_offset + offset_in_cluster,
>>>> +                                            cur_bytes);
>>>> +        if (ret < 0) {
>>>> +            goto out_locked;
>>>> +        }
>>>> +
>>>> +        qemu_co_mutex_unlock(&s->lock);
>>>
>>> The usage of lock() and unlock() functions inside and outside of the
>>> loop plus the two 'locked' and 'unlocked' exit paths is starting to make
>>> things a bit more messy.
>>>
>>> Having a look at the code it seems that there's only these three parts
>>> in the whole function that need to have the lock held:
>>>
>>> one:
>>>      ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
>>>                                       &cluster_offset, &l2meta);
>>>      /* ... */
>>>      ret = qcow2_pre_write_overlap_check(bs, 0,
>>>                                          cluster_offset +
>>>                                          offset_in_cluster,
>>>                                          cur_bytes);
>>>
>>> two:
>>>
>>>      ret = qcow2_handle_l2meta(bs, &l2meta, true);
>>>
>>>
>>> three:
>>>      qcow2_handle_l2meta(bs, &l2meta, false);
>>>
>>>
>>> So I wonder if it's perhaps simpler to add lock/unlock calls around
>>> those blocks?
>>
>> this means, that we'll unlock after qcow2_handle_l2meta and then
>> immediately lock on next iteration before qcow2_alloc_cluster_offset,
>> so current code avoids this extra unlock/lock..
> 
> I don't have a very strong opinion, but maybe it's worth having if it
> makes the code easier to read and maintain.
> 

Intuitively I think, that locks optimization worth this difficulty,
so I'd prefer to leave this logic as is, at least in these series.


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH v3 9/9] qcow2: do encryption in threads

[Vladimir Sementsov-Ogievskiy]

Do encryption/decryption in threads, like it is already done for
compression. This improves asynchronous encrypted io.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.h         |  8 ++++++
 block/qcow2-cluster.c |  7 ++---
 block/qcow2-threads.c | 65 +++++++++++++++++++++++++++++++++++++++++--
 block/qcow2.c         | 22 +++++----------
 4 files changed, 81 insertions(+), 21 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 9c2f6749ba..7e75f20373 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -257,6 +257,8 @@ typedef struct Qcow2BitmapHeaderExt {
     uint64_t bitmap_directory_offset;
 } QEMU_PACKED Qcow2BitmapHeaderExt;
 
+#define QCOW2_MAX_THREADS 4
+
 typedef struct BDRVQcow2State {
     int cluster_bits;
     int cluster_size;
@@ -701,5 +703,11 @@ qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
 ssize_t coroutine_fn
 qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
                     const void *src, size_t src_size);
+int coroutine_fn
+qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
+                 uint64_t offset, void *buf, size_t len);
+int coroutine_fn
+qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
+                 uint64_t offset, void *buf, size_t len);
 
 #endif
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 7815772116..55dd4144de 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -467,13 +467,12 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
 {
     if (bytes && bs->encrypted) {
         BDRVQcow2State *s = bs->opaque;
-        int64_t offset = (s->crypt_physical_offset ?
-                          (cluster_offset + offset_in_cluster) :
-                          (src_cluster_offset + offset_in_cluster));
         assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
         assert((bytes & ~BDRV_SECTOR_MASK) == 0);
         assert(s->crypto);
-        if (qcrypto_block_encrypt(s->crypto, offset, buffer, bytes, NULL) < 0) {
+        if (qcow2_co_encrypt(bs, cluster_offset,
+                             src_cluster_offset + offset_in_cluster,
+                             buffer, bytes) < 0) {
             return false;
         }
     }
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index e3066075da..0e20723a4b 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -30,8 +30,7 @@
 
 #include "qcow2.h"
 #include "block/thread-pool.h"
-
-#define QCOW2_MAX_THREADS 4
+#include "crypto.h"
 
 static int coroutine_fn
 qcow2_co_process(BlockDriverState *bs, ThreadPoolFunc *func, void *arg)
@@ -205,3 +204,65 @@ qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
     return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
                                 qcow2_decompress);
 }
+
+
+/*
+ * Cryptography
+ */
+
+/*
+ * Qcow2EncDecFunc: common prototype of qcrypto_block_encrypt() and
+ * qcrypto_block_decrypt() functions.
+ */
+typedef int (*Qcow2EncDecFunc)(QCryptoBlock *block, uint64_t offset,
+                               uint8_t *buf, size_t len, Error **errp);
+
+typedef struct Qcow2EncDecData {
+    QCryptoBlock *block;
+    uint64_t offset;
+    uint8_t *buf;
+    size_t len;
+
+    Qcow2EncDecFunc func;
+} Qcow2EncDecData;
+
+static int qcow2_encdec_pool_func(void *opaque)
+{
+    Qcow2EncDecData *data = opaque;
+
+    return data->func(data->block, data->offset, data->buf, data->len, NULL);
+}
+
+static int coroutine_fn
+qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
+                  uint64_t offset, void *buf, size_t len, Qcow2EncDecFunc func)
+{
+    BDRVQcow2State *s = bs->opaque;
+    Qcow2EncDecData arg = {
+        .block = s->crypto,
+        .offset = s->crypt_physical_offset ?
+                      file_cluster_offset + offset_into_cluster(s, offset) :
+                      offset,
+        .buf = buf,
+        .len = len,
+        .func = func,
+    };
+
+    return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
+}
+
+int coroutine_fn
+qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
+                 uint64_t offset, void *buf, size_t len)
+{
+    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
+                             qcrypto_block_encrypt);
+}
+
+int coroutine_fn
+qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
+                 uint64_t offset, void *buf, size_t len)
+{
+    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
+                             qcrypto_block_decrypt);
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 76d3715350..f94e5960f2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -296,7 +296,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
             }
             s->crypto = qcrypto_block_open(s->crypto_opts, "encrypt.",
                                            qcow2_crypto_hdr_read_func,
-                                           bs, cflags, 1, errp);
+                                           bs, cflags, QCOW2_MAX_THREADS, errp);
             if (!s->crypto) {
                 return -EINVAL;
             }
@@ -1446,7 +1446,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
                 cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
             }
             s->crypto = qcrypto_block_open(s->crypto_opts, "encrypt.",
-                                           NULL, NULL, cflags, 1, errp);
+                                           NULL, NULL, cflags,
+                                           QCOW2_MAX_THREADS, errp);
             if (!s->crypto) {
                 ret = -EINVAL;
                 goto fail;
@@ -1966,13 +1967,8 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
                 assert(s->crypto);
                 assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
                 assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
-                if (qcrypto_block_decrypt(s->crypto,
-                                          (s->crypt_physical_offset ?
-                                           cluster_offset + offset_in_cluster :
-                                           offset),
-                                          cluster_data,
-                                          cur_bytes,
-                                          NULL) < 0) {
+                if (qcow2_co_decrypt(bs, cluster_offset, offset,
+                                     cluster_data, cur_bytes) < 0) {
                     ret = -EIO;
                     goto fail;
                 }
@@ -2110,12 +2106,8 @@ static coroutine_fn int qcow2_co_pwritev(BlockDriverState *bs, uint64_t offset,
                    QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size);
             qemu_iovec_to_buf(&hd_qiov, 0, cluster_data, hd_qiov.size);
 
-            if (qcrypto_block_encrypt(s->crypto,
-                                      (s->crypt_physical_offset ?
-                                       cluster_offset + offset_in_cluster :
-                                       offset),
-                                      cluster_data,
-                                      cur_bytes, NULL) < 0) {
+            if (qcow2_co_encrypt(bs, cluster_offset, offset,
+                                 cluster_data, cur_bytes) < 0) {
                 ret = -EIO;
                 goto out_unlocked;
             }
-- 
2.18.0

Re: [Qemu-devel] [PATCH v3 9/9] qcow2: do encryption in threads

[Alberto Garcia]

On Tue 08 Jan 2019 06:06:55 PM CET, Vladimir Sementsov-Ogievskiy wrote:
> Do encryption/decryption in threads, like it is already done for
> compression. This improves asynchronous encrypted io.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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

Berto

Re: [Qemu-devel] [PATCH v3 0/9] qcow2: encryption threads

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190108170655.29766-1-vsementsov@virtuozzo.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14
=== TEST SCRIPT END ===

  CC      qapi/qapi-commands-crypto.o
  CC      qapi/qapi-commands-introspect.o
/tmp/qemu-test/src/block/sheepdog.c: In function 'find_vdi_name':
/tmp/qemu-test/src/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals destination size [-Werror=stringop-truncation]
     strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors


The full log is available at
http://patchew.org/logs/20190108170655.29766-1-vsementsov@virtuozzo.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v3 0/9] qcow2: encryption threads

[Vladimir Sementsov-Ogievskiy]

23.01.2019 17:54, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20190108170655.29766-1-vsementsov@virtuozzo.com/
> 
> 
> 
> Hi,
> 
> This series failed the docker-mingw@fedora build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> time make docker-test-mingw@fedora SHOW_ENV=1 J=14
> === TEST SCRIPT END ===
> 
>    CC      qapi/qapi-commands-crypto.o
>    CC      qapi/qapi-commands-introspect.o
> /tmp/qemu-test/src/block/sheepdog.c: In function 'find_vdi_name':
> /tmp/qemu-test/src/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals destination size [-Werror=stringop-truncation]
>       strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors

unrelated to my series.

> 
> 
> The full log is available at
> http://patchew.org/logs/20190108170655.29766-1-vsementsov@virtuozzo.com/testing.docker-mingw@fedora/?type=message.
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 0/9] qcow2: encryption threads

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1571 bytes --]

On 1/23/19 9:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> 23.01.2019 17:54, no-reply@patchew.org wrote:
>> Patchew URL: https://patchew.org/QEMU/20190108170655.29766-1-vsementsov@virtuozzo.com/
>>
>>
>>
>> Hi,
>>
>> This series failed the docker-mingw@fedora build test. Please find the testing commands and
>> their output below. If you have Docker installed, you can probably reproduce it
>> locally.
>>
>> === TEST SCRIPT BEGIN ===
>> #!/bin/bash
>> time make docker-test-mingw@fedora SHOW_ENV=1 J=14
>> === TEST SCRIPT END ===
>>
>>    CC      qapi/qapi-commands-crypto.o
>>    CC      qapi/qapi-commands-introspect.o
>> /tmp/qemu-test/src/block/sheepdog.c: In function 'find_vdi_name':
>> /tmp/qemu-test/src/block/sheepdog.c:1239:5: error: 'strncpy' specified bound 256 equals destination size [-Werror=stringop-truncation]
>>       strncpy(buf + SD_MAX_VDI_LEN, tag, SD_MAX_VDI_TAG_LEN);
>>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> cc1: all warnings being treated as errors
> 
> unrelated to my series.
> 

Known false alarm; the patchew processing queue is backlogged, and is
testing patches against a version of qemu.git that does not yet have
patches for newer gcc compliance incorporated, while at the same time
having recently upgraded its testing environment to use the newer gcc
that warns.  Commit 97b583f4 will silence the noise, once patchew
catches up to using it.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] ping Re: [PATCH v3 0/9] qcow2: encryption threads

[Vladimir Sementsov-Ogievskiy]

ping

Hi!

Mostly reviewed-by Alberto, "/* No sense in releasing the lock */" should be removed
from 06, should I resend just for this? I'd prefer to handle some other comments, if any.

08.01.2019 20:06, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> The series brings threads to qcow2 encryption/decryption path,
> like it is already done for compression.
> Now, based on master
> 
> v3:
> 01: drop 'include block_int.h' lines not needed more [Alberto]
> 02: fix comments style (open/close on separate line), add Alberto's r-b
> 03: improve commit message, add Alberto's r-b
> 04: new, follows Paolo's comment
> 
> 
> v2: (not very careful change list)
>      - multiple cipher inside QCryptoBlock instead of multiple
>        blocks inside qcow2, as suggested by Daniel, and it is
>        done in separate series
>      - use threaded encryption in do_perform_cow_encrypt() too
>      - some renaming and refactoring and simplifications
> 
> 
> == Performance testing ==
> 
> Hmm, do we have something like tests/qemu-iotests, but for performance?
> 
> [root@kvm up-qcow2-encrypt-threads]# cat test.sh
>      #!/bin/bash
> 
>      size=1G
>      src=/ssd/src.raw
>      dst=/ssd/dst.enc.qcow2
> 
>      # create source for tests
>      ./qemu-img create -f raw "$src" $size > /dev/null
>      ./qemu-io -f raw -c "write -P 0xa 0 $size" "$src" > /dev/null
> 
>      for w in "" "-W"; do
>          echo -e "Test with additional paramter for qemu-img: '$w'"
> 
>          # create target
>          ./qemu-img create -f qcow2 --object secret,id=sec0,data=test -o encrypt.format=luks,encrypt.key-secret=sec0 "$dst" $size > /dev/null
> 
>          time ./qemu-img convert $w -f raw --object secret,id=sec0,data=test --target-image-opts -n "$src" "driver=qcow2,file.filename=$dst,encrypt.key-secret=sec0"
>          echo
>      done
> 
> 
> before the series:
>      Test with additional paramter for qemu-img: ''
> 
>      real    0m14.318s
>      user    0m13.637s
>      sys     0m0.881s
> 
>      Test with additional paramter for qemu-img: '-W'
> 
>      real    0m13.962s
>      user    0m13.681s
>      sys     0m1.156s
> 
> 
> after the series:
>      Test with additional paramter for qemu-img: ''
> 
>      real    0m14.382s
>      user    0m13.735s
>      sys     0m0.835s
> 
>      Test with additional paramter for qemu-img: '-W'
> 
>      real    0m5.696s
>      user    0m15.931s
>      sys     0m1.144s
> 
> Vladimir Sementsov-Ogievskiy (9):
>    qcow2.h: add missing include
>    qcow2: add separate file for threaded data processing functions
>    qcow2-threads: use thread_pool_submit_co
>    qcow2-threads: qcow2_co_do_compress: protect queuing by mutex
>    qcow2-threads: split out generic path
>    qcow2: qcow2_co_preadv: improve locking
>    qcow2: qcow2_co_preadv: skip using hd_qiov when possible
>    qcow2: bdrv_co_pwritev: move encryption code out of the lock
>    qcow2: do encryption in threads
> 
>   block/qcow2.h          |  20 ++-
>   block/qcow2-bitmap.c   |   1 -
>   block/qcow2-cache.c    |   1 -
>   block/qcow2-cluster.c  |   8 +-
>   block/qcow2-refcount.c |   1 -
>   block/qcow2-snapshot.c |   1 -
>   block/qcow2-threads.c  | 268 +++++++++++++++++++++++++++++++++++++++
>   block/qcow2.c          | 277 ++++++++---------------------------------
>   block/Makefile.objs    |   2 +-
>   9 files changed, 343 insertions(+), 236 deletions(-)
>   create mode 100644 block/qcow2-threads.c
> 


-- 
Best regards,
Vladimir