Re: [PATCH v2 4/7] block-copy: add a CoMutex to the BlockCopyTask list

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

26.05.2021 17:58, Paolo Bonzini wrote:
> On 25/05/21 12:25, Vladimir Sementsov-Ogievskiy wrote:
>>>> Next, even if we take bitmaps lock in bdrv_dirty_bitmap_next_dirty_area() or around it, it doesn't bring thread-safety to block_copy_task_create():
>>>
>>> The simplest solution here seems to protect bdrv_dirty_bitmap_next_dirty_area and also bdrv_reset_dirty_bitmap with the tasks lock, so that once it is released the bitmap is updated accordingly and from my understanding no other task can get that region.
>>
>> Yes, we just need to protect larger areas by outer lock, to protect the logic, not only structures itself.
> 
> Locks protects data, not code; code must ensure invariants are respected at the end of critical sections.  Here we have both problems:

Hmm. Anyway, if code doesn't work with data that is potentially shared with other threads, it doesn't need any protection. So, I agree.. I just wanted to say that, if we have two data structures A and B, each protected by own lock, it doesn't mean that our code is thread-safe. In your terminology we can say that the whole data (which is a combination of A and B) is not protected by partial locks, we need another lock to protect the combination.

> 
> - it's protecting too little data (the bitmap is not protected).  This is a block/dirty-bitmap.c bug.
> 
> - it's not respecting the invariant that tasks always reflected a superset of what is set in the dirty bitmap.  This is solved by making the critical section larger.
> 



-- 
Best regards,
Vladimir