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

[Vladimir Sementsov-Ogievskiy]

26.05.2021 17:48, Paolo Bonzini wrote:
> 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).

Hmm, this "goto out" makes no sense actually, and should be removed.. It's a 
mistake or a kind of forgotten thing to refactor after some changes.

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

I think it's better to pass s->method as parameter to block_copy_do_copy. Then 
block_copy_do_copy() doesn't need any kind of synchronization.

In block_copy_task_entry() we'll have the whole logic of handling result of 
block_copy_do_copy (including changing of s->method). And then, only one 
question is:

before calling block_copy_do_copy() we should get s->method. Either by atomic 
operation or under separate critical section.. I'd be OK either way.


It's actually the original idea of block_copy_do_copy() function: do only simple 
copy, don't interact with the state. It's a kind of wrapper on top of bdrv_co<io 
functions>.

So, actually updating s->use_copy_range here was a bad idea.

Note also the comment above block_copy_do_copy():

 " No sync here: nor bitmap neighter intersecting requests handling, only copy."

Somehow, the comment conflicts with introducing synchronization primitives 
inside the function, although it was about another things :))

--
Best regards,
Vladimir