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

From: Paolo Bonzini <pbonzini@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	Emanuele Giuseppe Esposito <eesposit@redhat.com>,
	qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, John Snow <jsnow@redhat.com>,
	qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>,
	Max Reitz <mreitz@redhat.com>
Subject: Re: [PATCH v4 6/6] block-copy: atomic .cancelled and .finished fields in BlockCopyCallState
Date: Tue, 22 Jun 2021 10:15:42 +0200	[thread overview]
Message-ID: <c06306f6-e154-ec58-44cc-33d48d8ccdff@redhat.com> (raw)
In-Reply-To: <fcb8849c-bedf-aacc-3831-0239d89af2d6@virtuozzo.com>

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.

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