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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

20.04.2021 15:51, Emanuele Giuseppe Esposito wrote:
> 
> 
> 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?

Yes. I just mean, that ret should be protected too. If block_copy_task_entry() called concurently from different threads, we'll check-and-set ret concurently.

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

task.ret it not an "IN" parameter

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

"coroutine_fn" is restriction. block_copy_task_create doesn't need this restriction. It may be safely called from non-coroutine context. So, no reason to add the restriction.

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


-- 
Best regards,
Vladimir