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;
     bool error_is_read;
 } BlockCopyCallState;
 
 typedef struct BlockCopyTask {
+    /* IN parameters. Initialized in block_copy_task_create()
+     * and never changed.
+     */
     AioTask task;
-
     BlockCopyState *s;
     BlockCopyCallState *call_state;
+
+    /* State */
     int64_t 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 */
+    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)
 {
-- 
2.30.2

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

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

>       AioTask task;
> -
>       BlockCopyState *s;
>       BlockCopyCallState *call_state;
> +
> +    /* State */
>       int64_t offset;

I think, offset is never changed after block_copy_task_create()..

>       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

> +    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. block_copy_task_create() may be called from any context, both coroutine and non-coroutine. So, it shouldn't have this mark.


-- 
Best regards,
Vladimir

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

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