[RFC PATCH 0/3] block-copy: lock tasks and calls list

[RFC PATCH 0/3] block-copy: lock tasks and calls list

[Emanuele Giuseppe Esposito]

This serie of patches continues Paolo's series on making the
block layer thread safe. Add a CoMutex lock for both tasks and
calls list present in block/block-copy.c

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Emanuele Giuseppe Esposito (3):
  block-copy: improve documentation for BlockCopyTask type and functions
  block-copy: add a CoMutex to the BlockCopyTask list
  block-copy: add CoMutex lock for BlockCopyCallState list

 block/block-copy.c | 41 +++++++++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 8 deletions(-)

-- 
2.30.2

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

[Emanuele Giuseppe Esposito]

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

[RFC PATCH 2/3] block-copy: add a CoMutex to the BlockCopyTask list

[Emanuele Giuseppe Esposito]

Because the list of tasks is only modified by coroutine
functions, add a CoMutex in order to protect the list of tasks.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/block-copy.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index 03df50a354..e785ac57e0 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -65,7 +65,9 @@ typedef struct BlockCopyTask {
     BlockCopyState *s;
     BlockCopyCallState *call_state;
 
-    /* State */
+    /* State. These fields are protected by the tasks_lock
+     * in BlockCopyState
+     */
     int64_t offset;
     int64_t bytes;
     bool zeroes;
@@ -95,6 +97,8 @@ typedef struct BlockCopyState {
     bool use_copy_range;
     int64_t copy_size;
     uint64_t len;
+
+    CoMutex tasks_lock;
     QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls */
     QLIST_HEAD(, BlockCopyCallState) calls;
 
@@ -124,6 +128,7 @@ typedef struct BlockCopyState {
     RateLimit rate_limit;
 } BlockCopyState;
 
+/* Called with lock held */
 static BlockCopyTask *find_conflicting_task(BlockCopyState *s,
                                             int64_t offset, int64_t bytes)
 {
@@ -145,13 +150,19 @@ static BlockCopyTask *find_conflicting_task(BlockCopyState *s,
 static bool coroutine_fn block_copy_wait_one(BlockCopyState *s, int64_t offset,
                                              int64_t bytes)
 {
-    BlockCopyTask *task = find_conflicting_task(s, offset, bytes);
+    BlockCopyTask *task;
+
+    qemu_co_mutex_lock(&s->tasks_lock);
+    task = find_conflicting_task(s, offset, bytes);
 
     if (!task) {
+        qemu_co_mutex_unlock(&s->tasks_lock);
         return false;
     }
 
-    qemu_co_queue_wait(&task->wait_queue, NULL);
+    qemu_co_queue_wait(&task->wait_queue, &s->tasks_lock);
+
+    qemu_co_mutex_unlock(&s->tasks_lock);
 
     return true;
 }
@@ -178,7 +189,9 @@ static BlockCopyTask *coroutine_fn block_copy_task_create(BlockCopyState *s,
     bytes = QEMU_ALIGN_UP(bytes, s->cluster_size);
 
     /* region is dirty, so no existent tasks possible in it */
+    qemu_co_mutex_lock(&s->tasks_lock);
     assert(!find_conflicting_task(s, offset, bytes));
+    qemu_co_mutex_unlock(&s->tasks_lock);
 
     bdrv_reset_dirty_bitmap(s->copy_bitmap, offset, bytes);
     s->in_flight_bytes += bytes;
@@ -192,8 +205,9 @@ static BlockCopyTask *coroutine_fn block_copy_task_create(BlockCopyState *s,
         .bytes = bytes,
     };
     qemu_co_queue_init(&task->wait_queue);
+    qemu_co_mutex_lock(&s->tasks_lock);
     QLIST_INSERT_HEAD(&s->tasks, task, list);
-
+    qemu_co_mutex_unlock(&s->tasks_lock);
     return task;
 }
 
@@ -297,6 +311,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
     }
 
     QLIST_INIT(&s->tasks);
+    qemu_co_mutex_init(&s->tasks_lock);
     QLIST_INIT(&s->calls);
 
     return s;
-- 
2.30.2

[RFC PATCH 3/3] block-copy: add CoMutex lock for BlockCopyCallState list

[Emanuele Giuseppe Esposito]

Use the same tasks_lock, since the calls list is just used
for debug purposes.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/block-copy.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c
index e785ac57e0..555f5fb747 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -310,10 +310,9 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
         s->copy_size = MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER);
     }
 
-    QLIST_INIT(&s->tasks);
     qemu_co_mutex_init(&s->tasks_lock);
+    QLIST_INIT(&s->tasks);
     QLIST_INIT(&s->calls);
-
     return s;
 }
 
@@ -712,7 +711,9 @@ static int coroutine_fn block_copy_common(BlockCopyCallState *call_state)
 {
     int ret;
 
+    qemu_co_mutex_lock(&call_state->s->tasks_lock);
     QLIST_INSERT_HEAD(&call_state->s->calls, call_state, list);
+    qemu_co_mutex_unlock(&call_state->s->tasks_lock);
 
     do {
         ret = block_copy_dirty_clusters(call_state);
@@ -739,7 +740,9 @@ static int coroutine_fn block_copy_common(BlockCopyCallState *call_state)
         call_state->cb(call_state->cb_opaque);
     }
 
+    qemu_co_mutex_lock(&call_state->s->tasks_lock);
     QLIST_REMOVE(call_state, list);
+    qemu_co_mutex_unlock(&call_state->s->tasks_lock);
 
     return ret;
 }
-- 
2.30.2

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

[Vladimir Sementsov-Ogievskiy]

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

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

[Emanuele Giuseppe Esposito]

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

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

[Vladimir Sementsov-Ogievskiy]

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

Re: [RFC PATCH 0/3] block-copy: lock tasks and calls list

[Vladimir Sementsov-Ogievskiy]

20.04.2021 13:04, Emanuele Giuseppe Esposito wrote:
> This serie of patches continues Paolo's series on making the
> block layer thread safe. Add a CoMutex lock for both tasks and
> calls list present in block/block-copy.c
> 

I think, we need more information about what kind of thread-safety we want. Should the whole interface of block-copy be thread safe? Or only part of it? What is going to be shared between different threads? Which functions will be called concurrently from different threads? This should be documented in include/block/block-copy.h.

What I see here, is that some things are protected by mutex.. Some things not. What became thread-safe?

For example, in block_copy_dirty_clusters(), we modify task fields without any mutex held:

  block_copy_task_shrink doesn't take mutex.
  task->zeroes is set without mutex as well

Still all these accesses are done when task is already added to the list.

Looping in block_copy_common() is not thread safe as well.

You also forget to protect QLIST_REMOVE() call in block_copy_task_end()..

Next, block-copy uses co-shared-resource API, which is not thread-safe (as it is directly noted in include/qemu/co-shared-resource.h).

Same thing is block/aio_task API, which is not thread-safe too.

So, we should bring thread-safety first to these smaller helper APIs.

-- 
Best regards,
Vladimir

Re: [RFC PATCH 0/3] block-copy: lock tasks and calls list

[Paolo Bonzini]

On 20/04/21 15:12, Vladimir Sementsov-Ogievskiy wrote:
> 20.04.2021 13:04, Emanuele Giuseppe Esposito wrote:
>> This serie of patches continues Paolo's series on making the
>> block layer thread safe. Add a CoMutex lock for both tasks and
>> calls list present in block/block-copy.c
>>
> 
> I think, we need more information about what kind of thread-safety we 
> want. Should the whole interface of block-copy be thread safe? Or only 
> part of it? What is going to be shared between different threads? Which 
> functions will be called concurrently from different threads? This 
> should be documented in include/block/block-copy.h.

I guess all of it.  So more state fields should be identified in 
BlockCopyState, especially in_flight_bytes.  ProgressMeter and 
SharedResource should be made thread-safe on their own, just like the 
patch I posted for RateLimit.

> What I see here, is that some things are protected by mutex.. Some 
> things not. What became thread-safe?
> 
> For example, in block_copy_dirty_clusters(), we modify task fields 
> without any mutex held:
> 
>   block_copy_task_shrink doesn't take mutex.
>   task->zeroes is set without mutex as well

Agreed, these are bugs in the series.

> Still all these accesses are done when task is already added to the list.
> 
> Looping in block_copy_common() is not thread safe as well.

That one should be mostly safe, because only one coroutine ever writes 
to all fields except cancelled.  cancelled should be accessed with 
qatomic_read/qatomic_set, but there's also the problem that coroutine 
sleep/wake APIs are hard to use in a thread-safe manner (which affects 
block_copy_kick).  This is a different topic and it is something I'm 
working on,

> You also forget to protect QLIST_REMOVE() call in block_copy_task_end()..
> 
> Next, block-copy uses co-shared-resource API, which is not thread-safe 
> (as it is directly noted in include/qemu/co-shared-resource.h).
> 
> Same thing is block/aio_task API, which is not thread-safe too.
>
> So, we should bring thread-safety first to these smaller helper APIs.

Good point.  Emanuele, can you work on ProgressMeter and SharedResource? 
  AioTaskPool can also be converted to just use CoQueue instead of 
manually waking up coroutines.

Re: [RFC PATCH 0/3] block-copy: lock tasks and calls list

[Vladimir Sementsov-Ogievskiy]

21.04.2021 11:38, Paolo Bonzini wrote:
> On 20/04/21 15:12, Vladimir Sementsov-Ogievskiy wrote:
>> 20.04.2021 13:04, Emanuele Giuseppe Esposito wrote:
>>> This serie of patches continues Paolo's series on making the
>>> block layer thread safe. Add a CoMutex lock for both tasks and
>>> calls list present in block/block-copy.c
>>>
>>
>> I think, we need more information about what kind of thread-safety we want. Should the whole interface of block-copy be thread safe? Or only part of it? What is going to be shared between different threads? Which functions will be called concurrently from different threads? This should be documented in include/block/block-copy.h.
> 
> I guess all of it.  So more state fields should be identified in BlockCopyState, especially in_flight_bytes.  ProgressMeter and SharedResource should be made thread-safe on their own, just like the patch I posted for RateLimit.
> 
>> What I see here, is that some things are protected by mutex.. Some things not. What became thread-safe?
>>
>> For example, in block_copy_dirty_clusters(), we modify task fields without any mutex held:
>>
>>   block_copy_task_shrink doesn't take mutex.
>>   task->zeroes is set without mutex as well
> 
> Agreed, these are bugs in the series.
> 
>> Still all these accesses are done when task is already added to the list.
>>
>> Looping in block_copy_common() is not thread safe as well.
> 
> That one should be mostly safe, because only one coroutine ever writes to all fields except cancelled.  cancelled should be accessed with qatomic_read/qatomic_set, but there's also the problem that coroutine sleep/wake APIs are hard to use in a thread-safe manner (which affects block_copy_kick).  This is a different topic and it is something I'm working on,
> 
>> You also forget to protect QLIST_REMOVE() call in block_copy_task_end()..
>>
>> Next, block-copy uses co-shared-resource API, which is not thread-safe (as it is directly noted in include/qemu/co-shared-resource.h).
>>
>> Same thing is block/aio_task API, which is not thread-safe too.
>>
>> So, we should bring thread-safety first to these smaller helper APIs.
> 
> Good point.  Emanuele, can you work on ProgressMeter and SharedResource? AioTaskPool can also be converted to just use CoQueue instead of manually waking up coroutines.
> 

That would be great.

I have one more question in mind:

Is it effective to use CoMutex here? We are protecting only some fast manipulations with data, not io path or something like that. Will simple QemuMutex work better? Even if CoMutex doesn't have any overhead, I don't think than if thread A wants to modify task list, but mutex is held by thread B (for similar thing), there is a reason for thread A to yield and do some other things: it can just wait several moments on mutex while B is modifying task list..

-- 
Best regards,
Vladimir

Re: [RFC PATCH 0/3] block-copy: lock tasks and calls list

[Paolo Bonzini]

On 21/04/21 10:53, Vladimir Sementsov-Ogievskiy wrote:
>>
>> Good point. Emanuele, can you work on ProgressMeter and 
>> SharedResource? AioTaskPool can also be converted to just use CoQueue 
>> instead of manually waking up coroutines.
>>
> 
> That would be great.
> 
> I have one more question in mind:
> 
> Is it effective to use CoMutex here? We are protecting only some fast 
> manipulations with data, not io path or something like that. Will simple 
> QemuMutex work better? Even if CoMutex doesn't have any overhead, I 
> don't think than if thread A wants to modify task list, but mutex is 
> held by thread B (for similar thing), there is a reason for thread A to 
> yield and do some other things: it can just wait several moments on 
> mutex while B is modifying task list..

Indeed even CoQueue primitives count as simple manipulation of data, 
because they unlock/lock the mutex while the coroutine sleeps.  So 
you're right that it would be okay to use QemuMutex as well

The block copy code that Emanuele has touched so far is all coroutine 
based.  I like using CoMutex when that is the case, because it says 
implicitly "the monitor is not involved".  But we need to see what it 
will be like when the patches are complete.

Rate limiting ends up being called by the monitor, but it will have its 
own QemuMutex so it's fine.  What's left is cancellation and 
block_copy_kick; I think that we can make qemu_co_sleep thread-safe with 
an API similar to Linux's prepare_to_wait, so a QemuMutex wouldn't be 
needed there either.

Paolo