Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations

[Paolo Bonzini <pbonzini@redhat.com>]

On 11/4/22 08:35, Emanuele Giuseppe Esposito wrote:
> But isn't it a bug also not to mark a function _only_ called by
> coroutine_fn? My point is that if this function is an implementation of
> a BlockDriver callback marked as coroutine_fn (like in patch 6 with
> vmdk), then it would make sense.

If a function implements a coroutine_fn callback but does not suspend, 
then it makes sense to mark it coroutine_fn.

In general it's not a bug.  In most cases it would only be a coincidence 
that the function is called from a coroutine_fn.  For example consider 
bdrv_round_to_clusters().  Marking it coroutine_fn signals that it may 
suspend now (it doesn't) or in the future.  However it's only doing some 
math based on the result of bdrv_get_info(), so it is extremely unlikely 
that this will happen.

In this case... oh wait.  block_copy_is_cluster_allocated is calling 
bdrv_is_allocated, and block_copy_reset_unallocated calls 
block_copy_is_cluster_allocated.  bdrv_is_allocated is a mixed 
coroutine/non-coroutine function, and in this case it is useful to 
document that bdrv_is_allocated will suspend.  The patch is correct, 
only the commit message is wrong.

Likewise for blockstatus_to_extents in patch 3, where the commit message 
does mention bdrv_* functions.  As I mentioned in my quick review of 
patch 3, this can also snowball into a series of its own to clean up all 
callees of bdrv_co_common_block_status_above, similar to what Alberto 
did for read/write functions back in June, so that they are properly 
marked as coroutine_fn.  If you want to do it, don't do it by hand 
though, you can use his static analyzer.  It's slow but it's faster than 
doing it by hand.

> This is actually the point of this serie (which I might not have
> explained well in the cover letter), every function marked here is
> eventually called by/calling a BlockDriver callback marked as coroutine_fn.

Again I don't think this is useful in general, but your three patches 
(2/3/6) did catch cases that wants to be coroutine_fn.  So my objection 
is dropped with just a better commit message.

> Currently we have something like this:
> BlockDriver {
>      void coroutine_fn (*bdrv_A)(void) = implA;
> }
> 
> void coroutine_fn implA() {
>      funcB();
>      funcC();
> }
> 
> void funcB() {}; <--- missing coroutine_fn?
> void funcC() {}; <--- missing coroutine_fn?
> 
> In addition, as I understand draining is not allowed in coroutines.

... except we have bdrv_co_yield_to_drain() to allow that, sort of. :/

> If a function/callback only running in coroutines is not marked as
> coroutine_fn, then it will be less obvious to notice that draining is
> not allowed there.

I think it has to be judged case by base.  Your patches prove that, in 
most cases, you have coroutine_fn for things that ultimately do some 
kind of I/O or query.  In general the interesting path to explore is 
"coroutine_fn calls (indirectly) non-coroutine_fn calls (indirectly) 
generated_co_wrapper".  The vrc tool could be extended to help finding 
them, with commands like

     label coroutine_fn bdrv_co_read
     label coroutine_fn bdrv_co_write
     ...
     label generated_co_wrapper bdrv_read
     label generated_co_wrapper bdrv_write
     paths coroutine_fn !coroutine_fn generated_co_wrapper

Paolo