Re: [PATCH v2 4/7] block-copy: add a CoMutex to the BlockCopyTask list

[Vladimir Sementsov-Ogievskiy]

18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:
> Because the list of tasks is only modified by coroutine
> functions, add a CoMutex in order to protect them.
>
> Use the same mutex to protect also BlockCopyState in_flight_bytes
> field to avoid adding additional syncronization primitives.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block/block-copy.c | 55 +++++++++++++++++++++++++++++-----------------
>   1 file changed, 35 insertions(+), 20 deletions(-)
>
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 2e610b4142..3a949fab64 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -83,7 +83,7 @@ typedef struct BlockCopyTask {
>        */
>       bool zeroes;
>   
> -    /* State */
> +    /* State. Protected by tasks_lock */
>       CoQueue wait_queue; /* coroutines blocked on this task */
>   
>       /* To reference all call states from BlockCopyState */
> @@ -106,8 +106,9 @@ typedef struct BlockCopyState {
>       BdrvChild *target;
>   
>       /* State */
> -    int64_t in_flight_bytes;
> +    int64_t in_flight_bytes; /* protected by tasks_lock */

only this field is protected? or the whole State section?

>       BlockCopyMethod method;
> +    CoMutex tasks_lock;
>       QLIST_HEAD(, BlockCopyTask) tasks; /* All tasks from all block-copy calls 
> */
>       QLIST_HEAD(, BlockCopyCallState) calls;
>       /* State fields that use a thread-safe API */
> @@ -142,8 +143,10 @@ typedef struct BlockCopyState {
>       bool skip_unallocated;
>   } BlockCopyState;
>   
> -static BlockCopyTask *find_conflicting_task(BlockCopyState *s,
> -                                            int64_t offset, int64_t bytes)
> +/* Called with lock held */
> +static BlockCopyTask *find_conflicting_task_locked(BlockCopyState *s,
> +                                                   int64_t offset,
> +                                                   int64_t bytes)
>   {
>       BlockCopyTask *t;
>   
> @@ -163,13 +166,16 @@ 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_LOCK_GUARD(&s->tasks_lock);
> +    task = find_conflicting_task_locked(s, offset, bytes);
>   
>       if (!task) {
>           return false;
>       }
>   
> -    qemu_co_queue_wait(&task->wait_queue, NULL);
> +    qemu_co_queue_wait(&task->wait_queue, &s->tasks_lock);
>   
>       return true;
>   }
> @@ -213,11 +219,7 @@ static coroutine_fn BlockCopyTask 
> *block_copy_task_create(BlockCopyState *s,
>       assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
>       bytes = QEMU_ALIGN_UP(bytes, s->cluster_size);
>   

Looking at block_copy_task_create():

First, !bdrv_dirty_bitmap_next_dirty_area() doesn't take bitmaps lock, so it's 
not protected at all.

Next, even if we take bitmaps lock in bdrv_dirty_bitmap_next_dirty_area() or 
around it, it doesn't bring thread-safety to block_copy_task_create():

Imagine block_copy_task_create() is called from two threads simultaneously. Both calls 
will get same dirty area and create equal tasks. Then, 
"assert(!find_conflicting_task_locked(s, offset, bytes));" will crash.


> -    /* region is dirty, so no existent tasks possible in it */
> -    assert(!find_conflicting_task(s, offset, bytes));
> -
>       bdrv_reset_dirty_bitmap(s->copy_bitmap, offset, bytes);
> -    s->in_flight_bytes += bytes;
>   
>       task = g_new(BlockCopyTask, 1);
>       *task = (BlockCopyTask) {
> @@ -228,7 +230,13 @@ static coroutine_fn BlockCopyTask 
> *block_copy_task_create(BlockCopyState *s,
>           .bytes = bytes,
>       };
>       qemu_co_queue_init(&task->wait_queue);
> -    QLIST_INSERT_HEAD(&s->tasks, task, list);
> +
> +    WITH_QEMU_LOCK_GUARD(&s->tasks_lock) {
> +        s->in_flight_bytes += bytes;
> +        /* region is dirty, so no existent tasks possible in it */
> +        assert(!find_conflicting_task_locked(s, offset, bytes));
> +        QLIST_INSERT_HEAD(&s->tasks, task, list);
> +    }
>   
>       return task;
>   }
> @@ -249,25 +257,29 @@ static void coroutine_fn 
> block_copy_task_shrink(BlockCopyTask *task,
>   
>       assert(new_bytes > 0 && new_bytes < task->bytes);
>   
> -    task->s->in_flight_bytes -= task->bytes - new_bytes;
>       bdrv_set_dirty_bitmap(task->s->copy_bitmap,
>                             task->offset + new_bytes, task->bytes - new_bytes);

Then look here. Imagine, immediately after bdrv_set_dirty_bitmap() in parallel 
thread the new task is created, which consumes these new dirty bits. Again, it 
will find conflicting task (this one, as task->bytes is not modified yet) and 
crash.

>   
> -    task->bytes = new_bytes;
> -    qemu_co_queue_restart_all(&task->wait_queue);
> +    WITH_QEMU_LOCK_GUARD(&task->s->tasks_lock) {
> +        task->s->in_flight_bytes -= task->bytes - new_bytes;
> +        task->bytes = new_bytes;
> +        qemu_co_queue_restart_all(&task->wait_queue);
> +    }
>   }
>   
>   static void coroutine_fn block_copy_task_end(BlockCopyTask *task, int ret)
>   {
> -    task->s->in_flight_bytes -= task->bytes;
>       if (ret < 0) {
>           bdrv_set_dirty_bitmap(task->s->copy_bitmap, task->offset, 
> task->bytes);
>       }
> -    QLIST_REMOVE(task, list);
> -    progress_set_remaining(task->s->progress,
> -                           bdrv_get_dirty_count(task->s->copy_bitmap) +
> -                           task->s->in_flight_bytes);
> -    qemu_co_queue_restart_all(&task->wait_queue);
> +    WITH_QEMU_LOCK_GUARD(&task->s->tasks_lock) {
> +        task->s->in_flight_bytes -= task->bytes;
> +        QLIST_REMOVE(task, list);
> +        progress_set_remaining(task->s->progress,
> +                               bdrv_get_dirty_count(task->s->copy_bitmap) +
> +                               task->s->in_flight_bytes);
> +        qemu_co_queue_restart_all(&task->wait_queue);
> +    }
>   }
>   
>   void block_copy_state_free(BlockCopyState *s)
> @@ -336,6 +348,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, 
> BdrvChild *target,
>       }
>   
>       ratelimit_init(&s->rate_limit);
> +    qemu_co_mutex_init(&s->tasks_lock);
>       QLIST_INIT(&s->tasks);
>       QLIST_INIT(&s->calls);
>   
> @@ -586,9 +599,11 @@ int64_t block_copy_reset_unallocated(BlockCopyState *s,
>   
>       if (!ret) {
>           bdrv_reset_dirty_bitmap(s->copy_bitmap, offset, bytes);
> +        qemu_co_mutex_lock(&s->tasks_lock);
>           progress_set_remaining(s->progress,
>                                  bdrv_get_dirty_count(s->copy_bitmap) +
>                                  s->in_flight_bytes);
> +        qemu_co_mutex_unlock(&s->tasks_lock);
>       }
>   
>       *count = bytes;

--
Best regards,
Vladimir