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

[Vladimir Sementsov-Ogievskiy]

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