Re: [RFC PATCH 1/3] block-copy: improve documentation for BlockCopyTask type and functions

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

On 20/04/2021 14:03, Vladimir Sementsov-Ogievskiy wrote:
> 20.04.2021 13:04, Emanuele Giuseppe Esposito wrote:
>> As done in BlockCopyCallState, categorize BlockCopyTask
>> in IN, State and OUT fields. This is just to understand
>> which field has to be protected with a lock.
>>
>> Also add coroutine_fn attribute to block_copy_task_create,
>> because it is only usedn block_copy_dirty_clusters, a coroutine
>> function itself.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   block/block-copy.c | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/block/block-copy.c b/block/block-copy.c
>> index 39ae481c8b..03df50a354 100644
>> --- a/block/block-copy.c
>> +++ b/block/block-copy.c
>> @@ -48,25 +48,32 @@ typedef struct BlockCopyCallState {
>>       QLIST_ENTRY(BlockCopyCallState) list;
>>       /* State */
>> -    int ret;
>>       bool finished;
>>       QemuCoSleepState *sleep_state;
>>       bool cancelled;
>>       /* OUT parameters */
>> +    int ret;
> 
> Hmm. Somehow, ret may be considered is part of state too.. But I don't 
> really care. Here it looks not bad. Will see how and what you are going 
> protect by new lock.
> 
> Note, that ret is concurently set in block_copy_task_entry..

It is set but as far as I understood it contains the result of the 
operation (thus OUT), correct?

> 
>>       bool error_is_read;
>>   } BlockCopyCallState;
>>   typedef struct BlockCopyTask {
>> +    /* IN parameters. Initialized in block_copy_task_create()
>> +     * and never changed.
>> +     */
> 
> It's wrong about task field, as it has "ret" inside.
Not sure I understand what you mean here.

> 
>>       AioTask task;
>> -
>>       BlockCopyState *s;
>>       BlockCopyCallState *call_state;
>> +
>> +    /* State */
>>       int64_t offset;
> 
> I think, offset is never changed after block_copy_task_create()..

right, will revert that for offset
> 
>>       int64_t bytes;
>>       bool zeroes;
>> -    QLIST_ENTRY(BlockCopyTask) list;
>>       CoQueue wait_queue; /* coroutines blocked on this task */
>> +
>> +    /* To reference all call states from BlockCopyTask */
> 
> Amm.. Actually,
> 
> To reference all tasks from BlockCopyState

right, agree, thanks
> 
>> +    QLIST_ENTRY(BlockCopyTask) list;
>> +
>>   } BlockCopyTask;
>>   static int64_t task_end(BlockCopyTask *task)
>> @@ -153,7 +160,7 @@ static bool coroutine_fn 
>> block_copy_wait_one(BlockCopyState *s, int64_t offset,
>>    * Search for the first dirty area in offset/bytes range and create 
>> task at
>>    * the beginning of it.
>>    */
>> -static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
>> +static BlockCopyTask *coroutine_fn 
>> block_copy_task_create(BlockCopyState *s,
>>                                                BlockCopyCallState 
>> *call_state,
>>                                                int64_t offset, int64_t 
>> bytes)
>>   {
>>
> 
> We mark by "coroutine_fn" functions that can be called _only_ from 
> coroutine context. 
In my opinion, block_copy_task_create is a static function and it's 
called only by another coroutine_fn, block_copy_dirty_clusters, so it 
matches what you just wrote above.

> block_copy_task_create() may be called from any 
> context, both coroutine and non-coroutine. So, it shouldn't have this mark.

Thank you,
Emanuele