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

[Vladimir Sementsov-Ogievskiy]

25.05.2021 13:18, Emanuele Giuseppe Esposito wrote:
> On 21/05/2021 19:10, Vladimir Sementsov-Ogievskiy wrote:
> > 18.05.2021 13:07, Emanuele Giuseppe Esposito wrote:
> > > With tasks and calls lock protecting all State fields,
> > > .method is the last BlockCopyState field left unprotected.
> > > Set it as atomic.
> > >
> > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> >
> > OK, in 06 some things are out of coroutine. Here could we just reuse mutex?
> >
> > I believe, that we don't need any kind of protection for .method inside 
> > block_copy_state_new(), as it's just a creation and initialization of new 
> > structure.
>
> I agree here, will remove the atomic_set in block_copy_state_new.
> > And other things are called from coroutines. So, seems no reasons for 
> > additional atomic access logic?
>
> But... why should I use a mutex? I think the .method usage is pretty
> straightforward, adding a lock (which one, tasks_lock? does not seem 
> appropriate)

Paolo said patch 05 should go away. So, we have only one mutex. We can name it just 
"lock" and use for all the needs, like in qcow2.

> would just cover also functions that do not need it, since the field is 
> modified in if-else statements (see block_copy_do_copy).
> It looks to me that an atomic here won't hurt, and it's pretty straightforward 
> to understand.
>
> Thank you,
> Emanuele

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.

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


I think it's significant:

Drawbacks of atomics:

1. Code becomes harder to read. Just because instead of simple access to 
variable, we have to call atomic access functions. And the code become the mess 
of different qatomic_* calls.

2. The thread-safety is harder to analyze. You argue that use is 
straightforward: yes, it's obvious that atomic access protect the variable 
itself. But what about the logic? It's the same as questions I asked about 
critical sections in a patch 04. With critical sections things are clear: just 
protect the whole logic with a critical sections and you are sure that no other 
critical section intersects. With atomics you should analyze for example: are 
existing critical sections OK with the fact that the variable may be atomically 
changed by other thread not locking the mutex. It's not a simple question in 
general.

Probable benefits of atomics:

1. Performance.. anything else?

So, if we have some lockless performance-critical mechanism, atomics are 
needed. Still, in general lockless algorithms are a lot trickier and harder to 
understand than simple critical sections. Still, sometimes it worth the 
complexity.

But, if we already have the mutex to protect most of the logic inside some 
subsystem (block-copy for example), it's better to just protect the remaining 
bit of things in the subsystem by same mutex, than to add drawbacks of atomics 
with no reason. Especially when this remaining bit of accesses follows or goes 
directly before existing critical section. I don't believe that in this case 
atomics may bring better performance. I even think, that performance may become 
worse (remember atomic operations are not free, and simple accesses to variable 
may be faster).

And I really doubt, that someone can demonstrate a performance benefit of 
atomic accesses in block-layer. IO operations are a lot longer anyway than all 
these simple variable accesses.

So, I'm against adding atomics just because they won't hurt :)

--
Best regards,
Vladimir