[Qemu-devel] [PATCH 00/29] qed: Convert to coroutines

[Qemu-devel] [PATCH 00/29] qed: Convert to coroutines

[Kevin Wolf]

The qed block driver is one of the last remaining block drivers that use the
AIO callback style interfaces. This series converts it to the coroutine model
that other drivers are using and removes some AIO functions from the block
layer API afterwards.

If this isn't compelling enough, the diffstat should speak for itself.

This series is relatively long, but it consists mostly of mechanical
conversions of one function per patch, so it should be easy to review.

Kevin Wolf (29):
  qed: Use bottom half to resume waiting requests
  qed: Make qed_read_table() synchronous
  qed: Remove callback from qed_read_table()
  qed: Remove callback from qed_read_l2_table()
  qed: Remove callback from qed_find_cluster()
  qed: Make qed_read_backing_file() synchronous
  qed: Make qed_copy_from_backing_file() synchronous
  qed: Remove callback from qed_copy_from_backing_file()
  qed: Make qed_write_header() synchronous
  qed: Remove callback from qed_write_header()
  qed: Make qed_write_table() synchronous
  qed: Remove GenericCB
  qed: Remove callback from qed_write_table()
  qed: Make qed_aio_read_data() synchronous
  qed: Make qed_aio_write_main() synchronous
  qed: Inline qed_commit_l2_update()
  qed: Add return value to qed_aio_write_l1_update()
  qed: Add return value to qed_aio_write_l2_update()
  qed: Add return value to qed_aio_write_main()
  qed: Add return value to qed_aio_write_cow()
  qed: Add return value to qed_aio_write_inplace/alloc()
  qed: Add return value to qed_aio_read/write_data()
  qed: Remove ret argument from qed_aio_next_io()
  qed: Remove recursion in qed_aio_next_io()
  qed: Implement .bdrv_co_readv/writev
  qed: Use CoQueue for serialising allocations
  qed: Simplify request handling
  qed: Use a coroutine for need_check_timer
  block: Remove bdrv_aio_readv/writev_flush()

 block/Makefile.objs   |   2 +-
 block/io.c            | 171 -----------
 block/qed-cluster.c   | 123 ++++----
 block/qed-gencb.c     |  33 ---
 block/qed-table.c     | 261 ++++++-----------
 block/qed.c           | 763 +++++++++++++++++++-------------------------------
 block/qed.h           |  53 +---
 include/block/block.h |   8 -
 8 files changed, 432 insertions(+), 982 deletions(-)
 delete mode 100644 block/qed-gencb.c

-- 
1.8.3.1

[Qemu-devel] [PATCH 01/29] qed: Use bottom half to resume waiting requests

[Kevin Wolf]

The qed driver serialises allocating write requests. When the active
allocation is finished, the AIO callback is called, but after this, the
next allocating request is immediately processed instead of leaving the
coroutine. Resuming another allocation request in the same request
coroutine means that the request now runs in the wrong coroutine.

The following is one of the possible effects of this: The completed
request will generally reenter its request coroutine in a bottom half,
expecting that it completes the request in bdrv_driver_pwritev().
However, if the second request actually yielded before leaving the
coroutine, the reused request coroutine is in an entirely different
place and is reentered prematurely. Not a good idea.

Let's make sure that we exit the coroutine after completing the first
request by resuming the next allocating request only with a bottom
half.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 8d899fd..a837a28 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -967,6 +967,11 @@ static void qed_aio_complete_bh(void *opaque)
     qed_release(s);
 }
 
+static void qed_resume_alloc_bh(void *opaque)
+{
+    qed_aio_start_io(opaque);
+}
+
 static void qed_aio_complete(QEDAIOCB *acb, int ret)
 {
     BDRVQEDState *s = acb_to_s(acb);
@@ -995,10 +1000,12 @@ static void qed_aio_complete(QEDAIOCB *acb, int ret)
      * requests multiple times but rather finish one at a time completely.
      */
     if (acb == QSIMPLEQ_FIRST(&s->allocating_write_reqs)) {
+        QEDAIOCB *next_acb;
         QSIMPLEQ_REMOVE_HEAD(&s->allocating_write_reqs, next);
-        acb = QSIMPLEQ_FIRST(&s->allocating_write_reqs);
-        if (acb) {
-            qed_aio_start_io(acb);
+        next_acb = QSIMPLEQ_FIRST(&s->allocating_write_reqs);
+        if (next_acb) {
+            aio_bh_schedule_oneshot(bdrv_get_aio_context(acb->common.bs),
+                                    qed_resume_alloc_bh, next_acb);
         } else if (s->header.features & QED_F_NEED_CHECK) {
             qed_start_need_check_timer(s);
         }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 01/29] qed: Use bottom half to resume waiting requests

[Eric Blake]

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

On 05/26/2017 03:21 PM, Kevin Wolf wrote:
> The qed driver serialises allocating write requests. When the active
> allocation is finished, the AIO callback is called, but after this, the
> next allocating request is immediately processed instead of leaving the
> coroutine. Resuming another allocation request in the same request
> coroutine means that the request now runs in the wrong coroutine.
> 
> The following is one of the possible effects of this: The completed
> request will generally reenter its request coroutine in a bottom half,
> expecting that it completes the request in bdrv_driver_pwritev().
> However, if the second request actually yielded before leaving the
> coroutine, the reused request coroutine is in an entirely different
> place and is reentered prematurely. Not a good idea.
> 
> Let's make sure that we exit the coroutine after completing the first
> request by resuming the next allocating request only with a bottom
> half.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [Qemu-block] [PATCH 01/29] qed: Use bottom half to resume waiting requests

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:21:42PM +0200, Kevin Wolf wrote:
> The qed driver serialises allocating write requests. When the active
> allocation is finished, the AIO callback is called, but after this, the
> next allocating request is immediately processed instead of leaving the
> coroutine. Resuming another allocation request in the same request
> coroutine means that the request now runs in the wrong coroutine.
> 
> The following is one of the possible effects of this: The completed
> request will generally reenter its request coroutine in a bottom half,
> expecting that it completes the request in bdrv_driver_pwritev().
> However, if the second request actually yielded before leaving the
> coroutine, the reused request coroutine is in an entirely different
> place and is reentered prematurely. Not a good idea.
> 
> Let's make sure that we exit the coroutine after completing the first
> request by resuming the next allocating request only with a bottom
> half.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 02/29] qed: Make qed_read_table() synchronous

[Kevin Wolf]

Note that this code is generally not running in coroutine context, so
this is an actual blocking synchronous operation. We'll fix this in a
moment.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed-table.c | 56 ++++++++++++++++++-------------------------------------
 1 file changed, 18 insertions(+), 38 deletions(-)

diff --git a/block/qed-table.c b/block/qed-table.c
index b12c298..f330538 100644
--- a/block/qed-table.c
+++ b/block/qed-table.c
@@ -18,59 +18,39 @@
 #include "qed.h"
 #include "qemu/bswap.h"
 
-typedef struct {
-    GenericCB gencb;
-    BDRVQEDState *s;
-    QEDTable *table;
-
-    struct iovec iov;
+static void qed_read_table(BDRVQEDState *s, uint64_t offset, QEDTable *table,
+                           BlockCompletionFunc *cb, void *opaque)
+{
     QEMUIOVector qiov;
-} QEDReadTableCB;
+    int noffsets;
+    int i, ret;
 
-static void qed_read_table_cb(void *opaque, int ret)
-{
-    QEDReadTableCB *read_table_cb = opaque;
-    QEDTable *table = read_table_cb->table;
-    BDRVQEDState *s = read_table_cb->s;
-    int noffsets = read_table_cb->qiov.size / sizeof(uint64_t);
-    int i;
+    struct iovec iov = {
+        .iov_base = table->offsets,
+        .iov_len = s->header.cluster_size * s->header.table_size,
+    };
+    qemu_iovec_init_external(&qiov, &iov, 1);
 
-    /* Handle I/O error */
-    if (ret) {
+    trace_qed_read_table(s, offset, table);
+
+    ret = bdrv_preadv(s->bs->file, offset, &qiov);
+    if (ret < 0) {
         goto out;
     }
 
     /* Byteswap offsets */
     qed_acquire(s);
+    noffsets = qiov.size / sizeof(uint64_t);
     for (i = 0; i < noffsets; i++) {
         table->offsets[i] = le64_to_cpu(table->offsets[i]);
     }
     qed_release(s);
 
+    ret = 0;
 out:
     /* Completion */
-    trace_qed_read_table_cb(s, read_table_cb->table, ret);
-    gencb_complete(&read_table_cb->gencb, ret);
-}
-
-static void qed_read_table(BDRVQEDState *s, uint64_t offset, QEDTable *table,
-                           BlockCompletionFunc *cb, void *opaque)
-{
-    QEDReadTableCB *read_table_cb = gencb_alloc(sizeof(*read_table_cb),
-                                                cb, opaque);
-    QEMUIOVector *qiov = &read_table_cb->qiov;
-
-    trace_qed_read_table(s, offset, table);
-
-    read_table_cb->s = s;
-    read_table_cb->table = table;
-    read_table_cb->iov.iov_base = table->offsets,
-    read_table_cb->iov.iov_len = s->header.cluster_size * s->header.table_size,
-
-    qemu_iovec_init_external(qiov, &read_table_cb->iov, 1);
-    bdrv_aio_readv(s->bs->file, offset / BDRV_SECTOR_SIZE, qiov,
-                   qiov->size / BDRV_SECTOR_SIZE,
-                   qed_read_table_cb, read_table_cb);
+    trace_qed_read_table_cb(s, table, ret);
+    cb(opaque, ret);
 }
 
 typedef struct {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 02/29] qed: Make qed_read_table() synchronous

[Eric Blake]

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

On 05/26/2017 03:21 PM, Kevin Wolf wrote:
> Note that this code is generally not running in coroutine context, so
> this is an actual blocking synchronous operation. We'll fix this in a
> moment.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed-table.c | 56 ++++++++++++++++++-------------------------------------
>  1 file changed, 18 insertions(+), 38 deletions(-)
> 
> diff --git a/block/qed-table.c b/block/qed-table.c
> index b12c298..f330538 100644
> --- a/block/qed-table.c
> +++ b/block/qed-table.c
> @@ -18,59 +18,39 @@
>  #include "qed.h"
>  #include "qemu/bswap.h"

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [Qemu-block] [PATCH 02/29] qed: Make qed_read_table() synchronous

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:21:43PM +0200, Kevin Wolf wrote:
> Note that this code is generally not running in coroutine context, so
> this is an actual blocking synchronous operation. We'll fix this in a
> moment.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed-table.c | 56 ++++++++++++++++++-------------------------------------
>  1 file changed, 18 insertions(+), 38 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 03/29] qed: Remove callback from qed_read_table()

[Kevin Wolf]

Instead of passing the return value to a callback, return it to the
caller so that the callback can be inlined there.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed-table.c | 79 ++++++++++++++++++-------------------------------------
 1 file changed, 25 insertions(+), 54 deletions(-)

diff --git a/block/qed-table.c b/block/qed-table.c
index f330538..4270003 100644
--- a/block/qed-table.c
+++ b/block/qed-table.c
@@ -18,8 +18,7 @@
 #include "qed.h"
 #include "qemu/bswap.h"
 
-static void qed_read_table(BDRVQEDState *s, uint64_t offset, QEDTable *table,
-                           BlockCompletionFunc *cb, void *opaque)
+static int qed_read_table(BDRVQEDState *s, uint64_t offset, QEDTable *table)
 {
     QEMUIOVector qiov;
     int noffsets;
@@ -50,7 +49,7 @@ static void qed_read_table(BDRVQEDState *s, uint64_t offset, QEDTable *table,
 out:
     /* Completion */
     trace_qed_read_table_cb(s, table, ret);
-    cb(opaque, ret);
+    return ret;
 }
 
 typedef struct {
@@ -156,13 +155,7 @@ static void qed_sync_cb(void *opaque, int ret)
 
 int qed_read_l1_table_sync(BDRVQEDState *s)
 {
-    int ret = -EINPROGRESS;
-
-    qed_read_table(s, s->header.l1_table_offset,
-                   s->l1_table, qed_sync_cb, &ret);
-    BDRV_POLL_WHILE(s->bs, ret == -EINPROGRESS);
-
-    return ret;
+    return qed_read_table(s, s->header.l1_table_offset, s->l1_table);
 }
 
 void qed_write_l1_table(BDRVQEDState *s, unsigned int index, unsigned int n,
@@ -184,46 +177,10 @@ int qed_write_l1_table_sync(BDRVQEDState *s, unsigned int index,
     return ret;
 }
 
-typedef struct {
-    GenericCB gencb;
-    BDRVQEDState *s;
-    uint64_t l2_offset;
-    QEDRequest *request;
-} QEDReadL2TableCB;
-
-static void qed_read_l2_table_cb(void *opaque, int ret)
-{
-    QEDReadL2TableCB *read_l2_table_cb = opaque;
-    QEDRequest *request = read_l2_table_cb->request;
-    BDRVQEDState *s = read_l2_table_cb->s;
-    CachedL2Table *l2_table = request->l2_table;
-    uint64_t l2_offset = read_l2_table_cb->l2_offset;
-
-    qed_acquire(s);
-    if (ret) {
-        /* can't trust loaded L2 table anymore */
-        qed_unref_l2_cache_entry(l2_table);
-        request->l2_table = NULL;
-    } else {
-        l2_table->offset = l2_offset;
-
-        qed_commit_l2_cache_entry(&s->l2_cache, l2_table);
-
-        /* This is guaranteed to succeed because we just committed the entry
-         * to the cache.
-         */
-        request->l2_table = qed_find_l2_cache_entry(&s->l2_cache, l2_offset);
-        assert(request->l2_table != NULL);
-    }
-    qed_release(s);
-
-    gencb_complete(&read_l2_table_cb->gencb, ret);
-}
-
 void qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset,
                        BlockCompletionFunc *cb, void *opaque)
 {
-    QEDReadL2TableCB *read_l2_table_cb;
+    int ret;
 
     qed_unref_l2_cache_entry(request->l2_table);
 
@@ -237,14 +194,28 @@ void qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset,
     request->l2_table = qed_alloc_l2_cache_entry(&s->l2_cache);
     request->l2_table->table = qed_alloc_table(s);
 
-    read_l2_table_cb = gencb_alloc(sizeof(*read_l2_table_cb), cb, opaque);
-    read_l2_table_cb->s = s;
-    read_l2_table_cb->l2_offset = offset;
-    read_l2_table_cb->request = request;
-
     BLKDBG_EVENT(s->bs->file, BLKDBG_L2_LOAD);
-    qed_read_table(s, offset, request->l2_table->table,
-                   qed_read_l2_table_cb, read_l2_table_cb);
+    ret = qed_read_table(s, offset, request->l2_table->table);
+
+    qed_acquire(s);
+    if (ret) {
+        /* can't trust loaded L2 table anymore */
+        qed_unref_l2_cache_entry(request->l2_table);
+        request->l2_table = NULL;
+    } else {
+        request->l2_table->offset = offset;
+
+        qed_commit_l2_cache_entry(&s->l2_cache, request->l2_table);
+
+        /* This is guaranteed to succeed because we just committed the entry
+         * to the cache.
+         */
+        request->l2_table = qed_find_l2_cache_entry(&s->l2_cache, offset);
+        assert(request->l2_table != NULL);
+    }
+    qed_release(s);
+
+    cb(opaque, ret);
 }
 
 int qed_read_l2_table_sync(BDRVQEDState *s, QEDRequest *request, uint64_t offset)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 03/29] qed: Remove callback from qed_read_table()

[Eric Blake]

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

On 05/26/2017 03:21 PM, Kevin Wolf wrote:
> Instead of passing the return value to a callback, return it to the
> caller so that the callback can be inlined there.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed-table.c | 79 ++++++++++++++++++-------------------------------------
>  1 file changed, 25 insertions(+), 54 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH 03/29] qed: Remove callback from qed_read_table()

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:21:44PM +0200, Kevin Wolf wrote:
> Instead of passing the return value to a callback, return it to the
> caller so that the callback can be inlined there.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed-table.c | 79 ++++++++++++++++++-------------------------------------
>  1 file changed, 25 insertions(+), 54 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 04/29] qed: Remove callback from qed_read_l2_table()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed-cluster.c | 94 ++++++++++++++++++-----------------------------------
 block/qed-table.c   | 15 +++------
 block/qed.h         |  3 +-
 3 files changed, 36 insertions(+), 76 deletions(-)

diff --git a/block/qed-cluster.c b/block/qed-cluster.c
index 8f5da74..d279944 100644
--- a/block/qed-cluster.c
+++ b/block/qed-cluster.c
@@ -61,59 +61,6 @@ static unsigned int qed_count_contiguous_clusters(BDRVQEDState *s,
     return i - index;
 }
 
-typedef struct {
-    BDRVQEDState *s;
-    uint64_t pos;
-    size_t len;
-
-    QEDRequest *request;
-
-    /* User callback */
-    QEDFindClusterFunc *cb;
-    void *opaque;
-} QEDFindClusterCB;
-
-static void qed_find_cluster_cb(void *opaque, int ret)
-{
-    QEDFindClusterCB *find_cluster_cb = opaque;
-    BDRVQEDState *s = find_cluster_cb->s;
-    QEDRequest *request = find_cluster_cb->request;
-    uint64_t offset = 0;
-    size_t len = 0;
-    unsigned int index;
-    unsigned int n;
-
-    qed_acquire(s);
-    if (ret) {
-        goto out;
-    }
-
-    index = qed_l2_index(s, find_cluster_cb->pos);
-    n = qed_bytes_to_clusters(s,
-                              qed_offset_into_cluster(s, find_cluster_cb->pos) +
-                              find_cluster_cb->len);
-    n = qed_count_contiguous_clusters(s, request->l2_table->table,
-                                      index, n, &offset);
-
-    if (qed_offset_is_unalloc_cluster(offset)) {
-        ret = QED_CLUSTER_L2;
-    } else if (qed_offset_is_zero_cluster(offset)) {
-        ret = QED_CLUSTER_ZERO;
-    } else if (qed_check_cluster_offset(s, offset)) {
-        ret = QED_CLUSTER_FOUND;
-    } else {
-        ret = -EINVAL;
-    }
-
-    len = MIN(find_cluster_cb->len, n * s->header.cluster_size -
-              qed_offset_into_cluster(s, find_cluster_cb->pos));
-
-out:
-    find_cluster_cb->cb(find_cluster_cb->opaque, ret, offset, len);
-    qed_release(s);
-    g_free(find_cluster_cb);
-}
-
 /**
  * Find the offset of a data cluster
  *
@@ -137,8 +84,11 @@ out:
 void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
                       size_t len, QEDFindClusterFunc *cb, void *opaque)
 {
-    QEDFindClusterCB *find_cluster_cb;
     uint64_t l2_offset;
+    uint64_t offset = 0;
+    unsigned int index;
+    unsigned int n;
+    int ret;
 
     /* Limit length to L2 boundary.  Requests are broken up at the L2 boundary
      * so that a request acts on one L2 table at a time.
@@ -155,14 +105,32 @@ void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
         return;
     }
 
-    find_cluster_cb = g_malloc(sizeof(*find_cluster_cb));
-    find_cluster_cb->s = s;
-    find_cluster_cb->pos = pos;
-    find_cluster_cb->len = len;
-    find_cluster_cb->cb = cb;
-    find_cluster_cb->opaque = opaque;
-    find_cluster_cb->request = request;
+    ret = qed_read_l2_table(s, request, l2_offset);
+    qed_acquire(s);
+    if (ret) {
+        goto out;
+    }
+
+    index = qed_l2_index(s, pos);
+    n = qed_bytes_to_clusters(s,
+                              qed_offset_into_cluster(s, pos) + len);
+    n = qed_count_contiguous_clusters(s, request->l2_table->table,
+                                      index, n, &offset);
+
+    if (qed_offset_is_unalloc_cluster(offset)) {
+        ret = QED_CLUSTER_L2;
+    } else if (qed_offset_is_zero_cluster(offset)) {
+        ret = QED_CLUSTER_ZERO;
+    } else if (qed_check_cluster_offset(s, offset)) {
+        ret = QED_CLUSTER_FOUND;
+    } else {
+        ret = -EINVAL;
+    }
+
+    len = MIN(len,
+              n * s->header.cluster_size - qed_offset_into_cluster(s, pos));
 
-    qed_read_l2_table(s, request, l2_offset,
-                      qed_find_cluster_cb, find_cluster_cb);
+out:
+    cb(opaque, ret, offset, len);
+    qed_release(s);
 }
diff --git a/block/qed-table.c b/block/qed-table.c
index 4270003..ffecbea 100644
--- a/block/qed-table.c
+++ b/block/qed-table.c
@@ -177,8 +177,7 @@ int qed_write_l1_table_sync(BDRVQEDState *s, unsigned int index,
     return ret;
 }
 
-void qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset,
-                       BlockCompletionFunc *cb, void *opaque)
+int qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset)
 {
     int ret;
 
@@ -187,8 +186,7 @@ void qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset,
     /* Check for cached L2 entry */
     request->l2_table = qed_find_l2_cache_entry(&s->l2_cache, offset);
     if (request->l2_table) {
-        cb(opaque, 0);
-        return;
+        return 0;
     }
 
     request->l2_table = qed_alloc_l2_cache_entry(&s->l2_cache);
@@ -215,17 +213,12 @@ void qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset,
     }
     qed_release(s);
 
-    cb(opaque, ret);
+    return ret;
 }
 
 int qed_read_l2_table_sync(BDRVQEDState *s, QEDRequest *request, uint64_t offset)
 {
-    int ret = -EINPROGRESS;
-
-    qed_read_l2_table(s, request, offset, qed_sync_cb, &ret);
-    BDRV_POLL_WHILE(s->bs, ret == -EINPROGRESS);
-
-    return ret;
+    return qed_read_l2_table(s, request, offset);
 }
 
 void qed_write_l2_table(BDRVQEDState *s, QEDRequest *request,
diff --git a/block/qed.h b/block/qed.h
index ce8c314..c715058 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -237,8 +237,7 @@ int qed_write_l1_table_sync(BDRVQEDState *s, unsigned int index,
                             unsigned int n);
 int qed_read_l2_table_sync(BDRVQEDState *s, QEDRequest *request,
                            uint64_t offset);
-void qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset,
-                       BlockCompletionFunc *cb, void *opaque);
+int qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset);
 void qed_write_l2_table(BDRVQEDState *s, QEDRequest *request,
                         unsigned int index, unsigned int n, bool flush,
                         BlockCompletionFunc *cb, void *opaque);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 04/29] qed: Remove callback from qed_read_l2_table()

[Eric Blake]

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

On 05/26/2017 03:21 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed-cluster.c | 94 ++++++++++++++++++-----------------------------------
>  block/qed-table.c   | 15 +++------
>  block/qed.h         |  3 +-
>  3 files changed, 36 insertions(+), 76 deletions(-)

The diffstats show how opaque the code was; the cleanups are a lot
easier to read.

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [Qemu-block] [PATCH 04/29] qed: Remove callback from qed_read_l2_table()

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:21:45PM +0200, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed-cluster.c | 94 ++++++++++++++++++-----------------------------------
>  block/qed-table.c   | 15 +++------
>  block/qed.h         |  3 +-
>  3 files changed, 36 insertions(+), 76 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 05/29] qed: Remove callback from qed_find_cluster()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed-cluster.c | 39 ++++++++++++++++++++++-----------------
 block/qed.c         | 20 +++++++++++---------
 block/qed.h         |  4 ++--
 3 files changed, 35 insertions(+), 28 deletions(-)

diff --git a/block/qed-cluster.c b/block/qed-cluster.c
index d279944..88dc979 100644
--- a/block/qed-cluster.c
+++ b/block/qed-cluster.c
@@ -67,22 +67,27 @@ static unsigned int qed_count_contiguous_clusters(BDRVQEDState *s,
  * @s:          QED state
  * @request:    L2 cache entry
  * @pos:        Byte position in device
- * @len:        Number of bytes
- * @cb:         Completion function
- * @opaque:     User data for completion function
+ * @len:        Number of bytes (may be shortened on return)
+ * @img_offset: Contains offset in the image file on success
  *
  * This function translates a position in the block device to an offset in the
- * image file.  It invokes the cb completion callback to report back the
- * translated offset or unallocated range in the image file.
+ * image file. The translated offset or unallocated range in the image file is
+ * reported back in *img_offset and *len.
  *
  * If the L2 table exists, request->l2_table points to the L2 table cache entry
  * and the caller must free the reference when they are finished.  The cache
  * entry is exposed in this way to avoid callers having to read the L2 table
  * again later during request processing.  If request->l2_table is non-NULL it
  * will be unreferenced before taking on the new cache entry.
+ *
+ * On success QED_CLUSTER_FOUND is returned and img_offset/len are a contiguous
+ * range in the image file.
+ *
+ * On failure QED_CLUSTER_L2 or QED_CLUSTER_L1 is returned for missing L2 or L1
+ * table offset, respectively. len is number of contiguous unallocated bytes.
  */
-void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
-                      size_t len, QEDFindClusterFunc *cb, void *opaque)
+int qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
+                     size_t *len, uint64_t *img_offset)
 {
     uint64_t l2_offset;
     uint64_t offset = 0;
@@ -93,16 +98,16 @@ void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
     /* Limit length to L2 boundary.  Requests are broken up at the L2 boundary
      * so that a request acts on one L2 table at a time.
      */
-    len = MIN(len, (((pos >> s->l1_shift) + 1) << s->l1_shift) - pos);
+    *len = MIN(*len, (((pos >> s->l1_shift) + 1) << s->l1_shift) - pos);
 
     l2_offset = s->l1_table->offsets[qed_l1_index(s, pos)];
     if (qed_offset_is_unalloc_cluster(l2_offset)) {
-        cb(opaque, QED_CLUSTER_L1, 0, len);
-        return;
+        *img_offset = 0;
+        return QED_CLUSTER_L1;
     }
     if (!qed_check_table_offset(s, l2_offset)) {
-        cb(opaque, -EINVAL, 0, 0);
-        return;
+        *img_offset = *len = 0;
+        return -EINVAL;
     }
 
     ret = qed_read_l2_table(s, request, l2_offset);
@@ -112,8 +117,7 @@ void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
     }
 
     index = qed_l2_index(s, pos);
-    n = qed_bytes_to_clusters(s,
-                              qed_offset_into_cluster(s, pos) + len);
+    n = qed_bytes_to_clusters(s, qed_offset_into_cluster(s, pos) + *len);
     n = qed_count_contiguous_clusters(s, request->l2_table->table,
                                       index, n, &offset);
 
@@ -127,10 +131,11 @@ void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
         ret = -EINVAL;
     }
 
-    len = MIN(len,
-              n * s->header.cluster_size - qed_offset_into_cluster(s, pos));
+    *len = MIN(*len,
+               n * s->header.cluster_size - qed_offset_into_cluster(s, pos));
 
 out:
-    cb(opaque, ret, offset, len);
+    *img_offset = offset;
     qed_release(s);
+    return ret;
 }
diff --git a/block/qed.c b/block/qed.c
index a837a28..031bb0a 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -776,14 +776,14 @@ static int64_t coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState *bs,
         .file = file,
     };
     QEDRequest request = { .l2_table = NULL };
+    uint64_t offset;
+    int ret;
 
-    qed_find_cluster(s, &request, cb.pos, len, qed_is_allocated_cb, &cb);
+    ret = qed_find_cluster(s, &request, cb.pos, &len, &offset);
+    qed_is_allocated_cb(&cb, ret, offset, len);
 
-    /* Now sleep if the callback wasn't invoked immediately */
-    while (cb.status == BDRV_BLOCK_OFFSET_MASK) {
-        cb.co = qemu_coroutine_self();
-        qemu_coroutine_yield();
-    }
+    /* The callback was invoked immediately */
+    assert(cb.status != BDRV_BLOCK_OFFSET_MASK);
 
     qed_unref_l2_cache_entry(request.l2_table);
 
@@ -1393,6 +1393,8 @@ static void qed_aio_next_io(QEDAIOCB *acb, int ret)
     BDRVQEDState *s = acb_to_s(acb);
     QEDFindClusterFunc *io_fn = (acb->flags & QED_AIOCB_WRITE) ?
                                 qed_aio_write_data : qed_aio_read_data;
+    uint64_t offset;
+    size_t len;
 
     trace_qed_aio_next_io(s, acb, ret, acb->cur_pos + acb->cur_qiov.size);
 
@@ -1419,9 +1421,9 @@ static void qed_aio_next_io(QEDAIOCB *acb, int ret)
     }
 
     /* Find next cluster and start I/O */
-    qed_find_cluster(s, &acb->request,
-                      acb->cur_pos, acb->end_pos - acb->cur_pos,
-                      io_fn, acb);
+    len = acb->end_pos - acb->cur_pos;
+    ret = qed_find_cluster(s, &acb->request, acb->cur_pos, &len, &offset);
+    io_fn(acb, ret, offset, len);
 }
 
 static BlockAIOCB *qed_aio_setup(BlockDriverState *bs,
diff --git a/block/qed.h b/block/qed.h
index c715058..6ab5702 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -247,8 +247,8 @@ int qed_write_l2_table_sync(BDRVQEDState *s, QEDRequest *request,
 /**
  * Cluster functions
  */
-void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
-                      size_t len, QEDFindClusterFunc *cb, void *opaque);
+int qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
+                     size_t *len, uint64_t *img_offset);
 
 /**
  * Consistency check
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 05/29] qed: Remove callback from qed_find_cluster()

[Eric Blake]

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

On 05/26/2017 03:21 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed-cluster.c | 39 ++++++++++++++++++++++-----------------
>  block/qed.c         | 20 +++++++++++---------
>  block/qed.h         |  4 ++--
>  3 files changed, 35 insertions(+), 28 deletions(-)
> 

This diffstat is not as slick as the earlier patches, but the cleanup is
still worthwhile.

> @@ -93,16 +98,16 @@ void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
>      /* Limit length to L2 boundary.  Requests are broken up at the L2 boundary
>       * so that a request acts on one L2 table at a time.
>       */
> -    len = MIN(len, (((pos >> s->l1_shift) + 1) << s->l1_shift) - pos);
> +    *len = MIN(*len, (((pos >> s->l1_shift) + 1) << s->l1_shift) - pos);

Perhaps a separate cleanup, but wouldn't this be easier to read as:
*len = MIN(*len, QEMU_ALIGN_UP(pos, 1ULL << s->l1_shift) - pos);

(or if there is a direct variable for l1 size, rather than '1ULL <<
s->l1_shift')

> +++ b/block/qed.h
> @@ -247,8 +247,8 @@ int qed_write_l2_table_sync(BDRVQEDState *s, QEDRequest *request,
>  /**
>   * Cluster functions
>   */
> -void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
> -                      size_t len, QEDFindClusterFunc *cb, void *opaque);
> +int qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
> +                     size_t *len, uint64_t *img_offset);

While we're touching this, would it be better to switch to an
always-64-bit type, rather than using platform-dependent size_t for len?
 But even so, that cleanup would be a separate patch.

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [Qemu-block] [PATCH 05/29] qed: Remove callback from qed_find_cluster()

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:21:46PM +0200, Kevin Wolf wrote:
> diff --git a/block/qed.c b/block/qed.c
> index a837a28..031bb0a 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -776,14 +776,14 @@ static int64_t coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState *bs,
>          .file = file,
>      };
>      QEDRequest request = { .l2_table = NULL };
> +    uint64_t offset;
> +    int ret;
>  
> -    qed_find_cluster(s, &request, cb.pos, len, qed_is_allocated_cb, &cb);
> +    ret = qed_find_cluster(s, &request, cb.pos, &len, &offset);
> +    qed_is_allocated_cb(&cb, ret, offset, len);

qed_is_allocated_cb() used to be called within qed_acquire/release().
Now it's not?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 06/29] qed: Make qed_read_backing_file() synchronous

[Kevin Wolf]

Note that this code is generally not running in coroutine context, so
this is an actual blocking synchronous operation. We'll fix this in a
moment.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 031bb0a..507e051 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -808,13 +808,13 @@ static BDRVQEDState *acb_to_s(QEDAIOCB *acb)
  * This function reads qiov->size bytes starting at pos from the backing file.
  * If there is no backing file then zeroes are read.
  */
-static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
-                                  QEMUIOVector *qiov,
-                                  QEMUIOVector **backing_qiov,
-                                  BlockCompletionFunc *cb, void *opaque)
+static int qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
+                                 QEMUIOVector *qiov,
+                                 QEMUIOVector **backing_qiov)
 {
     uint64_t backing_length = 0;
     size_t size;
+    int ret;
 
     /* If there is a backing file, get its length.  Treat the absence of a
      * backing file like a zero length backing file.
@@ -822,8 +822,7 @@ static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
     if (s->bs->backing) {
         int64_t l = bdrv_getlength(s->bs->backing->bs);
         if (l < 0) {
-            cb(opaque, l);
-            return;
+            return l;
         }
         backing_length = l;
     }
@@ -836,8 +835,7 @@ static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
 
     /* Complete now if there are no backing file sectors to read */
     if (pos >= backing_length) {
-        cb(opaque, 0);
-        return;
+        return 0;
     }
 
     /* If the read straddles the end of the backing file, shorten it */
@@ -849,8 +847,11 @@ static void qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
     qemu_iovec_concat(*backing_qiov, qiov, 0, size);
 
     BLKDBG_EVENT(s->bs->file, BLKDBG_READ_BACKING_AIO);
-    bdrv_aio_readv(s->bs->backing, pos / BDRV_SECTOR_SIZE,
-                   *backing_qiov, size / BDRV_SECTOR_SIZE, cb, opaque);
+    ret = bdrv_preadv(s->bs->backing, pos, *backing_qiov);
+    if (ret < 0) {
+        return ret;
+    }
+    return 0;
 }
 
 typedef struct {
@@ -907,6 +908,7 @@ static void qed_copy_from_backing_file(BDRVQEDState *s, uint64_t pos,
                                        void *opaque)
 {
     CopyFromBackingFileCB *copy_cb;
+    int ret;
 
     /* Skip copy entirely if there is no work to do */
     if (len == 0) {
@@ -922,8 +924,9 @@ static void qed_copy_from_backing_file(BDRVQEDState *s, uint64_t pos,
     copy_cb->iov.iov_len = len;
     qemu_iovec_init_external(&copy_cb->qiov, &copy_cb->iov, 1);
 
-    qed_read_backing_file(s, pos, &copy_cb->qiov, &copy_cb->backing_qiov,
-                          qed_copy_from_backing_file_write, copy_cb);
+    ret = qed_read_backing_file(s, pos, &copy_cb->qiov,
+                                &copy_cb->backing_qiov);
+    qed_copy_from_backing_file_write(copy_cb, ret);
 }
 
 /**
@@ -1370,8 +1373,9 @@ static void qed_aio_read_data(void *opaque, int ret,
         qed_aio_start_io(acb);
         return;
     } else if (ret != QED_CLUSTER_FOUND) {
-        qed_read_backing_file(s, acb->cur_pos, &acb->cur_qiov,
-                              &acb->backing_qiov, qed_aio_next_io_cb, acb);
+        ret = qed_read_backing_file(s, acb->cur_pos, &acb->cur_qiov,
+                                    &acb->backing_qiov);
+        qed_aio_next_io(acb, ret);
         return;
     }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 06/29] qed: Make qed_read_backing_file() synchronous

[Eric Blake]

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

On 05/26/2017 03:21 PM, Kevin Wolf wrote:
> Note that this code is generally not running in coroutine context, so
> this is an actual blocking synchronous operation. We'll fix this in a
> moment.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH 06/29] qed: Make qed_read_backing_file() synchronous

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:21:47PM +0200, Kevin Wolf wrote:
> Note that this code is generally not running in coroutine context, so
> this is an actual blocking synchronous operation. We'll fix this in a
> moment.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 07/29] qed: Make qed_copy_from_backing_file() synchronous

[Kevin Wolf]

Note that this code is generally not running in coroutine context, so
this is an actual blocking synchronous operation. We'll fix this in a
moment.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 78 +++++++++++++++++++++++--------------------------------------
 1 file changed, 29 insertions(+), 49 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 507e051..584a5ba 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -854,44 +854,6 @@ static int qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
     return 0;
 }
 
-typedef struct {
-    GenericCB gencb;
-    BDRVQEDState *s;
-    QEMUIOVector qiov;
-    QEMUIOVector *backing_qiov;
-    struct iovec iov;
-    uint64_t offset;
-} CopyFromBackingFileCB;
-
-static void qed_copy_from_backing_file_cb(void *opaque, int ret)
-{
-    CopyFromBackingFileCB *copy_cb = opaque;
-    qemu_vfree(copy_cb->iov.iov_base);
-    gencb_complete(&copy_cb->gencb, ret);
-}
-
-static void qed_copy_from_backing_file_write(void *opaque, int ret)
-{
-    CopyFromBackingFileCB *copy_cb = opaque;
-    BDRVQEDState *s = copy_cb->s;
-
-    if (copy_cb->backing_qiov) {
-        qemu_iovec_destroy(copy_cb->backing_qiov);
-        g_free(copy_cb->backing_qiov);
-        copy_cb->backing_qiov = NULL;
-    }
-
-    if (ret) {
-        qed_copy_from_backing_file_cb(copy_cb, ret);
-        return;
-    }
-
-    BLKDBG_EVENT(s->bs->file, BLKDBG_COW_WRITE);
-    bdrv_aio_writev(s->bs->file, copy_cb->offset / BDRV_SECTOR_SIZE,
-                    &copy_cb->qiov, copy_cb->qiov.size / BDRV_SECTOR_SIZE,
-                    qed_copy_from_backing_file_cb, copy_cb);
-}
-
 /**
  * Copy data from backing file into the image
  *
@@ -907,7 +869,9 @@ static void qed_copy_from_backing_file(BDRVQEDState *s, uint64_t pos,
                                        BlockCompletionFunc *cb,
                                        void *opaque)
 {
-    CopyFromBackingFileCB *copy_cb;
+    QEMUIOVector qiov;
+    QEMUIOVector *backing_qiov = NULL;
+    struct iovec iov;
     int ret;
 
     /* Skip copy entirely if there is no work to do */
@@ -916,17 +880,33 @@ static void qed_copy_from_backing_file(BDRVQEDState *s, uint64_t pos,
         return;
     }
 
-    copy_cb = gencb_alloc(sizeof(*copy_cb), cb, opaque);
-    copy_cb->s = s;
-    copy_cb->offset = offset;
-    copy_cb->backing_qiov = NULL;
-    copy_cb->iov.iov_base = qemu_blockalign(s->bs, len);
-    copy_cb->iov.iov_len = len;
-    qemu_iovec_init_external(&copy_cb->qiov, &copy_cb->iov, 1);
+    iov = (struct iovec) {
+        .iov_base = qemu_blockalign(s->bs, len),
+        .iov_len = len,
+    };
+    qemu_iovec_init_external(&qiov, &iov, 1);
+
+    ret = qed_read_backing_file(s, pos, &qiov, &backing_qiov);
+
+    if (backing_qiov) {
+        qemu_iovec_destroy(backing_qiov);
+        g_free(backing_qiov);
+        backing_qiov = NULL;
+    }
+
+    if (ret) {
+        goto out;
+    }
 
-    ret = qed_read_backing_file(s, pos, &copy_cb->qiov,
-                                &copy_cb->backing_qiov);
-    qed_copy_from_backing_file_write(copy_cb, ret);
+    BLKDBG_EVENT(s->bs->file, BLKDBG_COW_WRITE);
+    ret = bdrv_pwritev(s->bs->file, offset, &qiov);
+    if (ret < 0) {
+        goto out;
+    }
+    ret = 0;
+out:
+    qemu_vfree(iov.iov_base);
+    cb(opaque, ret);
 }
 
 /**
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 07/29] qed: Make qed_copy_from_backing_file() synchronous

[Eric Blake]

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

On 05/26/2017 03:21 PM, Kevin Wolf wrote:
> Note that this code is generally not running in coroutine context, so
> this is an actual blocking synchronous operation. We'll fix this in a
> moment.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 78 +++++++++++++++++++++++--------------------------------------
>  1 file changed, 29 insertions(+), 49 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [Qemu-block] [PATCH 07/29] qed: Make qed_copy_from_backing_file() synchronous

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:21:48PM +0200, Kevin Wolf wrote:
> Note that this code is generally not running in coroutine context, so
> this is an actual blocking synchronous operation. We'll fix this in a
> moment.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 78 +++++++++++++++++++++++--------------------------------------
>  1 file changed, 29 insertions(+), 49 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 08/29] qed: Remove callback from qed_copy_from_backing_file()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 57 +++++++++++++++++++++++----------------------------------
 1 file changed, 23 insertions(+), 34 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 584a5ba..61ef732 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -861,13 +861,9 @@ static int qed_read_backing_file(BDRVQEDState *s, uint64_t pos,
  * @pos:        Byte position in device
  * @len:        Number of bytes
  * @offset:     Byte offset in image file
- * @cb:         Completion function
- * @opaque:     User data for completion function
  */
-static void qed_copy_from_backing_file(BDRVQEDState *s, uint64_t pos,
-                                       uint64_t len, uint64_t offset,
-                                       BlockCompletionFunc *cb,
-                                       void *opaque)
+static int qed_copy_from_backing_file(BDRVQEDState *s, uint64_t pos,
+                                      uint64_t len, uint64_t offset)
 {
     QEMUIOVector qiov;
     QEMUIOVector *backing_qiov = NULL;
@@ -876,8 +872,7 @@ static void qed_copy_from_backing_file(BDRVQEDState *s, uint64_t pos,
 
     /* Skip copy entirely if there is no work to do */
     if (len == 0) {
-        cb(opaque, 0);
-        return;
+        return 0;
     }
 
     iov = (struct iovec) {
@@ -906,7 +901,7 @@ static void qed_copy_from_backing_file(BDRVQEDState *s, uint64_t pos,
     ret = 0;
 out:
     qemu_vfree(iov.iov_base);
-    cb(opaque, ret);
+    return ret;
 }
 
 /**
@@ -1133,42 +1128,36 @@ static void qed_aio_write_main(void *opaque, int ret)
 }
 
 /**
- * Populate back untouched region of new data cluster
+ * Populate untouched regions of new data cluster
  */
-static void qed_aio_write_postfill(void *opaque, int ret)
+static void qed_aio_write_cow(void *opaque, int ret)
 {
     QEDAIOCB *acb = opaque;
     BDRVQEDState *s = acb_to_s(acb);
-    uint64_t start = acb->cur_pos + acb->cur_qiov.size;
-    uint64_t len =
-        qed_start_of_cluster(s, start + s->header.cluster_size - 1) - start;
-    uint64_t offset = acb->cur_cluster +
-                      qed_offset_into_cluster(s, acb->cur_pos) +
-                      acb->cur_qiov.size;
+    uint64_t start, len, offset;
+
+    /* Populate front untouched region of new data cluster */
+    start = qed_start_of_cluster(s, acb->cur_pos);
+    len = qed_offset_into_cluster(s, acb->cur_pos);
 
+    trace_qed_aio_write_prefill(s, acb, start, len, acb->cur_cluster);
+    ret = qed_copy_from_backing_file(s, start, len, acb->cur_cluster);
     if (ret) {
         qed_aio_complete(acb, ret);
         return;
     }
 
-    trace_qed_aio_write_postfill(s, acb, start, len, offset);
-    qed_copy_from_backing_file(s, start, len, offset,
-                                qed_aio_write_main, acb);
-}
+    /* Populate back untouched region of new data cluster */
+    start = acb->cur_pos + acb->cur_qiov.size;
+    len = qed_start_of_cluster(s, start + s->header.cluster_size - 1) - start;
+    offset = acb->cur_cluster +
+             qed_offset_into_cluster(s, acb->cur_pos) +
+             acb->cur_qiov.size;
 
-/**
- * Populate front untouched region of new data cluster
- */
-static void qed_aio_write_prefill(void *opaque, int ret)
-{
-    QEDAIOCB *acb = opaque;
-    BDRVQEDState *s = acb_to_s(acb);
-    uint64_t start = qed_start_of_cluster(s, acb->cur_pos);
-    uint64_t len = qed_offset_into_cluster(s, acb->cur_pos);
+    trace_qed_aio_write_postfill(s, acb, start, len, offset);
+    ret = qed_copy_from_backing_file(s, start, len, offset);
 
-    trace_qed_aio_write_prefill(s, acb, start, len, acb->cur_cluster);
-    qed_copy_from_backing_file(s, start, len, acb->cur_cluster,
-                                qed_aio_write_postfill, acb);
+    qed_aio_write_main(acb, ret);
 }
 
 /**
@@ -1236,7 +1225,7 @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len)
 
         cb = qed_aio_write_zero_cluster;
     } else {
-        cb = qed_aio_write_prefill;
+        cb = qed_aio_write_cow;
         acb->cur_cluster = qed_alloc_clusters(s, acb->cur_nclusters);
     }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 08/29] qed: Remove callback from qed_copy_from_backing_file()

[Eric Blake]

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

On 05/26/2017 03:21 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 57 +++++++++++++++++++++++----------------------------------
>  1 file changed, 23 insertions(+), 34 deletions(-)
> 

>  /**
> - * Populate back untouched region of new data cluster
> + * Populate untouched regions of new data cluster
>   */
> -static void qed_aio_write_postfill(void *opaque, int ret)
> +static void qed_aio_write_cow(void *opaque, int ret)
>  {

It may be worth a mention in the commit message that you are renaming
things to use more common terminology in the process (in part because
you reduced a chained callback into one: qed_aio_write_alloc called
qed_aio_write_prefill called qed_aio_write_postfill).  Up to you.

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [Qemu-block] [PATCH 08/29] qed: Remove callback from qed_copy_from_backing_file()

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:21:49PM +0200, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 57 +++++++++++++++++++++++----------------------------------
>  1 file changed, 23 insertions(+), 34 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 09/29] qed: Make qed_write_header() synchronous

[Kevin Wolf]

Note that this code is generally not running in coroutine context, so
this is an actual blocking synchronous operation. We'll fix this in a
moment.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 76 +++++++++++++++++++++++--------------------------------------
 1 file changed, 29 insertions(+), 47 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 61ef732..5548475 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -92,41 +92,6 @@ int qed_write_header_sync(BDRVQEDState *s)
     return 0;
 }
 
-typedef struct {
-    GenericCB gencb;
-    BDRVQEDState *s;
-    struct iovec iov;
-    QEMUIOVector qiov;
-    int nsectors;
-    uint8_t *buf;
-} QEDWriteHeaderCB;
-
-static void qed_write_header_cb(void *opaque, int ret)
-{
-    QEDWriteHeaderCB *write_header_cb = opaque;
-
-    qemu_vfree(write_header_cb->buf);
-    gencb_complete(write_header_cb, ret);
-}
-
-static void qed_write_header_read_cb(void *opaque, int ret)
-{
-    QEDWriteHeaderCB *write_header_cb = opaque;
-    BDRVQEDState *s = write_header_cb->s;
-
-    if (ret) {
-        qed_write_header_cb(write_header_cb, ret);
-        return;
-    }
-
-    /* Update header */
-    qed_header_cpu_to_le(&s->header, (QEDHeader *)write_header_cb->buf);
-
-    bdrv_aio_writev(s->bs->file, 0, &write_header_cb->qiov,
-                    write_header_cb->nsectors, qed_write_header_cb,
-                    write_header_cb);
-}
-
 /**
  * Update header in-place (does not rewrite backing filename or other strings)
  *
@@ -144,18 +109,35 @@ static void qed_write_header(BDRVQEDState *s, BlockCompletionFunc cb,
 
     int nsectors = DIV_ROUND_UP(sizeof(QEDHeader), BDRV_SECTOR_SIZE);
     size_t len = nsectors * BDRV_SECTOR_SIZE;
-    QEDWriteHeaderCB *write_header_cb = gencb_alloc(sizeof(*write_header_cb),
-                                                    cb, opaque);
-
-    write_header_cb->s = s;
-    write_header_cb->nsectors = nsectors;
-    write_header_cb->buf = qemu_blockalign(s->bs, len);
-    write_header_cb->iov.iov_base = write_header_cb->buf;
-    write_header_cb->iov.iov_len = len;
-    qemu_iovec_init_external(&write_header_cb->qiov, &write_header_cb->iov, 1);
-
-    bdrv_aio_readv(s->bs->file, 0, &write_header_cb->qiov, nsectors,
-                   qed_write_header_read_cb, write_header_cb);
+    uint8_t *buf;
+    struct iovec iov;
+    QEMUIOVector qiov;
+    int ret;
+
+    buf = qemu_blockalign(s->bs, len);
+    iov = (struct iovec) {
+        .iov_base = buf,
+        .iov_len = len,
+    };
+    qemu_iovec_init_external(&qiov, &iov, 1);
+
+    ret = bdrv_preadv(s->bs->file, 0, &qiov);
+    if (ret < 0) {
+        goto out;
+    }
+
+    /* Update header */
+    qed_header_cpu_to_le(&s->header, (QEDHeader *) buf);
+
+    ret = bdrv_pwritev(s->bs->file, 0, &qiov);
+    if (ret < 0) {
+        goto out;
+    }
+
+    ret = 0;
+out:
+    qemu_vfree(buf);
+    cb(opaque, ret);
 }
 
 static uint64_t qed_max_image_size(uint32_t cluster_size, uint32_t table_size)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 09/29] qed: Make qed_write_header() synchronous

[Eric Blake]

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

On 05/26/2017 03:21 PM, Kevin Wolf wrote:
> Note that this code is generally not running in coroutine context, so
> this is an actual blocking synchronous operation. We'll fix this in a
> moment.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 76 +++++++++++++++++++++++--------------------------------------
>  1 file changed, 29 insertions(+), 47 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [Qemu-block] [PATCH 09/29] qed: Make qed_write_header() synchronous

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:21:50PM +0200, Kevin Wolf wrote:
> Note that this code is generally not running in coroutine context, so
> this is an actual blocking synchronous operation. We'll fix this in a
> moment.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 76 +++++++++++++++++++++++--------------------------------------
>  1 file changed, 29 insertions(+), 47 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 10/29] qed: Remove callback from qed_write_header()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 36 ++++++++++++++----------------------
 1 file changed, 14 insertions(+), 22 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 5548475..134c98a 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -98,8 +98,7 @@ int qed_write_header_sync(BDRVQEDState *s)
  * This function only updates known header fields in-place and does not affect
  * extra data after the QED header.
  */
-static void qed_write_header(BDRVQEDState *s, BlockCompletionFunc cb,
-                             void *opaque)
+static int qed_write_header(BDRVQEDState *s)
 {
     /* We must write full sectors for O_DIRECT but cannot necessarily generate
      * the data following the header if an unrecognized compat feature is
@@ -137,7 +136,7 @@ static void qed_write_header(BDRVQEDState *s, BlockCompletionFunc cb,
     ret = 0;
 out:
     qemu_vfree(buf);
-    cb(opaque, ret);
+    return ret;
 }
 
 static uint64_t qed_max_image_size(uint32_t cluster_size, uint32_t table_size)
@@ -289,32 +288,23 @@ static void qed_unplug_allocating_write_reqs(BDRVQEDState *s)
     }
 }
 
-static void qed_finish_clear_need_check(void *opaque, int ret)
-{
-    /* Do nothing */
-}
-
-static void qed_flush_after_clear_need_check(void *opaque, int ret)
-{
-    BDRVQEDState *s = opaque;
-
-    bdrv_aio_flush(s->bs, qed_finish_clear_need_check, s);
-
-    /* No need to wait until flush completes */
-    qed_unplug_allocating_write_reqs(s);
-}
-
 static void qed_clear_need_check(void *opaque, int ret)
 {
     BDRVQEDState *s = opaque;
 
     if (ret) {
-        qed_unplug_allocating_write_reqs(s);
-        return;
+        goto out;
     }
 
     s->header.features &= ~QED_F_NEED_CHECK;
-    qed_write_header(s, qed_flush_after_clear_need_check, s);
+    ret = qed_write_header(s);
+    (void) ret;
+
+    ret = bdrv_flush(s->bs);
+    (void) ret;
+
+out:
+    qed_unplug_allocating_write_reqs(s);
 }
 
 static void qed_need_check_timer_cb(void *opaque)
@@ -1179,6 +1169,7 @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len)
 {
     BDRVQEDState *s = acb_to_s(acb);
     BlockCompletionFunc *cb;
+    int ret;
 
     /* Cancel timer when the first allocating request comes in */
     if (QSIMPLEQ_EMPTY(&s->allocating_write_reqs)) {
@@ -1213,7 +1204,8 @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len)
 
     if (qed_should_set_need_check(s)) {
         s->header.features |= QED_F_NEED_CHECK;
-        qed_write_header(s, cb, acb);
+        ret = qed_write_header(s);
+        cb(acb, ret);
     } else {
         cb(acb, 0);
     }
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-block] [PATCH 10/29] qed: Remove callback from qed_write_header()

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:21:51PM +0200, Kevin Wolf wrote:
>  static void qed_clear_need_check(void *opaque, int ret)
>  {
>      BDRVQEDState *s = opaque;
>  
>      if (ret) {
> -        qed_unplug_allocating_write_reqs(s);
> -        return;
> +        goto out;
>      }
>  
>      s->header.features &= ~QED_F_NEED_CHECK;
> -    qed_write_header(s, qed_flush_after_clear_need_check, s);
> +    ret = qed_write_header(s);
> +    (void) ret;
> +
> +    ret = bdrv_flush(s->bs);
> +    (void) ret;
> +
> +out:
> +    qed_unplug_allocating_write_reqs(s);
>  }

Should we unplug allocating write reqs before flushing?  The async code
kicks off a flush but doesn't wait for it to complete.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 10/29] qed: Remove callback from qed_write_header()

[Kevin Wolf]

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

Am 31.05.2017 um 14:32 hat Stefan Hajnoczi geschrieben:
> On Fri, May 26, 2017 at 10:21:51PM +0200, Kevin Wolf wrote:
> >  static void qed_clear_need_check(void *opaque, int ret)
> >  {
> >      BDRVQEDState *s = opaque;
> >  
> >      if (ret) {
> > -        qed_unplug_allocating_write_reqs(s);
> > -        return;
> > +        goto out;
> >      }
> >  
> >      s->header.features &= ~QED_F_NEED_CHECK;
> > -    qed_write_header(s, qed_flush_after_clear_need_check, s);
> > +    ret = qed_write_header(s);
> > +    (void) ret;
> > +
> > +    ret = bdrv_flush(s->bs);
> > +    (void) ret;
> > +
> > +out:
> > +    qed_unplug_allocating_write_reqs(s);
> >  }
> 
> Should we unplug allocating write reqs before flushing?  The async code
> kicks off a flush but doesn't wait for it to complete.

You're right that moving it up would match the old code. Not sure if it
would make much of a difference, though, isn't the request queue drained
in the kernel anyway while doing a flush? But if you prefer, I can
change it.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 10/29] qed: Remove callback from qed_write_header()

[Paolo Bonzini]

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



On 01/06/2017 17:59, Kevin Wolf wrote:
> Am 31.05.2017 um 14:32 hat Stefan Hajnoczi geschrieben:
>> On Fri, May 26, 2017 at 10:21:51PM +0200, Kevin Wolf wrote:
>>>  static void qed_clear_need_check(void *opaque, int ret)
>>>  {
>>>      BDRVQEDState *s = opaque;
>>>  
>>>      if (ret) {
>>> -        qed_unplug_allocating_write_reqs(s);
>>> -        return;
>>> +        goto out;
>>>      }
>>>  
>>>      s->header.features &= ~QED_F_NEED_CHECK;
>>> -    qed_write_header(s, qed_flush_after_clear_need_check, s);
>>> +    ret = qed_write_header(s);
>>> +    (void) ret;
>>> +
>>> +    ret = bdrv_flush(s->bs);
>>> +    (void) ret;
>>> +
>>> +out:
>>> +    qed_unplug_allocating_write_reqs(s);
>>>  }
>>
>> Should we unplug allocating write reqs before flushing?  The async code
>> kicks off a flush but doesn't wait for it to complete.
> 
> You're right that moving it up would match the old code. Not sure if it
> would make much of a difference, though, isn't the request queue drained
> in the kernel anyway while doing a flush?

No, it isn't AFAIK.

Paolo


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

Re: [Qemu-devel] [Qemu-block] [PATCH 10/29] qed: Remove callback from qed_write_header()

[Stefan Hajnoczi]

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

On Thu, Jun 01, 2017 at 06:04:25PM +0200, Paolo Bonzini wrote:
> 
> 
> On 01/06/2017 17:59, Kevin Wolf wrote:
> > Am 31.05.2017 um 14:32 hat Stefan Hajnoczi geschrieben:
> >> On Fri, May 26, 2017 at 10:21:51PM +0200, Kevin Wolf wrote:
> >>>  static void qed_clear_need_check(void *opaque, int ret)
> >>>  {
> >>>      BDRVQEDState *s = opaque;
> >>>  
> >>>      if (ret) {
> >>> -        qed_unplug_allocating_write_reqs(s);
> >>> -        return;
> >>> +        goto out;
> >>>      }
> >>>  
> >>>      s->header.features &= ~QED_F_NEED_CHECK;
> >>> -    qed_write_header(s, qed_flush_after_clear_need_check, s);
> >>> +    ret = qed_write_header(s);
> >>> +    (void) ret;
> >>> +
> >>> +    ret = bdrv_flush(s->bs);
> >>> +    (void) ret;
> >>> +
> >>> +out:
> >>> +    qed_unplug_allocating_write_reqs(s);
> >>>  }
> >>
> >> Should we unplug allocating write reqs before flushing?  The async code
> >> kicks off a flush but doesn't wait for it to complete.
> > 
> > You're right that moving it up would match the old code. Not sure if it
> > would make much of a difference, though, isn't the request queue drained
> > in the kernel anyway while doing a flush?
> 
> No, it isn't AFAIK.

I was also under the impression that applications must call fsync()
only after their write() calls have completed.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 10/29] qed: Remove callback from qed_write_header()

[Stefan Hajnoczi]

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

On Thu, Jun 01, 2017 at 05:59:53PM +0200, Kevin Wolf wrote:
> Am 31.05.2017 um 14:32 hat Stefan Hajnoczi geschrieben:
> > On Fri, May 26, 2017 at 10:21:51PM +0200, Kevin Wolf wrote:
> > >  static void qed_clear_need_check(void *opaque, int ret)
> > >  {
> > >      BDRVQEDState *s = opaque;
> > >  
> > >      if (ret) {
> > > -        qed_unplug_allocating_write_reqs(s);
> > > -        return;
> > > +        goto out;
> > >      }
> > >  
> > >      s->header.features &= ~QED_F_NEED_CHECK;
> > > -    qed_write_header(s, qed_flush_after_clear_need_check, s);
> > > +    ret = qed_write_header(s);
> > > +    (void) ret;
> > > +
> > > +    ret = bdrv_flush(s->bs);
> > > +    (void) ret;
> > > +
> > > +out:
> > > +    qed_unplug_allocating_write_reqs(s);
> > >  }
> > 
> > Should we unplug allocating write reqs before flushing?  The async code
> > kicks off a flush but doesn't wait for it to complete.
> 
> You're right that moving it up would match the old code. Not sure if it
> would make much of a difference, though, isn't the request queue drained
> in the kernel anyway while doing a flush? But if you prefer, I can
> change it.

I prefer keeping the behavior of the existing code where possible.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 11/29] qed: Make qed_write_table() synchronous

[Kevin Wolf]

Note that this code is generally not running in coroutine context, so
this is an actual blocking synchronous operation. We'll fix this in a
moment.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed-table.c | 84 ++++++++++++++++++++-----------------------------------
 1 file changed, 30 insertions(+), 54 deletions(-)

diff --git a/block/qed-table.c b/block/qed-table.c
index ffecbea..0cc93a7 100644
--- a/block/qed-table.c
+++ b/block/qed-table.c
@@ -52,46 +52,6 @@ out:
     return ret;
 }
 
-typedef struct {
-    GenericCB gencb;
-    BDRVQEDState *s;
-    QEDTable *orig_table;
-    QEDTable *table;
-    bool flush;             /* flush after write? */
-
-    struct iovec iov;
-    QEMUIOVector qiov;
-} QEDWriteTableCB;
-
-static void qed_write_table_cb(void *opaque, int ret)
-{
-    QEDWriteTableCB *write_table_cb = opaque;
-    BDRVQEDState *s = write_table_cb->s;
-
-    trace_qed_write_table_cb(s,
-                             write_table_cb->orig_table,
-                             write_table_cb->flush,
-                             ret);
-
-    if (ret) {
-        goto out;
-    }
-
-    if (write_table_cb->flush) {
-        /* We still need to flush first */
-        write_table_cb->flush = false;
-        qed_acquire(s);
-        bdrv_aio_flush(write_table_cb->s->bs, qed_write_table_cb,
-                       write_table_cb);
-        qed_release(s);
-        return;
-    }
-
-out:
-    qemu_vfree(write_table_cb->table);
-    gencb_complete(&write_table_cb->gencb, ret);
-}
-
 /**
  * Write out an updated part or all of a table
  *
@@ -108,10 +68,13 @@ static void qed_write_table(BDRVQEDState *s, uint64_t offset, QEDTable *table,
                             unsigned int index, unsigned int n, bool flush,
                             BlockCompletionFunc *cb, void *opaque)
 {
-    QEDWriteTableCB *write_table_cb;
     unsigned int sector_mask = BDRV_SECTOR_SIZE / sizeof(uint64_t) - 1;
     unsigned int start, end, i;
+    QEDTable *new_table;
+    struct iovec iov;
+    QEMUIOVector qiov;
     size_t len_bytes;
+    int ret;
 
     trace_qed_write_table(s, offset, table, index, n);
 
@@ -121,28 +84,41 @@ static void qed_write_table(BDRVQEDState *s, uint64_t offset, QEDTable *table,
 
     len_bytes = (end - start) * sizeof(uint64_t);
 
-    write_table_cb = gencb_alloc(sizeof(*write_table_cb), cb, opaque);
-    write_table_cb->s = s;
-    write_table_cb->orig_table = table;
-    write_table_cb->flush = flush;
-    write_table_cb->table = qemu_blockalign(s->bs, len_bytes);
-    write_table_cb->iov.iov_base = write_table_cb->table->offsets;
-    write_table_cb->iov.iov_len = len_bytes;
-    qemu_iovec_init_external(&write_table_cb->qiov, &write_table_cb->iov, 1);
+    new_table = qemu_blockalign(s->bs, len_bytes);
+    iov = (struct iovec) {
+        .iov_base = new_table->offsets,
+        .iov_len = len_bytes,
+    };
+    qemu_iovec_init_external(&qiov, &iov, 1);
 
     /* Byteswap table */
     for (i = start; i < end; i++) {
         uint64_t le_offset = cpu_to_le64(table->offsets[i]);
-        write_table_cb->table->offsets[i - start] = le_offset;
+        new_table->offsets[i - start] = le_offset;
     }
 
     /* Adjust for offset into table */
     offset += start * sizeof(uint64_t);
 
-    bdrv_aio_writev(s->bs->file, offset / BDRV_SECTOR_SIZE,
-                    &write_table_cb->qiov,
-                    write_table_cb->qiov.size / BDRV_SECTOR_SIZE,
-                    qed_write_table_cb, write_table_cb);
+    ret = bdrv_pwritev(s->bs->file, offset, &qiov);
+    trace_qed_write_table_cb(s, table, flush, ret);
+    if (ret < 0) {
+        goto out;
+    }
+
+    if (flush) {
+        qed_acquire(s);
+        ret = bdrv_flush(s->bs);
+        qed_release(s);
+        if (ret < 0) {
+            goto out;
+        }
+    }
+
+    ret = 0;
+out:
+    qemu_vfree(new_table);
+    cb(opaque, ret);
 }
 
 /**
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-block] [PATCH 11/29] qed: Make qed_write_table() synchronous

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:21:52PM +0200, Kevin Wolf wrote:
> Note that this code is generally not running in coroutine context, so
> this is an actual blocking synchronous operation. We'll fix this in a
> moment.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed-table.c | 84 ++++++++++++++++++++-----------------------------------
>  1 file changed, 30 insertions(+), 54 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 12/29] qed: Remove GenericCB

[Kevin Wolf]

The GenericCB infrastructure isn't used any more. Remove it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/Makefile.objs |  2 +-
 block/qed-gencb.c   | 33 ---------------------------------
 block/qed.h         | 11 -----------
 3 files changed, 1 insertion(+), 45 deletions(-)
 delete mode 100644 block/qed-gencb.c

diff --git a/block/Makefile.objs b/block/Makefile.objs
index ea95530..f9368b5 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -1,6 +1,6 @@
 block-obj-y += raw-format.o qcow.o vdi.o vmdk.o cloop.o bochs.o vpc.o vvfat.o dmg.o
 block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o
-block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
+block-obj-y += qed.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-obj-y += qed-check.o
 block-obj-y += vhdx.o vhdx-endian.o vhdx-log.o
 block-obj-y += quorum.o
diff --git a/block/qed-gencb.c b/block/qed-gencb.c
deleted file mode 100644
index faf8ecc..0000000
--- a/block/qed-gencb.c
+++ /dev/null
@@ -1,33 +0,0 @@
-/*
- * QEMU Enhanced Disk Format
- *
- * Copyright IBM, Corp. 2010
- *
- * Authors:
- *  Stefan Hajnoczi   <stefanha@linux.vnet.ibm.com>
- *
- * This work is licensed under the terms of the GNU LGPL, version 2 or later.
- * See the COPYING.LIB file in the top-level directory.
- *
- */
-
-#include "qemu/osdep.h"
-#include "qed.h"
-
-void *gencb_alloc(size_t len, BlockCompletionFunc *cb, void *opaque)
-{
-    GenericCB *gencb = g_malloc(len);
-    gencb->cb = cb;
-    gencb->opaque = opaque;
-    return gencb;
-}
-
-void gencb_complete(void *opaque, int ret)
-{
-    GenericCB *gencb = opaque;
-    BlockCompletionFunc *cb = gencb->cb;
-    void *user_opaque = gencb->opaque;
-
-    g_free(gencb);
-    cb(user_opaque, ret);
-}
diff --git a/block/qed.h b/block/qed.h
index 6ab5702..46843c4 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -202,17 +202,6 @@ void qed_acquire(BDRVQEDState *s);
 void qed_release(BDRVQEDState *s);
 
 /**
- * Generic callback for chaining async callbacks
- */
-typedef struct {
-    BlockCompletionFunc *cb;
-    void *opaque;
-} GenericCB;
-
-void *gencb_alloc(size_t len, BlockCompletionFunc *cb, void *opaque);
-void gencb_complete(void *opaque, int ret);
-
-/**
  * Header functions
  */
 int qed_write_header_sync(BDRVQEDState *s);
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-block] [PATCH 12/29] qed: Remove GenericCB

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:21:53PM +0200, Kevin Wolf wrote:
> The GenericCB infrastructure isn't used any more. Remove it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/Makefile.objs |  2 +-
>  block/qed-gencb.c   | 33 ---------------------------------
>  block/qed.h         | 11 -----------
>  3 files changed, 1 insertion(+), 45 deletions(-)
>  delete mode 100644 block/qed-gencb.c

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 13/29] qed: Remove callback from qed_write_table()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed-table.c | 47 ++++++++++++-----------------------------------
 block/qed.c       | 12 +++++++-----
 block/qed.h       |  8 +++-----
 3 files changed, 22 insertions(+), 45 deletions(-)

diff --git a/block/qed-table.c b/block/qed-table.c
index 0cc93a7..ebee2c5 100644
--- a/block/qed-table.c
+++ b/block/qed-table.c
@@ -61,12 +61,9 @@ out:
  * @index:      Index of first element
  * @n:          Number of elements
  * @flush:      Whether or not to sync to disk
- * @cb:         Completion function
- * @opaque:     Argument for completion function
  */
-static void qed_write_table(BDRVQEDState *s, uint64_t offset, QEDTable *table,
-                            unsigned int index, unsigned int n, bool flush,
-                            BlockCompletionFunc *cb, void *opaque)
+static int qed_write_table(BDRVQEDState *s, uint64_t offset, QEDTable *table,
+                           unsigned int index, unsigned int n, bool flush)
 {
     unsigned int sector_mask = BDRV_SECTOR_SIZE / sizeof(uint64_t) - 1;
     unsigned int start, end, i;
@@ -118,15 +115,7 @@ static void qed_write_table(BDRVQEDState *s, uint64_t offset, QEDTable *table,
     ret = 0;
 out:
     qemu_vfree(new_table);
-    cb(opaque, ret);
-}
-
-/**
- * Propagate return value from async callback
- */
-static void qed_sync_cb(void *opaque, int ret)
-{
-    *(int *)opaque = ret;
+    return ret;
 }
 
 int qed_read_l1_table_sync(BDRVQEDState *s)
@@ -134,23 +123,17 @@ int qed_read_l1_table_sync(BDRVQEDState *s)
     return qed_read_table(s, s->header.l1_table_offset, s->l1_table);
 }
 
-void qed_write_l1_table(BDRVQEDState *s, unsigned int index, unsigned int n,
-                        BlockCompletionFunc *cb, void *opaque)
+int qed_write_l1_table(BDRVQEDState *s, unsigned int index, unsigned int n)
 {
     BLKDBG_EVENT(s->bs->file, BLKDBG_L1_UPDATE);
-    qed_write_table(s, s->header.l1_table_offset,
-                    s->l1_table, index, n, false, cb, opaque);
+    return qed_write_table(s, s->header.l1_table_offset,
+                           s->l1_table, index, n, false);
 }
 
 int qed_write_l1_table_sync(BDRVQEDState *s, unsigned int index,
                             unsigned int n)
 {
-    int ret = -EINPROGRESS;
-
-    qed_write_l1_table(s, index, n, qed_sync_cb, &ret);
-    BDRV_POLL_WHILE(s->bs, ret == -EINPROGRESS);
-
-    return ret;
+    return qed_write_l1_table(s, index, n);
 }
 
 int qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset)
@@ -197,22 +180,16 @@ int qed_read_l2_table_sync(BDRVQEDState *s, QEDRequest *request, uint64_t offset
     return qed_read_l2_table(s, request, offset);
 }
 
-void qed_write_l2_table(BDRVQEDState *s, QEDRequest *request,
-                        unsigned int index, unsigned int n, bool flush,
-                        BlockCompletionFunc *cb, void *opaque)
+int qed_write_l2_table(BDRVQEDState *s, QEDRequest *request,
+                       unsigned int index, unsigned int n, bool flush)
 {
     BLKDBG_EVENT(s->bs->file, BLKDBG_L2_UPDATE);
-    qed_write_table(s, request->l2_table->offset,
-                    request->l2_table->table, index, n, flush, cb, opaque);
+    return qed_write_table(s, request->l2_table->offset,
+                           request->l2_table->table, index, n, flush);
 }
 
 int qed_write_l2_table_sync(BDRVQEDState *s, QEDRequest *request,
                             unsigned int index, unsigned int n, bool flush)
 {
-    int ret = -EINPROGRESS;
-
-    qed_write_l2_table(s, request, index, n, flush, qed_sync_cb, &ret);
-    BDRV_POLL_WHILE(s->bs, ret == -EINPROGRESS);
-
-    return ret;
+    return qed_write_l2_table(s, request, index, n, flush);
 }
diff --git a/block/qed.c b/block/qed.c
index 134c98a..e9417d0 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1000,7 +1000,8 @@ static void qed_aio_write_l1_update(void *opaque, int ret)
     index = qed_l1_index(s, acb->cur_pos);
     s->l1_table->offsets[index] = acb->request.l2_table->offset;
 
-    qed_write_l1_table(s, index, 1, qed_commit_l2_update, acb);
+    ret = qed_write_l1_table(s, index, 1);
+    qed_commit_l2_update(acb, ret);
 }
 
 /**
@@ -1027,12 +1028,13 @@ static void qed_aio_write_l2_update(QEDAIOCB *acb, int ret, uint64_t offset)
 
     if (need_alloc) {
         /* Write out the whole new L2 table */
-        qed_write_l2_table(s, &acb->request, 0, s->table_nelems, true,
-                           qed_aio_write_l1_update, acb);
+        ret = qed_write_l2_table(s, &acb->request, 0, s->table_nelems, true);
+        qed_aio_write_l1_update(acb, ret);
     } else {
         /* Write out only the updated part of the L2 table */
-        qed_write_l2_table(s, &acb->request, index, acb->cur_nclusters, false,
-                           qed_aio_next_io_cb, acb);
+        ret = qed_write_l2_table(s, &acb->request, index, acb->cur_nclusters,
+                                 false);
+        qed_aio_next_io(acb, ret);
     }
     return;
 
diff --git a/block/qed.h b/block/qed.h
index 46843c4..51443fa 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -220,16 +220,14 @@ void qed_commit_l2_cache_entry(L2TableCache *l2_cache, CachedL2Table *l2_table);
  * Table I/O functions
  */
 int qed_read_l1_table_sync(BDRVQEDState *s);
-void qed_write_l1_table(BDRVQEDState *s, unsigned int index, unsigned int n,
-                        BlockCompletionFunc *cb, void *opaque);
+int qed_write_l1_table(BDRVQEDState *s, unsigned int index, unsigned int n);
 int qed_write_l1_table_sync(BDRVQEDState *s, unsigned int index,
                             unsigned int n);
 int qed_read_l2_table_sync(BDRVQEDState *s, QEDRequest *request,
                            uint64_t offset);
 int qed_read_l2_table(BDRVQEDState *s, QEDRequest *request, uint64_t offset);
-void qed_write_l2_table(BDRVQEDState *s, QEDRequest *request,
-                        unsigned int index, unsigned int n, bool flush,
-                        BlockCompletionFunc *cb, void *opaque);
+int qed_write_l2_table(BDRVQEDState *s, QEDRequest *request,
+                       unsigned int index, unsigned int n, bool flush);
 int qed_write_l2_table_sync(BDRVQEDState *s, QEDRequest *request,
                             unsigned int index, unsigned int n, bool flush);
 
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-block] [PATCH 13/29] qed: Remove callback from qed_write_table()

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:21:54PM +0200, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed-table.c | 47 ++++++++++++-----------------------------------
>  block/qed.c       | 12 +++++++-----
>  block/qed.h       |  8 +++-----
>  3 files changed, 22 insertions(+), 45 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 14/29] qed: Make qed_aio_read_data() synchronous

[Kevin Wolf]

Note that this code is generally not running in coroutine context, so
this is an actual blocking synchronous operation. We'll fix this in a
moment.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index e9417d0..0972936 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1325,9 +1325,11 @@ static void qed_aio_read_data(void *opaque, int ret,
     }
 
     BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
-    bdrv_aio_readv(bs->file, offset / BDRV_SECTOR_SIZE,
-                   &acb->cur_qiov, acb->cur_qiov.size / BDRV_SECTOR_SIZE,
-                   qed_aio_next_io_cb, acb);
+    ret = bdrv_preadv(bs->file, offset, &acb->cur_qiov);
+    if (ret < 0) {
+        goto err;
+    }
+    qed_aio_next_io(acb, 0);
     return;
 
 err:
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-block] [PATCH 14/29] qed: Make qed_aio_read_data() synchronous

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:21:55PM +0200, Kevin Wolf wrote:
> Note that this code is generally not running in coroutine context, so
> this is an actual blocking synchronous operation. We'll fix this in a
> moment.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 15/29] qed: Make qed_aio_write_main() synchronous

[Kevin Wolf]

Note that this code is generally not running in coroutine context, so
this is an actual blocking synchronous operation. We'll fix this in a
moment.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 61 +++++++++++++++++++------------------------------------------
 1 file changed, 19 insertions(+), 42 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 0972936..a596c4d 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -260,13 +260,6 @@ static void qed_aio_start_io(QEDAIOCB *acb)
     qed_aio_next_io(acb, 0);
 }
 
-static void qed_aio_next_io_cb(void *opaque, int ret)
-{
-    QEDAIOCB *acb = opaque;
-
-    qed_aio_next_io(acb, ret);
-}
-
 static void qed_plug_allocating_write_reqs(BDRVQEDState *s)
 {
     assert(!s->allocating_write_reqs_plugged);
@@ -1042,31 +1035,6 @@ err:
     qed_aio_complete(acb, ret);
 }
 
-static void qed_aio_write_l2_update_cb(void *opaque, int ret)
-{
-    QEDAIOCB *acb = opaque;
-    qed_aio_write_l2_update(acb, ret, acb->cur_cluster);
-}
-
-/**
- * Flush new data clusters before updating the L2 table
- *
- * This flush is necessary when a backing file is in use.  A crash during an
- * allocating write could result in empty clusters in the image.  If the write
- * only touched a subregion of the cluster, then backing image sectors have
- * been lost in the untouched region.  The solution is to flush after writing a
- * new data cluster and before updating the L2 table.
- */
-static void qed_aio_write_flush_before_l2_update(void *opaque, int ret)
-{
-    QEDAIOCB *acb = opaque;
-    BDRVQEDState *s = acb_to_s(acb);
-
-    if (!bdrv_aio_flush(s->bs->file->bs, qed_aio_write_l2_update_cb, opaque)) {
-        qed_aio_complete(acb, -EIO);
-    }
-}
-
 /**
  * Write data to the image file
  */
@@ -1076,7 +1044,6 @@ static void qed_aio_write_main(void *opaque, int ret)
     BDRVQEDState *s = acb_to_s(acb);
     uint64_t offset = acb->cur_cluster +
                       qed_offset_into_cluster(s, acb->cur_pos);
-    BlockCompletionFunc *next_fn;
 
     trace_qed_aio_write_main(s, acb, ret, offset, acb->cur_qiov.size);
 
@@ -1085,20 +1052,30 @@ static void qed_aio_write_main(void *opaque, int ret)
         return;
     }
 
+    BLKDBG_EVENT(s->bs->file, BLKDBG_WRITE_AIO);
+    ret = bdrv_pwritev(s->bs->file, offset, &acb->cur_qiov);
+    if (ret >= 0) {
+        ret = 0;
+    }
+
     if (acb->find_cluster_ret == QED_CLUSTER_FOUND) {
-        next_fn = qed_aio_next_io_cb;
+        qed_aio_next_io(acb, ret);
     } else {
         if (s->bs->backing) {
-            next_fn = qed_aio_write_flush_before_l2_update;
-        } else {
-            next_fn = qed_aio_write_l2_update_cb;
+            /*
+             * Flush new data clusters before updating the L2 table
+             *
+             * This flush is necessary when a backing file is in use.  A crash
+             * during an allocating write could result in empty clusters in the
+             * image.  If the write only touched a subregion of the cluster,
+             * then backing image sectors have been lost in the untouched
+             * region.  The solution is to flush after writing a new data
+             * cluster and before updating the L2 table.
+             */
+            ret = bdrv_flush(s->bs->file->bs);
         }
+        qed_aio_write_l2_update(acb, ret, acb->cur_cluster);
     }
-
-    BLKDBG_EVENT(s->bs->file, BLKDBG_WRITE_AIO);
-    bdrv_aio_writev(s->bs->file, offset / BDRV_SECTOR_SIZE,
-                    &acb->cur_qiov, acb->cur_qiov.size / BDRV_SECTOR_SIZE,
-                    next_fn, acb);
 }
 
 /**
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-block] [PATCH 15/29] qed: Make qed_aio_write_main() synchronous

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:21:56PM +0200, Kevin Wolf wrote:
> Note that this code is generally not running in coroutine context, so
> this is an actual blocking synchronous operation. We'll fix this in a
> moment.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 61 +++++++++++++++++++------------------------------------------
>  1 file changed, 19 insertions(+), 42 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 16/29] qed: Inline qed_commit_l2_update()

[Kevin Wolf]

qed_commit_l2_update() is unconditionally called at the end of
qed_aio_write_l1_update(). Inline it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 36 ++++++++++++++----------------------
 1 file changed, 14 insertions(+), 22 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index a596c4d..3b1cce4 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -956,15 +956,27 @@ static void qed_aio_complete(QEDAIOCB *acb, int ret)
 }
 
 /**
- * Commit the current L2 table to the cache
+ * Update L1 table with new L2 table offset and write it out
  */
-static void qed_commit_l2_update(void *opaque, int ret)
+static void qed_aio_write_l1_update(void *opaque, int ret)
 {
     QEDAIOCB *acb = opaque;
     BDRVQEDState *s = acb_to_s(acb);
     CachedL2Table *l2_table = acb->request.l2_table;
     uint64_t l2_offset = l2_table->offset;
+    int index;
+
+    if (ret) {
+        qed_aio_complete(acb, ret);
+        return;
+    }
 
+    index = qed_l1_index(s, acb->cur_pos);
+    s->l1_table->offsets[index] = l2_table->offset;
+
+    ret = qed_write_l1_table(s, index, 1);
+
+    /* Commit the current L2 table to the cache */
     qed_commit_l2_cache_entry(&s->l2_cache, l2_table);
 
     /* This is guaranteed to succeed because we just committed the entry to the
@@ -976,26 +988,6 @@ static void qed_commit_l2_update(void *opaque, int ret)
     qed_aio_next_io(acb, ret);
 }
 
-/**
- * Update L1 table with new L2 table offset and write it out
- */
-static void qed_aio_write_l1_update(void *opaque, int ret)
-{
-    QEDAIOCB *acb = opaque;
-    BDRVQEDState *s = acb_to_s(acb);
-    int index;
-
-    if (ret) {
-        qed_aio_complete(acb, ret);
-        return;
-    }
-
-    index = qed_l1_index(s, acb->cur_pos);
-    s->l1_table->offsets[index] = acb->request.l2_table->offset;
-
-    ret = qed_write_l1_table(s, index, 1);
-    qed_commit_l2_update(acb, ret);
-}
 
 /**
  * Update L2 table with new cluster offsets and write them out
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 16/29] qed: Inline qed_commit_l2_update()

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:21:57PM +0200, Kevin Wolf wrote:
> qed_commit_l2_update() is unconditionally called at the end of
> qed_aio_write_l1_update(). Inline it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 36 ++++++++++++++----------------------
>  1 file changed, 14 insertions(+), 22 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 17/29] qed: Add return value to qed_aio_write_l1_update()

[Kevin Wolf]

Don't recurse into qed_aio_next_io() and qed_aio_complete() here, but
just return an error code and let the caller handle it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 3b1cce4..2034b58 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -958,18 +958,12 @@ static void qed_aio_complete(QEDAIOCB *acb, int ret)
 /**
  * Update L1 table with new L2 table offset and write it out
  */
-static void qed_aio_write_l1_update(void *opaque, int ret)
+static int qed_aio_write_l1_update(QEDAIOCB *acb)
 {
-    QEDAIOCB *acb = opaque;
     BDRVQEDState *s = acb_to_s(acb);
     CachedL2Table *l2_table = acb->request.l2_table;
     uint64_t l2_offset = l2_table->offset;
-    int index;
-
-    if (ret) {
-        qed_aio_complete(acb, ret);
-        return;
-    }
+    int index, ret;
 
     index = qed_l1_index(s, acb->cur_pos);
     s->l1_table->offsets[index] = l2_table->offset;
@@ -985,7 +979,7 @@ static void qed_aio_write_l1_update(void *opaque, int ret)
     acb->request.l2_table = qed_find_l2_cache_entry(&s->l2_cache, l2_offset);
     assert(acb->request.l2_table != NULL);
 
-    qed_aio_next_io(acb, ret);
+    return ret;
 }
 
 
@@ -1014,7 +1008,12 @@ static void qed_aio_write_l2_update(QEDAIOCB *acb, int ret, uint64_t offset)
     if (need_alloc) {
         /* Write out the whole new L2 table */
         ret = qed_write_l2_table(s, &acb->request, 0, s->table_nelems, true);
-        qed_aio_write_l1_update(acb, ret);
+        if (ret) {
+            goto err;
+        }
+        ret = qed_aio_write_l1_update(acb);
+        qed_aio_next_io(acb, ret);
+
     } else {
         /* Write out only the updated part of the L2 table */
         ret = qed_write_l2_table(s, &acb->request, index, acb->cur_nclusters,
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-block] [PATCH 17/29] qed: Add return value to qed_aio_write_l1_update()

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:21:58PM +0200, Kevin Wolf wrote:
> Don't recurse into qed_aio_next_io() and qed_aio_complete() here, but
> just return an error code and let the caller handle it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 18/29] qed: Add return value to qed_aio_write_l2_update()

[Kevin Wolf]

Don't recurse into qed_aio_next_io() and qed_aio_complete() here, but
just return an error code and let the caller handle it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 2034b58..259abb2 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -986,15 +986,11 @@ static int qed_aio_write_l1_update(QEDAIOCB *acb)
 /**
  * Update L2 table with new cluster offsets and write them out
  */
-static void qed_aio_write_l2_update(QEDAIOCB *acb, int ret, uint64_t offset)
+static int qed_aio_write_l2_update(QEDAIOCB *acb, uint64_t offset)
 {
     BDRVQEDState *s = acb_to_s(acb);
     bool need_alloc = acb->find_cluster_ret == QED_CLUSTER_L1;
-    int index;
-
-    if (ret) {
-        goto err;
-    }
+    int index, ret;
 
     if (need_alloc) {
         qed_unref_l2_cache_entry(acb->request.l2_table);
@@ -1009,21 +1005,18 @@ static void qed_aio_write_l2_update(QEDAIOCB *acb, int ret, uint64_t offset)
         /* Write out the whole new L2 table */
         ret = qed_write_l2_table(s, &acb->request, 0, s->table_nelems, true);
         if (ret) {
-            goto err;
+            return ret;
         }
-        ret = qed_aio_write_l1_update(acb);
-        qed_aio_next_io(acb, ret);
-
+        return qed_aio_write_l1_update(acb);
     } else {
         /* Write out only the updated part of the L2 table */
         ret = qed_write_l2_table(s, &acb->request, index, acb->cur_nclusters,
                                  false);
-        qed_aio_next_io(acb, ret);
+        if (ret) {
+            return ret;
+        }
     }
-    return;
-
-err:
-    qed_aio_complete(acb, ret);
+    return 0;
 }
 
 /**
@@ -1065,8 +1058,19 @@ static void qed_aio_write_main(void *opaque, int ret)
              */
             ret = bdrv_flush(s->bs->file->bs);
         }
-        qed_aio_write_l2_update(acb, ret, acb->cur_cluster);
+        if (ret) {
+            goto err;
+        }
+        ret = qed_aio_write_l2_update(acb, acb->cur_cluster);
+        if (ret) {
+            goto err;
+        }
+        qed_aio_next_io(acb, 0);
     }
+    return;
+
+err:
+    qed_aio_complete(acb, ret);
 }
 
 /**
@@ -1124,7 +1128,12 @@ static void qed_aio_write_zero_cluster(void *opaque, int ret)
         return;
     }
 
-    qed_aio_write_l2_update(acb, 0, 1);
+    ret = qed_aio_write_l2_update(acb, 1);
+    if (ret < 0) {
+        qed_aio_complete(acb, ret);
+        return;
+    }
+    qed_aio_next_io(acb, 0);
 }
 
 /**
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-block] [PATCH 18/29] qed: Add return value to qed_aio_write_l2_update()

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:21:59PM +0200, Kevin Wolf wrote:
> Don't recurse into qed_aio_next_io() and qed_aio_complete() here, but
> just return an error code and let the caller handle it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 43 ++++++++++++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 17 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 19/29] qed: Add return value to qed_aio_write_main()

[Kevin Wolf]

Don't recurse into qed_aio_next_io() and qed_aio_complete() here, but
just return an error code and let the caller handle it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 55 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 25 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 259abb2..e6d1b0d 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1022,29 +1022,22 @@ static int qed_aio_write_l2_update(QEDAIOCB *acb, uint64_t offset)
 /**
  * Write data to the image file
  */
-static void qed_aio_write_main(void *opaque, int ret)
+static int qed_aio_write_main(QEDAIOCB *acb)
 {
-    QEDAIOCB *acb = opaque;
     BDRVQEDState *s = acb_to_s(acb);
     uint64_t offset = acb->cur_cluster +
                       qed_offset_into_cluster(s, acb->cur_pos);
+    int ret;
 
-    trace_qed_aio_write_main(s, acb, ret, offset, acb->cur_qiov.size);
-
-    if (ret) {
-        qed_aio_complete(acb, ret);
-        return;
-    }
+    trace_qed_aio_write_main(s, acb, 0, offset, acb->cur_qiov.size);
 
     BLKDBG_EVENT(s->bs->file, BLKDBG_WRITE_AIO);
     ret = bdrv_pwritev(s->bs->file, offset, &acb->cur_qiov);
-    if (ret >= 0) {
-        ret = 0;
+    if (ret < 0) {
+        return ret;
     }
 
-    if (acb->find_cluster_ret == QED_CLUSTER_FOUND) {
-        qed_aio_next_io(acb, ret);
-    } else {
+    if (acb->find_cluster_ret != QED_CLUSTER_FOUND) {
         if (s->bs->backing) {
             /*
              * Flush new data clusters before updating the L2 table
@@ -1057,20 +1050,16 @@ static void qed_aio_write_main(void *opaque, int ret)
              * cluster and before updating the L2 table.
              */
             ret = bdrv_flush(s->bs->file->bs);
-        }
-        if (ret) {
-            goto err;
+            if (ret < 0) {
+                return ret;
+            }
         }
         ret = qed_aio_write_l2_update(acb, acb->cur_cluster);
-        if (ret) {
-            goto err;
+        if (ret < 0) {
+            return ret;
         }
-        qed_aio_next_io(acb, 0);
     }
-    return;
-
-err:
-    qed_aio_complete(acb, ret);
+    return 0;
 }
 
 /**
@@ -1102,8 +1091,17 @@ static void qed_aio_write_cow(void *opaque, int ret)
 
     trace_qed_aio_write_postfill(s, acb, start, len, offset);
     ret = qed_copy_from_backing_file(s, start, len, offset);
+    if (ret) {
+        qed_aio_complete(acb, ret);
+        return;
+    }
 
-    qed_aio_write_main(acb, ret);
+    ret = qed_aio_write_main(acb);
+    if (ret < 0) {
+        qed_aio_complete(acb, ret);
+        return;
+    }
+    qed_aio_next_io(acb, 0);
 }
 
 /**
@@ -1201,6 +1199,8 @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len)
  */
 static void qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, size_t len)
 {
+    int ret;
+
     /* Allocate buffer for zero writes */
     if (acb->flags & QED_AIOCB_ZERO) {
         struct iovec *iov = acb->qiov->iov;
@@ -1220,7 +1220,12 @@ static void qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, size_t len)
     qemu_iovec_concat(&acb->cur_qiov, acb->qiov, acb->qiov_offset, len);
 
     /* Do the actual write */
-    qed_aio_write_main(acb, 0);
+    ret = qed_aio_write_main(acb);
+    if (ret < 0) {
+        qed_aio_complete(acb, ret);
+        return;
+    }
+    qed_aio_next_io(acb, 0);
 }
 
 /**
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-block] [PATCH 19/29] qed: Add return value to qed_aio_write_main()

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:22:00PM +0200, Kevin Wolf wrote:
> Don't recurse into qed_aio_next_io() and qed_aio_complete() here, but
> just return an error code and let the caller handle it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 55 ++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 30 insertions(+), 25 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 20/29] qed: Add return value to qed_aio_write_cow()

[Kevin Wolf]

Don't recurse into qed_aio_next_io() and qed_aio_complete() here, but
just return an error code and let the caller handle it.

While refactoring qed_aio_write_alloc() to accomodate the change,
qed_aio_write_zero_cluster() ended up with a single line, so I chose to
inline that line and remove the function completely.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 58 +++++++++++++++++++++-------------------------------------
 1 file changed, 21 insertions(+), 37 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index e6d1b0d..66332f0 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1065,11 +1065,11 @@ static int qed_aio_write_main(QEDAIOCB *acb)
 /**
  * Populate untouched regions of new data cluster
  */
-static void qed_aio_write_cow(void *opaque, int ret)
+static int qed_aio_write_cow(QEDAIOCB *acb)
 {
-    QEDAIOCB *acb = opaque;
     BDRVQEDState *s = acb_to_s(acb);
     uint64_t start, len, offset;
+    int ret;
 
     /* Populate front untouched region of new data cluster */
     start = qed_start_of_cluster(s, acb->cur_pos);
@@ -1077,9 +1077,8 @@ static void qed_aio_write_cow(void *opaque, int ret)
 
     trace_qed_aio_write_prefill(s, acb, start, len, acb->cur_cluster);
     ret = qed_copy_from_backing_file(s, start, len, acb->cur_cluster);
-    if (ret) {
-        qed_aio_complete(acb, ret);
-        return;
+    if (ret < 0) {
+        return ret;
     }
 
     /* Populate back untouched region of new data cluster */
@@ -1091,17 +1090,11 @@ static void qed_aio_write_cow(void *opaque, int ret)
 
     trace_qed_aio_write_postfill(s, acb, start, len, offset);
     ret = qed_copy_from_backing_file(s, start, len, offset);
-    if (ret) {
-        qed_aio_complete(acb, ret);
-        return;
-    }
-
-    ret = qed_aio_write_main(acb);
     if (ret < 0) {
-        qed_aio_complete(acb, ret);
-        return;
+        return ret;
     }
-    qed_aio_next_io(acb, 0);
+
+    return qed_aio_write_main(acb);
 }
 
 /**
@@ -1117,23 +1110,6 @@ static bool qed_should_set_need_check(BDRVQEDState *s)
     return !(s->header.features & QED_F_NEED_CHECK);
 }
 
-static void qed_aio_write_zero_cluster(void *opaque, int ret)
-{
-    QEDAIOCB *acb = opaque;
-
-    if (ret) {
-        qed_aio_complete(acb, ret);
-        return;
-    }
-
-    ret = qed_aio_write_l2_update(acb, 1);
-    if (ret < 0) {
-        qed_aio_complete(acb, ret);
-        return;
-    }
-    qed_aio_next_io(acb, 0);
-}
-
 /**
  * Write new data cluster
  *
@@ -1145,7 +1121,6 @@ static void qed_aio_write_zero_cluster(void *opaque, int ret)
 static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len)
 {
     BDRVQEDState *s = acb_to_s(acb);
-    BlockCompletionFunc *cb;
     int ret;
 
     /* Cancel timer when the first allocating request comes in */
@@ -1172,20 +1147,29 @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len)
             qed_aio_start_io(acb);
             return;
         }
-
-        cb = qed_aio_write_zero_cluster;
     } else {
-        cb = qed_aio_write_cow;
         acb->cur_cluster = qed_alloc_clusters(s, acb->cur_nclusters);
     }
 
     if (qed_should_set_need_check(s)) {
         s->header.features |= QED_F_NEED_CHECK;
         ret = qed_write_header(s);
-        cb(acb, ret);
+        if (ret < 0) {
+            qed_aio_complete(acb, ret);
+            return;
+        }
+    }
+
+    if (acb->flags & QED_AIOCB_ZERO) {
+        ret = qed_aio_write_l2_update(acb, 1);
     } else {
-        cb(acb, 0);
+        ret = qed_aio_write_cow(acb);
     }
+    if (ret < 0) {
+        qed_aio_complete(acb, ret);
+        return;
+    }
+    qed_aio_next_io(acb, 0);
 }
 
 /**
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-block] [PATCH 20/29] qed: Add return value to qed_aio_write_cow()

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:22:01PM +0200, Kevin Wolf wrote:
> Don't recurse into qed_aio_next_io() and qed_aio_complete() here, but
> just return an error code and let the caller handle it.
> 
> While refactoring qed_aio_write_alloc() to accomodate the change,
> qed_aio_write_zero_cluster() ended up with a single line, so I chose to
> inline that line and remove the function completely.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 58 +++++++++++++++++++++-------------------------------------
>  1 file changed, 21 insertions(+), 37 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 21/29] qed: Add return value to qed_aio_write_inplace/alloc()

[Kevin Wolf]

Don't recurse into qed_aio_next_io() and qed_aio_complete() here, but
just return an error code and let the caller handle it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 43 ++++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 66332f0..7880df8 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1118,7 +1118,7 @@ static bool qed_should_set_need_check(BDRVQEDState *s)
  *
  * This path is taken when writing to previously unallocated clusters.
  */
-static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len)
+static int qed_aio_write_alloc(QEDAIOCB *acb, size_t len)
 {
     BDRVQEDState *s = acb_to_s(acb);
     int ret;
@@ -1134,7 +1134,7 @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len)
     }
     if (acb != QSIMPLEQ_FIRST(&s->allocating_write_reqs) ||
         s->allocating_write_reqs_plugged) {
-        return; /* wait for existing request to finish */
+        return -EINPROGRESS; /* wait for existing request to finish */
     }
 
     acb->cur_nclusters = qed_bytes_to_clusters(s,
@@ -1144,8 +1144,7 @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len)
     if (acb->flags & QED_AIOCB_ZERO) {
         /* Skip ahead if the clusters are already zero */
         if (acb->find_cluster_ret == QED_CLUSTER_ZERO) {
-            qed_aio_start_io(acb);
-            return;
+            return 0;
         }
     } else {
         acb->cur_cluster = qed_alloc_clusters(s, acb->cur_nclusters);
@@ -1155,8 +1154,7 @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len)
         s->header.features |= QED_F_NEED_CHECK;
         ret = qed_write_header(s);
         if (ret < 0) {
-            qed_aio_complete(acb, ret);
-            return;
+            return ret;
         }
     }
 
@@ -1166,10 +1164,9 @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len)
         ret = qed_aio_write_cow(acb);
     }
     if (ret < 0) {
-        qed_aio_complete(acb, ret);
-        return;
+        return ret;
     }
-    qed_aio_next_io(acb, 0);
+    return 0;
 }
 
 /**
@@ -1181,10 +1178,8 @@ static void qed_aio_write_alloc(QEDAIOCB *acb, size_t len)
  *
  * This path is taken when writing to already allocated clusters.
  */
-static void qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, size_t len)
+static int qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, size_t len)
 {
-    int ret;
-
     /* Allocate buffer for zero writes */
     if (acb->flags & QED_AIOCB_ZERO) {
         struct iovec *iov = acb->qiov->iov;
@@ -1192,8 +1187,7 @@ static void qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, size_t len)
         if (!iov->iov_base) {
             iov->iov_base = qemu_try_blockalign(acb->common.bs, iov->iov_len);
             if (iov->iov_base == NULL) {
-                qed_aio_complete(acb, -ENOMEM);
-                return;
+                return -ENOMEM;
             }
             memset(iov->iov_base, 0, iov->iov_len);
         }
@@ -1204,12 +1198,7 @@ static void qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, size_t len)
     qemu_iovec_concat(&acb->cur_qiov, acb->qiov, acb->qiov_offset, len);
 
     /* Do the actual write */
-    ret = qed_aio_write_main(acb);
-    if (ret < 0) {
-        qed_aio_complete(acb, ret);
-        return;
-    }
-    qed_aio_next_io(acb, 0);
+    return qed_aio_write_main(acb);
 }
 
 /**
@@ -1234,19 +1223,27 @@ static void qed_aio_write_data(void *opaque, int ret,
 
     switch (ret) {
     case QED_CLUSTER_FOUND:
-        qed_aio_write_inplace(acb, offset, len);
+        ret = qed_aio_write_inplace(acb, offset, len);
         break;
 
     case QED_CLUSTER_L2:
     case QED_CLUSTER_L1:
     case QED_CLUSTER_ZERO:
-        qed_aio_write_alloc(acb, len);
+        ret = qed_aio_write_alloc(acb, len);
         break;
 
     default:
-        qed_aio_complete(acb, ret);
+        assert(ret < 0);
         break;
     }
+
+    if (ret < 0) {
+        if (ret != -EINPROGRESS) {
+            qed_aio_complete(acb, ret);
+        }
+        return;
+    }
+    qed_aio_next_io(acb, 0);
 }
 
 /**
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 21/29] qed: Add return value to qed_aio_write_inplace/alloc()

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:22:02PM +0200, Kevin Wolf wrote:
> Don't recurse into qed_aio_next_io() and qed_aio_complete() here, but
> just return an error code and let the caller handle it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 43 ++++++++++++++++++++-----------------------
>  1 file changed, 20 insertions(+), 23 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 22/29] qed: Add return value to qed_aio_read/write_data()

[Kevin Wolf]

Don't recurse into qed_aio_next_io() and qed_aio_complete() here, but
just return an error code and let the caller handle it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 72 ++++++++++++++++++++++++++-----------------------------------
 block/qed.h | 21 ------------------
 2 files changed, 31 insertions(+), 62 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 7880df8..3a26fdf 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1205,15 +1205,14 @@ static int qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, size_t len)
  * Write data cluster
  *
  * @opaque:     Write request
- * @ret:        QED_CLUSTER_FOUND, QED_CLUSTER_L2, QED_CLUSTER_L1,
- *              or -errno
+ * @ret:        QED_CLUSTER_FOUND, QED_CLUSTER_L2 or QED_CLUSTER_L1
  * @offset:     Cluster offset in bytes
  * @len:        Length in bytes
  *
  * Callback from qed_find_cluster().
  */
-static void qed_aio_write_data(void *opaque, int ret,
-                               uint64_t offset, size_t len)
+static int qed_aio_write_data(void *opaque, int ret,
+                              uint64_t offset, size_t len)
 {
     QEDAIOCB *acb = opaque;
 
@@ -1223,42 +1222,29 @@ static void qed_aio_write_data(void *opaque, int ret,
 
     switch (ret) {
     case QED_CLUSTER_FOUND:
-        ret = qed_aio_write_inplace(acb, offset, len);
-        break;
+        return qed_aio_write_inplace(acb, offset, len);
 
     case QED_CLUSTER_L2:
     case QED_CLUSTER_L1:
     case QED_CLUSTER_ZERO:
-        ret = qed_aio_write_alloc(acb, len);
-        break;
+        return qed_aio_write_alloc(acb, len);
 
     default:
-        assert(ret < 0);
-        break;
-    }
-
-    if (ret < 0) {
-        if (ret != -EINPROGRESS) {
-            qed_aio_complete(acb, ret);
-        }
-        return;
+        g_assert_not_reached();
     }
-    qed_aio_next_io(acb, 0);
 }
 
 /**
  * Read data cluster
  *
  * @opaque:     Read request
- * @ret:        QED_CLUSTER_FOUND, QED_CLUSTER_L2, QED_CLUSTER_L1,
- *              or -errno
+ * @ret:        QED_CLUSTER_FOUND, QED_CLUSTER_L2 or QED_CLUSTER_L1
  * @offset:     Cluster offset in bytes
  * @len:        Length in bytes
  *
  * Callback from qed_find_cluster().
  */
-static void qed_aio_read_data(void *opaque, int ret,
-                              uint64_t offset, size_t len)
+static int qed_aio_read_data(void *opaque, int ret, uint64_t offset, size_t len)
 {
     QEDAIOCB *acb = opaque;
     BDRVQEDState *s = acb_to_s(acb);
@@ -1269,34 +1255,23 @@ static void qed_aio_read_data(void *opaque, int ret,
 
     trace_qed_aio_read_data(s, acb, ret, offset, len);
 
-    if (ret < 0) {
-        goto err;
-    }
-
     qemu_iovec_concat(&acb->cur_qiov, acb->qiov, acb->qiov_offset, len);
 
     /* Handle zero cluster and backing file reads */
     if (ret == QED_CLUSTER_ZERO) {
         qemu_iovec_memset(&acb->cur_qiov, 0, 0, acb->cur_qiov.size);
-        qed_aio_start_io(acb);
-        return;
+        return 0;
     } else if (ret != QED_CLUSTER_FOUND) {
-        ret = qed_read_backing_file(s, acb->cur_pos, &acb->cur_qiov,
-                                    &acb->backing_qiov);
-        qed_aio_next_io(acb, ret);
-        return;
+        return qed_read_backing_file(s, acb->cur_pos, &acb->cur_qiov,
+                                     &acb->backing_qiov);
     }
 
     BLKDBG_EVENT(bs->file, BLKDBG_READ_AIO);
     ret = bdrv_preadv(bs->file, offset, &acb->cur_qiov);
     if (ret < 0) {
-        goto err;
+        return ret;
     }
-    qed_aio_next_io(acb, 0);
-    return;
-
-err:
-    qed_aio_complete(acb, ret);
+    return 0;
 }
 
 /**
@@ -1305,8 +1280,6 @@ err:
 static void qed_aio_next_io(QEDAIOCB *acb, int ret)
 {
     BDRVQEDState *s = acb_to_s(acb);
-    QEDFindClusterFunc *io_fn = (acb->flags & QED_AIOCB_WRITE) ?
-                                qed_aio_write_data : qed_aio_read_data;
     uint64_t offset;
     size_t len;
 
@@ -1337,7 +1310,24 @@ static void qed_aio_next_io(QEDAIOCB *acb, int ret)
     /* Find next cluster and start I/O */
     len = acb->end_pos - acb->cur_pos;
     ret = qed_find_cluster(s, &acb->request, acb->cur_pos, &len, &offset);
-    io_fn(acb, ret, offset, len);
+    if (ret < 0) {
+        qed_aio_complete(acb, ret);
+        return;
+    }
+
+    if (acb->flags & QED_AIOCB_WRITE) {
+        ret = qed_aio_write_data(acb, ret, offset, len);
+    } else {
+        ret = qed_aio_read_data(acb, ret, offset, len);
+    }
+
+    if (ret < 0) {
+        if (ret != -EINPROGRESS) {
+            qed_aio_complete(acb, ret);
+        }
+        return;
+    }
+    qed_aio_next_io(acb, 0);
 }
 
 static BlockAIOCB *qed_aio_setup(BlockDriverState *bs,
diff --git a/block/qed.h b/block/qed.h
index 51443fa..8644fed 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -177,27 +177,6 @@ enum {
     QED_CLUSTER_L1,            /* cluster missing in L1 */
 };
 
-/**
- * qed_find_cluster() completion callback
- *
- * @opaque:     User data for completion callback
- * @ret:        QED_CLUSTER_FOUND   Success
- *              QED_CLUSTER_L2      Data cluster unallocated in L2
- *              QED_CLUSTER_L1      L2 unallocated in L1
- *              -errno              POSIX error occurred
- * @offset:     Data cluster offset
- * @len:        Contiguous bytes starting from cluster offset
- *
- * This function is invoked when qed_find_cluster() completes.
- *
- * On success ret is QED_CLUSTER_FOUND and offset/len are a contiguous range
- * in the image file.
- *
- * On failure ret is QED_CLUSTER_L2 or QED_CLUSTER_L1 for missing L2 or L1
- * table offset, respectively.  len is number of contiguous unallocated bytes.
- */
-typedef void QEDFindClusterFunc(void *opaque, int ret, uint64_t offset, size_t len);
-
 void qed_acquire(BDRVQEDState *s);
 void qed_release(BDRVQEDState *s);
 
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-block] [PATCH 22/29] qed: Add return value to qed_aio_read/write_data()

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:22:03PM +0200, Kevin Wolf wrote:
> Don't recurse into qed_aio_next_io() and qed_aio_complete() here, but
> just return an error code and let the caller handle it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 72 ++++++++++++++++++++++++++-----------------------------------
>  block/qed.h | 21 ------------------
>  2 files changed, 31 insertions(+), 62 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 23/29] qed: Remove ret argument from qed_aio_next_io()

[Kevin Wolf]

All callers pass ret = 0, so we can just remove it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 3a26fdf..078591b 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -253,11 +253,11 @@ static CachedL2Table *qed_new_l2_table(BDRVQEDState *s)
     return l2_table;
 }
 
-static void qed_aio_next_io(QEDAIOCB *acb, int ret);
+static void qed_aio_next_io(QEDAIOCB *acb);
 
 static void qed_aio_start_io(QEDAIOCB *acb)
 {
-    qed_aio_next_io(acb, 0);
+    qed_aio_next_io(acb);
 }
 
 static void qed_plug_allocating_write_reqs(BDRVQEDState *s)
@@ -1277,13 +1277,14 @@ static int qed_aio_read_data(void *opaque, int ret, uint64_t offset, size_t len)
 /**
  * Begin next I/O or complete the request
  */
-static void qed_aio_next_io(QEDAIOCB *acb, int ret)
+static void qed_aio_next_io(QEDAIOCB *acb)
 {
     BDRVQEDState *s = acb_to_s(acb);
     uint64_t offset;
     size_t len;
+    int ret;
 
-    trace_qed_aio_next_io(s, acb, ret, acb->cur_pos + acb->cur_qiov.size);
+    trace_qed_aio_next_io(s, acb, 0, acb->cur_pos + acb->cur_qiov.size);
 
     if (acb->backing_qiov) {
         qemu_iovec_destroy(acb->backing_qiov);
@@ -1291,12 +1292,6 @@ static void qed_aio_next_io(QEDAIOCB *acb, int ret)
         acb->backing_qiov = NULL;
     }
 
-    /* Handle I/O error */
-    if (ret) {
-        qed_aio_complete(acb, ret);
-        return;
-    }
-
     acb->qiov_offset += acb->cur_qiov.size;
     acb->cur_pos += acb->cur_qiov.size;
     qemu_iovec_reset(&acb->cur_qiov);
@@ -1327,7 +1322,7 @@ static void qed_aio_next_io(QEDAIOCB *acb, int ret)
         }
         return;
     }
-    qed_aio_next_io(acb, 0);
+    qed_aio_next_io(acb);
 }
 
 static BlockAIOCB *qed_aio_setup(BlockDriverState *bs,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 23/29] qed: Remove ret argument from qed_aio_next_io()

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:22:04PM +0200, Kevin Wolf wrote:
> All callers pass ret = 0, so we can just remove it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 24/29] qed: Remove recursion in qed_aio_next_io()

[Kevin Wolf]

Instead of calling itself recursively as the last thing, just convert
qed_aio_next_io() into a loop.

This patch is best reviewed with 'git show -w' because most of it is
just whitespace changes.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 63 +++++++++++++++++++++++++++++++------------------------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 078591b..6a83df2 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1284,45 +1284,46 @@ static void qed_aio_next_io(QEDAIOCB *acb)
     size_t len;
     int ret;
 
-    trace_qed_aio_next_io(s, acb, 0, acb->cur_pos + acb->cur_qiov.size);
+    while (1) {
+        trace_qed_aio_next_io(s, acb, 0, acb->cur_pos + acb->cur_qiov.size);
 
-    if (acb->backing_qiov) {
-        qemu_iovec_destroy(acb->backing_qiov);
-        g_free(acb->backing_qiov);
-        acb->backing_qiov = NULL;
-    }
+        if (acb->backing_qiov) {
+            qemu_iovec_destroy(acb->backing_qiov);
+            g_free(acb->backing_qiov);
+            acb->backing_qiov = NULL;
+        }
 
-    acb->qiov_offset += acb->cur_qiov.size;
-    acb->cur_pos += acb->cur_qiov.size;
-    qemu_iovec_reset(&acb->cur_qiov);
+        acb->qiov_offset += acb->cur_qiov.size;
+        acb->cur_pos += acb->cur_qiov.size;
+        qemu_iovec_reset(&acb->cur_qiov);
 
-    /* Complete request */
-    if (acb->cur_pos >= acb->end_pos) {
-        qed_aio_complete(acb, 0);
-        return;
-    }
+        /* Complete request */
+        if (acb->cur_pos >= acb->end_pos) {
+            qed_aio_complete(acb, 0);
+            return;
+        }
 
-    /* Find next cluster and start I/O */
-    len = acb->end_pos - acb->cur_pos;
-    ret = qed_find_cluster(s, &acb->request, acb->cur_pos, &len, &offset);
-    if (ret < 0) {
-        qed_aio_complete(acb, ret);
-        return;
-    }
+        /* Find next cluster and start I/O */
+        len = acb->end_pos - acb->cur_pos;
+        ret = qed_find_cluster(s, &acb->request, acb->cur_pos, &len, &offset);
+        if (ret < 0) {
+            qed_aio_complete(acb, ret);
+            return;
+        }
 
-    if (acb->flags & QED_AIOCB_WRITE) {
-        ret = qed_aio_write_data(acb, ret, offset, len);
-    } else {
-        ret = qed_aio_read_data(acb, ret, offset, len);
-    }
+        if (acb->flags & QED_AIOCB_WRITE) {
+            ret = qed_aio_write_data(acb, ret, offset, len);
+        } else {
+            ret = qed_aio_read_data(acb, ret, offset, len);
+        }
 
-    if (ret < 0) {
-        if (ret != -EINPROGRESS) {
-            qed_aio_complete(acb, ret);
+        if (ret < 0) {
+            if (ret != -EINPROGRESS) {
+                qed_aio_complete(acb, ret);
+            }
+            return;
         }
-        return;
     }
-    qed_aio_next_io(acb);
 }
 
 static BlockAIOCB *qed_aio_setup(BlockDriverState *bs,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 24/29] qed: Remove recursion in qed_aio_next_io()

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:22:05PM +0200, Kevin Wolf wrote:
> Instead of calling itself recursively as the last thing, just convert
> qed_aio_next_io() into a loop.
> 
> This patch is best reviewed with 'git show -w' because most of it is
> just whitespace changes.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 63 +++++++++++++++++++++++++++++++------------------------------
>  1 file changed, 32 insertions(+), 31 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 25/29] qed: Implement .bdrv_co_readv/writev

[Kevin Wolf]

Most of the qed code is now synchronous and matches the coroutine model.
One notable exception is the serialisation between requests which can
still schedule a callback. Before we can replace this with coroutine
locks, let's convert the driver's external interfaces to the coroutine
versions.

We need to be careful to handle both requests that call the completion
callback directly from the calling coroutine (i.e. fully synchronous
code) and requests that involve some callback, so that we need to yield
and wait for the completion callback coming from outside the coroutine.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 94 +++++++++++++++++++++++++------------------------------------
 1 file changed, 39 insertions(+), 55 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 6a83df2..29c3dc5 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1326,16 +1326,31 @@ static void qed_aio_next_io(QEDAIOCB *acb)
     }
 }
 
-static BlockAIOCB *qed_aio_setup(BlockDriverState *bs,
-                                 int64_t sector_num,
-                                 QEMUIOVector *qiov, int nb_sectors,
-                                 BlockCompletionFunc *cb,
-                                 void *opaque, int flags)
+typedef struct QEDRequestCo {
+    Coroutine *co;
+    bool done;
+    int ret;
+} QEDRequestCo;
+
+static void coroutine_fn qed_co_request_cb(void *opaque, int ret)
+{
+    QEDRequestCo *co = opaque;
+
+    co->done = true;
+    co->ret = ret;
+    qemu_coroutine_enter_if_inactive(co->co);
+}
+
+static int qed_co_request(BlockDriverState *bs, int64_t sector_num,
+                          QEMUIOVector *qiov, int nb_sectors, int flags)
 {
-    QEDAIOCB *acb = qemu_aio_get(&qed_aiocb_info, bs, cb, opaque);
+    QEDRequestCo co = {
+        .co     = qemu_coroutine_self(),
+        .done   = false,
+    };
+    QEDAIOCB *acb = qemu_aio_get(&qed_aiocb_info, bs, qed_co_request_cb, &co);
 
-    trace_qed_aio_setup(bs->opaque, acb, sector_num, nb_sectors,
-                        opaque, flags);
+    trace_qed_aio_setup(bs->opaque, acb, sector_num, nb_sectors, &co, flags);
 
     acb->flags = flags;
     acb->qiov = qiov;
@@ -1348,43 +1363,24 @@ static BlockAIOCB *qed_aio_setup(BlockDriverState *bs,
 
     /* Start request */
     qed_aio_start_io(acb);
-    return &acb->common;
-}
 
-static BlockAIOCB *bdrv_qed_aio_readv(BlockDriverState *bs,
-                                      int64_t sector_num,
-                                      QEMUIOVector *qiov, int nb_sectors,
-                                      BlockCompletionFunc *cb,
-                                      void *opaque)
-{
-    return qed_aio_setup(bs, sector_num, qiov, nb_sectors, cb, opaque, 0);
+    if (!co.done) {
+        qemu_coroutine_yield();
+    }
+
+    return co.ret;
 }
 
-static BlockAIOCB *bdrv_qed_aio_writev(BlockDriverState *bs,
-                                       int64_t sector_num,
-                                       QEMUIOVector *qiov, int nb_sectors,
-                                       BlockCompletionFunc *cb,
-                                       void *opaque)
+static int bdrv_qed_co_readv(BlockDriverState *bs, int64_t sector_num,
+                             int nb_sectors, QEMUIOVector *qiov)
 {
-    return qed_aio_setup(bs, sector_num, qiov, nb_sectors, cb,
-                         opaque, QED_AIOCB_WRITE);
+    return qed_co_request(bs, sector_num, qiov, nb_sectors, 0);
 }
 
-typedef struct {
-    Coroutine *co;
-    int ret;
-    bool done;
-} QEDWriteZeroesCB;
-
-static void coroutine_fn qed_co_pwrite_zeroes_cb(void *opaque, int ret)
+static int bdrv_qed_co_writev(BlockDriverState *bs, int64_t sector_num,
+                                      int nb_sectors, QEMUIOVector *qiov)
 {
-    QEDWriteZeroesCB *cb = opaque;
-
-    cb->done = true;
-    cb->ret = ret;
-    if (cb->co) {
-        aio_co_wake(cb->co);
-    }
+    return qed_co_request(bs, sector_num, qiov, nb_sectors, QED_AIOCB_WRITE);
 }
 
 static int coroutine_fn bdrv_qed_co_pwrite_zeroes(BlockDriverState *bs,
@@ -1392,9 +1388,7 @@ static int coroutine_fn bdrv_qed_co_pwrite_zeroes(BlockDriverState *bs,
                                                   int count,
                                                   BdrvRequestFlags flags)
 {
-    BlockAIOCB *blockacb;
     BDRVQEDState *s = bs->opaque;
-    QEDWriteZeroesCB cb = { .done = false };
     QEMUIOVector qiov;
     struct iovec iov;
 
@@ -1411,19 +1405,9 @@ static int coroutine_fn bdrv_qed_co_pwrite_zeroes(BlockDriverState *bs,
     iov.iov_len = count;
 
     qemu_iovec_init_external(&qiov, &iov, 1);
-    blockacb = qed_aio_setup(bs, offset >> BDRV_SECTOR_BITS, &qiov,
-                             count >> BDRV_SECTOR_BITS,
-                             qed_co_pwrite_zeroes_cb, &cb,
-                             QED_AIOCB_WRITE | QED_AIOCB_ZERO);
-    if (!blockacb) {
-        return -EIO;
-    }
-    if (!cb.done) {
-        cb.co = qemu_coroutine_self();
-        qemu_coroutine_yield();
-    }
-    assert(cb.done);
-    return cb.ret;
+    return qed_co_request(bs, offset >> BDRV_SECTOR_BITS, &qiov,
+                          count >> BDRV_SECTOR_BITS,
+                          QED_AIOCB_WRITE | QED_AIOCB_ZERO);
 }
 
 static int bdrv_qed_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
@@ -1619,8 +1603,8 @@ static BlockDriver bdrv_qed = {
     .bdrv_create              = bdrv_qed_create,
     .bdrv_has_zero_init       = bdrv_has_zero_init_1,
     .bdrv_co_get_block_status = bdrv_qed_co_get_block_status,
-    .bdrv_aio_readv           = bdrv_qed_aio_readv,
-    .bdrv_aio_writev          = bdrv_qed_aio_writev,
+    .bdrv_co_readv            = bdrv_qed_co_readv,
+    .bdrv_co_writev           = bdrv_qed_co_writev,
     .bdrv_co_pwrite_zeroes    = bdrv_qed_co_pwrite_zeroes,
     .bdrv_truncate            = bdrv_qed_truncate,
     .bdrv_getlength           = bdrv_qed_getlength,
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-block] [PATCH 25/29] qed: Implement .bdrv_co_readv/writev

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:22:06PM +0200, Kevin Wolf wrote:
> Most of the qed code is now synchronous and matches the coroutine model.
> One notable exception is the serialisation between requests which can
> still schedule a callback. Before we can replace this with coroutine
> locks, let's convert the driver's external interfaces to the coroutine
> versions.
> 
> We need to be careful to handle both requests that call the completion
> callback directly from the calling coroutine (i.e. fully synchronous
> code) and requests that involve some callback, so that we need to yield
> and wait for the completion callback coming from outside the coroutine.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 94 +++++++++++++++++++++++++------------------------------------
>  1 file changed, 39 insertions(+), 55 deletions(-)
> 
> diff --git a/block/qed.c b/block/qed.c
> index 6a83df2..29c3dc5 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -1326,16 +1326,31 @@ static void qed_aio_next_io(QEDAIOCB *acb)
>      }
>  }
>  
> -static BlockAIOCB *qed_aio_setup(BlockDriverState *bs,
> -                                 int64_t sector_num,
> -                                 QEMUIOVector *qiov, int nb_sectors,
> -                                 BlockCompletionFunc *cb,
> -                                 void *opaque, int flags)
> +typedef struct QEDRequestCo {
> +    Coroutine *co;
> +    bool done;
> +    int ret;
> +} QEDRequestCo;
> +
> +static void coroutine_fn qed_co_request_cb(void *opaque, int ret)
> +{
> +    QEDRequestCo *co = opaque;
> +
> +    co->done = true;
> +    co->ret = ret;
> +    qemu_coroutine_enter_if_inactive(co->co);
> +}
> +
> +static int qed_co_request(BlockDriverState *bs, int64_t sector_num,
> +                          QEMUIOVector *qiov, int nb_sectors, int flags)

Missing coroutine_fn.

>  {
> -    QEDAIOCB *acb = qemu_aio_get(&qed_aiocb_info, bs, cb, opaque);
> +    QEDRequestCo co = {
> +        .co     = qemu_coroutine_self(),
> +        .done   = false,
> +    };
> +    QEDAIOCB *acb = qemu_aio_get(&qed_aiocb_info, bs, qed_co_request_cb, &co);
>  
> -    trace_qed_aio_setup(bs->opaque, acb, sector_num, nb_sectors,
> -                        opaque, flags);
> +    trace_qed_aio_setup(bs->opaque, acb, sector_num, nb_sectors, &co, flags);
>  
>      acb->flags = flags;
>      acb->qiov = qiov;
> @@ -1348,43 +1363,24 @@ static BlockAIOCB *qed_aio_setup(BlockDriverState *bs,
>  
>      /* Start request */
>      qed_aio_start_io(acb);
> -    return &acb->common;
> -}
>  
> -static BlockAIOCB *bdrv_qed_aio_readv(BlockDriverState *bs,
> -                                      int64_t sector_num,
> -                                      QEMUIOVector *qiov, int nb_sectors,
> -                                      BlockCompletionFunc *cb,
> -                                      void *opaque)
> -{
> -    return qed_aio_setup(bs, sector_num, qiov, nb_sectors, cb, opaque, 0);
> +    if (!co.done) {
> +        qemu_coroutine_yield();
> +    }
> +
> +    return co.ret;
>  }
>  
> -static BlockAIOCB *bdrv_qed_aio_writev(BlockDriverState *bs,
> -                                       int64_t sector_num,
> -                                       QEMUIOVector *qiov, int nb_sectors,
> -                                       BlockCompletionFunc *cb,
> -                                       void *opaque)
> +static int bdrv_qed_co_readv(BlockDriverState *bs, int64_t sector_num,
> +                             int nb_sectors, QEMUIOVector *qiov)

Missing coroutine_fn.  More below.

>  {
> -    return qed_aio_setup(bs, sector_num, qiov, nb_sectors, cb,
> -                         opaque, QED_AIOCB_WRITE);
> +    return qed_co_request(bs, sector_num, qiov, nb_sectors, 0);
>  }
>  
> -typedef struct {
> -    Coroutine *co;
> -    int ret;
> -    bool done;
> -} QEDWriteZeroesCB;
> -
> -static void coroutine_fn qed_co_pwrite_zeroes_cb(void *opaque, int ret)
> +static int bdrv_qed_co_writev(BlockDriverState *bs, int64_t sector_num,
> +                                      int nb_sectors, QEMUIOVector *qiov)
>  {
> -    QEDWriteZeroesCB *cb = opaque;
> -
> -    cb->done = true;
> -    cb->ret = ret;
> -    if (cb->co) {
> -        aio_co_wake(cb->co);
> -    }
> +    return qed_co_request(bs, sector_num, qiov, nb_sectors, QED_AIOCB_WRITE);
>  }
>  
>  static int coroutine_fn bdrv_qed_co_pwrite_zeroes(BlockDriverState *bs,
> @@ -1392,9 +1388,7 @@ static int coroutine_fn bdrv_qed_co_pwrite_zeroes(BlockDriverState *bs,
>                                                    int count,
>                                                    BdrvRequestFlags flags)
>  {
> -    BlockAIOCB *blockacb;
>      BDRVQEDState *s = bs->opaque;
> -    QEDWriteZeroesCB cb = { .done = false };
>      QEMUIOVector qiov;
>      struct iovec iov;
>  
> @@ -1411,19 +1405,9 @@ static int coroutine_fn bdrv_qed_co_pwrite_zeroes(BlockDriverState *bs,
>      iov.iov_len = count;
>  
>      qemu_iovec_init_external(&qiov, &iov, 1);
> -    blockacb = qed_aio_setup(bs, offset >> BDRV_SECTOR_BITS, &qiov,
> -                             count >> BDRV_SECTOR_BITS,
> -                             qed_co_pwrite_zeroes_cb, &cb,
> -                             QED_AIOCB_WRITE | QED_AIOCB_ZERO);
> -    if (!blockacb) {
> -        return -EIO;
> -    }
> -    if (!cb.done) {
> -        cb.co = qemu_coroutine_self();
> -        qemu_coroutine_yield();
> -    }
> -    assert(cb.done);
> -    return cb.ret;
> +    return qed_co_request(bs, offset >> BDRV_SECTOR_BITS, &qiov,
> +                          count >> BDRV_SECTOR_BITS,
> +                          QED_AIOCB_WRITE | QED_AIOCB_ZERO);
>  }
>  
>  static int bdrv_qed_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
> @@ -1619,8 +1603,8 @@ static BlockDriver bdrv_qed = {
>      .bdrv_create              = bdrv_qed_create,
>      .bdrv_has_zero_init       = bdrv_has_zero_init_1,
>      .bdrv_co_get_block_status = bdrv_qed_co_get_block_status,
> -    .bdrv_aio_readv           = bdrv_qed_aio_readv,
> -    .bdrv_aio_writev          = bdrv_qed_aio_writev,
> +    .bdrv_co_readv            = bdrv_qed_co_readv,
> +    .bdrv_co_writev           = bdrv_qed_co_writev,
>      .bdrv_co_pwrite_zeroes    = bdrv_qed_co_pwrite_zeroes,
>      .bdrv_truncate            = bdrv_qed_truncate,
>      .bdrv_getlength           = bdrv_qed_getlength,
> -- 
> 1.8.3.1
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 26/29] qed: Use CoQueue for serialising allocations

[Kevin Wolf]

Now that we're running in coroutine context, the ad-hoc serialisation
code (which drops a request that has to wait out of coroutine context)
can be replaced by a CoQueue.

This means that when we resume a serialised request, it is running in
coroutine context again and its I/O isn't blocking any more.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 49 +++++++++++++++++--------------------------------
 block/qed.h |  3 ++-
 2 files changed, 19 insertions(+), 33 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 29c3dc5..2eee451 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -269,16 +269,10 @@ static void qed_plug_allocating_write_reqs(BDRVQEDState *s)
 
 static void qed_unplug_allocating_write_reqs(BDRVQEDState *s)
 {
-    QEDAIOCB *acb;
-
     assert(s->allocating_write_reqs_plugged);
 
     s->allocating_write_reqs_plugged = false;
-
-    acb = QSIMPLEQ_FIRST(&s->allocating_write_reqs);
-    if (acb) {
-        qed_aio_start_io(acb);
-    }
+    qemu_co_enter_next(&s->allocating_write_reqs);
 }
 
 static void qed_clear_need_check(void *opaque, int ret)
@@ -305,7 +299,7 @@ static void qed_need_check_timer_cb(void *opaque)
     BDRVQEDState *s = opaque;
 
     /* The timer should only fire when allocating writes have drained */
-    assert(!QSIMPLEQ_FIRST(&s->allocating_write_reqs));
+    assert(!s->allocating_acb);
 
     trace_qed_need_check_timer_cb(s);
 
@@ -388,7 +382,7 @@ static int bdrv_qed_do_open(BlockDriverState *bs, QDict *options, int flags,
     int ret;
 
     s->bs = bs;
-    QSIMPLEQ_INIT(&s->allocating_write_reqs);
+    qemu_co_queue_init(&s->allocating_write_reqs);
 
     ret = bdrv_pread(bs->file, 0, &le_header, sizeof(le_header));
     if (ret < 0) {
@@ -910,11 +904,6 @@ static void qed_aio_complete_bh(void *opaque)
     qed_release(s);
 }
 
-static void qed_resume_alloc_bh(void *opaque)
-{
-    qed_aio_start_io(opaque);
-}
-
 static void qed_aio_complete(QEDAIOCB *acb, int ret)
 {
     BDRVQEDState *s = acb_to_s(acb);
@@ -942,13 +931,10 @@ static void qed_aio_complete(QEDAIOCB *acb, int ret)
      * next request in the queue.  This ensures that we don't cycle through
      * requests multiple times but rather finish one at a time completely.
      */
-    if (acb == QSIMPLEQ_FIRST(&s->allocating_write_reqs)) {
-        QEDAIOCB *next_acb;
-        QSIMPLEQ_REMOVE_HEAD(&s->allocating_write_reqs, next);
-        next_acb = QSIMPLEQ_FIRST(&s->allocating_write_reqs);
-        if (next_acb) {
-            aio_bh_schedule_oneshot(bdrv_get_aio_context(acb->common.bs),
-                                    qed_resume_alloc_bh, next_acb);
+    if (acb == s->allocating_acb) {
+        s->allocating_acb = NULL;
+        if (!qemu_co_queue_empty(&s->allocating_write_reqs)) {
+            qemu_co_enter_next(&s->allocating_write_reqs);
         } else if (s->header.features & QED_F_NEED_CHECK) {
             qed_start_need_check_timer(s);
         }
@@ -1124,17 +1110,18 @@ static int qed_aio_write_alloc(QEDAIOCB *acb, size_t len)
     int ret;
 
     /* Cancel timer when the first allocating request comes in */
-    if (QSIMPLEQ_EMPTY(&s->allocating_write_reqs)) {
+    if (s->allocating_acb == NULL) {
         qed_cancel_need_check_timer(s);
     }
 
     /* Freeze this request if another allocating write is in progress */
-    if (acb != QSIMPLEQ_FIRST(&s->allocating_write_reqs)) {
-        QSIMPLEQ_INSERT_TAIL(&s->allocating_write_reqs, acb, next);
-    }
-    if (acb != QSIMPLEQ_FIRST(&s->allocating_write_reqs) ||
-        s->allocating_write_reqs_plugged) {
-        return -EINPROGRESS; /* wait for existing request to finish */
+    if (s->allocating_acb != acb || s->allocating_write_reqs_plugged) {
+        if (s->allocating_acb != NULL) {
+            qemu_co_queue_wait(&s->allocating_write_reqs, NULL);
+            assert(s->allocating_acb == NULL);
+        }
+        s->allocating_acb = acb;
+        return -EAGAIN; /* start over with looking up table entries */
     }
 
     acb->cur_nclusters = qed_bytes_to_clusters(s,
@@ -1317,10 +1304,8 @@ static void qed_aio_next_io(QEDAIOCB *acb)
             ret = qed_aio_read_data(acb, ret, offset, len);
         }
 
-        if (ret < 0) {
-            if (ret != -EINPROGRESS) {
-                qed_aio_complete(acb, ret);
-            }
+        if (ret < 0 && ret != -EAGAIN) {
+            qed_aio_complete(acb, ret);
             return;
         }
     }
diff --git a/block/qed.h b/block/qed.h
index 8644fed..37558e4 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -163,7 +163,8 @@ typedef struct {
     uint32_t l2_mask;
 
     /* Allocating write request queue */
-    QSIMPLEQ_HEAD(, QEDAIOCB) allocating_write_reqs;
+    QEDAIOCB *allocating_acb;
+    CoQueue allocating_write_reqs;
     bool allocating_write_reqs_plugged;
 
     /* Periodic flush and clear need check flag */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 26/29] qed: Use CoQueue for serialising allocations

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:22:07PM +0200, Kevin Wolf wrote:
> Now that we're running in coroutine context, the ad-hoc serialisation
> code (which drops a request that has to wait out of coroutine context)
> can be replaced by a CoQueue.
> 
> This means that when we resume a serialised request, it is running in
> coroutine context again and its I/O isn't blocking any more.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 49 +++++++++++++++++--------------------------------
>  block/qed.h |  3 ++-
>  2 files changed, 19 insertions(+), 33 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 27/29] qed: Simplify request handling

[Kevin Wolf]

Now that we process a request in the same coroutine from beginning to
end and don't drop out of it any more, we can look like a proper
coroutine-based driver and simply call qed_aio_next_io() and get a
return value from it instead of spawning an additional coroutine that
reenters the parent when it's done.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 106 ++++++++++++++++--------------------------------------------
 block/qed.h |   3 +-
 2 files changed, 28 insertions(+), 81 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 2eee451..d3f7d0c 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -21,10 +21,6 @@
 #include "qapi/qmp/qerror.h"
 #include "sysemu/block-backend.h"
 
-static const AIOCBInfo qed_aiocb_info = {
-    .aiocb_size         = sizeof(QEDAIOCB),
-};
-
 static int bdrv_qed_probe(const uint8_t *buf, int buf_size,
                           const char *filename)
 {
@@ -253,13 +249,6 @@ static CachedL2Table *qed_new_l2_table(BDRVQEDState *s)
     return l2_table;
 }
 
-static void qed_aio_next_io(QEDAIOCB *acb);
-
-static void qed_aio_start_io(QEDAIOCB *acb)
-{
-    qed_aio_next_io(acb);
-}
-
 static void qed_plug_allocating_write_reqs(BDRVQEDState *s)
 {
     assert(!s->allocating_write_reqs_plugged);
@@ -751,7 +740,7 @@ static int64_t coroutine_fn bdrv_qed_co_get_block_status(BlockDriverState *bs,
 
 static BDRVQEDState *acb_to_s(QEDAIOCB *acb)
 {
-    return acb->common.bs->opaque;
+    return acb->bs->opaque;
 }
 
 /**
@@ -888,27 +877,9 @@ static void qed_update_l2_table(BDRVQEDState *s, QEDTable *table, int index,
     }
 }
 
-static void qed_aio_complete_bh(void *opaque)
+static void qed_aio_complete(QEDAIOCB *acb)
 {
-    QEDAIOCB *acb = opaque;
     BDRVQEDState *s = acb_to_s(acb);
-    BlockCompletionFunc *cb = acb->common.cb;
-    void *user_opaque = acb->common.opaque;
-    int ret = acb->bh_ret;
-
-    qemu_aio_unref(acb);
-
-    /* Invoke callback */
-    qed_acquire(s);
-    cb(user_opaque, ret);
-    qed_release(s);
-}
-
-static void qed_aio_complete(QEDAIOCB *acb, int ret)
-{
-    BDRVQEDState *s = acb_to_s(acb);
-
-    trace_qed_aio_complete(s, acb, ret);
 
     /* Free resources */
     qemu_iovec_destroy(&acb->cur_qiov);
@@ -920,11 +891,6 @@ static void qed_aio_complete(QEDAIOCB *acb, int ret)
         acb->qiov->iov[0].iov_base = NULL;
     }
 
-    /* Arrange for a bh to invoke the completion function */
-    acb->bh_ret = ret;
-    aio_bh_schedule_oneshot(bdrv_get_aio_context(acb->common.bs),
-                            qed_aio_complete_bh, acb);
-
     /* Start next allocating write request waiting behind this one.  Note that
      * requests enqueue themselves when they first hit an unallocated cluster
      * but they wait until the entire request is finished before waking up the
@@ -1172,7 +1138,7 @@ static int qed_aio_write_inplace(QEDAIOCB *acb, uint64_t offset, size_t len)
         struct iovec *iov = acb->qiov->iov;
 
         if (!iov->iov_base) {
-            iov->iov_base = qemu_try_blockalign(acb->common.bs, iov->iov_len);
+            iov->iov_base = qemu_try_blockalign(acb->bs, iov->iov_len);
             if (iov->iov_base == NULL) {
                 return -ENOMEM;
             }
@@ -1235,7 +1201,7 @@ static int qed_aio_read_data(void *opaque, int ret, uint64_t offset, size_t len)
 {
     QEDAIOCB *acb = opaque;
     BDRVQEDState *s = acb_to_s(acb);
-    BlockDriverState *bs = acb->common.bs;
+    BlockDriverState *bs = acb->bs;
 
     /* Adjust offset into cluster */
     offset += qed_offset_into_cluster(s, acb->cur_pos);
@@ -1264,7 +1230,7 @@ static int qed_aio_read_data(void *opaque, int ret, uint64_t offset, size_t len)
 /**
  * Begin next I/O or complete the request
  */
-static void qed_aio_next_io(QEDAIOCB *acb)
+static int qed_aio_next_io(QEDAIOCB *acb)
 {
     BDRVQEDState *s = acb_to_s(acb);
     uint64_t offset;
@@ -1286,16 +1252,15 @@ static void qed_aio_next_io(QEDAIOCB *acb)
 
         /* Complete request */
         if (acb->cur_pos >= acb->end_pos) {
-            qed_aio_complete(acb, 0);
-            return;
+            ret = 0;
+            break;
         }
 
         /* Find next cluster and start I/O */
         len = acb->end_pos - acb->cur_pos;
         ret = qed_find_cluster(s, &acb->request, acb->cur_pos, &len, &offset);
         if (ret < 0) {
-            qed_aio_complete(acb, ret);
-            return;
+            break;
         }
 
         if (acb->flags & QED_AIOCB_WRITE) {
@@ -1305,55 +1270,38 @@ static void qed_aio_next_io(QEDAIOCB *acb)
         }
 
         if (ret < 0 && ret != -EAGAIN) {
-            qed_aio_complete(acb, ret);
-            return;
+            break;
         }
     }
-}
-
-typedef struct QEDRequestCo {
-    Coroutine *co;
-    bool done;
-    int ret;
-} QEDRequestCo;
 
-static void coroutine_fn qed_co_request_cb(void *opaque, int ret)
-{
-    QEDRequestCo *co = opaque;
-
-    co->done = true;
-    co->ret = ret;
-    qemu_coroutine_enter_if_inactive(co->co);
+    trace_qed_aio_complete(s, acb, ret);
+    qed_aio_complete(acb);
+    return ret;
 }
 
 static int qed_co_request(BlockDriverState *bs, int64_t sector_num,
                           QEMUIOVector *qiov, int nb_sectors, int flags)
 {
-    QEDRequestCo co = {
-        .co     = qemu_coroutine_self(),
-        .done   = false,
-    };
-    QEDAIOCB *acb = qemu_aio_get(&qed_aiocb_info, bs, qed_co_request_cb, &co);
-
-    trace_qed_aio_setup(bs->opaque, acb, sector_num, nb_sectors, &co, flags);
+    QEDAIOCB *acb;
+    int ret;
 
-    acb->flags = flags;
-    acb->qiov = qiov;
-    acb->qiov_offset = 0;
-    acb->cur_pos = (uint64_t)sector_num * BDRV_SECTOR_SIZE;
-    acb->end_pos = acb->cur_pos + nb_sectors * BDRV_SECTOR_SIZE;
-    acb->backing_qiov = NULL;
-    acb->request.l2_table = NULL;
+    acb = g_new(QEDAIOCB, 1);
+    *acb = (QEDAIOCB) {
+        .bs         = bs,
+        .cur_pos    = (uint64_t) sector_num * BDRV_SECTOR_SIZE,
+        .end_pos    = (sector_num + nb_sectors) * BDRV_SECTOR_SIZE,
+        .qiov       = qiov,
+        .flags      = flags,
+    };
     qemu_iovec_init(&acb->cur_qiov, qiov->niov);
 
-    /* Start request */
-    qed_aio_start_io(acb);
+    trace_qed_aio_setup(bs->opaque, acb, sector_num, nb_sectors, NULL, flags);
 
-    if (!co.done) {
-        qemu_coroutine_yield();
-    }
+    /* Start request */
+    ret = qed_aio_next_io(acb);
 
-    return co.ret;
+    g_free(acb);
+    return ret;
 }
 
 static int bdrv_qed_co_readv(BlockDriverState *bs, int64_t sector_num,
diff --git a/block/qed.h b/block/qed.h
index 37558e4..fb80943 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -129,8 +129,7 @@ enum {
 };
 
 typedef struct QEDAIOCB {
-    BlockAIOCB common;
-    int bh_ret;                     /* final return status for completion bh */
+    BlockDriverState *bs;
     QSIMPLEQ_ENTRY(QEDAIOCB) next;  /* next request */
     int flags;                      /* QED_AIOCB_* bits ORed together */
     uint64_t end_pos;               /* request end on block device, in bytes */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 27/29] qed: Simplify request handling

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:22:08PM +0200, Kevin Wolf wrote:
> Now that we process a request in the same coroutine from beginning to
> end and don't drop out of it any more, we can look like a proper
> coroutine-based driver and simply call qed_aio_next_io() and get a
> return value from it instead of spawning an additional coroutine that
> reenters the parent when it's done.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 106 ++++++++++++++++--------------------------------------------
>  block/qed.h |   3 +-
>  2 files changed, 28 insertions(+), 81 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 28/29] qed: Use a coroutine for need_check_timer

[Kevin Wolf]

This fixes the last place where we degraded from AIO to actual blocking
synchronous I/O requests. Putting it into a coroutine means that instead
of blocking, the coroutine simply yields while doing I/O.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qed.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index d3f7d0c..20e81a0 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -264,11 +264,23 @@ static void qed_unplug_allocating_write_reqs(BDRVQEDState *s)
     qemu_co_enter_next(&s->allocating_write_reqs);
 }
 
-static void qed_clear_need_check(void *opaque, int ret)
+static void qed_need_check_timer_entry(void *opaque)
 {
     BDRVQEDState *s = opaque;
+    int ret;
 
-    if (ret) {
+    /* The timer should only fire when allocating writes have drained */
+    assert(!s->allocating_acb);
+
+    trace_qed_need_check_timer_cb(s);
+
+    qed_acquire(s);
+    qed_plug_allocating_write_reqs(s);
+
+    /* Ensure writes are on disk before clearing flag */
+    ret = bdrv_co_flush(s->bs->file->bs);
+    qed_release(s);
+    if (ret < 0) {
         goto out;
     }
 
@@ -276,7 +288,7 @@ static void qed_clear_need_check(void *opaque, int ret)
     ret = qed_write_header(s);
     (void) ret;
 
-    ret = bdrv_flush(s->bs);
+    ret = bdrv_co_flush(s->bs);
     (void) ret;
 
 out:
@@ -285,19 +297,8 @@ out:
 
 static void qed_need_check_timer_cb(void *opaque)
 {
-    BDRVQEDState *s = opaque;
-
-    /* The timer should only fire when allocating writes have drained */
-    assert(!s->allocating_acb);
-
-    trace_qed_need_check_timer_cb(s);
-
-    qed_acquire(s);
-    qed_plug_allocating_write_reqs(s);
-
-    /* Ensure writes are on disk before clearing flag */
-    bdrv_aio_flush(s->bs->file->bs, qed_clear_need_check, s);
-    qed_release(s);
+    Coroutine *co = qemu_coroutine_create(qed_need_check_timer_entry, opaque);
+    qemu_coroutine_enter(co);
 }
 
 void qed_acquire(BDRVQEDState *s)
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-block] [PATCH 28/29] qed: Use a coroutine for need_check_timer

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:22:09PM +0200, Kevin Wolf wrote:
> This fixes the last place where we degraded from AIO to actual blocking
> synchronous I/O requests. Putting it into a coroutine means that instead
> of blocking, the coroutine simply yields while doing I/O.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qed.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/block/qed.c b/block/qed.c
> index d3f7d0c..20e81a0 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -264,11 +264,23 @@ static void qed_unplug_allocating_write_reqs(BDRVQEDState *s)
>      qemu_co_enter_next(&s->allocating_write_reqs);
>  }
>  
> -static void qed_clear_need_check(void *opaque, int ret)
> +static void qed_need_check_timer_entry(void *opaque)

Missing coroutine_fn.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 29/29] block: Remove bdrv_aio_readv/writev_flush()

[Kevin Wolf]

These functions are unused now.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c            | 171 --------------------------------------------------
 include/block/block.h |   8 ---
 2 files changed, 179 deletions(-)

diff --git a/block/io.c b/block/io.c
index fdd7485..1fec424 100644
--- a/block/io.c
+++ b/block/io.c
@@ -33,14 +33,6 @@
 
 #define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */
 
-static BlockAIOCB *bdrv_co_aio_prw_vector(BdrvChild *child,
-                                          int64_t offset,
-                                          QEMUIOVector *qiov,
-                                          BdrvRequestFlags flags,
-                                          BlockCompletionFunc *cb,
-                                          void *opaque,
-                                          bool is_write);
-static void coroutine_fn bdrv_co_do_rw(void *opaque);
 static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
     int64_t offset, int count, BdrvRequestFlags flags);
 
@@ -2083,28 +2075,6 @@ int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
 /**************************************************************/
 /* async I/Os */
 
-BlockAIOCB *bdrv_aio_readv(BdrvChild *child, int64_t sector_num,
-                           QEMUIOVector *qiov, int nb_sectors,
-                           BlockCompletionFunc *cb, void *opaque)
-{
-    trace_bdrv_aio_readv(child->bs, sector_num, nb_sectors, opaque);
-
-    assert(nb_sectors << BDRV_SECTOR_BITS == qiov->size);
-    return bdrv_co_aio_prw_vector(child, sector_num << BDRV_SECTOR_BITS, qiov,
-                                  0, cb, opaque, false);
-}
-
-BlockAIOCB *bdrv_aio_writev(BdrvChild *child, int64_t sector_num,
-                            QEMUIOVector *qiov, int nb_sectors,
-                            BlockCompletionFunc *cb, void *opaque)
-{
-    trace_bdrv_aio_writev(child->bs, sector_num, nb_sectors, opaque);
-
-    assert(nb_sectors << BDRV_SECTOR_BITS == qiov->size);
-    return bdrv_co_aio_prw_vector(child, sector_num << BDRV_SECTOR_BITS, qiov,
-                                  0, cb, opaque, true);
-}
-
 void bdrv_aio_cancel(BlockAIOCB *acb)
 {
     qemu_aio_ref(acb);
@@ -2137,147 +2107,6 @@ void bdrv_aio_cancel_async(BlockAIOCB *acb)
 }
 
 /**************************************************************/
-/* async block device emulation */
-
-typedef struct BlockRequest {
-    union {
-        /* Used during read, write, trim */
-        struct {
-            int64_t offset;
-            int bytes;
-            int flags;
-            QEMUIOVector *qiov;
-        };
-        /* Used during ioctl */
-        struct {
-            int req;
-            void *buf;
-        };
-    };
-    BlockCompletionFunc *cb;
-    void *opaque;
-
-    int error;
-} BlockRequest;
-
-typedef struct BlockAIOCBCoroutine {
-    BlockAIOCB common;
-    BdrvChild *child;
-    BlockRequest req;
-    bool is_write;
-    bool need_bh;
-    bool *done;
-} BlockAIOCBCoroutine;
-
-static const AIOCBInfo bdrv_em_co_aiocb_info = {
-    .aiocb_size         = sizeof(BlockAIOCBCoroutine),
-};
-
-static void bdrv_co_complete(BlockAIOCBCoroutine *acb)
-{
-    if (!acb->need_bh) {
-        bdrv_dec_in_flight(acb->common.bs);
-        acb->common.cb(acb->common.opaque, acb->req.error);
-        qemu_aio_unref(acb);
-    }
-}
-
-static void bdrv_co_em_bh(void *opaque)
-{
-    BlockAIOCBCoroutine *acb = opaque;
-
-    assert(!acb->need_bh);
-    bdrv_co_complete(acb);
-}
-
-static void bdrv_co_maybe_schedule_bh(BlockAIOCBCoroutine *acb)
-{
-    acb->need_bh = false;
-    if (acb->req.error != -EINPROGRESS) {
-        BlockDriverState *bs = acb->common.bs;
-
-        aio_bh_schedule_oneshot(bdrv_get_aio_context(bs), bdrv_co_em_bh, acb);
-    }
-}
-
-/* Invoke bdrv_co_do_readv/bdrv_co_do_writev */
-static void coroutine_fn bdrv_co_do_rw(void *opaque)
-{
-    BlockAIOCBCoroutine *acb = opaque;
-
-    if (!acb->is_write) {
-        acb->req.error = bdrv_co_preadv(acb->child, acb->req.offset,
-            acb->req.qiov->size, acb->req.qiov, acb->req.flags);
-    } else {
-        acb->req.error = bdrv_co_pwritev(acb->child, acb->req.offset,
-            acb->req.qiov->size, acb->req.qiov, acb->req.flags);
-    }
-
-    bdrv_co_complete(acb);
-}
-
-static BlockAIOCB *bdrv_co_aio_prw_vector(BdrvChild *child,
-                                          int64_t offset,
-                                          QEMUIOVector *qiov,
-                                          BdrvRequestFlags flags,
-                                          BlockCompletionFunc *cb,
-                                          void *opaque,
-                                          bool is_write)
-{
-    Coroutine *co;
-    BlockAIOCBCoroutine *acb;
-
-    /* Matched by bdrv_co_complete's bdrv_dec_in_flight.  */
-    bdrv_inc_in_flight(child->bs);
-
-    acb = qemu_aio_get(&bdrv_em_co_aiocb_info, child->bs, cb, opaque);
-    acb->child = child;
-    acb->need_bh = true;
-    acb->req.error = -EINPROGRESS;
-    acb->req.offset = offset;
-    acb->req.qiov = qiov;
-    acb->req.flags = flags;
-    acb->is_write = is_write;
-
-    co = qemu_coroutine_create(bdrv_co_do_rw, acb);
-    bdrv_coroutine_enter(child->bs, co);
-
-    bdrv_co_maybe_schedule_bh(acb);
-    return &acb->common;
-}
-
-static void coroutine_fn bdrv_aio_flush_co_entry(void *opaque)
-{
-    BlockAIOCBCoroutine *acb = opaque;
-    BlockDriverState *bs = acb->common.bs;
-
-    acb->req.error = bdrv_co_flush(bs);
-    bdrv_co_complete(acb);
-}
-
-BlockAIOCB *bdrv_aio_flush(BlockDriverState *bs,
-        BlockCompletionFunc *cb, void *opaque)
-{
-    trace_bdrv_aio_flush(bs, opaque);
-
-    Coroutine *co;
-    BlockAIOCBCoroutine *acb;
-
-    /* Matched by bdrv_co_complete's bdrv_dec_in_flight.  */
-    bdrv_inc_in_flight(bs);
-
-    acb = qemu_aio_get(&bdrv_em_co_aiocb_info, bs, cb, opaque);
-    acb->need_bh = true;
-    acb->req.error = -EINPROGRESS;
-
-    co = qemu_coroutine_create(bdrv_aio_flush_co_entry, acb);
-    bdrv_coroutine_enter(bs, co);
-
-    bdrv_co_maybe_schedule_bh(acb);
-    return &acb->common;
-}
-
-/**************************************************************/
 /* Coroutine block device emulation */
 
 typedef struct FlushCo {
diff --git a/include/block/block.h b/include/block/block.h
index 9b355e9..c2dc243 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -353,14 +353,6 @@ BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
                                         const char *node_name, Error **errp);
 
 /* async block I/O */
-BlockAIOCB *bdrv_aio_readv(BdrvChild *child, int64_t sector_num,
-                           QEMUIOVector *iov, int nb_sectors,
-                           BlockCompletionFunc *cb, void *opaque);
-BlockAIOCB *bdrv_aio_writev(BdrvChild *child, int64_t sector_num,
-                            QEMUIOVector *iov, int nb_sectors,
-                            BlockCompletionFunc *cb, void *opaque);
-BlockAIOCB *bdrv_aio_flush(BlockDriverState *bs,
-                           BlockCompletionFunc *cb, void *opaque);
 void bdrv_aio_cancel(BlockAIOCB *acb);
 void bdrv_aio_cancel_async(BlockAIOCB *acb);
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 29/29] block: Remove bdrv_aio_readv/writev_flush()

[Stefan Hajnoczi]

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

On Fri, May 26, 2017 at 10:22:10PM +0200, Kevin Wolf wrote:
> @@ -2083,28 +2075,6 @@ int bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
>  /**************************************************************/
>  /* async I/Os */
>  
> -BlockAIOCB *bdrv_aio_readv(BdrvChild *child, int64_t sector_num,
> -                           QEMUIOVector *qiov, int nb_sectors,
> -                           BlockCompletionFunc *cb, void *opaque)
> -{
> -    trace_bdrv_aio_readv(child->bs, sector_num, nb_sectors, opaque);

Please remove unused trace events from the trace-events file.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 00/29] qed: Convert to coroutines

[Paolo Bonzini]

On 26/05/2017 22:21, Kevin Wolf wrote:
> The qed block driver is one of the last remaining block drivers that use the
> AIO callback style interfaces. This series converts it to the coroutine model
> that other drivers are using and removes some AIO functions from the block
> layer API afterwards.
> 
> If this isn't compelling enough, the diffstat should speak for itself.
> 
> This series is relatively long, but it consists mostly of mechanical
> conversions of one function per patch, so it should be easy to review.

Looks great, just a few notes:

- There are a couple "Callback from qed_find_cluster()" comments that 
are obsolete.

- qed_acquire/qed_release can be removed as you inline stuff, but this
should not cause bugs so you can either do it as a final patch or let
me remove it later.

- coroutine_fn annotations are missing.  Also can be done as a final
patch.  It's a bit ugly that L1/L2 table access can happen both in a
coroutine (for regular I/O) and outside (for create/check).

- qed_is_allocated_cb can be inlined and QEDIsAllocatedCB removed.  Of
course a lot more cleanup could be done (I will do a couple more such
changes when adding a CoMutex to make the driver thread-safe) but this
one is pretty low-hanging fruit and seems to be in line with the rest
of the patches.

- qed_co_request doesn't need g_new for the QEDAIOCB.

Paolo

> Kevin Wolf (29):
>   qed: Use bottom half to resume waiting requests
>   qed: Make qed_read_table() synchronous
>   qed: Remove callback from qed_read_table()
>   qed: Remove callback from qed_read_l2_table()
>   qed: Remove callback from qed_find_cluster()
>   qed: Make qed_read_backing_file() synchronous
>   qed: Make qed_copy_from_backing_file() synchronous
>   qed: Remove callback from qed_copy_from_backing_file()
>   qed: Make qed_write_header() synchronous
>   qed: Remove callback from qed_write_header()
>   qed: Make qed_write_table() synchronous
>   qed: Remove GenericCB
>   qed: Remove callback from qed_write_table()
>   qed: Make qed_aio_read_data() synchronous
>   qed: Make qed_aio_write_main() synchronous
>   qed: Inline qed_commit_l2_update()
>   qed: Add return value to qed_aio_write_l1_update()
>   qed: Add return value to qed_aio_write_l2_update()
>   qed: Add return value to qed_aio_write_main()
>   qed: Add return value to qed_aio_write_cow()
>   qed: Add return value to qed_aio_write_inplace/alloc()
>   qed: Add return value to qed_aio_read/write_data()
>   qed: Remove ret argument from qed_aio_next_io()
>   qed: Remove recursion in qed_aio_next_io()
>   qed: Implement .bdrv_co_readv/writev
>   qed: Use CoQueue for serialising allocations
>   qed: Simplify request handling
>   qed: Use a coroutine for need_check_timer
>   block: Remove bdrv_aio_readv/writev_flush()
> 
>  block/Makefile.objs   |   2 +-
>  block/io.c            | 171 -----------
>  block/qed-cluster.c   | 123 ++++----
>  block/qed-gencb.c     |  33 ---
>  block/qed-table.c     | 261 ++++++-----------
>  block/qed.c           | 763 +++++++++++++++++++-------------------------------
>  block/qed.h           |  53 +---
>  include/block/block.h |   8 -
>  8 files changed, 432 insertions(+), 982 deletions(-)
>  delete mode 100644 block/qed-gencb.c
> 

Re: [Qemu-devel] [Qemu-block] [PATCH 00/29] qed: Convert to coroutines

[Kevin Wolf]

Am 29.05.2017 um 13:22 hat Paolo Bonzini geschrieben:
> On 26/05/2017 22:21, Kevin Wolf wrote:
> > The qed block driver is one of the last remaining block drivers that use the
> > AIO callback style interfaces. This series converts it to the coroutine model
> > that other drivers are using and removes some AIO functions from the block
> > layer API afterwards.
> > 
> > If this isn't compelling enough, the diffstat should speak for itself.
> > 
> > This series is relatively long, but it consists mostly of mechanical
> > conversions of one function per patch, so it should be easy to review.
> 
> Looks great, just a few notes:
> 
> - There are a couple "Callback from qed_find_cluster()" comments that 
> are obsolete.

Fixed.

> - qed_acquire/qed_release can be removed as you inline stuff, but this
> should not cause bugs so you can either do it as a final patch or let
> me remove it later.

To be honest, I don't completely understand what they are protecting in
the first place. The places where they are look quite strange to me. So
I tried to simply leave them alone.

What is the reason that we can remove them when we inline stuff?
Shouldn't the inlining be semantically equivalent?

I guess the answer is important for the case in patch 5, where Stefan
commented that I moved some code out of the lock.

> - coroutine_fn annotations are missing.  Also can be done as a final
> patch.  It's a bit ugly that L1/L2 table access can happen both in a
> coroutine (for regular I/O) and outside (for create/check).

Create is coroutine-based these days. Maybe we should convert check,
too.

> - qed_is_allocated_cb can be inlined and QEDIsAllocatedCB removed.  Of
> course a lot more cleanup could be done (I will do a couple more such
> changes when adding a CoMutex to make the driver thread-safe) but this
> one is pretty low-hanging fruit and seems to be in line with the rest
> of the patches.
> 
> - qed_co_request doesn't need g_new for the QEDAIOCB.

I stopped intentionally when the thing was fully coroutine based
because I thought the series is already long enough. There are many more
cleanups that could be done (e.g. QEDAIOCB should be completely
avoided), but let's do that on top.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH 00/29] qed: Convert to coroutines

[Paolo Bonzini]

On 01/06/2017 18:28, Kevin Wolf wrote:
>> - qed_acquire/qed_release can be removed as you inline stuff, but this
>> should not cause bugs so you can either do it as a final patch or let
>> me remove it later.
> To be honest, I don't completely understand what they are protecting in
> the first place. The places where they are look quite strange to me. So
> I tried to simply leave them alone.
> 
> What is the reason that we can remove them when we inline stuff?
> Shouldn't the inlining be semantically equivalent?

You're right, they can be removed when going from callback to direct
call.  Callbacks are invoked without the AioContext acquire (aio_co_wake
does it for the callbacks).

>> - coroutine_fn annotations are missing.  Also can be done as a final
>> patch.  It's a bit ugly that L1/L2 table access can happen both in a
>> coroutine (for regular I/O) and outside (for create/check).
>
> Create is coroutine-based these days. Maybe we should convert check,
> too.

Good idea, separate series. :)

>> - qed_is_allocated_cb can be inlined and QEDIsAllocatedCB removed.  Of
>> course a lot more cleanup could be done (I will do a couple more such
>> changes when adding a CoMutex to make the driver thread-safe) but this
>> one is pretty low-hanging fruit and seems to be in line with the rest
>> of the patches.
>>
>> - qed_co_request doesn't need g_new for the QEDAIOCB.
>
> I stopped intentionally when the thing was fully coroutine based
> because I thought the series is already long enough. There are many more
> cleanups that could be done (e.g. QEDAIOCB should be completely
> avoided), but let's do that on top.

Agreed about qed_is_allocated_cb.  Regarding qed_co_request you're
already doing an almost complete rewrite of it in patch 27, and making
QEDAIOCB not heap allocated would be a very small change removing half a
dozen lines of code.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH 00/29] qed: Convert to coroutines

[Kevin Wolf]

Am 01.06.2017 um 18:40 hat Paolo Bonzini geschrieben:
> On 01/06/2017 18:28, Kevin Wolf wrote:
> >> - qed_acquire/qed_release can be removed as you inline stuff, but this
> >> should not cause bugs so you can either do it as a final patch or let
> >> me remove it later.
> > To be honest, I don't completely understand what they are protecting in
> > the first place. The places where they are look quite strange to me. So
> > I tried to simply leave them alone.
> > 
> > What is the reason that we can remove them when we inline stuff?
> > Shouldn't the inlining be semantically equivalent?
> 
> You're right, they can be removed when going from callback to direct
> call.  Callbacks are invoked without the AioContext acquire (aio_co_wake
> does it for the callbacks).

So if we take qed_read_table_cb() for example:

    qed_acquire(s);
    for (i = 0; i < noffsets; i++) {
        table->offsets[i] = le64_to_cpu(table->offsets[i]);
    }
    qed_release(s);

First of all, I don't see what it protects. If we wanted to avoid that
someone else sees table->offsets with wrong endianness, we would be
taking the lock much too late. And if nobody else knows about the table
yet, what is there to be locked?

But anyway, given your explanation that acquiring the AioContext lock is
getting replaced by coroutine magic, the qed_acquire/release() pair
actually can't be removed in patch 2 when the callback is converted to a
direct call, but only when the whole call path between .bdrv_aio_readv/
writev and this specific callback is converted, so that we never drop
out of coroutine context before reaching this code. Correct?

This happens only very late in the series, so it probably also means
that patch 5 is indeed wrong because it removes the lock too early?

> >> - coroutine_fn annotations are missing.  Also can be done as a final
> >> patch.  It's a bit ugly that L1/L2 table access can happen both in a
> >> coroutine (for regular I/O) and outside (for create/check).
> >
> > Create is coroutine-based these days. Maybe we should convert check,
> > too.
> 
> Good idea, separate series. :)
> 
> >> - qed_is_allocated_cb can be inlined and QEDIsAllocatedCB removed.  Of
> >> course a lot more cleanup could be done (I will do a couple more such
> >> changes when adding a CoMutex to make the driver thread-safe) but this
> >> one is pretty low-hanging fruit and seems to be in line with the rest
> >> of the patches.
> >>
> >> - qed_co_request doesn't need g_new for the QEDAIOCB.
> >
> > I stopped intentionally when the thing was fully coroutine based
> > because I thought the series is already long enough. There are many more
> > cleanups that could be done (e.g. QEDAIOCB should be completely
> > avoided), but let's do that on top.
> 
> Agreed about qed_is_allocated_cb.  Regarding qed_co_request you're
> already doing an almost complete rewrite of it in patch 27, and making
> QEDAIOCB not heap allocated would be a very small change removing half a
> dozen lines of code.

You're right, wasn't aware of this change any more. :-)

I'll do that then.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH 00/29] qed: Convert to coroutines

[Paolo Bonzini]

On 01/06/2017 19:08, Kevin Wolf wrote:
> Am 01.06.2017 um 18:40 hat Paolo Bonzini geschrieben:
>> On 01/06/2017 18:28, Kevin Wolf wrote:
>>>> - qed_acquire/qed_release can be removed as you inline stuff, but this
>>>> should not cause bugs so you can either do it as a final patch or let
>>>> me remove it later.
>>> To be honest, I don't completely understand what they are protecting in
>>> the first place. The places where they are look quite strange to me. So
>>> I tried to simply leave them alone.
>>>
>>> What is the reason that we can remove them when we inline stuff?
>>> Shouldn't the inlining be semantically equivalent?
>>
>> You're right, they can be removed when going from callback to direct
>> call.  Callbacks are invoked without the AioContext acquire (aio_co_wake
>> does it for the callbacks).
> 
> So if we take qed_read_table_cb() for example:
> 
>     qed_acquire(s);
>     for (i = 0; i < noffsets; i++) {
>         table->offsets[i] = le64_to_cpu(table->offsets[i]);
>     }
>     qed_release(s);
> 
> First of all, I don't see what it protects. If we wanted to avoid that
> someone else sees table->offsets with wrong endianness, we would be
> taking the lock much too late. And if nobody else knows about the table
> yet, what is there to be locked?

That is the product of a mechanical conversion where all callbacks grew
a qed_acquire/qed_release pair (commit b9e413dd37, "block: explicitly
acquire aiocontext in aio callbacks that need it", 2017-02-21).

In this case:

        qed_acquire(s);
        bdrv_aio_flush(write_table_cb->s->bs, qed_write_table_cb,
                       write_table_cb);
        qed_release(s);

the AioContext protects write_table_cb->s->bs.

> But anyway, given your explanation that acquiring the AioContext lock is
> getting replaced by coroutine magic, the qed_acquire/release() pair
> actually can't be removed in patch 2 when the callback is converted to a
> direct call, but only when the whole call path between .bdrv_aio_readv/
> writev and this specific callback is converted, so that we never drop
> out of coroutine context before reaching this code. Correct?

Yes.

> This happens only very late in the series, so it probably also means
> that patch 5 is indeed wrong because it removes the lock too early?

bdrv_qed_co_get_block_status is entirely in coroutine context, so I
think that one is fine.

Paolo