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

2021-06-23 Thread 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

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

2021-06-22 Thread Emanuele Giuseppe Esposito
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

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

2021-06-22 Thread Vladimir Sementsov-Ogievskiy
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

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

2021-06-22 Thread Paolo Bonzini
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

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

2021-06-22 Thread Vladimir Sementsov-Ogievskiy
21.06.2021 12:30, Emanuele Giuseppe Esposito wrote: On 19/06/2021 22:06, Vladimir Sementsov-Ogievskiy wrote: 14.06.2021 10:33, Emanuele Giuseppe Esposito wrote: By adding acquire/release pairs, we ensure that .ret and .error_is_read fields are written by block_copy_dirty_clusters before

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

2021-06-22 Thread 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(_state->finished)); Hmm. Here qatomic_load_acquire protects nothing (assertion will crash if not yet finished anyway). So, caller

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

2021-06-22 Thread Paolo Bonzini
On 19/06/21 22:06, Vladimir Sementsov-Ogievskiy wrote: -    assert(call_state->finished); +    assert(qatomic_load_acquire(_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

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

2021-06-21 Thread Emanuele Giuseppe Esposito
On 19/06/2021 22:06, Vladimir Sementsov-Ogievskiy wrote: 14.06.2021 10:33, Emanuele Giuseppe Esposito wrote: By adding acquire/release pairs, we ensure that .ret and .error_is_read fields are written by block_copy_dirty_clusters before .finished is true. And that they are read by API user

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

2021-06-19 Thread Vladimir Sementsov-Ogievskiy
14.06.2021 10:33, Emanuele Giuseppe Esposito wrote: By adding acquire/release pairs, we ensure that .ret and .error_is_read fields are written by block_copy_dirty_clusters before .finished is true. And that they are read by API user after .finished is true. The atomic here are necessary