[Qemu-devel] [PATCH v5 00/10] qcow2: encryption threads

[Qemu-devel] [PATCH v5 00/10] qcow2: encryption threads

[Vladimir Sementsov-Ogievskiy]

v5: rebase on master, some conflicts resolved due to data-file feature

01: new patch, just move test from cover letter to a file. I really hope that it
    will not hang the whole series, so, if we don't want it as is or with really
    tiny improvements, I'd prefer to skip it and queue 02-10 first.
09: "true" parameter added to moved qcow2_pre_write_overlap_check() call due to
    rebase on master (both before and after patch). Seems OK, so keep Alberto's r-b.

performance:

after 01:
 # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
14.18
 # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
13.77

after 10:
 # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
14.35
 # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
5.62

Vladimir Sementsov-Ogievskiy (10):
  tests/perf: Test qemu-img convert from raw to encrypted qcow2
  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                               | 275 ++++----------------
 block/Makefile.objs                         |   2 +-
 tests/perf/block/qcow2/convert-to-encrypted |  48 ++++
 10 files changed, 389 insertions(+), 236 deletions(-)
 create mode 100644 block/qcow2-threads.c
 create mode 100755 tests/perf/block/qcow2/convert-to-encrypted

-- 
2.18.0

[Qemu-devel] [PATCH v5 01/10] tests/perf: Test qemu-img convert from raw to encrypted qcow2

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/perf/block/qcow2/convert-to-encrypted | 48 +++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100755 tests/perf/block/qcow2/convert-to-encrypted

diff --git a/tests/perf/block/qcow2/convert-to-encrypted b/tests/perf/block/qcow2/convert-to-encrypted
new file mode 100755
index 0000000000..7a6b7b1cab
--- /dev/null
+++ b/tests/perf/block/qcow2/convert-to-encrypted
@@ -0,0 +1,48 @@
+#!/bin/bash
+#
+# Test qemu-img convert from raw to encrypted qcow2
+#
+# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+if [ "$#" -lt 2 ]; then
+    echo "Usage: $0 SOURCE_FILE DESTINATION_FILE [additional qemu-img convert parameters]"
+    exit 1
+fi
+
+ROOT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )/../../../.." >/dev/null 2>&1 && pwd )"
+QEMU_IMG="$ROOT_DIR/qemu-img"
+QEMU_IO="$ROOT_DIR/qemu-io"
+
+size=1G
+
+src="$1"
+shift
+
+dst="$1"
+shift
+
+(
+# create source
+$QEMU_IMG create -f raw "$src" $size
+$QEMU_IO -f raw -c "write -P 0xa 0 $size" "$src"
+
+# 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
+
+# test with additional parameters left in $@
+/usr/bin/time -f %e $QEMU_IMG convert "$@" -f raw --object secret,id=sec0,data=test --target-image-opts -n "$src" "driver=qcow2,file.filename=$dst,encrypt.key-secret=sec0"
-- 
2.18.0

[Qemu-devel] [PATCH v5 02/10] 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>
Reviewed-by: Alberto Garcia <berto@igalia.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 fdee297f33..ecb026e19a 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 e53a1609d7..337c0ac680 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 df02e7b20a..b33bcbc984 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 974a4e8656..c4965479f6 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 e0fe322500..ff27b6afd3 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 a6ffae89a6..d0e7fa9311 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 d507ee0686..7c68a5b5f9 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

[Qemu-devel] [PATCH v5 03/10] 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 ecb026e19a..61079f7ee8 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -735,4 +735,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 7c68a5b5f9..0d1d39c3e6 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:
@@ -3914,171 +3910,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 v5 04/10] 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 v5 05/10] qcow2-threads: qcow2_co_do_compress: protect queuing by mutex

[Vladimir Sementsov-Ogievskiy]

Drop dependence on AioContext lock.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.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

[Qemu-devel] [PATCH v5 06/10] 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>
Reviewed-by: Alberto Garcia <berto@igalia.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 61079f7ee8..3089e9a68b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -347,8 +347,8 @@ typedef struct BDRVQcow2State {
     char *image_backing_format;
     char *image_data_file;
 
-    CoQueue compress_wait_queue;
-    int nb_compress_threads;
+    CoQueue thread_task_queue;
+    int nb_threads;
 
     BdrvChild *data_file;
 } BDRVQcow2State;
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 0d1d39c3e6..46e8e39da5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1694,7 +1694,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

[Qemu-devel] [PATCH v5 07/10] 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>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.c | 46 ++++++++++++++++++++++------------------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 46e8e39da5..fcf92a7eb6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1983,6 +1983,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;
         }
 
@@ -1991,39 +1992,36 @@ 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))
+        {
+            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:
@@ -2056,11 +2054,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(s->data_file,
                                  cluster_offset + offset_in_cluster,
                                  cur_bytes, &hd_qiov, 0);
-            qemu_co_mutex_lock(&s->lock);
             if (ret < 0) {
                 goto fail;
             }
@@ -2091,12 +2087,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

[Qemu-devel] [PATCH v5 08/10] 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>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index fcf92a7eb6..b44b455ad1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1987,16 +1987,11 @@ 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))
         {
-            qemu_iovec_memset(&hd_qiov, 0, 0, cur_bytes);
+            qemu_iovec_memset(qiov, bytes_done, 0, cur_bytes);
 
             bytes -= cur_bytes;
             offset += cur_bytes;
@@ -2004,6 +1999,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

[Qemu-devel] [PATCH v5 09/10] 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>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index b44b455ad1..9bd5e8e48c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2175,11 +2175,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, true);
+        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);
 
@@ -2191,7 +2200,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;
                 }
             }
 
@@ -2206,40 +2215,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, true);
-        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(s->data_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;
@@ -2248,8 +2251,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

[Qemu-devel] [PATCH v5 10/10] 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>
Reviewed-by: Alberto Garcia <berto@igalia.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 3089e9a68b..5169c0d8ab 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -264,6 +264,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;
@@ -741,5 +743,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 c4965479f6..e6624c6e4e 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -471,13 +471,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 9bd5e8e48c..ba9725bcd5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -297,7 +297,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;
             }
@@ -1539,7 +1539,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;
@@ -2064,13 +2065,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;
                 }
@@ -2208,12 +2204,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 v5 00/10] qcow2: encryption threads

[Vladimir Sementsov-Ogievskiy]

ping

02.04.2019 18:37, Vladimir Sementsov-Ogievskiy wrote:
> v5: rebase on master, some conflicts resolved due to data-file feature
> 
> 01: new patch, just move test from cover letter to a file. I really hope that it
>      will not hang the whole series, so, if we don't want it as is or with really
>      tiny improvements, I'd prefer to skip it and queue 02-10 first.
> 09: "true" parameter added to moved qcow2_pre_write_overlap_check() call due to
>      rebase on master (both before and after patch). Seems OK, so keep Alberto's r-b.
> 
> performance:
> 
> after 01:
>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
> 14.18
>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
> 13.77
> 
> after 10:
>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
> 14.35
>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
> 5.62
> 
> Vladimir Sementsov-Ogievskiy (10):
>    tests/perf: Test qemu-img convert from raw to encrypted qcow2
>    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                               | 275 ++++----------------
>   block/Makefile.objs                         |   2 +-
>   tests/perf/block/qcow2/convert-to-encrypted |  48 ++++
>   10 files changed, 389 insertions(+), 236 deletions(-)
>   create mode 100644 block/qcow2-threads.c
>   create mode 100755 tests/perf/block/qcow2/convert-to-encrypted
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 00/10] qcow2: encryption threads

[Vladimir Sementsov-Ogievskiy]

ping

02.04.2019 18:37, Vladimir Sementsov-Ogievskiy wrote:
> v5: rebase on master, some conflicts resolved due to data-file feature
> 
> 01: new patch, just move test from cover letter to a file. I really hope that it
>      will not hang the whole series, so, if we don't want it as is or with really
>      tiny improvements, I'd prefer to skip it and queue 02-10 first.
> 09: "true" parameter added to moved qcow2_pre_write_overlap_check() call due to
>      rebase on master (both before and after patch). Seems OK, so keep Alberto's r-b.
> 
> performance:
> 
> after 01:
>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
> 14.18
>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
> 13.77
> 
> after 10:
>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
> 14.35
>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
> 5.62
> 
> Vladimir Sementsov-Ogievskiy (10):
>    tests/perf: Test qemu-img convert from raw to encrypted qcow2
>    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                               | 275 ++++----------------
>   block/Makefile.objs                         |   2 +-
>   tests/perf/block/qcow2/convert-to-encrypted |  48 ++++
>   10 files changed, 389 insertions(+), 236 deletions(-)
>   create mode 100644 block/qcow2-threads.c
>   create mode 100755 tests/perf/block/qcow2/convert-to-encrypted
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 01/10] tests/perf: Test qemu-img convert from raw to encrypted qcow2

[Max Reitz]

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

On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/perf/block/qcow2/convert-to-encrypted | 48 +++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100755 tests/perf/block/qcow2/convert-to-encrypted

Thanks for the test case, but I don’t know whether this is the right way
to include it.

A concrete problem is that it doesn’t work with out-of-tree builds (I
only do out-of-tree builds).  I wonder whether it would be possible and
make sense (I have no idea) to add a subdirectory "perf" to the iotests
and reuse its infrastructure?  Those tests wouldn’t run by default.

Max

> diff --git a/tests/perf/block/qcow2/convert-to-encrypted b/tests/perf/block/qcow2/convert-to-encrypted
> new file mode 100755
> index 0000000000..7a6b7b1cab
> --- /dev/null
> +++ b/tests/perf/block/qcow2/convert-to-encrypted
> @@ -0,0 +1,48 @@
> +#!/bin/bash
> +#
> +# Test qemu-img convert from raw to encrypted qcow2
> +#
> +# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +if [ "$#" -lt 2 ]; then
> +    echo "Usage: $0 SOURCE_FILE DESTINATION_FILE [additional qemu-img convert parameters]"
> +    exit 1
> +fi
> +
> +ROOT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )/../../../.." >/dev/null 2>&1 && pwd )"
> +QEMU_IMG="$ROOT_DIR/qemu-img"
> +QEMU_IO="$ROOT_DIR/qemu-io"
> +
> +size=1G
> +
> +src="$1"
> +shift
> +
> +dst="$1"
> +shift
> +
> +(
> +# create source
> +$QEMU_IMG create -f raw "$src" $size
> +$QEMU_IO -f raw -c "write -P 0xa 0 $size" "$src"
> +
> +# 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
> +
> +# test with additional parameters left in $@
> +/usr/bin/time -f %e $QEMU_IMG convert "$@" -f raw --object secret,id=sec0,data=test --target-image-opts -n "$src" "driver=qcow2,file.filename=$dst,encrypt.key-secret=sec0"
> 



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

Re: [Qemu-devel] [PATCH v5 01/10] tests/perf: Test qemu-img convert from raw to encrypted qcow2

[Max Reitz]

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

On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/perf/block/qcow2/convert-to-encrypted | 48 +++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100755 tests/perf/block/qcow2/convert-to-encrypted

Thanks for the test case, but I don’t know whether this is the right way
to include it.

A concrete problem is that it doesn’t work with out-of-tree builds (I
only do out-of-tree builds).  I wonder whether it would be possible and
make sense (I have no idea) to add a subdirectory "perf" to the iotests
and reuse its infrastructure?  Those tests wouldn’t run by default.

Max

> diff --git a/tests/perf/block/qcow2/convert-to-encrypted b/tests/perf/block/qcow2/convert-to-encrypted
> new file mode 100755
> index 0000000000..7a6b7b1cab
> --- /dev/null
> +++ b/tests/perf/block/qcow2/convert-to-encrypted
> @@ -0,0 +1,48 @@
> +#!/bin/bash
> +#
> +# Test qemu-img convert from raw to encrypted qcow2
> +#
> +# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +if [ "$#" -lt 2 ]; then
> +    echo "Usage: $0 SOURCE_FILE DESTINATION_FILE [additional qemu-img convert parameters]"
> +    exit 1
> +fi
> +
> +ROOT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )/../../../.." >/dev/null 2>&1 && pwd )"
> +QEMU_IMG="$ROOT_DIR/qemu-img"
> +QEMU_IO="$ROOT_DIR/qemu-io"
> +
> +size=1G
> +
> +src="$1"
> +shift
> +
> +dst="$1"
> +shift
> +
> +(
> +# create source
> +$QEMU_IMG create -f raw "$src" $size
> +$QEMU_IO -f raw -c "write -P 0xa 0 $size" "$src"
> +
> +# 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
> +
> +# test with additional parameters left in $@
> +/usr/bin/time -f %e $QEMU_IMG convert "$@" -f raw --object secret,id=sec0,data=test --target-image-opts -n "$src" "driver=qcow2,file.filename=$dst,encrypt.key-secret=sec0"
> 



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

Re: [Qemu-devel] [PATCH v5 00/10] qcow2: encryption threads

[Max Reitz]

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

On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
> v5: rebase on master, some conflicts resolved due to data-file feature
> 
> 01: new patch, just move test from cover letter to a file. I really hope that it
>     will not hang the whole series, so, if we don't want it as is or with really
>     tiny improvements, I'd prefer to skip it and queue 02-10 first.

Yup, noted.

> 09: "true" parameter added to moved qcow2_pre_write_overlap_check() call due to
>     rebase on master (both before and after patch). Seems OK, so keep Alberto's r-b.
> 
> performance:
> 
> after 01:
>  # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
> 14.18
>  # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
> 13.77
> 
> after 10:
>  # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
> 14.35
>  # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
> 5.62

Hm, I see those results as well:

Before:
w/o -W: 7.15
w/  -W: 6.77

After:
w/o -W: 7.19
w/  -W: 3.65


But with -t none, this is what I get:

Before:
w/o -W: 15.98
w/  -W: 10.91

After:
w/o -W: 19.95
w/  -W: 11.77


For good measure, on tmpfs:

Before:
w/o -W: 6.98
w/  -W: 6.75

After:
w/o -W: 7.04
w/  -W: 3.63


So it looks like the results with cache enabled are really only in the
cache.  When it goes down to disk, this series seems to decrease
performance.

To confirm whether that’s actually the case, I took another machine with
an SSD where I have more empty space and increased the size to 8 GB (not
the $size, because qemu-io doesn't like that, but well, yeah), and then
ran it again without cache:

Before:
w/o -W: ~50 - ~60 (varies...)
w/  -W: ~50 - ~70

After:
w/o -W: ~60
w/  -W: ~55 - ~85


So it does seem slower, although the values vary so much that it’s
difficult to tell.

Hmm...  And on that machine, there is no difference between before and
after when using -t none.  So I suppose it also depends on the device?



OK, I tried using qemu-img bench.  After patching it to accept --object,
these are the results I got with -t none -w on a preallocated (full) 8
GB image:

Before:
HDD: 17.7 s, 17.8 s, 18.0 s
SSD 1: 12.9 s, 15.8 s, 15.1 s
SSD 2: 1.8 s, 1.7 s, 1.7 s

After:
HDD: 16.1 s, 15.8 s, 15.8 s
SSD 1: 16.3 s, 18.0 s, 17.9 s
SSD 2: 2.0 s, 1.5 s, 1.5 s

Result #1: My SSD 1 is trash.

Result #2: I need more requests for SSD 2 to get a useful results.
Let's try again with 2^20:
Before: 23.8, 23.5, 23.3
After:  21.0, 20.6, 20.5

OK, and I can clearly see that this series increases the CPU usage
(which is good).


So...  Hm.  I suppose I conclude that this series decreases performance
on trashy SSDs?  But it gets better on my HDD and on my good SSD, so...
 Good thing I tested it, or something.

The only really interesting thing that came out of this is that I didn't
see an improvement with qemu-img convert (only on tmpfs), but that I do
see it with qemu-img bench.  So I'm wondering why you aren't using
qemu-img bench in the test in your first patch...?

Max

> Vladimir Sementsov-Ogievskiy (10):
>   tests/perf: Test qemu-img convert from raw to encrypted qcow2
>   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                               | 275 ++++----------------
>  block/Makefile.objs                         |   2 +-
>  tests/perf/block/qcow2/convert-to-encrypted |  48 ++++
>  10 files changed, 389 insertions(+), 236 deletions(-)
>  create mode 100644 block/qcow2-threads.c
>  create mode 100755 tests/perf/block/qcow2/convert-to-encrypted
> 



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

Re: [Qemu-devel] [PATCH v5 00/10] qcow2: encryption threads

[Max Reitz]

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

On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
> v5: rebase on master, some conflicts resolved due to data-file feature
> 
> 01: new patch, just move test from cover letter to a file. I really hope that it
>     will not hang the whole series, so, if we don't want it as is or with really
>     tiny improvements, I'd prefer to skip it and queue 02-10 first.

Yup, noted.

> 09: "true" parameter added to moved qcow2_pre_write_overlap_check() call due to
>     rebase on master (both before and after patch). Seems OK, so keep Alberto's r-b.
> 
> performance:
> 
> after 01:
>  # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
> 14.18
>  # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
> 13.77
> 
> after 10:
>  # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
> 14.35
>  # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
> 5.62

Hm, I see those results as well:

Before:
w/o -W: 7.15
w/  -W: 6.77

After:
w/o -W: 7.19
w/  -W: 3.65


But with -t none, this is what I get:

Before:
w/o -W: 15.98
w/  -W: 10.91

After:
w/o -W: 19.95
w/  -W: 11.77


For good measure, on tmpfs:

Before:
w/o -W: 6.98
w/  -W: 6.75

After:
w/o -W: 7.04
w/  -W: 3.63


So it looks like the results with cache enabled are really only in the
cache.  When it goes down to disk, this series seems to decrease
performance.

To confirm whether that’s actually the case, I took another machine with
an SSD where I have more empty space and increased the size to 8 GB (not
the $size, because qemu-io doesn't like that, but well, yeah), and then
ran it again without cache:

Before:
w/o -W: ~50 - ~60 (varies...)
w/  -W: ~50 - ~70

After:
w/o -W: ~60
w/  -W: ~55 - ~85


So it does seem slower, although the values vary so much that it’s
difficult to tell.

Hmm...  And on that machine, there is no difference between before and
after when using -t none.  So I suppose it also depends on the device?



OK, I tried using qemu-img bench.  After patching it to accept --object,
these are the results I got with -t none -w on a preallocated (full) 8
GB image:

Before:
HDD: 17.7 s, 17.8 s, 18.0 s
SSD 1: 12.9 s, 15.8 s, 15.1 s
SSD 2: 1.8 s, 1.7 s, 1.7 s

After:
HDD: 16.1 s, 15.8 s, 15.8 s
SSD 1: 16.3 s, 18.0 s, 17.9 s
SSD 2: 2.0 s, 1.5 s, 1.5 s

Result #1: My SSD 1 is trash.

Result #2: I need more requests for SSD 2 to get a useful results.
Let's try again with 2^20:
Before: 23.8, 23.5, 23.3
After:  21.0, 20.6, 20.5

OK, and I can clearly see that this series increases the CPU usage
(which is good).


So...  Hm.  I suppose I conclude that this series decreases performance
on trashy SSDs?  But it gets better on my HDD and on my good SSD, so...
 Good thing I tested it, or something.

The only really interesting thing that came out of this is that I didn't
see an improvement with qemu-img convert (only on tmpfs), but that I do
see it with qemu-img bench.  So I'm wondering why you aren't using
qemu-img bench in the test in your first patch...?

Max

> Vladimir Sementsov-Ogievskiy (10):
>   tests/perf: Test qemu-img convert from raw to encrypted qcow2
>   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                               | 275 ++++----------------
>  block/Makefile.objs                         |   2 +-
>  tests/perf/block/qcow2/convert-to-encrypted |  48 ++++
>  10 files changed, 389 insertions(+), 236 deletions(-)
>  create mode 100644 block/qcow2-threads.c
>  create mode 100755 tests/perf/block/qcow2/convert-to-encrypted
> 



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

Re: [Qemu-devel] [PATCH v5 00/10] qcow2: encryption threads

[Max Reitz]

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

On 29.04.19 02:37, Max Reitz wrote:
> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>> v5: rebase on master, some conflicts resolved due to data-file feature

(Forgot to note that I'll look at the more interesting patches in this
series later today.  I just got so hung up on the benchmark...)


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

Re: [Qemu-devel] [PATCH v5 00/10] qcow2: encryption threads

[Max Reitz]

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

On 29.04.19 02:37, Max Reitz wrote:
> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>> v5: rebase on master, some conflicts resolved due to data-file feature

(Forgot to note that I'll look at the more interesting patches in this
series later today.  I just got so hung up on the benchmark...)


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

Re: [Qemu-devel] [PATCH v5 00/10] qcow2: encryption threads

[Vladimir Sementsov-Ogievskiy]

29.04.2019 3:37, Max Reitz wrote:
> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>> v5: rebase on master, some conflicts resolved due to data-file feature
>>
>> 01: new patch, just move test from cover letter to a file. I really hope that it
>>      will not hang the whole series, so, if we don't want it as is or with really
>>      tiny improvements, I'd prefer to skip it and queue 02-10 first.
> 
> Yup, noted.
> 
>> 09: "true" parameter added to moved qcow2_pre_write_overlap_check() call due to
>>      rebase on master (both before and after patch). Seems OK, so keep Alberto's r-b.
>>
>> performance:
>>
>> after 01:
>>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
>> 14.18
>>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
>> 13.77
>>
>> after 10:
>>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
>> 14.35
>>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
>> 5.62
> 
> Hm, I see those results as well:
> 
> Before:
> w/o -W: 7.15
> w/  -W: 6.77
> 
> After:
> w/o -W: 7.19
> w/  -W: 3.65
> 
> 
> But with -t none, this is what I get:
> 
> Before:
> w/o -W: 15.98
> w/  -W: 10.91
> 
> After:
> w/o -W: 19.95
> w/  -W: 11.77
> 
> 
> For good measure, on tmpfs:
> 
> Before:
> w/o -W: 6.98
> w/  -W: 6.75
> 
> After:
> w/o -W: 7.04
> w/  -W: 3.63
> 
> 
> So it looks like the results with cache enabled are really only in the
> cache.  When it goes down to disk, this series seems to decrease
> performance.
> 
> To confirm whether that’s actually the case, I took another machine with
> an SSD where I have more empty space and increased the size to 8 GB (not
> the $size, because qemu-io doesn't like that, but well, yeah), and then
> ran it again without cache:
> 
> Before:
> w/o -W: ~50 - ~60 (varies...)
> w/  -W: ~50 - ~70
> 
> After:
> w/o -W: ~60
> w/  -W: ~55 - ~85
> 
> 
> So it does seem slower, although the values vary so much that it’s
> difficult to tell.
> 
> Hmm...  And on that machine, there is no difference between before and
> after when using -t none.  So I suppose it also depends on the device?
> 
> 
> 
> OK, I tried using qemu-img bench.  After patching it to accept --object,
> these are the results I got with -t none -w on a preallocated (full) 8
> GB image:
> 
> Before:
> HDD: 17.7 s, 17.8 s, 18.0 s
> SSD 1: 12.9 s, 15.8 s, 15.1 s
> SSD 2: 1.8 s, 1.7 s, 1.7 s
> 
> After:
> HDD: 16.1 s, 15.8 s, 15.8 s
> SSD 1: 16.3 s, 18.0 s, 17.9 s
> SSD 2: 2.0 s, 1.5 s, 1.5 s
> 
> Result #1: My SSD 1 is trash.
> 
> Result #2: I need more requests for SSD 2 to get a useful results.
> Let's try again with 2^20:
> Before: 23.8, 23.5, 23.3
> After:  21.0, 20.6, 20.5
> 
> OK, and I can clearly see that this series increases the CPU usage
> (which is good).
> 
> 
> So...  Hm.  I suppose I conclude that this series decreases performance
> on trashy SSDs?  But it gets better on my HDD and on my good SSD, so...
>   Good thing I tested it, or something.

Interesting, thanks for testing! No idea about degradation on bad SSD..
You can try to check threads overhead by set QCOW2_MAX_THREADS to 1..


> 
> The only really interesting thing that came out of this is that I didn't
> see an improvement with qemu-img convert (only on tmpfs), but that I do
> see it with qemu-img bench.  So I'm wondering why you aren't using
> qemu-img bench in the test in your first patch...?
> 

the idea was that performance tests should do one run, and there should be
common script to run test several times and calculate overage.


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 00/10] qcow2: encryption threads

[Vladimir Sementsov-Ogievskiy]

29.04.2019 3:37, Max Reitz wrote:
> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>> v5: rebase on master, some conflicts resolved due to data-file feature
>>
>> 01: new patch, just move test from cover letter to a file. I really hope that it
>>      will not hang the whole series, so, if we don't want it as is or with really
>>      tiny improvements, I'd prefer to skip it and queue 02-10 first.
> 
> Yup, noted.
> 
>> 09: "true" parameter added to moved qcow2_pre_write_overlap_check() call due to
>>      rebase on master (both before and after patch). Seems OK, so keep Alberto's r-b.
>>
>> performance:
>>
>> after 01:
>>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
>> 14.18
>>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
>> 13.77
>>
>> after 10:
>>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2
>> 14.35
>>   # ./tests/perf/block/qcow2/convert-to-encrypted /ssd/src.raw /ssd/dst.enc.qcow2 -W
>> 5.62
> 
> Hm, I see those results as well:
> 
> Before:
> w/o -W: 7.15
> w/  -W: 6.77
> 
> After:
> w/o -W: 7.19
> w/  -W: 3.65
> 
> 
> But with -t none, this is what I get:
> 
> Before:
> w/o -W: 15.98
> w/  -W: 10.91
> 
> After:
> w/o -W: 19.95
> w/  -W: 11.77
> 
> 
> For good measure, on tmpfs:
> 
> Before:
> w/o -W: 6.98
> w/  -W: 6.75
> 
> After:
> w/o -W: 7.04
> w/  -W: 3.63
> 
> 
> So it looks like the results with cache enabled are really only in the
> cache.  When it goes down to disk, this series seems to decrease
> performance.
> 
> To confirm whether that’s actually the case, I took another machine with
> an SSD where I have more empty space and increased the size to 8 GB (not
> the $size, because qemu-io doesn't like that, but well, yeah), and then
> ran it again without cache:
> 
> Before:
> w/o -W: ~50 - ~60 (varies...)
> w/  -W: ~50 - ~70
> 
> After:
> w/o -W: ~60
> w/  -W: ~55 - ~85
> 
> 
> So it does seem slower, although the values vary so much that it’s
> difficult to tell.
> 
> Hmm...  And on that machine, there is no difference between before and
> after when using -t none.  So I suppose it also depends on the device?
> 
> 
> 
> OK, I tried using qemu-img bench.  After patching it to accept --object,
> these are the results I got with -t none -w on a preallocated (full) 8
> GB image:
> 
> Before:
> HDD: 17.7 s, 17.8 s, 18.0 s
> SSD 1: 12.9 s, 15.8 s, 15.1 s
> SSD 2: 1.8 s, 1.7 s, 1.7 s
> 
> After:
> HDD: 16.1 s, 15.8 s, 15.8 s
> SSD 1: 16.3 s, 18.0 s, 17.9 s
> SSD 2: 2.0 s, 1.5 s, 1.5 s
> 
> Result #1: My SSD 1 is trash.
> 
> Result #2: I need more requests for SSD 2 to get a useful results.
> Let's try again with 2^20:
> Before: 23.8, 23.5, 23.3
> After:  21.0, 20.6, 20.5
> 
> OK, and I can clearly see that this series increases the CPU usage
> (which is good).
> 
> 
> So...  Hm.  I suppose I conclude that this series decreases performance
> on trashy SSDs?  But it gets better on my HDD and on my good SSD, so...
>   Good thing I tested it, or something.

Interesting, thanks for testing! No idea about degradation on bad SSD..
You can try to check threads overhead by set QCOW2_MAX_THREADS to 1..


> 
> The only really interesting thing that came out of this is that I didn't
> see an improvement with qemu-img convert (only on tmpfs), but that I do
> see it with qemu-img bench.  So I'm wondering why you aren't using
> qemu-img bench in the test in your first patch...?
> 

the idea was that performance tests should do one run, and there should be
common script to run test several times and calculate overage.


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 07/10] qcow2: qcow2_co_preadv: improve locking

[Max Reitz]

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

On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
> Background: decryption will be done in threads, to take benefit of it,
> we should move it out of the lock first.

...which is safe after your commit c972fa123c73501b4, I presume.

(At first glance, the patched looked a bit weird to me because it
doesn't give a reason why dropping the lock around
qcrypto_block_decrypt() would be OK.)

> 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>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2.c | 46 ++++++++++++++++++++++------------------------
>  1 file changed, 22 insertions(+), 24 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 46e8e39da5..fcf92a7eb6 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1983,6 +1983,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>  
>          ret = qcow2_get_cluster_offset(bs, offset, &cur_bytes, &cluster_offset);

Isn't this the only function in the loop that actually needs the lock?
Wouldn't it make more sense to just take it around this call?

Max

>          if (ret < 0) {
> +            qemu_co_mutex_unlock(&s->lock);
>              goto fail;
>          }
>  


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

Re: [Qemu-devel] [PATCH v5 07/10] qcow2: qcow2_co_preadv: improve locking

[Max Reitz]

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

On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
> Background: decryption will be done in threads, to take benefit of it,
> we should move it out of the lock first.

...which is safe after your commit c972fa123c73501b4, I presume.

(At first glance, the patched looked a bit weird to me because it
doesn't give a reason why dropping the lock around
qcrypto_block_decrypt() would be OK.)

> 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>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2.c | 46 ++++++++++++++++++++++------------------------
>  1 file changed, 22 insertions(+), 24 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 46e8e39da5..fcf92a7eb6 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1983,6 +1983,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>  
>          ret = qcow2_get_cluster_offset(bs, offset, &cur_bytes, &cluster_offset);

Isn't this the only function in the loop that actually needs the lock?
Wouldn't it make more sense to just take it around this call?

Max

>          if (ret < 0) {
> +            qemu_co_mutex_unlock(&s->lock);
>              goto fail;
>          }
>  


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

Re: [Qemu-devel] [PATCH v5 07/10] qcow2: qcow2_co_preadv: improve locking

[Max Reitz]

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

On 29.04.19 18:37, Max Reitz wrote:
> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>> Background: decryption will be done in threads, to take benefit of it,
>> we should move it out of the lock first.
> 
> ...which is safe after your commit c972fa123c73501b4, I presume.
> 
> (At first glance, the patched looked a bit weird to me because it
> doesn't give a reason why dropping the lock around
> qcrypto_block_decrypt() would be OK.)

On second thought, I guess the actual reason it's safe is because the
crypto code never yields.

Max

>> 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>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  block/qcow2.c | 46 ++++++++++++++++++++++------------------------
>>  1 file changed, 22 insertions(+), 24 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 46e8e39da5..fcf92a7eb6 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1983,6 +1983,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>>  
>>          ret = qcow2_get_cluster_offset(bs, offset, &cur_bytes, &cluster_offset);
> 
> Isn't this the only function in the loop that actually needs the lock?
> Wouldn't it make more sense to just take it around this call?
> 
> Max
> 
>>          if (ret < 0) {
>> +            qemu_co_mutex_unlock(&s->lock);
>>              goto fail;
>>          }
>>  
> 



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

Re: [Qemu-devel] [PATCH v5 07/10] qcow2: qcow2_co_preadv: improve locking

[Max Reitz]

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

On 29.04.19 18:37, Max Reitz wrote:
> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>> Background: decryption will be done in threads, to take benefit of it,
>> we should move it out of the lock first.
> 
> ...which is safe after your commit c972fa123c73501b4, I presume.
> 
> (At first glance, the patched looked a bit weird to me because it
> doesn't give a reason why dropping the lock around
> qcrypto_block_decrypt() would be OK.)

On second thought, I guess the actual reason it's safe is because the
crypto code never yields.

Max

>> 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>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  block/qcow2.c | 46 ++++++++++++++++++++++------------------------
>>  1 file changed, 22 insertions(+), 24 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 46e8e39da5..fcf92a7eb6 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1983,6 +1983,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>>  
>>          ret = qcow2_get_cluster_offset(bs, offset, &cur_bytes, &cluster_offset);
> 
> Isn't this the only function in the loop that actually needs the lock?
> Wouldn't it make more sense to just take it around this call?
> 
> Max
> 
>>          if (ret < 0) {
>> +            qemu_co_mutex_unlock(&s->lock);
>>              goto fail;
>>          }
>>  
> 



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

Re: [Qemu-devel] [PATCH v5 00/10] qcow2: encryption threads

[Max Reitz]

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

On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
> v5: rebase on master, some conflicts resolved due to data-file feature
> 
> 01: new patch, just move test from cover letter to a file. I really hope that it
>     will not hang the whole series, so, if we don't want it as is or with really
>     tiny improvements, I'd prefer to skip it and queue 02-10 first.
> 09: "true" parameter added to moved qcow2_pre_write_overlap_check() call due to
>     rebase on master (both before and after patch). Seems OK, so keep Alberto's r-b.

Patches 2 – 6, 8 – 10:

Reviewed-by: Max Reitz <mreitz@redhat.com>

For 7 I wonder whether the locking can be even tighter.

Max


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

Re: [Qemu-devel] [PATCH v5 00/10] qcow2: encryption threads

[Max Reitz]

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

On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
> v5: rebase on master, some conflicts resolved due to data-file feature
> 
> 01: new patch, just move test from cover letter to a file. I really hope that it
>     will not hang the whole series, so, if we don't want it as is or with really
>     tiny improvements, I'd prefer to skip it and queue 02-10 first.
> 09: "true" parameter added to moved qcow2_pre_write_overlap_check() call due to
>     rebase on master (both before and after patch). Seems OK, so keep Alberto's r-b.

Patches 2 – 6, 8 – 10:

Reviewed-by: Max Reitz <mreitz@redhat.com>

For 7 I wonder whether the locking can be even tighter.

Max


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

Re: [Qemu-devel] [PATCH v5 07/10] qcow2: qcow2_co_preadv: improve locking

[Vladimir Sementsov-Ogievskiy]

29.04.2019 19:37, Max Reitz wrote:
> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>> Background: decryption will be done in threads, to take benefit of it,
>> we should move it out of the lock first.
> 
> ...which is safe after your commit c972fa123c73501b4, I presume.
> 
> (At first glance, the patched looked a bit weird to me because it
> doesn't give a reason why dropping the lock around
> qcrypto_block_decrypt() would be OK.)
> 
>> 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>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>>   block/qcow2.c | 46 ++++++++++++++++++++++------------------------
>>   1 file changed, 22 insertions(+), 24 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 46e8e39da5..fcf92a7eb6 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1983,6 +1983,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>>   
>>           ret = qcow2_get_cluster_offset(bs, offset, &cur_bytes, &cluster_offset);
> 
> Isn't this the only function in the loop that actually needs the lock?
> Wouldn't it make more sense to just take it around this call?
> 

Hmm, looks correct, I'll resend.



-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 07/10] qcow2: qcow2_co_preadv: improve locking

[Vladimir Sementsov-Ogievskiy]

29.04.2019 19:37, Max Reitz wrote:
> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>> Background: decryption will be done in threads, to take benefit of it,
>> we should move it out of the lock first.
> 
> ...which is safe after your commit c972fa123c73501b4, I presume.
> 
> (At first glance, the patched looked a bit weird to me because it
> doesn't give a reason why dropping the lock around
> qcrypto_block_decrypt() would be OK.)
> 
>> 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>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>>   block/qcow2.c | 46 ++++++++++++++++++++++------------------------
>>   1 file changed, 22 insertions(+), 24 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 46e8e39da5..fcf92a7eb6 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1983,6 +1983,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>>   
>>           ret = qcow2_get_cluster_offset(bs, offset, &cur_bytes, &cluster_offset);
> 
> Isn't this the only function in the loop that actually needs the lock?
> Wouldn't it make more sense to just take it around this call?
> 

Hmm, looks correct, I'll resend.



-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 01/10] tests/perf: Test qemu-img convert from raw to encrypted qcow2

[Vladimir Sementsov-Ogievskiy]

29.04.2019 1:55, Max Reitz wrote:
> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/perf/block/qcow2/convert-to-encrypted | 48 +++++++++++++++++++++
>>   1 file changed, 48 insertions(+)
>>   create mode 100755 tests/perf/block/qcow2/convert-to-encrypted
> 
> Thanks for the test case, but I don’t know whether this is the right way
> to include it.
> 
> A concrete problem is that it doesn’t work with out-of-tree builds (I
> only do out-of-tree builds).  I wonder whether it would be possible and
> make sense (I have no idea) to add a subdirectory "perf" to the iotests
> and reuse its infrastructure?  Those tests wouldn’t run by default.
> 

Honestly, I don't really like existing iotests infrastructure, bound to check
script, which I don't like too (and any other large script in bash, sorry :(..

What do you mean? You have env variables QEMU_IMG, etc, and want them to be
accepted by script?

I'd prefer to commit something simple and separate, to be able to build up
infrastructure around it gradually.. Finally, I want a simple way to run
a set of perf tests on a set of git commits and get an html and ascii
tables of performance comparison between these commits.

> 
>> diff --git a/tests/perf/block/qcow2/convert-to-encrypted b/tests/perf/block/qcow2/convert-to-encrypted
>> new file mode 100755
>> index 0000000000..7a6b7b1cab
>> --- /dev/null
>> +++ b/tests/perf/block/qcow2/convert-to-encrypted
>> @@ -0,0 +1,48 @@
>> +#!/bin/bash
>> +#
>> +# Test qemu-img convert from raw to encrypted qcow2
>> +#
>> +# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 2 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +#
>> +
>> +if [ "$#" -lt 2 ]; then
>> +    echo "Usage: $0 SOURCE_FILE DESTINATION_FILE [additional qemu-img convert parameters]"
>> +    exit 1
>> +fi
>> +
>> +ROOT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )/../../../.." >/dev/null 2>&1 && pwd )"
>> +QEMU_IMG="$ROOT_DIR/qemu-img"
>> +QEMU_IO="$ROOT_DIR/qemu-io"
>> +
>> +size=1G
>> +
>> +src="$1"
>> +shift
>> +
>> +dst="$1"
>> +shift
>> +
>> +(
>> +# create source
>> +$QEMU_IMG create -f raw "$src" $size
>> +$QEMU_IO -f raw -c "write -P 0xa 0 $size" "$src"
>> +
>> +# 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
>> +
>> +# test with additional parameters left in $@
>> +/usr/bin/time -f %e $QEMU_IMG convert "$@" -f raw --object secret,id=sec0,data=test --target-image-opts -n "$src" "driver=qcow2,file.filename=$dst,encrypt.key-secret=sec0"
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 01/10] tests/perf: Test qemu-img convert from raw to encrypted qcow2

[Vladimir Sementsov-Ogievskiy]

29.04.2019 1:55, Max Reitz wrote:
> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/perf/block/qcow2/convert-to-encrypted | 48 +++++++++++++++++++++
>>   1 file changed, 48 insertions(+)
>>   create mode 100755 tests/perf/block/qcow2/convert-to-encrypted
> 
> Thanks for the test case, but I don’t know whether this is the right way
> to include it.
> 
> A concrete problem is that it doesn’t work with out-of-tree builds (I
> only do out-of-tree builds).  I wonder whether it would be possible and
> make sense (I have no idea) to add a subdirectory "perf" to the iotests
> and reuse its infrastructure?  Those tests wouldn’t run by default.
> 

Honestly, I don't really like existing iotests infrastructure, bound to check
script, which I don't like too (and any other large script in bash, sorry :(..

What do you mean? You have env variables QEMU_IMG, etc, and want them to be
accepted by script?

I'd prefer to commit something simple and separate, to be able to build up
infrastructure around it gradually.. Finally, I want a simple way to run
a set of perf tests on a set of git commits and get an html and ascii
tables of performance comparison between these commits.

> 
>> diff --git a/tests/perf/block/qcow2/convert-to-encrypted b/tests/perf/block/qcow2/convert-to-encrypted
>> new file mode 100755
>> index 0000000000..7a6b7b1cab
>> --- /dev/null
>> +++ b/tests/perf/block/qcow2/convert-to-encrypted
>> @@ -0,0 +1,48 @@
>> +#!/bin/bash
>> +#
>> +# Test qemu-img convert from raw to encrypted qcow2
>> +#
>> +# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 2 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +#
>> +
>> +if [ "$#" -lt 2 ]; then
>> +    echo "Usage: $0 SOURCE_FILE DESTINATION_FILE [additional qemu-img convert parameters]"
>> +    exit 1
>> +fi
>> +
>> +ROOT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )/../../../.." >/dev/null 2>&1 && pwd )"
>> +QEMU_IMG="$ROOT_DIR/qemu-img"
>> +QEMU_IO="$ROOT_DIR/qemu-io"
>> +
>> +size=1G
>> +
>> +src="$1"
>> +shift
>> +
>> +dst="$1"
>> +shift
>> +
>> +(
>> +# create source
>> +$QEMU_IMG create -f raw "$src" $size
>> +$QEMU_IO -f raw -c "write -P 0xa 0 $size" "$src"
>> +
>> +# 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
>> +
>> +# test with additional parameters left in $@
>> +/usr/bin/time -f %e $QEMU_IMG convert "$@" -f raw --object secret,id=sec0,data=test --target-image-opts -n "$src" "driver=qcow2,file.filename=$dst,encrypt.key-secret=sec0"
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 07/10] qcow2: qcow2_co_preadv: improve locking

[Vladimir Sementsov-Ogievskiy]

30.04.2019 11:38, Vladimir Sementsov-Ogievskiy wrote:
> 29.04.2019 19:37, Max Reitz wrote:
>> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>>> Background: decryption will be done in threads, to take benefit of it,
>>> we should move it out of the lock first.
>>
>> ...which is safe after your commit c972fa123c73501b4, I presume.
>>
>> (At first glance, the patched looked a bit weird to me because it
>> doesn't give a reason why dropping the lock around
>> qcrypto_block_decrypt() would be OK.)
>>
>>> 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>
>>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>>> ---
>>>   block/qcow2.c | 46 ++++++++++++++++++++++------------------------
>>>   1 file changed, 22 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 46e8e39da5..fcf92a7eb6 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -1983,6 +1983,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>>>           ret = qcow2_get_cluster_offset(bs, offset, &cur_bytes, &cluster_offset);
>>
>> Isn't this the only function in the loop that actually needs the lock?
>> Wouldn't it make more sense to just take it around this call?
>>
> 
> Hmm, looks correct, I'll resend.
> 
> 

Or not, actually, we may have several qcow2_get_data_offset calls under one lock,
if clusters are different kinds of ZERO. So, I think better to keep it as is for now.



-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 07/10] qcow2: qcow2_co_preadv: improve locking

[Vladimir Sementsov-Ogievskiy]

30.04.2019 11:38, Vladimir Sementsov-Ogievskiy wrote:
> 29.04.2019 19:37, Max Reitz wrote:
>> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>>> Background: decryption will be done in threads, to take benefit of it,
>>> we should move it out of the lock first.
>>
>> ...which is safe after your commit c972fa123c73501b4, I presume.
>>
>> (At first glance, the patched looked a bit weird to me because it
>> doesn't give a reason why dropping the lock around
>> qcrypto_block_decrypt() would be OK.)
>>
>>> 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>
>>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>>> ---
>>>   block/qcow2.c | 46 ++++++++++++++++++++++------------------------
>>>   1 file changed, 22 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 46e8e39da5..fcf92a7eb6 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -1983,6 +1983,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>>>           ret = qcow2_get_cluster_offset(bs, offset, &cur_bytes, &cluster_offset);
>>
>> Isn't this the only function in the loop that actually needs the lock?
>> Wouldn't it make more sense to just take it around this call?
>>
> 
> Hmm, looks correct, I'll resend.
> 
> 

Or not, actually, we may have several qcow2_get_data_offset calls under one lock,
if clusters are different kinds of ZERO. So, I think better to keep it as is for now.



-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v5 01/10] tests/perf: Test qemu-img convert from raw to encrypted qcow2

[Max Reitz]

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

On 30.04.19 10:53, Vladimir Sementsov-Ogievskiy wrote:
> 29.04.2019 1:55, Max Reitz wrote:
>> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   tests/perf/block/qcow2/convert-to-encrypted | 48 +++++++++++++++++++++
>>>   1 file changed, 48 insertions(+)
>>>   create mode 100755 tests/perf/block/qcow2/convert-to-encrypted
>>
>> Thanks for the test case, but I don’t know whether this is the right way
>> to include it.
>>
>> A concrete problem is that it doesn’t work with out-of-tree builds (I
>> only do out-of-tree builds).  I wonder whether it would be possible and
>> make sense (I have no idea) to add a subdirectory "perf" to the iotests
>> and reuse its infrastructure?  Those tests wouldn’t run by default.
>>
> 
> Honestly, I don't really like existing iotests infrastructure, bound to check
> script, which I don't like too (and any other large script in bash, sorry :(..

Hm, OK.  It would need some modifications, because it’d need to accept
non-numeric test names, and for perf tests, it probably shouldn’t
compare against a reference output.  But I don’t like bash either, and
that doesn’t sound impossible to me.

> What do you mean? You have env variables QEMU_IMG, etc, and want them to be
> accepted by script?

That would work, although it’d be cumbersome.  As for check, you just
run it from the build tree, so it auto-detects the binaries.

> I'd prefer to commit something simple and separate, to be able to build up
> infrastructure around it gradually..

Well, I’d prefer something that works.  I’d also very much prefer
something that is not separate, because that’s just adding complexity
for no good reason.  I don’t see how new infrastructure that works can
be simple.

There are Avocado tests, maybe you prefer using that.

> Finally, I want a simple way to run
> a set of perf tests on a set of git commits and get an html and ascii
> tables of performance comparison between these commits.

That doesn’t sound very simple to me, implementation-wise.  It doesn’t
sound overly complicated either, it sounds useful – but throwing out
check now because you say you just need something simple, and then
intending to make it not-so-simple later makes me raise my eyebrows.

I mean, feel free to rewrite the check script in Python, but having two
separate test infrastructures feels like a bit of waste to me.

(And you don’t have to add these new features like comparison of
performance results between commits to the check script, I think.  For
instance, you can write a wrapper script in e.g. Python or whatever that
just calls check to run the test and then processes the test result
itself.  I don’t want to force you to write more bash code, nobody wants
that, I just think that check works fine as a test launcher.)

Max

>>> diff --git a/tests/perf/block/qcow2/convert-to-encrypted b/tests/perf/block/qcow2/convert-to-encrypted
>>> new file mode 100755
>>> index 0000000000..7a6b7b1cab
>>> --- /dev/null
>>> +++ b/tests/perf/block/qcow2/convert-to-encrypted
>>> @@ -0,0 +1,48 @@
>>> +#!/bin/bash
>>> +#
>>> +# Test qemu-img convert from raw to encrypted qcow2
>>> +#
>>> +# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
>>> +#
>>> +# This program is free software; you can redistribute it and/or modify
>>> +# it under the terms of the GNU General Public License as published by
>>> +# the Free Software Foundation; either version 2 of the License, or
>>> +# (at your option) any later version.
>>> +#
>>> +# This program is distributed in the hope that it will be useful,
>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +# GNU General Public License for more details.
>>> +#
>>> +# You should have received a copy of the GNU General Public License
>>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> +#
>>> +
>>> +if [ "$#" -lt 2 ]; then
>>> +    echo "Usage: $0 SOURCE_FILE DESTINATION_FILE [additional qemu-img convert parameters]"
>>> +    exit 1
>>> +fi
>>> +
>>> +ROOT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )/../../../.." >/dev/null 2>&1 && pwd )"
>>> +QEMU_IMG="$ROOT_DIR/qemu-img"
>>> +QEMU_IO="$ROOT_DIR/qemu-io"
>>> +
>>> +size=1G
>>> +
>>> +src="$1"
>>> +shift
>>> +
>>> +dst="$1"
>>> +shift
>>> +
>>> +(
>>> +# create source
>>> +$QEMU_IMG create -f raw "$src" $size
>>> +$QEMU_IO -f raw -c "write -P 0xa 0 $size" "$src"
>>> +
>>> +# 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
>>> +
>>> +# test with additional parameters left in $@
>>> +/usr/bin/time -f %e $QEMU_IMG convert "$@" -f raw --object secret,id=sec0,data=test --target-image-opts -n "$src" "driver=qcow2,file.filename=$dst,encrypt.key-secret=sec0"
>>>
>>
>>
> 
> 



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

Re: [Qemu-devel] [PATCH v5 01/10] tests/perf: Test qemu-img convert from raw to encrypted qcow2

[Max Reitz]

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

On 30.04.19 10:53, Vladimir Sementsov-Ogievskiy wrote:
> 29.04.2019 1:55, Max Reitz wrote:
>> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   tests/perf/block/qcow2/convert-to-encrypted | 48 +++++++++++++++++++++
>>>   1 file changed, 48 insertions(+)
>>>   create mode 100755 tests/perf/block/qcow2/convert-to-encrypted
>>
>> Thanks for the test case, but I don’t know whether this is the right way
>> to include it.
>>
>> A concrete problem is that it doesn’t work with out-of-tree builds (I
>> only do out-of-tree builds).  I wonder whether it would be possible and
>> make sense (I have no idea) to add a subdirectory "perf" to the iotests
>> and reuse its infrastructure?  Those tests wouldn’t run by default.
>>
> 
> Honestly, I don't really like existing iotests infrastructure, bound to check
> script, which I don't like too (and any other large script in bash, sorry :(..

Hm, OK.  It would need some modifications, because it’d need to accept
non-numeric test names, and for perf tests, it probably shouldn’t
compare against a reference output.  But I don’t like bash either, and
that doesn’t sound impossible to me.

> What do you mean? You have env variables QEMU_IMG, etc, and want them to be
> accepted by script?

That would work, although it’d be cumbersome.  As for check, you just
run it from the build tree, so it auto-detects the binaries.

> I'd prefer to commit something simple and separate, to be able to build up
> infrastructure around it gradually..

Well, I’d prefer something that works.  I’d also very much prefer
something that is not separate, because that’s just adding complexity
for no good reason.  I don’t see how new infrastructure that works can
be simple.

There are Avocado tests, maybe you prefer using that.

> Finally, I want a simple way to run
> a set of perf tests on a set of git commits and get an html and ascii
> tables of performance comparison between these commits.

That doesn’t sound very simple to me, implementation-wise.  It doesn’t
sound overly complicated either, it sounds useful – but throwing out
check now because you say you just need something simple, and then
intending to make it not-so-simple later makes me raise my eyebrows.

I mean, feel free to rewrite the check script in Python, but having two
separate test infrastructures feels like a bit of waste to me.

(And you don’t have to add these new features like comparison of
performance results between commits to the check script, I think.  For
instance, you can write a wrapper script in e.g. Python or whatever that
just calls check to run the test and then processes the test result
itself.  I don’t want to force you to write more bash code, nobody wants
that, I just think that check works fine as a test launcher.)

Max

>>> diff --git a/tests/perf/block/qcow2/convert-to-encrypted b/tests/perf/block/qcow2/convert-to-encrypted
>>> new file mode 100755
>>> index 0000000000..7a6b7b1cab
>>> --- /dev/null
>>> +++ b/tests/perf/block/qcow2/convert-to-encrypted
>>> @@ -0,0 +1,48 @@
>>> +#!/bin/bash
>>> +#
>>> +# Test qemu-img convert from raw to encrypted qcow2
>>> +#
>>> +# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
>>> +#
>>> +# This program is free software; you can redistribute it and/or modify
>>> +# it under the terms of the GNU General Public License as published by
>>> +# the Free Software Foundation; either version 2 of the License, or
>>> +# (at your option) any later version.
>>> +#
>>> +# This program is distributed in the hope that it will be useful,
>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +# GNU General Public License for more details.
>>> +#
>>> +# You should have received a copy of the GNU General Public License
>>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> +#
>>> +
>>> +if [ "$#" -lt 2 ]; then
>>> +    echo "Usage: $0 SOURCE_FILE DESTINATION_FILE [additional qemu-img convert parameters]"
>>> +    exit 1
>>> +fi
>>> +
>>> +ROOT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )/../../../.." >/dev/null 2>&1 && pwd )"
>>> +QEMU_IMG="$ROOT_DIR/qemu-img"
>>> +QEMU_IO="$ROOT_DIR/qemu-io"
>>> +
>>> +size=1G
>>> +
>>> +src="$1"
>>> +shift
>>> +
>>> +dst="$1"
>>> +shift
>>> +
>>> +(
>>> +# create source
>>> +$QEMU_IMG create -f raw "$src" $size
>>> +$QEMU_IO -f raw -c "write -P 0xa 0 $size" "$src"
>>> +
>>> +# 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
>>> +
>>> +# test with additional parameters left in $@
>>> +/usr/bin/time -f %e $QEMU_IMG convert "$@" -f raw --object secret,id=sec0,data=test --target-image-opts -n "$src" "driver=qcow2,file.filename=$dst,encrypt.key-secret=sec0"
>>>
>>
>>
> 
> 



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

Re: [Qemu-devel] [PATCH v5 07/10] qcow2: qcow2_co_preadv: improve locking

[Max Reitz]

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

On 30.04.19 11:44, Vladimir Sementsov-Ogievskiy wrote:
> 30.04.2019 11:38, Vladimir Sementsov-Ogievskiy wrote:
>> 29.04.2019 19:37, Max Reitz wrote:
>>> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>>>> Background: decryption will be done in threads, to take benefit of it,
>>>> we should move it out of the lock first.
>>>
>>> ...which is safe after your commit c972fa123c73501b4, I presume.
>>>
>>> (At first glance, the patched looked a bit weird to me because it
>>> doesn't give a reason why dropping the lock around
>>> qcrypto_block_decrypt() would be OK.)
>>>
>>>> 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>
>>>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>>>> ---
>>>>   block/qcow2.c | 46 ++++++++++++++++++++++------------------------
>>>>   1 file changed, 22 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>> index 46e8e39da5..fcf92a7eb6 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>>> @@ -1983,6 +1983,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>>>>           ret = qcow2_get_cluster_offset(bs, offset, &cur_bytes, &cluster_offset);
>>>
>>> Isn't this the only function in the loop that actually needs the lock?
>>> Wouldn't it make more sense to just take it around this call?
>>>
>>
>> Hmm, looks correct, I'll resend.
>>
>>
> 
> Or not, actually, we may have several qcow2_get_data_offset calls under one lock,
> if clusters are different kinds of ZERO. So, I think better to keep it as is for now.

Hm, but how is this relevant?  For one thing, if that was a problem if
some other party concurrently changes the image, then that would be a
problem in general.  Keeping the lock would hide it for different kinds
of read-as-zero clusters, but it would still appear if data clusters and
other clusters are interleaved, wouldn’t it?

Also, this is a coroutine.  As long as nothing yields, nothing gets
concurrent access.  I don’t see anything outside of
qcow2_get_cluster_offset() that could yield as long as we only see
read-as-zero clusters.

Max


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

Re: [Qemu-devel] [PATCH v5 07/10] qcow2: qcow2_co_preadv: improve locking

[Max Reitz]

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

On 30.04.19 11:44, Vladimir Sementsov-Ogievskiy wrote:
> 30.04.2019 11:38, Vladimir Sementsov-Ogievskiy wrote:
>> 29.04.2019 19:37, Max Reitz wrote:
>>> On 02.04.19 17:37, Vladimir Sementsov-Ogievskiy wrote:
>>>> Background: decryption will be done in threads, to take benefit of it,
>>>> we should move it out of the lock first.
>>>
>>> ...which is safe after your commit c972fa123c73501b4, I presume.
>>>
>>> (At first glance, the patched looked a bit weird to me because it
>>> doesn't give a reason why dropping the lock around
>>> qcrypto_block_decrypt() would be OK.)
>>>
>>>> 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>
>>>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>>>> ---
>>>>   block/qcow2.c | 46 ++++++++++++++++++++++------------------------
>>>>   1 file changed, 22 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>> index 46e8e39da5..fcf92a7eb6 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>>> @@ -1983,6 +1983,7 @@ static coroutine_fn int qcow2_co_preadv(BlockDriverState *bs, uint64_t offset,
>>>>           ret = qcow2_get_cluster_offset(bs, offset, &cur_bytes, &cluster_offset);
>>>
>>> Isn't this the only function in the loop that actually needs the lock?
>>> Wouldn't it make more sense to just take it around this call?
>>>
>>
>> Hmm, looks correct, I'll resend.
>>
>>
> 
> Or not, actually, we may have several qcow2_get_data_offset calls under one lock,
> if clusters are different kinds of ZERO. So, I think better to keep it as is for now.

Hm, but how is this relevant?  For one thing, if that was a problem if
some other party concurrently changes the image, then that would be a
problem in general.  Keeping the lock would hide it for different kinds
of read-as-zero clusters, but it would still appear if data clusters and
other clusters are interleaved, wouldn’t it?

Also, this is a coroutine.  As long as nothing yields, nothing gets
concurrent access.  I don’t see anything outside of
qcow2_get_cluster_offset() that could yield as long as we only see
read-as-zero clusters.

Max


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