* [PATCH v4 00/11] Still more coroutine and various fixes in block layer
@ 2022-11-16 12:22 Emanuele Giuseppe Esposito
2022-11-16 12:22 ` [PATCH v4 01/11] block-copy: add missing coroutine_fn annotations Emanuele Giuseppe Esposito
` (10 more replies)
0 siblings, 11 replies; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-16 12:22 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel, 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.
This serie is based on Kevin Wolf's series "block: Simplify drain".
Based-on: <20221108123738.530873-1-kwolf@redhat.com>
Emanuele
---
v4:
* use v2 commit messages
* introduce generated_co_wrapper_simple to simplify patches
v3:
* Remove patch 1, base on kevin "drain semplification serie"
v2:
* clarified commit message in patches 2/3/6 on why we add coroutine_fn
Emanuele Giuseppe Esposito (11):
block-copy: add missing coroutine_fn annotations
nbd/server.c: add missing coroutine_fn annotations
block-backend: replace bdrv_*_above with blk_*_above
block-coroutine-wrapper.py: introduce generated_co_wrapper_simple
block-coroutine-wrapper.py: default to main loop aiocontext if
function does not have a BlockDriverState parameter
block-coroutine-wrapper.py: support also basic return types
block/vmdk: add missing coroutine_fn annotations
block: distinguish between bdrv_create running in coroutine and not
block: bdrv_create_file is a coroutine_fn
block: convert bdrv_create to generated_co_wrapper_simple
block/dirty-bitmap: convert coroutine-only functions to
generated_co_wrapper_simple
block.c | 68 +++++--------------
block/block-backend.c | 21 ++++++
block/block-copy.c | 15 +++--
block/block-gen.h | 5 +-
block/commit.c | 4 +-
block/dirty-bitmap.c | 88 +------------------------
block/meson.build | 1 +
block/vmdk.c | 36 +++++-----
include/block/block-common.h | 6 +-
include/block/block-global-state.h | 13 +++-
include/block/block-io.h | 9 ++-
include/block/dirty-bitmap.h | 10 ++-
include/sysemu/block-backend-io.h | 9 +++
nbd/server.c | 43 ++++++------
qemu-img.c | 4 +-
scripts/block-coroutine-wrapper.py | 102 +++++++++++++++++++++--------
16 files changed, 209 insertions(+), 225 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v4 01/11] block-copy: add missing coroutine_fn annotations
2022-11-16 12:22 [PATCH v4 00/11] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
@ 2022-11-16 12:22 ` Emanuele Giuseppe Esposito
2022-11-18 19:05 ` Kevin Wolf
2022-11-16 12:22 ` [PATCH v4 02/11] nbd/server.c: " Emanuele Giuseppe Esposito
` (9 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-16 12:22 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel, Emanuele Giuseppe Esposito
These functions end up calling bdrv_common_block_status_above(), a
generated_co_wrapper function.
In addition, they also happen to be always called in coroutine context,
meaning all callers are coroutine_fn.
This means that the g_c_w function will enter the qemu_in_coroutine()
case and eventually suspend (or in other words call qemu_coroutine_yield()).
Therefore we need to mark such functions coroutine_fn too.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
block/block-copy.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index bb947afdda..f33ab1d0b6 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -577,8 +577,9 @@ static coroutine_fn int block_copy_task_entry(AioTask *task)
return ret;
}
-static int block_copy_block_status(BlockCopyState *s, int64_t offset,
- int64_t bytes, int64_t *pnum)
+static coroutine_fn int block_copy_block_status(BlockCopyState *s,
+ int64_t offset,
+ int64_t bytes, int64_t *pnum)
{
int64_t num;
BlockDriverState *base;
@@ -613,8 +614,9 @@ static int block_copy_block_status(BlockCopyState *s, int64_t offset,
* Check if the cluster starting at offset is allocated or not.
* return via pnum the number of contiguous clusters sharing this allocation.
*/
-static int block_copy_is_cluster_allocated(BlockCopyState *s, int64_t offset,
- int64_t *pnum)
+static int coroutine_fn block_copy_is_cluster_allocated(BlockCopyState *s,
+ int64_t offset,
+ int64_t *pnum)
{
BlockDriverState *bs = s->source->bs;
int64_t count, total_count = 0;
@@ -669,8 +671,9 @@ void block_copy_reset(BlockCopyState *s, int64_t offset, int64_t bytes)
* @return 0 when the cluster at @offset was unallocated,
* 1 otherwise, and -ret on error.
*/
-int64_t block_copy_reset_unallocated(BlockCopyState *s,
- int64_t offset, int64_t *count)
+int64_t coroutine_fn block_copy_reset_unallocated(BlockCopyState *s,
+ int64_t offset,
+ int64_t *count)
{
int ret;
int64_t clusters, bytes;
--
2.31.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 02/11] nbd/server.c: add missing coroutine_fn annotations
2022-11-16 12:22 [PATCH v4 00/11] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
2022-11-16 12:22 ` [PATCH v4 01/11] block-copy: add missing coroutine_fn annotations Emanuele Giuseppe Esposito
@ 2022-11-16 12:22 ` Emanuele Giuseppe Esposito
2022-11-18 19:08 ` Kevin Wolf
2022-11-16 12:22 ` [PATCH v4 03/11] block-backend: replace bdrv_*_above with blk_*_above Emanuele Giuseppe Esposito
` (8 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-16 12:22 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel, Emanuele Giuseppe Esposito
These functions end up calling bdrv_*() implemented as generated_co_wrapper
functions.
In addition, they also happen to be always called in coroutine context,
meaning all callers are coroutine_fn.
This means that the g_c_w function will enter the qemu_in_coroutine()
case and eventually suspend (or in other words call qemu_coroutine_yield()).
Therefore we need to mark such functions coroutine_fn too.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
nbd/server.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index ada16089f3..e2eec0cf46 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2141,8 +2141,9 @@ static int nbd_extent_array_add(NBDExtentArray *ea,
return 0;
}
-static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
- uint64_t bytes, NBDExtentArray *ea)
+static int coroutine_fn blockstatus_to_extents(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes,
+ NBDExtentArray *ea)
{
while (bytes) {
uint32_t flags;
@@ -2168,8 +2169,9 @@ static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
return 0;
}
-static int blockalloc_to_extents(BlockDriverState *bs, uint64_t offset,
- uint64_t bytes, NBDExtentArray *ea)
+static int coroutine_fn blockalloc_to_extents(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes,
+ NBDExtentArray *ea)
{
while (bytes) {
int64_t num;
@@ -2220,11 +2222,12 @@ static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
}
/* Get block status from the exported device and send it to the client */
-static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
- BlockDriverState *bs, uint64_t offset,
- uint32_t length, bool dont_fragment,
- bool last, uint32_t context_id,
- Error **errp)
+static int
+coroutine_fn nbd_co_send_block_status(NBDClient *client, uint64_t handle,
+ BlockDriverState *bs, uint64_t offset,
+ uint32_t length, bool dont_fragment,
+ bool last, uint32_t context_id,
+ Error **errp)
{
int ret;
unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
--
2.31.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 03/11] block-backend: replace bdrv_*_above with blk_*_above
2022-11-16 12:22 [PATCH v4 00/11] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
2022-11-16 12:22 ` [PATCH v4 01/11] block-copy: add missing coroutine_fn annotations Emanuele Giuseppe Esposito
2022-11-16 12:22 ` [PATCH v4 02/11] nbd/server.c: " Emanuele Giuseppe Esposito
@ 2022-11-16 12:22 ` Emanuele Giuseppe Esposito
2022-11-18 19:15 ` Kevin Wolf
2022-11-16 12:22 ` [PATCH v4 04/11] block-coroutine-wrapper.py: introduce generated_co_wrapper_simple Emanuele Giuseppe Esposito
` (7 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-16 12:22 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel, 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 742efa7955..333d50fb3f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1424,6 +1424,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 a3b64c88af..4282a34bc0 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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 04/11] block-coroutine-wrapper.py: introduce generated_co_wrapper_simple
2022-11-16 12:22 [PATCH v4 00/11] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (2 preceding siblings ...)
2022-11-16 12:22 ` [PATCH v4 03/11] block-backend: replace bdrv_*_above with blk_*_above Emanuele Giuseppe Esposito
@ 2022-11-16 12:22 ` Emanuele Giuseppe Esposito
2022-11-21 12:21 ` Kevin Wolf
2022-11-16 12:22 ` [PATCH v4 05/11] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter Emanuele Giuseppe Esposito
` (6 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-16 12:22 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel, Emanuele Giuseppe Esposito
This new annotation creates just a function wrapper that creates
a new coroutine. It assumes the caller is not a coroutine.
This is much better as g_c_w, because it is clear if the caller
is a coroutine or not, and provides the advantage of automating
the code creation. In the future all g_c_w functions will be
substituted on g_c_w_simple.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
include/block/block-common.h | 1 +
scripts/block-coroutine-wrapper.py | 87 ++++++++++++++++++++++--------
2 files changed, 66 insertions(+), 22 deletions(-)
diff --git a/include/block/block-common.h b/include/block/block-common.h
index 297704c1e9..8ae750c7cf 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -43,6 +43,7 @@
* Read more in docs/devel/block-coroutine-wrapper.rst
*/
#define generated_co_wrapper
+#define generated_co_wrapper_simple
/* block.c */
typedef struct BlockDriver BlockDriver;
diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
index 08be813407..f88ef53964 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -62,10 +62,15 @@ def __init__(self, param_decl: str) -> None:
class FuncDecl:
- def __init__(self, return_type: str, name: str, args: str) -> None:
+ def __init__(self, return_type: str, name: str, args: str,
+ variant: str) -> None:
self.return_type = return_type.strip()
self.name = name.strip()
self.args = [ParamDecl(arg.strip()) for arg in args.split(',')]
+ self.create_only_co = False
+
+ if variant == '_simple':
+ self.create_only_co = True
def gen_list(self, format: str) -> str:
return ', '.join(format.format_map(arg.__dict__) for arg in self.args)
@@ -75,7 +80,8 @@ def gen_block(self, format: str) -> str:
# Match wrappers declared with a generated_co_wrapper mark
-func_decl_re = re.compile(r'^int\s*generated_co_wrapper\s*'
+func_decl_re = re.compile(r'^int\s*generated_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)
@@ -84,7 +90,8 @@ def func_decl_iter(text: str) -> Iterator:
for m in func_decl_re.finditer(text):
yield FuncDecl(return_type='int',
name=m.group('wrapper_name'),
- args=m.group('args'))
+ args=m.group('args'),
+ variant=m.group('variant'))
def snake_to_camel(func_name: str) -> str:
@@ -97,6 +104,51 @@ def snake_to_camel(func_name: str) -> str:
return ''.join(words)
+# Checks if we are already in coroutine
+def create_g_c_w(func: FuncDecl) -> str:
+ name = func.co_name
+ struct_name = func.struct_name
+ return f"""\
+int {func.name}({ func.gen_list('{decl}') })
+{{
+ if (qemu_in_coroutine()) {{
+ return {name}({ func.gen_list('{name}') });
+ }} else {{
+ {struct_name} s = {{
+ .poll_state.bs = {func.bs},
+ .poll_state.in_progress = true,
+
+{ func.gen_block(' .{name} = {name},') }
+ }};
+
+ s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
+
+ return bdrv_poll_co(&s.poll_state);
+ }}
+}}"""
+
+
+# Assumes we are not in coroutine, and creates one
+def create_coroutine_only(func: FuncDecl) -> str:
+ name = func.co_name
+ struct_name = func.struct_name
+ return f"""\
+int {func.name}({ func.gen_list('{decl}') })
+{{
+ assert(!qemu_in_coroutine());
+ {struct_name} s = {{
+ .poll_state.bs = {func.bs},
+ .poll_state.in_progress = true,
+
+{ func.gen_block(' .{name} = {name},') }
+ }};
+
+ s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
+
+ return bdrv_poll_co(&s.poll_state);
+}}"""
+
+
def gen_wrapper(func: FuncDecl) -> str:
assert not '_co_' in func.name
assert func.return_type == 'int'
@@ -105,7 +157,8 @@ def gen_wrapper(func: FuncDecl) -> str:
subsystem, subname = func.name.split('_', 1)
- name = f'{subsystem}_co_{subname}'
+ func.co_name = f'{subsystem}_co_{subname}'
+ name = func.co_name
t = func.args[0].type
if t == 'BlockDriverState *':
@@ -114,7 +167,13 @@ def gen_wrapper(func: FuncDecl) -> str:
bs = 'child->bs'
else:
bs = 'blk_bs(blk)'
- struct_name = snake_to_camel(name)
+ func.bs = bs
+ func.struct_name = snake_to_camel(func.name)
+ struct_name = func.struct_name
+
+ creation_function = create_g_c_w
+ if func.create_only_co:
+ creation_function = create_coroutine_only
return f"""\
/*
@@ -136,23 +195,7 @@ def gen_wrapper(func: FuncDecl) -> str:
aio_wait_kick();
}}
-int {func.name}({ func.gen_list('{decl}') })
-{{
- if (qemu_in_coroutine()) {{
- return {name}({ func.gen_list('{name}') });
- }} else {{
- {struct_name} s = {{
- .poll_state.bs = {bs},
- .poll_state.in_progress = true,
-
-{ func.gen_block(' .{name} = {name},') }
- }};
-
- s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
-
- return bdrv_poll_co(&s.poll_state);
- }}
-}}"""
+{creation_function(func)}"""
def gen_wrappers(input_code: str) -> str:
--
2.31.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 05/11] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter
2022-11-16 12:22 [PATCH v4 00/11] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (3 preceding siblings ...)
2022-11-16 12:22 ` [PATCH v4 04/11] block-coroutine-wrapper.py: introduce generated_co_wrapper_simple Emanuele Giuseppe Esposito
@ 2022-11-16 12:22 ` Emanuele Giuseppe Esposito
2022-11-21 15:30 ` Kevin Wolf
2022-11-16 12:22 ` [PATCH v4 06/11] block-coroutine-wrapper.py: support also basic return types Emanuele Giuseppe Esposito
` (5 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-16 12:22 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel, Emanuele Giuseppe Esposito
Basically BdrvPollCo->bs is only used by bdrv_poll_co(), and the
functions that it uses are both using bdrv_get_aio_context, that
defaults to qemu_get_aio_context() if bs is NULL.
Therefore pass NULL to BdrvPollCo to automatically generate a function
that create and runs a coroutine in the main loop.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
scripts/block-coroutine-wrapper.py | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
index f88ef53964..0f842386d4 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -152,8 +152,6 @@ def create_coroutine_only(func: FuncDecl) -> str:
def gen_wrapper(func: FuncDecl) -> str:
assert not '_co_' in func.name
assert func.return_type == 'int'
- assert func.args[0].type in ['BlockDriverState *', 'BdrvChild *',
- 'BlockBackend *']
subsystem, subname = func.name.split('_', 1)
@@ -165,8 +163,10 @@ def gen_wrapper(func: FuncDecl) -> str:
bs = 'bs'
elif t == 'BdrvChild *':
bs = 'child->bs'
- else:
+ elif t == 'BlockBackend *':
bs = 'blk_bs(blk)'
+ else:
+ bs = 'NULL'
func.bs = bs
func.struct_name = snake_to_camel(func.name)
struct_name = func.struct_name
--
2.31.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 06/11] block-coroutine-wrapper.py: support also basic return types
2022-11-16 12:22 [PATCH v4 00/11] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (4 preceding siblings ...)
2022-11-16 12:22 ` [PATCH v4 05/11] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter Emanuele Giuseppe Esposito
@ 2022-11-16 12:22 ` Emanuele Giuseppe Esposito
2022-11-21 15:55 ` Kevin Wolf
2022-11-16 12:22 ` [PATCH v4 07/11] block/vmdk: add missing coroutine_fn annotations Emanuele Giuseppe Esposito
` (4 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-16 12:22 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel, Emanuele Giuseppe Esposito
Extend the regex to cover also return type, pointers included.
This implies that the value returned by the function cannot be
a simple "int" anymore, but the custom return type.
Therefore remove poll_state->ret and instead use a per-function
custom "ret" field.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
block/block-gen.h | 5 +----
scripts/block-coroutine-wrapper.py | 19 +++++++++++--------
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/block/block-gen.h b/block/block-gen.h
index f80cf4897d..8ac9d5bd4f 100644
--- a/block/block-gen.h
+++ b/block/block-gen.h
@@ -32,18 +32,15 @@
typedef struct BdrvPollCo {
BlockDriverState *bs;
bool in_progress;
- int ret;
Coroutine *co; /* Keep pointer here for debugging */
} BdrvPollCo;
-static inline int bdrv_poll_co(BdrvPollCo *s)
+static inline void bdrv_poll_co(BdrvPollCo *s)
{
assert(!qemu_in_coroutine());
bdrv_coroutine_enter(s->bs, s->co);
BDRV_POLL_WHILE(s->bs, s->in_progress);
-
- return s->ret;
}
#endif /* BLOCK_BLOCK_GEN_H */
diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
index 0f842386d4..21ecb3e896 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -80,7 +80,8 @@ def gen_block(self, format: str) -> str:
# Match wrappers declared with a generated_co_wrapper mark
-func_decl_re = re.compile(r'^int\s*generated_co_wrapper'
+func_decl_re = re.compile(r'^(?P<return_type>[a-zA-Z][a-zA-Z0-9_]* [*]?)'
+ r'\s*generated_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)
@@ -88,7 +89,7 @@ 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='int',
+ yield FuncDecl(return_type=m.group('return_type'),
name=m.group('wrapper_name'),
args=m.group('args'),
variant=m.group('variant'))
@@ -109,7 +110,7 @@ def create_g_c_w(func: FuncDecl) -> str:
name = func.co_name
struct_name = func.struct_name
return f"""\
-int {func.name}({ func.gen_list('{decl}') })
+{func.return_type} {func.name}({ func.gen_list('{decl}') })
{{
if (qemu_in_coroutine()) {{
return {name}({ func.gen_list('{name}') });
@@ -123,7 +124,8 @@ def create_g_c_w(func: FuncDecl) -> str:
s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
- return bdrv_poll_co(&s.poll_state);
+ bdrv_poll_co(&s.poll_state);
+ return s.ret;
}}
}}"""
@@ -133,7 +135,7 @@ def create_coroutine_only(func: FuncDecl) -> str:
name = func.co_name
struct_name = func.struct_name
return f"""\
-int {func.name}({ func.gen_list('{decl}') })
+{func.return_type} {func.name}({ func.gen_list('{decl}') })
{{
assert(!qemu_in_coroutine());
{struct_name} s = {{
@@ -145,13 +147,13 @@ def create_coroutine_only(func: FuncDecl) -> str:
s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
- return bdrv_poll_co(&s.poll_state);
+ bdrv_poll_co(&s.poll_state);
+ return s.ret;
}}"""
def gen_wrapper(func: FuncDecl) -> str:
assert not '_co_' in func.name
- assert func.return_type == 'int'
subsystem, subname = func.name.split('_', 1)
@@ -182,6 +184,7 @@ def gen_wrapper(func: FuncDecl) -> str:
typedef struct {struct_name} {{
BdrvPollCo poll_state;
+ {func.return_type} ret;
{ func.gen_block(' {decl};') }
}} {struct_name};
@@ -189,7 +192,7 @@ def gen_wrapper(func: FuncDecl) -> str:
{{
{struct_name} *s = opaque;
- s->poll_state.ret = {name}({ func.gen_list('s->{name}') });
+ s->ret = {name}({ func.gen_list('s->{name}') });
s->poll_state.in_progress = false;
aio_wait_kick();
--
2.31.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 07/11] block/vmdk: add missing coroutine_fn annotations
2022-11-16 12:22 [PATCH v4 00/11] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (5 preceding siblings ...)
2022-11-16 12:22 ` [PATCH v4 06/11] block-coroutine-wrapper.py: support also basic return types Emanuele Giuseppe Esposito
@ 2022-11-16 12:22 ` Emanuele Giuseppe Esposito
2022-11-21 16:01 ` Kevin Wolf
2022-11-16 12:22 ` [PATCH v4 08/11] block: distinguish between bdrv_create running in coroutine and not Emanuele Giuseppe Esposito
` (3 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-16 12:22 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel, Emanuele Giuseppe Esposito
These functions end up calling bdrv_create() implemented as generated_co_wrapper
functions.
In addition, they also happen to be always called in coroutine context,
meaning all callers are coroutine_fn.
This means that the g_c_w function will enter the qemu_in_coroutine()
case and eventually suspend (or in other words call qemu_coroutine_yield()).
Therefore we need to mark such functions coroutine_fn too.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
block/vmdk.c | 36 +++++++++++++++++++-----------------
1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/block/vmdk.c b/block/vmdk.c
index 26376352b9..0c32bf2e83 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2285,10 +2285,11 @@ exit:
return ret;
}
-static int vmdk_create_extent(const char *filename, int64_t filesize,
- bool flat, bool compress, bool zeroed_grain,
- BlockBackend **pbb,
- QemuOpts *opts, Error **errp)
+static int coroutine_fn vmdk_create_extent(const char *filename,
+ int64_t filesize, bool flat,
+ bool compress, bool zeroed_grain,
+ BlockBackend **pbb,
+ QemuOpts *opts, Error **errp)
{
int ret;
BlockBackend *blk = NULL;
@@ -2366,14 +2367,14 @@ static int filename_decompose(const char *filename, char *path, char *prefix,
* non-split format.
* idx >= 1: get the n-th extent if in a split subformat
*/
-typedef BlockBackend *(*vmdk_create_extent_fn)(int64_t size,
- int idx,
- bool flat,
- bool split,
- bool compress,
- bool zeroed_grain,
- void *opaque,
- Error **errp);
+typedef BlockBackend * coroutine_fn (*vmdk_create_extent_fn)(int64_t size,
+ int idx,
+ bool flat,
+ bool split,
+ bool compress,
+ bool zeroed_grain,
+ void *opaque,
+ Error **errp);
static void vmdk_desc_add_extent(GString *desc,
const char *extent_line_fmt,
@@ -2616,7 +2617,7 @@ typedef struct {
QemuOpts *opts;
} VMDKCreateOptsData;
-static BlockBackend *vmdk_co_create_opts_cb(int64_t size, int idx,
+static BlockBackend * coroutine_fn vmdk_co_create_opts_cb(int64_t size, int idx,
bool flat, bool split, bool compress,
bool zeroed_grain, void *opaque,
Error **errp)
@@ -2768,10 +2769,11 @@ exit:
return ret;
}
-static BlockBackend *vmdk_co_create_cb(int64_t size, int idx,
- bool flat, bool split, bool compress,
- bool zeroed_grain, void *opaque,
- Error **errp)
+static BlockBackend * coroutine_fn vmdk_co_create_cb(int64_t size, int idx,
+ bool flat, bool split,
+ bool compress,
+ bool zeroed_grain,
+ void *opaque, Error **errp)
{
int ret;
BlockDriverState *bs;
--
2.31.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 08/11] block: distinguish between bdrv_create running in coroutine and not
2022-11-16 12:22 [PATCH v4 00/11] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (6 preceding siblings ...)
2022-11-16 12:22 ` [PATCH v4 07/11] block/vmdk: add missing coroutine_fn annotations Emanuele Giuseppe Esposito
@ 2022-11-16 12:22 ` Emanuele Giuseppe Esposito
2022-11-22 8:45 ` Kevin Wolf
2022-11-16 12:22 ` [PATCH v4 09/11] block: bdrv_create_file is a coroutine_fn Emanuele Giuseppe Esposito
` (2 subsequent siblings)
10 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-16 12:22 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel, 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 | 76 ++++++++++++++++++++++++++++-----------------------------
1 file changed, 38 insertions(+), 38 deletions(-)
diff --git a/block.c b/block.c
index 577639c7e0..c610a32e77 100644
--- a/block.c
+++ b/block.c
@@ -528,66 +528,66 @@ 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;
+ char *filename_copy;
+ GLOBAL_STATE_CODE();
+ assert(*errp == NULL);
+ assert(drv);
+
+ if (!drv->bdrv_co_create_opts) {
+ error_setg(errp, "Driver '%s' does not support image creation",
+ drv->format_name);
+ return -ENOTSUP;
+ }
+ filename_copy = g_strdup(filename);
+ ret = drv->bdrv_co_create_opts(drv, filename_copy, opts, errp);
+
+ if (ret < 0 && !*errp) {
+ error_setg_errno(errp, -ret, "Could not create image");
+ }
+
+ g_free(filename_copy);
+ return ret;
+}
+
+static void coroutine_fn bdrv_create_co_entry(void *opaque)
+{
CreateCo *cco = opaque;
- assert(cco->drv);
GLOBAL_STATE_CODE();
- 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 = 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);
+ 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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 09/11] block: bdrv_create_file is a coroutine_fn
2022-11-16 12:22 [PATCH v4 00/11] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (7 preceding siblings ...)
2022-11-16 12:22 ` [PATCH v4 08/11] block: distinguish between bdrv_create running in coroutine and not Emanuele Giuseppe Esposito
@ 2022-11-16 12:22 ` Emanuele Giuseppe Esposito
2022-11-22 8:58 ` Kevin Wolf
2022-11-16 12:22 ` [PATCH v4 10/11] block: convert bdrv_create to generated_co_wrapper_simple Emanuele Giuseppe Esposito
2022-11-16 12:22 ` [PATCH v4 11/11] block/dirty-bitmap: convert coroutine-only functions " Emanuele Giuseppe Esposito
10 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-16 12:22 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel, 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 c610a32e77..7a4c3eb540 100644
--- a/block.c
+++ b/block.c
@@ -534,6 +534,7 @@ static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
int ret;
char *filename_copy;
GLOBAL_STATE_CODE();
+ assert(qemu_in_coroutine());
assert(*errp == NULL);
assert(drv);
@@ -725,7 +726,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;
@@ -766,7 +768,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 00e0cf8aea..6f35ed99e3 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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 10/11] block: convert bdrv_create to generated_co_wrapper_simple
2022-11-16 12:22 [PATCH v4 00/11] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (8 preceding siblings ...)
2022-11-16 12:22 ` [PATCH v4 09/11] block: bdrv_create_file is a coroutine_fn Emanuele Giuseppe Esposito
@ 2022-11-16 12:22 ` Emanuele Giuseppe Esposito
2022-11-22 9:16 ` Kevin Wolf
2022-11-16 12:22 ` [PATCH v4 11/11] block/dirty-bitmap: convert coroutine-only functions " Emanuele Giuseppe Esposito
10 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-16 12:22 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel, Emanuele Giuseppe Esposito
This function is never called in coroutine context, therefore
instead of manually creating a new coroutine, delegate it to the
block-coroutine-wrapper script, defining it as g_c_w_simple.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
block.c | 38 +-----------------------------
include/block/block-global-state.h | 10 ++++++--
2 files changed, 9 insertions(+), 39 deletions(-)
diff --git a/block.c b/block.c
index 7a4c3eb540..d3e168408a 100644
--- a/block.c
+++ b/block.c
@@ -528,7 +528,7 @@ typedef struct CreateCo {
Error *err;
} CreateCo;
-static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
+int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
QemuOpts *opts, Error **errp)
{
int ret;
@@ -555,42 +555,6 @@ static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
return ret;
}
-static void coroutine_fn bdrv_create_co_entry(void *opaque)
-{
- CreateCo *cco = opaque;
- GLOBAL_STATE_CODE();
-
- 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)
-{
- GLOBAL_STATE_CODE();
-
- 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 = 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);
- return cco.ret;
- }
-}
-
/**
* Helper function for bdrv_create_file_fallback(): Resize @blk to at
* least the given @minimum_size.
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 6f35ed99e3..305336bdb9 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -55,8 +55,14 @@ BlockDriver *bdrv_find_protocol(const char *filename,
bool allow_protocol_prefix,
Error **errp);
BlockDriver *bdrv_find_format(const char *format_name);
-int bdrv_create(BlockDriver *drv, const char* filename,
- QemuOpts *opts, Error **errp);
+
+int coroutine_fn bdrv_co_create(BlockDriver *drv, const char* filename,
+ QemuOpts *opts, Error **errp);
+int generated_co_wrapper_simple bdrv_create(BlockDriver *drv,
+ const char* filename,
+ QemuOpts *opts,
+ Error **errp);
+
int coroutine_fn bdrv_create_file(const char *filename, QemuOpts *opts,
Error **errp);
--
2.31.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v4 11/11] block/dirty-bitmap: convert coroutine-only functions to generated_co_wrapper_simple
2022-11-16 12:22 [PATCH v4 00/11] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (9 preceding siblings ...)
2022-11-16 12:22 ` [PATCH v4 10/11] block: convert bdrv_create to generated_co_wrapper_simple Emanuele Giuseppe Esposito
@ 2022-11-16 12:22 ` Emanuele Giuseppe Esposito
2022-11-22 10:04 ` Kevin Wolf
10 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-16 12:22 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel, 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, and function creation can be offloaded to
g_c_w_simple.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
block/dirty-bitmap.c | 88 +-----------------------------------
block/meson.build | 1 +
include/block/block-common.h | 5 +-
include/block/block-io.h | 9 +++-
include/block/dirty-bitmap.h | 10 +++-
5 files changed, 21 insertions(+), 92 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index bf3dc0512a..21cf592889 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -388,7 +388,7 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
* not fail.
* This function doesn't release corresponding BdrvDirtyBitmap.
*/
-static int coroutine_fn
+int coroutine_fn
bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
Error **errp)
{
@@ -399,45 +399,6 @@ bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
return 0;
}
-typedef struct BdrvRemovePersistentDirtyBitmapCo {
- BlockDriverState *bs;
- const char *name;
- Error **errp;
- int ret;
-} BdrvRemovePersistentDirtyBitmapCo;
-
-static void coroutine_fn
-bdrv_co_remove_persistent_dirty_bitmap_entry(void *opaque)
-{
- BdrvRemovePersistentDirtyBitmapCo *s = opaque;
-
- s->ret = bdrv_co_remove_persistent_dirty_bitmap(s->bs, s->name, s->errp);
- aio_wait_kick();
-}
-
-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;
- }
-}
-
bool
bdrv_supports_persistent_dirty_bitmap(BlockDriverState *bs)
{
@@ -447,7 +408,7 @@ bdrv_supports_persistent_dirty_bitmap(BlockDriverState *bs)
return false;
}
-static bool coroutine_fn
+bool coroutine_fn
bdrv_co_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
uint32_t granularity, Error **errp)
{
@@ -470,51 +431,6 @@ bdrv_co_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
return drv->bdrv_co_can_store_new_dirty_bitmap(bs, name, granularity, errp);
}
-typedef struct BdrvCanStoreNewDirtyBitmapCo {
- BlockDriverState *bs;
- const char *name;
- uint32_t granularity;
- Error **errp;
- bool ret;
-
- bool in_progress;
-} BdrvCanStoreNewDirtyBitmapCo;
-
-static void coroutine_fn bdrv_co_can_store_new_dirty_bitmap_entry(void *opaque)
-{
- BdrvCanStoreNewDirtyBitmapCo *s = opaque;
-
- s->ret = bdrv_co_can_store_new_dirty_bitmap(s->bs, s->name, s->granularity,
- s->errp);
- s->in_progress = false;
- aio_wait_kick();
-}
-
-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;
- }
-}
-
void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
{
bdrv_dirty_bitmaps_lock(bitmap->bs);
diff --git a/block/meson.build b/block/meson.build
index b7c68b83a3..c48a59571e 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -137,6 +137,7 @@ block_gen_c = custom_target('block-gen.c',
output: 'block-gen.c',
input: files(
'../include/block/block-io.h',
+ '../include/block/dirty-bitmap.h',
'../include/block/block-global-state.h',
'../include/sysemu/block-backend-io.h',
'coroutines.h'
diff --git a/include/block/block-common.h b/include/block/block-common.h
index 8ae750c7cf..683e3d1c51 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -29,8 +29,6 @@
#include "qemu/iov.h"
#include "qemu/coroutine.h"
#include "block/accounting.h"
-#include "block/dirty-bitmap.h"
-#include "block/blockjob.h"
#include "qemu/hbitmap.h"
#include "qemu/transactions.h"
@@ -45,6 +43,9 @@
#define generated_co_wrapper
#define generated_co_wrapper_simple
+#include "block/dirty-bitmap.h"
+#include "block/blockjob.h"
+
/* block.c */
typedef struct BlockDriver BlockDriver;
typedef struct BdrvChild BdrvChild;
diff --git a/include/block/block-io.h b/include/block/block-io.h
index 0b49cb33da..f45ef6206f 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -200,8 +200,13 @@ AioContext *child_of_bds_get_parent_aio_context(BdrvChild *c);
void bdrv_io_plug(BlockDriverState *bs);
void bdrv_io_unplug(BlockDriverState *bs);
-bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
- uint32_t granularity, Error **errp);
+bool coroutine_fn bdrv_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
+ const char *name,
+ uint32_t granularity,
+ Error **errp);
+bool generated_co_wrapper_simple
+bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
+ uint32_t granularity, Error **errp);
/**
*
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 6528336c4c..2290e7fa28 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -34,8 +34,14 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
Error **errp);
void bdrv_release_dirty_bitmap(BdrvDirtyBitmap *bitmap);
void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
-int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
- Error **errp);
+
+int coroutine_fn bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+ const char *name,
+ Error **errp);
+int generated_co_wrapper_simple
+bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+ Error **errp);
+
void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap);
--
2.31.1
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v4 01/11] block-copy: add missing coroutine_fn annotations
2022-11-16 12:22 ` [PATCH v4 01/11] block-copy: add missing coroutine_fn annotations Emanuele Giuseppe Esposito
@ 2022-11-18 19:05 ` Kevin Wolf
2022-11-21 8:32 ` Emanuele Giuseppe Esposito
0 siblings, 1 reply; 31+ messages in thread
From: Kevin Wolf @ 2022-11-18 19:05 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> These functions end up calling bdrv_common_block_status_above(), a
> generated_co_wrapper function.
> In addition, they also happen to be always called in coroutine context,
> meaning all callers are coroutine_fn.
> This means that the g_c_w function will enter the qemu_in_coroutine()
> case and eventually suspend (or in other words call qemu_coroutine_yield()).
> Therefore we need to mark such functions coroutine_fn too.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Ideally, we'd convert them to new wrappers bdrv_co_is_allocated() and
bdrv_co_block_status_above() instead of having to argue that they always
take the coroutine path in g_c_w.
> 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
> @@ -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;
This one is a public function. Its prototype in block-copy.h should be
updated, too.
Kevin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 02/11] nbd/server.c: add missing coroutine_fn annotations
2022-11-16 12:22 ` [PATCH v4 02/11] nbd/server.c: " Emanuele Giuseppe Esposito
@ 2022-11-18 19:08 ` Kevin Wolf
0 siblings, 0 replies; 31+ messages in thread
From: Kevin Wolf @ 2022-11-18 19:08 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> These functions end up calling bdrv_*() implemented as generated_co_wrapper
> functions.
> In addition, they also happen to be always called in coroutine context,
> meaning all callers are coroutine_fn.
> This means that the g_c_w function will enter the qemu_in_coroutine()
> case and eventually suspend (or in other words call qemu_coroutine_yield()).
> Therefore we need to mark such functions coroutine_fn too.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Here, too, it would be better to convert the callers to new blk_co_*()
wrappers instead of going through g_c_w even though we know that we are
in coroutine context.
Doesn't make the patch less correct, though.
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 03/11] block-backend: replace bdrv_*_above with blk_*_above
2022-11-16 12:22 ` [PATCH v4 03/11] block-backend: replace bdrv_*_above with blk_*_above Emanuele Giuseppe Esposito
@ 2022-11-18 19:15 ` Kevin Wolf
0 siblings, 0 replies; 31+ messages in thread
From: Kevin Wolf @ 2022-11-18 19:15 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> 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 742efa7955..333d50fb3f 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1424,6 +1424,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,
Let's call it blk_co_block_status_above() to stay consistent with other
functions.
> + 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);
coroutine_fn calling into a g_c_w. As mentioned in the first patch, we
really want a bdrv_co_block_status_above() than can be called without
the g_c_w wrapper.
> +}
> +
> +int coroutine_fn blk_is_allocated_above(BlockBackend *blk,
blk_co_is_allocated_above()
> + 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);
Again a g_c_w.
> +}
> +
> 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);
Indentation is off now.
>
> 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);
Here, too.
> 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 a3b64c88af..4282a34bc0 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);
This one looks odd. Did you intend to change more than the line wrapping
here?
Kevin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 01/11] block-copy: add missing coroutine_fn annotations
2022-11-18 19:05 ` Kevin Wolf
@ 2022-11-21 8:32 ` Emanuele Giuseppe Esposito
2022-11-21 8:51 ` Emanuele Giuseppe Esposito
0 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-21 8:32 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 18/11/2022 um 20:05 schrieb Kevin Wolf:
> Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
>> These functions end up calling bdrv_common_block_status_above(), a
>> generated_co_wrapper function.
>> In addition, they also happen to be always called in coroutine context,
>> meaning all callers are coroutine_fn.
>> This means that the g_c_w function will enter the qemu_in_coroutine()
>> case and eventually suspend (or in other words call qemu_coroutine_yield()).
>> Therefore we need to mark such functions coroutine_fn too.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>
> Ideally, we'd convert them to new wrappers bdrv_co_is_allocated() and
> bdrv_co_block_status_above() instead of having to argue that they always
> take the coroutine path in g_c_w.
>
Ok so basically I should introduce bdrv_co_is_allocated, because so far
in this and next series I never thought about creating it.
Since these functions will be eventually split anyways, I agree let's
start doing this now.
Thank you,
Emanuele
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 01/11] block-copy: add missing coroutine_fn annotations
2022-11-21 8:32 ` Emanuele Giuseppe Esposito
@ 2022-11-21 8:51 ` Emanuele Giuseppe Esposito
2022-11-21 11:50 ` Kevin Wolf
0 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-21 8:51 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 21/11/2022 um 09:32 schrieb Emanuele Giuseppe Esposito:
>
>
> Am 18/11/2022 um 20:05 schrieb Kevin Wolf:
>> Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
>>> These functions end up calling bdrv_common_block_status_above(), a
>>> generated_co_wrapper function.
>>> In addition, they also happen to be always called in coroutine context,
>>> meaning all callers are coroutine_fn.
>>> This means that the g_c_w function will enter the qemu_in_coroutine()
>>> case and eventually suspend (or in other words call qemu_coroutine_yield()).
>>> Therefore we need to mark such functions coroutine_fn too.
>>>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>
>> Ideally, we'd convert them to new wrappers bdrv_co_is_allocated() and
>> bdrv_co_block_status_above() instead of having to argue that they always
>> take the coroutine path in g_c_w.
>>
>
> Ok so basically I should introduce bdrv_co_is_allocated, because so far
> in this and next series I never thought about creating it.
> Since these functions will be eventually split anyways, I agree let's
> start doing this now.
Actually bdrv_is_allocated would be a g_c_w functions in itself, that
calls another g_c_w and it is probably called by functions that are or
will be g_c_w.
Is this actually the scope of this series? I think switching this
specific function and its callers or similar will require a lot of
efforts, and if I do it here it won't cover all the cases for sure.
Wouldn't it be better to do these kind of things in a different serie
using Paolo's vrc tool?
> Thank you,
> Emanuele
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 01/11] block-copy: add missing coroutine_fn annotations
2022-11-21 8:51 ` Emanuele Giuseppe Esposito
@ 2022-11-21 11:50 ` Kevin Wolf
2022-11-21 13:26 ` Emanuele Giuseppe Esposito
0 siblings, 1 reply; 31+ messages in thread
From: Kevin Wolf @ 2022-11-21 11:50 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 21.11.2022 um 09:51 hat Emanuele Giuseppe Esposito geschrieben:
>
>
> Am 21/11/2022 um 09:32 schrieb Emanuele Giuseppe Esposito:
> >
> >
> > Am 18/11/2022 um 20:05 schrieb Kevin Wolf:
> >> Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> >>> These functions end up calling bdrv_common_block_status_above(), a
> >>> generated_co_wrapper function.
> >>> In addition, they also happen to be always called in coroutine context,
> >>> meaning all callers are coroutine_fn.
> >>> This means that the g_c_w function will enter the qemu_in_coroutine()
> >>> case and eventually suspend (or in other words call qemu_coroutine_yield()).
> >>> Therefore we need to mark such functions coroutine_fn too.
> >>>
> >>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> >>
> >> Ideally, we'd convert them to new wrappers bdrv_co_is_allocated() and
> >> bdrv_co_block_status_above() instead of having to argue that they always
> >> take the coroutine path in g_c_w.
> >
> > Ok so basically I should introduce bdrv_co_is_allocated, because so far
> > in this and next series I never thought about creating it.
> > Since these functions will be eventually split anyways, I agree let's
> > start doing this now.
>
> Actually bdrv_is_allocated would be a g_c_w functions in itself, that
> calls another g_c_w and it is probably called by functions that are or
> will be g_c_w.
I'm not sure if I understand. bdrv_is_allocated() is essentially a g_c_w
function today, just indirectly. But we have callers that know that they
are running in a coroutine (which is why you're adding coroutine_fn to
them), so they shouldn't call a g_c_w function, but directly the
coroutine version of the function.
The only reason why you can't currently do that is that
bdrv_is_allocated() exists as a wrapper around the g_c_w function
bdrv_common_block_status_above(), but the same wrapper doesn't exist for
the pure coroutine version bdrv_co_common_block_status_above().
All I'm suggesting is introducing a bdrv_co_is_allocated() that is a
wrapper directly around bdrv_co_common_block_status_above(), so that
the functions you're marking as coroutine_fn can use it instead of
calling g_c_w. This should be about 10 lines of code.
I'm not implying that you should convert any other callers in this
patch, or that you should touch bdrv_is_allocated() at all.
> Is this actually the scope of this series? I think switching this
> specific function and its callers or similar will require a lot of
> efforts, and if I do it here it won't cover all the cases for sure.
>
> Wouldn't it be better to do these kind of things in a different serie
> using Paolo's vrc tool?
I'm not sure what the scope of this series is, because you already do
introduce new wrappers in other patches of the series. I assumed it's
just to improve the situation a little, with no claim of being
exhaustive.
Finding and fully converting all callers might indeed be a job for
something like vrc, but here I'm only looking at local consistency in
functions where you're adding coroutine_fn.
Kevin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 04/11] block-coroutine-wrapper.py: introduce generated_co_wrapper_simple
2022-11-16 12:22 ` [PATCH v4 04/11] block-coroutine-wrapper.py: introduce generated_co_wrapper_simple Emanuele Giuseppe Esposito
@ 2022-11-21 12:21 ` Kevin Wolf
0 siblings, 0 replies; 31+ messages in thread
From: Kevin Wolf @ 2022-11-21 12:21 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> This new annotation creates just a function wrapper that creates
> a new coroutine. It assumes the caller is not a coroutine.
>
> This is much better as g_c_w, because it is clear if the caller
> is a coroutine or not, and provides the advantage of automating
> the code creation. In the future all g_c_w functions will be
> substituted on g_c_w_simple.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
$ mypy --strict scripts/block-coroutine-wrapper.py
scripts/block-coroutine-wrapper.py:31: error: Function is missing a return type annotation
scripts/block-coroutine-wrapper.py:90: error: Missing type parameters for generic type "Iterator"
scripts/block-coroutine-wrapper.py:110: error: "FuncDecl" has no attribute "co_name"
scripts/block-coroutine-wrapper.py:111: error: "FuncDecl" has no attribute "struct_name"
scripts/block-coroutine-wrapper.py:112: error: "FuncDecl" has no attribute "bs"
scripts/block-coroutine-wrapper.py:135: error: "FuncDecl" has no attribute "co_name"
scripts/block-coroutine-wrapper.py:136: error: "FuncDecl" has no attribute "struct_name"
scripts/block-coroutine-wrapper.py:137: error: "FuncDecl" has no attribute "bs"
scripts/block-coroutine-wrapper.py:160: error: "FuncDecl" has no attribute "co_name"
scripts/block-coroutine-wrapper.py:161: error: "FuncDecl" has no attribute "co_name"
scripts/block-coroutine-wrapper.py:172: error: "FuncDecl" has no attribute "bs"
scripts/block-coroutine-wrapper.py:173: error: "FuncDecl" has no attribute "struct_name"
scripts/block-coroutine-wrapper.py:174: error: "FuncDecl" has no attribute "struct_name"
scripts/block-coroutine-wrapper.py:218: error: Call to untyped function "gen_header" in typed context
Found 14 errors in 1 file (checked 1 source file)
The first two and the last one isn't the fault of this patch, but the
others are. When you add new attributes, they should be declared in
FuncDecl.__init__(). And actually, this even seems like a better place
to initialise them already with the right value than gen_wrapper().
> include/block/block-common.h | 1 +
> scripts/block-coroutine-wrapper.py | 87 ++++++++++++++++++++++--------
> 2 files changed, 66 insertions(+), 22 deletions(-)
>
> diff --git a/include/block/block-common.h b/include/block/block-common.h
> index 297704c1e9..8ae750c7cf 100644
> --- a/include/block/block-common.h
> +++ b/include/block/block-common.h
> @@ -43,6 +43,7 @@
> * Read more in docs/devel/block-coroutine-wrapper.rst
> */
> #define generated_co_wrapper
> +#define generated_co_wrapper_simple
>
> /* block.c */
> typedef struct BlockDriver BlockDriver;
> diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
> index 08be813407..f88ef53964 100644
> --- a/scripts/block-coroutine-wrapper.py
> +++ b/scripts/block-coroutine-wrapper.py
> @@ -62,10 +62,15 @@ def __init__(self, param_decl: str) -> None:
>
>
> class FuncDecl:
> - def __init__(self, return_type: str, name: str, args: str) -> None:
> + def __init__(self, return_type: str, name: str, args: str,
> + variant: str) -> None:
> self.return_type = return_type.strip()
> self.name = name.strip()
> self.args = [ParamDecl(arg.strip()) for arg in args.split(',')]
> + self.create_only_co = False
> +
> + if variant == '_simple':
> + self.create_only_co = True
>
> def gen_list(self, format: str) -> str:
> return ', '.join(format.format_map(arg.__dict__) for arg in self.args)
> @@ -75,7 +80,8 @@ def gen_block(self, format: str) -> str:
>
>
> # Match wrappers declared with a generated_co_wrapper mark
> -func_decl_re = re.compile(r'^int\s*generated_co_wrapper\s*'
> +func_decl_re = re.compile(r'^int\s*generated_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)
>
> @@ -84,7 +90,8 @@ def func_decl_iter(text: str) -> Iterator:
> for m in func_decl_re.finditer(text):
> yield FuncDecl(return_type='int',
> name=m.group('wrapper_name'),
> - args=m.group('args'))
> + args=m.group('args'),
> + variant=m.group('variant'))
>
>
> def snake_to_camel(func_name: str) -> str:
> @@ -97,6 +104,51 @@ def snake_to_camel(func_name: str) -> str:
> return ''.join(words)
>
>
> +# Checks if we are already in coroutine
> +def create_g_c_w(func: FuncDecl) -> str:
> + name = func.co_name
> + struct_name = func.struct_name
> + return f"""\
> +int {func.name}({ func.gen_list('{decl}') })
> +{{
> + if (qemu_in_coroutine()) {{
> + return {name}({ func.gen_list('{name}') });
> + }} else {{
> + {struct_name} s = {{
> + .poll_state.bs = {func.bs},
> + .poll_state.in_progress = true,
> +
> +{ func.gen_block(' .{name} = {name},') }
> + }};
> +
> + s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
> +
> + return bdrv_poll_co(&s.poll_state);
> + }}
> +}}"""
> +
> +
> +# Assumes we are not in coroutine, and creates one
> +def create_coroutine_only(func: FuncDecl) -> str:
> + name = func.co_name
> + struct_name = func.struct_name
> + return f"""\
> +int {func.name}({ func.gen_list('{decl}') })
> +{{
> + assert(!qemu_in_coroutine());
> + {struct_name} s = {{
> + .poll_state.bs = {func.bs},
> + .poll_state.in_progress = true,
> +
> +{ func.gen_block(' .{name} = {name},') }
> + }};
Not sure how strict we are about this in generated code, but generally
the QEMU coding style requires declarations to come first, so the
assertion should be below it.
> +
> + s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
> +
> + return bdrv_poll_co(&s.poll_state);
> +}}"""
Kevin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 01/11] block-copy: add missing coroutine_fn annotations
2022-11-21 11:50 ` Kevin Wolf
@ 2022-11-21 13:26 ` Emanuele Giuseppe Esposito
0 siblings, 0 replies; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-21 13:26 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 21/11/2022 um 12:50 schrieb Kevin Wolf:
> Am 21.11.2022 um 09:51 hat Emanuele Giuseppe Esposito geschrieben:
>>
>>
>> Am 21/11/2022 um 09:32 schrieb Emanuele Giuseppe Esposito:
>>>
>>>
>>> Am 18/11/2022 um 20:05 schrieb Kevin Wolf:
>>>> Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
>>>>> These functions end up calling bdrv_common_block_status_above(), a
>>>>> generated_co_wrapper function.
>>>>> In addition, they also happen to be always called in coroutine context,
>>>>> meaning all callers are coroutine_fn.
>>>>> This means that the g_c_w function will enter the qemu_in_coroutine()
>>>>> case and eventually suspend (or in other words call qemu_coroutine_yield()).
>>>>> Therefore we need to mark such functions coroutine_fn too.
>>>>>
>>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>>
>>>> Ideally, we'd convert them to new wrappers bdrv_co_is_allocated() and
>>>> bdrv_co_block_status_above() instead of having to argue that they always
>>>> take the coroutine path in g_c_w.
>>>
>>> Ok so basically I should introduce bdrv_co_is_allocated, because so far
>>> in this and next series I never thought about creating it.
>>> Since these functions will be eventually split anyways, I agree let's
>>> start doing this now.
>>
>> Actually bdrv_is_allocated would be a g_c_w functions in itself, that
>> calls another g_c_w and it is probably called by functions that are or
>> will be g_c_w.
>
> I'm not sure if I understand. bdrv_is_allocated() is essentially a g_c_w
> function today, just indirectly. But we have callers that know that they
> are running in a coroutine (which is why you're adding coroutine_fn to
> them), so they shouldn't call a g_c_w function, but directly the
> coroutine version of the function.
>
> The only reason why you can't currently do that is that
> bdrv_is_allocated() exists as a wrapper around the g_c_w function
> bdrv_common_block_status_above(), but the same wrapper doesn't exist for
> the pure coroutine version bdrv_co_common_block_status_above().
>
> All I'm suggesting is introducing a bdrv_co_is_allocated() that is a
> wrapper directly around bdrv_co_common_block_status_above(), so that
> the functions you're marking as coroutine_fn can use it instead of
> calling g_c_w. This should be about 10 lines of code.
>
> I'm not implying that you should convert any other callers in this
> patch, or that you should touch bdrv_is_allocated() at all.
>
>> Is this actually the scope of this series? I think switching this
>> specific function and its callers or similar will require a lot of
>> efforts, and if I do it here it won't cover all the cases for sure.
>>
>> Wouldn't it be better to do these kind of things in a different serie
>> using Paolo's vrc tool?
>
> I'm not sure what the scope of this series is, because you already do
> introduce new wrappers in other patches of the series. I assumed it's
> just to improve the situation a little, with no claim of being
> exhaustive.
>
> Finding and fully converting all callers might indeed be a job for
> something like vrc, but here I'm only looking at local consistency in
> functions where you're adding coroutine_fn.
>
Oh ok now I see what you mean. I was thinking (and did in "[PATCH 04/15]
block: convert bdrv_refresh_total_sectors in generated_co_wrapper") to
instead convert all callers in g_c_w, and that ended up being a big pain.
I'll also correct the patch I mentioned above.
Thank you,
Emanuele
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 05/11] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter
2022-11-16 12:22 ` [PATCH v4 05/11] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter Emanuele Giuseppe Esposito
@ 2022-11-21 15:30 ` Kevin Wolf
2022-11-21 15:52 ` Emanuele Giuseppe Esposito
0 siblings, 1 reply; 31+ messages in thread
From: Kevin Wolf @ 2022-11-21 15:30 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> Basically BdrvPollCo->bs is only used by bdrv_poll_co(), and the
> functions that it uses are both using bdrv_get_aio_context, that
> defaults to qemu_get_aio_context() if bs is NULL.
>
> Therefore pass NULL to BdrvPollCo to automatically generate a function
> that create and runs a coroutine in the main loop.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
It happens to work, but it's kind of ugly to call bdrv_coroutine_enter()
and BDRV_POLL_WHILE() with a NULL bs.
How hard would it be to generate code that doesn't use these functions,
but directly aio_co_enter() and AIO_WAIT_WHILE() for wrappers that are
not related to a BDS?
Kevin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 05/11] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter
2022-11-21 15:30 ` Kevin Wolf
@ 2022-11-21 15:52 ` Emanuele Giuseppe Esposito
2022-11-22 8:27 ` Kevin Wolf
0 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-21 15:52 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 21/11/2022 um 16:30 schrieb Kevin Wolf:
> Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
>> Basically BdrvPollCo->bs is only used by bdrv_poll_co(), and the
>> functions that it uses are both using bdrv_get_aio_context, that
>> defaults to qemu_get_aio_context() if bs is NULL.
>>
>> Therefore pass NULL to BdrvPollCo to automatically generate a function
>> that create and runs a coroutine in the main loop.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>
> It happens to work, but it's kind of ugly to call bdrv_coroutine_enter()
> and BDRV_POLL_WHILE() with a NULL bs.
>
> How hard would it be to generate code that doesn't use these functions,
> but directly aio_co_enter() and AIO_WAIT_WHILE() for wrappers that are
> not related to a BDS?
>
At this point, I would get rid of s->poll_state.bs and instead use
s->poll_state.aio_context. Then call directly aio_co_enter and
AIO_WAIT_WHILE, as you suggested but just everywhere, without
differentiating the cases.
Then we would have something similar to what it is currently done with bs:
if t == 'BlockDriverState *':
bs = 'bdrv_get_aio_context(bs)'
elif t == 'BdrvChild *':
bs = 'bdrv_get_aio_context(child->bs)'
elif t == 'BlockBackend *':
bs = 'bdrv_get_aio_context(blk_bs(blk))'
else:
bs = 'qemu_get_aio_context()'
I haven't tried it yet, but it should work.
Thank you,
Emanuele
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 06/11] block-coroutine-wrapper.py: support also basic return types
2022-11-16 12:22 ` [PATCH v4 06/11] block-coroutine-wrapper.py: support also basic return types Emanuele Giuseppe Esposito
@ 2022-11-21 15:55 ` Kevin Wolf
0 siblings, 0 replies; 31+ messages in thread
From: Kevin Wolf @ 2022-11-21 15:55 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> Extend the regex to cover also return type, pointers included.
> This implies that the value returned by the function cannot be
> a simple "int" anymore, but the custom return type.
> Therefore remove poll_state->ret and instead use a per-function
> custom "ret" field.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 07/11] block/vmdk: add missing coroutine_fn annotations
2022-11-16 12:22 ` [PATCH v4 07/11] block/vmdk: add missing coroutine_fn annotations Emanuele Giuseppe Esposito
@ 2022-11-21 16:01 ` Kevin Wolf
2022-11-21 16:07 ` Emanuele Giuseppe Esposito
0 siblings, 1 reply; 31+ messages in thread
From: Kevin Wolf @ 2022-11-21 16:01 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> These functions end up calling bdrv_create() implemented as generated_co_wrapper
> functions.
> In addition, they also happen to be always called in coroutine context,
> meaning all callers are coroutine_fn.
> This means that the g_c_w function will enter the qemu_in_coroutine()
> case and eventually suspend (or in other words call qemu_coroutine_yield()).
> Therefore we need to mark such functions coroutine_fn too.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Just one remark about patch ordering: This doesn't require the
g_c_w_simple patches, so wouldn't it make more sense to move the
g_c_w_simple right before the first patch that actually makes use of
them?
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 07/11] block/vmdk: add missing coroutine_fn annotations
2022-11-21 16:01 ` Kevin Wolf
@ 2022-11-21 16:07 ` Emanuele Giuseppe Esposito
0 siblings, 0 replies; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-21 16:07 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 21/11/2022 um 17:01 schrieb Kevin Wolf:
> Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
>> These functions end up calling bdrv_create() implemented as generated_co_wrapper
>> functions.
>> In addition, they also happen to be always called in coroutine context,
>> meaning all callers are coroutine_fn.
>> This means that the g_c_w function will enter the qemu_in_coroutine()
>> case and eventually suspend (or in other words call qemu_coroutine_yield()).
>> Therefore we need to mark such functions coroutine_fn too.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>
> Just one remark about patch ordering: This doesn't require the
> g_c_w_simple patches, so wouldn't it make more sense to move the
> g_c_w_simple right before the first patch that actually makes use of
> them?
I thought about it, but then I thought it was too far from bdrv_create
patches.
Both g_c_w_simple and this patch are needed by bdrv_create_file, so I
thought if you read this one first then you wouldn't have a clue on what
is the end goal.
Anyways, I'll change the order.
Thank you,
Emanuele
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 05/11] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter
2022-11-21 15:52 ` Emanuele Giuseppe Esposito
@ 2022-11-22 8:27 ` Kevin Wolf
0 siblings, 0 replies; 31+ messages in thread
From: Kevin Wolf @ 2022-11-22 8:27 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 21.11.2022 um 16:52 hat Emanuele Giuseppe Esposito geschrieben:
> Am 21/11/2022 um 16:30 schrieb Kevin Wolf:
> > Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> >> Basically BdrvPollCo->bs is only used by bdrv_poll_co(), and the
> >> functions that it uses are both using bdrv_get_aio_context, that
> >> defaults to qemu_get_aio_context() if bs is NULL.
> >>
> >> Therefore pass NULL to BdrvPollCo to automatically generate a function
> >> that create and runs a coroutine in the main loop.
> >>
> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> >
> > It happens to work, but it's kind of ugly to call bdrv_coroutine_enter()
> > and BDRV_POLL_WHILE() with a NULL bs.
> >
> > How hard would it be to generate code that doesn't use these functions,
> > but directly aio_co_enter() and AIO_WAIT_WHILE() for wrappers that are
> > not related to a BDS?
> >
>
> At this point, I would get rid of s->poll_state.bs and instead use
> s->poll_state.aio_context. Then call directly aio_co_enter and
> AIO_WAIT_WHILE, as you suggested but just everywhere, without
> differentiating the cases.
Oh, yes, that's better.
> Then we would have something similar to what it is currently done with bs:
>
> if t == 'BlockDriverState *':
> bs = 'bdrv_get_aio_context(bs)'
> elif t == 'BdrvChild *':
> bs = 'bdrv_get_aio_context(child->bs)'
> elif t == 'BlockBackend *':
> bs = 'bdrv_get_aio_context(blk_bs(blk))'
blk_get_aio_context(blk) seems more correct.
> else:
> bs = 'qemu_get_aio_context()'
>
> I haven't tried it yet, but it should work.
Kevin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 08/11] block: distinguish between bdrv_create running in coroutine and not
2022-11-16 12:22 ` [PATCH v4 08/11] block: distinguish between bdrv_create running in coroutine and not Emanuele Giuseppe Esposito
@ 2022-11-22 8:45 ` Kevin Wolf
0 siblings, 0 replies; 31+ messages in thread
From: Kevin Wolf @ 2022-11-22 8:45 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> 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 | 76 ++++++++++++++++++++++++++++-----------------------------
> 1 file changed, 38 insertions(+), 38 deletions(-)
>
> diff --git a/block.c b/block.c
> index 577639c7e0..c610a32e77 100644
> --- a/block.c
> +++ b/block.c
> @@ -528,66 +528,66 @@ 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;
> + char *filename_copy;
> + GLOBAL_STATE_CODE();
> + assert(*errp == NULL);
Your use of *errp in this function will crash if NULL is passed. You
need ERRP_GUARD() first before you can do this.
> + assert(drv);
> +
> + if (!drv->bdrv_co_create_opts) {
> + error_setg(errp, "Driver '%s' does not support image creation",
> + drv->format_name);
> + return -ENOTSUP;
> + }
>
> + filename_copy = g_strdup(filename);
It's only preserved from the pre-patch state, but I don't think this is
necessary? We know that the string will stay around until the function
returns, and the parameter of drv->bdrv_co_create_opts is const char*,
so it must not be modified either.
Maybe worth a cleanup patch on top to just use filename directly?
> + ret = drv->bdrv_co_create_opts(drv, filename_copy, opts, errp);
> +
> + if (ret < 0 && !*errp) {
> + error_setg_errno(errp, -ret, "Could not create image");
> + }
> +
> + g_free(filename_copy);
> + return ret;
> +}
Kevin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 09/11] block: bdrv_create_file is a coroutine_fn
2022-11-16 12:22 ` [PATCH v4 09/11] block: bdrv_create_file is a coroutine_fn Emanuele Giuseppe Esposito
@ 2022-11-22 8:58 ` Kevin Wolf
2022-11-22 9:04 ` Emanuele Giuseppe Esposito
0 siblings, 1 reply; 31+ messages in thread
From: Kevin Wolf @ 2022-11-22 8:58 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> 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 c610a32e77..7a4c3eb540 100644
> --- a/block.c
> +++ b/block.c
> @@ -534,6 +534,7 @@ static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
> int ret;
> char *filename_copy;
> GLOBAL_STATE_CODE();
> + assert(qemu_in_coroutine());
We don't generally assert this, otherwise it would have to be in every
coroutine_fn.
> assert(*errp == NULL);
> assert(drv);
>
> @@ -725,7 +726,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)
Should it be renamed as bdrv_co_create_file()?
> {
> QemuOpts *protocol_opts;
> BlockDriver *drv;
> @@ -766,7 +768,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);
Kevin
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 09/11] block: bdrv_create_file is a coroutine_fn
2022-11-22 8:58 ` Kevin Wolf
@ 2022-11-22 9:04 ` Emanuele Giuseppe Esposito
0 siblings, 0 replies; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-22 9:04 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 22/11/2022 um 09:58 schrieb Kevin Wolf:
> Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
>> 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 c610a32e77..7a4c3eb540 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -534,6 +534,7 @@ static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
>> int ret;
>> char *filename_copy;
>> GLOBAL_STATE_CODE();
>> + assert(qemu_in_coroutine());
>
> We don't generally assert this, otherwise it would have to be in every
> coroutine_fn.
That was my plan for the serie "Protect the block layer with a rwlock:
part 3", where I convert BlockDriver callbacks in coroutine, and thus
assert there because I know all the callers are coroutine_fn.
>
>> assert(*errp == NULL);
>> assert(drv);
>>
>> @@ -725,7 +726,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)
>
> Should it be renamed as bdrv_co_create_file()?
>
Ok (when I don't answer just assume that I agree).
Thank you,
Emanuele
>> {
>> QemuOpts *protocol_opts;
>> BlockDriver *drv;
>> @@ -766,7 +768,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);
>
> Kevin
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 10/11] block: convert bdrv_create to generated_co_wrapper_simple
2022-11-16 12:22 ` [PATCH v4 10/11] block: convert bdrv_create to generated_co_wrapper_simple Emanuele Giuseppe Esposito
@ 2022-11-22 9:16 ` Kevin Wolf
0 siblings, 0 replies; 31+ messages in thread
From: Kevin Wolf @ 2022-11-22 9:16 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> This function is never called in coroutine context, therefore
> instead of manually creating a new coroutine, delegate it to the
> block-coroutine-wrapper script, defining it as g_c_w_simple.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
At first I thought that "never called in coroutine context" was not
entirely true because of paths like this:
qcow2_co_create() -> bdrv_open_blockdev_ref() -> bdrv_open_inherit() ->
bdrv_append_temp_snapshot() -> bdrv_create().
The only reason why it doesn't happen is that with a BlockdevRef, it's
impossible to get BDRV_O_SNAPSHOT set, so bdrv_append_temp_snapshot()
can't actually be called when you come from bdrv_open_blockdev_ref().
Of course, opening images in a coroutine is likely to already do similar
things. We should probably drop out of coroutine context for bdrv_open
to be safe. In practice, I suspect it might work anyway because nothing
is going to wait on the current coroutine, but maybe better not to rely
on it.
Anyway, not a problem of your patch, it's just something it made me
think of.
> diff --git a/block.c b/block.c
> index 7a4c3eb540..d3e168408a 100644
> --- a/block.c
> +++ b/block.c
> @@ -528,7 +528,7 @@ typedef struct CreateCo {
> Error *err;
> } CreateCo;
>
> -static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
> +int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
> QemuOpts *opts, Error **errp)
The indentation is off now. With this fixed:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v4 11/11] block/dirty-bitmap: convert coroutine-only functions to generated_co_wrapper_simple
2022-11-16 12:22 ` [PATCH v4 11/11] block/dirty-bitmap: convert coroutine-only functions " Emanuele Giuseppe Esposito
@ 2022-11-22 10:04 ` Kevin Wolf
0 siblings, 0 replies; 31+ messages in thread
From: Kevin Wolf @ 2022-11-22 10:04 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito
Cc: qemu-block, Hanna Reitz, John Snow, Vladimir Sementsov-Ogievskiy,
Eric Blake, Fam Zheng, qemu-devel
Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> 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, and function creation can be offloaded to
> g_c_w_simple.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2022-11-22 10:05 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-16 12:22 [PATCH v4 00/11] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
2022-11-16 12:22 ` [PATCH v4 01/11] block-copy: add missing coroutine_fn annotations Emanuele Giuseppe Esposito
2022-11-18 19:05 ` Kevin Wolf
2022-11-21 8:32 ` Emanuele Giuseppe Esposito
2022-11-21 8:51 ` Emanuele Giuseppe Esposito
2022-11-21 11:50 ` Kevin Wolf
2022-11-21 13:26 ` Emanuele Giuseppe Esposito
2022-11-16 12:22 ` [PATCH v4 02/11] nbd/server.c: " Emanuele Giuseppe Esposito
2022-11-18 19:08 ` Kevin Wolf
2022-11-16 12:22 ` [PATCH v4 03/11] block-backend: replace bdrv_*_above with blk_*_above Emanuele Giuseppe Esposito
2022-11-18 19:15 ` Kevin Wolf
2022-11-16 12:22 ` [PATCH v4 04/11] block-coroutine-wrapper.py: introduce generated_co_wrapper_simple Emanuele Giuseppe Esposito
2022-11-21 12:21 ` Kevin Wolf
2022-11-16 12:22 ` [PATCH v4 05/11] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter Emanuele Giuseppe Esposito
2022-11-21 15:30 ` Kevin Wolf
2022-11-21 15:52 ` Emanuele Giuseppe Esposito
2022-11-22 8:27 ` Kevin Wolf
2022-11-16 12:22 ` [PATCH v4 06/11] block-coroutine-wrapper.py: support also basic return types Emanuele Giuseppe Esposito
2022-11-21 15:55 ` Kevin Wolf
2022-11-16 12:22 ` [PATCH v4 07/11] block/vmdk: add missing coroutine_fn annotations Emanuele Giuseppe Esposito
2022-11-21 16:01 ` Kevin Wolf
2022-11-21 16:07 ` Emanuele Giuseppe Esposito
2022-11-16 12:22 ` [PATCH v4 08/11] block: distinguish between bdrv_create running in coroutine and not Emanuele Giuseppe Esposito
2022-11-22 8:45 ` Kevin Wolf
2022-11-16 12:22 ` [PATCH v4 09/11] block: bdrv_create_file is a coroutine_fn Emanuele Giuseppe Esposito
2022-11-22 8:58 ` Kevin Wolf
2022-11-22 9:04 ` Emanuele Giuseppe Esposito
2022-11-16 12:22 ` [PATCH v4 10/11] block: convert bdrv_create to generated_co_wrapper_simple Emanuele Giuseppe Esposito
2022-11-22 9:16 ` Kevin Wolf
2022-11-16 12:22 ` [PATCH v4 11/11] block/dirty-bitmap: convert coroutine-only functions " Emanuele Giuseppe Esposito
2022-11-22 10:04 ` Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.