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

[Vladimir Sementsov-Ogievskiy]

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