[PATCH v4 00/11] Still more coroutine and various fixes in block layer

[PATCH v4 00/11] Still more coroutine and various fixes in block layer

[Emanuele Giuseppe Esposito]

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

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

This serie is based on Kevin Wolf's series "block: Simplify drain".

Based-on: <20221108123738.530873-1-kwolf@redhat.com>

Emanuele
---
v4:
* use v2 commit messages
* introduce generated_co_wrapper_simple to simplify patches

v3:
* Remove patch 1, base on kevin "drain semplification serie"

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

Emanuele Giuseppe Esposito (11):
  block-copy: add missing coroutine_fn annotations
  nbd/server.c: add missing coroutine_fn annotations
  block-backend: replace bdrv_*_above with blk_*_above
  block-coroutine-wrapper.py: introduce generated_co_wrapper_simple
  block-coroutine-wrapper.py: default to main loop aiocontext if
    function does not have a BlockDriverState parameter
  block-coroutine-wrapper.py: support also basic return types
  block/vmdk: add missing coroutine_fn annotations
  block: distinguish between bdrv_create running in coroutine and not
  block: bdrv_create_file is a coroutine_fn
  block: convert bdrv_create to generated_co_wrapper_simple
  block/dirty-bitmap: convert coroutine-only functions to
    generated_co_wrapper_simple

 block.c                            |  68 +++++--------------
 block/block-backend.c              |  21 ++++++
 block/block-copy.c                 |  15 +++--
 block/block-gen.h                  |   5 +-
 block/commit.c                     |   4 +-
 block/dirty-bitmap.c               |  88 +------------------------
 block/meson.build                  |   1 +
 block/vmdk.c                       |  36 +++++-----
 include/block/block-common.h       |   6 +-
 include/block/block-global-state.h |  13 +++-
 include/block/block-io.h           |   9 ++-
 include/block/dirty-bitmap.h       |  10 ++-
 include/sysemu/block-backend-io.h  |   9 +++
 nbd/server.c                       |  43 ++++++------
 qemu-img.c                         |   4 +-
 scripts/block-coroutine-wrapper.py | 102 +++++++++++++++++++++--------
 16 files changed, 209 insertions(+), 225 deletions(-)

-- 
2.31.1

[PATCH v4 01/11] block-copy: add missing coroutine_fn annotations

[Emanuele Giuseppe Esposito]

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

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

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

Re: [PATCH v4 01/11] block-copy: add missing coroutine_fn annotations

[Kevin Wolf]

Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> These functions end up calling bdrv_common_block_status_above(), a
> generated_co_wrapper function.
> In addition, they also happen to be always called in coroutine context,
> meaning all callers are coroutine_fn.
> This means that the g_c_w function will enter the qemu_in_coroutine()
> case and eventually suspend (or in other words call qemu_coroutine_yield()).
> Therefore we need to mark such functions coroutine_fn too.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Ideally, we'd convert them to new wrappers bdrv_co_is_allocated() and
bdrv_co_block_status_above() instead of having to argue that they always
take the coroutine path in g_c_w.

> diff --git a/block/block-copy.c b/block/block-copy.c
> index bb947afdda..f33ab1d0b6 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -669,8 +671,9 @@ void block_copy_reset(BlockCopyState *s, int64_t offset, int64_t bytes)
>   * @return 0 when the cluster at @offset was unallocated,
>   *         1 otherwise, and -ret on error.
>   */
> -int64_t block_copy_reset_unallocated(BlockCopyState *s,
> -                                     int64_t offset, int64_t *count)
> +int64_t coroutine_fn block_copy_reset_unallocated(BlockCopyState *s,
> +                                                  int64_t offset,
> +                                                  int64_t *count)
>  {
>      int ret;
>      int64_t clusters, bytes;

This one is a public function. Its prototype in block-copy.h should be
updated, too.

Kevin

Re: [PATCH v4 01/11] block-copy: add missing coroutine_fn annotations

[Emanuele Giuseppe Esposito]

Am 18/11/2022 um 20:05 schrieb Kevin Wolf:
> Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
>> These functions end up calling bdrv_common_block_status_above(), a
>> generated_co_wrapper function.
>> In addition, they also happen to be always called in coroutine context,
>> meaning all callers are coroutine_fn.
>> This means that the g_c_w function will enter the qemu_in_coroutine()
>> case and eventually suspend (or in other words call qemu_coroutine_yield()).
>> Therefore we need to mark such functions coroutine_fn too.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> Ideally, we'd convert them to new wrappers bdrv_co_is_allocated() and
> bdrv_co_block_status_above() instead of having to argue that they always
> take the coroutine path in g_c_w.
> 

Ok so basically I should introduce bdrv_co_is_allocated, because so far
in this and next series I never thought about creating it.
Since these functions will be eventually split anyways, I agree let's
start doing this now.

Thank you,
Emanuele

Re: [PATCH v4 01/11] block-copy: add missing coroutine_fn annotations

[Emanuele Giuseppe Esposito]

Am 21/11/2022 um 09:32 schrieb Emanuele Giuseppe Esposito:
> 
> 
> Am 18/11/2022 um 20:05 schrieb Kevin Wolf:
>> Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
>>> These functions end up calling bdrv_common_block_status_above(), a
>>> generated_co_wrapper function.
>>> In addition, they also happen to be always called in coroutine context,
>>> meaning all callers are coroutine_fn.
>>> This means that the g_c_w function will enter the qemu_in_coroutine()
>>> case and eventually suspend (or in other words call qemu_coroutine_yield()).
>>> Therefore we need to mark such functions coroutine_fn too.
>>>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>
>> Ideally, we'd convert them to new wrappers bdrv_co_is_allocated() and
>> bdrv_co_block_status_above() instead of having to argue that they always
>> take the coroutine path in g_c_w.
>>
> 
> Ok so basically I should introduce bdrv_co_is_allocated, because so far
> in this and next series I never thought about creating it.
> Since these functions will be eventually split anyways, I agree let's
> start doing this now.

Actually bdrv_is_allocated would be a g_c_w functions in itself, that
calls another g_c_w and it is probably called by functions that are or
will be g_c_w.
Is this actually the scope of this series? I think switching this
specific function and its callers or similar will require a lot of
efforts, and if I do it here it won't cover all the cases for sure.

Wouldn't it be better to do these kind of things in a different serie
using Paolo's vrc tool?

> Thank you,
> Emanuele
> 

Re: [PATCH v4 01/11] block-copy: add missing coroutine_fn annotations

[Kevin Wolf]

Am 21.11.2022 um 09:51 hat Emanuele Giuseppe Esposito geschrieben:
> 
> 
> Am 21/11/2022 um 09:32 schrieb Emanuele Giuseppe Esposito:
> > 
> > 
> > Am 18/11/2022 um 20:05 schrieb Kevin Wolf:
> >> Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> >>> These functions end up calling bdrv_common_block_status_above(), a
> >>> generated_co_wrapper function.
> >>> In addition, they also happen to be always called in coroutine context,
> >>> meaning all callers are coroutine_fn.
> >>> This means that the g_c_w function will enter the qemu_in_coroutine()
> >>> case and eventually suspend (or in other words call qemu_coroutine_yield()).
> >>> Therefore we need to mark such functions coroutine_fn too.
> >>>
> >>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> >>
> >> Ideally, we'd convert them to new wrappers bdrv_co_is_allocated() and
> >> bdrv_co_block_status_above() instead of having to argue that they always
> >> take the coroutine path in g_c_w.
> > 
> > Ok so basically I should introduce bdrv_co_is_allocated, because so far
> > in this and next series I never thought about creating it.
> > Since these functions will be eventually split anyways, I agree let's
> > start doing this now.
> 
> Actually bdrv_is_allocated would be a g_c_w functions in itself, that
> calls another g_c_w and it is probably called by functions that are or
> will be g_c_w.

I'm not sure if I understand. bdrv_is_allocated() is essentially a g_c_w
function today, just indirectly. But we have callers that know that they
are running in a coroutine (which is why you're adding coroutine_fn to
them), so they shouldn't call a g_c_w function, but directly the
coroutine version of the function.

The only reason why you can't currently do that is that
bdrv_is_allocated() exists as a wrapper around the g_c_w function
bdrv_common_block_status_above(), but the same wrapper doesn't exist for
the pure coroutine version bdrv_co_common_block_status_above().

All I'm suggesting is introducing a bdrv_co_is_allocated() that is a
wrapper directly around bdrv_co_common_block_status_above(), so that
the functions you're marking as coroutine_fn can use it instead of
calling g_c_w. This should be about 10 lines of code.

I'm not implying that you should convert any other callers in this
patch, or that you should touch bdrv_is_allocated() at all.

> Is this actually the scope of this series? I think switching this
> specific function and its callers or similar will require a lot of
> efforts, and if I do it here it won't cover all the cases for sure.
> 
> Wouldn't it be better to do these kind of things in a different serie
> using Paolo's vrc tool?

I'm not sure what the scope of this series is, because you already do
introduce new wrappers in other patches of the series. I assumed it's
just to improve the situation a little, with no claim of being
exhaustive.

Finding and fully converting all callers might indeed be a job for
something like vrc, but here I'm only looking at local consistency in
functions where you're adding coroutine_fn.

Kevin

Re: [PATCH v4 01/11] block-copy: add missing coroutine_fn annotations

[Emanuele Giuseppe Esposito]

Am 21/11/2022 um 12:50 schrieb Kevin Wolf:
> Am 21.11.2022 um 09:51 hat Emanuele Giuseppe Esposito geschrieben:
>>
>>
>> Am 21/11/2022 um 09:32 schrieb Emanuele Giuseppe Esposito:
>>>
>>>
>>> Am 18/11/2022 um 20:05 schrieb Kevin Wolf:
>>>> Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
>>>>> These functions end up calling bdrv_common_block_status_above(), a
>>>>> generated_co_wrapper function.
>>>>> In addition, they also happen to be always called in coroutine context,
>>>>> meaning all callers are coroutine_fn.
>>>>> This means that the g_c_w function will enter the qemu_in_coroutine()
>>>>> case and eventually suspend (or in other words call qemu_coroutine_yield()).
>>>>> Therefore we need to mark such functions coroutine_fn too.
>>>>>
>>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>>
>>>> Ideally, we'd convert them to new wrappers bdrv_co_is_allocated() and
>>>> bdrv_co_block_status_above() instead of having to argue that they always
>>>> take the coroutine path in g_c_w.
>>>
>>> Ok so basically I should introduce bdrv_co_is_allocated, because so far
>>> in this and next series I never thought about creating it.
>>> Since these functions will be eventually split anyways, I agree let's
>>> start doing this now.
>>
>> Actually bdrv_is_allocated would be a g_c_w functions in itself, that
>> calls another g_c_w and it is probably called by functions that are or
>> will be g_c_w.
> 
> I'm not sure if I understand. bdrv_is_allocated() is essentially a g_c_w
> function today, just indirectly. But we have callers that know that they
> are running in a coroutine (which is why you're adding coroutine_fn to
> them), so they shouldn't call a g_c_w function, but directly the
> coroutine version of the function.
> 
> The only reason why you can't currently do that is that
> bdrv_is_allocated() exists as a wrapper around the g_c_w function
> bdrv_common_block_status_above(), but the same wrapper doesn't exist for
> the pure coroutine version bdrv_co_common_block_status_above().
> 
> All I'm suggesting is introducing a bdrv_co_is_allocated() that is a
> wrapper directly around bdrv_co_common_block_status_above(), so that
> the functions you're marking as coroutine_fn can use it instead of
> calling g_c_w. This should be about 10 lines of code.
> 
> I'm not implying that you should convert any other callers in this
> patch, or that you should touch bdrv_is_allocated() at all.
> 
>> Is this actually the scope of this series? I think switching this
>> specific function and its callers or similar will require a lot of
>> efforts, and if I do it here it won't cover all the cases for sure.
>>
>> Wouldn't it be better to do these kind of things in a different serie
>> using Paolo's vrc tool?
> 
> I'm not sure what the scope of this series is, because you already do
> introduce new wrappers in other patches of the series. I assumed it's
> just to improve the situation a little, with no claim of being
> exhaustive.
> 
> Finding and fully converting all callers might indeed be a job for
> something like vrc, but here I'm only looking at local consistency in
> functions where you're adding coroutine_fn.
> 

Oh ok now I see what you mean. I was thinking (and did in "[PATCH 04/15]
block: convert bdrv_refresh_total_sectors in generated_co_wrapper") to
instead convert all callers in g_c_w, and that ended up being a big pain.

I'll also correct the patch I mentioned above.

Thank you,
Emanuele

[PATCH v4 02/11] nbd/server.c: add missing coroutine_fn annotations

[Emanuele Giuseppe Esposito]

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

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

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

Re: [PATCH v4 02/11] nbd/server.c: add missing coroutine_fn annotations

[Kevin Wolf]

Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> These functions end up calling bdrv_*() implemented as generated_co_wrapper
> functions.
> In addition, they also happen to be always called in coroutine context,
> meaning all callers are coroutine_fn.
> This means that the g_c_w function will enter the qemu_in_coroutine()
> case and eventually suspend (or in other words call qemu_coroutine_yield()).
> Therefore we need to mark such functions coroutine_fn too.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Here, too, it would be better to convert the callers to new blk_co_*()
wrappers instead of going through g_c_w even though we know that we are
in coroutine context.

Doesn't make the patch less correct, though.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH v4 03/11] block-backend: replace bdrv_*_above with blk_*_above

[Emanuele Giuseppe Esposito]

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

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

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

Re: [PATCH v4 03/11] block-backend: replace bdrv_*_above with blk_*_above

[Kevin Wolf]

Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> Avoid mixing bdrv_* functions with blk_*, so create blk_* counterparts
> for:
> - bdrv_block_status_above
> - bdrv_is_allocated_above
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  block/block-backend.c             | 21 +++++++++++++++++++++
>  block/commit.c                    |  4 ++--
>  include/sysemu/block-backend-io.h |  9 +++++++++
>  nbd/server.c                      | 28 ++++++++++++++--------------
>  qemu-img.c                        |  4 ++--
>  5 files changed, 48 insertions(+), 18 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 742efa7955..333d50fb3f 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1424,6 +1424,27 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
>      return blk_co_pwritev_part(blk, offset, bytes, qiov, 0, flags);
>  }
>  
> +int coroutine_fn blk_block_status_above(BlockBackend *blk,

Let's call it blk_co_block_status_above() to stay consistent with other
functions.

> +                                        BlockDriverState *base,
> +                                        int64_t offset, int64_t bytes,
> +                                        int64_t *pnum, int64_t *map,
> +                                        BlockDriverState **file)
> +{
> +    IO_CODE();
> +    return bdrv_block_status_above(blk_bs(blk), base, offset, bytes, pnum, map,
> +                                   file);

coroutine_fn calling into a g_c_w. As mentioned in the first patch, we
really want a bdrv_co_block_status_above() than can be called without
the g_c_w wrapper.

> +}
> +
> +int coroutine_fn blk_is_allocated_above(BlockBackend *blk,

blk_co_is_allocated_above()

> +                                        BlockDriverState *base,
> +                                        bool include_base, int64_t offset,
> +                                        int64_t bytes, int64_t *pnum)
> +{
> +    IO_CODE();
> +    return bdrv_is_allocated_above(blk_bs(blk), base, include_base, offset,
> +                                   bytes, pnum);

Again a g_c_w.

> +}
> +
>  typedef struct BlkRwCo {
>      BlockBackend *blk;
>      int64_t offset;
> diff --git a/block/commit.c b/block/commit.c
> index 0029b31944..9d4d908344 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -155,8 +155,8 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
>              break;
>          }
>          /* Copy if allocated above the base */
> -        ret = bdrv_is_allocated_above(blk_bs(s->top), s->base_overlay, true,
> -                                      offset, COMMIT_BUFFER_SIZE, &n);
> +        ret = blk_is_allocated_above(s->top, s->base_overlay, true,
> +                                     offset, COMMIT_BUFFER_SIZE, &n);
>          copy = (ret > 0);
>          trace_commit_one_iteration(s, offset, n, ret);
>          if (copy) {
> diff --git a/include/sysemu/block-backend-io.h b/include/sysemu/block-backend-io.h
> index 50f5aa2e07..a47cb825e5 100644
> --- a/include/sysemu/block-backend-io.h
> +++ b/include/sysemu/block-backend-io.h
> @@ -92,6 +92,15 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
>                                     int64_t bytes, BdrvRequestFlags read_flags,
>                                     BdrvRequestFlags write_flags);
>  
> +int coroutine_fn blk_block_status_above(BlockBackend *blk,
> +                                        BlockDriverState *base,
> +                                        int64_t offset, int64_t bytes,
> +                                        int64_t *pnum, int64_t *map,
> +                                        BlockDriverState **file);
> +int coroutine_fn blk_is_allocated_above(BlockBackend *blk,
> +                                        BlockDriverState *base,
> +                                        bool include_base, int64_t offset,
> +                                        int64_t bytes, int64_t *pnum);
>  
>  /*
>   * "I/O or GS" API functions. These functions can run without
> diff --git a/nbd/server.c b/nbd/server.c
> index e2eec0cf46..6389b46396 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1991,7 +1991,7 @@ static int coroutine_fn nbd_co_send_structured_error(NBDClient *client,
>  }
>  
>  /* Do a sparse read and send the structured reply to the client.
> - * Returns -errno if sending fails. bdrv_block_status_above() failure is
> + * Returns -errno if sending fails. blk_block_status_above() failure is
>   * reported to the client, at which point this function succeeds.
>   */
>  static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
> @@ -2007,10 +2007,10 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
>  
>      while (progress < size) {
>          int64_t pnum;
> -        int status = bdrv_block_status_above(blk_bs(exp->common.blk), NULL,
> -                                             offset + progress,
> -                                             size - progress, &pnum, NULL,
> -                                             NULL);
> +        int status = blk_block_status_above(exp->common.blk, NULL,
> +                                            offset + progress,
> +                                            size - progress, &pnum, NULL,
> +                                            NULL);
>          bool final;
>  
>          if (status < 0) {
> @@ -2141,14 +2141,14 @@ static int nbd_extent_array_add(NBDExtentArray *ea,
>      return 0;
>  }
>  
> -static int coroutine_fn blockstatus_to_extents(BlockDriverState *bs,
> +static int coroutine_fn blockstatus_to_extents(BlockBackend *blk,
>                                                 uint64_t offset, uint64_t bytes,
>                                                 NBDExtentArray *ea)
>  {
>      while (bytes) {
>          uint32_t flags;
>          int64_t num;
> -        int ret = bdrv_block_status_above(bs, NULL, offset, bytes, &num,
> +        int ret = blk_block_status_above(blk, NULL, offset, bytes, &num,
>                                            NULL, NULL);

Indentation is off now.

>  
>          if (ret < 0) {
> @@ -2169,13 +2169,13 @@ static int coroutine_fn blockstatus_to_extents(BlockDriverState *bs,
>      return 0;
>  }
>  
> -static int coroutine_fn blockalloc_to_extents(BlockDriverState *bs,
> +static int coroutine_fn blockalloc_to_extents(BlockBackend *blk,
>                                                uint64_t offset, uint64_t bytes,
>                                                NBDExtentArray *ea)
>  {
>      while (bytes) {
>          int64_t num;
> -        int ret = bdrv_is_allocated_above(bs, NULL, false, offset, bytes,
> +        int ret = blk_is_allocated_above(blk, NULL, false, offset, bytes,
>                                            &num);

Here, too.

>          if (ret < 0) {
> @@ -2224,7 +2224,7 @@ static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
>  /* Get block status from the exported device and send it to the client */
>  static int
>  coroutine_fn nbd_co_send_block_status(NBDClient *client, uint64_t handle,
> -                                      BlockDriverState *bs, uint64_t offset,
> +                                      BlockBackend *blk, uint64_t offset,
>                                        uint32_t length, bool dont_fragment,
>                                        bool last, uint32_t context_id,
>                                        Error **errp)
> @@ -2234,9 +2234,9 @@ coroutine_fn nbd_co_send_block_status(NBDClient *client, uint64_t handle,
>      g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents);
>  
>      if (context_id == NBD_META_ID_BASE_ALLOCATION) {
> -        ret = blockstatus_to_extents(bs, offset, length, ea);
> +        ret = blockstatus_to_extents(blk, offset, length, ea);
>      } else {
> -        ret = blockalloc_to_extents(bs, offset, length, ea);
> +        ret = blockalloc_to_extents(blk, offset, length, ea);
>      }
>      if (ret < 0) {
>          return nbd_co_send_structured_error(
> @@ -2563,7 +2563,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
>  
>              if (client->export_meta.base_allocation) {
>                  ret = nbd_co_send_block_status(client, request->handle,
> -                                               blk_bs(exp->common.blk),
> +                                               exp->common.blk,
>                                                 request->from,
>                                                 request->len, dont_fragment,
>                                                 !--contexts_remaining,
> @@ -2576,7 +2576,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
>  
>              if (client->export_meta.allocation_depth) {
>                  ret = nbd_co_send_block_status(client, request->handle,
> -                                               blk_bs(exp->common.blk),
> +                                               exp->common.blk,
>                                                 request->from, request->len,
>                                                 dont_fragment,
>                                                 !--contexts_remaining,
> diff --git a/qemu-img.c b/qemu-img.c
> index a3b64c88af..4282a34bc0 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1730,8 +1730,8 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
>          do {
>              count = n * BDRV_SECTOR_SIZE;
>  
> -            ret = bdrv_block_status_above(src_bs, base, offset, count, &count,
> -                                          NULL, NULL);
> +            ret = bdrv_block_status_above(src_bs, base, offset, count,
> +                                          &count, NULL, NULL);

This one looks odd. Did you intend to change more than the line wrapping
here?

Kevin

[PATCH v4 04/11] block-coroutine-wrapper.py: introduce generated_co_wrapper_simple

[Emanuele Giuseppe Esposito]

This new annotation creates just a function wrapper that creates
a new coroutine. It assumes the caller is not a coroutine.

This is much better as g_c_w, because it is clear if the caller
is a coroutine or not, and provides the advantage of automating
the code creation. In the future all g_c_w functions will be
substituted on g_c_w_simple.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 include/block/block-common.h       |  1 +
 scripts/block-coroutine-wrapper.py | 87 ++++++++++++++++++++++--------
 2 files changed, 66 insertions(+), 22 deletions(-)

diff --git a/include/block/block-common.h b/include/block/block-common.h
index 297704c1e9..8ae750c7cf 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -43,6 +43,7 @@
  * Read more in docs/devel/block-coroutine-wrapper.rst
  */
 #define generated_co_wrapper
+#define generated_co_wrapper_simple
 
 /* block.c */
 typedef struct BlockDriver BlockDriver;
diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
index 08be813407..f88ef53964 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -62,10 +62,15 @@ def __init__(self, param_decl: str) -> None:
 
 
 class FuncDecl:
-    def __init__(self, return_type: str, name: str, args: str) -> None:
+    def __init__(self, return_type: str, name: str, args: str,
+                 variant: str) -> None:
         self.return_type = return_type.strip()
         self.name = name.strip()
         self.args = [ParamDecl(arg.strip()) for arg in args.split(',')]
+        self.create_only_co = False
+
+        if variant == '_simple':
+            self.create_only_co = True
 
     def gen_list(self, format: str) -> str:
         return ', '.join(format.format_map(arg.__dict__) for arg in self.args)
@@ -75,7 +80,8 @@ def gen_block(self, format: str) -> str:
 
 
 # Match wrappers declared with a generated_co_wrapper mark
-func_decl_re = re.compile(r'^int\s*generated_co_wrapper\s*'
+func_decl_re = re.compile(r'^int\s*generated_co_wrapper'
+                          r'(?P<variant>(_[a-z][a-z0-9_]*)?)\s*'
                           r'(?P<wrapper_name>[a-z][a-z0-9_]*)'
                           r'\((?P<args>[^)]*)\);$', re.MULTILINE)
 
@@ -84,7 +90,8 @@ def func_decl_iter(text: str) -> Iterator:
     for m in func_decl_re.finditer(text):
         yield FuncDecl(return_type='int',
                        name=m.group('wrapper_name'),
-                       args=m.group('args'))
+                       args=m.group('args'),
+                       variant=m.group('variant'))
 
 
 def snake_to_camel(func_name: str) -> str:
@@ -97,6 +104,51 @@ def snake_to_camel(func_name: str) -> str:
     return ''.join(words)
 
 
+# Checks if we are already in coroutine
+def create_g_c_w(func: FuncDecl) -> str:
+    name = func.co_name
+    struct_name = func.struct_name
+    return f"""\
+int {func.name}({ func.gen_list('{decl}') })
+{{
+    if (qemu_in_coroutine()) {{
+        return {name}({ func.gen_list('{name}') });
+    }} else {{
+        {struct_name} s = {{
+            .poll_state.bs = {func.bs},
+            .poll_state.in_progress = true,
+
+{ func.gen_block('            .{name} = {name},') }
+        }};
+
+        s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
+
+        return bdrv_poll_co(&s.poll_state);
+    }}
+}}"""
+
+
+# Assumes we are not in coroutine, and creates one
+def create_coroutine_only(func: FuncDecl) -> str:
+    name = func.co_name
+    struct_name = func.struct_name
+    return f"""\
+int {func.name}({ func.gen_list('{decl}') })
+{{
+    assert(!qemu_in_coroutine());
+    {struct_name} s = {{
+        .poll_state.bs = {func.bs},
+        .poll_state.in_progress = true,
+
+{ func.gen_block('        .{name} = {name},') }
+    }};
+
+    s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
+
+    return bdrv_poll_co(&s.poll_state);
+}}"""
+
+
 def gen_wrapper(func: FuncDecl) -> str:
     assert not '_co_' in func.name
     assert func.return_type == 'int'
@@ -105,7 +157,8 @@ def gen_wrapper(func: FuncDecl) -> str:
 
     subsystem, subname = func.name.split('_', 1)
 
-    name = f'{subsystem}_co_{subname}'
+    func.co_name = f'{subsystem}_co_{subname}'
+    name = func.co_name
 
     t = func.args[0].type
     if t == 'BlockDriverState *':
@@ -114,7 +167,13 @@ def gen_wrapper(func: FuncDecl) -> str:
         bs = 'child->bs'
     else:
         bs = 'blk_bs(blk)'
-    struct_name = snake_to_camel(name)
+    func.bs = bs
+    func.struct_name = snake_to_camel(func.name)
+    struct_name = func.struct_name
+
+    creation_function = create_g_c_w
+    if func.create_only_co:
+        creation_function = create_coroutine_only
 
     return f"""\
 /*
@@ -136,23 +195,7 @@ def gen_wrapper(func: FuncDecl) -> str:
     aio_wait_kick();
 }}
 
-int {func.name}({ func.gen_list('{decl}') })
-{{
-    if (qemu_in_coroutine()) {{
-        return {name}({ func.gen_list('{name}') });
-    }} else {{
-        {struct_name} s = {{
-            .poll_state.bs = {bs},
-            .poll_state.in_progress = true,
-
-{ func.gen_block('            .{name} = {name},') }
-        }};
-
-        s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
-
-        return bdrv_poll_co(&s.poll_state);
-    }}
-}}"""
+{creation_function(func)}"""
 
 
 def gen_wrappers(input_code: str) -> str:
-- 
2.31.1

Re: [PATCH v4 04/11] block-coroutine-wrapper.py: introduce generated_co_wrapper_simple

[Kevin Wolf]

Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> This new annotation creates just a function wrapper that creates
> a new coroutine. It assumes the caller is not a coroutine.
> 
> This is much better as g_c_w, because it is clear if the caller
> is a coroutine or not, and provides the advantage of automating
> the code creation. In the future all g_c_w functions will be
> substituted on g_c_w_simple.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

$ mypy --strict scripts/block-coroutine-wrapper.py
scripts/block-coroutine-wrapper.py:31: error: Function is missing a return type annotation
scripts/block-coroutine-wrapper.py:90: error: Missing type parameters for generic type "Iterator"
scripts/block-coroutine-wrapper.py:110: error: "FuncDecl" has no attribute "co_name"
scripts/block-coroutine-wrapper.py:111: error: "FuncDecl" has no attribute "struct_name"
scripts/block-coroutine-wrapper.py:112: error: "FuncDecl" has no attribute "bs"
scripts/block-coroutine-wrapper.py:135: error: "FuncDecl" has no attribute "co_name"
scripts/block-coroutine-wrapper.py:136: error: "FuncDecl" has no attribute "struct_name"
scripts/block-coroutine-wrapper.py:137: error: "FuncDecl" has no attribute "bs"
scripts/block-coroutine-wrapper.py:160: error: "FuncDecl" has no attribute "co_name"
scripts/block-coroutine-wrapper.py:161: error: "FuncDecl" has no attribute "co_name"
scripts/block-coroutine-wrapper.py:172: error: "FuncDecl" has no attribute "bs"
scripts/block-coroutine-wrapper.py:173: error: "FuncDecl" has no attribute "struct_name"
scripts/block-coroutine-wrapper.py:174: error: "FuncDecl" has no attribute "struct_name"
scripts/block-coroutine-wrapper.py:218: error: Call to untyped function "gen_header" in typed context
Found 14 errors in 1 file (checked 1 source file)

The first two and the last one isn't the fault of this patch, but the
others are. When you add new attributes, they should be declared in
FuncDecl.__init__(). And actually, this even seems like a better place
to initialise them already with the right value than gen_wrapper().

>  include/block/block-common.h       |  1 +
>  scripts/block-coroutine-wrapper.py | 87 ++++++++++++++++++++++--------
>  2 files changed, 66 insertions(+), 22 deletions(-)
> 
> diff --git a/include/block/block-common.h b/include/block/block-common.h
> index 297704c1e9..8ae750c7cf 100644
> --- a/include/block/block-common.h
> +++ b/include/block/block-common.h
> @@ -43,6 +43,7 @@
>   * Read more in docs/devel/block-coroutine-wrapper.rst
>   */
>  #define generated_co_wrapper
> +#define generated_co_wrapper_simple
>  
>  /* block.c */
>  typedef struct BlockDriver BlockDriver;
> diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
> index 08be813407..f88ef53964 100644
> --- a/scripts/block-coroutine-wrapper.py
> +++ b/scripts/block-coroutine-wrapper.py
> @@ -62,10 +62,15 @@ def __init__(self, param_decl: str) -> None:
>  
>  
>  class FuncDecl:
> -    def __init__(self, return_type: str, name: str, args: str) -> None:
> +    def __init__(self, return_type: str, name: str, args: str,
> +                 variant: str) -> None:
>          self.return_type = return_type.strip()
>          self.name = name.strip()
>          self.args = [ParamDecl(arg.strip()) for arg in args.split(',')]
> +        self.create_only_co = False
> +
> +        if variant == '_simple':
> +            self.create_only_co = True
>  
>      def gen_list(self, format: str) -> str:
>          return ', '.join(format.format_map(arg.__dict__) for arg in self.args)
> @@ -75,7 +80,8 @@ def gen_block(self, format: str) -> str:
>  
>  
>  # Match wrappers declared with a generated_co_wrapper mark
> -func_decl_re = re.compile(r'^int\s*generated_co_wrapper\s*'
> +func_decl_re = re.compile(r'^int\s*generated_co_wrapper'
> +                          r'(?P<variant>(_[a-z][a-z0-9_]*)?)\s*'
>                            r'(?P<wrapper_name>[a-z][a-z0-9_]*)'
>                            r'\((?P<args>[^)]*)\);$', re.MULTILINE)
>  
> @@ -84,7 +90,8 @@ def func_decl_iter(text: str) -> Iterator:
>      for m in func_decl_re.finditer(text):
>          yield FuncDecl(return_type='int',
>                         name=m.group('wrapper_name'),
> -                       args=m.group('args'))
> +                       args=m.group('args'),
> +                       variant=m.group('variant'))
>  
>  
>  def snake_to_camel(func_name: str) -> str:
> @@ -97,6 +104,51 @@ def snake_to_camel(func_name: str) -> str:
>      return ''.join(words)
>  
>  
> +# Checks if we are already in coroutine
> +def create_g_c_w(func: FuncDecl) -> str:
> +    name = func.co_name
> +    struct_name = func.struct_name
> +    return f"""\
> +int {func.name}({ func.gen_list('{decl}') })
> +{{
> +    if (qemu_in_coroutine()) {{
> +        return {name}({ func.gen_list('{name}') });
> +    }} else {{
> +        {struct_name} s = {{
> +            .poll_state.bs = {func.bs},
> +            .poll_state.in_progress = true,
> +
> +{ func.gen_block('            .{name} = {name},') }
> +        }};
> +
> +        s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
> +
> +        return bdrv_poll_co(&s.poll_state);
> +    }}
> +}}"""
> +
> +
> +# Assumes we are not in coroutine, and creates one
> +def create_coroutine_only(func: FuncDecl) -> str:
> +    name = func.co_name
> +    struct_name = func.struct_name
> +    return f"""\
> +int {func.name}({ func.gen_list('{decl}') })
> +{{
> +    assert(!qemu_in_coroutine());
> +    {struct_name} s = {{
> +        .poll_state.bs = {func.bs},
> +        .poll_state.in_progress = true,
> +
> +{ func.gen_block('        .{name} = {name},') }
> +    }};

Not sure how strict we are about this in generated code, but generally
the QEMU coding style requires declarations to come first, so the
assertion should be below it.

> +
> +    s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
> +
> +    return bdrv_poll_co(&s.poll_state);
> +}}"""

Kevin

[PATCH v4 05/11] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter

[Emanuele Giuseppe Esposito]

Basically BdrvPollCo->bs is only used by bdrv_poll_co(), and the
functions that it uses are both using bdrv_get_aio_context, that
defaults to qemu_get_aio_context() if bs is NULL.

Therefore pass NULL to BdrvPollCo to automatically generate a function
that create and runs a coroutine in the main loop.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 scripts/block-coroutine-wrapper.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
index f88ef53964..0f842386d4 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -152,8 +152,6 @@ def create_coroutine_only(func: FuncDecl) -> str:
 def gen_wrapper(func: FuncDecl) -> str:
     assert not '_co_' in func.name
     assert func.return_type == 'int'
-    assert func.args[0].type in ['BlockDriverState *', 'BdrvChild *',
-                                 'BlockBackend *']
 
     subsystem, subname = func.name.split('_', 1)
 
@@ -165,8 +163,10 @@ def gen_wrapper(func: FuncDecl) -> str:
         bs = 'bs'
     elif t == 'BdrvChild *':
         bs = 'child->bs'
-    else:
+    elif t == 'BlockBackend *':
         bs = 'blk_bs(blk)'
+    else:
+        bs = 'NULL'
     func.bs = bs
     func.struct_name = snake_to_camel(func.name)
     struct_name = func.struct_name
-- 
2.31.1

Re: [PATCH v4 05/11] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter

[Kevin Wolf]

Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> Basically BdrvPollCo->bs is only used by bdrv_poll_co(), and the
> functions that it uses are both using bdrv_get_aio_context, that
> defaults to qemu_get_aio_context() if bs is NULL.
> 
> Therefore pass NULL to BdrvPollCo to automatically generate a function
> that create and runs a coroutine in the main loop.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

It happens to work, but it's kind of ugly to call bdrv_coroutine_enter()
and BDRV_POLL_WHILE() with a NULL bs.

How hard would it be to generate code that doesn't use these functions,
but directly aio_co_enter() and AIO_WAIT_WHILE() for wrappers that are
not related to a BDS?

Kevin

Re: [PATCH v4 05/11] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter

[Emanuele Giuseppe Esposito]

Am 21/11/2022 um 16:30 schrieb Kevin Wolf:
> Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
>> Basically BdrvPollCo->bs is only used by bdrv_poll_co(), and the
>> functions that it uses are both using bdrv_get_aio_context, that
>> defaults to qemu_get_aio_context() if bs is NULL.
>>
>> Therefore pass NULL to BdrvPollCo to automatically generate a function
>> that create and runs a coroutine in the main loop.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> It happens to work, but it's kind of ugly to call bdrv_coroutine_enter()
> and BDRV_POLL_WHILE() with a NULL bs.
> 
> How hard would it be to generate code that doesn't use these functions,
> but directly aio_co_enter() and AIO_WAIT_WHILE() for wrappers that are
> not related to a BDS?
> 

At this point, I would get rid of s->poll_state.bs and instead use
s->poll_state.aio_context. Then call directly aio_co_enter and
AIO_WAIT_WHILE, as you suggested but just everywhere, without
differentiating the cases.

Then we would have something similar to what it is currently done with bs:

if t == 'BlockDriverState *':
            bs = 'bdrv_get_aio_context(bs)'
        elif t == 'BdrvChild *':
            bs = 'bdrv_get_aio_context(child->bs)'
        elif t == 'BlockBackend *':
            bs = 'bdrv_get_aio_context(blk_bs(blk))'
        else:
            bs = 'qemu_get_aio_context()'

I haven't tried it yet, but it should work.

Thank you,
Emanuele

Re: [PATCH v4 05/11] block-coroutine-wrapper.py: default to main loop aiocontext if function does not have a BlockDriverState parameter

[Kevin Wolf]

Am 21.11.2022 um 16:52 hat Emanuele Giuseppe Esposito geschrieben:
> Am 21/11/2022 um 16:30 schrieb Kevin Wolf:
> > Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> >> Basically BdrvPollCo->bs is only used by bdrv_poll_co(), and the
> >> functions that it uses are both using bdrv_get_aio_context, that
> >> defaults to qemu_get_aio_context() if bs is NULL.
> >>
> >> Therefore pass NULL to BdrvPollCo to automatically generate a function
> >> that create and runs a coroutine in the main loop.
> >>
> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > 
> > It happens to work, but it's kind of ugly to call bdrv_coroutine_enter()
> > and BDRV_POLL_WHILE() with a NULL bs.
> > 
> > How hard would it be to generate code that doesn't use these functions,
> > but directly aio_co_enter() and AIO_WAIT_WHILE() for wrappers that are
> > not related to a BDS?
> > 
> 
> At this point, I would get rid of s->poll_state.bs and instead use
> s->poll_state.aio_context. Then call directly aio_co_enter and
> AIO_WAIT_WHILE, as you suggested but just everywhere, without
> differentiating the cases.

Oh, yes, that's better.

> Then we would have something similar to what it is currently done with bs:
> 
> if t == 'BlockDriverState *':
>             bs = 'bdrv_get_aio_context(bs)'
>         elif t == 'BdrvChild *':
>             bs = 'bdrv_get_aio_context(child->bs)'
>         elif t == 'BlockBackend *':
>             bs = 'bdrv_get_aio_context(blk_bs(blk))'

blk_get_aio_context(blk) seems more correct.

>         else:
>             bs = 'qemu_get_aio_context()'
> 
> I haven't tried it yet, but it should work.

Kevin

[PATCH v4 06/11] block-coroutine-wrapper.py: support also basic return types

[Emanuele Giuseppe Esposito]

Extend the regex to cover also return type, pointers included.
This implies that the value returned by the function cannot be
a simple "int" anymore, but the custom return type.
Therefore remove poll_state->ret and instead use a per-function
custom "ret" field.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/block-gen.h                  |  5 +----
 scripts/block-coroutine-wrapper.py | 19 +++++++++++--------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/block/block-gen.h b/block/block-gen.h
index f80cf4897d..8ac9d5bd4f 100644
--- a/block/block-gen.h
+++ b/block/block-gen.h
@@ -32,18 +32,15 @@
 typedef struct BdrvPollCo {
     BlockDriverState *bs;
     bool in_progress;
-    int ret;
     Coroutine *co; /* Keep pointer here for debugging */
 } BdrvPollCo;
 
-static inline int bdrv_poll_co(BdrvPollCo *s)
+static inline void bdrv_poll_co(BdrvPollCo *s)
 {
     assert(!qemu_in_coroutine());
 
     bdrv_coroutine_enter(s->bs, s->co);
     BDRV_POLL_WHILE(s->bs, s->in_progress);
-
-    return s->ret;
 }
 
 #endif /* BLOCK_BLOCK_GEN_H */
diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py
index 0f842386d4..21ecb3e896 100644
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -80,7 +80,8 @@ def gen_block(self, format: str) -> str:
 
 
 # Match wrappers declared with a generated_co_wrapper mark
-func_decl_re = re.compile(r'^int\s*generated_co_wrapper'
+func_decl_re = re.compile(r'^(?P<return_type>[a-zA-Z][a-zA-Z0-9_]* [*]?)'
+                          r'\s*generated_co_wrapper'
                           r'(?P<variant>(_[a-z][a-z0-9_]*)?)\s*'
                           r'(?P<wrapper_name>[a-z][a-z0-9_]*)'
                           r'\((?P<args>[^)]*)\);$', re.MULTILINE)
@@ -88,7 +89,7 @@ def gen_block(self, format: str) -> str:
 
 def func_decl_iter(text: str) -> Iterator:
     for m in func_decl_re.finditer(text):
-        yield FuncDecl(return_type='int',
+        yield FuncDecl(return_type=m.group('return_type'),
                        name=m.group('wrapper_name'),
                        args=m.group('args'),
                        variant=m.group('variant'))
@@ -109,7 +110,7 @@ def create_g_c_w(func: FuncDecl) -> str:
     name = func.co_name
     struct_name = func.struct_name
     return f"""\
-int {func.name}({ func.gen_list('{decl}') })
+{func.return_type} {func.name}({ func.gen_list('{decl}') })
 {{
     if (qemu_in_coroutine()) {{
         return {name}({ func.gen_list('{name}') });
@@ -123,7 +124,8 @@ def create_g_c_w(func: FuncDecl) -> str:
 
         s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
 
-        return bdrv_poll_co(&s.poll_state);
+        bdrv_poll_co(&s.poll_state);
+        return s.ret;
     }}
 }}"""
 
@@ -133,7 +135,7 @@ def create_coroutine_only(func: FuncDecl) -> str:
     name = func.co_name
     struct_name = func.struct_name
     return f"""\
-int {func.name}({ func.gen_list('{decl}') })
+{func.return_type} {func.name}({ func.gen_list('{decl}') })
 {{
     assert(!qemu_in_coroutine());
     {struct_name} s = {{
@@ -145,13 +147,13 @@ def create_coroutine_only(func: FuncDecl) -> str:
 
     s.poll_state.co = qemu_coroutine_create({name}_entry, &s);
 
-    return bdrv_poll_co(&s.poll_state);
+    bdrv_poll_co(&s.poll_state);
+    return s.ret;
 }}"""
 
 
 def gen_wrapper(func: FuncDecl) -> str:
     assert not '_co_' in func.name
-    assert func.return_type == 'int'
 
     subsystem, subname = func.name.split('_', 1)
 
@@ -182,6 +184,7 @@ def gen_wrapper(func: FuncDecl) -> str:
 
 typedef struct {struct_name} {{
     BdrvPollCo poll_state;
+    {func.return_type} ret;
 { func.gen_block('    {decl};') }
 }} {struct_name};
 
@@ -189,7 +192,7 @@ def gen_wrapper(func: FuncDecl) -> str:
 {{
     {struct_name} *s = opaque;
 
-    s->poll_state.ret = {name}({ func.gen_list('s->{name}') });
+    s->ret = {name}({ func.gen_list('s->{name}') });
     s->poll_state.in_progress = false;
 
     aio_wait_kick();
-- 
2.31.1

Re: [PATCH v4 06/11] block-coroutine-wrapper.py: support also basic return types

[Kevin Wolf]

Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> Extend the regex to cover also return type, pointers included.
> This implies that the value returned by the function cannot be
> a simple "int" anymore, but the custom return type.
> Therefore remove poll_state->ret and instead use a per-function
> custom "ret" field.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH v4 07/11] block/vmdk: add missing coroutine_fn annotations

[Emanuele Giuseppe Esposito]

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

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

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

Re: [PATCH v4 07/11] block/vmdk: add missing coroutine_fn annotations

[Kevin Wolf]

Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> These functions end up calling bdrv_create() implemented as generated_co_wrapper
> functions.
> In addition, they also happen to be always called in coroutine context,
> meaning all callers are coroutine_fn.
> This means that the g_c_w function will enter the qemu_in_coroutine()
> case and eventually suspend (or in other words call qemu_coroutine_yield()).
> Therefore we need to mark such functions coroutine_fn too.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Just one remark about patch ordering: This doesn't require the
g_c_w_simple patches, so wouldn't it make more sense to move the
g_c_w_simple right before the first patch that actually makes use of
them?

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH v4 07/11] block/vmdk: add missing coroutine_fn annotations

[Emanuele Giuseppe Esposito]

Am 21/11/2022 um 17:01 schrieb Kevin Wolf:
> Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
>> These functions end up calling bdrv_create() implemented as generated_co_wrapper
>> functions.
>> In addition, they also happen to be always called in coroutine context,
>> meaning all callers are coroutine_fn.
>> This means that the g_c_w function will enter the qemu_in_coroutine()
>> case and eventually suspend (or in other words call qemu_coroutine_yield()).
>> Therefore we need to mark such functions coroutine_fn too.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> Just one remark about patch ordering: This doesn't require the
> g_c_w_simple patches, so wouldn't it make more sense to move the
> g_c_w_simple right before the first patch that actually makes use of
> them?

I thought about it, but then I thought it was too far from bdrv_create
patches.
Both g_c_w_simple and this patch are needed by bdrv_create_file, so I
thought if you read this one first then you wouldn't have a clue on what
is the end goal.

Anyways, I'll change the order.

Thank you,
Emanuele

> 
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> 

[PATCH v4 08/11] block: distinguish between bdrv_create running in coroutine and not

[Emanuele Giuseppe Esposito]

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

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

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

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

Re: [PATCH v4 08/11] block: distinguish between bdrv_create running in coroutine and not

[Kevin Wolf]

Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> Call two different functions depending on whether bdrv_create
> is in coroutine or not, following the same pattern as
> generated_co_wrapper functions.
> 
> This allows to also call the coroutine function directly,
> without using CreateCo or relying in bdrv_create().
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  block.c | 76 ++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 38 insertions(+), 38 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 577639c7e0..c610a32e77 100644
> --- a/block.c
> +++ b/block.c
> @@ -528,66 +528,66 @@ typedef struct CreateCo {
>      Error *err;
>  } CreateCo;
>  
> -static void coroutine_fn bdrv_create_co_entry(void *opaque)
> +static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
> +                                       QemuOpts *opts, Error **errp)
>  {
> -    Error *local_err = NULL;
>      int ret;
> +    char *filename_copy;
> +    GLOBAL_STATE_CODE();
> +    assert(*errp == NULL);

Your use of *errp in this function will crash if NULL is passed. You
need ERRP_GUARD() first before you can do this.

> +    assert(drv);
> +
> +    if (!drv->bdrv_co_create_opts) {
> +        error_setg(errp, "Driver '%s' does not support image creation",
> +                   drv->format_name);
> +        return -ENOTSUP;
> +    }
>  
> +    filename_copy = g_strdup(filename);

It's only preserved from the pre-patch state, but I don't think this is
necessary? We know that the string will stay around until the function
returns, and the parameter of drv->bdrv_co_create_opts is const char*,
so it must not be modified either.

Maybe worth a cleanup patch on top to just use filename directly?

> +    ret = drv->bdrv_co_create_opts(drv, filename_copy, opts, errp);
> +
> +    if (ret < 0 && !*errp) {
> +        error_setg_errno(errp, -ret, "Could not create image");
> +    }
> +
> +    g_free(filename_copy);
> +    return ret;
> +}

Kevin

[PATCH v4 09/11] block: bdrv_create_file is a coroutine_fn

[Emanuele Giuseppe Esposito]

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

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

diff --git a/block.c b/block.c
index c610a32e77..7a4c3eb540 100644
--- a/block.c
+++ b/block.c
@@ -534,6 +534,7 @@ static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
     int ret;
     char *filename_copy;
     GLOBAL_STATE_CODE();
+    assert(qemu_in_coroutine());
     assert(*errp == NULL);
     assert(drv);
 
@@ -725,7 +726,8 @@ out:
     return ret;
 }
 
-int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
+int coroutine_fn bdrv_create_file(const char *filename, QemuOpts *opts,
+                                  Error **errp)
 {
     QemuOpts *protocol_opts;
     BlockDriver *drv;
@@ -766,7 +768,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
         goto out;
     }
 
-    ret = bdrv_create(drv, filename, protocol_opts, errp);
+    ret = bdrv_co_create(drv, filename, protocol_opts, errp);
 out:
     qemu_opts_del(protocol_opts);
     qobject_unref(qdict);
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 00e0cf8aea..6f35ed99e3 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -57,7 +57,8 @@ BlockDriver *bdrv_find_protocol(const char *filename,
 BlockDriver *bdrv_find_format(const char *format_name);
 int bdrv_create(BlockDriver *drv, const char* filename,
                 QemuOpts *opts, Error **errp);
-int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
+int coroutine_fn bdrv_create_file(const char *filename, QemuOpts *opts,
+                                  Error **errp);
 
 BlockDriverState *bdrv_new(void);
 int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
-- 
2.31.1

Re: [PATCH v4 09/11] block: bdrv_create_file is a coroutine_fn

[Kevin Wolf]

Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> It is always called in coroutine_fn callbacks, therefore
> it can directly call bdrv_co_create().
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  block.c                            | 6 ++++--
>  include/block/block-global-state.h | 3 ++-
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c610a32e77..7a4c3eb540 100644
> --- a/block.c
> +++ b/block.c
> @@ -534,6 +534,7 @@ static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
>      int ret;
>      char *filename_copy;
>      GLOBAL_STATE_CODE();
> +    assert(qemu_in_coroutine());

We don't generally assert this, otherwise it would have to be in every
coroutine_fn.

>      assert(*errp == NULL);
>      assert(drv);
>  
> @@ -725,7 +726,8 @@ out:
>      return ret;
>  }
>  
> -int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
> +int coroutine_fn bdrv_create_file(const char *filename, QemuOpts *opts,
> +                                  Error **errp)

Should it be renamed as bdrv_co_create_file()?

>  {
>      QemuOpts *protocol_opts;
>      BlockDriver *drv;
> @@ -766,7 +768,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
>          goto out;
>      }
>  
> -    ret = bdrv_create(drv, filename, protocol_opts, errp);
> +    ret = bdrv_co_create(drv, filename, protocol_opts, errp);
>  out:
>      qemu_opts_del(protocol_opts);
>      qobject_unref(qdict);

Kevin

Re: [PATCH v4 09/11] block: bdrv_create_file is a coroutine_fn

[Emanuele Giuseppe Esposito]

Am 22/11/2022 um 09:58 schrieb Kevin Wolf:
> Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
>> It is always called in coroutine_fn callbacks, therefore
>> it can directly call bdrv_co_create().
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>  block.c                            | 6 ++++--
>>  include/block/block-global-state.h | 3 ++-
>>  2 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index c610a32e77..7a4c3eb540 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -534,6 +534,7 @@ static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
>>      int ret;
>>      char *filename_copy;
>>      GLOBAL_STATE_CODE();
>> +    assert(qemu_in_coroutine());
> 
> We don't generally assert this, otherwise it would have to be in every
> coroutine_fn.

That was my plan for the serie "Protect the block layer with a rwlock:
part 3", where I convert BlockDriver callbacks in coroutine, and thus
assert there because I know all the callers are coroutine_fn.

> 
>>      assert(*errp == NULL);
>>      assert(drv);
>>  
>> @@ -725,7 +726,8 @@ out:
>>      return ret;
>>  }
>>  
>> -int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
>> +int coroutine_fn bdrv_create_file(const char *filename, QemuOpts *opts,
>> +                                  Error **errp)
> 
> Should it be renamed as bdrv_co_create_file()?
> 
Ok (when I don't answer just assume that I agree).

Thank you,
Emanuele
>>  {
>>      QemuOpts *protocol_opts;
>>      BlockDriver *drv;
>> @@ -766,7 +768,7 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
>>          goto out;
>>      }
>>  
>> -    ret = bdrv_create(drv, filename, protocol_opts, errp);
>> +    ret = bdrv_co_create(drv, filename, protocol_opts, errp);
>>  out:
>>      qemu_opts_del(protocol_opts);
>>      qobject_unref(qdict);
> 
> Kevin
> 

[PATCH v4 10/11] block: convert bdrv_create to generated_co_wrapper_simple

[Emanuele Giuseppe Esposito]

This function is never called in coroutine context, therefore
instead of manually creating a new coroutine, delegate it to the
block-coroutine-wrapper script, defining it as g_c_w_simple.

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

diff --git a/block.c b/block.c
index 7a4c3eb540..d3e168408a 100644
--- a/block.c
+++ b/block.c
@@ -528,7 +528,7 @@ typedef struct CreateCo {
     Error *err;
 } CreateCo;
 
-static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
+int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
                                        QemuOpts *opts, Error **errp)
 {
     int ret;
@@ -555,42 +555,6 @@ static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
     return ret;
 }
 
-static void coroutine_fn bdrv_create_co_entry(void *opaque)
-{
-    CreateCo *cco = opaque;
-    GLOBAL_STATE_CODE();
-
-    cco->ret = bdrv_co_create(cco->drv, cco->filename, cco->opts, &cco->err);
-}
-
-int bdrv_create(BlockDriver *drv, const char* filename,
-                QemuOpts *opts, Error **errp)
-{
-    GLOBAL_STATE_CODE();
-
-    if (qemu_in_coroutine()) {
-        /* Fast-path if already in coroutine context */
-        return bdrv_co_create(drv, filename, opts, errp);
-    } else {
-        Coroutine *co;
-        CreateCo cco = {
-            .drv = drv,
-            .filename = filename,
-            .opts = opts,
-            .ret = NOT_DONE,
-            .err = NULL,
-        };
-
-        co = qemu_coroutine_create(bdrv_create_co_entry, &cco);
-        qemu_coroutine_enter(co);
-        while (cco.ret == NOT_DONE) {
-            aio_poll(qemu_get_aio_context(), true);
-        }
-        error_propagate(errp, cco.err);
-        return cco.ret;
-    }
-}
-
 /**
  * Helper function for bdrv_create_file_fallback(): Resize @blk to at
  * least the given @minimum_size.
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 6f35ed99e3..305336bdb9 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -55,8 +55,14 @@ BlockDriver *bdrv_find_protocol(const char *filename,
                                 bool allow_protocol_prefix,
                                 Error **errp);
 BlockDriver *bdrv_find_format(const char *format_name);
-int bdrv_create(BlockDriver *drv, const char* filename,
-                QemuOpts *opts, Error **errp);
+
+int coroutine_fn bdrv_co_create(BlockDriver *drv, const char* filename,
+                                QemuOpts *opts, Error **errp);
+int generated_co_wrapper_simple bdrv_create(BlockDriver *drv,
+                                            const char* filename,
+                                            QemuOpts *opts,
+                                            Error **errp);
+
 int coroutine_fn bdrv_create_file(const char *filename, QemuOpts *opts,
                                   Error **errp);
 
-- 
2.31.1

Re: [PATCH v4 10/11] block: convert bdrv_create to generated_co_wrapper_simple

[Kevin Wolf]

Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> This function is never called in coroutine context, therefore
> instead of manually creating a new coroutine, delegate it to the
> block-coroutine-wrapper script, defining it as g_c_w_simple.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

At first I thought that "never called in coroutine context" was not
entirely true because of paths like this:

qcow2_co_create() -> bdrv_open_blockdev_ref() -> bdrv_open_inherit() ->
bdrv_append_temp_snapshot() -> bdrv_create().

The only reason why it doesn't happen is that with a BlockdevRef, it's
impossible to get BDRV_O_SNAPSHOT set, so bdrv_append_temp_snapshot()
can't actually be called when you come from bdrv_open_blockdev_ref().

Of course, opening images in a coroutine is likely to already do similar
things. We should probably drop out of coroutine context for bdrv_open
to be safe. In practice, I suspect it might work anyway because nothing
is going to wait on the current coroutine, but maybe better not to rely
on it.

Anyway, not a problem of your patch, it's just something it made me
think of.

> diff --git a/block.c b/block.c
> index 7a4c3eb540..d3e168408a 100644
> --- a/block.c
> +++ b/block.c
> @@ -528,7 +528,7 @@ typedef struct CreateCo {
>      Error *err;
>  } CreateCo;
>  
> -static int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
> +int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
>                                         QemuOpts *opts, Error **errp)

The indentation is off now. With this fixed:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH v4 11/11] block/dirty-bitmap: convert coroutine-only functions to generated_co_wrapper_simple

[Emanuele Giuseppe Esposito]

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

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/dirty-bitmap.c         | 88 +-----------------------------------
 block/meson.build            |  1 +
 include/block/block-common.h |  5 +-
 include/block/block-io.h     |  9 +++-
 include/block/dirty-bitmap.h | 10 +++-
 5 files changed, 21 insertions(+), 92 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index bf3dc0512a..21cf592889 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -388,7 +388,7 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
  * not fail.
  * This function doesn't release corresponding BdrvDirtyBitmap.
  */
-static int coroutine_fn
+int coroutine_fn
 bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
                                        Error **errp)
 {
@@ -399,45 +399,6 @@ bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
     return 0;
 }
 
-typedef struct BdrvRemovePersistentDirtyBitmapCo {
-    BlockDriverState *bs;
-    const char *name;
-    Error **errp;
-    int ret;
-} BdrvRemovePersistentDirtyBitmapCo;
-
-static void coroutine_fn
-bdrv_co_remove_persistent_dirty_bitmap_entry(void *opaque)
-{
-    BdrvRemovePersistentDirtyBitmapCo *s = opaque;
-
-    s->ret = bdrv_co_remove_persistent_dirty_bitmap(s->bs, s->name, s->errp);
-    aio_wait_kick();
-}
-
-int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
-                                        Error **errp)
-{
-    if (qemu_in_coroutine()) {
-        return bdrv_co_remove_persistent_dirty_bitmap(bs, name, errp);
-    } else {
-        Coroutine *co;
-        BdrvRemovePersistentDirtyBitmapCo s = {
-            .bs = bs,
-            .name = name,
-            .errp = errp,
-            .ret = -EINPROGRESS,
-        };
-
-        co = qemu_coroutine_create(bdrv_co_remove_persistent_dirty_bitmap_entry,
-                                   &s);
-        bdrv_coroutine_enter(bs, co);
-        BDRV_POLL_WHILE(bs, s.ret == -EINPROGRESS);
-
-        return s.ret;
-    }
-}
-
 bool
 bdrv_supports_persistent_dirty_bitmap(BlockDriverState *bs)
 {
@@ -447,7 +408,7 @@ bdrv_supports_persistent_dirty_bitmap(BlockDriverState *bs)
     return false;
 }
 
-static bool coroutine_fn
+bool coroutine_fn
 bdrv_co_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
                                    uint32_t granularity, Error **errp)
 {
@@ -470,51 +431,6 @@ bdrv_co_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
     return drv->bdrv_co_can_store_new_dirty_bitmap(bs, name, granularity, errp);
 }
 
-typedef struct BdrvCanStoreNewDirtyBitmapCo {
-    BlockDriverState *bs;
-    const char *name;
-    uint32_t granularity;
-    Error **errp;
-    bool ret;
-
-    bool in_progress;
-} BdrvCanStoreNewDirtyBitmapCo;
-
-static void coroutine_fn bdrv_co_can_store_new_dirty_bitmap_entry(void *opaque)
-{
-    BdrvCanStoreNewDirtyBitmapCo *s = opaque;
-
-    s->ret = bdrv_co_can_store_new_dirty_bitmap(s->bs, s->name, s->granularity,
-                                                s->errp);
-    s->in_progress = false;
-    aio_wait_kick();
-}
-
-bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
-                                     uint32_t granularity, Error **errp)
-{
-    IO_CODE();
-    if (qemu_in_coroutine()) {
-        return bdrv_co_can_store_new_dirty_bitmap(bs, name, granularity, errp);
-    } else {
-        Coroutine *co;
-        BdrvCanStoreNewDirtyBitmapCo s = {
-            .bs = bs,
-            .name = name,
-            .granularity = granularity,
-            .errp = errp,
-            .in_progress = true,
-        };
-
-        co = qemu_coroutine_create(bdrv_co_can_store_new_dirty_bitmap_entry,
-                                   &s);
-        bdrv_coroutine_enter(bs, co);
-        BDRV_POLL_WHILE(bs, s.in_progress);
-
-        return s.ret;
-    }
-}
-
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
     bdrv_dirty_bitmaps_lock(bitmap->bs);
diff --git a/block/meson.build b/block/meson.build
index b7c68b83a3..c48a59571e 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -137,6 +137,7 @@ block_gen_c = custom_target('block-gen.c',
                             output: 'block-gen.c',
                             input: files(
                                       '../include/block/block-io.h',
+                                      '../include/block/dirty-bitmap.h',
                                       '../include/block/block-global-state.h',
                                       '../include/sysemu/block-backend-io.h',
                                       'coroutines.h'
diff --git a/include/block/block-common.h b/include/block/block-common.h
index 8ae750c7cf..683e3d1c51 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -29,8 +29,6 @@
 #include "qemu/iov.h"
 #include "qemu/coroutine.h"
 #include "block/accounting.h"
-#include "block/dirty-bitmap.h"
-#include "block/blockjob.h"
 #include "qemu/hbitmap.h"
 #include "qemu/transactions.h"
 
@@ -45,6 +43,9 @@
 #define generated_co_wrapper
 #define generated_co_wrapper_simple
 
+#include "block/dirty-bitmap.h"
+#include "block/blockjob.h"
+
 /* block.c */
 typedef struct BlockDriver BlockDriver;
 typedef struct BdrvChild BdrvChild;
diff --git a/include/block/block-io.h b/include/block/block-io.h
index 0b49cb33da..f45ef6206f 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -200,8 +200,13 @@ AioContext *child_of_bds_get_parent_aio_context(BdrvChild *c);
 void bdrv_io_plug(BlockDriverState *bs);
 void bdrv_io_unplug(BlockDriverState *bs);
 
-bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
-                                     uint32_t granularity, Error **errp);
+bool coroutine_fn bdrv_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
+                                                     const char *name,
+                                                     uint32_t granularity,
+                                                     Error **errp);
+bool generated_co_wrapper_simple
+bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
+                                uint32_t granularity, Error **errp);
 
 /**
  *
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 6528336c4c..2290e7fa28 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -34,8 +34,14 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
                             Error **errp);
 void bdrv_release_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
-int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
-                                        Error **errp);
+
+int coroutine_fn bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+                                                        const char *name,
+                                                        Error **errp);
+int generated_co_wrapper_simple
+bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
+                                    Error **errp);
+
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap);
-- 
2.31.1

Re: [PATCH v4 11/11] block/dirty-bitmap: convert coroutine-only functions to generated_co_wrapper_simple

[Kevin Wolf]

Am 16.11.2022 um 13:22 hat Emanuele Giuseppe Esposito geschrieben:
> bdrv_can_store_new_dirty_bitmap and bdrv_remove_persistent_dirty_bitmap
> check if they are running in a coroutine, directly calling the
> coroutine callback if it's the case.
> Except that no coroutine calls such functions, therefore that check
> can be removed, and function creation can be offloaded to
> g_c_w_simple.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>