[PATCH v2 0/9] Still more coroutine and various fixes in block layer

[PATCH v2 0/9] Still more coroutine and various fixes in block layer

[Emanuele Giuseppe Esposito]

This is a dump of all minor coroutine-related fixes found while looking
around and testing various things in the QEMU block layer.

Patches aim to:
- add missing coroutine_fn annotation to the functions
- simplify to avoid the typical "if in coroutine: fn()
  // else create_coroutine(fn)" already present in generated_co_wraper
  functions.
- make sure that if a BlockDriver callback is defined as coroutine_fn, then
  it is always running in a coroutine.

Emanuele
---
v2:
* clarified commit message in patches 2/3/6 on why we add coroutine_fn

Emanuele Giuseppe Esposito (9):
  block: call bdrv_co_drain_begin in a coroutine
  block-copy: add missing coroutine_fn annotations
  nbd/server.c: add missing coroutine_fn annotations
  block-backend: replace bdrv_*_above with blk_*_above
  block: distinguish between bdrv_create running in coroutine and not
  block/vmdk: add missing coroutine_fn annotations
  block: bdrv_create_file is a coroutine_fn
  block: bdrv_create is never called in non-coroutine context
  block/dirty-bitmap: remove unnecessary qemu_in_coroutine() case

 block.c                            | 111 +++++++++++++++++------------
 block/block-backend.c              |  21 ++++++
 block/block-copy.c                 |  15 ++--
 block/commit.c                     |   4 +-
 block/dirty-bitmap.c               |  66 ++++++++---------
 block/vmdk.c                       |  36 +++++-----
 include/block/block-global-state.h |   3 +-
 include/sysemu/block-backend-io.h  |   9 +++
 nbd/server.c                       |  43 +++++------
 qemu-img.c                         |   4 +-
 10 files changed, 182 insertions(+), 130 deletions(-)

-- 
2.31.1

[PATCH v2 1/9] block: call bdrv_co_drain_begin in a coroutine

[Emanuele Giuseppe Esposito]

It seems that bdrv_open_driver() forgot to create a coroutine
where to call bs->drv->bdrv_co_drain_begin(), a callback
marked as coroutine_fn.

Because there is no active I/O at this point, the coroutine
should end right after entering, so the caller does not need
to poll until it is finished.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 5311b21f8e..d2b2800039 100644
--- a/block.c
+++ b/block.c
@@ -1637,12 +1637,34 @@ out:
     g_free(gen_node_name);
 }
 
+typedef struct DrainCo {
+    BlockDriverState *bs;
+    int ret;
+} DrainCo;
+
+static void coroutine_fn bdrv_co_drain_begin(void *opaque)
+{
+    int i;
+    DrainCo *co = opaque;
+    BlockDriverState *bs = co->bs;
+
+    for (i = 0; i < bs->quiesce_counter; i++) {
+        bs->drv->bdrv_co_drain_begin(bs);
+    }
+    co->ret = 0;
+}
+
 static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
                             const char *node_name, QDict *options,
                             int open_flags, Error **errp)
 {
     Error *local_err = NULL;
-    int i, ret;
+    int ret;
+    Coroutine *co;
+    DrainCo dco = {
+        .bs = bs,
+        .ret = NOT_DONE,
+    };
     GLOBAL_STATE_CODE();
 
     bdrv_assign_node_name(bs, node_name, &local_err);
@@ -1690,10 +1712,14 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
     assert(bdrv_min_mem_align(bs) != 0);
     assert(is_power_of_2(bs->bl.request_alignment));
 
-    for (i = 0; i < bs->quiesce_counter; i++) {
-        if (drv->bdrv_co_drain_begin) {
-            drv->bdrv_co_drain_begin(bs);
-        }
+    if (drv->bdrv_co_drain_begin) {
+        co = qemu_coroutine_create(bdrv_co_drain_begin, &dco);
+        qemu_coroutine_enter(co);
+        /*
+         * There should be no reason for drv->bdrv_co_drain_begin to wait at
+         * this point, because the device does not have any active I/O.
+         */
+        assert(dco.ret != NOT_DONE);
     }
 
     return 0;
-- 
2.31.1

[PATCH v2 2/9] block-copy: add missing coroutine_fn annotations

[Emanuele Giuseppe Esposito]

These functions end up calling bdrv_common_block_status_above(), a
generated_co_wrapper function.
In addition, they also happen to be always called in coroutine context,
meaning all callers are coroutine_fn.
This means that the g_c_w function will enter the qemu_in_coroutine()
case and eventually suspend (or in other words call qemu_coroutine_yield()).
Therefore we need to mark such functions coroutine_fn too.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/block-copy.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index bb947afdda..f33ab1d0b6 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -577,8 +577,9 @@ static coroutine_fn int block_copy_task_entry(AioTask *task)
     return ret;
 }
 
-static int block_copy_block_status(BlockCopyState *s, int64_t offset,
-                                   int64_t bytes, int64_t *pnum)
+static coroutine_fn int block_copy_block_status(BlockCopyState *s,
+                                                int64_t offset,
+                                                int64_t bytes, int64_t *pnum)
 {
     int64_t num;
     BlockDriverState *base;
@@ -613,8 +614,9 @@ static int block_copy_block_status(BlockCopyState *s, int64_t offset,
  * Check if the cluster starting at offset is allocated or not.
  * return via pnum the number of contiguous clusters sharing this allocation.
  */
-static int block_copy_is_cluster_allocated(BlockCopyState *s, int64_t offset,
-                                           int64_t *pnum)
+static int coroutine_fn block_copy_is_cluster_allocated(BlockCopyState *s,
+                                                        int64_t offset,
+                                                        int64_t *pnum)
 {
     BlockDriverState *bs = s->source->bs;
     int64_t count, total_count = 0;
@@ -669,8 +671,9 @@ void block_copy_reset(BlockCopyState *s, int64_t offset, int64_t bytes)
  * @return 0 when the cluster at @offset was unallocated,
  *         1 otherwise, and -ret on error.
  */
-int64_t block_copy_reset_unallocated(BlockCopyState *s,
-                                     int64_t offset, int64_t *count)
+int64_t coroutine_fn block_copy_reset_unallocated(BlockCopyState *s,
+                                                  int64_t offset,
+                                                  int64_t *count)
 {
     int ret;
     int64_t clusters, bytes;
-- 
2.31.1

[PATCH v2 3/9] nbd/server.c: add missing coroutine_fn annotations

[Emanuele Giuseppe Esposito]

These functions end up calling bdrv_*() implemented as generated_co_wrapper
functions.
In addition, they also happen to be always called in coroutine context,
meaning all callers are coroutine_fn.
This means that the g_c_w function will enter the qemu_in_coroutine()
case and eventually suspend (or in other words call qemu_coroutine_yield()).
Therefore we need to mark such functions coroutine_fn too.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 nbd/server.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index ada16089f3..e2eec0cf46 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2141,8 +2141,9 @@ static int nbd_extent_array_add(NBDExtentArray *ea,
     return 0;
 }
 
-static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
-                                  uint64_t bytes, NBDExtentArray *ea)
+static int coroutine_fn blockstatus_to_extents(BlockDriverState *bs,
+                                               uint64_t offset, uint64_t bytes,
+                                               NBDExtentArray *ea)
 {
     while (bytes) {
         uint32_t flags;
@@ -2168,8 +2169,9 @@ static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
     return 0;
 }
 
-static int blockalloc_to_extents(BlockDriverState *bs, uint64_t offset,
-                                 uint64_t bytes, NBDExtentArray *ea)
+static int coroutine_fn blockalloc_to_extents(BlockDriverState *bs,
+                                              uint64_t offset, uint64_t bytes,
+                                              NBDExtentArray *ea)
 {
     while (bytes) {
         int64_t num;
@@ -2220,11 +2222,12 @@ static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
 }
 
 /* Get block status from the exported device and send it to the client */
-static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
-                                    BlockDriverState *bs, uint64_t offset,
-                                    uint32_t length, bool dont_fragment,
-                                    bool last, uint32_t context_id,
-                                    Error **errp)
+static int
+coroutine_fn nbd_co_send_block_status(NBDClient *client, uint64_t handle,
+                                      BlockDriverState *bs, uint64_t offset,
+                                      uint32_t length, bool dont_fragment,
+                                      bool last, uint32_t context_id,
+                                      Error **errp)
 {
     int ret;
     unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
-- 
2.31.1

[PATCH v2 4/9] block-backend: replace bdrv_*_above with blk_*_above

[Emanuele Giuseppe Esposito]

Avoid mixing bdrv_* functions with blk_*, so create blk_* counterparts
for:
- bdrv_block_status_above
- bdrv_is_allocated_above

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/block-backend.c             | 21 +++++++++++++++++++++
 block/commit.c                    |  4 ++--
 include/sysemu/block-backend-io.h |  9 +++++++++
 nbd/server.c                      | 28 ++++++++++++++--------------
 qemu-img.c                        |  4 ++--
 5 files changed, 48 insertions(+), 18 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index ec17dc49a9..eb8787a3d9 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1423,6 +1423,27 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
     return blk_co_pwritev_part(blk, offset, bytes, qiov, 0, flags);
 }
 
+int coroutine_fn blk_block_status_above(BlockBackend *blk,
+                                        BlockDriverState *base,
+                                        int64_t offset, int64_t bytes,
+                                        int64_t *pnum, int64_t *map,
+                                        BlockDriverState **file)
+{
+    IO_CODE();
+    return bdrv_block_status_above(blk_bs(blk), base, offset, bytes, pnum, map,
+                                   file);
+}
+
+int coroutine_fn blk_is_allocated_above(BlockBackend *blk,
+                                        BlockDriverState *base,
+                                        bool include_base, int64_t offset,
+                                        int64_t bytes, int64_t *pnum)
+{
+    IO_CODE();
+    return bdrv_is_allocated_above(blk_bs(blk), base, include_base, offset,
+                                   bytes, pnum);
+}
+
 typedef struct BlkRwCo {
     BlockBackend *blk;
     int64_t offset;
diff --git a/block/commit.c b/block/commit.c
index 0029b31944..9d4d908344 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -155,8 +155,8 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
             break;
         }
         /* Copy if allocated above the base */
-        ret = bdrv_is_allocated_above(blk_bs(s->top), s->base_overlay, true,
-                                      offset, COMMIT_BUFFER_SIZE, &n);
+        ret = blk_is_allocated_above(s->top, s->base_overlay, true,
+                                     offset, COMMIT_BUFFER_SIZE, &n);
         copy = (ret > 0);
         trace_commit_one_iteration(s, offset, n, ret);
         if (copy) {
diff --git a/include/sysemu/block-backend-io.h b/include/sysemu/block-backend-io.h
index 50f5aa2e07..a47cb825e5 100644
--- a/include/sysemu/block-backend-io.h
+++ b/include/sysemu/block-backend-io.h
@@ -92,6 +92,15 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
                                    int64_t bytes, BdrvRequestFlags read_flags,
                                    BdrvRequestFlags write_flags);
 
+int coroutine_fn blk_block_status_above(BlockBackend *blk,
+                                        BlockDriverState *base,
+                                        int64_t offset, int64_t bytes,
+                                        int64_t *pnum, int64_t *map,
+                                        BlockDriverState **file);
+int coroutine_fn blk_is_allocated_above(BlockBackend *blk,
+                                        BlockDriverState *base,
+                                        bool include_base, int64_t offset,
+                                        int64_t bytes, int64_t *pnum);
 
 /*
  * "I/O or GS" API functions. These functions can run without
diff --git a/nbd/server.c b/nbd/server.c
index e2eec0cf46..6389b46396 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1991,7 +1991,7 @@ static int coroutine_fn nbd_co_send_structured_error(NBDClient *client,
 }
 
 /* Do a sparse read and send the structured reply to the client.
- * Returns -errno if sending fails. bdrv_block_status_above() failure is
+ * Returns -errno if sending fails. blk_block_status_above() failure is
  * reported to the client, at which point this function succeeds.
  */
 static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
@@ -2007,10 +2007,10 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
 
     while (progress < size) {
         int64_t pnum;
-        int status = bdrv_block_status_above(blk_bs(exp->common.blk), NULL,
-                                             offset + progress,
-                                             size - progress, &pnum, NULL,
-                                             NULL);
+        int status = blk_block_status_above(exp->common.blk, NULL,
+                                            offset + progress,
+                                            size - progress, &pnum, NULL,
+                                            NULL);
         bool final;
 
         if (status < 0) {
@@ -2141,14 +2141,14 @@ static int nbd_extent_array_add(NBDExtentArray *ea,
     return 0;
 }
 
-static int coroutine_fn blockstatus_to_extents(BlockDriverState *bs,
+static int coroutine_fn blockstatus_to_extents(BlockBackend *blk,
                                                uint64_t offset, uint64_t bytes,
                                                NBDExtentArray *ea)
 {
     while (bytes) {
         uint32_t flags;
         int64_t num;
-        int ret = bdrv_block_status_above(bs, NULL, offset, bytes, &num,
+        int ret = blk_block_status_above(blk, NULL, offset, bytes, &num,
                                           NULL, NULL);
 
         if (ret < 0) {
@@ -2169,13 +2169,13 @@ static int coroutine_fn blockstatus_to_extents(BlockDriverState *bs,
     return 0;
 }
 
-static int coroutine_fn blockalloc_to_extents(BlockDriverState *bs,
+static int coroutine_fn blockalloc_to_extents(BlockBackend *blk,
                                               uint64_t offset, uint64_t bytes,
                                               NBDExtentArray *ea)
 {
     while (bytes) {
         int64_t num;
-        int ret = bdrv_is_allocated_above(bs, NULL, false, offset, bytes,
+        int ret = blk_is_allocated_above(blk, NULL, false, offset, bytes,
                                           &num);
 
         if (ret < 0) {
@@ -2224,7 +2224,7 @@ static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
 /* Get block status from the exported device and send it to the client */
 static int
 coroutine_fn nbd_co_send_block_status(NBDClient *client, uint64_t handle,
-                                      BlockDriverState *bs, uint64_t offset,
+                                      BlockBackend *blk, uint64_t offset,
                                       uint32_t length, bool dont_fragment,
                                       bool last, uint32_t context_id,
                                       Error **errp)
@@ -2234,9 +2234,9 @@ coroutine_fn nbd_co_send_block_status(NBDClient *client, uint64_t handle,
     g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents);
 
     if (context_id == NBD_META_ID_BASE_ALLOCATION) {
-        ret = blockstatus_to_extents(bs, offset, length, ea);
+        ret = blockstatus_to_extents(blk, offset, length, ea);
     } else {
-        ret = blockalloc_to_extents(bs, offset, length, ea);
+        ret = blockalloc_to_extents(blk, offset, length, ea);
     }
     if (ret < 0) {
         return nbd_co_send_structured_error(
@@ -2563,7 +2563,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
 
             if (client->export_meta.base_allocation) {
                 ret = nbd_co_send_block_status(client, request->handle,
-                                               blk_bs(exp->common.blk),
+                                               exp->common.blk,
                                                request->from,
                                                request->len, dont_fragment,
                                                !--contexts_remaining,
@@ -2576,7 +2576,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
 
             if (client->export_meta.allocation_depth) {
                 ret = nbd_co_send_block_status(client, request->handle,
-                                               blk_bs(exp->common.blk),
+                                               exp->common.blk,
                                                request->from, request->len,
                                                dont_fragment,
                                                !--contexts_remaining,
diff --git a/qemu-img.c b/qemu-img.c
index ace3adf8ae..35f8ceb6ff 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1730,8 +1730,8 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
         do {
             count = n * BDRV_SECTOR_SIZE;
 
-            ret = bdrv_block_status_above(src_bs, base, offset, count, &count,
-                                          NULL, NULL);
+            ret = bdrv_block_status_above(src_bs, base, offset, count,
+                                          &count, NULL, NULL);
 
             if (ret < 0) {
                 if (s->salvage) {
-- 
2.31.1

[PATCH v2 5/9] block: distinguish between bdrv_create running in coroutine and not

[Emanuele Giuseppe Esposito]

Call two different functions depending on whether bdrv_create
is in coroutine or not, following the same pattern as
generated_co_wrapper functions.

This allows to also call the coroutine function directly,
without using CreateCo or relying in bdrv_create().

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block.c | 74 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 36 insertions(+), 38 deletions(-)

diff --git a/block.c b/block.c
index d2b2800039..0823563e4d 100644
--- a/block.c
+++ b/block.c
@@ -522,66 +522,64 @@ typedef struct CreateCo {
     Error *err;
 } CreateCo;
 
-static void coroutine_fn bdrv_create_co_entry(void *opaque)
+static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
+                                       QemuOpts *opts, Error **errp)
 {
-    Error *local_err = NULL;
     int ret;
+    GLOBAL_STATE_CODE();
+    assert(*errp == NULL);
+
+    if (!drv->bdrv_co_create_opts) {
+        error_setg(errp, "Driver '%s' does not support image creation",
+                   drv->format_name);
+        return -ENOTSUP;
+    }
+
+    ret = drv->bdrv_co_create_opts(drv, filename, opts, errp);
 
+    if (ret < 0 && !*errp) {
+        error_setg_errno(errp, -ret, "Could not create image");
+    }
+
+    return ret;
+}
+
+static void coroutine_fn bdrv_create_co_entry(void *opaque)
+{
     CreateCo *cco = opaque;
-    assert(cco->drv);
     GLOBAL_STATE_CODE();
+    assert(cco->drv);
 
-    ret = cco->drv->bdrv_co_create_opts(cco->drv,
-                                        cco->filename, cco->opts, &local_err);
-    error_propagate(&cco->err, local_err);
-    cco->ret = ret;
+    cco->ret = bdrv_co_create(cco->drv, cco->filename, cco->opts, &cco->err);
 }
 
 int bdrv_create(BlockDriver *drv, const char* filename,
                 QemuOpts *opts, Error **errp)
 {
-    int ret;
-
     GLOBAL_STATE_CODE();
 
-    Coroutine *co;
-    CreateCo cco = {
-        .drv = drv,
-        .filename = g_strdup(filename),
-        .opts = opts,
-        .ret = NOT_DONE,
-        .err = NULL,
-    };
-
-    if (!drv->bdrv_co_create_opts) {
-        error_setg(errp, "Driver '%s' does not support image creation", drv->format_name);
-        ret = -ENOTSUP;
-        goto out;
-    }
-
     if (qemu_in_coroutine()) {
         /* Fast-path if already in coroutine context */
-        bdrv_create_co_entry(&cco);
+        return bdrv_co_create(drv, filename, opts, errp);
     } else {
+        Coroutine *co;
+        CreateCo cco = {
+            .drv = drv,
+            .filename = g_strdup(filename),
+            .opts = opts,
+            .ret = NOT_DONE,
+            .err = NULL,
+        };
+
         co = qemu_coroutine_create(bdrv_create_co_entry, &cco);
         qemu_coroutine_enter(co);
         while (cco.ret == NOT_DONE) {
             aio_poll(qemu_get_aio_context(), true);
         }
+        error_propagate(errp, cco.err);
+        g_free(cco.filename);
+        return cco.ret;
     }
-
-    ret = cco.ret;
-    if (ret < 0) {
-        if (cco.err) {
-            error_propagate(errp, cco.err);
-        } else {
-            error_setg_errno(errp, -ret, "Could not create image");
-        }
-    }
-
-out:
-    g_free(cco.filename);
-    return ret;
 }
 
 /**
-- 
2.31.1

[PATCH v2 6/9] block/vmdk: add missing coroutine_fn annotations

[Emanuele Giuseppe Esposito]

These functions end up calling bdrv_create() implemented as generated_co_wrapper
functions.
In addition, they also happen to be always called in coroutine context,
meaning all callers are coroutine_fn.
This means that the g_c_w function will enter the qemu_in_coroutine()
case and eventually suspend (or in other words call qemu_coroutine_yield()).
Therefore we need to mark such functions coroutine_fn too.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/vmdk.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 26376352b9..0c32bf2e83 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2285,10 +2285,11 @@ exit:
     return ret;
 }
 
-static int vmdk_create_extent(const char *filename, int64_t filesize,
-                              bool flat, bool compress, bool zeroed_grain,
-                              BlockBackend **pbb,
-                              QemuOpts *opts, Error **errp)
+static int coroutine_fn vmdk_create_extent(const char *filename,
+                                           int64_t filesize, bool flat,
+                                           bool compress, bool zeroed_grain,
+                                           BlockBackend **pbb,
+                                           QemuOpts *opts, Error **errp)
 {
     int ret;
     BlockBackend *blk = NULL;
@@ -2366,14 +2367,14 @@ static int filename_decompose(const char *filename, char *path, char *prefix,
  *           non-split format.
  * idx >= 1: get the n-th extent if in a split subformat
  */
-typedef BlockBackend *(*vmdk_create_extent_fn)(int64_t size,
-                                               int idx,
-                                               bool flat,
-                                               bool split,
-                                               bool compress,
-                                               bool zeroed_grain,
-                                               void *opaque,
-                                               Error **errp);
+typedef BlockBackend * coroutine_fn (*vmdk_create_extent_fn)(int64_t size,
+                                                             int idx,
+                                                             bool flat,
+                                                             bool split,
+                                                             bool compress,
+                                                             bool zeroed_grain,
+                                                             void *opaque,
+                                                             Error **errp);
 
 static void vmdk_desc_add_extent(GString *desc,
                                  const char *extent_line_fmt,
@@ -2616,7 +2617,7 @@ typedef struct {
     QemuOpts *opts;
 } VMDKCreateOptsData;
 
-static BlockBackend *vmdk_co_create_opts_cb(int64_t size, int idx,
+static BlockBackend * coroutine_fn vmdk_co_create_opts_cb(int64_t size, int idx,
                                             bool flat, bool split, bool compress,
                                             bool zeroed_grain, void *opaque,
                                             Error **errp)
@@ -2768,10 +2769,11 @@ exit:
     return ret;
 }
 
-static BlockBackend *vmdk_co_create_cb(int64_t size, int idx,
-                                       bool flat, bool split, bool compress,
-                                       bool zeroed_grain, void *opaque,
-                                       Error **errp)
+static BlockBackend * coroutine_fn vmdk_co_create_cb(int64_t size, int idx,
+                                                     bool flat, bool split,
+                                                     bool compress,
+                                                     bool zeroed_grain,
+                                                     void *opaque, Error **errp)
 {
     int ret;
     BlockDriverState *bs;
-- 
2.31.1

[PATCH v2 7/9] block: bdrv_create_file is a coroutine_fn

[Emanuele Giuseppe Esposito]

It is always called in coroutine_fn callbacks, therefore
it can directly call bdrv_co_create().

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block.c                            | 6 ++++--
 include/block/block-global-state.h | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 0823563e4d..c6a80d775c 100644
--- a/block.c
+++ b/block.c
@@ -527,6 +527,7 @@ static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
 {
     int ret;
     GLOBAL_STATE_CODE();
+    assert(qemu_in_coroutine());
     assert(*errp == NULL);
 
     if (!drv->bdrv_co_create_opts) {
@@ -717,7 +718,8 @@ out:
     return ret;
 }
 
-int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
+int coroutine_fn bdrv_create_file(const char *filename, QemuOpts *opts,
+                                  Error **errp)
 {
     QemuOpts *protocol_opts;
     BlockDriver *drv;
@@ -758,7 +760,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
         goto out;
     }
 
-    ret = bdrv_create(drv, filename, protocol_opts, errp);
+    ret = bdrv_co_create(drv, filename, protocol_opts, errp);
 out:
     qemu_opts_del(protocol_opts);
     qobject_unref(qdict);
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 73795a0095..bd461f06a1 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -57,7 +57,8 @@ BlockDriver *bdrv_find_protocol(const char *filename,
 BlockDriver *bdrv_find_format(const char *format_name);
 int bdrv_create(BlockDriver *drv, const char* filename,
                 QemuOpts *opts, Error **errp);
-int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
+int coroutine_fn bdrv_create_file(const char *filename, QemuOpts *opts,
+                                  Error **errp);
 
 BlockDriverState *bdrv_new(void);
 int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
-- 
2.31.1

[PATCH v2 8/9] block: bdrv_create is never called in non-coroutine context

[Emanuele Giuseppe Esposito]

Delete the if case and make sure it won't be called again
in coroutines.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/block.c b/block.c
index c6a80d775c..d03a541e4f 100644
--- a/block.c
+++ b/block.c
@@ -557,30 +557,25 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
 int bdrv_create(BlockDriver *drv, const char* filename,
                 QemuOpts *opts, Error **errp)
 {
+    Coroutine *co;
+    CreateCo cco = {
+        .drv = drv,
+        .filename = g_strdup(filename),
+        .opts = opts,
+        .ret = NOT_DONE,
+        .err = NULL,
+    };
     GLOBAL_STATE_CODE();
+    assert(!qemu_in_coroutine());
 
-    if (qemu_in_coroutine()) {
-        /* Fast-path if already in coroutine context */
-        return bdrv_co_create(drv, filename, opts, errp);
-    } else {
-        Coroutine *co;
-        CreateCo cco = {
-            .drv = drv,
-            .filename = g_strdup(filename),
-            .opts = opts,
-            .ret = NOT_DONE,
-            .err = NULL,
-        };
-
-        co = qemu_coroutine_create(bdrv_create_co_entry, &cco);
-        qemu_coroutine_enter(co);
-        while (cco.ret == NOT_DONE) {
-            aio_poll(qemu_get_aio_context(), true);
-        }
-        error_propagate(errp, cco.err);
-        g_free(cco.filename);
-        return cco.ret;
+    co = qemu_coroutine_create(bdrv_create_co_entry, &cco);
+    qemu_coroutine_enter(co);
+    while (cco.ret == NOT_DONE) {
+        aio_poll(qemu_get_aio_context(), true);
     }
+    error_propagate(errp, cco.err);
+    g_free(cco.filename);
+    return cco.ret;
 }
 
 /**
-- 
2.31.1

[PATCH v2 9/9] block/dirty-bitmap: remove unnecessary qemu_in_coroutine() case

[Emanuele Giuseppe Esposito]

bdrv_can_store_new_dirty_bitmap and bdrv_remove_persistent_dirty_bitmap
check if they are running in a coroutine, directly calling the
coroutine callback if it's the case.
Except that no coroutine calls such functions, therefore that check
can be removed.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/dirty-bitmap.c | 66 +++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 37 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index bf3dc0512a..8092d08261 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -418,24 +418,20 @@ bdrv_co_remove_persistent_dirty_bitmap_entry(void *opaque)
 int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
                                         Error **errp)
 {
-    if (qemu_in_coroutine()) {
-        return bdrv_co_remove_persistent_dirty_bitmap(bs, name, errp);
-    } else {
-        Coroutine *co;
-        BdrvRemovePersistentDirtyBitmapCo s = {
-            .bs = bs,
-            .name = name,
-            .errp = errp,
-            .ret = -EINPROGRESS,
-        };
-
-        co = qemu_coroutine_create(bdrv_co_remove_persistent_dirty_bitmap_entry,
-                                   &s);
-        bdrv_coroutine_enter(bs, co);
-        BDRV_POLL_WHILE(bs, s.ret == -EINPROGRESS);
-
-        return s.ret;
-    }
+    Coroutine *co;
+    BdrvRemovePersistentDirtyBitmapCo s = {
+        .bs = bs,
+        .name = name,
+        .errp = errp,
+        .ret = -EINPROGRESS,
+    };
+    assert(!qemu_in_coroutine());
+    co = qemu_coroutine_create(bdrv_co_remove_persistent_dirty_bitmap_entry,
+                                &s);
+    bdrv_coroutine_enter(bs, co);
+    BDRV_POLL_WHILE(bs, s.ret == -EINPROGRESS);
+
+    return s.ret;
 }
 
 bool
@@ -494,25 +490,21 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
                                      uint32_t granularity, Error **errp)
 {
     IO_CODE();
-    if (qemu_in_coroutine()) {
-        return bdrv_co_can_store_new_dirty_bitmap(bs, name, granularity, errp);
-    } else {
-        Coroutine *co;
-        BdrvCanStoreNewDirtyBitmapCo s = {
-            .bs = bs,
-            .name = name,
-            .granularity = granularity,
-            .errp = errp,
-            .in_progress = true,
-        };
-
-        co = qemu_coroutine_create(bdrv_co_can_store_new_dirty_bitmap_entry,
-                                   &s);
-        bdrv_coroutine_enter(bs, co);
-        BDRV_POLL_WHILE(bs, s.in_progress);
-
-        return s.ret;
-    }
+    Coroutine *co;
+    BdrvCanStoreNewDirtyBitmapCo s = {
+        .bs = bs,
+        .name = name,
+        .granularity = granularity,
+        .errp = errp,
+        .in_progress = true,
+    };
+    assert(!qemu_in_coroutine());
+    co = qemu_coroutine_create(bdrv_co_can_store_new_dirty_bitmap_entry,
+                                &s);
+    bdrv_coroutine_enter(bs, co);
+    BDRV_POLL_WHILE(bs, s.in_progress);
+
+    return s.ret;
 }
 
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
-- 
2.31.1

Re: [PATCH v2 0/9] Still more coroutine and various fixes in block layer

[Paolo Bonzini]

On 11/4/22 10:56, Emanuele Giuseppe Esposito wrote:
> This is a dump of all minor coroutine-related fixes found while looking
> around and testing various things in the QEMU block layer.
> 
> Patches aim to:
> - add missing coroutine_fn annotation to the functions
> - simplify to avoid the typical "if in coroutine: fn()
>    // else create_coroutine(fn)" already present in generated_co_wraper
>    functions.
> - make sure that if a BlockDriver callback is defined as coroutine_fn, then
>    it is always running in a coroutine.

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

Re: [PATCH v2 1/9] block: call bdrv_co_drain_begin in a coroutine

[Vladimir Sementsov-Ogievskiy]

On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote:
> It seems that bdrv_open_driver() forgot to create a coroutine
> where to call bs->drv->bdrv_co_drain_begin(), a callback
> marked as coroutine_fn.
> 
> Because there is no active I/O at this point, the coroutine
> should end right after entering, so the caller does not need
> to poll until it is finished.

Hmm. I see your point. But isn't it better to go the generic way and use a generated coroutine wrapper? Nothing guarantees that .bdrv_co_drain_begin() handlers will never do any yield point even on driver open...

Look for example at bdrv_co_check(). It has a generated wrapper bdrv_check(), declared in include/block/block-io.h

So you just need to declare the wrapper, and use it in bdrv_open_driver(), the code would be clearer too.

> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block.c | 36 +++++++++++++++++++++++++++++++-----
>   1 file changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 5311b21f8e..d2b2800039 100644
> --- a/block.c
> +++ b/block.c
> @@ -1637,12 +1637,34 @@ out:
>       g_free(gen_node_name);
>   }
>   
> +typedef struct DrainCo {
> +    BlockDriverState *bs;
> +    int ret;
> +} DrainCo;
> +
> +static void coroutine_fn bdrv_co_drain_begin(void *opaque)
> +{
> +    int i;
> +    DrainCo *co = opaque;
> +    BlockDriverState *bs = co->bs;
> +
> +    for (i = 0; i < bs->quiesce_counter; i++) {
> +        bs->drv->bdrv_co_drain_begin(bs);
> +    }
> +    co->ret = 0;
> +}
> +
>   static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
>                               const char *node_name, QDict *options,
>                               int open_flags, Error **errp)
>   {
>       Error *local_err = NULL;
> -    int i, ret;
> +    int ret;
> +    Coroutine *co;
> +    DrainCo dco = {
> +        .bs = bs,
> +        .ret = NOT_DONE,
> +    };
>       GLOBAL_STATE_CODE();
>   
>       bdrv_assign_node_name(bs, node_name, &local_err);
> @@ -1690,10 +1712,14 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
>       assert(bdrv_min_mem_align(bs) != 0);
>       assert(is_power_of_2(bs->bl.request_alignment));
>   
> -    for (i = 0; i < bs->quiesce_counter; i++) {
> -        if (drv->bdrv_co_drain_begin) {
> -            drv->bdrv_co_drain_begin(bs);
> -        }
> +    if (drv->bdrv_co_drain_begin) {
> +        co = qemu_coroutine_create(bdrv_co_drain_begin, &dco);
> +        qemu_coroutine_enter(co);
> +        /*
> +         * There should be no reason for drv->bdrv_co_drain_begin to wait at
> +         * this point, because the device does not have any active I/O.
> +         */
> +        assert(dco.ret != NOT_DONE);
>       }
>   
>       return 0;

-- 
Best regards,
Vladimir

Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations

[Vladimir Sementsov-Ogievskiy]

On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote:
> These functions end up calling bdrv_common_block_status_above(), a
> generated_co_wrapper function.

generated_co_wrapper is not a coroutine_fn. Сonversely it's a function that do a class coroutine wrapping - start a coroutine and do POLL to wait for the coroutine to finish.

> In addition, they also happen to be always called in coroutine context,
> meaning all callers are coroutine_fn.

That's also not a reason for marking them coroutine_fn. "coroutine_fn" means that the function can be called only from coroutine context.

> This means that the g_c_w function will enter the qemu_in_coroutine()
> case and eventually suspend (or in other words call qemu_coroutine_yield()).
> Therefore we need to mark such functions coroutine_fn too.

I don't think so. Moreover, this breaks the concept, as your new coroutine_fn functions will call generated_co_wrapper functions which are not marked coroutine_fn and never was.

> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block/block-copy.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/block/block-copy.c b/block/block-copy.c
> index bb947afdda..f33ab1d0b6 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -577,8 +577,9 @@ static coroutine_fn int block_copy_task_entry(AioTask *task)
>       return ret;
>   }
>   
> -static int block_copy_block_status(BlockCopyState *s, int64_t offset,
> -                                   int64_t bytes, int64_t *pnum)
> +static coroutine_fn int block_copy_block_status(BlockCopyState *s,
> +                                                int64_t offset,
> +                                                int64_t bytes, int64_t *pnum)
>   {
>       int64_t num;
>       BlockDriverState *base;
> @@ -613,8 +614,9 @@ static int block_copy_block_status(BlockCopyState *s, int64_t offset,
>    * Check if the cluster starting at offset is allocated or not.
>    * return via pnum the number of contiguous clusters sharing this allocation.
>    */
> -static int block_copy_is_cluster_allocated(BlockCopyState *s, int64_t offset,
> -                                           int64_t *pnum)
> +static int coroutine_fn block_copy_is_cluster_allocated(BlockCopyState *s,
> +                                                        int64_t offset,
> +                                                        int64_t *pnum)
>   {
>       BlockDriverState *bs = s->source->bs;
>       int64_t count, total_count = 0;
> @@ -669,8 +671,9 @@ void block_copy_reset(BlockCopyState *s, int64_t offset, int64_t bytes)
>    * @return 0 when the cluster at @offset was unallocated,
>    *         1 otherwise, and -ret on error.
>    */
> -int64_t block_copy_reset_unallocated(BlockCopyState *s,
> -                                     int64_t offset, int64_t *count)
> +int64_t coroutine_fn block_copy_reset_unallocated(BlockCopyState *s,
> +                                                  int64_t offset,
> +                                                  int64_t *count)
>   {
>       int ret;
>       int64_t clusters, bytes;

-- 
Best regards,
Vladimir

Re: [PATCH v2 3/9] nbd/server.c: add missing coroutine_fn annotations

[Vladimir Sementsov-Ogievskiy]

On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote:
> These functions end up calling bdrv_*() implemented as generated_co_wrapper
> functions.

Same here. Sorry that I joined only on v3.

In past we had a lot of "coroutine wrappers", each IO function in block/io.c and many in block.c had two variants:

coroutine_fn bdrv_co_foo(...)

and a wrapper

bdrv_foo(...)

And that wrapper is not a coroutine_fn, it's for calling from any context: coroutine or not. Now many of these wrappers are auto-generated, you may find them in build/block/block-gen.c after successful make.

"generated_co_wrapper" is a sign for code generation script to generate the wrapper code.

> In addition, they also happen to be always called in coroutine context,
> meaning all callers are coroutine_fn.
> This means that the g_c_w function will enter the qemu_in_coroutine()
> case and eventually suspend (or in other words call qemu_coroutine_yield()).
> Therefore we need to mark such functions coroutine_fn too.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   nbd/server.c | 21 ++++++++++++---------
>   1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index ada16089f3..e2eec0cf46 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -2141,8 +2141,9 @@ static int nbd_extent_array_add(NBDExtentArray *ea,
>       return 0;
>   }
>   
> -static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
> -                                  uint64_t bytes, NBDExtentArray *ea)
> +static int coroutine_fn blockstatus_to_extents(BlockDriverState *bs,
> +                                               uint64_t offset, uint64_t bytes,
> +                                               NBDExtentArray *ea)
>   {
>       while (bytes) {
>           uint32_t flags;
> @@ -2168,8 +2169,9 @@ static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
>       return 0;
>   }
>   
> -static int blockalloc_to_extents(BlockDriverState *bs, uint64_t offset,
> -                                 uint64_t bytes, NBDExtentArray *ea)
> +static int coroutine_fn blockalloc_to_extents(BlockDriverState *bs,
> +                                              uint64_t offset, uint64_t bytes,
> +                                              NBDExtentArray *ea)
>   {
>       while (bytes) {
>           int64_t num;
> @@ -2220,11 +2222,12 @@ static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
>   }
>   
>   /* Get block status from the exported device and send it to the client */
> -static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
> -                                    BlockDriverState *bs, uint64_t offset,
> -                                    uint32_t length, bool dont_fragment,
> -                                    bool last, uint32_t context_id,
> -                                    Error **errp)
> +static int
> +coroutine_fn nbd_co_send_block_status(NBDClient *client, uint64_t handle,
> +                                      BlockDriverState *bs, uint64_t offset,
> +                                      uint32_t length, bool dont_fragment,
> +                                      bool last, uint32_t context_id,
> +                                      Error **errp)
>   {
>       int ret;
>       unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;

-- 
Best regards,
Vladimir

Re: [PATCH v2 5/9] block: distinguish between bdrv_create running in coroutine and not

[Vladimir Sementsov-Ogievskiy]

On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote:
> Call two different functions depending on whether bdrv_create
> is in coroutine or not, following the same pattern as
> generated_co_wrapper functions.
> 
> This allows to also call the coroutine function directly,
> without using CreateCo or relying in bdrv_create().
> 

Can we move to auto-generation of bdrv_create(), like for bdrv_check() and friends?

> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block.c | 74 ++++++++++++++++++++++++++++-----------------------------
>   1 file changed, 36 insertions(+), 38 deletions(-)
> 
> diff --git a/block.c b/block.c
> index d2b2800039..0823563e4d 100644
> --- a/block.c
> +++ b/block.c
> @@ -522,66 +522,64 @@ typedef struct CreateCo {
>       Error *err;
>   } CreateCo;
>   
> -static void coroutine_fn bdrv_create_co_entry(void *opaque)
> +static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
> +                                       QemuOpts *opts, Error **errp)
>   {
> -    Error *local_err = NULL;
>       int ret;
> +    GLOBAL_STATE_CODE();
> +    assert(*errp == NULL);
> +
> +    if (!drv->bdrv_co_create_opts) {
> +        error_setg(errp, "Driver '%s' does not support image creation",
> +                   drv->format_name);
> +        return -ENOTSUP;
> +    }
> +
> +    ret = drv->bdrv_co_create_opts(drv, filename, opts, errp);
>   
> +    if (ret < 0 && !*errp) {
> +        error_setg_errno(errp, -ret, "Could not create image");
> +    }
> +
> +    return ret;
> +}
> +
> +static void coroutine_fn bdrv_create_co_entry(void *opaque)
> +{
>       CreateCo *cco = opaque;
> -    assert(cco->drv);
>       GLOBAL_STATE_CODE();
> +    assert(cco->drv);
>   
> -    ret = cco->drv->bdrv_co_create_opts(cco->drv,
> -                                        cco->filename, cco->opts, &local_err);
> -    error_propagate(&cco->err, local_err);
> -    cco->ret = ret;
> +    cco->ret = bdrv_co_create(cco->drv, cco->filename, cco->opts, &cco->err);
>   }
>   
>   int bdrv_create(BlockDriver *drv, const char* filename,
>                   QemuOpts *opts, Error **errp)
>   {
> -    int ret;
> -
>       GLOBAL_STATE_CODE();
>   
> -    Coroutine *co;
> -    CreateCo cco = {
> -        .drv = drv,
> -        .filename = g_strdup(filename),
> -        .opts = opts,
> -        .ret = NOT_DONE,
> -        .err = NULL,
> -    };
> -
> -    if (!drv->bdrv_co_create_opts) {
> -        error_setg(errp, "Driver '%s' does not support image creation", drv->format_name);
> -        ret = -ENOTSUP;
> -        goto out;
> -    }
> -
>       if (qemu_in_coroutine()) {
>           /* Fast-path if already in coroutine context */
> -        bdrv_create_co_entry(&cco);
> +        return bdrv_co_create(drv, filename, opts, errp);
>       } else {
> +        Coroutine *co;
> +        CreateCo cco = {
> +            .drv = drv,
> +            .filename = g_strdup(filename),
> +            .opts = opts,
> +            .ret = NOT_DONE,
> +            .err = NULL,
> +        };
> +
>           co = qemu_coroutine_create(bdrv_create_co_entry, &cco);
>           qemu_coroutine_enter(co);
>           while (cco.ret == NOT_DONE) {
>               aio_poll(qemu_get_aio_context(), true);
>           }
> +        error_propagate(errp, cco.err);
> +        g_free(cco.filename);
> +        return cco.ret;
>       }
> -
> -    ret = cco.ret;
> -    if (ret < 0) {
> -        if (cco.err) {
> -            error_propagate(errp, cco.err);
> -        } else {
> -            error_setg_errno(errp, -ret, "Could not create image");
> -        }
> -    }
> -
> -out:
> -    g_free(cco.filename);
> -    return ret;
>   }
>   
>   /**

-- 
Best regards,
Vladimir

Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations

[Emanuele Giuseppe Esposito]

Am 08/11/2022 um 15:48 schrieb Vladimir Sementsov-Ogievskiy:
> On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote:
>> These functions end up calling bdrv_common_block_status_above(), a
>> generated_co_wrapper function.
> 
> generated_co_wrapper is not a coroutine_fn. Сonversely it's a function
> that do a class coroutine wrapping - start a coroutine and do POLL to
> wait for the coroutine to finish.
> 
>> In addition, they also happen to be always called in coroutine context,
>> meaning all callers are coroutine_fn.
> 
> That's also not a reason for marking them coroutine_fn. "coroutine_fn"
> means that the function can be called only from coroutine context.
> 
>> This means that the g_c_w function will enter the qemu_in_coroutine()
>> case and eventually suspend (or in other words call
>> qemu_coroutine_yield()).
>> Therefore we need to mark such functions coroutine_fn too.
> 
> I don't think so. Moreover, this breaks the concept, as your new
> coroutine_fn functions will call generated_co_wrapper functions which
> are not marked coroutine_fn and never was.

Theoretically not, because marking it coroutine_fn we know that we are
going in the if(qemu_in_coroutine()) branch of the g_c_w, so we could
directly replace it with the actual function. Because it's a pain to do
it with hand, and at some point I/someone should use Alberto static
analyzer to get rid of that, I decided to leave g_c_w there.

As I understand it, it seems that you and Paolo have a different
understanding on what coroutine_fn means and where it should be used.
Honestly I don't understand your point, as you said

> "coroutine_fn"
> means that the function can be called only from coroutine context.

which is the case for these functions. So could you please clarify?

What I do know is that it's extremely confusing to understand if a
function that is *not* marked as coroutine_fn is actually being called
also from coroutines or not.

Which complicates A LOT whatever change or operation I want to perform
on the BlockDriver callbacks or any other function in the block layer,
because in the current approach for the AioContext lock removal (which
you are not aware of, I understand) we need to distinguish between
functions running in coroutine context and not, and throwing g_c_w
functions everywhere makes my work much harder that it already is.

Thank you,
Emanuele

> 
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   block/block-copy.c | 15 +++++++++------
>>   1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/block/block-copy.c b/block/block-copy.c
>> index bb947afdda..f33ab1d0b6 100644
>> --- a/block/block-copy.c
>> +++ b/block/block-copy.c
>> @@ -577,8 +577,9 @@ static coroutine_fn int
>> block_copy_task_entry(AioTask *task)
>>       return ret;
>>   }
>>   -static int block_copy_block_status(BlockCopyState *s, int64_t offset,
>> -                                   int64_t bytes, int64_t *pnum)
>> +static coroutine_fn int block_copy_block_status(BlockCopyState *s,
>> +                                                int64_t offset,
>> +                                                int64_t bytes,
>> int64_t *pnum)
>>   {
>>       int64_t num;
>>       BlockDriverState *base;
>> @@ -613,8 +614,9 @@ static int block_copy_block_status(BlockCopyState
>> *s, int64_t offset,
>>    * Check if the cluster starting at offset is allocated or not.
>>    * return via pnum the number of contiguous clusters sharing this
>> allocation.
>>    */
>> -static int block_copy_is_cluster_allocated(BlockCopyState *s, int64_t
>> offset,
>> -                                           int64_t *pnum)
>> +static int coroutine_fn
>> block_copy_is_cluster_allocated(BlockCopyState *s,
>> +                                                        int64_t offset,
>> +                                                        int64_t *pnum)
>>   {
>>       BlockDriverState *bs = s->source->bs;
>>       int64_t count, total_count = 0;
>> @@ -669,8 +671,9 @@ void block_copy_reset(BlockCopyState *s, int64_t
>> offset, int64_t bytes)
>>    * @return 0 when the cluster at @offset was unallocated,
>>    *         1 otherwise, and -ret on error.
>>    */
>> -int64_t block_copy_reset_unallocated(BlockCopyState *s,
>> -                                     int64_t offset, int64_t *count)
>> +int64_t coroutine_fn block_copy_reset_unallocated(BlockCopyState *s,
>> +                                                  int64_t offset,
>> +                                                  int64_t *count)
>>   {
>>       int ret;
>>       int64_t clusters, bytes;
> 

Re: [PATCH v2 1/9] block: call bdrv_co_drain_begin in a coroutine

[Emanuele Giuseppe Esposito]

Am 08/11/2022 um 15:33 schrieb Vladimir Sementsov-Ogievskiy:
> On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote:
>> It seems that bdrv_open_driver() forgot to create a coroutine
>> where to call bs->drv->bdrv_co_drain_begin(), a callback
>> marked as coroutine_fn.
>>
>> Because there is no active I/O at this point, the coroutine
>> should end right after entering, so the caller does not need
>> to poll until it is finished.
> 
> Hmm. I see your point. But isn't it better to go the generic way and use
> a generated coroutine wrapper? Nothing guarantees that
> .bdrv_co_drain_begin() handlers will never do any yield point even on
> driver open...
> 
> Look for example at bdrv_co_check(). It has a generated wrapper
> bdrv_check(), declared in include/block/block-io.h
> 
> So you just need to declare the wrapper, and use it in
> bdrv_open_driver(), the code would be clearer too.

I think (and this is a repetition from my previous email) it confuses
the code. It is clear, but then you don't know if we are in a coroutine
context or not.

I am very well aware of what you did with your script, and I am working
on extending your g_c_w class with g_c_w_simple, where we drop the
if(qemu_in_coroutine()) case and leave just the coroutine creation.
Therefore, coroutine functions we use only the _co_ function, otherwise
we use g_c_w_simple.
In this way code is clear as you point out, but there is no confusion.

Thank you,
Emanuele
> 
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   block.c | 36 +++++++++++++++++++++++++++++++-----
>>   1 file changed, 31 insertions(+), 5 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 5311b21f8e..d2b2800039 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1637,12 +1637,34 @@ out:
>>       g_free(gen_node_name);
>>   }
>>   +typedef struct DrainCo {
>> +    BlockDriverState *bs;
>> +    int ret;
>> +} DrainCo;
>> +
>> +static void coroutine_fn bdrv_co_drain_begin(void *opaque)
>> +{
>> +    int i;
>> +    DrainCo *co = opaque;
>> +    BlockDriverState *bs = co->bs;
>> +
>> +    for (i = 0; i < bs->quiesce_counter; i++) {
>> +        bs->drv->bdrv_co_drain_begin(bs);
>> +    }
>> +    co->ret = 0;
>> +}
>> +
>>   static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
>>                               const char *node_name, QDict *options,
>>                               int open_flags, Error **errp)
>>   {
>>       Error *local_err = NULL;
>> -    int i, ret;
>> +    int ret;
>> +    Coroutine *co;
>> +    DrainCo dco = {
>> +        .bs = bs,
>> +        .ret = NOT_DONE,
>> +    };
>>       GLOBAL_STATE_CODE();
>>         bdrv_assign_node_name(bs, node_name, &local_err);
>> @@ -1690,10 +1712,14 @@ static int bdrv_open_driver(BlockDriverState
>> *bs, BlockDriver *drv,
>>       assert(bdrv_min_mem_align(bs) != 0);
>>       assert(is_power_of_2(bs->bl.request_alignment));
>>   -    for (i = 0; i < bs->quiesce_counter; i++) {
>> -        if (drv->bdrv_co_drain_begin) {
>> -            drv->bdrv_co_drain_begin(bs);
>> -        }
>> +    if (drv->bdrv_co_drain_begin) {
>> +        co = qemu_coroutine_create(bdrv_co_drain_begin, &dco);
>> +        qemu_coroutine_enter(co);
>> +        /*
>> +         * There should be no reason for drv->bdrv_co_drain_begin to
>> wait at
>> +         * this point, because the device does not have any active I/O.
>> +         */
>> +        assert(dco.ret != NOT_DONE);
>>       }
>>         return 0;
> 

Re: [PATCH v2 1/9] block: call bdrv_co_drain_begin in a coroutine

[Kevin Wolf]

Am 08.11.2022 um 15:33 hat Vladimir Sementsov-Ogievskiy geschrieben:
> On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote:
> > It seems that bdrv_open_driver() forgot to create a coroutine
> > where to call bs->drv->bdrv_co_drain_begin(), a callback
> > marked as coroutine_fn.
> > 
> > Because there is no active I/O at this point, the coroutine
> > should end right after entering, so the caller does not need
> > to poll until it is finished.
> 
> Hmm. I see your point. But isn't it better to go the generic way and
> use a generated coroutine wrapper? Nothing guarantees that
> .bdrv_co_drain_begin() handlers will never do any yield point even on
> driver open...
> 
> Look for example at bdrv_co_check(). It has a generated wrapper
> bdrv_check(), declared in include/block/block-io.h
> 
> So you just need to declare the wrapper, and use it in
> bdrv_open_driver(), the code would be clearer too.

Note that if we apply the drain simplification series I sent today up to
at least patch 3 ('block: Revert .bdrv_drained_begin/end to
non-coroutine_fn') [1], then this patch isn't actually needed any more.

Kevin

[1] https://lists.gnu.org/archive/html/qemu-block/2022-11/msg00206.html

Re: [PATCH v2 8/9] block: bdrv_create is never called in non-coroutine context

[Kevin Wolf]

Am 04.11.2022 um 10:56 hat Emanuele Giuseppe Esposito geschrieben:
> Delete the if case and make sure it won't be called again
> in coroutines.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

In the subject line, it should be "never called in coroutine context"
rather than "non-coroutine context", right?

Kevin

Re: [PATCH v2 1/9] block: call bdrv_co_drain_begin in a coroutine

[Vladimir Sementsov-Ogievskiy]

On 11/8/22 18:13, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 08/11/2022 um 15:33 schrieb Vladimir Sementsov-Ogievskiy:
>> On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote:
>>> It seems that bdrv_open_driver() forgot to create a coroutine
>>> where to call bs->drv->bdrv_co_drain_begin(), a callback
>>> marked as coroutine_fn.
>>>
>>> Because there is no active I/O at this point, the coroutine
>>> should end right after entering, so the caller does not need
>>> to poll until it is finished.
>>
>> Hmm. I see your point. But isn't it better to go the generic way and use
>> a generated coroutine wrapper? Nothing guarantees that
>> .bdrv_co_drain_begin() handlers will never do any yield point even on
>> driver open...
>>
>> Look for example at bdrv_co_check(). It has a generated wrapper
>> bdrv_check(), declared in include/block/block-io.h
>>
>> So you just need to declare the wrapper, and use it in
>> bdrv_open_driver(), the code would be clearer too.
> 
> I think (and this is a repetition from my previous email) it confuses
> the code. It is clear, but then you don't know if we are in a coroutine
> context or not.

Hmm. But same thing is true for all callers of coroutine wrappers.

I agree that "coroutine wrapper" is a workaround for the design problem. Really, the fact that in many places we don't know are we in coroutine or not is very uncomfortable.

But still, I'm not sure that's it good to avoid this workaround in one place and continue to use it in all other places. I think following common design is better. Or rework it deeply by getting rid of the wrappers somehow.

> 
> I am very well aware of what you did with your script, and I am working
> on extending your g_c_w class with g_c_w_simple, where we drop the
> if(qemu_in_coroutine()) case and leave just the coroutine creation.
> Therefore, coroutine functions we use only the _co_ function, otherwise
> we use g_c_w_simple.
> In this way code is clear as you point out, but there is no confusion.

Hmm sounds good, I missed it. Why not use g_c_w_simple here than?

> 
> Thank you,
> Emanuele
>>
>>>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>    block.c | 36 +++++++++++++++++++++++++++++++-----
>>>    1 file changed, 31 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 5311b21f8e..d2b2800039 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -1637,12 +1637,34 @@ out:
>>>        g_free(gen_node_name);
>>>    }
>>>    +typedef struct DrainCo {
>>> +    BlockDriverState *bs;
>>> +    int ret;
>>> +} DrainCo;
>>> +
>>> +static void coroutine_fn bdrv_co_drain_begin(void *opaque)
>>> +{
>>> +    int i;
>>> +    DrainCo *co = opaque;
>>> +    BlockDriverState *bs = co->bs;
>>> +
>>> +    for (i = 0; i < bs->quiesce_counter; i++) {
>>> +        bs->drv->bdrv_co_drain_begin(bs);
>>> +    }
>>> +    co->ret = 0;
>>> +}
>>> +
>>>    static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
>>>                                const char *node_name, QDict *options,
>>>                                int open_flags, Error **errp)
>>>    {
>>>        Error *local_err = NULL;
>>> -    int i, ret;
>>> +    int ret;
>>> +    Coroutine *co;
>>> +    DrainCo dco = {
>>> +        .bs = bs,
>>> +        .ret = NOT_DONE,
>>> +    };
>>>        GLOBAL_STATE_CODE();
>>>          bdrv_assign_node_name(bs, node_name, &local_err);
>>> @@ -1690,10 +1712,14 @@ static int bdrv_open_driver(BlockDriverState
>>> *bs, BlockDriver *drv,
>>>        assert(bdrv_min_mem_align(bs) != 0);
>>>        assert(is_power_of_2(bs->bl.request_alignment));
>>>    -    for (i = 0; i < bs->quiesce_counter; i++) {
>>> -        if (drv->bdrv_co_drain_begin) {
>>> -            drv->bdrv_co_drain_begin(bs);
>>> -        }
>>> +    if (drv->bdrv_co_drain_begin) {
>>> +        co = qemu_coroutine_create(bdrv_co_drain_begin, &dco);
>>> +        qemu_coroutine_enter(co);
>>> +        /*
>>> +         * There should be no reason for drv->bdrv_co_drain_begin to
>>> wait at
>>> +         * this point, because the device does not have any active I/O.
>>> +         */
>>> +        assert(dco.ret != NOT_DONE);
>>>        }
>>>          return 0;
>>
> 

-- 
Best regards,
Vladimir

Re: [PATCH v2 1/9] block: call bdrv_co_drain_begin in a coroutine

[Emanuele Giuseppe Esposito]

Am 08/11/2022 um 16:52 schrieb Vladimir Sementsov-Ogievskiy:
> On 11/8/22 18:13, Emanuele Giuseppe Esposito wrote:
>>
>>
>> Am 08/11/2022 um 15:33 schrieb Vladimir Sementsov-Ogievskiy:
>>> On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote:
>>>> It seems that bdrv_open_driver() forgot to create a coroutine
>>>> where to call bs->drv->bdrv_co_drain_begin(), a callback
>>>> marked as coroutine_fn.
>>>>
>>>> Because there is no active I/O at this point, the coroutine
>>>> should end right after entering, so the caller does not need
>>>> to poll until it is finished.
>>>
>>> Hmm. I see your point. But isn't it better to go the generic way and use
>>> a generated coroutine wrapper? Nothing guarantees that
>>> .bdrv_co_drain_begin() handlers will never do any yield point even on
>>> driver open...
>>>
>>> Look for example at bdrv_co_check(). It has a generated wrapper
>>> bdrv_check(), declared in include/block/block-io.h
>>>
>>> So you just need to declare the wrapper, and use it in
>>> bdrv_open_driver(), the code would be clearer too.
>>
>> I think (and this is a repetition from my previous email) it confuses
>> the code. It is clear, but then you don't know if we are in a coroutine
>> context or not.
> 
> Hmm. But same thing is true for all callers of coroutine wrappers.
> 
> I agree that "coroutine wrapper" is a workaround for the design problem.
> Really, the fact that in many places we don't know are we in coroutine
> or not is very uncomfortable.

And the only way to figure if it's a coroutine or not is by adding
assertions and pray that the iotests don't fail *and* cover all cases.

> 
> But still, I'm not sure that's it good to avoid this workaround in one
> place and continue to use it in all other places. I think following
> common design is better. Or rework it deeply by getting rid of the
> wrappers somehow.

Well, one step at the time :) it's already difficult to verify that such
replacement cover and is correct for all cases :)

> 
>>
>> I am very well aware of what you did with your script, and I am working
>> on extending your g_c_w class with g_c_w_simple, where we drop the
>> if(qemu_in_coroutine()) case and leave just the coroutine creation.
>> Therefore, coroutine functions we use only the _co_ function, otherwise
>> we use g_c_w_simple.
>> In this way code is clear as you point out, but there is no confusion.
> 
> Hmm sounds good, I missed it. Why not use g_c_w_simple here than?

Because I came up with it this morning.

Thank you,
Emanuele

> 
>>
>> Thank you,
>> Emanuele
>>>
>>>>
>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> ---
>>>>    block.c | 36 +++++++++++++++++++++++++++++++-----
>>>>    1 file changed, 31 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index 5311b21f8e..d2b2800039 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -1637,12 +1637,34 @@ out:
>>>>        g_free(gen_node_name);
>>>>    }
>>>>    +typedef struct DrainCo {
>>>> +    BlockDriverState *bs;
>>>> +    int ret;
>>>> +} DrainCo;
>>>> +
>>>> +static void coroutine_fn bdrv_co_drain_begin(void *opaque)
>>>> +{
>>>> +    int i;
>>>> +    DrainCo *co = opaque;
>>>> +    BlockDriverState *bs = co->bs;
>>>> +
>>>> +    for (i = 0; i < bs->quiesce_counter; i++) {
>>>> +        bs->drv->bdrv_co_drain_begin(bs);
>>>> +    }
>>>> +    co->ret = 0;
>>>> +}
>>>> +
>>>>    static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
>>>>                                const char *node_name, QDict *options,
>>>>                                int open_flags, Error **errp)
>>>>    {
>>>>        Error *local_err = NULL;
>>>> -    int i, ret;
>>>> +    int ret;
>>>> +    Coroutine *co;
>>>> +    DrainCo dco = {
>>>> +        .bs = bs,
>>>> +        .ret = NOT_DONE,
>>>> +    };
>>>>        GLOBAL_STATE_CODE();
>>>>          bdrv_assign_node_name(bs, node_name, &local_err);
>>>> @@ -1690,10 +1712,14 @@ static int bdrv_open_driver(BlockDriverState
>>>> *bs, BlockDriver *drv,
>>>>        assert(bdrv_min_mem_align(bs) != 0);
>>>>        assert(is_power_of_2(bs->bl.request_alignment));
>>>>    -    for (i = 0; i < bs->quiesce_counter; i++) {
>>>> -        if (drv->bdrv_co_drain_begin) {
>>>> -            drv->bdrv_co_drain_begin(bs);
>>>> -        }
>>>> +    if (drv->bdrv_co_drain_begin) {
>>>> +        co = qemu_coroutine_create(bdrv_co_drain_begin, &dco);
>>>> +        qemu_coroutine_enter(co);
>>>> +        /*
>>>> +         * There should be no reason for drv->bdrv_co_drain_begin to
>>>> wait at
>>>> +         * this point, because the device does not have any active
>>>> I/O.
>>>> +         */
>>>> +        assert(dco.ret != NOT_DONE);
>>>>        }
>>>>          return 0;
>>>
>>
> 

Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations

[Vladimir Sementsov-Ogievskiy]

[add Stefan]


On 11/8/22 18:09, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 08/11/2022 um 15:48 schrieb Vladimir Sementsov-Ogievskiy:
>> On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote:
>>> These functions end up calling bdrv_common_block_status_above(), a
>>> generated_co_wrapper function.
>>
>> generated_co_wrapper is not a coroutine_fn. Сonversely it's a function
>> that do a class coroutine wrapping - start a coroutine and do POLL to
>> wait for the coroutine to finish.
>>
>>> In addition, they also happen to be always called in coroutine context,
>>> meaning all callers are coroutine_fn.
>>
>> That's also not a reason for marking them coroutine_fn. "coroutine_fn"
>> means that the function can be called only from coroutine context.
>>
>>> This means that the g_c_w function will enter the qemu_in_coroutine()
>>> case and eventually suspend (or in other words call
>>> qemu_coroutine_yield()).
>>> Therefore we need to mark such functions coroutine_fn too.
>>
>> I don't think so. Moreover, this breaks the concept, as your new
>> coroutine_fn functions will call generated_co_wrapper functions which
>> are not marked coroutine_fn and never was.
> 
> Theoretically not, 

Agree, I was wrong in this point

> because marking it coroutine_fn we know that we are
> going in the if(qemu_in_coroutine()) branch of the g_c_w, so we could
> directly replace it with the actual function. Because it's a pain to do
> it with hand, and at some point I/someone should use Alberto static
> analyzer to get rid of that, I decided to leave g_c_w there.
> 
> As I understand it, it seems that you and Paolo have a different
> understanding on what coroutine_fn means and where it should be used.

Looks so...

But we have a documentation in coroutine.h, let's check:

  * Mark a function that executes in coroutine context
  *
  * Functions that execute in coroutine context cannot be called directly from
  * normal functions.  In the future it would be nice to enable compiler or
  * static checker support for catching such errors.  This annotation might make
  * it possible and in the meantime it serves as documentation.

Not very clear, but still it say:

  coroutine_fn = "function that executes in coroutine context"
  "functions that execute in coroutine context"  =  "cannot be called directly from normal functions"

So, IMHO that corresponds to my point of view: we shouldn't mark by coroutine_fn functions that can be called from both coroutine context and not.

If we want to change the concept, we should start with rewriting this documentation.

> Honestly I don't understand your point, as you said
> 
>> "coroutine_fn"
>> means that the function can be called only from coroutine context.
> 
> which is the case for these functions. So could you please clarify?
> 
> What I do know is that it's extremely confusing to understand if a
> function that is *not* marked as coroutine_fn is actually being called
> also from coroutines or not.
> 
> Which complicates A LOT whatever change or operation I want to perform
> on the BlockDriver callbacks or any other function in the block layer,
> because in the current approach for the AioContext lock removal (which
> you are not aware of, I understand) we need to distinguish between
> functions running in coroutine context and not, and throwing g_c_w
> functions everywhere makes my work much harder that it already is.

OK, I understand the problem.

Hmm. Formally marking by "coroutine_fn" a function that theoretically can be called from any context doesn't break things. We just say that since that moment we don't allow to call this function from non-coroutine context.

OK, I tend to agree that you are on the right way, sorry for my skepticism)

PS: you recently introduced a lot of IO_CODE() / GLOBAL_STATE_CODE() marks, which (will be/already) turned into assertions.

This is a lot better than our "coroutine_fn" sign, which actually do no check (and can't do). Don't you plan to swap a "coroutine_fn" noop marker with more meaningful IN_COROUTINE(); (or something like this, which just do assert(qemu_in_coroutine())) at start of the function? It would be a lot safer.


> 
> Thank you,
> Emanuele
> 
>>
>>>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>> ---
>>>    block/block-copy.c | 15 +++++++++------
>>>    1 file changed, 9 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/block/block-copy.c b/block/block-copy.c
>>> index bb947afdda..f33ab1d0b6 100644
>>> --- a/block/block-copy.c
>>> +++ b/block/block-copy.c
>>> @@ -577,8 +577,9 @@ static coroutine_fn int
>>> block_copy_task_entry(AioTask *task)
>>>        return ret;
>>>    }
>>>    -static int block_copy_block_status(BlockCopyState *s, int64_t offset,
>>> -                                   int64_t bytes, int64_t *pnum)
>>> +static coroutine_fn int block_copy_block_status(BlockCopyState *s,
>>> +                                                int64_t offset,
>>> +                                                int64_t bytes,
>>> int64_t *pnum)
>>>    {
>>>        int64_t num;
>>>        BlockDriverState *base;
>>> @@ -613,8 +614,9 @@ static int block_copy_block_status(BlockCopyState
>>> *s, int64_t offset,
>>>     * Check if the cluster starting at offset is allocated or not.
>>>     * return via pnum the number of contiguous clusters sharing this
>>> allocation.
>>>     */
>>> -static int block_copy_is_cluster_allocated(BlockCopyState *s, int64_t
>>> offset,
>>> -                                           int64_t *pnum)
>>> +static int coroutine_fn
>>> block_copy_is_cluster_allocated(BlockCopyState *s,
>>> +                                                        int64_t offset,
>>> +                                                        int64_t *pnum)
>>>    {
>>>        BlockDriverState *bs = s->source->bs;
>>>        int64_t count, total_count = 0;
>>> @@ -669,8 +671,9 @@ void block_copy_reset(BlockCopyState *s, int64_t
>>> offset, int64_t bytes)
>>>     * @return 0 when the cluster at @offset was unallocated,
>>>     *         1 otherwise, and -ret on error.
>>>     */
>>> -int64_t block_copy_reset_unallocated(BlockCopyState *s,
>>> -                                     int64_t offset, int64_t *count)
>>> +int64_t coroutine_fn block_copy_reset_unallocated(BlockCopyState *s,
>>> +                                                  int64_t offset,
>>> +                                                  int64_t *count)
>>>    {
>>>        int ret;
>>>        int64_t clusters, bytes;
>>
> 

-- 
Best regards,
Vladimir

Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations

[Vladimir Sementsov-Ogievskiy]

On 11/8/22 19:19, Vladimir Sementsov-Ogievskiy wrote:
> This is a lot better than our "coroutine_fn" sign, which actually do no check (and can't do). Don't you plan to swap a "coroutine_fn" noop marker with more meaningful IN_COROUTINE(); (or something like this, which just do assert(qemu_in_coroutine())) at start of the function? It would be a lot safer.


Moreover, we can introduce two macros:

IN_COROUTINE() and NOT_COROUTINE(), mark functions correspondingly and drop coroutine_fn mark. Than the picture would be very deterministic:

IN_COROUTINE - function is called only from coroutine context
NOT_COROUTINE - function is never called from coroutine context
<no mark> - function may be called from both coroutine and non-coroutine context

-- 
Best regards,
Vladimir

Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations

[Emanuele Giuseppe Esposito]

Am 08/11/2022 um 17:19 schrieb Vladimir Sementsov-Ogievskiy:
> [add Stefan]
> 
> 
> On 11/8/22 18:09, Emanuele Giuseppe Esposito wrote:
>>
>>
>> Am 08/11/2022 um 15:48 schrieb Vladimir Sementsov-Ogievskiy:
>>> On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote:
>>>> These functions end up calling bdrv_common_block_status_above(), a
>>>> generated_co_wrapper function.
>>>
>>> generated_co_wrapper is not a coroutine_fn. Сonversely it's a function
>>> that do a class coroutine wrapping - start a coroutine and do POLL to
>>> wait for the coroutine to finish.
>>>
>>>> In addition, they also happen to be always called in coroutine context,
>>>> meaning all callers are coroutine_fn.
>>>
>>> That's also not a reason for marking them coroutine_fn. "coroutine_fn"
>>> means that the function can be called only from coroutine context.
>>>
>>>> This means that the g_c_w function will enter the qemu_in_coroutine()
>>>> case and eventually suspend (or in other words call
>>>> qemu_coroutine_yield()).
>>>> Therefore we need to mark such functions coroutine_fn too.
>>>
>>> I don't think so. Moreover, this breaks the concept, as your new
>>> coroutine_fn functions will call generated_co_wrapper functions which
>>> are not marked coroutine_fn and never was.
>>
>> Theoretically not, 
> 
> Agree, I was wrong in this point
> 
>> because marking it coroutine_fn we know that we are
>> going in the if(qemu_in_coroutine()) branch of the g_c_w, so we could
>> directly replace it with the actual function. Because it's a pain to do
>> it with hand, and at some point I/someone should use Alberto static
>> analyzer to get rid of that, I decided to leave g_c_w there.
>>
>> As I understand it, it seems that you and Paolo have a different
>> understanding on what coroutine_fn means and where it should be used.
> 
> Looks so...
> 
> But we have a documentation in coroutine.h, let's check:
> 
>  * Mark a function that executes in coroutine context
>  *
>  * Functions that execute in coroutine context cannot be called directly
> from
>  * normal functions.  In the future it would be nice to enable compiler or
>  * static checker support for catching such errors.  This annotation
> might make
>  * it possible and in the meantime it serves as documentation.
> 
> Not very clear, but still it say:
> 
>  coroutine_fn = "function that executes in coroutine context"
>  "functions that execute in coroutine context"  =  "cannot be called
> directly from normal functions"
> 
> So, IMHO that corresponds to my point of view: we shouldn't mark by
> coroutine_fn functions that can be called from both coroutine context
> and not.

Yes and the point is that these functions are not called by
non-coroutine context.

> 
> If we want to change the concept, we should start with rewriting this
> documentation.
> 
>> Honestly I don't understand your point, as you said
>>
>>> "coroutine_fn"
>>> means that the function can be called only from coroutine context.
>>
>> which is the case for these functions. So could you please clarify?
>>
>> What I do know is that it's extremely confusing to understand if a
>> function that is *not* marked as coroutine_fn is actually being called
>> also from coroutines or not.
>>
>> Which complicates A LOT whatever change or operation I want to perform
>> on the BlockDriver callbacks or any other function in the block layer,
>> because in the current approach for the AioContext lock removal (which
>> you are not aware of, I understand) we need to distinguish between
>> functions running in coroutine context and not, and throwing g_c_w
>> functions everywhere makes my work much harder that it already is.
> 
> OK, I understand the problem.
> 
> Hmm. Formally marking by "coroutine_fn" a function that theoretically
> can be called from any context doesn't break things. We just say that
> since that moment we don't allow to call this function from
> non-coroutine context.
> 
> OK, I tend to agree that you are on the right way, sorry for my skepticism)
> 
> PS: you recently introduced a lot of IO_CODE() / GLOBAL_STATE_CODE()
> marks, which (will be/already) turned into assertions.
> 
> This is a lot better than our "coroutine_fn" sign, which actually do no
> check (and can't do). Don't you plan to swap a "coroutine_fn" noop
> marker with more meaningful IN_COROUTINE(); (or something like this,
> which just do assert(qemu_in_coroutine())) at start of the function? It
> would be a lot safer.

CCing also Alberto and Paolo

So basically I think what we need is something that scans the whole
block layer code and puts the right coroutine_fn annotations (or
assertions, if you want) in the right places.

The rule should be (anyone please correct me if I am wrong):
- if a function is only called by coroutine_fn functions, then it's a
coroutine_fn. Theoretically all these functions should eventually end up
calling qemu_coroutine_yield or similar.

- if it's called only from non-coroutine, then leave it as it is.
Probably asserting is a good idea.

- if it's used by both, then it's a case-by-case decision: I think for
simple functions, we can use a special annotation and document that they
should always consider that they could run in both cases.
If it's a big function like the g_c_w, call only the _co_ version in
coroutine_fn, and create a coroutine or call the non-coroutine
counterpart if we are not in coroutine context.
Which is also what I am trying to do with g_c_w_simple.

However, it would be nice to assign this to someone and do this
automatically, not doing it by hand. I am not sure if Alberto static
analyzer is currently capable of doing that.

What do you all think?

Thank you,
Emanuele

> 
> 
>>
>> Thank you,
>> Emanuele
>>
>>>
>>>>
>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>> ---
>>>>    block/block-copy.c | 15 +++++++++------
>>>>    1 file changed, 9 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/block/block-copy.c b/block/block-copy.c
>>>> index bb947afdda..f33ab1d0b6 100644
>>>> --- a/block/block-copy.c
>>>> +++ b/block/block-copy.c
>>>> @@ -577,8 +577,9 @@ static coroutine_fn int
>>>> block_copy_task_entry(AioTask *task)
>>>>        return ret;
>>>>    }
>>>>    -static int block_copy_block_status(BlockCopyState *s, int64_t
>>>> offset,
>>>> -                                   int64_t bytes, int64_t *pnum)
>>>> +static coroutine_fn int block_copy_block_status(BlockCopyState *s,
>>>> +                                                int64_t offset,
>>>> +                                                int64_t bytes,
>>>> int64_t *pnum)
>>>>    {
>>>>        int64_t num;
>>>>        BlockDriverState *base;
>>>> @@ -613,8 +614,9 @@ static int block_copy_block_status(BlockCopyState
>>>> *s, int64_t offset,
>>>>     * Check if the cluster starting at offset is allocated or not.
>>>>     * return via pnum the number of contiguous clusters sharing this
>>>> allocation.
>>>>     */
>>>> -static int block_copy_is_cluster_allocated(BlockCopyState *s, int64_t
>>>> offset,
>>>> -                                           int64_t *pnum)
>>>> +static int coroutine_fn
>>>> block_copy_is_cluster_allocated(BlockCopyState *s,
>>>> +                                                        int64_t
>>>> offset,
>>>> +                                                        int64_t *pnum)
>>>>    {
>>>>        BlockDriverState *bs = s->source->bs;
>>>>        int64_t count, total_count = 0;
>>>> @@ -669,8 +671,9 @@ void block_copy_reset(BlockCopyState *s, int64_t
>>>> offset, int64_t bytes)
>>>>     * @return 0 when the cluster at @offset was unallocated,
>>>>     *         1 otherwise, and -ret on error.
>>>>     */
>>>> -int64_t block_copy_reset_unallocated(BlockCopyState *s,
>>>> -                                     int64_t offset, int64_t *count)
>>>> +int64_t coroutine_fn block_copy_reset_unallocated(BlockCopyState *s,
>>>> +                                                  int64_t offset,
>>>> +                                                  int64_t *count)
>>>>    {
>>>>        int ret;
>>>>        int64_t clusters, bytes;
>>>
>>
> 

Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations

[Alberto Faria]

On Wed, Nov 9, 2022 at 12:24 PM Emanuele Giuseppe Esposito
<eesposit@redhat.com> wrote:
> CCing also Alberto and Paolo
>
> So basically I think what we need is something that scans the whole
> block layer code and puts the right coroutine_fn annotations (or
> assertions, if you want) in the right places.
>
> The rule should be (anyone please correct me if I am wrong):
> - if a function is only called by coroutine_fn functions, then it's a
> coroutine_fn. Theoretically all these functions should eventually end up
> calling qemu_coroutine_yield or similar.
>
> - if it's called only from non-coroutine, then leave it as it is.
> Probably asserting is a good idea.
>
> - if it's used by both, then it's a case-by-case decision: I think for
> simple functions, we can use a special annotation and document that they
> should always consider that they could run in both cases.
> If it's a big function like the g_c_w, call only the _co_ version in
> coroutine_fn, and create a coroutine or call the non-coroutine
> counterpart if we are not in coroutine context.
> Which is also what I am trying to do with g_c_w_simple.
>
> However, it would be nice to assign this to someone and do this
> automatically, not doing it by hand. I am not sure if Alberto static
> analyzer is currently capable of doing that.
>
> What do you all think?

From what I've seen so far of coroutine_fn, its intended semantics
seem to align completely with the `async` of many languages. The only
restriction is that a function that calls coroutine_fn functions
(a.k.a. coroutines) must itself be coroutine_fn. Marking other
functions as coroutine_fn, as you mention in your first bullet above,
just artificially restricts them to coroutine context. Similarly,
restricting functions to non-coroutine context may not generally be
useful, except when there is an alternative version of the function
that is optimized for coroutine context in some way (e.g., calling a
coroutine_fn instead of the corresponding generated_co_wrapper).

But maybe you're writing a function that you predict will eventually
need to call a coroutine, even though it doesn't today. In those cases
it could make sense to mark it coroutine_fn, to prevent non-coroutine
callers from appearing and later breaking, but this should probably be
the exception, not the rule.

My WIP static analyzer [1] should be able to find most cases where a
non-coroutine_fn function calls a coroutine (some complicated cases
involving function pointers and typedefs are not yet implemented). It
also complains about cases where a coroutine calls a
generated_co_wrapper (see the no_coroutine_fn annotation, which you
can also apply to functions other than generated_co_wrappers). You can
use it today: just merge [1] with your code and run (after building
QEMU):

    ./static-analyzer.py build block

Alberto

[1] https://gitlab.com/albertofaria/qemu/-/tree/static-analysis

Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations

[Paolo Bonzini]

On Wed, Nov 9, 2022 at 1:24 PM Emanuele Giuseppe Esposito
<eesposit@redhat.com> wrote:
> > > What I do know is that it's extremely confusing to understand if a
> > > function that is *not* marked as coroutine_fn is actually being called
> > > also from coroutines or not.

Agreed. This is a huge point in favor of pushing coroutine wrappers as
far up in the call stack as possible, because it means more
coroutine_fns and fewer mixed functions.

> > This is a lot better than our "coroutine_fn" sign, which actually do no
> > check (and can't do). Don't you plan to swap a "coroutine_fn" noop
> > marker with more meaningful IN_COROUTINE(); (or something like this,
> > which just do assert(qemu_in_coroutine())) at start of the function? It
> > would be a lot safer.
>
> CCing also Alberto and Paolo
>
> So basically I think what we need is something that scans the whole
> block layer code and puts the right coroutine_fn annotations (or
> assertions, if you want) in the right places.

coroutine_fn markers are done by Alberto's static analyzer, which I
used to add coroutine_fn pretty much everywhere in the code base where
they are *needed*. My rules are simple:

* there MUST be no calls from non-coroutine_fn to coroutine_fn, this is obvious

* there MUST be no blocking in coroutine_fn

* there SHOULD be no calls from coroutine_fn to generated_co_wrapper;
use the wrapped *_co_* function directly instead.

To catch the last one, or possibly the last two, Alberto added
no_coroutine_fn. In a perfect world non-marked functions would be
"valid either in coroutine or non-coroutine function": they would call
neither coroutine_fns nor no_coroutine_fns.

This is unfortunately easier said than done, but in order to move
towards that case, I think we can look again at vrc and extend it with
new commands. Alberto's work covers *local* tests, looking at one
caller and one callee at a time. With vrc's knowledge of the global
call graph, we can find *all* paths from a coroutine_fn to a
generated_co_wrapper, including those that go through unmarked
functions. Then there are two cases:

* if the unmarked function is never called from outside a coroutine,
call the wrapped function and change it to coroutine_fn

* if the unmarked function can be called from outside a coroutine,
change it to a coroutine_fn (renaming it) and add a
generated_co_wrapper. Rinse and repeat.

> However, it would be nice to assign this to someone and do this
> automatically, not doing it by hand. I am not sure if Alberto static
> analyzer is currently capable of doing that.

I think the first step has to be done by hand, because it entails
creating new generated_co_wrappers. Checking for regressions can then
be done automatically though.

Paolo

Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations

[Emanuele Giuseppe Esposito]

To sum up on what was discussed in this serie, I don't really see any
strong objection against these patches, so I will soon send v3 which is
pretty much the same except for patch 1, which will be removed.

I think these patches are useful and will be even more meaningful to the
reviewer when in the next few days I send all the rwlock patches.

What has been discussed so far (using QEMU_IN_COROUTINE, using some sort
of tool to automate everything, etc.) has been noted and as I understand
will be researched by Alberto.

Thank you,
Emanuele

Am 10/11/2022 um 11:52 schrieb Paolo Bonzini:
> On Wed, Nov 9, 2022 at 1:24 PM Emanuele Giuseppe Esposito
> <eesposit@redhat.com> wrote:
>>>> What I do know is that it's extremely confusing to understand if a
>>>> function that is *not* marked as coroutine_fn is actually being called
>>>> also from coroutines or not.
> 
> Agreed. This is a huge point in favor of pushing coroutine wrappers as
> far up in the call stack as possible, because it means more
> coroutine_fns and fewer mixed functions.
> 
>>> This is a lot better than our "coroutine_fn" sign, which actually do no
>>> check (and can't do). Don't you plan to swap a "coroutine_fn" noop
>>> marker with more meaningful IN_COROUTINE(); (or something like this,
>>> which just do assert(qemu_in_coroutine())) at start of the function? It
>>> would be a lot safer.
>>
>> CCing also Alberto and Paolo
>>
>> So basically I think what we need is something that scans the whole
>> block layer code and puts the right coroutine_fn annotations (or
>> assertions, if you want) in the right places.
> 
> coroutine_fn markers are done by Alberto's static analyzer, which I
> used to add coroutine_fn pretty much everywhere in the code base where
> they are *needed*. My rules are simple:
> 
> * there MUST be no calls from non-coroutine_fn to coroutine_fn, this is obvious
> 
> * there MUST be no blocking in coroutine_fn
> 
> * there SHOULD be no calls from coroutine_fn to generated_co_wrapper;
> use the wrapped *_co_* function directly instead.
> 
> To catch the last one, or possibly the last two, Alberto added
> no_coroutine_fn. In a perfect world non-marked functions would be
> "valid either in coroutine or non-coroutine function": they would call
> neither coroutine_fns nor no_coroutine_fns.
> 
> This is unfortunately easier said than done, but in order to move
> towards that case, I think we can look again at vrc and extend it with
> new commands. Alberto's work covers *local* tests, looking at one
> caller and one callee at a time. With vrc's knowledge of the global
> call graph, we can find *all* paths from a coroutine_fn to a
> generated_co_wrapper, including those that go through unmarked
> functions. Then there are two cases:
> 
> * if the unmarked function is never called from outside a coroutine,
> call the wrapped function and change it to coroutine_fn
> 
> * if the unmarked function can be called from outside a coroutine,
> change it to a coroutine_fn (renaming it) and add a
> generated_co_wrapper. Rinse and repeat.
> 
>> However, it would be nice to assign this to someone and do this
>> automatically, not doing it by hand. I am not sure if Alberto static
>> analyzer is currently capable of doing that.
> 
> I think the first step has to be done by hand, because it entails
> creating new generated_co_wrappers. Checking for regressions can then
> be done automatically though.
> 
> Paolo
> 

Re: [PATCH v2 2/9] block-copy: add missing coroutine_fn annotations

[Paolo Bonzini]

On 11/15/22 16:41, Emanuele Giuseppe Esposito wrote:
> To sum up on what was discussed in this serie, I don't really see any
> strong objection against these patches, so I will soon send v3 which is
> pretty much the same except for patch 1, which will be removed.
> 
> I think these patches are useful and will be even more meaningful to the
> reviewer when in the next few days I send all the rwlock patches.

Yes, I agree.

FWIW I implemented path search in vrc and it found 133 candidates 
(functions that are only called by coroutine_fn are not coroutine_fns 
themselves).  I only list them after the signature because as expected, 
most of them are pointless; however there are some are obviously correct:

1) some have _co_ in their name :)

2) these five directly call a generated_co_wrapper so they're an easy catch:

vhdx_log_write_and_flush    -> bdrv_flush
vhdx_log_write_and_flush    -> bdrv_pread
vhdx_log_write_and_flush    -> bdrv_pwrite
mirror_flush                -> blk_flush
qcow2_check_refcounts       -> bdrv_pwrite
qcow2_check_refcounts       -> bdrv_pwrite_sync
qcow2_check_refcounts       -> bdrv_pread
qcow2_read_extensions       -> bdrv_pread
check_directory_consistency -> bdrv_pwrite

(vrc lets me query this with "paths [coroutine_fn_candidate] 
[no_coroutine_fn]")

3) I can also query (with "paths [coroutine_fn_candidate] ... 
[no_coroutine_fn]") those that end up calling a generated_co_wrapper. 
Among these, vrc catches block_copy_reset_unallocated from this patch:

block_copy_reset_unallocated
block_crypto_co_create_generic
calculate_l2_meta
check_directory_consistency
commit_direntries
commit_one_file
is_zero
mirror_flush
qcow2_alloc_bytes
qcow2_alloc_cluster_abort
qcow2_alloc_clusters_at
qcow2_check_refcounts
qcow2_get_last_cluster
qcow2_read_extensions
qcow2_read_snapshots
qcow2_truncate_bitmaps_check
qcow2_update_options
vhdx_log_write_and_flush
vmdk_is_cid_valid
zero_l2_subclusters

Another possibility is to identify common "entry points" in the paths to 
the no_coroutine_fn and make them generated_co_wrappers.  For example in 
qcow2 these include bitmap_list_load, update_refcount and 
get_cluster_table and the qcow2_snapshot_* functions.

Of course the analysis would have to be rerun after doing every change.

The most time consuming part is labeling coroutine_fn/no_coroutine_fn, 
which would be useful to do with clang (and at this point you might as 
well extract the CFG with it).  Doing the queries totally by hand 
doesn't quite scale (for example vrc's blind spot is inlining and I 
forgot to disable it, but I only noticed too late...), but it should be 
scriptable since after all VRC is just a Python package + a nice CLI.

Thanks,

Paolo



label coroutine_fn_candidate aio_get_thread_pool
label coroutine_fn_candidate aio_task_pool_free
label coroutine_fn_candidate aio_task_pool_status
label coroutine_fn_candidate bdrv_bsc_fill
label coroutine_fn_candidate bdrv_bsc_invalidate_range
label coroutine_fn_candidate bdrv_bsc_is_data
label coroutine_fn_candidate bdrv_can_write_zeroes_with_unmap
label coroutine_fn_candidate bdrv_check_request
label coroutine_fn_candidate bdrv_dirty_bitmap_get
label coroutine_fn_candidate bdrv_dirty_bitmap_get_locked
label coroutine_fn_candidate bdrv_dirty_bitmap_lock
label coroutine_fn_candidate bdrv_dirty_bitmap_next_dirty_area
label coroutine_fn_candidate bdrv_dirty_bitmap_next_zero
label coroutine_fn_candidate bdrv_dirty_bitmap_set_inconsistent
label coroutine_fn_candidate bdrv_dirty_bitmap_status
label coroutine_fn_candidate bdrv_dirty_bitmap_truncate
label coroutine_fn_candidate bdrv_dirty_bitmap_unlock
label coroutine_fn_candidate bdrv_dirty_iter_free
label coroutine_fn_candidate bdrv_dirty_iter_new
label coroutine_fn_candidate bdrv_dirty_iter_next
label coroutine_fn_candidate bdrv_has_readonly_bitmaps
label coroutine_fn_candidate bdrv_inc_in_flight
label coroutine_fn_candidate bdrv_min_mem_align
label coroutine_fn_candidate bdrv_pad_request
label coroutine_fn_candidate bdrv_probe_all
label coroutine_fn_candidate bdrv_reset_dirty_bitmap_locked
label coroutine_fn_candidate bdrv_round_to_clusters
label coroutine_fn_candidate bdrv_set_dirty
label coroutine_fn_candidate bdrv_set_dirty_iter
label coroutine_fn_candidate bdrv_write_threshold_check_write
label coroutine_fn_candidate blk_check_byte_request
label coroutine_fn_candidate blkverify_err
label coroutine_fn_candidate block_copy_async
label coroutine_fn_candidate block_copy_call_cancel
label coroutine_fn_candidate block_copy_call_cancelled
label coroutine_fn_candidate block_copy_call_failed
label coroutine_fn_candidate block_copy_call_finished
label coroutine_fn_candidate block_copy_call_free
label coroutine_fn_candidate block_copy_call_status
label coroutine_fn_candidate block_copy_call_succeeded
label coroutine_fn_candidate block_copy_reset_unallocated
label coroutine_fn_candidate block_copy_set_skip_unallocated
label coroutine_fn_candidate block_crypto_co_create_generic
label coroutine_fn_candidate BlockDriver.bdrv_aio_flush
label coroutine_fn_candidate BlockDriver.bdrv_aio_ioctl
label coroutine_fn_candidate BlockDriver.bdrv_aio_pdiscard
label coroutine_fn_candidate BlockDriver.bdrv_aio_preadv
label coroutine_fn_candidate BlockDriver.bdrv_aio_pwritev
label coroutine_fn_candidate BlockDriver.bdrv_co_drain_end
label coroutine_fn_candidate block_job_ratelimit_get_delay
label coroutine_fn_candidate block_job_ratelimit_get_delay
label coroutine_fn_candidate block_status
label coroutine_fn_candidate calculate_l2_meta
label coroutine_fn_candidate check_directory_consistency
label coroutine_fn_candidate commit_direntries
label coroutine_fn_candidate commit_one_file
label coroutine_fn_candidate count_single_write_clusters
label coroutine_fn_candidate iov_send_recv
label coroutine_fn_candidate is_allocated_sectors
label coroutine_fn_candidate iscsi_allocmap_is_allocated
label coroutine_fn_candidate iscsi_allocmap_is_valid
label coroutine_fn_candidate iscsi_allocmap_update
label coroutine_fn_candidate iscsi_populate_target_desc
label coroutine_fn_candidate is_sector_request_lun_aligned
label coroutine_fn_candidate is_zero
label coroutine_fn_candidate job_cancel_requested
label coroutine_fn_candidate job_enter
label coroutine_fn_candidate job_is_cancelled
label coroutine_fn_candidate job_progress_increase_remaining
label coroutine_fn_candidate job_progress_set_remaining
label coroutine_fn_candidate job_progress_update
label coroutine_fn_candidate job_transition_to_ready
label coroutine_fn_candidate mirror_flush
label coroutine_fn_candidate mirror_perform
label coroutine_fn_candidate nbd_client_will_reconnect
label coroutine_fn_candidate nbd_client_will_reconnect
label coroutine_fn_candidate nbd_err_lookup
label coroutine_fn_candidate nbd_errno_to_system_errno
label coroutine_fn_candidate nbd_iter_channel_error
label coroutine_fn_candidate nfs_file_co_create
label coroutine_fn_candidate nvme_get_free_req
label coroutine_fn_candidate nvme_put_free_req_and_wake
label coroutine_fn_candidate pstrcat
label coroutine_fn_candidate qapi_event_send_quorum_failure
label coroutine_fn_candidate qcow2_alloc_bytes
label coroutine_fn_candidate qcow2_alloc_cluster_abort
label coroutine_fn_candidate qcow2_alloc_clusters_at
label coroutine_fn_candidate qcow2_check_refcounts
label coroutine_fn_candidate qcow2_check_vmstate_request
label coroutine_fn_candidate qcow2_get_last_cluster
label coroutine_fn_candidate qcow2_read_extensions
label coroutine_fn_candidate qcow2_read_snapshots
label coroutine_fn_candidate qcow2_truncate_bitmaps_check
label coroutine_fn_candidate qcow2_update_options
label coroutine_fn_candidate qcrypto_block_decrypt
label coroutine_fn_candidate qcrypto_block_encrypt
label coroutine_fn_candidate qdict_rename_keys
label coroutine_fn_candidate qed_alloc_l2_cache_entry
label coroutine_fn_candidate qed_alloc_table
label coroutine_fn_candidate qed_commit_l2_cache_entry
label coroutine_fn_candidate qed_set_used_clusters
label coroutine_fn_candidate qemu_blockalign0
label coroutine_fn_candidate qemu_coroutine_enter_if_inactive
label coroutine_fn_candidate qemu_iovec_clone
label coroutine_fn_candidate qemu_iovec_compare
label coroutine_fn_candidate qemu_iovec_init_slice
label coroutine_fn_candidate qemu_iovec_is_zero
label coroutine_fn_candidate qemu_iovec_subvec_niov
label coroutine_fn_candidate qemu_iovec_to_buf
label coroutine_fn_candidate qemu_rbd_co_create
label coroutine_fn_candidate qio_channel_readv
label coroutine_fn_candidate qio_channel_readv_all
label coroutine_fn_candidate qio_channel_readv_all_eof
label coroutine_fn_candidate quorum_copy_qiov
label coroutine_fn_candidate quorum_report_bad_acb
label coroutine_fn_candidate quorum_vote_error
label coroutine_fn_candidate reqlist_find_conflict
label coroutine_fn_candidate reqlist_init_req
label coroutine_fn_candidate rule_check
label coroutine_fn_candidate throttle_account
label coroutine_fn_candidate tracked_request_set_serialising
label coroutine_fn_candidate vhdx_block_translate
label coroutine_fn_candidate vhdx_log_write_and_flush
label coroutine_fn_candidate vhdx_metadata_entry_le_export
label coroutine_fn_candidate vhdx_metadata_header_le_export
label coroutine_fn_candidate vhdx_region_entry_le_export
label coroutine_fn_candidate vhdx_region_header_le_export
label coroutine_fn_candidate vhdx_update_bat_table_entry
label coroutine_fn_candidate vmdk_is_cid_valid
label coroutine_fn_candidate warn_reportf_err
label coroutine_fn_candidate yank_register_function
label coroutine_fn_candidate zero_l2_subclusters