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

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

22.06.2021 11:15, Paolo Bonzini wrote:
> On 19/06/21 22:06, Vladimir Sementsov-Ogievskiy wrote:
>>>
>>> -    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.
> 
> It does.  If it returns true, you still want the load of finished to happen before the reads that follow.
> 

Hmm.. The worst case if we use just qatomic_read is that assertion will not crash when it actually should. That doesn't break the logic. But that's not good anyway.

OK, I agree, let's keep it.

> Otherwise I agree with your remarks.
> 
> Paolo
> 
>> 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;
>>>       } 
> 


-- 
Best regards,
Vladimir