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

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

On 19/06/2021 22:06, Vladimir Sementsov-Ogievskiy wrote:
> 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.

So what you say makes sense, the only thing that I wonder is: wouldn't 
it be better to have the acquire without assertion (or assert 
afterwards), just to be sure that we delete when finished is true?

[...]

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

Same applies here.

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

Good! :)

Emanuele