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

[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-write lock.
>>
>> Ok, finally I understand (not all, but the main idea=), sorry for
>> annoying.
> 
> Is there any example of accessing dirty bitmaps for read not under BQL
> but only under dirty bitmap lock?

Yes, search for bdrv_dirty_bitmap_lock...unlock in block/mirror.c.
There is also block/backup.c, which operates on a frozen bitmap.

Paolo

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

[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
strange that only accessors are protected by the mutex and not the 
whole

transactions.

There must be something else protecting the whole transaction.


One example I've wrote above, other examples from code: are
qmp_block_dirty_bitmap** functions:

bdrv_create_dirty_bitmap() {

bdrv_find_dirty_bitmap()



bdrv_dirty_bitmaps_lock(bs);
QLIST_INSERT_HEAD(>dirty_bitmaps, bitmap, list);
bdrv_dirty_bitmaps_unlock(bs);

}

- 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-write lock.


Ok, finally I understand (not all, but the main idea=), sorry for 
annoying.


Is there any example of accessing dirty bitmaps for read not under BQL 
but only under dirty bitmap lock?






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 created.
If that doesn't happen, the bitmap cannot change.  And it can also
disappear because _your_ thread is the one with the big QEMU lock.

Paolo





--
Best regards,
Vladimir

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

[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 not the whole
transactions.

There must be something else protecting the whole transaction.


One example I've wrote above, other examples from code: are
qmp_block_dirty_bitmap** functions:

bdrv_create_dirty_bitmap() {

bdrv_find_dirty_bitmap()



bdrv_dirty_bitmaps_lock(bs);
QLIST_INSERT_HEAD(>dirty_bitmaps, bitmap, list);
bdrv_dirty_bitmaps_unlock(bs);

}

- 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-write lock.


Ok, finally I understand (not all, but the main idea=), sorry for annoying.



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 created.
If that doesn't happen, the bitmap cannot change.  And it can also
disappear because _your_ thread is the one with the big QEMU lock.

Paolo



--
Best regards,
Vladimir

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

[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 created.
> 
> no, I'm not about touching just created bitmap. I'm about creating
> bitmap with the same name by other thread (unlikely case, but possible).

You can't unless you drop the BQL.

>> If that doesn't happen, the bitmap cannot change.  And it can also
>> disappear because _your_ thread is the one with the big QEMU lock.
> 
> So, if I under BQL, I don't need dirty_bitmap_lock?

Writing to the list requires _both_ BQL and dirty_bitmap_lock.  Write
functions actually have "called with BQL taken" in dirty-bitmap.c,
because dirty-bitmap.c will call dirty_bitmap_lock/unlock itself.

Reading from the list requires one of the two locks.  Such functions
have "called with BQL or dirty_bitmap lock taken".

For reading/writing to the bitmap itself, you need dirty_bitmap_lock.
dirty-bitmap.c can take the lock itself but, there are also functions
named *_locked where the caller takes the lock.

Paolo

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

[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 not the whole
transactions.

There must be something else protecting the whole transaction.


But it doesn't? Before it (qmp_block_dirty_bitmap*) was protected by aio 
context acquire/release...





One example I've wrote above, other examples from code: are
qmp_block_dirty_bitmap** functions:

bdrv_create_dirty_bitmap() {

bdrv_find_dirty_bitmap()



bdrv_dirty_bitmaps_lock(bs);
QLIST_INSERT_HEAD(>dirty_bitmaps, bitmap, list);
bdrv_dirty_bitmaps_unlock(bs);

}

- 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-write lock.

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 created.


no, I'm not about touching just created bitmap. I'm about creating 
bitmap with the same name by other thread (unlikely case, but possible).



If that doesn't happen, the bitmap cannot change.  And it can also
disappear because _your_ thread is the one with the big QEMU lock.



So, if I under BQL, I don't need dirty_bitmap_lock?



Paolo



--
Best regards,
Vladimir

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

[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.

There must be something else protecting the whole transaction.

> One example I've wrote above, other examples from code: are
> qmp_block_dirty_bitmap** functions:
> 
> bdrv_create_dirty_bitmap() {
> 
>bdrv_find_dirty_bitmap()
> 
>
> 
> bdrv_dirty_bitmaps_lock(bs);
>QLIST_INSERT_HEAD(>dirty_bitmaps, bitmap, list);
>bdrv_dirty_bitmaps_unlock(bs);
> 
> }
> 
> - 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-write lock.

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 created.
If that doesn't happen, the bitmap cannot change.  And it can also
disappear because _your_ thread is the one with the big QEMU lock.

Paolo

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

[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_create_dirty_bitmap, as it releases bitmap lock before return.

If you have the big QEMU lock (you do if you are in the QEMU monitor),
you are protected from changes to the list of bitmaps.

Paolo

but you wrote "Writing to the list requires the BQL _and_ the
dirty_bitmap_mutex".

should it be BQL only?

bdrv_dirty_bitmaps_lock/unlock is (should be) called by the functions
that write to the list.  I hope I can post the patch today already.

Paolo



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.


One example I've wrote above, other examples from code: are 
qmp_block_dirty_bitmap** functions:


bdrv_create_dirty_bitmap() {

   bdrv_find_dirty_bitmap()

   

bdrv_dirty_bitmaps_lock(bs);
   QLIST_INSERT_HEAD(>dirty_bitmaps, bitmap, list);
   bdrv_dirty_bitmaps_unlock(bs);

}

- 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() ?



-

qmp_block_dirty_bitmap_clear() {

bitmap = block_dirty_bitmap_lookup()



bdrv_clear_dirty_bitmap()

}

- bdrv_clear_dirty_bitmap is protected by lock, but what prevent 
deleting this bitmap by other thread between block_dirty_bitmap_lookup() 
and bdrv_clear_dirty_bitmap() ?



--- it is not the whole list ---

--
Best regards,
Vladimir

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

[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
>>> bdrv_create_dirty_bitmap, as it releases bitmap lock before return.
>> If you have the big QEMU lock (you do if you are in the QEMU monitor),
>> you are protected from changes to the list of bitmaps.
>>
>> Paolo
> 
> but you wrote "Writing to the list requires the BQL _and_ the
> dirty_bitmap_mutex".
> 
> should it be BQL only?

bdrv_dirty_bitmaps_lock/unlock is (should be) called by the functions
that write to the list.  I hope I can post the patch today already.

Paolo

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

[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 be a concurrent call to
bdrv_set_dirty, indeed.  I'll send a patch.


Also, for example, if I want to create a new bitmap and than somehow
change it, should I do it like this:

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_create_dirty_bitmap, as it releases bitmap lock before return.

If you have the big QEMU lock (you do if you are in the QEMU monitor),
you are protected from changes to the list of bitmaps.

Paolo


but you wrote "Writing to the list requires the BQL _and_ the 
dirty_bitmap_mutex".


should it be BQL only?


--
Best regards,
Vladimir

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

[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_dirty, indeed.  I'll send a patch.

> Also, for example, if I want to create a new bitmap and than somehow
> change it, should I do it like this:
> 
> 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_create_dirty_bitmap, as it releases bitmap lock before return.

If you have the big QEMU lock (you do if you are in the QEMU monitor),
you are protected from changes to the list of bitmaps.

Paolo

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

[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 dirty bitmap functions between bdrv_clear_dirty_bitmap and
bdrv_undo_clear_dirty_bitmap is problematic anyway, so
bdrv_clear_dirty_bitmap really only needs the lock in the !out case;
bdrv_undo_clear_dirty_bitmap is only called when out != NULL.

However, I agree it would be cleaner to add the lock there, too.

Also, you've added comment "Called with BQL taken" both to functions
that calls bdrv_dirty_bitmaps_lock and that do not... What is BQL?

It's the "big QEMU lock", also known as "iothread lock".  The locking policy
is documented in block_int.h:

 /* Writing to the list requires the BQL _and_ the dirty_bitmap_mutex.
  * Reading from the list can be done with either the BQL or the
  * dirty_bitmap_mutex.  Modifying a bitmap only requires
  * dirty_bitmap_mutex.  */
 QemuMutex dirty_bitmap_mutex;
 QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;

and the comments in block/dirty-bitmap.c reflect the above comment.

Paolo


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.


Also, for example, if I want to create a new bitmap and than somehow 
change it, should I do it like this:


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_create_dirty_bitmap, as it releases bitmap lock before return.





--
Best regards,
Vladimir

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

[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 functions between bdrv_clear_dirty_bitmap and
>> bdrv_undo_clear_dirty_bitmap is problematic anyway, so
>> bdrv_clear_dirty_bitmap really only needs the lock in the !out case;
>> bdrv_undo_clear_dirty_bitmap is only called when out != NULL.
>>
>> However, I agree it would be cleaner to add the lock there, too.
> 
> Also, you've added comment "Called with BQL taken" both to functions
> that calls bdrv_dirty_bitmaps_lock and that do not... What is BQL?

It's the "big QEMU lock", also known as "iothread lock".  The locking policy
is documented in block_int.h:

/* Writing to the list requires the BQL _and_ the dirty_bitmap_mutex.
 * Reading from the list can be done with either the BQL or the
 * dirty_bitmap_mutex.  Modifying a bitmap only requires
 * dirty_bitmap_mutex.  */
QemuMutex dirty_bitmap_mutex;
QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps;

and the comments in block/dirty-bitmap.c reflect the above comment.

Paolo

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

[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 problematic anyway, so
bdrv_clear_dirty_bitmap really only needs the lock in the !out case;
bdrv_undo_clear_dirty_bitmap is only called when out != NULL.

However, I agree it would be cleaner to add the lock there, too.

Paolo



Also, you've added comment "Called with BQL taken" both to functions 
that calls bdrv_dirty_bitmaps_lock and that do not... What is BQL?



--
Best regards,
Vladimir

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

[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_bitmap really only needs the lock in the !out case;
bdrv_undo_clear_dirty_bitmap is only called when out != NULL.

However, I agree it would be cleaner to add the lock there, too.

Paolo

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

[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   | 11 +--
  include/block/block_int.h|  4 +--
  include/block/dirty-bitmap.h | 25 +++-
  migration/block.c| 10 ---
  5 files changed, 94 insertions(+), 24 deletions(-)


--
Best regards,
Vladimir