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