Re: [Qemu-devel] [PATCH 16/19] block: protect modification of dirty bitmaps with a mutex

2017-06-27 Thread Paolo Bonzini
On 27/06/2017 17:32, Vladimir Sementsov-Ogievskiy wrote: - we protect inserting into list from other threads, but what prevent creating bitmap with the same name from other thread after bdrv_find_dirty_bitmap() and before bdrv_dirty_bitmaps_lock() ? >>> It's like a read-w

Re: [Qemu-devel] [PATCH 16/19] block: protect modification of dirty bitmaps with a mutex

2017-06-27 Thread Vladimir Sementsov-Ogievskiy
27.06.2017 18:31, Vladimir Sementsov-Ogievskiy wrote: 27.06.2017 17:26, Paolo Bonzini wrote: On 27/06/2017 16:20, Vladimir Sementsov-Ogievskiy wrote: I'm likely not right, but for me introducing this mutex looks like dirty bitmaps may be accessed from concurrent threads. So for me it looks st

Re: [Qemu-devel] [PATCH 16/19] block: protect modification of dirty bitmaps with a mutex

2017-06-27 Thread Vladimir Sementsov-Ogievskiy
27.06.2017 17:26, Paolo Bonzini wrote: On 27/06/2017 16:20, Vladimir Sementsov-Ogievskiy wrote: I'm likely not right, but for me introducing this mutex looks like dirty bitmaps may be accessed from concurrent threads. So for me it looks strange that only accessors are protected by the mutex and

Re: [Qemu-devel] [PATCH 16/19] block: protect modification of dirty bitmaps with a mutex

2017-06-27 Thread Paolo Bonzini
On 27/06/2017 16:43, Vladimir Sementsov-Ogievskiy wrote: >> The write side is invoked under the 'big QEMU lock' so there cannot be >> two concurrent writes. >> >> A bitmap can be written to after bdrv_find_dirty_bitmap returns, but >> only if _you_ tell another thread about the bitmap you've just

Re: [Qemu-devel] [PATCH 16/19] block: protect modification of dirty bitmaps with a mutex

2017-06-27 Thread Vladimir Sementsov-Ogievskiy
27.06.2017 17:26, Paolo Bonzini wrote: On 27/06/2017 16:20, Vladimir Sementsov-Ogievskiy wrote: I'm likely not right, but for me introducing this mutex looks like dirty bitmaps may be accessed from concurrent threads. So for me it looks strange that only accessors are protected by the mutex and

Re: [Qemu-devel] [PATCH 16/19] block: protect modification of dirty bitmaps with a mutex

2017-06-27 Thread Paolo Bonzini
On 27/06/2017 16:20, Vladimir Sementsov-Ogievskiy wrote: > I'm likely not right, but for me introducing this mutex looks like dirty > bitmaps may be accessed from concurrent threads. So for me it looks > strange that only accessors are protected by the mutex and not the whole > transactions. The

Re: [Qemu-devel] [PATCH 16/19] block: protect modification of dirty bitmaps with a mutex

2017-06-27 Thread Vladimir Sementsov-Ogievskiy
27.06.2017 16:58, Paolo Bonzini wrote: On 27/06/2017 15:51, Vladimir Sementsov-Ogievskiy wrote: bdrv_create_dirty_bitmap(...) bdrv_dirty_bitmaps_lock(bs) bitmap = bdrv_find_dirty_bitmap(bs, name) bdrv_dirty_bitmaps_unlock(bs) - because we can't now trust the pointer returned by bdrv_crea

Re: [Qemu-devel] [PATCH 16/19] block: protect modification of dirty bitmaps with a mutex

2017-06-27 Thread Paolo Bonzini
On 27/06/2017 15:51, Vladimir Sementsov-Ogievskiy wrote: >>> >>> bdrv_create_dirty_bitmap(...) >>> >>> bdrv_dirty_bitmaps_lock(bs) >>> >>> bitmap = bdrv_find_dirty_bitmap(bs, name) >>> >>> >>> >>> bdrv_dirty_bitmaps_unlock(bs) >>> >>> - because we can't now trust the pointer returned by >>> bdr

Re: [Qemu-devel] [PATCH 16/19] block: protect modification of dirty bitmaps with a mutex

2017-06-27 Thread Vladimir Sementsov-Ogievskiy
27.06.2017 15:52, Paolo Bonzini wrote: On 27/06/2017 11:47, Vladimir Sementsov-Ogievskiy wrote: bdrv_enable_dirty_bitmap - writes to the list, it changes 'disabled' field. So it requires both BQL and dirty_bitmap_mutex? But the comment says only about BQL. This one is interesting. There could

Re: [Qemu-devel] [PATCH 16/19] block: protect modification of dirty bitmaps with a mutex

2017-06-27 Thread Paolo Bonzini
On 27/06/2017 11:47, Vladimir Sementsov-Ogievskiy wrote: > bdrv_enable_dirty_bitmap - writes to the list, it changes 'disabled' > field. So it requires both BQL and dirty_bitmap_mutex? But the comment > says only about BQL. This one is interesting. There could be a concurrent call to bdrv_set_d

Re: [Qemu-devel] [PATCH 16/19] block: protect modification of dirty bitmaps with a mutex

2017-06-27 Thread Vladimir Sementsov-Ogievskiy
27.06.2017 12:27, Paolo Bonzini wrote: On 27/06/2017 11:07, Vladimir Sementsov-Ogievskiy wrote: 26.06.2017 19:54, Paolo Bonzini wrote: On 26/06/2017 18:07, Vladimir Sementsov-Ogievskiy wrote: HI! One question here, should not 'bdrv_undo_clear_dirty_bitmap' be under lock too? Any call to dir

Re: [Qemu-devel] [PATCH 16/19] block: protect modification of dirty bitmaps with a mutex

2017-06-27 Thread Paolo Bonzini
On 27/06/2017 11:07, Vladimir Sementsov-Ogievskiy wrote: > 26.06.2017 19:54, Paolo Bonzini wrote: >> >> On 26/06/2017 18:07, Vladimir Sementsov-Ogievskiy wrote: >>> HI! >>> >>> One question here, should not 'bdrv_undo_clear_dirty_bitmap' be under >>> lock too? >> Any call to dirty bitmap function

Re: [Qemu-devel] [PATCH 16/19] block: protect modification of dirty bitmaps with a mutex

2017-06-27 Thread Vladimir Sementsov-Ogievskiy
26.06.2017 19:54, Paolo Bonzini wrote: On 26/06/2017 18:07, Vladimir Sementsov-Ogievskiy wrote: HI! One question here, should not 'bdrv_undo_clear_dirty_bitmap' be under lock too? Any call to dirty bitmap functions between bdrv_clear_dirty_bitmap and bdrv_undo_clear_dirty_bitmap is problemati

Re: [Qemu-devel] [PATCH 16/19] block: protect modification of dirty bitmaps with a mutex

2017-06-26 Thread Paolo Bonzini
On 26/06/2017 18:07, Vladimir Sementsov-Ogievskiy wrote: > HI! > > One question here, should not 'bdrv_undo_clear_dirty_bitmap' be under > lock too? Any call to dirty bitmap functions between bdrv_clear_dirty_bitmap and bdrv_undo_clear_dirty_bitmap is problematic anyway, so bdrv_clear_dirty_bit

Re: [Qemu-devel] [PATCH 16/19] block: protect modification of dirty bitmaps with a mutex

2017-06-26 Thread Vladimir Sementsov-Ogievskiy
HI! One question here, should not 'bdrv_undo_clear_dirty_bitmap' be under lock too? 05.06.2017 15:39, Paolo Bonzini wrote: Reviewed-by: Stefan Hajnoczi Signed-off-by: Paolo Bonzini --- block/dirty-bitmap.c | 68 ++-- block/mirror.c