Re: [PATCH v2 7/7] block-copy: protect BlockCopyState .method fields

[Paolo Bonzini]

On 25/05/21 13:00, Vladimir Sementsov-Ogievskiy wrote:
> Hmm. OK, let me think:
>
> First look at block_copy_do_copy(). It's called only from 
> block_copy_task_entry. block_copy_task_entry() has 
> mutex-critical-section anyway around handling return value. That means 
> that we can simply move s->method modification logic to this already 
> existing critical section.
>
> Next, block_copy_chunk_size() is called only from 
> block_copy_task_create(), where we should have critical section too.

block_copy_do_copy would have to release the mutex around the 
reads/writes (including the bdrv_co_pwrite_zeroes that has nothing to do 
with s->method).

There's also the "goto out" if the copy-range operation succeeds, which 
makes things a bit more complicated.  The goto suggests using 
QEMU_WITH_LOCK_GUARD, but that doesn't work too well either, because 
there are two accesses (one before the bdrv_co_copy_range and one after).

So I understand why you want to avoid atomics and I agree that they are 
more complicated than the other solutions, on the other hand I think 
this patch is the simplest code.

Paolo

> So, no reason for atomics, as we already have critical sections.