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

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

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, 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.
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.

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;
>