diff mbox series

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

Message ID 20221116122241.2856527-6-eesposit@redhat.com
State New
Headers show
Series Still more coroutine and various fixes in block layer | expand

Commit Message

Emanuele Giuseppe Esposito Nov. 16, 2022, 12:22 p.m. UTC 
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(-)

Comments

Kevin Wolf Nov. 21, 2022, 3:30 p.m. UTC | #1 
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
Emanuele Giuseppe Esposito Nov. 21, 2022, 3:52 p.m. UTC | #2 
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
Kevin Wolf Nov. 22, 2022, 8:27 a.m. UTC | #3 
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
diff mbox series

Patch

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