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

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Paolo Bonzini <pbonzini@redhat.com>,
	Emanuele Giuseppe Esposito <eesposit@redhat.com>,
	qemu-block@nongnu.org
Cc: John Snow <jsnow@redhat.com>, Kevin Wolf <kwolf@redhat.com>,
	Max Reitz <mreitz@redhat.com>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	qemu-devel@nongnu.org
Subject: Re: [PATCH v4 6/6] block-copy: atomic .cancelled and .finished fields in BlockCopyCallState
Date: Tue, 22 Jun 2021 13:39:55 +0300	[thread overview]
Message-ID: <5e019d88-3551-4a08-6a67-e0699dd4f72e@virtuozzo.com> (raw)
In-Reply-To: <c8b4e9b9-15a6-3ae4-cb8f-5289a221dcbe@redhat.com>

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

-- 
Best regards,
Vladimir