* [PATCH v6 01/14] block-io: introduce coroutine_fn duplicates for bdrv_common_block_status_above callers
2022-11-25 13:35 [PATCH v6 00/14] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
@ 2022-11-25 13:35 ` Emanuele Giuseppe Esposito
2022-11-25 17:00 ` Vladimir Sementsov-Ogievskiy
2022-11-25 13:35 ` [PATCH v6 02/14] block-copy: add missing coroutine_fn annotations Emanuele Giuseppe Esposito
` (12 subsequent siblings)
13 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-25 13:35 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow,
Vladimir Sementsov-Ogievskiy, Eric Blake, Fam Zheng,
Stefan Hajnoczi, Denis V. Lunev, Stefan Weil, Jeff Cody,
Cleber Rosa, qemu-devel, Emanuele Giuseppe Esposito
bdrv_common_block_status_above() is a g_c_w, and it is being called by
many "wrapper" functions like bdrv_is_allocated(),
bdrv_is_allocated_above() and bdrv_block_status_above().
Because we want to eventually split the coroutine from non-coroutine
case in g_c_w, create duplicate wrappers that take care of directly
calling the same coroutine functions called in the g_c_w.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block/io.c | 58 +++++++++++++++++++++++++++++++++++++---
include/block/block-io.h | 15 +++++++++++
2 files changed, 70 insertions(+), 3 deletions(-)
diff --git a/block/io.c b/block/io.c
index 38e57d1f67..f4444b7777 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2533,6 +2533,17 @@ bdrv_co_common_block_status_above(BlockDriverState *bs,
return ret;
}
+int coroutine_fn bdrv_co_block_status_above(BlockDriverState *bs,
+ BlockDriverState *base,
+ int64_t offset, int64_t bytes,
+ int64_t *pnum, int64_t *map,
+ BlockDriverState **file)
+{
+ IO_CODE();
+ return bdrv_co_common_block_status_above(bs, base, false, true, offset,
+ bytes, pnum, map, file, NULL);
+}
+
int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base,
int64_t offset, int64_t bytes, int64_t *pnum,
int64_t *map, BlockDriverState **file)
@@ -2578,6 +2589,22 @@ int coroutine_fn bdrv_co_is_zero_fast(BlockDriverState *bs, int64_t offset,
return (pnum == bytes) && (ret & BDRV_BLOCK_ZERO);
}
+int coroutine_fn bdrv_co_is_allocated(BlockDriverState *bs, int64_t offset,
+ int64_t bytes, int64_t *pnum)
+{
+ int ret;
+ int64_t dummy;
+ IO_CODE();
+
+ ret = bdrv_co_common_block_status_above(bs, bs, true, false, offset,
+ bytes, pnum ? pnum : &dummy, NULL,
+ NULL, NULL);
+ if (ret < 0) {
+ return ret;
+ }
+ return !!(ret & BDRV_BLOCK_ALLOCATED);
+}
+
int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
int64_t *pnum)
{
@@ -2594,6 +2621,29 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
return !!(ret & BDRV_BLOCK_ALLOCATED);
}
+/* See bdrv_is_allocated_above for documentation */
+int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
+ BlockDriverState *base,
+ bool include_base, int64_t offset,
+ int64_t bytes, int64_t *pnum)
+{
+ int depth;
+ int ret;
+ IO_CODE();
+
+ ret = bdrv_co_common_block_status_above(top, base, include_base, false,
+ offset, bytes, pnum, NULL, NULL,
+ &depth);
+ if (ret < 0) {
+ return ret;
+ }
+
+ if (ret & BDRV_BLOCK_ALLOCATED) {
+ return depth;
+ }
+ return 0;
+}
+
/*
* Given an image chain: ... -> [BASE] -> [INTER1] -> [INTER2] -> [TOP]
*
@@ -2617,10 +2667,12 @@ int bdrv_is_allocated_above(BlockDriverState *top,
int64_t bytes, int64_t *pnum)
{
int depth;
- int ret = bdrv_common_block_status_above(top, base, include_base, false,
- offset, bytes, pnum, NULL, NULL,
- &depth);
+ int ret;
IO_CODE();
+
+ ret = bdrv_common_block_status_above(top, base, include_base, false,
+ offset, bytes, pnum, NULL, NULL,
+ &depth);
if (ret < 0) {
return ret;
}
diff --git a/include/block/block-io.h b/include/block/block-io.h
index 92aaa7c1e9..72919254cd 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -94,14 +94,29 @@ bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs);
int bdrv_block_status(BlockDriverState *bs, int64_t offset,
int64_t bytes, int64_t *pnum, int64_t *map,
BlockDriverState **file);
+
+int coroutine_fn bdrv_co_block_status_above(BlockDriverState *bs,
+ BlockDriverState *base,
+ int64_t offset, int64_t bytes,
+ int64_t *pnum, int64_t *map,
+ BlockDriverState **file);
int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base,
int64_t offset, int64_t bytes, int64_t *pnum,
int64_t *map, BlockDriverState **file);
+
+int coroutine_fn bdrv_co_is_allocated(BlockDriverState *bs, int64_t offset,
+ int64_t bytes, int64_t *pnum);
int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
int64_t *pnum);
+
+int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
+ BlockDriverState *base,
+ bool include_base, int64_t offset,
+ int64_t bytes, int64_t *pnum);
int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
bool include_base, int64_t offset, int64_t bytes,
int64_t *pnum);
+
int coroutine_fn bdrv_co_is_zero_fast(BlockDriverState *bs, int64_t offset,
int64_t bytes);
--
2.31.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v6 01/14] block-io: introduce coroutine_fn duplicates for bdrv_common_block_status_above callers
2022-11-25 13:35 ` [PATCH v6 01/14] block-io: introduce coroutine_fn duplicates for bdrv_common_block_status_above callers Emanuele Giuseppe Esposito
@ 2022-11-25 17:00 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-11-25 17:00 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito, qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow, Eric Blake,
Fam Zheng, Stefan Hajnoczi, Denis V. Lunev, Stefan Weil,
Jeff Cody, Cleber Rosa, qemu-devel
On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
> bdrv_common_block_status_above() is a g_c_w, and it is being called by
> many "wrapper" functions like bdrv_is_allocated(),
> bdrv_is_allocated_above() and bdrv_block_status_above().
>
> Because we want to eventually split the coroutine from non-coroutine
> case in g_c_w, create duplicate wrappers that take care of directly
> calling the same coroutine functions called in the g_c_w.
>
> Signed-off-by: Emanuele Giuseppe Esposito<eesposit@redhat.com>
> Reviewed-by: Kevin Wolf<kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v6 02/14] block-copy: add missing coroutine_fn annotations
2022-11-25 13:35 [PATCH v6 00/14] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
2022-11-25 13:35 ` [PATCH v6 01/14] block-io: introduce coroutine_fn duplicates for bdrv_common_block_status_above callers Emanuele Giuseppe Esposito
@ 2022-11-25 13:35 ` Emanuele Giuseppe Esposito
2022-11-25 17:08 ` Vladimir Sementsov-Ogievskiy
2022-11-25 13:35 ` [PATCH v6 03/14] nbd/server.c: " Emanuele Giuseppe Esposito
` (11 subsequent siblings)
13 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-25 13:35 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow,
Vladimir Sementsov-Ogievskiy, Eric Blake, Fam Zheng,
Stefan Hajnoczi, Denis V. Lunev, Stefan Weil, Jeff Cody,
Cleber Rosa, 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>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block/block-copy.c | 21 ++++++++++++---------
include/block/block-copy.h | 5 +++--
2 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/block/block-copy.c b/block/block-copy.c
index bb947afdda..5e59d6262f 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;
@@ -590,8 +591,8 @@ static int block_copy_block_status(BlockCopyState *s, int64_t offset,
base = NULL;
}
- ret = bdrv_block_status_above(s->source->bs, base, offset, bytes, &num,
- NULL, NULL);
+ ret = bdrv_co_block_status_above(s->source->bs, base, offset, bytes, &num,
+ NULL, NULL);
if (ret < 0 || num < s->cluster_size) {
/*
* On error or if failed to obtain large enough chunk just fallback to
@@ -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;
@@ -624,7 +626,7 @@ static int block_copy_is_cluster_allocated(BlockCopyState *s, int64_t offset,
assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
while (true) {
- ret = bdrv_is_allocated(bs, offset, bytes, &count);
+ ret = bdrv_co_is_allocated(bs, offset, bytes, &count);
if (ret < 0) {
return ret;
}
@@ -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;
diff --git a/include/block/block-copy.h b/include/block/block-copy.h
index ba0b425d78..8cea4f9b90 100644
--- a/include/block/block-copy.h
+++ b/include/block/block-copy.h
@@ -36,8 +36,9 @@ void block_copy_set_progress_meter(BlockCopyState *s, ProgressMeter *pm);
void block_copy_state_free(BlockCopyState *s);
void block_copy_reset(BlockCopyState *s, int64_t offset, int64_t bytes);
-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 coroutine_fn block_copy(BlockCopyState *s, int64_t offset, int64_t bytes,
bool ignore_ratelimit, uint64_t timeout_ns,
--
2.31.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v6 02/14] block-copy: add missing coroutine_fn annotations
2022-11-25 13:35 ` [PATCH v6 02/14] block-copy: add missing coroutine_fn annotations Emanuele Giuseppe Esposito
@ 2022-11-25 17:08 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-11-25 17:08 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito, qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow, Eric Blake,
Fam Zheng, Stefan Hajnoczi, Denis V. Lunev, Stefan Weil,
Jeff Cody, Cleber Rosa, qemu-devel
On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
> 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.
I still think that "we need to mark" is not quite correct. Actually "we can mark", as such marking doesn't brake things. And we do have a reason for it, which comes in the following patches, but the commit message say nothing about it.
As well as in subject: they are not "missing", as nothing wrong for function not to be "coroutine_fn", when it doesn't call other "coroutine_fn"s.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
change is OK:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v6 03/14] nbd/server.c: add missing coroutine_fn annotations
2022-11-25 13:35 [PATCH v6 00/14] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
2022-11-25 13:35 ` [PATCH v6 01/14] block-io: introduce coroutine_fn duplicates for bdrv_common_block_status_above callers Emanuele Giuseppe Esposito
2022-11-25 13:35 ` [PATCH v6 02/14] block-copy: add missing coroutine_fn annotations Emanuele Giuseppe Esposito
@ 2022-11-25 13:35 ` Emanuele Giuseppe Esposito
2022-11-25 17:11 ` Vladimir Sementsov-Ogievskiy
2022-11-25 13:35 ` [PATCH v6 04/14] block-backend: replace bdrv_*_above with blk_*_above Emanuele Giuseppe Esposito
` (10 subsequent siblings)
13 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-25 13:35 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow,
Vladimir Sementsov-Ogievskiy, Eric Blake, Fam Zheng,
Stefan Hajnoczi, Denis V. Lunev, Stefan Weil, Jeff Cody,
Cleber Rosa, 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>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
nbd/server.c | 29 ++++++++++++++++-------------
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index ada16089f3..4af5c793a7 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2141,14 +2141,15 @@ 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;
int64_t num;
- int ret = bdrv_block_status_above(bs, NULL, offset, bytes, &num,
- NULL, NULL);
+ int ret = bdrv_co_block_status_above(bs, NULL, offset, bytes, &num,
+ NULL, NULL);
if (ret < 0) {
return ret;
@@ -2168,13 +2169,14 @@ 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;
- int ret = bdrv_is_allocated_above(bs, NULL, false, offset, bytes,
- &num);
+ int ret = bdrv_co_is_allocated_above(bs, NULL, false, offset, bytes,
+ &num);
if (ret < 0) {
return ret;
@@ -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* Re: [PATCH v6 03/14] nbd/server.c: add missing coroutine_fn annotations
2022-11-25 13:35 ` [PATCH v6 03/14] nbd/server.c: " Emanuele Giuseppe Esposito
@ 2022-11-25 17:11 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-11-25 17:11 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito, qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow, Eric Blake,
Fam Zheng, Stefan Hajnoczi, Denis V. Lunev, Stefan Weil,
Jeff Cody, Cleber Rosa, qemu-devel
On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
> 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.
Same here about "need" and "missing".
>
> Signed-off-by: Emanuele Giuseppe Esposito<eesposit@redhat.com>
> Reviewed-by: Kevin Wolf<kwolf@redhat.com>
> Reviewed-by: Paolo Bonzini<pbonzini@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v6 04/14] block-backend: replace bdrv_*_above with blk_*_above
2022-11-25 13:35 [PATCH v6 00/14] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (2 preceding siblings ...)
2022-11-25 13:35 ` [PATCH v6 03/14] nbd/server.c: " Emanuele Giuseppe Esposito
@ 2022-11-25 13:35 ` Emanuele Giuseppe Esposito
2022-11-25 17:16 ` Vladimir Sementsov-Ogievskiy
2022-11-25 13:35 ` [PATCH v6 05/14] block/vmdk: add missing coroutine_fn annotations Emanuele Giuseppe Esposito
` (9 subsequent siblings)
13 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-25 13:35 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow,
Vladimir Sementsov-Ogievskiy, Eric Blake, Fam Zheng,
Stefan Hajnoczi, Denis V. Lunev, Stefan Weil, Jeff Cody,
Cleber Rosa, qemu-devel, Emanuele Giuseppe Esposito
Avoid mixing bdrv_* functions with blk_*, so create blk_* counterparts
for bdrv_block_status_above and bdrv_is_allocated_above.
Note that since blk_co_block_status_above only calls the g_c_w function
bdrv_common_block_status_above and is marked as coroutine_fn, call
directly bdrv_co_common_block_status_above() to avoid using a g_c_w.
Same applies to blk_co_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 | 32 +++++++++++++++----------------
4 files changed, 48 insertions(+), 18 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 742efa7955..1d2d8526ef 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_co_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_co_block_status_above(blk_bs(blk), base, offset, bytes, pnum,
+ map, file);
+}
+
+int coroutine_fn blk_co_is_allocated_above(BlockBackend *blk,
+ BlockDriverState *base,
+ bool include_base, int64_t offset,
+ int64_t bytes, int64_t *pnum)
+{
+ IO_CODE();
+ return bdrv_co_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..b346341767 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_co_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..ee3eb12610 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_co_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_co_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 4af5c793a7..c53c39560e 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_co_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_co_block_status_above(exp->common.blk, NULL,
+ offset + progress,
+ size - progress, &pnum, NULL,
+ NULL);
bool final;
if (status < 0) {
@@ -2141,15 +2141,15 @@ 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_co_block_status_above(bs, NULL, offset, bytes, &num,
- NULL, NULL);
+ int ret = blk_co_block_status_above(blk, NULL, offset, bytes, &num,
+ NULL, NULL);
if (ret < 0) {
return ret;
@@ -2169,14 +2169,14 @@ 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_co_is_allocated_above(bs, NULL, false, offset, bytes,
- &num);
+ int ret = blk_co_is_allocated_above(blk, NULL, false, offset, bytes,
+ &num);
if (ret < 0) {
return ret;
@@ -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,
--
2.31.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v6 04/14] block-backend: replace bdrv_*_above with blk_*_above
2022-11-25 13:35 ` [PATCH v6 04/14] block-backend: replace bdrv_*_above with blk_*_above Emanuele Giuseppe Esposito
@ 2022-11-25 17:16 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-11-25 17:16 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito, qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow, Eric Blake,
Fam Zheng, Stefan Hajnoczi, Denis V. Lunev, Stefan Weil,
Jeff Cody, Cleber Rosa, qemu-devel
On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
> Avoid mixing bdrv_* functions with blk_*, so create blk_* counterparts
> for bdrv_block_status_above and bdrv_is_allocated_above.
>
> Note that since blk_co_block_status_above only calls the g_c_w function
> bdrv_common_block_status_above and is marked as coroutine_fn, call
> directly bdrv_co_common_block_status_above() to avoid using a g_c_w.
> Same applies to blk_co_is_allocated_above.
>
> Signed-off-by: Emanuele Giuseppe Esposito<eesposit@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v6 05/14] block/vmdk: add missing coroutine_fn annotations
2022-11-25 13:35 [PATCH v6 00/14] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (3 preceding siblings ...)
2022-11-25 13:35 ` [PATCH v6 04/14] block-backend: replace bdrv_*_above with blk_*_above Emanuele Giuseppe Esposito
@ 2022-11-25 13:35 ` Emanuele Giuseppe Esposito
2022-11-25 17:21 ` Vladimir Sementsov-Ogievskiy
2022-11-25 13:35 ` [PATCH v6 06/14] block: avoid duplicating filename string in bdrv_create Emanuele Giuseppe Esposito
` (8 subsequent siblings)
13 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-25 13:35 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow,
Vladimir Sementsov-Ogievskiy, Eric Blake, Fam Zheng,
Stefan Hajnoczi, Denis V. Lunev, Stefan Weil, Jeff Cody,
Cleber Rosa, 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>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@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* Re: [PATCH v6 05/14] block/vmdk: add missing coroutine_fn annotations
2022-11-25 13:35 ` [PATCH v6 05/14] block/vmdk: add missing coroutine_fn annotations Emanuele Giuseppe Esposito
@ 2022-11-25 17:21 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-11-25 17:21 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito, qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow, Eric Blake,
Fam Zheng, Stefan Hajnoczi, Denis V. Lunev, Stefan Weil,
Jeff Cody, Cleber Rosa, qemu-devel
On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
> 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.
same as 02 and 03
>
> Signed-off-by: Emanuele Giuseppe Esposito<eesposit@redhat.com>
> Reviewed-by: Paolo Bonzini<pbonzini@redhat.com>
> Reviewed-by: Kevin Wolf<kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v6 06/14] block: avoid duplicating filename string in bdrv_create
2022-11-25 13:35 [PATCH v6 00/14] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (4 preceding siblings ...)
2022-11-25 13:35 ` [PATCH v6 05/14] block/vmdk: add missing coroutine_fn annotations Emanuele Giuseppe Esposito
@ 2022-11-25 13:35 ` Emanuele Giuseppe Esposito
2022-11-25 17:24 ` Vladimir Sementsov-Ogievskiy
2022-11-25 13:35 ` [PATCH v6 07/14] block: distinguish between bdrv_create running in coroutine and not Emanuele Giuseppe Esposito
` (7 subsequent siblings)
13 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-25 13:35 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow,
Vladimir Sementsov-Ogievskiy, Eric Blake, Fam Zheng,
Stefan Hajnoczi, Denis V. Lunev, Stefan Weil, Jeff Cody,
Cleber Rosa, qemu-devel, Emanuele Giuseppe Esposito
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.
Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/block.c b/block.c
index 8c9f4ee37c..9d51e7b6e5 100644
--- a/block.c
+++ b/block.c
@@ -553,7 +553,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
Coroutine *co;
CreateCo cco = {
.drv = drv,
- .filename = g_strdup(filename),
+ .filename = filename,
.opts = opts,
.ret = NOT_DONE,
.err = NULL,
@@ -561,8 +561,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
if (!drv->bdrv_co_create_opts) {
error_setg(errp, "Driver '%s' does not support image creation", drv->format_name);
- ret = -ENOTSUP;
- goto out;
+ return -ENOTSUP;
}
if (qemu_in_coroutine()) {
@@ -585,8 +584,6 @@ int bdrv_create(BlockDriver *drv, const char* filename,
}
}
-out:
- g_free(cco.filename);
return ret;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v6 06/14] block: avoid duplicating filename string in bdrv_create
2022-11-25 13:35 ` [PATCH v6 06/14] block: avoid duplicating filename string in bdrv_create Emanuele Giuseppe Esposito
@ 2022-11-25 17:24 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-11-25 17:24 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito, qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow, Eric Blake,
Fam Zheng, Stefan Hajnoczi, Denis V. Lunev, Stefan Weil,
Jeff Cody, Cleber Rosa, qemu-devel
On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
> 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.
>
> Suggested-by: Kevin Wolf<kwolf@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito<eesposit@redhat.com>
> Reviewed-by: Kevin Wolf<kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v6 07/14] block: distinguish between bdrv_create running in coroutine and not
2022-11-25 13:35 [PATCH v6 00/14] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (5 preceding siblings ...)
2022-11-25 13:35 ` [PATCH v6 06/14] block: avoid duplicating filename string in bdrv_create Emanuele Giuseppe Esposito
@ 2022-11-25 13:35 ` Emanuele Giuseppe Esposito
2022-11-25 18:03 ` Vladimir Sementsov-Ogievskiy
2022-11-25 13:35 ` [PATCH v6 08/14] block: bdrv_create_file is a coroutine_fn Emanuele Giuseppe Esposito
` (6 subsequent siblings)
13 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-25 13:35 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow,
Vladimir Sementsov-Ogievskiy, Eric Blake, Fam Zheng,
Stefan Hajnoczi, Denis V. Lunev, Stefan Weil, Jeff Cody,
Cleber Rosa, 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>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 71 +++++++++++++++++++++++++++++----------------------------
1 file changed, 36 insertions(+), 35 deletions(-)
diff --git a/block.c b/block.c
index 9d51e7b6e5..2cf50b37c4 100644
--- a/block.c
+++ b/block.c
@@ -528,63 +528,64 @@ typedef struct CreateCo {
Error *err;
} CreateCo;
-static void coroutine_fn bdrv_create_co_entry(void *opaque)
+static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
+ QemuOpts *opts, Error **errp)
{
- Error *local_err = NULL;
int ret;
+ GLOBAL_STATE_CODE();
+ ERRP_GUARD();
+ 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;
+ }
+
+ ret = drv->bdrv_co_create_opts(drv, filename, opts, errp);
+ if (ret < 0 && !*errp) {
+ error_setg_errno(errp, -ret, "Could not create image");
+ }
+
+ return ret;
+}
+
+static void coroutine_fn bdrv_create_co_entry(void *opaque)
+{
CreateCo *cco = opaque;
- assert(cco->drv);
GLOBAL_STATE_CODE();
- 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 = 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);
- return -ENOTSUP;
- }
-
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");
- }
- }
-
- return ret;
}
/**
--
2.31.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v6 07/14] block: distinguish between bdrv_create running in coroutine and not
2022-11-25 13:35 ` [PATCH v6 07/14] block: distinguish between bdrv_create running in coroutine and not Emanuele Giuseppe Esposito
@ 2022-11-25 18:03 ` Vladimir Sementsov-Ogievskiy
2022-11-28 9:11 ` Emanuele Giuseppe Esposito
0 siblings, 1 reply; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-11-25 18:03 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito, qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow, Eric Blake,
Fam Zheng, Stefan Hajnoczi, Denis V. Lunev, Stefan Weil,
Jeff Cody, Cleber Rosa, qemu-devel
On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
> Call two different functions depending on whether bdrv_create
> is in coroutine or not, following the same pattern as
> generated_co_wrapper functions.
>
> This allows to also call the coroutine function directly,
> without using CreateCo or relying in bdrv_create().
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block.c | 71 +++++++++++++++++++++++++++++----------------------------
> 1 file changed, 36 insertions(+), 35 deletions(-)
>
> diff --git a/block.c b/block.c
> index 9d51e7b6e5..2cf50b37c4 100644
> --- a/block.c
> +++ b/block.c
> @@ -528,63 +528,64 @@ typedef struct CreateCo {
> Error *err;
> } CreateCo;
>
> -static void coroutine_fn bdrv_create_co_entry(void *opaque)
> +static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
> + QemuOpts *opts, Error **errp)
> {
> - Error *local_err = NULL;
> int ret;
> + GLOBAL_STATE_CODE();
> + ERRP_GUARD();
> + assert(*errp == NULL);
> + assert(drv);
Why we need these two assertions? These are general assumptions, and we don't assert it in all functions. Dereference of NULL will crash not worse than assertion. I'd drop them.
> +
> + if (!drv->bdrv_co_create_opts) {
> + error_setg(errp, "Driver '%s' does not support image creation",
> + drv->format_name);
> + return -ENOTSUP;
> + }
> +
> + ret = drv->bdrv_co_create_opts(drv, filename, opts, errp);
>
and this empty line, looks accidental.
Offtopic: hope one day we fix *open* functions to always set errp on error paths.
> + if (ret < 0 && !*errp) {
> + error_setg_errno(errp, -ret, "Could not create image");
> + }
> +
> + return ret;
> +}
> +
> +static void coroutine_fn bdrv_create_co_entry(void *opaque)
> +{
> CreateCo *cco = opaque;
> - assert(cco->drv);
> GLOBAL_STATE_CODE();
>
> - 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);
We need aio_wait_kick() call here, like in other co_entry() functions. Otherwise we may stuck in aio_poll()
with it:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Hmm actually, we can simply merge this patch into patch 13 (and move 08 to be after 13). Why to refactor bdrv_create twice?
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v6 07/14] block: distinguish between bdrv_create running in coroutine and not
2022-11-25 18:03 ` Vladimir Sementsov-Ogievskiy
@ 2022-11-28 9:11 ` Emanuele Giuseppe Esposito
0 siblings, 0 replies; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-28 9:11 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow, Eric Blake,
Fam Zheng, Stefan Hajnoczi, Denis V. Lunev, Stefan Weil,
Jeff Cody, Cleber Rosa, qemu-devel
Am 25/11/2022 um 19:03 schrieb Vladimir Sementsov-Ogievskiy:
> On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
>> Call two different functions depending on whether bdrv_create
>> is in coroutine or not, following the same pattern as
>> generated_co_wrapper functions.
>>
>> This allows to also call the coroutine function directly,
>> without using CreateCo or relying in bdrv_create().
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>> block.c | 71 +++++++++++++++++++++++++++++----------------------------
>> 1 file changed, 36 insertions(+), 35 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 9d51e7b6e5..2cf50b37c4 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -528,63 +528,64 @@ typedef struct CreateCo {
>> Error *err;
>> } CreateCo;
>> -static void coroutine_fn bdrv_create_co_entry(void *opaque)
>> +static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char
>> *filename,
>> + QemuOpts *opts, Error **errp)
>> {
>> - Error *local_err = NULL;
>> int ret;
>> + GLOBAL_STATE_CODE();
>> + ERRP_GUARD();
>> + assert(*errp == NULL);
>> + assert(drv);
>
> Why we need these two assertions? These are general assumptions, and we
> don't assert it in all functions. Dereference of NULL will crash not
> worse than assertion. I'd drop them.
>
>> +
>> + if (!drv->bdrv_co_create_opts) {
>> + error_setg(errp, "Driver '%s' does not support image creation",
>> + drv->format_name);
>> + return -ENOTSUP;
>> + }
>> +
>> + ret = drv->bdrv_co_create_opts(drv, filename, opts, errp);
>>
>
> and this empty line, looks accidental.
>
> Offtopic: hope one day we fix *open* functions to always set errp on
> error paths.
>
>> + if (ret < 0 && !*errp) {
>> + error_setg_errno(errp, -ret, "Could not create image");
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static void coroutine_fn bdrv_create_co_entry(void *opaque)
>> +{
>> CreateCo *cco = opaque;
>> - assert(cco->drv);
>> GLOBAL_STATE_CODE();
>> - 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);
>
> We need aio_wait_kick() call here, like in other co_entry() functions.
> Otherwise we may stuck in aio_poll()
>
> with it:
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
> Hmm actually, we can simply merge this patch into patch 13 (and move 08
> to be after 13). Why to refactor bdrv_create twice?
>
I think I'll leave the patches separate, otherwise I am worried that it
will be difficult to understand what is happending and why when we merge
these 2 operations (move all logic to _co_ and using co_wrapper) together.
I agree with the rest.
Thank you,
Emanuele
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v6 08/14] block: bdrv_create_file is a coroutine_fn
2022-11-25 13:35 [PATCH v6 00/14] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (6 preceding siblings ...)
2022-11-25 13:35 ` [PATCH v6 07/14] block: distinguish between bdrv_create running in coroutine and not Emanuele Giuseppe Esposito
@ 2022-11-25 13:35 ` Emanuele Giuseppe Esposito
2022-11-25 18:08 ` Vladimir Sementsov-Ogievskiy
2022-11-25 13:35 ` [PATCH v6 09/14] block: rename generated_co_wrapper in co_wrapper_mixed Emanuele Giuseppe Esposito
` (5 subsequent siblings)
13 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-25 13:35 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow,
Vladimir Sementsov-Ogievskiy, Eric Blake, Fam Zheng,
Stefan Hajnoczi, Denis V. Lunev, Stefan Weil, Jeff Cody,
Cleber Rosa, qemu-devel, Emanuele Giuseppe Esposito
It is always called in coroutine_fn callbacks, therefore
it can directly call bdrv_co_create().
Rename it to bdrv_co_create_file too.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 5 +++--
block/crypto.c | 2 +-
block/parallels.c | 2 +-
block/qcow.c | 2 +-
block/qcow2.c | 4 ++--
block/qed.c | 2 +-
block/raw-format.c | 2 +-
block/vdi.c | 2 +-
block/vhdx.c | 2 +-
block/vmdk.c | 2 +-
block/vpc.c | 2 +-
include/block/block-global-state.h | 3 ++-
12 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/block.c b/block.c
index 2cf50b37c4..772df02d54 100644
--- a/block.c
+++ b/block.c
@@ -723,7 +723,8 @@ out:
return ret;
}
-int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
+int coroutine_fn bdrv_co_create_file(const char *filename, QemuOpts *opts,
+ Error **errp)
{
QemuOpts *protocol_opts;
BlockDriver *drv;
@@ -764,7 +765,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/block/crypto.c b/block/crypto.c
index 2fb8add458..bbeb9f437c 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -703,7 +703,7 @@ static int coroutine_fn block_crypto_co_create_opts_luks(BlockDriver *drv,
}
/* Create protocol layer */
- ret = bdrv_create_file(filename, opts, errp);
+ ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto fail;
}
diff --git a/block/parallels.c b/block/parallels.c
index fa08c1104b..bbea2f2221 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -646,7 +646,7 @@ static int coroutine_fn parallels_co_create_opts(BlockDriver *drv,
}
/* Create and open the file (protocol layer) */
- ret = bdrv_create_file(filename, opts, errp);
+ ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto done;
}
diff --git a/block/qcow.c b/block/qcow.c
index daa38839ab..18e17a5b12 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -973,7 +973,7 @@ static int coroutine_fn qcow_co_create_opts(BlockDriver *drv,
}
/* Create and open the file (protocol layer) */
- ret = bdrv_create_file(filename, opts, errp);
+ ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto fail;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index 4d6666d3ff..7cc49a3a6c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3871,7 +3871,7 @@ static int coroutine_fn qcow2_co_create_opts(BlockDriver *drv,
}
/* Create and open the file (protocol layer) */
- ret = bdrv_create_file(filename, opts, errp);
+ ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto finish;
}
@@ -3886,7 +3886,7 @@ static int coroutine_fn qcow2_co_create_opts(BlockDriver *drv,
/* Create and open an external data file (protocol layer) */
val = qdict_get_try_str(qdict, BLOCK_OPT_DATA_FILE);
if (val) {
- ret = bdrv_create_file(val, opts, errp);
+ ret = bdrv_co_create_file(val, opts, errp);
if (ret < 0) {
goto finish;
}
diff --git a/block/qed.c b/block/qed.c
index c2691a85b1..9d54c8eec5 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -778,7 +778,7 @@ static int coroutine_fn bdrv_qed_co_create_opts(BlockDriver *drv,
}
/* Create and open the file (protocol layer) */
- ret = bdrv_create_file(filename, opts, errp);
+ ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto fail;
}
diff --git a/block/raw-format.c b/block/raw-format.c
index a68014ef0b..28905b09ee 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -433,7 +433,7 @@ static int coroutine_fn raw_co_create_opts(BlockDriver *drv,
QemuOpts *opts,
Error **errp)
{
- return bdrv_create_file(filename, opts, errp);
+ return bdrv_co_create_file(filename, opts, errp);
}
static int raw_open(BlockDriverState *bs, QDict *options, int flags,
diff --git a/block/vdi.c b/block/vdi.c
index c0c111c4b9..479bcfe820 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -934,7 +934,7 @@ static int coroutine_fn vdi_co_create_opts(BlockDriver *drv,
qdict = qemu_opts_to_qdict_filtered(opts, NULL, &vdi_create_opts, true);
/* Create and open the file (protocol layer) */
- ret = bdrv_create_file(filename, opts, errp);
+ ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto done;
}
diff --git a/block/vhdx.c b/block/vhdx.c
index bad9ca691b..4c929800fe 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -2084,7 +2084,7 @@ static int coroutine_fn vhdx_co_create_opts(BlockDriver *drv,
}
/* Create and open the file (protocol layer) */
- ret = bdrv_create_file(filename, opts, errp);
+ ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto fail;
}
diff --git a/block/vmdk.c b/block/vmdk.c
index 0c32bf2e83..afd3471915 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2294,7 +2294,7 @@ static int coroutine_fn vmdk_create_extent(const char *filename,
int ret;
BlockBackend *blk = NULL;
- ret = bdrv_create_file(filename, opts, errp);
+ ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto exit;
}
diff --git a/block/vpc.c b/block/vpc.c
index 95841f259a..6ee95dcb96 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1111,7 +1111,7 @@ static int coroutine_fn vpc_co_create_opts(BlockDriver *drv,
}
/* Create and open the file (protocol layer) */
- ret = bdrv_create_file(filename, opts, errp);
+ ret = bdrv_co_create_file(filename, opts, errp);
if (ret < 0) {
goto fail;
}
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 00e0cf8aea..387a7cbb2e 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_co_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* Re: [PATCH v6 08/14] block: bdrv_create_file is a coroutine_fn
2022-11-25 13:35 ` [PATCH v6 08/14] block: bdrv_create_file is a coroutine_fn Emanuele Giuseppe Esposito
@ 2022-11-25 18:08 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-11-25 18:08 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito, qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow, Eric Blake,
Fam Zheng, Stefan Hajnoczi, Denis V. Lunev, Stefan Weil,
Jeff Cody, Cleber Rosa, qemu-devel
On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
> It is always called in coroutine_fn callbacks, therefore
> it can directly call bdrv_co_create().
>
> Rename it to bdrv_co_create_file too.
>
> Signed-off-by: Emanuele Giuseppe Esposito<eesposit@redhat.com>
> Reviewed-by: Kevin Wolf<kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v6 09/14] block: rename generated_co_wrapper in co_wrapper_mixed
2022-11-25 13:35 [PATCH v6 00/14] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (7 preceding siblings ...)
2022-11-25 13:35 ` [PATCH v6 08/14] block: bdrv_create_file is a coroutine_fn Emanuele Giuseppe Esposito
@ 2022-11-25 13:35 ` Emanuele Giuseppe Esposito
2022-11-25 18:11 ` Vladimir Sementsov-Ogievskiy
2022-11-25 13:35 ` [PATCH v6 10/14] block-coroutine-wrapper.py: introduce co_wrapper Emanuele Giuseppe Esposito
` (4 subsequent siblings)
13 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-25 13:35 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow,
Vladimir Sementsov-Ogievskiy, Eric Blake, Fam Zheng,
Stefan Hajnoczi, Denis V. Lunev, Stefan Weil, Jeff Cody,
Cleber Rosa, qemu-devel, Emanuele Giuseppe Esposito
In preparation to the incoming new function specifiers,
rename g_c_w with a more meaningful name and document it.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
block/coroutines.h | 4 +-
docs/devel/block-coroutine-wrapper.rst | 6 +--
include/block/block-common.h | 11 +++--
include/block/block-io.h | 44 ++++++++---------
include/sysemu/block-backend-io.h | 68 +++++++++++++-------------
scripts/block-coroutine-wrapper.py | 6 +--
6 files changed, 71 insertions(+), 68 deletions(-)
diff --git a/block/coroutines.h b/block/coroutines.h
index 3a2bad564f..17da4db963 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -71,7 +71,7 @@ nbd_co_do_establish_connection(BlockDriverState *bs, bool blocking,
* the "I/O or GS" API.
*/
-int generated_co_wrapper
+int co_wrapper_mixed
bdrv_common_block_status_above(BlockDriverState *bs,
BlockDriverState *base,
bool include_base,
@@ -82,7 +82,7 @@ bdrv_common_block_status_above(BlockDriverState *bs,
int64_t *map,
BlockDriverState **file,
int *depth);
-int generated_co_wrapper
+int co_wrapper_mixed
nbd_do_establish_connection(BlockDriverState *bs, bool blocking, Error **errp);
#endif /* BLOCK_COROUTINES_H */
diff --git a/docs/devel/block-coroutine-wrapper.rst b/docs/devel/block-coroutine-wrapper.rst
index 412851986b..64acc8d65d 100644
--- a/docs/devel/block-coroutine-wrapper.rst
+++ b/docs/devel/block-coroutine-wrapper.rst
@@ -26,12 +26,12 @@ called ``bdrv_foo(<same args>)``. In this case the script can help. To
trigger the generation:
1. You need ``bdrv_foo`` declaration somewhere (for example, in
- ``block/coroutines.h``) with the ``generated_co_wrapper`` mark,
+ ``block/coroutines.h``) with the ``co_wrapper_mixed`` mark,
like this:
.. code-block:: c
- int generated_co_wrapper bdrv_foo(<some args>);
+ int co_wrapper_mixed bdrv_foo(<some args>);
2. You need to feed this declaration to block-coroutine-wrapper script.
For this, add the .h (or .c) file with the declaration to the
@@ -46,7 +46,7 @@ Links
1. The script location is ``scripts/block-coroutine-wrapper.py``.
-2. Generic place for private ``generated_co_wrapper`` declarations is
+2. Generic place for private ``co_wrapper_mixed`` declarations is
``block/coroutines.h``, for public declarations:
``include/block/block.h``
diff --git a/include/block/block-common.h b/include/block/block-common.h
index 297704c1e9..ec2309055b 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -35,14 +35,17 @@
#include "qemu/transactions.h"
/*
- * generated_co_wrapper
+ * co_wrapper{*}: Function specifiers used by block-coroutine-wrapper.py
*
- * Function specifier, which does nothing but mark functions to be
+ * Function specifiers, which do nothing but mark functions to be
* generated by scripts/block-coroutine-wrapper.py
*
- * Read more in docs/devel/block-coroutine-wrapper.rst
+ * Usage: read docs/devel/block-coroutine-wrapper.rst
+ *
+ * co_wrapper_mixed functions can be called by both coroutine and
+ * non-coroutine context.
*/
-#define generated_co_wrapper
+#define co_wrapper_mixed
/* block.c */
typedef struct BlockDriver BlockDriver;
diff --git a/include/block/block-io.h b/include/block/block-io.h
index 72919254cd..72cf45975b 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -39,19 +39,19 @@
* to catch when they are accidentally called by the wrong API.
*/
-int generated_co_wrapper bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
- int64_t bytes,
- BdrvRequestFlags flags);
+int co_wrapper_mixed bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
+ int64_t bytes,
+ BdrvRequestFlags flags);
int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags);
-int generated_co_wrapper bdrv_pread(BdrvChild *child, int64_t offset,
- int64_t bytes, void *buf,
- BdrvRequestFlags flags);
-int generated_co_wrapper bdrv_pwrite(BdrvChild *child, int64_t offset,
- int64_t bytes, const void *buf,
- BdrvRequestFlags flags);
-int generated_co_wrapper bdrv_pwrite_sync(BdrvChild *child, int64_t offset,
- int64_t bytes, const void *buf,
- BdrvRequestFlags flags);
+int co_wrapper_mixed bdrv_pread(BdrvChild *child, int64_t offset,
+ int64_t bytes, void *buf,
+ BdrvRequestFlags flags);
+int co_wrapper_mixed bdrv_pwrite(BdrvChild *child, int64_t offset,
+ int64_t bytes, const void *buf,
+ BdrvRequestFlags flags);
+int co_wrapper_mixed bdrv_pwrite_sync(BdrvChild *child, int64_t offset,
+ int64_t bytes, const void *buf,
+ BdrvRequestFlags flags);
int coroutine_fn bdrv_co_pwrite_sync(BdrvChild *child, int64_t offset,
int64_t bytes, const void *buf,
BdrvRequestFlags flags);
@@ -281,22 +281,22 @@ int coroutine_fn bdrv_co_copy_range(BdrvChild *src, int64_t src_offset,
void bdrv_drain(BlockDriverState *bs);
-int generated_co_wrapper
+int co_wrapper_mixed
bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
PreallocMode prealloc, BdrvRequestFlags flags, Error **errp);
-int generated_co_wrapper bdrv_check(BlockDriverState *bs, BdrvCheckResult *res,
- BdrvCheckMode fix);
+int co_wrapper_mixed bdrv_check(BlockDriverState *bs, BdrvCheckResult *res,
+ BdrvCheckMode fix);
/* Invalidate any cached metadata used by image formats */
-int generated_co_wrapper bdrv_invalidate_cache(BlockDriverState *bs,
- Error **errp);
-int generated_co_wrapper bdrv_flush(BlockDriverState *bs);
-int generated_co_wrapper bdrv_pdiscard(BdrvChild *child, int64_t offset,
- int64_t bytes);
-int generated_co_wrapper
+int co_wrapper_mixed bdrv_invalidate_cache(BlockDriverState *bs,
+ Error **errp);
+int co_wrapper_mixed bdrv_flush(BlockDriverState *bs);
+int co_wrapper_mixed bdrv_pdiscard(BdrvChild *child, int64_t offset,
+ int64_t bytes);
+int co_wrapper_mixed
bdrv_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
-int generated_co_wrapper
+int co_wrapper_mixed
bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
/**
diff --git a/include/sysemu/block-backend-io.h b/include/sysemu/block-backend-io.h
index ee3eb12610..7ec6d978d4 100644
--- a/include/sysemu/block-backend-io.h
+++ b/include/sysemu/block-backend-io.h
@@ -110,77 +110,77 @@ int coroutine_fn blk_co_is_allocated_above(BlockBackend *blk,
* the "I/O or GS" API.
*/
-int generated_co_wrapper blk_pread(BlockBackend *blk, int64_t offset,
- int64_t bytes, void *buf,
- BdrvRequestFlags flags);
+int co_wrapper_mixed blk_pread(BlockBackend *blk, int64_t offset,
+ int64_t bytes, void *buf,
+ BdrvRequestFlags flags);
int coroutine_fn blk_co_pread(BlockBackend *blk, int64_t offset, int64_t bytes,
void *buf, BdrvRequestFlags flags);
-int generated_co_wrapper blk_preadv(BlockBackend *blk, int64_t offset,
- int64_t bytes, QEMUIOVector *qiov,
- BdrvRequestFlags flags);
+int co_wrapper_mixed blk_preadv(BlockBackend *blk, int64_t offset,
+ int64_t bytes, QEMUIOVector *qiov,
+ BdrvRequestFlags flags);
int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
int64_t bytes, QEMUIOVector *qiov,
BdrvRequestFlags flags);
-int generated_co_wrapper blk_preadv_part(BlockBackend *blk, int64_t offset,
- int64_t bytes, QEMUIOVector *qiov,
- size_t qiov_offset,
- BdrvRequestFlags flags);
+int co_wrapper_mixed blk_preadv_part(BlockBackend *blk, int64_t offset,
+ int64_t bytes, QEMUIOVector *qiov,
+ size_t qiov_offset,
+ BdrvRequestFlags flags);
int coroutine_fn blk_co_preadv_part(BlockBackend *blk, int64_t offset,
int64_t bytes, QEMUIOVector *qiov,
size_t qiov_offset, BdrvRequestFlags flags);
-int generated_co_wrapper blk_pwrite(BlockBackend *blk, int64_t offset,
- int64_t bytes, const void *buf,
- BdrvRequestFlags flags);
+int co_wrapper_mixed blk_pwrite(BlockBackend *blk, int64_t offset,
+ int64_t bytes, const void *buf,
+ BdrvRequestFlags flags);
int coroutine_fn blk_co_pwrite(BlockBackend *blk, int64_t offset, int64_t bytes,
const void *buf, BdrvRequestFlags flags);
-int generated_co_wrapper blk_pwritev(BlockBackend *blk, int64_t offset,
- int64_t bytes, QEMUIOVector *qiov,
- BdrvRequestFlags flags);
+int co_wrapper_mixed blk_pwritev(BlockBackend *blk, int64_t offset,
+ int64_t bytes, QEMUIOVector *qiov,
+ BdrvRequestFlags flags);
int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
int64_t bytes, QEMUIOVector *qiov,
BdrvRequestFlags flags);
-int generated_co_wrapper blk_pwritev_part(BlockBackend *blk, int64_t offset,
- int64_t bytes, QEMUIOVector *qiov,
- size_t qiov_offset,
- BdrvRequestFlags flags);
+int co_wrapper_mixed blk_pwritev_part(BlockBackend *blk, int64_t offset,
+ int64_t bytes, QEMUIOVector *qiov,
+ size_t qiov_offset,
+ BdrvRequestFlags flags);
int coroutine_fn blk_co_pwritev_part(BlockBackend *blk, int64_t offset,
int64_t bytes,
QEMUIOVector *qiov, size_t qiov_offset,
BdrvRequestFlags flags);
-int generated_co_wrapper blk_pwrite_compressed(BlockBackend *blk,
- int64_t offset, int64_t bytes,
- const void *buf);
+int co_wrapper_mixed blk_pwrite_compressed(BlockBackend *blk,
+ int64_t offset, int64_t bytes,
+ const void *buf);
int coroutine_fn blk_co_pwrite_compressed(BlockBackend *blk, int64_t offset,
int64_t bytes, const void *buf);
-int generated_co_wrapper blk_pwrite_zeroes(BlockBackend *blk, int64_t offset,
- int64_t bytes,
- BdrvRequestFlags flags);
+int co_wrapper_mixed blk_pwrite_zeroes(BlockBackend *blk, int64_t offset,
+ int64_t bytes,
+ BdrvRequestFlags flags);
int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
int64_t bytes, BdrvRequestFlags flags);
-int generated_co_wrapper blk_pdiscard(BlockBackend *blk, int64_t offset,
- int64_t bytes);
+int co_wrapper_mixed blk_pdiscard(BlockBackend *blk, int64_t offset,
+ int64_t bytes);
int coroutine_fn blk_co_pdiscard(BlockBackend *blk, int64_t offset,
int64_t bytes);
-int generated_co_wrapper blk_flush(BlockBackend *blk);
+int co_wrapper_mixed blk_flush(BlockBackend *blk);
int coroutine_fn blk_co_flush(BlockBackend *blk);
-int generated_co_wrapper blk_ioctl(BlockBackend *blk, unsigned long int req,
- void *buf);
+int co_wrapper_mixed blk_ioctl(BlockBackend *blk, unsigned long int req,
+ void *buf);
int coroutine_fn blk_co_ioctl(BlockBackend *blk, unsigned long int req,
void *buf);
-int generated_co_wrapper blk_truncate(BlockBackend *blk, int64_t offset,
- bool exact, PreallocMode prealloc,
- BdrvRequestFlags flags, Error **errp);
+int co_wrapper_mixed blk_truncate(BlockBackend *blk, int64_t offset,
+ bool exact, PreallocMode prealloc,
+ BdrvRequestFlags flags, Error **errp);
int coroutine_fn blk_co_truncate(BlockBackend *blk, int64_t offset, bool exact,
PreallocMode prealloc, BdrvRequestFlags flags,
Error **errp);
diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
index 08be813407..56e6425356 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -2,7 +2,7 @@
"""Generate coroutine wrappers for block subsystem.
The program parses one or several concatenated c files from stdin,
-searches for functions with the 'generated_co_wrapper' specifier
+searches for functions with the 'co_wrapper_mixed' specifier
and generates corresponding wrappers on stdout.
Usage: block-coroutine-wrapper.py generated-file.c FILE.[ch]...
@@ -74,8 +74,8 @@ def gen_block(self, format: str) -> str:
return '\n'.join(format.format_map(arg.__dict__) for arg in self.args)
-# Match wrappers declared with a generated_co_wrapper mark
-func_decl_re = re.compile(r'^int\s*generated_co_wrapper\s*'
+# Match wrappers declared with a co_wrapper_mixed mark
+func_decl_re = re.compile(r'^int\s*co_wrapper_mixed\s*'
r'(?P<wrapper_name>[a-z][a-z0-9_]*)'
r'\((?P<args>[^)]*)\);$', re.MULTILINE)
--
2.31.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v6 09/14] block: rename generated_co_wrapper in co_wrapper_mixed
2022-11-25 13:35 ` [PATCH v6 09/14] block: rename generated_co_wrapper in co_wrapper_mixed Emanuele Giuseppe Esposito
@ 2022-11-25 18:11 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-11-25 18:11 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito, qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow, Eric Blake,
Fam Zheng, Stefan Hajnoczi, Denis V. Lunev, Stefan Weil,
Jeff Cody, Cleber Rosa, qemu-devel
On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
> In preparation to the incoming new function specifiers,
> rename g_c_w with a more meaningful name and document it.
>
> Signed-off-by: Emanuele Giuseppe Esposito<eesposit@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v6 10/14] block-coroutine-wrapper.py: introduce co_wrapper
2022-11-25 13:35 [PATCH v6 00/14] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (8 preceding siblings ...)
2022-11-25 13:35 ` [PATCH v6 09/14] block: rename generated_co_wrapper in co_wrapper_mixed Emanuele Giuseppe Esposito
@ 2022-11-25 13:35 ` Emanuele Giuseppe Esposito
2022-11-25 20:32 ` Vladimir Sementsov-Ogievskiy
2022-11-25 13:35 ` [PATCH v6 11/14] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter Emanuele Giuseppe Esposito
` (3 subsequent siblings)
13 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-25 13:35 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow,
Vladimir Sementsov-Ogievskiy, Eric Blake, Fam Zheng,
Stefan Hajnoczi, Denis V. Lunev, Stefan Weil, Jeff Cody,
Cleber Rosa, 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.
It will be the default annotation to be used in the future.
This is much better as c_w_mixed, 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 c_w_mixed functions will be
substituted by co_wrapper.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
docs/devel/block-coroutine-wrapper.rst | 6 +-
include/block/block-common.h | 8 +-
scripts/block-coroutine-wrapper.py | 109 +++++++++++++++++--------
3 files changed, 85 insertions(+), 38 deletions(-)
diff --git a/docs/devel/block-coroutine-wrapper.rst b/docs/devel/block-coroutine-wrapper.rst
index 64acc8d65d..6dd2cdcab3 100644
--- a/docs/devel/block-coroutine-wrapper.rst
+++ b/docs/devel/block-coroutine-wrapper.rst
@@ -26,12 +26,12 @@ called ``bdrv_foo(<same args>)``. In this case the script can help. To
trigger the generation:
1. You need ``bdrv_foo`` declaration somewhere (for example, in
- ``block/coroutines.h``) with the ``co_wrapper_mixed`` mark,
+ ``block/coroutines.h``) with the ``co_wrapper`` mark,
like this:
.. code-block:: c
- int co_wrapper_mixed bdrv_foo(<some args>);
+ int co_wrapper bdrv_foo(<some args>);
2. You need to feed this declaration to block-coroutine-wrapper script.
For this, add the .h (or .c) file with the declaration to the
@@ -46,7 +46,7 @@ Links
1. The script location is ``scripts/block-coroutine-wrapper.py``.
-2. Generic place for private ``co_wrapper_mixed`` declarations is
+2. Generic place for private ``co_wrapper`` declarations is
``block/coroutines.h``, for public declarations:
``include/block/block.h``
diff --git a/include/block/block-common.h b/include/block/block-common.h
index ec2309055b..847e4d4626 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -42,9 +42,13 @@
*
* Usage: read docs/devel/block-coroutine-wrapper.rst
*
- * co_wrapper_mixed functions can be called by both coroutine and
- * non-coroutine context.
+ * There are 2 kind of specifiers:
+ * - co_wrapper functions can be called by only non-coroutine context, because
+ * they always generate a new coroutine.
+ * - co_wrapper_mixed functions can be called by both coroutine and
+ * non-coroutine context.
*/
+#define co_wrapper
#define co_wrapper_mixed
/* block.c */
diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
index 56e6425356..7972d3fe01 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -2,7 +2,7 @@
"""Generate coroutine wrappers for block subsystem.
The program parses one or several concatenated c files from stdin,
-searches for functions with the 'co_wrapper_mixed' specifier
+searches for functions with the 'co_wrapper' specifier
and generates corresponding wrappers on stdout.
Usage: block-coroutine-wrapper.py generated-file.c FILE.[ch]...
@@ -62,10 +62,28 @@ 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.struct_name = snake_to_camel(self.name)
self.args = [ParamDecl(arg.strip()) for arg in args.split(',')]
+ self.create_only_co = True
+
+ if 'mixed' in variant:
+ self.create_only_co = False
+
+ subsystem, subname = self.name.split('_', 1)
+ self.co_name = f'{subsystem}_co_{subname}'
+
+ t = self.args[0].type
+ if t == 'BlockDriverState *':
+ bs = 'bs'
+ elif t == 'BdrvChild *':
+ bs = 'child->bs'
+ else:
+ bs = 'blk_bs(blk)'
+ self.bs = bs
def gen_list(self, format: str) -> str:
return ', '.join(format.format_map(arg.__dict__) for arg in self.args)
@@ -74,8 +92,9 @@ def gen_block(self, format: str) -> str:
return '\n'.join(format.format_map(arg.__dict__) for arg in self.args)
-# Match wrappers declared with a co_wrapper_mixed mark
-func_decl_re = re.compile(r'^int\s*co_wrapper_mixed\s*'
+# Match wrappers declared with a co_wrapper mark
+func_decl_re = re.compile(r'^int\s*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 +103,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,24 +117,63 @@ 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}') })
+{{
+ {struct_name} s = {{
+ .poll_state.bs = {func.bs},
+ .poll_state.in_progress = true,
+
+{ func.gen_block(' .{name} = {name},') }
+ }};
+ assert(!qemu_in_coroutine());
+
+ 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'
assert func.args[0].type in ['BlockDriverState *', 'BdrvChild *',
'BlockBackend *']
- subsystem, subname = func.name.split('_', 1)
-
- name = f'{subsystem}_co_{subname}'
+ name = func.co_name
+ struct_name = func.struct_name
- t = func.args[0].type
- if t == 'BlockDriverState *':
- bs = 'bs'
- elif t == 'BdrvChild *':
- bs = 'child->bs'
- else:
- bs = 'blk_bs(blk)'
- struct_name = snake_to_camel(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* Re: [PATCH v6 10/14] block-coroutine-wrapper.py: introduce co_wrapper
2022-11-25 13:35 ` [PATCH v6 10/14] block-coroutine-wrapper.py: introduce co_wrapper Emanuele Giuseppe Esposito
@ 2022-11-25 20:32 ` Vladimir Sementsov-Ogievskiy
2022-11-28 9:19 ` Emanuele Giuseppe Esposito
0 siblings, 1 reply; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-11-25 20:32 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito, qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow, Eric Blake,
Fam Zheng, Stefan Hajnoczi, Denis V. Lunev, Stefan Weil,
Jeff Cody, Cleber Rosa, qemu-devel
On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
> This new annotation creates just a function wrapper that creates
> a new coroutine.
Actually, not just create, but create, start and wait for finish.. Maybe s/creates/starts/
> It assumes the caller is not a coroutine.
> It will be the default annotation to be used in the future.
>
> This is much better as c_w_mixed, 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 c_w_mixed functions will be
> substituted by co_wrapper.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
[..]
>
> 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:
I'd prefer mixed: bool parameter instead, to be more strict.
> self.return_type = return_type.strip()
> self.name = name.strip()
> + self.struct_name = snake_to_camel(self.name)
> self.args = [ParamDecl(arg.strip()) for arg in args.split(',')]
> + self.create_only_co = True
> +
> + if 'mixed' in variant:
> + self.create_only_co = False
hmm, just
self.create_only_co = 'mixed' not in variant
? And even better with boolean argument.
> +
> + subsystem, subname = self.name.split('_', 1)
> + self.co_name = f'{subsystem}_co_{subname}'
> +
> + t = self.args[0].type
> + if t == 'BlockDriverState *':
> + bs = 'bs'
> + elif t == 'BdrvChild *':
> + bs = 'child->bs'
> + else:
> + bs = 'blk_bs(blk)'
> + self.bs = bs
>
> def gen_list(self, format: str) -> str:
> return ', '.join(format.format_map(arg.__dict__) for arg in self.args)
> @@ -74,8 +92,9 @@ def gen_block(self, format: str) -> str:
> return '\n'.join(format.format_map(arg.__dict__) for arg in self.args)
>
>
> -# Match wrappers declared with a co_wrapper_mixed mark
> -func_decl_re = re.compile(r'^int\s*co_wrapper_mixed\s*'
> +# Match wrappers declared with a co_wrapper mark
> +func_decl_re = re.compile(r'^int\s*co_wrapper'
> + r'(?P<variant>(_[a-z][a-z0-9_]*)?)\s*'
Why you allow everything here?
I'd write just
(?P<mixed>(_mixed)?)
or
(?P<marker>co_wrapper(_mixed)?)
> r'(?P<wrapper_name>[a-z][a-z0-9_]*)'
> r'\((?P<args>[^)]*)\);$', re.MULTILINE)
>
> @@ -84,7 +103,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,24 +117,63 @@ def snake_to_camel(func_name: str) -> str:
> return ''.join(words)
>
>
> +# Checks if we are already in coroutine
Please, do function documentation in doc-sting
> +def create_g_c_w(func: FuncDecl) -> str:
maybe, create_mixed_wrapper?
g_c_w refer to "generated_co_wrapper" and it was dropped from everywhere already
> + name = func.co_name
> + struct_name = func.struct_name
> + return f"""\
[..]
> +
> +# Assumes we are not in coroutine, and creates one
move to doc-string
> +def create_coroutine_only(func: FuncDecl) -> str:
maybe, create_co_wrapper ?
Also, in the file functions that generate code are called gen_*,
so better gen_co_wrapper() and gen_co_wrapper_mixed()
> + name = func.co_name
> + struct_name = func.struct_name
> + return f"""\
> +int {func.name}({ func.gen_list('{decl}') })
> +{{
[..]
>
> def gen_wrappers(input_code: str) -> str:
With at least comments for new functions turned into doc-strings:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH v6 10/14] block-coroutine-wrapper.py: introduce co_wrapper
2022-11-25 20:32 ` Vladimir Sementsov-Ogievskiy
@ 2022-11-28 9:19 ` Emanuele Giuseppe Esposito
0 siblings, 0 replies; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-28 9:19 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow, Eric Blake,
Fam Zheng, Stefan Hajnoczi, Denis V. Lunev, Stefan Weil,
Jeff Cody, Cleber Rosa, qemu-devel
Am 25/11/2022 um 21:32 schrieb Vladimir Sementsov-Ogievskiy:
>
>> 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:
>
> I'd prefer mixed: bool parameter instead, to be more strict.
>
>> self.return_type = return_type.strip()
>> self.name = name.strip()
>> + self.struct_name = snake_to_camel(self.name)
>> self.args = [ParamDecl(arg.strip()) for arg in args.split(',')]
>> + self.create_only_co = True
>> +
>> + if 'mixed' in variant:
>> + self.create_only_co = False
>
> hmm, just
>
> self.create_only_co = 'mixed' not in variant
>
> ? And even better with boolean argument.
>
>> +
>> + subsystem, subname = self.name.split('_', 1)
>> + self.co_name = f'{subsystem}_co_{subname}'
>> +
>> + t = self.args[0].type
>> + if t == 'BlockDriverState *':
>> + bs = 'bs'
>> + elif t == 'BdrvChild *':
>> + bs = 'child->bs'
>> + else:
>> + bs = 'blk_bs(blk)'
>> + self.bs = bs
>> def gen_list(self, format: str) -> str:
>> return ', '.join(format.format_map(arg.__dict__) for arg in
>> self.args)
>> @@ -74,8 +92,9 @@ def gen_block(self, format: str) -> str:
>> return '\n'.join(format.format_map(arg.__dict__) for arg in
>> self.args)
>> -# Match wrappers declared with a co_wrapper_mixed mark
>> -func_decl_re = re.compile(r'^int\s*co_wrapper_mixed\s*'
>> +# Match wrappers declared with a co_wrapper mark
>> +func_decl_re = re.compile(r'^int\s*co_wrapper'
>> + r'(?P<variant>(_[a-z][a-z0-9_]*)?)\s*'
>
> Why you allow everything here?
> I'd write just
>
> (?P<mixed>(_mixed)?)
>
> or
>
> (?P<marker>co_wrapper(_mixed)?)
Ok you couldn't possibly know that, but we are also adding other type of
"variants":
co_wrapper
co_wrapper_mixed
co_wrapper_bdrv_rdlock
co_wrapper_mixed_bdrv_rdlock
Therefore I need to keep variant : str and the regex as it is, but maybe
get rid of the if else condition. I'll change the docstring of course.
If you want to know more, see the thread in
"[PATCH 00/15] Protect the block layer with a rwlock: part 3"
Thank you,
Emanuele
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v6 11/14] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter
2022-11-25 13:35 [PATCH v6 00/14] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (9 preceding siblings ...)
2022-11-25 13:35 ` [PATCH v6 10/14] block-coroutine-wrapper.py: introduce co_wrapper Emanuele Giuseppe Esposito
@ 2022-11-25 13:35 ` Emanuele Giuseppe Esposito
2022-11-25 20:41 ` Vladimir Sementsov-Ogievskiy
2022-11-25 13:35 ` [PATCH v6 12/14] block-coroutine-wrapper.py: support also basic return types Emanuele Giuseppe Esposito
` (2 subsequent siblings)
13 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-25 13:35 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow,
Vladimir Sementsov-Ogievskiy, Eric Blake, Fam Zheng,
Stefan Hajnoczi, Denis V. Lunev, Stefan Weil, Jeff Cody,
Cleber Rosa, qemu-devel, Emanuele Giuseppe Esposito
Right now, we take the first parameter of the function to get the
BlockDriverState to pass to bdrv_poll_co(), that internally calls
functions that figure in which aiocontext the coroutine should run.
However, it is useless to pass a bs just to get its own AioContext,
so instead pass it directly, and default to the main loop if no
BlockDriverState is passed as parameter.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
block/block-gen.h | 6 +++---
scripts/block-coroutine-wrapper.py | 16 ++++++++--------
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/block/block-gen.h b/block/block-gen.h
index f80cf4897d..08d977f493 100644
--- a/block/block-gen.h
+++ b/block/block-gen.h
@@ -30,7 +30,7 @@
/* Base structure for argument packing structures */
typedef struct BdrvPollCo {
- BlockDriverState *bs;
+ AioContext *ctx;
bool in_progress;
int ret;
Coroutine *co; /* Keep pointer here for debugging */
@@ -40,8 +40,8 @@ static inline int bdrv_poll_co(BdrvPollCo *s)
{
assert(!qemu_in_coroutine());
- bdrv_coroutine_enter(s->bs, s->co);
- BDRV_POLL_WHILE(s->bs, s->in_progress);
+ aio_co_enter(s->ctx, s->co);
+ AIO_WAIT_WHILE(s->ctx, s->in_progress);
return s->ret;
}
diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
index 7972d3fe01..61a718ea2f 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -78,12 +78,14 @@ def __init__(self, return_type: str, name: str, args: str,
t = self.args[0].type
if t == 'BlockDriverState *':
- bs = 'bs'
+ ctx = 'bdrv_get_aio_context(bs)'
elif t == 'BdrvChild *':
- bs = 'child->bs'
+ ctx = 'bdrv_get_aio_context(child->bs)'
+ elif t == 'BlockBackend *':
+ ctx = 'blk_get_aio_context(blk)'
else:
- bs = 'blk_bs(blk)'
- self.bs = bs
+ ctx = 'qemu_get_aio_context()'
+ self.ctx = ctx
def gen_list(self, format: str) -> str:
return ', '.join(format.format_map(arg.__dict__) for arg in self.args)
@@ -128,7 +130,7 @@ def create_g_c_w(func: FuncDecl) -> str:
return {name}({ func.gen_list('{name}') });
}} else {{
{struct_name} s = {{
- .poll_state.bs = {func.bs},
+ .poll_state.ctx = {func.ctx},
.poll_state.in_progress = true,
{ func.gen_block(' .{name} = {name},') }
@@ -149,7 +151,7 @@ def create_coroutine_only(func: FuncDecl) -> str:
int {func.name}({ func.gen_list('{decl}') })
{{
{struct_name} s = {{
- .poll_state.bs = {func.bs},
+ .poll_state.ctx = {func.ctx},
.poll_state.in_progress = true,
{ func.gen_block(' .{name} = {name},') }
@@ -165,8 +167,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 *']
name = func.co_name
struct_name = func.struct_name
--
2.31.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v6 11/14] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter
2022-11-25 13:35 ` [PATCH v6 11/14] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter Emanuele Giuseppe Esposito
@ 2022-11-25 20:41 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-11-25 20:41 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito, qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow, Eric Blake,
Fam Zheng, Stefan Hajnoczi, Denis V. Lunev, Stefan Weil,
Jeff Cody, Cleber Rosa, qemu-devel
too long subject.
suggest:
block-coroutine-wrapper.py: support functions without bs arg
On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
> Right now, we take the first parameter of the function to get the
> BlockDriverState to pass to bdrv_poll_co(), that internally calls
> functions that figure in which aiocontext the coroutine should run.
>
> However, it is useless to pass a bs just to get its own AioContext,
> so instead pass it directly, and default to the main loop if no
> BlockDriverState is passed as parameter.
Great!
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v6 12/14] block-coroutine-wrapper.py: support also basic return types
2022-11-25 13:35 [PATCH v6 00/14] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (10 preceding siblings ...)
2022-11-25 13:35 ` [PATCH v6 11/14] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter Emanuele Giuseppe Esposito
@ 2022-11-25 13:35 ` Emanuele Giuseppe Esposito
2022-11-25 20:52 ` Vladimir Sementsov-Ogievskiy
2022-11-25 13:35 ` [PATCH v6 13/14] block: convert bdrv_create to co_wrapper Emanuele Giuseppe Esposito
2022-11-25 13:35 ` [PATCH v6 14/14] block/dirty-bitmap: convert coroutine-only functions " Emanuele Giuseppe Esposito
13 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-25 13:35 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow,
Vladimir Sementsov-Ogievskiy, Eric Blake, Fam Zheng,
Stefan Hajnoczi, Denis V. Lunev, Stefan Weil, Jeff Cody,
Cleber Rosa, 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>
Reviewed-by: Kevin Wolf <kwolf@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 08d977f493..89b7daaa1f 100644
--- a/block/block-gen.h
+++ b/block/block-gen.h
@@ -32,18 +32,15 @@
typedef struct BdrvPollCo {
AioContext *ctx;
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());
aio_co_enter(s->ctx, s->co);
AIO_WAIT_WHILE(s->ctx, 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 61a718ea2f..9d5d70a7dd 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -95,7 +95,8 @@ def gen_block(self, format: str) -> str:
# Match wrappers declared with a co_wrapper mark
-func_decl_re = re.compile(r'^int\s*co_wrapper'
+func_decl_re = re.compile(r'^(?P<return_type>[a-zA-Z][a-zA-Z0-9_]* [*]?)'
+ r'\s*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)
@@ -103,7 +104,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'))
@@ -124,7 +125,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}') });
@@ -138,7 +139,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;
}}
}}"""
@@ -148,7 +150,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}') })
{{
{struct_name} s = {{
.poll_state.ctx = {func.ctx},
@@ -160,13 +162,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'
name = func.co_name
struct_name = func.struct_name
@@ -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* Re: [PATCH v6 12/14] block-coroutine-wrapper.py: support also basic return types
2022-11-25 13:35 ` [PATCH v6 12/14] block-coroutine-wrapper.py: support also basic return types Emanuele Giuseppe Esposito
@ 2022-11-25 20:52 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-11-25 20:52 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito, qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow, Eric Blake,
Fam Zheng, Stefan Hajnoczi, Denis V. Lunev, Stefan Weil,
Jeff Cody, Cleber Rosa, qemu-devel
On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
> 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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> 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 08d977f493..89b7daaa1f 100644
> --- a/block/block-gen.h
> +++ b/block/block-gen.h
[..]
>
> # Match wrappers declared with a co_wrapper mark
> -func_decl_re = re.compile(r'^int\s*co_wrapper'
> +func_decl_re = re.compile(r'^(?P<return_type>[a-zA-Z][a-zA-Z0-9_]* [*]?)'
hmm interesting. I'd just write \* to mean '*' symbol. Probably it's a tiny bit faster (and tiny bit shorter:)
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v6 13/14] block: convert bdrv_create to co_wrapper
2022-11-25 13:35 [PATCH v6 00/14] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (11 preceding siblings ...)
2022-11-25 13:35 ` [PATCH v6 12/14] block-coroutine-wrapper.py: support also basic return types Emanuele Giuseppe Esposito
@ 2022-11-25 13:35 ` Emanuele Giuseppe Esposito
2022-11-25 20:56 ` Vladimir Sementsov-Ogievskiy
2022-11-25 13:35 ` [PATCH v6 14/14] block/dirty-bitmap: convert coroutine-only functions " Emanuele Giuseppe Esposito
13 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-25 13:35 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow,
Vladimir Sementsov-Ogievskiy, Eric Blake, Fam Zheng,
Stefan Hajnoczi, Denis V. Lunev, Stefan Weil, Jeff Cody,
Cleber Rosa, 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 co_wrapper.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 40 ++----------------------------
include/block/block-global-state.h | 8 ++++--
2 files changed, 8 insertions(+), 40 deletions(-)
diff --git a/block.c b/block.c
index 772df02d54..a6465bac74 100644
--- a/block.c
+++ b/block.c
@@ -528,8 +528,8 @@ typedef struct CreateCo {
Error *err;
} CreateCo;
-static int coroutine_fn bdrv_co_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 ret;
GLOBAL_STATE_CODE();
@@ -552,42 +552,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 387a7cbb2e..1f8b54f2df 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -55,8 +55,12 @@ 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 co_wrapper bdrv_create(BlockDriver *drv, const char *filename,
+ QemuOpts *opts, Error **errp);
+
int coroutine_fn bdrv_co_create_file(const char *filename, QemuOpts *opts,
Error **errp);
--
2.31.1
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [PATCH v6 13/14] block: convert bdrv_create to co_wrapper
2022-11-25 13:35 ` [PATCH v6 13/14] block: convert bdrv_create to co_wrapper Emanuele Giuseppe Esposito
@ 2022-11-25 20:56 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-11-25 20:56 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito, qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow, Eric Blake,
Fam Zheng, Stefan Hajnoczi, Denis V. Lunev, Stefan Weil,
Jeff Cody, Cleber Rosa, qemu-devel
On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
> 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 co_wrapper.
>
> Signed-off-by: Emanuele Giuseppe Esposito<eesposit@redhat.com>
> Reviewed-by: Kevin Wolf<kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
(if you merge 07 here, keep my r-b)
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v6 14/14] block/dirty-bitmap: convert coroutine-only functions to co_wrapper
2022-11-25 13:35 [PATCH v6 00/14] Still more coroutine and various fixes in block layer Emanuele Giuseppe Esposito
` (12 preceding siblings ...)
2022-11-25 13:35 ` [PATCH v6 13/14] block: convert bdrv_create to co_wrapper Emanuele Giuseppe Esposito
@ 2022-11-25 13:35 ` Emanuele Giuseppe Esposito
2022-11-25 21:09 ` Vladimir Sementsov-Ogievskiy
13 siblings, 1 reply; 31+ messages in thread
From: Emanuele Giuseppe Esposito @ 2022-11-25 13:35 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow,
Vladimir Sementsov-Ogievskiy, Eric Blake, Fam Zheng,
Stefan Hajnoczi, Denis V. Lunev, Stefan Weil, Jeff Cody,
Cleber Rosa, 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
c_w.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block/dirty-bitmap.c | 88 +-----------------------------------
block/meson.build | 1 +
include/block/block-common.h | 5 +-
include/block/block-io.h | 10 +++-
include/block/dirty-bitmap.h | 10 +++-
5 files changed, 22 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 847e4d4626..6cf603ab06 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"
@@ -51,6 +49,9 @@
#define co_wrapper
#define co_wrapper_mixed
+#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 72cf45975b..52869ea08e 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -215,8 +215,14 @@ 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 co_wrapper 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..c3700cec76 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 co_wrapper 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 v6 14/14] block/dirty-bitmap: convert coroutine-only functions to co_wrapper
2022-11-25 13:35 ` [PATCH v6 14/14] block/dirty-bitmap: convert coroutine-only functions " Emanuele Giuseppe Esposito
@ 2022-11-25 21:09 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2022-11-25 21:09 UTC (permalink / raw)
To: Emanuele Giuseppe Esposito, qemu-block
Cc: Kevin Wolf, Paolo Bonzini, Hanna Reitz, John Snow, Eric Blake,
Fam Zheng, Stefan Hajnoczi, Denis V. Lunev, Stefan Weil,
Jeff Cody, Cleber Rosa, qemu-devel
On 11/25/22 16:35, Emanuele Giuseppe Esposito wrote:
> 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
> c_w.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> block/dirty-bitmap.c | 88 +-----------------------------------
> block/meson.build | 1 +
> include/block/block-common.h | 5 +-
> include/block/block-io.h | 10 +++-
> include/block/dirty-bitmap.h | 10 +++-
> 5 files changed, 22 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
Please do
git config diff.orderFile scripts/git.orderfile
so that .h files appear first.
> @@ -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)
> {
[..]
> --- 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"
>
> @@ -51,6 +49,9 @@
> #define co_wrapper
> #define co_wrapper_mixed
>
> +#include "block/dirty-bitmap.h"
> +#include "block/blockjob.h"
> +
that's not good.. we have unclean dependency on include order and these wrapper definitions.
Actually that means that we want separate header file for co_wrapper and co_wrapper_mixed.. But OK, let's keep it as is for now, not hard to fix it later if really needed.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread