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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

21.06.2021 12:30, Emanuele Giuseppe Esposito wrote:
> 
> 
> 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?
> 

Hmm. I think neither compiler nor processor should reorder read structure field and free() call on the whole structure :)

And anyway for block_copy_call_free() caller is responsible for the structure not being used by other thread.

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

Here I agree with Paolo, assertion works better as written..

So we can just keep it as is.

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


-- 
Best regards,
Vladimir