[PATCH 00/13] block: Fix bdrv_open*() calls from coroutine context

[PATCH 00/13] block: Fix bdrv_open*() calls from coroutine context

[Kevin Wolf]

bdrv_open*() must not be called from coroutine context, amongst others
because it modifies the block graph. However, some functions - in
particular all .bdrv_co_create* implementations of image formats - do
call it from coroutine context. This is already wrong today, but when we
add locking, it actually becomes visible.

This series adds no_co_wrapper functions, which are automatically
generated wrappers that run in coroutine context and use a BH to call
the wrapped function outside of coroutine context. It then uses these
wrappers to fix the problematic bdrv_open*() calls.

Kevin Wolf (13):
  block-coroutine-wrapper: Introduce no_co_wrapper
  block: Create no_co_wrappers for open functions
  luks: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper
  parallels: Fix .bdrv_co_create(_opts) to open images with
    no_co_wrapper
  qcow: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper
  qcow2: Fix open/create to open images with no_co_wrapper
  qed: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper
  vdi: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper
  vhdx: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper
  vmdk: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper
  vpc: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper
  block: Fix bdrv_co_create_opts_simple() to open images with
    no_co_wrapper
  block: Assert non-coroutine context for bdrv_open_inherit()

 include/block/block-common.h                | 14 ++++
 include/block/block-global-state.h          | 35 ++++++---
 include/sysemu/block-backend-global-state.h | 21 +++++-
 block.c                                     | 17 ++---
 block/crypto.c                              | 10 +--
 block/parallels.c                           | 10 +--
 block/qcow.c                                | 10 +--
 block/qcow2.c                               | 43 +++++------
 block/qed.c                                 | 10 +--
 block/vdi.c                                 | 10 +--
 block/vhdx.c                                | 10 +--
 block/vmdk.c                                | 22 +++---
 block/vpc.c                                 | 10 +--
 scripts/block-coroutine-wrapper.py          | 83 ++++++++++++++++++---
 block/meson.build                           |  1 +
 15 files changed, 207 insertions(+), 99 deletions(-)

-- 
2.38.1

[PATCH 01/13] block-coroutine-wrapper: Introduce no_co_wrapper

[Kevin Wolf]

Some functions must not be called from coroutine context. The common
pattern to use them anyway from a coroutine is running them in a BH and
letting the calling coroutine yield to be woken up when the BH is
completed.

Instead of manually writing such wrappers, add support for generating
them to block-coroutine-wrapper.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block-common.h       | 14 +++++
 scripts/block-coroutine-wrapper.py | 83 ++++++++++++++++++++++++++----
 2 files changed, 86 insertions(+), 11 deletions(-)

diff --git a/include/block/block-common.h b/include/block/block-common.h
index 469300fe8d..b5122ef8ab 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -54,6 +54,20 @@
 #define co_wrapper_bdrv_rdlock         no_coroutine_fn
 #define co_wrapper_mixed_bdrv_rdlock   no_coroutine_fn coroutine_mixed_fn
 
+/*
+ * no_co_wrapper: Function specifier used by block-coroutine-wrapper.py
+ *
+ * Function specifier which does nothing but mark functions to be generated by
+ * scripts/block-coroutine-wrapper.py.
+ *
+ * A no_co_wrapper function declaration creates a coroutine_fn wrapper around
+ * functions that must not be called in coroutine context. It achieves this by
+ * scheduling a BH in the bottom half that runs the respective non-coroutine
+ * function. The coroutine yields after scheduling the BH and is reentered when
+ * the wrapped function returns.
+ */
+#define no_co_wrapper
+
 #include "block/blockjob.h"
 
 /* block.c */
diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
index e82b648127..60e9b3107c 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -63,8 +63,8 @@ def __init__(self, param_decl: str) -> None:
 
 
 class FuncDecl:
-    def __init__(self, return_type: str, name: str, args: str,
-                 variant: str) -> None:
+    def __init__(self, wrapper_type: str, return_type: str, name: str,
+                 args: str, variant: str) -> None:
         self.return_type = return_type.strip()
         self.name = name.strip()
         self.struct_name = snake_to_camel(self.name)
@@ -72,8 +72,21 @@ def __init__(self, return_type: str, name: str, args: str,
         self.create_only_co = 'mixed' not in variant
         self.graph_rdlock = 'bdrv_rdlock' in variant
 
-        subsystem, subname = self.name.split('_', 1)
-        self.co_name = f'{subsystem}_co_{subname}'
+        self.wrapper_type = wrapper_type
+
+        if wrapper_type == 'co':
+            subsystem, subname = self.name.split('_', 1)
+            self.target_name = f'{subsystem}_co_{subname}'
+        else:
+            assert wrapper_type == 'no_co'
+            subsystem, co_infix, subname = self.name.split('_', 2)
+            if co_infix != 'co':
+                raise ValueError(f"Invalid no_co function name: {self.name}")
+            if not self.create_only_co:
+                raise ValueError(f"no_co function can't be mixed: {self.name}")
+            if self.graph_rdlock:
+                raise ValueError(f"no_co function can't be rdlock: {self.name}")
+            self.target_name = f'{subsystem}_{subname}'
 
         t = self.args[0].type
         if t == 'BlockDriverState *':
@@ -105,7 +118,8 @@ def gen_block(self, format: str) -> str:
 
 # Match wrappers declared with a co_wrapper mark
 func_decl_re = re.compile(r'^(?P<return_type>[a-zA-Z][a-zA-Z0-9_]* [\*]?)'
-                          r'\s*co_wrapper'
+                          r'(\s*coroutine_fn)?'
+                          r'\s*(?P<wrapper_type>(no_)?co)_wrapper'
                           r'(?P<variant>(_[a-z][a-z0-9_]*)?)\s*'
                           r'(?P<wrapper_name>[a-z][a-z0-9_]*)'
                           r'\((?P<args>[^)]*)\);$', re.MULTILINE)
@@ -113,7 +127,8 @@ def gen_block(self, format: str) -> str:
 
 def func_decl_iter(text: str) -> Iterator:
     for m in func_decl_re.finditer(text):
-        yield FuncDecl(return_type=m.group('return_type'),
+        yield FuncDecl(wrapper_type=m.group('wrapper_type'),
+                       return_type=m.group('return_type'),
                        name=m.group('wrapper_name'),
                        args=m.group('args'),
                        variant=m.group('variant'))
@@ -133,7 +148,7 @@ def create_mixed_wrapper(func: FuncDecl) -> str:
     """
     Checks if we are already in coroutine
     """
-    name = func.co_name
+    name = func.target_name
     struct_name = func.struct_name
     graph_assume_lock = 'assume_graph_lock();' if func.graph_rdlock else ''
 
@@ -163,7 +178,7 @@ def create_co_wrapper(func: FuncDecl) -> str:
     """
     Assumes we are not in coroutine, and creates one
     """
-    name = func.co_name
+    name = func.target_name
     struct_name = func.struct_name
     return f"""\
 {func.return_type} {func.name}({ func.gen_list('{decl}') })
@@ -183,10 +198,11 @@ def create_co_wrapper(func: FuncDecl) -> str:
 }}"""
 
 
-def gen_wrapper(func: FuncDecl) -> str:
+def gen_co_wrapper(func: FuncDecl) -> str:
     assert not '_co_' in func.name
+    assert func.wrapper_type == 'co'
 
-    name = func.co_name
+    name = func.target_name
     struct_name = func.struct_name
 
     graph_lock=''
@@ -225,11 +241,56 @@ def gen_wrapper(func: FuncDecl) -> str:
 {creation_function(func)}"""
 
 
+def gen_no_co_wrapper(func: FuncDecl) -> str:
+    assert '_co_' in func.name
+    assert func.wrapper_type == 'no_co'
+
+    name = func.target_name
+    struct_name = func.struct_name
+
+    return f"""\
+/*
+ * Wrappers for {name}
+ */
+
+typedef struct {struct_name} {{
+    Coroutine *co;
+    {func.return_field}
+{ func.gen_block('    {decl};') }
+}} {struct_name};
+
+static void {name}_bh(void *opaque)
+{{
+    {struct_name} *s = opaque;
+
+    {func.get_result}{name}({ func.gen_list('s->{name}') });
+
+    aio_co_wake(s->co);
+}}
+
+{func.return_type} coroutine_fn {func.name}({ func.gen_list('{decl}') })
+{{
+    {struct_name} s = {{
+        .co = qemu_coroutine_self(),
+{ func.gen_block('        .{name} = {name},') }
+    }};
+    assert(qemu_in_coroutine());
+
+    aio_bh_schedule_oneshot(qemu_get_aio_context(), {name}_bh, &s);
+    qemu_coroutine_yield();
+
+    {func.ret}
+}}"""
+
+
 def gen_wrappers(input_code: str) -> str:
     res = ''
     for func in func_decl_iter(input_code):
         res += '\n\n\n'
-        res += gen_wrapper(func)
+        if func.wrapper_type == 'co':
+            res += gen_co_wrapper(func)
+        else:
+            res += gen_no_co_wrapper(func)
 
     return res
 
-- 
2.38.1

[PATCH 02/13] block: Create no_co_wrappers for open functions

[Kevin Wolf]

Images can't be opened in coroutine context because opening needs to
change the block graph. Add no_co_wrappers so that coroutines have a
simple way of opening images in a BH instead.

At the same time, mark the wrapped functions as no_coroutine_fn.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block-global-state.h          | 35 +++++++++++++++------
 include/sysemu/block-backend-global-state.h | 21 ++++++++++---
 block/meson.build                           |  1 +
 3 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index a38f86dc15..447176414e 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -77,16 +77,26 @@ BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
                                    int flags, Error **errp);
 int bdrv_drop_filter(BlockDriverState *bs, Error **errp);
 
-BdrvChild *bdrv_open_child(const char *filename,
-                           QDict *options, const char *bdref_key,
-                           BlockDriverState *parent,
-                           const BdrvChildClass *child_class,
-                           BdrvChildRole child_role,
-                           bool allow_none, Error **errp);
+BdrvChild * no_coroutine_fn
+bdrv_open_child(const char *filename, QDict *options, const char *bdref_key,
+                BlockDriverState *parent, const BdrvChildClass *child_class,
+                BdrvChildRole child_role, bool allow_none, Error **errp);
+
+BdrvChild * coroutine_fn no_co_wrapper
+bdrv_co_open_child(const char *filename, QDict *options, const char *bdref_key,
+                BlockDriverState *parent, const BdrvChildClass *child_class,
+                BdrvChildRole child_role, bool allow_none, Error **errp);
+
 int bdrv_open_file_child(const char *filename,
                          QDict *options, const char *bdref_key,
                          BlockDriverState *parent, Error **errp);
-BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
+
+BlockDriverState * no_coroutine_fn
+bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
+
+BlockDriverState * coroutine_fn no_co_wrapper
+bdrv_co_open_blockdev_ref(BlockdevRef *ref, Error **errp);
+
 int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
                         Error **errp);
 int bdrv_set_backing_hd_drained(BlockDriverState *bs,
@@ -94,8 +104,15 @@ int bdrv_set_backing_hd_drained(BlockDriverState *bs,
                                 Error **errp);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
                            const char *bdref_key, Error **errp);
-BlockDriverState *bdrv_open(const char *filename, const char *reference,
-                            QDict *options, int flags, Error **errp);
+
+BlockDriverState * no_coroutine_fn
+bdrv_open(const char *filename, const char *reference, QDict *options,
+          int flags, Error **errp);
+
+BlockDriverState * coroutine_fn no_co_wrapper
+bdrv_co_open(const char *filename, const char *reference,
+             QDict *options, int flags, Error **errp);
+
 BlockDriverState *bdrv_new_open_driver_opts(BlockDriver *drv,
                                             const char *node_name,
                                             QDict *options, int flags,
diff --git a/include/sysemu/block-backend-global-state.h b/include/sysemu/block-backend-global-state.h
index 6858e39cb6..2b6d27db7c 100644
--- a/include/sysemu/block-backend-global-state.h
+++ b/include/sysemu/block-backend-global-state.h
@@ -23,10 +23,23 @@
  */
 
 BlockBackend *blk_new(AioContext *ctx, uint64_t perm, uint64_t shared_perm);
-BlockBackend *blk_new_with_bs(BlockDriverState *bs, uint64_t perm,
-                              uint64_t shared_perm, Error **errp);
-BlockBackend *blk_new_open(const char *filename, const char *reference,
-                           QDict *options, int flags, Error **errp);
+
+BlockBackend * no_coroutine_fn
+blk_new_with_bs(BlockDriverState *bs, uint64_t perm, uint64_t shared_perm,
+                Error **errp);
+
+BlockBackend * coroutine_fn no_co_wrapper
+blk_co_new_with_bs(BlockDriverState *bs, uint64_t perm, uint64_t shared_perm,
+                   Error **errp);
+
+BlockBackend * no_coroutine_fn
+blk_new_open(const char *filename, const char *reference, QDict *options,
+             int flags, Error **errp);
+
+BlockBackend * coroutine_fn no_co_wrapper
+blk_co_new_open(const char *filename, const char *reference, QDict *options,
+                int flags, Error **errp);
+
 int blk_get_refcnt(BlockBackend *blk);
 void blk_ref(BlockBackend *blk);
 void blk_unref(BlockBackend *blk);
diff --git a/block/meson.build b/block/meson.build
index 3662852dc2..382bec0e7d 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -141,6 +141,7 @@ block_gen_c = custom_target('block-gen.c',
                                       '../include/block/dirty-bitmap.h',
                                       '../include/block/block_int-io.h',
                                       '../include/block/block-global-state.h',
+                                      '../include/sysemu/block-backend-global-state.h',
                                       '../include/sysemu/block-backend-io.h',
                                       'coroutines.h'
                                       ),
-- 
2.38.1

[PATCH 03/13] luks: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper

[Kevin Wolf]

.bdrv_co_create implementations run in a coroutine. Therefore they are
not allowed to open images directly. Fix the calls to use the
corresponding no_co_wrappers instead.

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

diff --git a/block/crypto.c b/block/crypto.c
index b70cec97c7..9c477397c7 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -325,8 +325,8 @@ static int block_crypto_co_create_generic(BlockDriverState *bs,
     QCryptoBlock *crypto = NULL;
     struct BlockCryptoCreateData data;
 
-    blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
-                          errp);
+    blk = blk_co_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
+                             errp);
     if (!blk) {
         ret = -EPERM;
         goto cleanup;
@@ -639,7 +639,7 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp)
     assert(create_options->driver == BLOCKDEV_DRIVER_LUKS);
     luks_opts = &create_options->u.luks;
 
-    bs = bdrv_open_blockdev_ref(luks_opts->file, errp);
+    bs = bdrv_co_open_blockdev_ref(luks_opts->file, errp);
     if (bs == NULL) {
         return -EIO;
     }
@@ -708,8 +708,8 @@ static int coroutine_fn block_crypto_co_create_opts_luks(BlockDriver *drv,
         goto fail;
     }
 
-    bs = bdrv_open(filename, NULL, NULL,
-                   BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+    bs = bdrv_co_open(filename, NULL, NULL,
+                      BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
     if (!bs) {
         ret = -EINVAL;
         goto fail;
-- 
2.38.1

[PATCH 04/13] parallels: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper

[Kevin Wolf]

.bdrv_co_create implementations run in a coroutine. Therefore they are
not allowed to open images directly. Fix the calls to use the
corresponding no_co_wrappers instead.

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

diff --git a/block/parallels.c b/block/parallels.c
index bbea2f2221..d4378e09de 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -565,13 +565,13 @@ static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
     }
 
     /* Create BlockBackend to write to the image */
-    bs = bdrv_open_blockdev_ref(parallels_opts->file, errp);
+    bs = bdrv_co_open_blockdev_ref(parallels_opts->file, errp);
     if (bs == NULL) {
         return -EIO;
     }
 
-    blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
-                          errp);
+    blk = blk_co_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
+                             errp);
     if (!blk) {
         ret = -EPERM;
         goto out;
@@ -651,8 +651,8 @@ static int coroutine_fn parallels_co_create_opts(BlockDriver *drv,
         goto done;
     }
 
-    bs = bdrv_open(filename, NULL, NULL,
-                   BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+    bs = bdrv_co_open(filename, NULL, NULL,
+                      BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
     if (bs == NULL) {
         ret = -EIO;
         goto done;
-- 
2.38.1

[PATCH 05/13] qcow: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper

[Kevin Wolf]

.bdrv_co_create implementations run in a coroutine. Therefore they are
not allowed to open images directly. Fix the calls to use the
corresponding no_co_wrappers instead.

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

diff --git a/block/qcow.c b/block/qcow.c
index 5f0801f545..20c53b447b 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -833,13 +833,13 @@ static int coroutine_fn qcow_co_create(BlockdevCreateOptions *opts,
     }
 
     /* Create BlockBackend to write to the image */
-    bs = bdrv_open_blockdev_ref(qcow_opts->file, errp);
+    bs = bdrv_co_open_blockdev_ref(qcow_opts->file, errp);
     if (bs == NULL) {
         return -EIO;
     }
 
-    qcow_blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE,
-                               BLK_PERM_ALL, errp);
+    qcow_blk = blk_co_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE,
+                                  BLK_PERM_ALL, errp);
     if (!qcow_blk) {
         ret = -EPERM;
         goto exit;
@@ -978,8 +978,8 @@ static int coroutine_fn qcow_co_create_opts(BlockDriver *drv,
         goto fail;
     }
 
-    bs = bdrv_open(filename, NULL, NULL,
-                   BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+    bs = bdrv_co_open(filename, NULL, NULL,
+                      BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
     if (bs == NULL) {
         ret = -EIO;
         goto fail;
-- 
2.38.1

[PATCH 06/13] qcow2: Fix open/create to open images with no_co_wrapper

[Kevin Wolf]

.bdrv_co_create implementations run in a coroutine, as does
qcow2_do_open(). Therefore they are not allowed to open images directly.
Fix the calls to use the corresponding no_co_wrappers instead.

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

diff --git a/block/qcow2.c b/block/qcow2.c
index 21aa4c6b7a..ee0e5b45cc 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1617,9 +1617,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
 
     if (open_data_file) {
         /* Open external data file */
-        s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
-                                       &child_of_bds, BDRV_CHILD_DATA,
-                                       true, errp);
+        s->data_file = bdrv_co_open_child(NULL, options, "data-file", bs,
+                                          &child_of_bds, BDRV_CHILD_DATA,
+                                          true, errp);
         if (*errp) {
             ret = -EINVAL;
             goto fail;
@@ -1627,9 +1627,10 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
 
         if (s->incompatible_features & QCOW2_INCOMPAT_DATA_FILE) {
             if (!s->data_file && s->image_data_file) {
-                s->data_file = bdrv_open_child(s->image_data_file, options,
-                                               "data-file", bs, &child_of_bds,
-                                               BDRV_CHILD_DATA, false, errp);
+                s->data_file = bdrv_co_open_child(s->image_data_file, options,
+                                                  "data-file", bs,
+                                                  &child_of_bds,
+                                                  BDRV_CHILD_DATA, false, errp);
                 if (!s->data_file) {
                     ret = -EINVAL;
                     goto fail;
@@ -3454,7 +3455,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
     assert(create_options->driver == BLOCKDEV_DRIVER_QCOW2);
     qcow2_opts = &create_options->u.qcow2;
 
-    bs = bdrv_open_blockdev_ref(qcow2_opts->file, errp);
+    bs = bdrv_co_open_blockdev_ref(qcow2_opts->file, errp);
     if (bs == NULL) {
         return -EIO;
     }
@@ -3596,7 +3597,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
             ret = -EINVAL;
             goto out;
         }
-        data_bs = bdrv_open_blockdev_ref(qcow2_opts->data_file, errp);
+        data_bs = bdrv_co_open_blockdev_ref(qcow2_opts->data_file, errp);
         if (data_bs == NULL) {
             ret = -EIO;
             goto out;
@@ -3629,8 +3630,8 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
     }
 
     /* Create BlockBackend to write to the image */
-    blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
-                          errp);
+    blk = blk_co_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
+                             errp);
     if (!blk) {
         ret = -EPERM;
         goto out;
@@ -3712,9 +3713,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
     if (data_bs) {
         qdict_put_str(options, "data-file", data_bs->node_name);
     }
-    blk = blk_new_open(NULL, NULL, options,
-                       BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_NO_FLUSH,
-                       errp);
+    blk = blk_co_new_open(NULL, NULL, options,
+                          BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_NO_FLUSH,
+                          errp);
     if (blk == NULL) {
         ret = -EIO;
         goto out;
@@ -3793,9 +3794,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
     if (data_bs) {
         qdict_put_str(options, "data-file", data_bs->node_name);
     }
-    blk = blk_new_open(NULL, NULL, options,
-                       BDRV_O_RDWR | BDRV_O_NO_BACKING | BDRV_O_NO_IO,
-                       errp);
+    blk = blk_co_new_open(NULL, NULL, options,
+                          BDRV_O_RDWR | BDRV_O_NO_BACKING | BDRV_O_NO_IO,
+                          errp);
     if (blk == NULL) {
         ret = -EIO;
         goto out;
@@ -3877,8 +3878,8 @@ static int coroutine_fn qcow2_co_create_opts(BlockDriver *drv,
         goto finish;
     }
 
-    bs = bdrv_open(filename, NULL, NULL,
-                   BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+    bs = bdrv_co_open(filename, NULL, NULL,
+                      BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
     if (bs == NULL) {
         ret = -EIO;
         goto finish;
@@ -3892,9 +3893,9 @@ static int coroutine_fn qcow2_co_create_opts(BlockDriver *drv,
             goto finish;
         }
 
-        data_bs = bdrv_open(val, NULL, NULL,
-                            BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
-                            errp);
+        data_bs = bdrv_co_open(val, NULL, NULL,
+                               BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
+                               errp);
         if (data_bs == NULL) {
             ret = -EIO;
             goto finish;
-- 
2.38.1

[PATCH 07/13] qed: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper

[Kevin Wolf]

.bdrv_co_create implementations run in a coroutine. Therefore they are
not allowed to open images directly. Fix the calls to use the
corresponding no_co_wrappers instead.

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

diff --git a/block/qed.c b/block/qed.c
index 4473465bba..175a46c67b 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -676,13 +676,13 @@ static int coroutine_fn bdrv_qed_co_create(BlockdevCreateOptions *opts,
     }
 
     /* Create BlockBackend to write to the image */
-    bs = bdrv_open_blockdev_ref(qed_opts->file, errp);
+    bs = bdrv_co_open_blockdev_ref(qed_opts->file, errp);
     if (bs == NULL) {
         return -EIO;
     }
 
-    blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
-                          errp);
+    blk = blk_co_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
+                             errp);
     if (!blk) {
         ret = -EPERM;
         goto out;
@@ -783,8 +783,8 @@ static int coroutine_fn bdrv_qed_co_create_opts(BlockDriver *drv,
         goto fail;
     }
 
-    bs = bdrv_open(filename, NULL, NULL,
-                   BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+    bs = bdrv_co_open(filename, NULL, NULL,
+                      BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
     if (bs == NULL) {
         ret = -EIO;
         goto fail;
-- 
2.38.1

[PATCH 08/13] vdi: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper

[Kevin Wolf]

.bdrv_co_create implementations run in a coroutine. Therefore they are
not allowed to open images directly. Fix the calls to use the
corresponding no_co_wrappers instead.

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

diff --git a/block/vdi.c b/block/vdi.c
index 9c8736b26f..27db67d493 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -800,14 +800,14 @@ static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options,
     }
 
     /* Create BlockBackend to write to the image */
-    bs_file = bdrv_open_blockdev_ref(vdi_opts->file, errp);
+    bs_file = bdrv_co_open_blockdev_ref(vdi_opts->file, errp);
     if (!bs_file) {
         ret = -EIO;
         goto exit;
     }
 
-    blk = blk_new_with_bs(bs_file, BLK_PERM_WRITE | BLK_PERM_RESIZE,
-                          BLK_PERM_ALL, errp);
+    blk = blk_co_new_with_bs(bs_file, BLK_PERM_WRITE | BLK_PERM_RESIZE,
+                             BLK_PERM_ALL, errp);
     if (!blk) {
         ret = -EPERM;
         goto exit;
@@ -940,8 +940,8 @@ static int coroutine_fn vdi_co_create_opts(BlockDriver *drv,
         goto done;
     }
 
-    bs_file = bdrv_open(filename, NULL, NULL,
-                        BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+    bs_file = bdrv_co_open(filename, NULL, NULL,
+                           BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
     if (!bs_file) {
         ret = -EIO;
         goto done;
-- 
2.38.1

[PATCH 09/13] vhdx: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper

[Kevin Wolf]

.bdrv_co_create implementations run in a coroutine. Therefore they are
not allowed to open images directly. Fix the calls to use the
corresponding no_co_wrappers instead.

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

diff --git a/block/vhdx.c b/block/vhdx.c
index ef1f65d917..59fbdb413b 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1991,13 +1991,13 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
     }
 
     /* Create BlockBackend to write to the image */
-    bs = bdrv_open_blockdev_ref(vhdx_opts->file, errp);
+    bs = bdrv_co_open_blockdev_ref(vhdx_opts->file, errp);
     if (bs == NULL) {
         return -EIO;
     }
 
-    blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
-                          errp);
+    blk = blk_co_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
+                             errp);
     if (!blk) {
         ret = -EPERM;
         goto delete_and_exit;
@@ -2090,8 +2090,8 @@ static int coroutine_fn vhdx_co_create_opts(BlockDriver *drv,
         goto fail;
     }
 
-    bs = bdrv_open(filename, NULL, NULL,
-                   BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+    bs = bdrv_co_open(filename, NULL, NULL,
+                      BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
     if (bs == NULL) {
         ret = -EIO;
         goto fail;
-- 
2.38.1

[PATCH 10/13] vmdk: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper

[Kevin Wolf]

.bdrv_co_create implementations run in a coroutine. Therefore they are
not allowed to open images directly. Fix the calls to use the
corresponding no_co_wrappers instead.

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

diff --git a/block/vmdk.c b/block/vmdk.c
index 5b0eae877e..171c9272ca 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2299,9 +2299,9 @@ static int coroutine_fn vmdk_create_extent(const char *filename,
         goto exit;
     }
 
-    blk = blk_new_open(filename, NULL, NULL,
-                       BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
-                       errp);
+    blk = blk_co_new_open(filename, NULL, NULL,
+                          BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
+                          errp);
     if (blk == NULL) {
         ret = -EIO;
         goto exit;
@@ -2518,8 +2518,8 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
         }
         assert(full_backing);
 
-        backing = blk_new_open(full_backing, NULL, NULL,
-                               BDRV_O_NO_BACKING, errp);
+        backing = blk_co_new_open(full_backing, NULL, NULL,
+                                  BDRV_O_NO_BACKING, errp);
         g_free(full_backing);
         if (backing == NULL) {
             ret = -EIO;
@@ -2781,7 +2781,7 @@ static BlockBackend * coroutine_fn vmdk_co_create_cb(int64_t size, int idx,
     BlockdevCreateOptionsVmdk *opts = opaque;
 
     if (idx == 0) {
-        bs = bdrv_open_blockdev_ref(opts->file, errp);
+        bs = bdrv_co_open_blockdev_ref(opts->file, errp);
     } else {
         int i;
         BlockdevRefList *list = opts->extents;
@@ -2796,14 +2796,16 @@ static BlockBackend * coroutine_fn vmdk_co_create_cb(int64_t size, int idx,
             error_setg(errp, "Extent [%d] not specified", idx - 1);
             return NULL;
         }
-        bs = bdrv_open_blockdev_ref(list->value, errp);
+        bs = bdrv_co_open_blockdev_ref(list->value, errp);
     }
     if (!bs) {
         return NULL;
     }
-    blk = blk_new_with_bs(bs,
-                          BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE | BLK_PERM_RESIZE,
-                          BLK_PERM_ALL, errp);
+    blk = blk_co_new_with_bs(bs,
+                             BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE |
+                                BLK_PERM_RESIZE,
+                             BLK_PERM_ALL,
+                             errp);
     if (!blk) {
         return NULL;
     }
-- 
2.38.1

[PATCH 11/13] vpc: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper

[Kevin Wolf]

.bdrv_co_create implementations run in a coroutine. Therefore they are
not allowed to open images directly. Fix the calls to use the
corresponding no_co_wrappers instead.

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

diff --git a/block/vpc.c b/block/vpc.c
index cfdea7db80..3c256fc5a4 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1005,13 +1005,13 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
     }
 
     /* Create BlockBackend to write to the image */
-    bs = bdrv_open_blockdev_ref(vpc_opts->file, errp);
+    bs = bdrv_co_open_blockdev_ref(vpc_opts->file, errp);
     if (bs == NULL) {
         return -EIO;
     }
 
-    blk = blk_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
-                          errp);
+    blk = blk_co_new_with_bs(bs, BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL,
+                             errp);
     if (!blk) {
         ret = -EPERM;
         goto out;
@@ -1117,8 +1117,8 @@ static int coroutine_fn vpc_co_create_opts(BlockDriver *drv,
         goto fail;
     }
 
-    bs = bdrv_open(filename, NULL, NULL,
-                   BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+    bs = bdrv_co_open(filename, NULL, NULL,
+                      BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
     if (bs == NULL) {
         ret = -EIO;
         goto fail;
-- 
2.38.1

[PATCH 12/13] block: Fix bdrv_co_create_opts_simple() to open images with no_co_wrapper

[Kevin Wolf]

bdrv_co_create_opts_simpl() runs in a coroutine. Therefore it is not
allowed to open images directly. Fix the call to use the corresponding
no_co_wrapper instead.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index aa9062f2c1..6eac16eac5 100644
--- a/block.c
+++ b/block.c
@@ -657,8 +657,8 @@ int coroutine_fn bdrv_co_create_opts_simple(BlockDriver *drv,
     options = qdict_new();
     qdict_put_str(options, "driver", drv->format_name);
 
-    blk = blk_new_open(filename, NULL, options,
-                       BDRV_O_RDWR | BDRV_O_RESIZE, errp);
+    blk = blk_co_new_open(filename, NULL, options,
+                          BDRV_O_RDWR | BDRV_O_RESIZE, errp);
     if (!blk) {
         error_prepend(errp, "Protocol driver '%s' does not support image "
                       "creation, and opening the image failed: ",
-- 
2.38.1

[PATCH 13/13] block: Assert non-coroutine context for bdrv_open_inherit()

[Kevin Wolf]

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

diff --git a/block.c b/block.c
index 6eac16eac5..122aa9a9ac 100644
--- a/block.c
+++ b/block.c
@@ -3807,13 +3807,11 @@ out:
  * function eventually calls bdrv_refresh_total_sectors() which polls
  * when called from non-coroutine context.
  */
-static BlockDriverState *bdrv_open_inherit(const char *filename,
-                                           const char *reference,
-                                           QDict *options, int flags,
-                                           BlockDriverState *parent,
-                                           const BdrvChildClass *child_class,
-                                           BdrvChildRole child_role,
-                                           Error **errp)
+static BlockDriverState * no_coroutine_fn
+bdrv_open_inherit(const char *filename, const char *reference, QDict *options,
+                  int flags, BlockDriverState *parent,
+                  const BdrvChildClass *child_class, BdrvChildRole child_role,
+                  Error **errp)
 {
     int ret;
     BlockBackend *file = NULL;
@@ -3829,6 +3827,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
     assert(!child_class || !flags);
     assert(!child_class == !parent);
     GLOBAL_STATE_CODE();
+    assert(!qemu_in_coroutine());
 
     if (reference) {
         bool options_non_empty = options ? qdict_size(options) : false;
-- 
2.38.1

Re: [PATCH 00/13] block: Fix bdrv_open*() calls from coroutine context

[Emanuele Giuseppe Esposito]

Am 26/01/2023 um 18:24 schrieb Kevin Wolf:
> bdrv_open*() must not be called from coroutine context, amongst others
> because it modifies the block graph. However, some functions - in
> particular all .bdrv_co_create* implementations of image formats - do
> call it from coroutine context. This is already wrong today, but when we
> add locking, it actually becomes visible.
> 
> This series adds no_co_wrapper functions, which are automatically
> generated wrappers that run in coroutine context and use a BH to call
> the wrapped function outside of coroutine context. It then uses these
> wrappers to fix the problematic bdrv_open*() calls.
> 
> Kevin Wolf (13):
>   block-coroutine-wrapper: Introduce no_co_wrapper
>   block: Create no_co_wrappers for open functions
>   luks: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper
>   parallels: Fix .bdrv_co_create(_opts) to open images with
>     no_co_wrapper
>   qcow: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper
>   qcow2: Fix open/create to open images with no_co_wrapper
>   qed: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper
>   vdi: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper
>   vhdx: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper
>   vmdk: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper
>   vpc: Fix .bdrv_co_create(_opts) to open images with no_co_wrapper
>   block: Fix bdrv_co_create_opts_simple() to open images with
>     no_co_wrapper
>   block: Assert non-coroutine context for bdrv_open_inherit()
> 

Apart from a small nitpick in patch 3 where the functions are not marked
as coroutine_fn (but I think this is because BDS callbacks usually don't
have such annotations), looks good to me.

Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

>  include/block/block-common.h                | 14 ++++
>  include/block/block-global-state.h          | 35 ++++++---
>  include/sysemu/block-backend-global-state.h | 21 +++++-
>  block.c                                     | 17 ++---
>  block/crypto.c                              | 10 +--
>  block/parallels.c                           | 10 +--
>  block/qcow.c                                | 10 +--
>  block/qcow2.c                               | 43 +++++------
>  block/qed.c                                 | 10 +--
>  block/vdi.c                                 | 10 +--
>  block/vhdx.c                                | 10 +--
>  block/vmdk.c                                | 22 +++---
>  block/vpc.c                                 | 10 +--
>  scripts/block-coroutine-wrapper.py          | 83 ++++++++++++++++++---
>  block/meson.build                           |  1 +
>  15 files changed, 207 insertions(+), 99 deletions(-)
> 

Re: [PATCH 12/13] block: Fix bdrv_co_create_opts_simple() to open images with no_co_wrapper

[Eric Blake]

On Thu, Jan 26, 2023 at 06:24:31PM +0100, Kevin Wolf wrote:
> bdrv_co_create_opts_simpl() runs in a coroutine. Therefore it is not

simple

> allowed to open images directly. Fix the call to use the corresponding
> no_co_wrapper instead.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index aa9062f2c1..6eac16eac5 100644
> --- a/block.c
> +++ b/block.c
> @@ -657,8 +657,8 @@ int coroutine_fn bdrv_co_create_opts_simple(BlockDriver *drv,
>      options = qdict_new();
>      qdict_put_str(options, "driver", drv->format_name);
>  
> -    blk = blk_new_open(filename, NULL, options,
> -                       BDRV_O_RDWR | BDRV_O_RESIZE, errp);
> +    blk = blk_co_new_open(filename, NULL, options,
> +                          BDRV_O_RDWR | BDRV_O_RESIZE, errp);
>      if (!blk) {
>          error_prepend(errp, "Protocol driver '%s' does not support image "
>                        "creation, and opening the image failed: ",
> -- 
> 2.38.1
> 
> 

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

Re: [PATCH 00/13] block: Fix bdrv_open*() calls from coroutine context

[Hanna Czenczek]

On 26.01.23 18:24, Kevin Wolf wrote:
> bdrv_open*() must not be called from coroutine context, amongst others
> because it modifies the block graph. However, some functions - in
> particular all .bdrv_co_create* implementations of image formats - do
> call it from coroutine context. This is already wrong today, but when we
> add locking, it actually becomes visible.
>
> This series adds no_co_wrapper functions, which are automatically
> generated wrappers that run in coroutine context and use a BH to call
> the wrapped function outside of coroutine context. It then uses these
> wrappers to fix the problematic bdrv_open*() calls.

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Re: [PATCH 00/13] block: Fix bdrv_open*() calls from coroutine context

[Kevin Wolf]

Am 26.01.2023 um 18:24 hat Kevin Wolf geschrieben:
> bdrv_open*() must not be called from coroutine context, amongst others
> because it modifies the block graph. However, some functions - in
> particular all .bdrv_co_create* implementations of image formats - do
> call it from coroutine context. This is already wrong today, but when we
> add locking, it actually becomes visible.
> 
> This series adds no_co_wrapper functions, which are automatically
> generated wrappers that run in coroutine context and use a BH to call
> the wrapped function outside of coroutine context. It then uses these
> wrappers to fix the problematic bdrv_open*() calls.

Thanks for the review, fixed up the missing coroutine_fn in patch 3 (as
pointed out by Emanuele) and applied to the block branch.

Kevin