* [Qemu-devel] [PATCH v3 1/4] block: introduce aio task pool
2019-08-15 12:10 [Qemu-devel] [PATCH v3 0/4] qcow2: async handling of fragmented io Vladimir Sementsov-Ogievskiy
@ 2019-08-15 12:10 ` Vladimir Sementsov-Ogievskiy
2019-08-15 13:41 ` Max Reitz
2019-08-15 12:10 ` [Qemu-devel] [PATCH v3 2/4] block/qcow2: refactor qcow2_co_preadv_part Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-08-15 12:10 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, armbru, qemu-devel, mreitz, stefanha, den
Common interface for aio task loops. To be used for improving
performance of synchronous io loops in qcow2, block-stream,
copy-on-read, and may be other places.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
include/block/aio_task.h | 54 +++++++++++++++++
block/aio_task.c | 124 +++++++++++++++++++++++++++++++++++++++
block/Makefile.objs | 2 +
3 files changed, 180 insertions(+)
create mode 100644 include/block/aio_task.h
create mode 100644 block/aio_task.c
diff --git a/include/block/aio_task.h b/include/block/aio_task.h
new file mode 100644
index 0000000000..58b4d99e59
--- /dev/null
+++ b/include/block/aio_task.h
@@ -0,0 +1,54 @@
+/*
+ * Aio tasks loops
+ *
+ * Copyright (c) 2019 Virtuozzo International GmbH.
+ *
+ * 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.
+ */
+
+#ifndef BLOCK_AIO_TASK_H
+#define BLOCK_AIO_TASK_H
+
+#include "qemu/coroutine.h"
+
+typedef struct AioTaskPool AioTaskPool;
+typedef struct AioTask AioTask;
+typedef int coroutine_fn (*AioTaskFunc)(AioTask *task);
+struct AioTask {
+ AioTaskPool *pool;
+ AioTaskFunc func;
+ int ret;
+};
+
+AioTaskPool *aio_task_pool_new(int max_busy_tasks);
+void aio_task_pool_free(AioTaskPool *);
+
+/* error code of failed task or 0 if all is OK */
+int aio_task_pool_status(AioTaskPool *pool);
+
+bool aio_task_pool_empty(AioTaskPool *pool);
+
+/* User provides filled @task, however task->pool will be set automatically */
+void coroutine_fn aio_task_pool_start_task(AioTaskPool *pool, AioTask *task);
+
+void coroutine_fn aio_task_pool_wait_slot(AioTaskPool *pool);
+void coroutine_fn aio_task_pool_wait_one(AioTaskPool *pool);
+void coroutine_fn aio_task_pool_wait_all(AioTaskPool *pool);
+
+#endif /* BLOCK_AIO_TASK_H */
diff --git a/block/aio_task.c b/block/aio_task.c
new file mode 100644
index 0000000000..3eacfd1f40
--- /dev/null
+++ b/block/aio_task.c
@@ -0,0 +1,124 @@
+/*
+ * Aio tasks loops
+ *
+ * Copyright (c) 2019 Virtuozzo International GmbH.
+ *
+ * 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"
+#include "block/aio.h"
+#include "block/aio_task.h"
+
+struct AioTaskPool {
+ Coroutine *main_co;
+ int status;
+ int max_busy_tasks;
+ int busy_tasks;
+ bool waiting;
+};
+
+static void coroutine_fn aio_task_co(void *opaque)
+{
+ AioTask *task = opaque;
+ AioTaskPool *pool = task->pool;
+
+ assert(pool->busy_tasks < pool->max_busy_tasks);
+ pool->busy_tasks++;
+
+ task->ret = task->func(task);
+
+ pool->busy_tasks--;
+
+ if (task->ret < 0 && pool->status == 0) {
+ pool->status = task->ret;
+ }
+
+ g_free(task);
+
+ if (pool->waiting) {
+ pool->waiting = false;
+ aio_co_wake(pool->main_co);
+ }
+}
+
+void coroutine_fn aio_task_pool_wait_one(AioTaskPool *pool)
+{
+ assert(pool->busy_tasks > 0);
+ assert(qemu_coroutine_self() == pool->main_co);
+
+ pool->waiting = true;
+ qemu_coroutine_yield();
+
+ assert(!pool->waiting);
+ assert(pool->busy_tasks < pool->max_busy_tasks);
+}
+
+void coroutine_fn aio_task_pool_wait_slot(AioTaskPool *pool)
+{
+ if (pool->busy_tasks < pool->max_busy_tasks) {
+ return;
+ }
+
+ aio_task_pool_wait_one(pool);
+}
+
+void coroutine_fn aio_task_pool_wait_all(AioTaskPool *pool)
+{
+ while (pool->busy_tasks > 0) {
+ aio_task_pool_wait_one(pool);
+ }
+}
+
+void coroutine_fn aio_task_pool_start_task(AioTaskPool *pool, AioTask *task)
+{
+ aio_task_pool_wait_slot(pool);
+
+ task->pool = pool;
+ qemu_coroutine_enter(qemu_coroutine_create(aio_task_co, task));
+}
+
+AioTaskPool *aio_task_pool_new(int max_busy_tasks)
+{
+ AioTaskPool *pool = g_new0(AioTaskPool, 1);
+
+ pool->main_co = qemu_coroutine_self();
+ pool->max_busy_tasks = max_busy_tasks;
+
+ return pool;
+}
+
+void aio_task_pool_free(AioTaskPool *pool)
+{
+ g_free(pool);
+}
+
+int aio_task_pool_status(AioTaskPool *pool)
+{
+ if (!pool) {
+ return 0; /* Sugar for lazy allocation of aio pool */
+ }
+
+ return pool->status;
+}
+
+bool aio_task_pool_empty(AioTaskPool *pool)
+{
+ return pool->busy_tasks == 0;
+}
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 35f3bca4d9..c2eb8c8769 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -40,6 +40,8 @@ block-obj-y += throttle.o copy-on-read.o
block-obj-y += crypto.o
+block-obj-y += aio_task.o
+
common-obj-y += stream.o
nfs.o-libs := $(LIBNFS_LIBS)
--
2.18.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/4] block: introduce aio task pool
2019-08-15 12:10 ` [Qemu-devel] [PATCH v3 1/4] block: introduce aio task pool Vladimir Sementsov-Ogievskiy
@ 2019-08-15 13:41 ` Max Reitz
0 siblings, 0 replies; 14+ messages in thread
From: Max Reitz @ 2019-08-15 13:41 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, qemu-devel, armbru, stefanha, den
[-- Attachment #1.1: Type: text/plain, Size: 1165 bytes --]
On 15.08.19 14:10, Vladimir Sementsov-Ogievskiy wrote:
> Common interface for aio task loops. To be used for improving
> performance of synchronous io loops in qcow2, block-stream,
> copy-on-read, and may be other places.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> include/block/aio_task.h | 54 +++++++++++++++++
> block/aio_task.c | 124 +++++++++++++++++++++++++++++++++++++++
> block/Makefile.objs | 2 +
> 3 files changed, 180 insertions(+)
> create mode 100644 include/block/aio_task.h
> create mode 100644 block/aio_task.c
>
> diff --git a/include/block/aio_task.h b/include/block/aio_task.h
> new file mode 100644
> index 0000000000..58b4d99e59
> --- /dev/null
> +++ b/include/block/aio_task.h
[...]
> +AioTaskPool *aio_task_pool_new(int max_busy_tasks);
Because aio_task_pool_wait_one() asserts that it runs in the same
coroutine as aio_task_pool_new(), this should be a coroutine_fn as well.
O:-)
But I don’t want to be responsible for breaking your “1” key (assuming
you have the exclamation mark there):
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v3 2/4] block/qcow2: refactor qcow2_co_preadv_part
2019-08-15 12:10 [Qemu-devel] [PATCH v3 0/4] qcow2: async handling of fragmented io Vladimir Sementsov-Ogievskiy
2019-08-15 12:10 ` [Qemu-devel] [PATCH v3 1/4] block: introduce aio task pool Vladimir Sementsov-Ogievskiy
@ 2019-08-15 12:10 ` Vladimir Sementsov-Ogievskiy
2019-08-15 13:56 ` Max Reitz
2019-08-15 12:10 ` [Qemu-devel] [PATCH v3 3/4] block/qcow2: refactor qcow2_co_pwritev_part Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-08-15 12:10 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, armbru, qemu-devel, mreitz, stefanha, den
Further patch will run partial requests of iterations of
qcow2_co_preadv in parallel for performance reasons. To prepare for
this, separate part which may be parallelized into separate function
(qcow2_co_preadv_task).
While being here, also separate encrypted clusters reading to own
function, like it is done for compressed reading.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
qapi/block-core.json | 2 +-
block/qcow2.c | 205 +++++++++++++++++++++++--------------------
2 files changed, 111 insertions(+), 96 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0d43d4f37c..dd80aa11db 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3266,7 +3266,7 @@
'pwritev_rmw_tail', 'pwritev_rmw_after_tail', 'pwritev',
'pwritev_zero', 'pwritev_done', 'empty_image_prepare',
'l1_shrink_write_table', 'l1_shrink_free_l2_clusters',
- 'cor_write', 'cluster_alloc_space', 'none'] }
+ 'cor_write', 'cluster_alloc_space', 'none', 'read_encrypted'] }
##
# @BlkdebugIOType:
diff --git a/block/qcow2.c b/block/qcow2.c
index 93ab7edcea..89afb4272e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1967,17 +1967,114 @@ out:
return ret;
}
+static coroutine_fn int
+qcow2_co_preadv_encrypted(BlockDriverState *bs,
+ uint64_t file_cluster_offset,
+ uint64_t offset,
+ uint64_t bytes,
+ QEMUIOVector *qiov,
+ uint64_t qiov_offset)
+{
+ int ret;
+ BDRVQcow2State *s = bs->opaque;
+ uint8_t *buf;
+
+ assert(bs->encrypted && s->crypto);
+ assert(bytes <= QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size);
+
+ /*
+ * For encrypted images, read everything into a temporary
+ * contiguous buffer on which the AES functions can work.
+ * Also, decryption in a separate buffer is better as it
+ * prevents the guest from learning information about the
+ * encrypted nature of the virtual disk.
+ */
+
+ buf = qemu_try_blockalign(s->data_file->bs, bytes);
+ if (buf == NULL) {
+ return -ENOMEM;
+ }
+
+ BLKDBG_EVENT(bs->file, BLKDBG_READ_ENCRYPTED);
+ ret = bdrv_co_pread(s->data_file,
+ file_cluster_offset + offset_into_cluster(s, offset),
+ bytes, buf, 0);
+ if (ret < 0) {
+ goto fail;
+ }
+
+ assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
+ assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
+ if (qcow2_co_decrypt(bs, file_cluster_offset, offset, buf, bytes) < 0) {
+ ret = -EIO;
+ goto fail;
+ }
+ qemu_iovec_from_buf(qiov, qiov_offset, buf, bytes);
+
+fail:
+ qemu_vfree(buf);
+
+ return ret;
+}
+
+static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
+ QCow2ClusterType cluster_type,
+ uint64_t file_cluster_offset,
+ uint64_t offset, uint64_t bytes,
+ QEMUIOVector *qiov,
+ size_t qiov_offset)
+{
+ BDRVQcow2State *s = bs->opaque;
+ int offset_in_cluster = offset_into_cluster(s, offset);
+
+ switch (cluster_type) {
+ case QCOW2_CLUSTER_ZERO_PLAIN:
+ case QCOW2_CLUSTER_ZERO_ALLOC:
+ /* Both zero types are handled in qcow2_co_preadv_part */
+ g_assert_not_reached();
+
+ case QCOW2_CLUSTER_UNALLOCATED:
+ assert(bs->backing); /* otherwise handled in qcow2_co_preadv_part */
+
+ BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
+ return bdrv_co_preadv_part(bs->backing, offset, bytes,
+ qiov, qiov_offset, 0);
+
+ case QCOW2_CLUSTER_COMPRESSED:
+ return qcow2_co_preadv_compressed(bs, file_cluster_offset,
+ offset, bytes, qiov, qiov_offset);
+
+ case QCOW2_CLUSTER_NORMAL:
+ if ((file_cluster_offset & 511) != 0) {
+ return -EIO;
+ }
+
+ if (bs->encrypted) {
+ return qcow2_co_preadv_encrypted(bs, file_cluster_offset,
+ offset, bytes, qiov, qiov_offset);
+ }
+
+ BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
+ return bdrv_co_preadv_part(s->data_file,
+ file_cluster_offset + offset_in_cluster,
+ bytes, qiov, qiov_offset, 0);
+
+ default:
+ g_assert_not_reached();
+ }
+
+ g_assert_not_reached();
+}
+
static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
uint64_t offset, uint64_t bytes,
QEMUIOVector *qiov,
size_t qiov_offset, int flags)
{
BDRVQcow2State *s = bs->opaque;
- int offset_in_cluster;
int ret;
unsigned int cur_bytes; /* number of bytes in current iteration */
uint64_t cluster_offset = 0;
- uint8_t *cluster_data = NULL;
while (bytes != 0) {
@@ -1992,111 +2089,29 @@ static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
ret = qcow2_get_cluster_offset(bs, offset, &cur_bytes, &cluster_offset);
qemu_co_mutex_unlock(&s->lock);
if (ret < 0) {
- goto fail;
+ return ret;
}
- offset_in_cluster = offset_into_cluster(s, offset);
-
- switch (ret) {
- case QCOW2_CLUSTER_UNALLOCATED:
-
- if (bs->backing) {
- BLKDBG_EVENT(bs->file, BLKDBG_READ_BACKING_AIO);
- ret = bdrv_co_preadv_part(bs->backing, offset, cur_bytes,
- qiov, qiov_offset, 0);
- if (ret < 0) {
- goto fail;
- }
- } else {
- /* Note: in this case, no need to wait */
- qemu_iovec_memset(qiov, qiov_offset, 0, cur_bytes);
- }
- break;
-
- case QCOW2_CLUSTER_ZERO_PLAIN:
- case QCOW2_CLUSTER_ZERO_ALLOC:
+ if (ret == QCOW2_CLUSTER_ZERO_PLAIN ||
+ ret == QCOW2_CLUSTER_ZERO_ALLOC ||
+ (ret == QCOW2_CLUSTER_UNALLOCATED && !bs->backing))
+ {
qemu_iovec_memset(qiov, qiov_offset, 0, cur_bytes);
- break;
-
- case QCOW2_CLUSTER_COMPRESSED:
- ret = qcow2_co_preadv_compressed(bs, cluster_offset,
- offset, cur_bytes,
- qiov, qiov_offset);
+ } else {
+ ret = qcow2_co_preadv_task(bs, ret,
+ cluster_offset, offset, cur_bytes,
+ qiov, qiov_offset);
if (ret < 0) {
- goto fail;
- }
-
- break;
-
- case QCOW2_CLUSTER_NORMAL:
- if ((cluster_offset & 511) != 0) {
- ret = -EIO;
- goto fail;
- }
-
- if (bs->encrypted) {
- assert(s->crypto);
-
- /*
- * For encrypted images, read everything into a temporary
- * contiguous buffer on which the AES functions can work.
- */
- if (!cluster_data) {
- cluster_data =
- qemu_try_blockalign(s->data_file->bs,
- QCOW_MAX_CRYPT_CLUSTERS
- * s->cluster_size);
- if (cluster_data == NULL) {
- ret = -ENOMEM;
- goto fail;
- }
- }
-
- assert(cur_bytes <= QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size);
-
- BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
- ret = bdrv_co_pread(s->data_file,
- cluster_offset + offset_in_cluster,
- cur_bytes, cluster_data, 0);
- if (ret < 0) {
- goto fail;
- }
-
- assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
- assert((cur_bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
- if (qcow2_co_decrypt(bs, cluster_offset, offset,
- cluster_data, cur_bytes) < 0) {
- ret = -EIO;
- goto fail;
- }
- qemu_iovec_from_buf(qiov, qiov_offset, cluster_data, cur_bytes);
- } else {
- BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
- ret = bdrv_co_preadv_part(s->data_file,
- cluster_offset + offset_in_cluster,
- cur_bytes, qiov, qiov_offset, 0);
- if (ret < 0) {
- goto fail;
- }
+ return ret;
}
- break;
-
- default:
- g_assert_not_reached();
- ret = -EIO;
- goto fail;
}
bytes -= cur_bytes;
offset += cur_bytes;
qiov_offset += cur_bytes;
}
- ret = 0;
-
-fail:
- qemu_vfree(cluster_data);
- return ret;
+ return 0;
}
/* Check if it's possible to merge a write request with the writing of
--
2.18.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/4] block/qcow2: refactor qcow2_co_preadv_part
2019-08-15 12:10 ` [Qemu-devel] [PATCH v3 2/4] block/qcow2: refactor qcow2_co_preadv_part Vladimir Sementsov-Ogievskiy
@ 2019-08-15 13:56 ` Max Reitz
0 siblings, 0 replies; 14+ messages in thread
From: Max Reitz @ 2019-08-15 13:56 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, qemu-devel, armbru, stefanha, den
[-- Attachment #1.1: Type: text/plain, Size: 710 bytes --]
On 15.08.19 14:10, Vladimir Sementsov-Ogievskiy wrote:
> Further patch will run partial requests of iterations of
> qcow2_co_preadv in parallel for performance reasons. To prepare for
> this, separate part which may be parallelized into separate function
> (qcow2_co_preadv_task).
>
> While being here, also separate encrypted clusters reading to own
> function, like it is done for compressed reading.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> qapi/block-core.json | 2 +-
> block/qcow2.c | 205 +++++++++++++++++++++++--------------------
> 2 files changed, 111 insertions(+), 96 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v3 3/4] block/qcow2: refactor qcow2_co_pwritev_part
2019-08-15 12:10 [Qemu-devel] [PATCH v3 0/4] qcow2: async handling of fragmented io Vladimir Sementsov-Ogievskiy
2019-08-15 12:10 ` [Qemu-devel] [PATCH v3 1/4] block: introduce aio task pool Vladimir Sementsov-Ogievskiy
2019-08-15 12:10 ` [Qemu-devel] [PATCH v3 2/4] block/qcow2: refactor qcow2_co_preadv_part Vladimir Sementsov-Ogievskiy
@ 2019-08-15 12:10 ` Vladimir Sementsov-Ogievskiy
2019-08-15 14:02 ` Max Reitz
2019-08-15 12:10 ` [Qemu-devel] [PATCH v3 4/4] block/qcow2: introduce parallel subrequest handling in read and write Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-08-15 12:10 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, armbru, qemu-devel, mreitz, stefanha, den
Similarly to previous commit, prepare for parallelizing write-loop
iterations.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2.c | 153 +++++++++++++++++++++++++++++---------------------
1 file changed, 89 insertions(+), 64 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 89afb4272e..3aaa180e2b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2234,6 +2234,87 @@ static int handle_alloc_space(BlockDriverState *bs, QCowL2Meta *l2meta)
return 0;
}
+/*
+ * qcow2_co_pwritev_task
+ * Called with s->lock unlocked
+ * l2meta - if not NULL, qcow2_co_do_pwritev() will consume it. Caller must not
+ * use it somehow after qcow2_co_pwritev_task() call
+ */
+static coroutine_fn int qcow2_co_pwritev_task(BlockDriverState *bs,
+ uint64_t file_cluster_offset,
+ uint64_t offset, uint64_t bytes,
+ QEMUIOVector *qiov,
+ uint64_t qiov_offset,
+ QCowL2Meta *l2meta)
+{
+ int ret;
+ BDRVQcow2State *s = bs->opaque;
+ void *crypt_buf = NULL;
+ int offset_in_cluster = offset_into_cluster(s, offset);
+ QEMUIOVector encrypted_qiov;
+
+ if (bs->encrypted) {
+ assert(s->crypto);
+ assert(bytes <= QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size);
+ crypt_buf = qemu_try_blockalign(bs->file->bs, bytes);
+ if (crypt_buf == NULL) {
+ ret = -ENOMEM;
+ goto out_unlocked;
+ }
+ qemu_iovec_to_buf(qiov, qiov_offset, crypt_buf, bytes);
+
+ if (qcow2_co_encrypt(bs, file_cluster_offset, offset,
+ crypt_buf, bytes) < 0) {
+ ret = -EIO;
+ goto out_unlocked;
+ }
+
+ qemu_iovec_init_buf(&encrypted_qiov, crypt_buf, bytes);
+ qiov = &encrypted_qiov;
+ qiov_offset = 0;
+ }
+
+ /* Try to efficiently initialize the physical space with zeroes */
+ ret = handle_alloc_space(bs, l2meta);
+ if (ret < 0) {
+ goto out_unlocked;
+ }
+
+ /*
+ * 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, bytes, qiov, qiov_offset, l2meta)) {
+ BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
+ trace_qcow2_writev_data(qemu_coroutine_self(),
+ file_cluster_offset + offset_in_cluster);
+ ret = bdrv_co_pwritev_part(s->data_file,
+ file_cluster_offset + offset_in_cluster,
+ bytes, qiov, qiov_offset, 0);
+ if (ret < 0) {
+ goto out_unlocked;
+ }
+ }
+
+ qemu_co_mutex_lock(&s->lock);
+
+ ret = qcow2_handle_l2meta(bs, &l2meta, true);
+ goto out_locked;
+
+out_unlocked:
+ qemu_co_mutex_lock(&s->lock);
+
+out_locked:
+ qcow2_handle_l2meta(bs, &l2meta, false);
+ qemu_co_mutex_unlock(&s->lock);
+
+ qemu_vfree(crypt_buf);
+
+ return ret;
+}
+
static coroutine_fn int qcow2_co_pwritev_part(
BlockDriverState *bs, uint64_t offset, uint64_t bytes,
QEMUIOVector *qiov, size_t qiov_offset, int flags)
@@ -2243,15 +2324,10 @@ static coroutine_fn int qcow2_co_pwritev_part(
int ret;
unsigned int cur_bytes; /* number of sectors in current iteration */
uint64_t cluster_offset;
- QEMUIOVector encrypted_qiov;
- uint64_t bytes_done = 0;
- uint8_t *cluster_data = NULL;
QCowL2Meta *l2meta = NULL;
trace_qcow2_writev_start_req(qemu_coroutine_self(), offset, bytes);
- qemu_co_mutex_lock(&s->lock);
-
while (bytes != 0) {
l2meta = NULL;
@@ -2265,6 +2341,8 @@ static coroutine_fn int qcow2_co_pwritev_part(
- offset_in_cluster);
}
+ qemu_co_mutex_lock(&s->lock);
+
ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
&cluster_offset, &l2meta);
if (ret < 0) {
@@ -2282,73 +2360,20 @@ static coroutine_fn int qcow2_co_pwritev_part(
qemu_co_mutex_unlock(&s->lock);
- if (bs->encrypted) {
- assert(s->crypto);
- if (!cluster_data) {
- cluster_data = qemu_try_blockalign(bs->file->bs,
- QCOW_MAX_CRYPT_CLUSTERS
- * s->cluster_size);
- if (cluster_data == NULL) {
- ret = -ENOMEM;
- goto out_unlocked;
- }
- }
-
- assert(cur_bytes <= QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size);
- qemu_iovec_to_buf(qiov, qiov_offset + bytes_done,
- cluster_data, cur_bytes);
-
- if (qcow2_co_encrypt(bs, cluster_offset, offset,
- cluster_data, cur_bytes) < 0) {
- ret = -EIO;
- goto out_unlocked;
- }
-
- qemu_iovec_init_buf(&encrypted_qiov, cluster_data, cur_bytes);
- }
-
- /* Try to efficiently initialize the physical space with zeroes */
- ret = handle_alloc_space(bs, l2meta);
+ ret = qcow2_co_pwritev_task(bs, cluster_offset, offset, cur_bytes,
+ qiov, qiov_offset, l2meta);
+ l2meta = NULL; /* l2meta is consumed by qcow2_co_pwritev_task() */
if (ret < 0) {
- goto out_unlocked;
- }
-
- /* 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,
- bs->encrypted ? &encrypted_qiov : qiov,
- bs->encrypted ? 0 : qiov_offset + bytes_done, l2meta))
- {
- BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
- trace_qcow2_writev_data(qemu_coroutine_self(),
- cluster_offset + offset_in_cluster);
- ret = bdrv_co_pwritev_part(
- s->data_file, cluster_offset + offset_in_cluster, cur_bytes,
- bs->encrypted ? &encrypted_qiov : qiov,
- bs->encrypted ? 0 : qiov_offset + bytes_done, 0);
- if (ret < 0) {
- goto out_unlocked;
- }
- }
-
- qemu_co_mutex_lock(&s->lock);
-
- ret = qcow2_handle_l2meta(bs, &l2meta, true);
- if (ret) {
- goto out_locked;
+ goto fail_nometa;
}
bytes -= cur_bytes;
offset += cur_bytes;
- bytes_done += cur_bytes;
+ qiov_offset += cur_bytes;
trace_qcow2_writev_done_part(qemu_coroutine_self(), cur_bytes);
}
ret = 0;
- goto out_locked;
-out_unlocked:
qemu_co_mutex_lock(&s->lock);
out_locked:
@@ -2356,7 +2381,7 @@ out_locked:
qemu_co_mutex_unlock(&s->lock);
- qemu_vfree(cluster_data);
+fail_nometa:
trace_qcow2_writev_done_req(qemu_coroutine_self(), ret);
return ret;
--
2.18.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/4] block/qcow2: refactor qcow2_co_pwritev_part
2019-08-15 12:10 ` [Qemu-devel] [PATCH v3 3/4] block/qcow2: refactor qcow2_co_pwritev_part Vladimir Sementsov-Ogievskiy
@ 2019-08-15 14:02 ` Max Reitz
0 siblings, 0 replies; 14+ messages in thread
From: Max Reitz @ 2019-08-15 14:02 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, qemu-devel, armbru, stefanha, den
[-- Attachment #1.1: Type: text/plain, Size: 902 bytes --]
On 15.08.19 14:10, Vladimir Sementsov-Ogievskiy wrote:
> Similarly to previous commit, prepare for parallelizing write-loop
> iterations.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/qcow2.c | 153 +++++++++++++++++++++++++++++---------------------
> 1 file changed, 89 insertions(+), 64 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 89afb4272e..3aaa180e2b 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2234,6 +2234,87 @@ static int handle_alloc_space(BlockDriverState *bs, QCowL2Meta *l2meta)
> return 0;
> }
>
> +/*
> + * qcow2_co_pwritev_task
> + * Called with s->lock unlocked
> + * l2meta - if not NULL, qcow2_co_do_pwritev() will consume it. Caller must not
You missed this instance of “qcow2_co_do_pwritev()”.
With that fixed:
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v3 4/4] block/qcow2: introduce parallel subrequest handling in read and write
2019-08-15 12:10 [Qemu-devel] [PATCH v3 0/4] qcow2: async handling of fragmented io Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2019-08-15 12:10 ` [Qemu-devel] [PATCH v3 3/4] block/qcow2: refactor qcow2_co_pwritev_part Vladimir Sementsov-Ogievskiy
@ 2019-08-15 12:10 ` Vladimir Sementsov-Ogievskiy
2019-08-15 14:07 ` Max Reitz
2019-08-15 13:21 ` [Qemu-devel] [PATCH v3 0/4] qcow2: async handling of fragmented io Max Reitz
2019-08-15 14:09 ` Max Reitz
5 siblings, 1 reply; 14+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-08-15 12:10 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, armbru, qemu-devel, mreitz, stefanha, den
It improves performance for fragmented qcow2 images.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2.h | 3 ++
block/qcow2.c | 125 ++++++++++++++++++++++++++++++++++++++++-----
block/trace-events | 1 +
3 files changed, 117 insertions(+), 12 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 998bcdaef1..fdfa9c31cd 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -65,6 +65,9 @@
#define QCOW2_MAX_BITMAPS 65535
#define QCOW2_MAX_BITMAP_DIRECTORY_SIZE (1024 * QCOW2_MAX_BITMAPS)
+/* Maximum of parallel sub-request per guest request */
+#define QCOW2_MAX_WORKERS 8
+
/* indicate that the refcount of the referenced cluster is exactly one. */
#define QCOW_OFLAG_COPIED (1ULL << 63)
/* indicate that the cluster is compressed (they never have the copied flag) */
diff --git a/block/qcow2.c b/block/qcow2.c
index 3aaa180e2b..36b41e8536 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -40,6 +40,7 @@
#include "qapi/qobject-input-visitor.h"
#include "qapi/qapi-visit-block-core.h"
#include "crypto.h"
+#include "block/aio_task.h"
/*
Differences with QCOW:
@@ -2017,6 +2018,60 @@ fail:
return ret;
}
+typedef struct Qcow2AioTask {
+ AioTask task;
+
+ BlockDriverState *bs;
+ QCow2ClusterType cluster_type; /* only for read */
+ uint64_t file_cluster_offset;
+ uint64_t offset;
+ uint64_t bytes;
+ QEMUIOVector *qiov;
+ uint64_t qiov_offset;
+ QCowL2Meta *l2meta; /* only for write */
+} Qcow2AioTask;
+
+static coroutine_fn int qcow2_co_preadv_task_entry(AioTask *task);
+static coroutine_fn int qcow2_add_task(BlockDriverState *bs,
+ AioTaskPool *pool,
+ AioTaskFunc func,
+ QCow2ClusterType cluster_type,
+ uint64_t file_cluster_offset,
+ uint64_t offset,
+ uint64_t bytes,
+ QEMUIOVector *qiov,
+ size_t qiov_offset,
+ QCowL2Meta *l2meta)
+{
+ Qcow2AioTask local_task;
+ Qcow2AioTask *task = pool ? g_new(Qcow2AioTask, 1) : &local_task;
+
+ *task = (Qcow2AioTask) {
+ .task.func = func,
+ .bs = bs,
+ .cluster_type = cluster_type,
+ .qiov = qiov,
+ .file_cluster_offset = file_cluster_offset,
+ .offset = offset,
+ .bytes = bytes,
+ .qiov_offset = qiov_offset,
+ .l2meta = l2meta,
+ };
+
+ trace_qcow2_add_task(qemu_coroutine_self(), bs, pool,
+ func == qcow2_co_preadv_task_entry ? "read" : "write",
+ cluster_type, file_cluster_offset, offset, bytes,
+ qiov, qiov_offset);
+
+ if (!pool) {
+ return func(&task->task);
+ }
+
+ aio_task_pool_start_task(pool, &task->task);
+
+ return 0;
+}
+
static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
QCow2ClusterType cluster_type,
uint64_t file_cluster_offset,
@@ -2066,18 +2121,28 @@ static coroutine_fn int qcow2_co_preadv_task(BlockDriverState *bs,
g_assert_not_reached();
}
+static coroutine_fn int qcow2_co_preadv_task_entry(AioTask *task)
+{
+ Qcow2AioTask *t = container_of(task, Qcow2AioTask, task);
+
+ assert(!t->l2meta);
+
+ return qcow2_co_preadv_task(t->bs, t->cluster_type, t->file_cluster_offset,
+ t->offset, t->bytes, t->qiov, t->qiov_offset);
+}
+
static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
uint64_t offset, uint64_t bytes,
QEMUIOVector *qiov,
size_t qiov_offset, int flags)
{
BDRVQcow2State *s = bs->opaque;
- int ret;
+ int ret = 0;
unsigned int cur_bytes; /* number of bytes in current iteration */
uint64_t cluster_offset = 0;
+ AioTaskPool *aio = NULL;
- while (bytes != 0) {
-
+ while (bytes != 0 && aio_task_pool_status(aio) == 0) {
/* prepare next request */
cur_bytes = MIN(bytes, INT_MAX);
if (s->crypto) {
@@ -2089,7 +2154,7 @@ static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
ret = qcow2_get_cluster_offset(bs, offset, &cur_bytes, &cluster_offset);
qemu_co_mutex_unlock(&s->lock);
if (ret < 0) {
- return ret;
+ goto out;
}
if (ret == QCOW2_CLUSTER_ZERO_PLAIN ||
@@ -2098,11 +2163,14 @@ static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
{
qemu_iovec_memset(qiov, qiov_offset, 0, cur_bytes);
} else {
- ret = qcow2_co_preadv_task(bs, ret,
- cluster_offset, offset, cur_bytes,
- qiov, qiov_offset);
+ if (!aio && cur_bytes != bytes) {
+ aio = aio_task_pool_new(QCOW2_MAX_WORKERS);
+ }
+ ret = qcow2_add_task(bs, aio, qcow2_co_preadv_task_entry, ret,
+ cluster_offset, offset, cur_bytes,
+ qiov, qiov_offset, NULL);
if (ret < 0) {
- return ret;
+ goto out;
}
}
@@ -2111,7 +2179,16 @@ static coroutine_fn int qcow2_co_preadv_part(BlockDriverState *bs,
qiov_offset += cur_bytes;
}
- return 0;
+out:
+ if (aio) {
+ aio_task_pool_wait_all(aio);
+ if (ret == 0) {
+ ret = aio_task_pool_status(aio);
+ }
+ g_free(aio);
+ }
+
+ return ret;
}
/* Check if it's possible to merge a write request with the writing of
@@ -2315,6 +2392,17 @@ out_locked:
return ret;
}
+static coroutine_fn int qcow2_co_pwritev_task_entry(AioTask *task)
+{
+ Qcow2AioTask *t = container_of(task, Qcow2AioTask, task);
+
+ assert(!t->cluster_type);
+
+ return qcow2_co_pwritev_task(t->bs, t->file_cluster_offset,
+ t->offset, t->bytes, t->qiov, t->qiov_offset,
+ t->l2meta);
+}
+
static coroutine_fn int qcow2_co_pwritev_part(
BlockDriverState *bs, uint64_t offset, uint64_t bytes,
QEMUIOVector *qiov, size_t qiov_offset, int flags)
@@ -2325,10 +2413,11 @@ static coroutine_fn int qcow2_co_pwritev_part(
unsigned int cur_bytes; /* number of sectors in current iteration */
uint64_t cluster_offset;
QCowL2Meta *l2meta = NULL;
+ AioTaskPool *aio = NULL;
trace_qcow2_writev_start_req(qemu_coroutine_self(), offset, bytes);
- while (bytes != 0) {
+ while (bytes != 0 && aio_task_pool_status(aio) == 0) {
l2meta = NULL;
@@ -2360,8 +2449,12 @@ static coroutine_fn int qcow2_co_pwritev_part(
qemu_co_mutex_unlock(&s->lock);
- ret = qcow2_co_pwritev_task(bs, cluster_offset, offset, cur_bytes,
- qiov, qiov_offset, l2meta);
+ if (!aio && cur_bytes != bytes) {
+ aio = aio_task_pool_new(QCOW2_MAX_WORKERS);
+ }
+ ret = qcow2_add_task(bs, aio, qcow2_co_pwritev_task_entry, 0,
+ cluster_offset, offset, cur_bytes,
+ qiov, qiov_offset, l2meta);
l2meta = NULL; /* l2meta is consumed by qcow2_co_pwritev_task() */
if (ret < 0) {
goto fail_nometa;
@@ -2382,6 +2475,14 @@ out_locked:
qemu_co_mutex_unlock(&s->lock);
fail_nometa:
+ if (aio) {
+ aio_task_pool_wait_all(aio);
+ if (ret == 0) {
+ ret = aio_task_pool_status(aio);
+ }
+ g_free(aio);
+ }
+
trace_qcow2_writev_done_req(qemu_coroutine_self(), ret);
return ret;
diff --git a/block/trace-events b/block/trace-events
index d724df0117..7f51550ba3 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -61,6 +61,7 @@ file_paio_submit(void *acb, void *opaque, int64_t offset, int count, int type) "
file_copy_file_range(void *bs, int src, int64_t src_off, int dst, int64_t dst_off, int64_t bytes, int flags, int64_t ret) "bs %p src_fd %d offset %"PRIu64" dst_fd %d offset %"PRIu64" bytes %"PRIu64" flags %d ret %"PRId64
# qcow2.c
+qcow2_add_task(void *co, void *bs, void *pool, const char *action, int cluster_type, uint64_t file_cluster_offset, uint64_t offset, uint64_t bytes, void *qiov, size_t qiov_offset) "co %p bs %p pool %p: %s: cluster_type %d file_cluster_offset %" PRIu64 " offset %" PRIu64 " bytes %" PRIu64 " qiov %p qiov_offset %zu"
qcow2_writev_start_req(void *co, int64_t offset, int bytes) "co %p offset 0x%" PRIx64 " bytes %d"
qcow2_writev_done_req(void *co, int ret) "co %p ret %d"
qcow2_writev_start_part(void *co) "co %p"
--
2.18.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v3 4/4] block/qcow2: introduce parallel subrequest handling in read and write
2019-08-15 12:10 ` [Qemu-devel] [PATCH v3 4/4] block/qcow2: introduce parallel subrequest handling in read and write Vladimir Sementsov-Ogievskiy
@ 2019-08-15 14:07 ` Max Reitz
0 siblings, 0 replies; 14+ messages in thread
From: Max Reitz @ 2019-08-15 14:07 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, qemu-devel, armbru, stefanha, den
[-- Attachment #1.1: Type: text/plain, Size: 439 bytes --]
On 15.08.19 14:10, Vladimir Sementsov-Ogievskiy wrote:
> It improves performance for fragmented qcow2 images.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/qcow2.h | 3 ++
> block/qcow2.c | 125 ++++++++++++++++++++++++++++++++++++++++-----
> block/trace-events | 1 +
> 3 files changed, 117 insertions(+), 12 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/4] qcow2: async handling of fragmented io
2019-08-15 12:10 [Qemu-devel] [PATCH v3 0/4] qcow2: async handling of fragmented io Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2019-08-15 12:10 ` [Qemu-devel] [PATCH v3 4/4] block/qcow2: introduce parallel subrequest handling in read and write Vladimir Sementsov-Ogievskiy
@ 2019-08-15 13:21 ` Max Reitz
2019-08-15 15:35 ` Vladimir Sementsov-Ogievskiy
2019-08-15 14:09 ` Max Reitz
5 siblings, 1 reply; 14+ messages in thread
From: Max Reitz @ 2019-08-15 13:21 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, qemu-devel, armbru, stefanha, den
[-- Attachment #1.1: Type: text/plain, Size: 133 bytes --]
On 15.08.19 14:10, Vladimir Sementsov-Ogievskiy wrote:
> 01: - use coroutine_fn where appropriate !!!!!!!!!!!!!!!!!!!!!!!
:-)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/4] qcow2: async handling of fragmented io
2019-08-15 13:21 ` [Qemu-devel] [PATCH v3 0/4] qcow2: async handling of fragmented io Max Reitz
@ 2019-08-15 15:35 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-08-15 15:35 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: kwolf, Denis Lunev, qemu-devel, armbru, stefanha
15.08.2019 16:21, Max Reitz wrote:
> On 15.08.19 14:10, Vladimir Sementsov-Ogievskiy wrote:
>> 01: - use coroutine_fn where appropriate !!!!!!!!!!!!!!!!!!!!!!!
>
> :-)
>
Ahahaha, I'll explain:
When comparing v2 vs v3 and writing this difference script I noticed
that I added coroutine_fn marks only to .h and not to .c. So I added
some of "!!!" to not foreget to fix it.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/4] qcow2: async handling of fragmented io
2019-08-15 12:10 [Qemu-devel] [PATCH v3 0/4] qcow2: async handling of fragmented io Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2019-08-15 13:21 ` [Qemu-devel] [PATCH v3 0/4] qcow2: async handling of fragmented io Max Reitz
@ 2019-08-15 14:09 ` Max Reitz
2019-08-15 15:39 ` Vladimir Sementsov-Ogievskiy
5 siblings, 1 reply; 14+ messages in thread
From: Max Reitz @ 2019-08-15 14:09 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, qemu-devel, armbru, stefanha, den
[-- Attachment #1.1: Type: text/plain, Size: 693 bytes --]
On 15.08.19 14:10, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
>
> Here is an asynchronous scheme for handling fragmented qcow2
> reads and writes. Both qcow2 read and write functions loops through
> sequential portions of data. The series aim it to parallelize these
> loops iterations.
> It improves performance for fragmented qcow2 images, I've tested it
> as described below.
Looks good to me, but I can’t take it yet because I need to wait for
Stefan’s branch to be merged, of course.
Speaking of which, why didn’t you add any tests for the *_part()
methods? I find it a bit unsettling that nothing would have caught the
bug you had in v2 in patch 3.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/4] qcow2: async handling of fragmented io
2019-08-15 14:09 ` Max Reitz
@ 2019-08-15 15:39 ` Vladimir Sementsov-Ogievskiy
2019-08-16 16:42 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 14+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-08-15 15:39 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: kwolf, Denis Lunev, qemu-devel, armbru, stefanha
15.08.2019 17:09, Max Reitz wrote:
> On 15.08.19 14:10, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here is an asynchronous scheme for handling fragmented qcow2
>> reads and writes. Both qcow2 read and write functions loops through
>> sequential portions of data. The series aim it to parallelize these
>> loops iterations.
>> It improves performance for fragmented qcow2 images, I've tested it
>> as described below.
>
> Looks good to me, but I can’t take it yet because I need to wait for
> Stefan’s branch to be merged, of course.
>
> Speaking of which, why didn’t you add any tests for the *_part()
> methods? I find it a bit unsettling that nothing would have caught the
> bug you had in v2 in patch 3.
>
Hmm, any test with write to fragmented area should have caught it.. Ok,
I'll think of something.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v3 0/4] qcow2: async handling of fragmented io
2019-08-15 15:39 ` Vladimir Sementsov-Ogievskiy
@ 2019-08-16 16:42 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 14+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-08-16 16:42 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: kwolf, Denis Lunev, qemu-devel, armbru, stefanha
15.08.2019 18:39, Vladimir Sementsov-Ogievskiy wrote:
> 15.08.2019 17:09, Max Reitz wrote:
>> On 15.08.19 14:10, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all!
>>>
>>> Here is an asynchronous scheme for handling fragmented qcow2
>>> reads and writes. Both qcow2 read and write functions loops through
>>> sequential portions of data. The series aim it to parallelize these
>>> loops iterations.
>>> It improves performance for fragmented qcow2 images, I've tested it
>>> as described below.
>>
>> Looks good to me, but I can’t take it yet because I need to wait for
>> Stefan’s branch to be merged, of course.
>>
>> Speaking of which, why didn’t you add any tests for the *_part()
>> methods? I find it a bit unsettling that nothing would have caught the
>> bug you had in v2 in patch 3.
>>
>
> Hmm, any test with write to fragmented area should have caught it.. Ok,
> I'll think of something.
>
>
And now I see that it's not trivial to make such a test:
1. qcow2 write is broken when we give nonzero qiov_offset to it, but only
qcow2_write calls bdrv_co_pwritev_part, so we need to have a test where qcow2
is a file child for qcow2
2. Then, the bug leads to the beginning of the qiov will be written to all parts.
But our testing tool qemu-io has only "write -P" command with buffer filled with
the same one byte, so we can't catch it
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 14+ messages in thread