Re: [PATCH v2 13/15] block: Drop out of coroutine in bdrv_do_drained_begin_quiesce()

2022-11-25 Thread Vladimir Sementsov-Ogievskiy

On 11/18/22 20:41, Kevin Wolf wrote:

The next patch adds a parent drain to bdrv_attach_child_common(), which
shouldn't be, but is currently called from coroutines in some cases (e.g.
.bdrv_co_create implementations generally open new nodes). Therefore,
the assertion that we're not in a coroutine doesn't hold true any more.

We could just remove the assertion because there is nothing in the
function that should be in conflict with running in a coroutine, but
just to be on the safe side, we can reverse the caller relationship
between bdrv_do_drained_begin() and bdrv_do_drained_begin_quiesce() so
that the latter also just drops out of coroutine context and we can
still be certain in the future that any drain code doesn't run in
coroutines.

As a nice side effect, the structure of bdrv_do_drained_begin() is now
symmetrical with bdrv_do_drained_end().

Signed-off-by: Kevin Wolf


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir




[PATCH v2 13/15] block: Drop out of coroutine in bdrv_do_drained_begin_quiesce()

2022-11-18 Thread Kevin Wolf
The next patch adds a parent drain to bdrv_attach_child_common(), which
shouldn't be, but is currently called from coroutines in some cases (e.g.
.bdrv_co_create implementations generally open new nodes). Therefore,
the assertion that we're not in a coroutine doesn't hold true any more.

We could just remove the assertion because there is nothing in the
function that should be in conflict with running in a coroutine, but
just to be on the safe side, we can reverse the caller relationship
between bdrv_do_drained_begin() and bdrv_do_drained_begin_quiesce() so
that the latter also just drops out of coroutine context and we can
still be certain in the future that any drain code doesn't run in
coroutines.

As a nice side effect, the structure of bdrv_do_drained_begin() is now
symmetrical with bdrv_do_drained_end().

Signed-off-by: Kevin Wolf 
---
 block/io.c | 25 -
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/block/io.c b/block/io.c
index 2e9503df6a..5e9150d92c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -346,10 +346,15 @@ static void coroutine_fn 
bdrv_co_yield_to_drain(BlockDriverState *bs,
 }
 }
 
-void bdrv_do_drained_begin_quiesce(BlockDriverState *bs, BdrvChild *parent)
+static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent,
+  bool poll)
 {
 IO_OR_GS_CODE();
-assert(!qemu_in_coroutine());
+
+if (qemu_in_coroutine()) {
+bdrv_co_yield_to_drain(bs, true, parent, poll);
+return;
+}
 
 /* Stop things in parent-to-child order */
 if (qatomic_fetch_inc(>quiesce_counter) == 0) {
@@ -359,17 +364,6 @@ void bdrv_do_drained_begin_quiesce(BlockDriverState *bs, 
BdrvChild *parent)
 bs->drv->bdrv_drain_begin(bs);
 }
 }
-}
-
-static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent,
-  bool poll)
-{
-if (qemu_in_coroutine()) {
-bdrv_co_yield_to_drain(bs, true, parent, poll);
-return;
-}
-
-bdrv_do_drained_begin_quiesce(bs, parent);
 
 /*
  * Wait for drained requests to finish.
@@ -385,6 +379,11 @@ static void bdrv_do_drained_begin(BlockDriverState *bs, 
BdrvChild *parent,
 }
 }
 
+void bdrv_do_drained_begin_quiesce(BlockDriverState *bs, BdrvChild *parent)
+{
+bdrv_do_drained_begin(bs, parent, false);
+}
+
 void bdrv_drained_begin(BlockDriverState *bs)
 {
 IO_OR_GS_CODE();
-- 
2.38.1