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

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

On 22/06/2021 12:39, Vladimir Sementsov-Ogievskiy wrote:
> 22.06.2021 13:20, Paolo Bonzini wrote:
>> On 22/06/21 11:36, Vladimir Sementsov-Ogievskiy wrote:
>>>> 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.
>>
>> You can also have a finished job, but get a stale value for 
>> error_is_read or ret.  The issue is not in getting the stale value per 
>> se, but in block_copy_call_status's caller not expecting it.
>>
>> (I understand you agree, but I guess it can be interesting to learn 
>> about this too).
>>
> 
> Hmm. So, do you mean that we can read ret and error_is_read ONLY after 
> explicitly doing load_acquire(finished) and checking that it's true?
> 
> That means that we must do it not in assertion (to not be compiled out):
> 
> bool finished = load_acquire()
> 
> assert(finished);
> 
> ... read reat and error_is_read ...
> 
> 

If I understand correctly, this was what I was trying to say before: 
maybe it's better that we make sure that @finished is set before reading 
@ret and @error_is_read. And because assert can be disabled, we can do 
like you wrote above.

Anyways, let's wait Paolo's answer for this. Once this is ready, I will 
send v5.

Emanuele