Re: [PATCH v4 6/6] block-copy: atomic .cancelled and .finished fields in BlockCopyCallState

[Vladimir Sementsov-Ogievskiy]

14.06.2021 10:33, Emanuele Giuseppe Esposito wrote:
> By adding acquire/release pairs, we ensure that .ret and .error_is_read
> fields are written by block_copy_dirty_clusters before .finished is true.

And that they are read by API user after .finished is true.

> The atomic here are necessary because the fields are concurrently modified
> also outside coroutines.

To be honest, finished is modified only in coroutine. And read outside.

> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block/block-copy.c | 33 ++++++++++++++++++---------------
>   1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/block/block-copy.c b/block/block-copy.c
> index 6416929abd..5348e1f61b 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -53,14 +53,14 @@ typedef struct BlockCopyCallState {
>       Coroutine *co;
>   
>       /* State */
> -    bool finished;
> +    bool finished; /* atomic */

So, logic around finished:

Thread of block_copy does:
0. finished is false
1. tasks set ret and error_is_read
2. qatomic_store_release finished -> true
3. after that point ret and error_is_read are not modified

Other threads can:

- qatomic_read finished, just to check are we finished or not

- if finished, can read ret and error_is_read safely. If you not sure that 
block-copy finished, use qatomic_load_acquire() of finished first, to be sure 
that you read ret and error_is_read AFTER finished read and checked to be true.

>       QemuCoSleep sleep; /* TODO: protect API with a lock */
>   
>       /* To reference all call states from BlockCopyState */
>       QLIST_ENTRY(BlockCopyCallState) list;
>   
>       /* OUT parameters */
> -    bool cancelled;
> +    bool cancelled; /* atomic */

Logic around cancelled is simpler:

- false at start

- qatomic_read is allowed from any thread

- qatomic_write to true is allowed from any thread

- never write to false

Note that cancelling and finishing are racy. User can cancel block-copy that's 
already finished. We probably may improve change it, but I'm not sure that it 
worth doing. Still, maybe leave some comment in API documentation.

>       /* Fields protected by lock in BlockCopyState */
>       bool error_is_read;
>       int ret;
> @@ -650,7 +650,8 @@ block_copy_dirty_clusters(BlockCopyCallState *call_state)
>       assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
>       assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
>   
> -    while (bytes && aio_task_pool_status(aio) == 0 && !call_state->cancelled) {
> +    while (bytes && aio_task_pool_status(aio) == 0 &&
> +           !qatomic_read(&call_state->cancelled)) {
>           BlockCopyTask *task;
>           int64_t status_bytes;
>   
> @@ -761,7 +762,7 @@ static int coroutine_fn block_copy_common(BlockCopyCallState *call_state)
>       do {
>           ret = block_copy_dirty_clusters(call_state);
>   
> -        if (ret == 0 && !call_state->cancelled) {
> +        if (ret == 0 && !qatomic_read(&call_state->cancelled)) {
>               WITH_QEMU_LOCK_GUARD(&s->lock) {
>                   /*
>                    * Check that there is no task we still need to
> @@ -792,9 +793,9 @@ static int coroutine_fn 
> block_copy_common(BlockCopyCallState *call_state)
>            * 2. We have waited for some intersecting block-copy request
>            *    It may have failed and produced new dirty bits.
>            */
> -    } while (ret > 0 && !call_state->cancelled);
> +    } while (ret > 0 && !qatomic_read(&call_state->cancelled));
>   
> -    call_state->finished = true;
> +    qatomic_store_release(&call_state->finished, true);

so, all writes to ret and error_is_read are finished to this point.

>   
>       if (call_state->cb) {
>           call_state->cb(call_state->cb_opaque);
> @@ -857,35 +858,37 @@ void block_copy_call_free(BlockCopyCallState *call_state)
>           return;
>       }
>   
> -    assert(call_state->finished);
> +    assert(qatomic_load_acquire(&call_state->finished));

Here we don't need load_aquire, as we don't read other fields. qatomic_read is 
enough.

>       g_free(call_state);
>   }
>   
>   bool block_copy_call_finished(BlockCopyCallState *call_state)
>   {
> -    return call_state->finished;
> +    return qatomic_load_acquire(&call_state->finished);

and here

>   }
>   
>   bool block_copy_call_succeeded(BlockCopyCallState *call_state)
>   {
> -    return call_state->finished && !call_state->cancelled &&
> -        call_state->ret == 0;
> +    return qatomic_load_acquire(&call_state->finished) &&
> +           !qatomic_read(&call_state->cancelled) &&
> +           call_state->ret == 0;

Here qatomic_load_acquire() is reasonable: it protects read of ->ret

>   }
>   
>   bool block_copy_call_failed(BlockCopyCallState *call_state)
>   {
> -    return call_state->finished && !call_state->cancelled &&
> -        call_state->ret < 0;
> +    return qatomic_load_acquire(&call_state->finished) &&
> +           !qatomic_read(&call_state->cancelled) &&
> +           call_state->ret < 0;

Here reasonable.

>   }
>   
>   bool block_copy_call_cancelled(BlockCopyCallState *call_state)
>   {
> -    return call_state->cancelled;
> +    return qatomic_read(&call_state->cancelled);
>   }
>   
>   int block_copy_call_status(BlockCopyCallState *call_state, bool *error_is_read)
>   {
> -    assert(call_state->finished);
> +    assert(qatomic_load_acquire(&call_state->finished));

Hmm. Here qatomic_load_acquire protects nothing (assertion will crash if not 
yet finished anyway). So, caller is double sure that block-copy is finished.

Also it's misleading: if we think that it do some protection, we are doing 
wrong thing: assertions may be simply compiled out, we can't rely on statements 
inside assert() to be executed.

So, let's use simple qatomic_read here too.

>       if (error_is_read) {
>           *error_is_read = call_state->error_is_read;
>       }
> @@ -894,7 +897,7 @@ int block_copy_call_status(BlockCopyCallState *call_state, 
> bool *error_is_read)
>   
>   void block_copy_call_cancel(BlockCopyCallState *call_state)
>   {
> -    call_state->cancelled = true;
> +    qatomic_set(&call_state->cancelled, true);
>       block_copy_kick(call_state);
>   }
>   

Uhh :)

Ok, that looks close too. Or in other words, I feel that I have good enough 
understanding of all the thread-safe logic that you have implemented :)

--
Best regards,
Vladimir