Am 04/11/2022 um 09:44 schrieb Paolo Bonzini:
> 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.
[...]
>> 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.
I see your point about "in general it's not a bug".
At this point I just want to make sure that we agree that it's correct
to add coroutine_fn because of "the function calls a g_c_w that
suspends" *&&* "all the callers are coroutine_fn". If the callers
weren't coroutine_fn then g_c_w would just create a new coroutine and
poll, and I don't think that would be part of your definition of "can
suspend".
Thank you,
Emanuele
>
>> 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
>
