[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations
From: |
Emanuele Giuseppe Esposito |
Subject: |
Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations |
Date: |
Fri, 4 Nov 2022 10:20:47 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 |
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
>
- [PATCH 3/9] nbd/server.c: add missing coroutine_fn annotations, (continued)
- [PATCH 3/9] nbd/server.c: add missing coroutine_fn annotations, Emanuele Giuseppe Esposito, 2022/11/03
- [PATCH 8/9] block: bdrv_create is never called in non-coroutine context, Emanuele Giuseppe Esposito, 2022/11/03
- [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Emanuele Giuseppe Esposito, 2022/11/03
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Paolo Bonzini, 2022/11/03
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Kevin Wolf, 2022/11/03
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Paolo Bonzini, 2022/11/03
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Emanuele Giuseppe Esposito, 2022/11/04
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Paolo Bonzini, 2022/11/04
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations,
Emanuele Giuseppe Esposito <=
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Paolo Bonzini, 2022/11/04
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Kevin Wolf, 2022/11/04
[PATCH 6/9] block/vmdk: add missing coroutine_fn annotations, Emanuele Giuseppe Esposito, 2022/11/03
[PATCH 9/9] block/dirty-bitmap: remove unnecessary qemu_in_coroutine() case, Emanuele Giuseppe Esposito, 2022/11/03