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

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

Am 08/11/2022 um 17:19 schrieb Vladimir Sementsov-Ogievskiy:
> [add Stefan]
> 
> 
> On 11/8/22 18:09, Emanuele Giuseppe Esposito wrote:
>>
>>
>> Am 08/11/2022 um 15:48 schrieb Vladimir Sementsov-Ogievskiy:
>>> On 11/4/22 12:56, Emanuele Giuseppe Esposito wrote:
>>>> These functions end up calling bdrv_common_block_status_above(), a
>>>> generated_co_wrapper function.
>>>
>>> generated_co_wrapper is not a coroutine_fn. Сonversely it's a function
>>> that do a class coroutine wrapping - start a coroutine and do POLL to
>>> wait for the coroutine to finish.
>>>
>>>> In addition, they also happen to be always called in coroutine context,
>>>> meaning all callers are coroutine_fn.
>>>
>>> That's also not a reason for marking them coroutine_fn. "coroutine_fn"
>>> means that the function can be called only from coroutine context.
>>>
>>>> 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.
>>>
>>> I don't think so. Moreover, this breaks the concept, as your new
>>> coroutine_fn functions will call generated_co_wrapper functions which
>>> are not marked coroutine_fn and never was.
>>
>> Theoretically not, 
> 
> Agree, I was wrong in this point
> 
>> because marking it coroutine_fn we know that we are
>> going in the if(qemu_in_coroutine()) branch of the g_c_w, so we could
>> directly replace it with the actual function. Because it's a pain to do
>> it with hand, and at some point I/someone should use Alberto static
>> analyzer to get rid of that, I decided to leave g_c_w there.
>>
>> As I understand it, it seems that you and Paolo have a different
>> understanding on what coroutine_fn means and where it should be used.
> 
> Looks so...
> 
> But we have a documentation in coroutine.h, let's check:
> 
>  * Mark a function that executes in coroutine context
>  *
>  * Functions that execute in coroutine context cannot be called directly
> from
>  * normal functions.  In the future it would be nice to enable compiler or
>  * static checker support for catching such errors.  This annotation
> might make
>  * it possible and in the meantime it serves as documentation.
> 
> Not very clear, but still it say:
> 
>  coroutine_fn = "function that executes in coroutine context"
>  "functions that execute in coroutine context"  =  "cannot be called
> directly from normal functions"
> 
> So, IMHO that corresponds to my point of view: we shouldn't mark by
> coroutine_fn functions that can be called from both coroutine context
> and not.

Yes and the point is that these functions are not called by
non-coroutine context.

> 
> If we want to change the concept, we should start with rewriting this
> documentation.
> 
>> Honestly I don't understand your point, as you said
>>
>>> "coroutine_fn"
>>> means that the function can be called only from coroutine context.
>>
>> which is the case for these functions. So could you please clarify?
>>
>> What I do know is that it's extremely confusing to understand if a
>> function that is *not* marked as coroutine_fn is actually being called
>> also from coroutines or not.
>>
>> Which complicates A LOT whatever change or operation I want to perform
>> on the BlockDriver callbacks or any other function in the block layer,
>> because in the current approach for the AioContext lock removal (which
>> you are not aware of, I understand) we need to distinguish between
>> functions running in coroutine context and not, and throwing g_c_w
>> functions everywhere makes my work much harder that it already is.
> 
> OK, I understand the problem.
> 
> Hmm. Formally marking by "coroutine_fn" a function that theoretically
> can be called from any context doesn't break things. We just say that
> since that moment we don't allow to call this function from
> non-coroutine context.
> 
> OK, I tend to agree that you are on the right way, sorry for my skepticism)
> 
> PS: you recently introduced a lot of IO_CODE() / GLOBAL_STATE_CODE()
> marks, which (will be/already) turned into assertions.
> 
> This is a lot better than our "coroutine_fn" sign, which actually do no
> check (and can't do). Don't you plan to swap a "coroutine_fn" noop
> marker with more meaningful IN_COROUTINE(); (or something like this,
> which just do assert(qemu_in_coroutine())) at start of the function? It
> would be a lot safer.

CCing also Alberto and Paolo

So basically I think what we need is something that scans the whole
block layer code and puts the right coroutine_fn annotations (or
assertions, if you want) in the right places.

The rule should be (anyone please correct me if I am wrong):
- if a function is only called by coroutine_fn functions, then it's a
coroutine_fn. Theoretically all these functions should eventually end up
calling qemu_coroutine_yield or similar.

- if it's called only from non-coroutine, then leave it as it is.
Probably asserting is a good idea.

- if it's used by both, then it's a case-by-case decision: I think for
simple functions, we can use a special annotation and document that they
should always consider that they could run in both cases.
If it's a big function like the g_c_w, call only the _co_ version in
coroutine_fn, and create a coroutine or call the non-coroutine
counterpart if we are not in coroutine context.
Which is also what I am trying to do with g_c_w_simple.

However, it would be nice to assign this to someone and do this
automatically, not doing it by hand. I am not sure if Alberto static
analyzer is currently capable of doing that.

What do you all think?

Thank you,
Emanuele

> 
> 
>>
>> Thank you,
>> Emanuele
>>
>>>
>>>>
>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>> ---
>>>>    block/block-copy.c | 15 +++++++++------
>>>>    1 file changed, 9 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/block/block-copy.c b/block/block-copy.c
>>>> index bb947afdda..f33ab1d0b6 100644
>>>> --- a/block/block-copy.c
>>>> +++ b/block/block-copy.c
>>>> @@ -577,8 +577,9 @@ static coroutine_fn int
>>>> block_copy_task_entry(AioTask *task)
>>>>        return ret;
>>>>    }
>>>>    -static int block_copy_block_status(BlockCopyState *s, int64_t
>>>> offset,
>>>> -                                   int64_t bytes, int64_t *pnum)
>>>> +static coroutine_fn int block_copy_block_status(BlockCopyState *s,
>>>> +                                                int64_t offset,
>>>> +                                                int64_t bytes,
>>>> int64_t *pnum)
>>>>    {
>>>>        int64_t num;
>>>>        BlockDriverState *base;
>>>> @@ -613,8 +614,9 @@ static int block_copy_block_status(BlockCopyState
>>>> *s, int64_t offset,
>>>>     * Check if the cluster starting at offset is allocated or not.
>>>>     * return via pnum the number of contiguous clusters sharing this
>>>> allocation.
>>>>     */
>>>> -static int block_copy_is_cluster_allocated(BlockCopyState *s, int64_t
>>>> offset,
>>>> -                                           int64_t *pnum)
>>>> +static int coroutine_fn
>>>> block_copy_is_cluster_allocated(BlockCopyState *s,
>>>> +                                                        int64_t
>>>> offset,
>>>> +                                                        int64_t *pnum)
>>>>    {
>>>>        BlockDriverState *bs = s->source->bs;
>>>>        int64_t count, total_count = 0;
>>>> @@ -669,8 +671,9 @@ void block_copy_reset(BlockCopyState *s, int64_t
>>>> offset, int64_t bytes)
>>>>     * @return 0 when the cluster at @offset was unallocated,
>>>>     *         1 otherwise, and -ret on error.
>>>>     */
>>>> -int64_t block_copy_reset_unallocated(BlockCopyState *s,
>>>> -                                     int64_t offset, int64_t *count)
>>>> +int64_t coroutine_fn block_copy_reset_unallocated(BlockCopyState *s,
>>>> +                                                  int64_t offset,
>>>> +                                                  int64_t *count)
>>>>    {
>>>>        int ret;
>>>>        int64_t clusters, bytes;
>>>
>>
>