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

[PATCH 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

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                            | 107 +++++++++++++++++------------
 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, 178 insertions(+), 130 deletions(-)

-- 
2.31.1

[PATCH 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.

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

diff --git a/block.c b/block.c
index 5311b21f8e..7211f62001 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,10 @@ 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);
+        AIO_WAIT_WHILE(qemu_get_aio_context(), dco.ret == NOT_DONE);
     }
 
     return 0;
-- 
2.31.1

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

[Emanuele Giuseppe Esposito]

block_copy_reset_unallocated and block_copy_is_cluster_allocated are
only called by backup_run, a corotuine_fn itself.

Same applies to block_copy_block_status, called by
block_copy_dirty_clusters.

Therefore mark them as coroutine 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 3/9] nbd/server.c: add missing coroutine_fn annotations

[Emanuele Giuseppe Esposito]

There are probably more missing, but right now it is necessary that
we extend coroutine_fn to block{allock/status}_to_extents, because
they use bdrv_* functions calling the generated_co_wrapper API, which
checks for the qemu_in_coroutine() case.

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 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 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 7211f62001..eeb7a02aa2 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 6/9] block/vmdk: add missing coroutine_fn annotations

[Emanuele Giuseppe Esposito]

vmdk_co_create_opts() is a coroutine_fn, and calls vmdk_co_do_create()
which in turn can call two callbacks: vmdk_co_create_opts_cb and
vmdk_co_create_cb.

Mark all these functions as coroutine_fn, since vmdk_co_create_opts()
is the only caller.

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 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 eeb7a02aa2..e5e70acf15 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 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>
---
 block.c | 37 ++++++++++++++++---------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/block.c b/block.c
index e5e70acf15..1ee76a8694 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 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 2/9] block-copy: add missing coroutine_fn annotations

[Paolo Bonzini]

On 11/3/22 14:41, Emanuele Giuseppe Esposito wrote:
> block_copy_reset_unallocated and block_copy_is_cluster_allocated are
> only called by backup_run, a corotuine_fn itself.
> 
> Same applies to block_copy_block_status, called by
> block_copy_dirty_clusters.
> 
> Therefore mark them as coroutine too.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

They don't need to be coroutine_fn.  coroutine_fn is needed if you call 
another coroutine_fn, but not if you _are only called_ by coroutine_fn. 
There is nothing in these functions that needs them to be called from a 
coroutine.

The only exception is qemu_coroutine_yield(), which is the only leaf 
coroutine_fn.

Paolo

> ---
>   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 3/9] nbd/server.c: add missing coroutine_fn annotations

[Paolo Bonzini]

On 11/3/22 14:42, Emanuele Giuseppe Esposito wrote:
> There are probably more missing, but right now it is necessary that
> we extend coroutine_fn to block{allock/status}_to_extents, because
> they use bdrv_* functions calling the generated_co_wrapper API, which
> checks for the qemu_in_coroutine() case.
> 
> Signed-off-by: Emanuele Giuseppe Esposito<eesposit@redhat.com>

generated_co_wrappers should only be called from functions that are 
*not* coroutine_fn.  If they are coroutine_fn, they can call the 
bdrv_co_* version directly.

See for example 
https://patchew.org/QEMU/20221013123711.620631-1-pbonzini@redhat.com/20221013123711.620631-17-pbonzini@redhat.com/.

Paolo

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

[Paolo Bonzini]

On 11/3/22 14:42, Emanuele Giuseppe Esposito wrote:
> vmdk_co_create_opts() is a coroutine_fn, and calls vmdk_co_do_create()
> which in turn can call two callbacks: vmdk_co_create_opts_cb and
> vmdk_co_create_cb.
> 
> Mark all these functions as coroutine_fn, since vmdk_co_create_opts()
> is the only caller.
> 
> 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;

Should not be needed either.

Paolo

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

[Paolo Bonzini]

On 11/3/22 14:42, Emanuele Giuseppe Esposito wrote:
> 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 eeb7a02aa2..e5e70acf15 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,

Ah, I see now why patch 6 is needed, but please adjust the commit message.

Paolo

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

[Paolo Bonzini]

On 11/3/22 14:41, Emanuele Giuseppe Esposito wrote:
> -    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);
> +        AIO_WAIT_WHILE(qemu_get_aio_context(), dco.ret == NOT_DONE);
>       }
>   

Alternatively 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.  So 
you could also assert that the coroutine is terminated after 
qemu_coroutine_enter(), i.e. that dco.ret != NOT_DONE.

Since you need to respin, perhaps put it the above in the commit message 
in case this needs a change in the future; however your patch is simple 
and should indeed work, so

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

Paolo

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

[Paolo Bonzini]

On 11/3/22 14:42, Emanuele Giuseppe Esposito wrote:
> Delete the if case and make sure it won't be called again
> in coroutines.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block.c | 37 ++++++++++++++++---------------------
>   1 file changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/block.c b/block.c
> index e5e70acf15..1ee76a8694 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;
>   }
>   
>   /**

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

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

[Kevin Wolf]

Am 03.11.2022 um 17:56 hat Paolo Bonzini geschrieben:
> On 11/3/22 14:41, Emanuele Giuseppe Esposito wrote:
> > block_copy_reset_unallocated and block_copy_is_cluster_allocated are
> > only called by backup_run, a corotuine_fn itself.

s/corotuine_fn/coroutine_fn/

> > 
> > Same applies to block_copy_block_status, called by
> > block_copy_dirty_clusters.
> > 
> > Therefore mark them as coroutine too.
> > 
> > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> They don't need to be coroutine_fn.  coroutine_fn is needed if you call
> another coroutine_fn, but not if you _are only called_ by coroutine_fn.
> There is nothing in these functions that needs them to be called from a
> coroutine.
> 
> The only exception is qemu_coroutine_yield(), which is the only leaf
> coroutine_fn.

I think it can make sense to have coroutine_fn as a documentation for
things that are only ever called in a coroutine even if they could
theoretically also work outside of coroutine context.

Otherwise, when we want to introduce a coroutine_fn call somewhere, it's
not only less obvious that it's even possible to do, but we'll have to
add potentially many additional coroutine_fn annotations in the whole
call chain in an otherwise unrelated patch.

Kevin

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

[Paolo Bonzini]

On 11/3/22 19:06, Kevin Wolf wrote:
> I think it can make sense to have coroutine_fn as a documentation for
> things that are only ever called in a coroutine even if they could
> theoretically also work outside of coroutine context.
> 
> Otherwise, when we want to introduce a coroutine_fn call somewhere, it's
> not only less obvious that it's even possible to do, but we'll have to
> add potentially many additional coroutine_fn annotations in the whole
> call chain in an otherwise unrelated patch.

This is true.  On the other hand, coroutine_fn also means "this is 
allowed to suspend", which may have implications on the need for locking 
in the caller.  So you need to judge case-by-case.

If there are good reasons to add the note, you could add an assertion 
that you are qemu_in_coroutine(), which notes that you are in a 
coroutine but you don't suspend.  In this case however I don't think 
it's likely that there will be a coroutine_fn call added later.

I guess I tend to err on the side of "it's good that it's not obvious 
that you can call a coroutine_fn", but I also need to correct myself as 
qemu_coroutine_yield() is not the only leaf; there is also 
qemu_co_queue_next() and qemu_co_queue_restart_all(), which are 
coroutine_fn because they rely on the queuing behavior of 
aio_co_enter().  In this case I violated my own rule because it is 
always a bug to call these functions outside coroutine context.

Paolo

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

[Emanuele Giuseppe Esposito]

Am 03/11/2022 um 19:36 schrieb Paolo Bonzini:
> On 11/3/22 19:06, Kevin Wolf wrote:
>> I think it can make sense to have coroutine_fn as a documentation for
>> things that are only ever called in a coroutine even if they could
>> theoretically also work outside of coroutine context.
>>
>> Otherwise, when we want to introduce a coroutine_fn call somewhere, it's
>> not only less obvious that it's even possible to do, but we'll have to
>> add potentially many additional coroutine_fn annotations in the whole
>> call chain in an otherwise unrelated patch.
> 
> This is true.  On the other hand, coroutine_fn also means "this is
> allowed to suspend", which may have implications on the need for locking
> in the caller.  So you need to judge case-by-case.
> 
> If there are good reasons to add the note, you could add an assertion
> that you are qemu_in_coroutine(), which notes that you are in a
> coroutine but you don't suspend.  In this case however I don't think
> it's likely that there will be a coroutine_fn call added later.
> 
> I guess I tend to err on the side of "it's good that it's not obvious
> that you can call a coroutine_fn", but I also need to correct myself as
> qemu_coroutine_yield() is not the only leaf; there is also
> qemu_co_queue_next() and qemu_co_queue_restart_all(), which are
> coroutine_fn because they rely on the queuing behavior of
> aio_co_enter().  In this case I violated my own rule because it is
> always a bug to call these functions outside coroutine context.
> 

But isn't it a bug also not to mark a function _only_ called by
coroutine_fn? My point is that if this function is an implementation of
a BlockDriver callback marked as coroutine_fn (like in patch 6 with
vmdk), then it would make sense.

This is actually the point of this serie (which I might not have
explained well in the cover letter), every function marked here is
eventually called by/calling a BlockDriver callback marked as coroutine_fn.

Currently we have something like this:
BlockDriver {
    void coroutine_fn (*bdrv_A)(void) = implA;
}

void coroutine_fn implA() {
    funcB();
    funcC();
}

void funcB() {}; <--- missing coroutine_fn?
void funcC() {}; <--- missing coroutine_fn?

In addition, as I understand draining is not allowed in coroutines. If a
function/callback only running in coroutines is not marked as
coroutine_fn, then it will be less obvious to notice that draining is
not allowed there.

Emanuele

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

[Paolo Bonzini]

On 11/4/22 08:35, Emanuele Giuseppe Esposito wrote:
> But isn't it a bug also not to mark a function _only_ called by
> coroutine_fn? My point is that if this function is an implementation of
> a BlockDriver callback marked as coroutine_fn (like in patch 6 with
> vmdk), then it would make sense.

If a function implements a coroutine_fn callback but does not suspend, 
then it makes sense to mark it coroutine_fn.

In general it's not a bug.  In most cases it would only be a coincidence 
that the function is called from a coroutine_fn.  For example consider 
bdrv_round_to_clusters().  Marking it coroutine_fn signals that it may 
suspend now (it doesn't) or in the future.  However it's only doing some 
math based on the result of bdrv_get_info(), so it is extremely unlikely 
that this will happen.

In this case... oh wait.  block_copy_is_cluster_allocated is calling 
bdrv_is_allocated, and block_copy_reset_unallocated calls 
block_copy_is_cluster_allocated.  bdrv_is_allocated is a mixed 
coroutine/non-coroutine function, and in this case it is useful to 
document that bdrv_is_allocated will suspend.  The patch is correct, 
only the commit message is wrong.

Likewise for blockstatus_to_extents in patch 3, where the commit message 
does mention bdrv_* functions.  As I mentioned in my quick review of 
patch 3, this can also snowball into a series of its own to clean up all 
callees of bdrv_co_common_block_status_above, similar to what Alberto 
did for read/write functions back in June, so that they are properly 
marked as coroutine_fn.  If you want to do it, don't do it by hand 
though, you can use his static analyzer.  It's slow but it's faster than 
doing it by hand.

> This is actually the point of this serie (which I might not have
> explained well in the cover letter), every function marked here is
> eventually called by/calling a BlockDriver callback marked as coroutine_fn.

Again I don't think this is useful in general, but your three patches 
(2/3/6) did catch cases that wants to be coroutine_fn.  So my objection 
is dropped with just a better commit message.

> Currently we have something like this:
> BlockDriver {
>      void coroutine_fn (*bdrv_A)(void) = implA;
> }
> 
> void coroutine_fn implA() {
>      funcB();
>      funcC();
> }
> 
> void funcB() {}; <--- missing coroutine_fn?
> void funcC() {}; <--- missing coroutine_fn?
> 
> In addition, as I understand draining is not allowed in coroutines.

... except we have bdrv_co_yield_to_drain() to allow that, sort of. :/

> If a function/callback only running in coroutines is not marked as
> coroutine_fn, then it will be less obvious to notice that draining is
> not allowed there.

I think it has to be judged case by base.  Your patches prove that, in 
most cases, you have coroutine_fn for things that ultimately do some 
kind of I/O or query.  In general the interesting path to explore is 
"coroutine_fn calls (indirectly) non-coroutine_fn calls (indirectly) 
generated_co_wrapper".  The vrc tool could be extended to help finding 
them, with commands like

     label coroutine_fn bdrv_co_read
     label coroutine_fn bdrv_co_write
     ...
     label generated_co_wrapper bdrv_read
     label generated_co_wrapper bdrv_write
     paths coroutine_fn !coroutine_fn generated_co_wrapper

Paolo

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

[Emanuele Giuseppe Esposito]

Am 04/11/2022 um 09:44 schrieb Paolo Bonzini:
> On 11/4/22 08:35, Emanuele Giuseppe Esposito wrote:
>> But isn't it a bug also not to mark a function _only_ called by
>> coroutine_fn? My point is that if this function is an implementation of
>> a BlockDriver callback marked as coroutine_fn (like in patch 6 with
>> vmdk), then it would make sense.
> 
> If a function implements a coroutine_fn callback but does not suspend,
> then it makes sense to mark it coroutine_fn.
> 
> In general it's not a bug.  In most cases it would only be a coincidence
> that the function is called from a coroutine_fn.  For example consider
> bdrv_round_to_clusters().  Marking it coroutine_fn signals that it may
> suspend now (it doesn't) or in the future.  However it's only doing some
> math based on the result of bdrv_get_info(), so it is extremely unlikely
> that this will happen.
> 
> In this case... oh wait.  block_copy_is_cluster_allocated is calling
> bdrv_is_allocated, and block_copy_reset_unallocated calls
> block_copy_is_cluster_allocated.  bdrv_is_allocated is a mixed
> coroutine/non-coroutine function, and in this case it is useful to
> document that bdrv_is_allocated will suspend.  The patch is correct,
> only the commit message is wrong.

[...]

>> This is actually the point of this serie (which I might not have
>> explained well in the cover letter), every function marked here is
>> eventually called by/calling a BlockDriver callback marked as
>> coroutine_fn.
> 
> Again I don't think this is useful in general, but your three patches
> (2/3/6) did catch cases that wants to be coroutine_fn.  So my objection
> is dropped with just a better commit message.

I see your point about "in general it's not a bug".
At this point I just want to make sure that we agree that it's correct
to add coroutine_fn because of "the function calls a g_c_w that
suspends" *&&* "all the callers are coroutine_fn". If the callers
weren't coroutine_fn then g_c_w would just create a new coroutine and
poll, and I don't think that would be part of your definition of "can
suspend".

Thank you,
Emanuele

> 
>> Currently we have something like this:
>> BlockDriver {
>>      void coroutine_fn (*bdrv_A)(void) = implA;
>> }
>>
>> void coroutine_fn implA() {
>>      funcB();
>>      funcC();
>> }
>>
>> void funcB() {}; <--- missing coroutine_fn?
>> void funcC() {}; <--- missing coroutine_fn?
>>
>> In addition, as I understand draining is not allowed in coroutines.
> 
> ... except we have bdrv_co_yield_to_drain() to allow that, sort of. :/
> 
>> If a function/callback only running in coroutines is not marked as
>> coroutine_fn, then it will be less obvious to notice that draining is
>> not allowed there.
> 
> I think it has to be judged case by base.  Your patches prove that, in
> most cases, you have coroutine_fn for things that ultimately do some
> kind of I/O or query.  In general the interesting path to explore is
> "coroutine_fn calls (indirectly) non-coroutine_fn calls (indirectly)
> generated_co_wrapper".  The vrc tool could be extended to help finding
> them, with commands like
> 
>     label coroutine_fn bdrv_co_read
>     label coroutine_fn bdrv_co_write
>     ...
>     label generated_co_wrapper bdrv_read
>     label generated_co_wrapper bdrv_write
>     paths coroutine_fn !coroutine_fn generated_co_wrapper
> 
> Paolo
> 

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

[Kevin Wolf]

Am 04.11.2022 um 09:44 hat Paolo Bonzini geschrieben:
> On 11/4/22 08:35, Emanuele Giuseppe Esposito wrote:
> > But isn't it a bug also not to mark a function _only_ called by
> > coroutine_fn? My point is that if this function is an implementation of
> > a BlockDriver callback marked as coroutine_fn (like in patch 6 with
> > vmdk), then it would make sense.
> 
> If a function implements a coroutine_fn callback but does not suspend, then
> it makes sense to mark it coroutine_fn.
> 
> In general it's not a bug.  In most cases it would only be a coincidence
> that the function is called from a coroutine_fn.  For example consider
> bdrv_round_to_clusters().  Marking it coroutine_fn signals that it may
> suspend now (it doesn't) or in the future.  However it's only doing some
> math based on the result of bdrv_get_info(), so it is extremely unlikely
> that this will happen.
> 
> In this case... oh wait.  block_copy_is_cluster_allocated is calling
> bdrv_is_allocated, and block_copy_reset_unallocated calls
> block_copy_is_cluster_allocated.  bdrv_is_allocated is a mixed
> coroutine/non-coroutine function, and in this case it is useful to document
> that bdrv_is_allocated will suspend.  The patch is correct, only the commit
> message is wrong.

Ah, right, now I remember that this is what I had discussed with
Emanuele. I knew that there was a reason for it...

What we probably should really do is a bdrv_co_is_allocated() that
doesn't go through the mixed function, but directly calls
bdrv_co_common_block_status_above(). bdrv_is_allocated() is only a
smaller wrapper anyway, so it wouldn't duplicate much code.

I seem to remember that Emanuele had a few more bdrv_is_allocated()
calls that actually took the coroutine path and could use the same new
function.

Kevin

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

[Paolo Bonzini]

On 11/4/22 10:20, Emanuele Giuseppe Esposito wrote:
> At this point I just want to make sure that we agree that it's correct
> to add coroutine_fn because of "the function calls a g_c_w that
> suspends" *&&* "all the callers are coroutine_fn". If the callers
> weren't coroutine_fn then g_c_w would just create a new coroutine and
> poll, and I don't think that would be part of your definition of "can
> suspend".

Yes, we agree on that.  The confusion was just on the commit message.

The even-better fix would be to also call coroutine_fn from 
coroutine_fn, instead of calling mixed coroutine/non-coroutine functions 
such as g_c_w functions.  However, adding coroutine_fn *is* correct.

Paolo