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

[Paolo Bonzini]

On 22/06/21 12:39, Vladimir Sementsov-Ogievskiy wrote:
> 22.06.2021 13:20, Paolo Bonzini wrote:
> > On 22/06/21 11:36, Vladimir Sementsov-Ogievskiy wrote:
> > > 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.
>
> 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 ...

In reality you must have synchronized in some other way; that outside 
synchronization outside block-copy.c is what guarantees that the 
assertion does not fail.  The simplest cases are:

- a mutex: "unlock" counts as release, "lock" counts as acquire;

- a bottom half: "schedule" counts as release, running counts as acquire.

Therefore, removing the assertion would work just fine because the 
synchronization would be like this:

   write ret/error_is_read
   write finished
   trigger bottom half or something -->    bottom half runs
                                           read ret/error_is_read

So there is no need at all to read ->finished, much less to load it 
outside the assertion.  At the same time there are two problems with 
"assert(qatomic_read(&call_state->finished))".  Note that they are not 
logic issues, they are maintenance issues.

First, if *there is a bug elsewhere* and the above synchronization 
doesn't happen, it may manifest sometimes as an assertion failure and 
sometimes as a memory reordering.  This is what I was talking about in 
the previous message, and it is probably the last thing that you want 
when debugging; since we're adding asserts defensively, we might as well 
do it well.

Second, if somebody later carelessly changes the function to

  if (qatomic_read(&call_state->finished)) {
      ...
  } else {
      error_setg(...);
  }

that would be broken.  Using qatomic_load_acquire makes the code more 
future-proof against a change like the one above.

Paolo