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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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