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

Emanuele Giuseppe Esposito posted 9 patches 3 years, 3 months ago
Maintainers: Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, John Snow <jsnow@redhat.com>, Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>, Eric Blake <eblake@redhat.com>, Fam Zheng <fam@euphon.net>
There is a newer version of this series
[PATCH 1/9] block: call bdrv_co_drain_begin in a coroutine
Posted by Emanuele Giuseppe Esposito 3 years, 3 months ago
It seems that bdrv_open_driver() forgot to create a coroutine
where to call bs->drv->bdrv_co_drain_begin(), a callback
marked as coroutine_fn.

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

diff --git a/block.c b/block.c
index 5311b21f8e..7211f62001 100644
--- a/block.c
+++ b/block.c
@@ -1637,12 +1637,34 @@ out:
     g_free(gen_node_name);
 }
 
+typedef struct DrainCo {
+    BlockDriverState *bs;
+    int ret;
+} DrainCo;
+
+static void coroutine_fn bdrv_co_drain_begin(void *opaque)
+{
+    int i;
+    DrainCo *co = opaque;
+    BlockDriverState *bs = co->bs;
+
+    for (i = 0; i < bs->quiesce_counter; i++) {
+        bs->drv->bdrv_co_drain_begin(bs);
+    }
+    co->ret = 0;
+}
+
 static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
                             const char *node_name, QDict *options,
                             int open_flags, Error **errp)
 {
     Error *local_err = NULL;
-    int i, ret;
+    int ret;
+    Coroutine *co;
+    DrainCo dco = {
+        .bs = bs,
+        .ret = NOT_DONE,
+    };
     GLOBAL_STATE_CODE();
 
     bdrv_assign_node_name(bs, node_name, &local_err);
@@ -1690,10 +1712,10 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
     assert(bdrv_min_mem_align(bs) != 0);
     assert(is_power_of_2(bs->bl.request_alignment));
 
-    for (i = 0; i < bs->quiesce_counter; i++) {
-        if (drv->bdrv_co_drain_begin) {
-            drv->bdrv_co_drain_begin(bs);
-        }
+    if (drv->bdrv_co_drain_begin) {
+        co = qemu_coroutine_create(bdrv_co_drain_begin, &dco);
+        qemu_coroutine_enter(co);
+        AIO_WAIT_WHILE(qemu_get_aio_context(), dco.ret == NOT_DONE);
     }
 
     return 0;
-- 
2.31.1
Re: [PATCH 1/9] block: call bdrv_co_drain_begin in a coroutine
Posted by Paolo Bonzini 3 years, 3 months ago
On 11/3/22 14:41, Emanuele Giuseppe Esposito wrote:
> -    for (i = 0; i < bs->quiesce_counter; i++) {
> -        if (drv->bdrv_co_drain_begin) {
> -            drv->bdrv_co_drain_begin(bs);
> -        }
> +    if (drv->bdrv_co_drain_begin) {
> +        co = qemu_coroutine_create(bdrv_co_drain_begin, &dco);
> +        qemu_coroutine_enter(co);
> +        AIO_WAIT_WHILE(qemu_get_aio_context(), dco.ret == NOT_DONE);
>       }
>   

Alternatively there should be no reason for drv->bdrv_co_drain_begin to 
wait at this point, because the device does not have any active I/O.  So 
you could also assert that the coroutine is terminated after 
qemu_coroutine_enter(), i.e. that dco.ret != NOT_DONE.

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

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

Paolo