Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

[Paolo Bonzini]

On 12/02/2018 18:30, Vladimir Sementsov-Ogievskiy wrote:
> 18.01.2018 13:09, Paolo Bonzini wrote:>> We have three cases:
>>
>> 1) monitor creates and destroy bitmaps.
>>
>> 2) monitor also has to read the list.  We know it operates with BQL.
>>
>> 3) users such as mirror.c create a dirty bitmap in the monitor command
>> (under BQL), but they can operate without BQL in a separate iothread so
>> we create a separate lock (bitmap->mutex).
>>
>> While in the second and third case, bitmaps cannot disappear.  So in the
>> first case you operate with BQL+dirty bitmap mutex.  The result is that
>> you lock out both the second and the third case while creating and
>> destroying bitmaps.
>>
>>> Why do we do not need them
>>> on read from the bitmap, only on write?
>>
>> Indeed, reading the bitmap also requires taking the lock.  So
>> s/Modifying/Accessing/ in that comment.
> 
> So, finally, the whole thing is:
> 
> 1. any access to dirty_bitmaps list needs BQL or dirty_bitmap_mutex
> 2. bitmap creation or removing needs both BQL and dirty_bitmap_mutex

3. any access to a dirty bitmap needs dirty_bitmap_mutex

Paolo

> yes?
> 
> and one more question:
> Do we really have users, which accesses dirty bitmaps with only BQL?
> query-block uses dirty_bitmap_mutex..
> 
> 

Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

18.01.2018 13:09, Paolo Bonzini wrote:

On 18/01/2018 10:55, Vladimir Sementsov-Ogievskiy wrote:

Most functions that looks at the list are "called with BQL taken".
Functions that write to the list are "called with BQL taken" and call
bdrv_dirty_bitmaps_lock/bdrv_dirty_bitmaps_unlock themselves.

Paolo, could you please explain about bitmap locking in more details?
Why do we need mutexes?

We have three cases:

1) monitor creates and destroy bitmaps.

2) monitor also has to read the list.  We know it operates with BQL.

3) users such as mirror.c create a dirty bitmap in the monitor command
(under BQL), but they can operate without BQL in a separate iothread so
we create a separate lock (bitmap->mutex).

While in the second and third case, bitmaps cannot disappear.  So in the
first case you operate with BQL+dirty bitmap mutex.  The result is that
you lock out both the second and the third case while creating and
destroying bitmaps.


Why do we do not need them
on read from the bitmap, only on write?

Indeed, reading the bitmap also requires taking the lock.  So
s/Modifying/Accessing/ in that comment.

Paolo


So, finally, the whole thing is:

1. any access to dirty_bitmaps list needs BQL or dirty_bitmap_mutex
2. bitmap creation or removing needs both BQL and dirty_bitmap_mutex

yes?

and one more question:
Do we really have users, which accesses dirty bitmaps with only BQL? 
query-block uses dirty_bitmap_mutex..



--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

24.01.2018 13:16, Paolo Bonzini wrote:

On 22/01/2018 13:14, Vladimir Sementsov-Ogievskiy wrote:

so, accessing the bitmap needs mutex lock?

Then what do you mean under accessing the bitmap? Any touch of
BdrvDirtyBitmap fields? Then "reading the list" will require bitmap
mutex too.
Or accessing the bitmap is accessing any field except
BdrvDirtyBitmap.list? Then in (2), what do you mean? For example
query-block will go through
the list, but it touches other fields too, so it should lock mutex.

The bitmap mutex is internal to block/dirty-bitmap.c.


yes and query-block actually calls bdrv_query_dirty_bitmaps, which locks 
mutex..





and one more question:

What about qmp transactions? Should we lock mutex during the whole
transaction?

Transactions hold the BQL, but you don't need to lock the bitmap mutex.


I don't understand. But "Accessing a bitmap only requires 
dirty_bitmap_mutex".. So, if we have several operations on one bitmap,
each of them will lock/unlock mutex by itself? Then we'll have unlocked 
"holes" in our transaction. Or this doesn't matter?




Paolo



--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

[John Snow]

On 01/24/2018 05:16 AM, Paolo Bonzini wrote:
> On 22/01/2018 13:14, Vladimir Sementsov-Ogievskiy wrote:
>>> so, accessing the bitmap needs mutex lock?
>>>
>>> Then what do you mean under accessing the bitmap? Any touch of
>>> BdrvDirtyBitmap fields? Then "reading the list" will require bitmap
>>> mutex too.
>>> Or accessing the bitmap is accessing any field except
>>> BdrvDirtyBitmap.list? Then in (2), what do you mean? For example
>>> query-block will go through
>>> the list, but it touches other fields too, so it should lock mutex.
> 
> The bitmap mutex is internal to block/dirty-bitmap.c.
> 
>> and one more question:
>>
>> What about qmp transactions? Should we lock mutex during the whole
>> transaction?
> 
> Transactions hold the BQL, but you don't need to lock the bitmap mutex.
> 
> Paolo
> 

I would further add that attempting to lock all of the bitmap mutexes
during a transaction might cause complications if you try to perform two
actions on the same bitmap, and it deadlocks.

With the BQL held we're probably okay, but worst-case scenario locking
and unlocking per-each action is probably sufficient, I'd think.

Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

[Paolo Bonzini]

On 22/01/2018 13:14, Vladimir Sementsov-Ogievskiy wrote:
>> so, accessing the bitmap needs mutex lock?
>>
>> Then what do you mean under accessing the bitmap? Any touch of
>> BdrvDirtyBitmap fields? Then "reading the list" will require bitmap
>> mutex too.
>> Or accessing the bitmap is accessing any field except
>> BdrvDirtyBitmap.list? Then in (2), what do you mean? For example
>> query-block will go through
>> the list, but it touches other fields too, so it should lock mutex.

The bitmap mutex is internal to block/dirty-bitmap.c.

> and one more question:
> 
> What about qmp transactions? Should we lock mutex during the whole
> transaction?

Transactions hold the BQL, but you don't need to lock the bitmap mutex.

Paolo

Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

[John Snow]

On 01/22/2018 07:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> 19.01.2018 17:12, Vladimir Sementsov-Ogievskiy wrote:
>> 18.01.2018 13:09, Paolo Bonzini wrote:
>>> On 18/01/2018 10:55, Vladimir Sementsov-Ogievskiy wrote:
> Most functions that looks at the list are "called with BQL taken".
> Functions that write to the list are "called with BQL taken" and call
> bdrv_dirty_bitmaps_lock/bdrv_dirty_bitmaps_unlock themselves.
 Paolo, could you please explain about bitmap locking in more details?
 Why do we need mutexes?
>>> We have three cases:
>>>
>>> 1) monitor creates and destroy bitmaps.
>>>
>>> 2) monitor also has to read the list.  We know it operates with BQL.
>>>
>>> 3) users such as mirror.c create a dirty bitmap in the monitor command
>>> (under BQL), but they can operate without BQL in a separate iothread so
>>> we create a separate lock (bitmap->mutex).
>>>
>>> While in the second and third case, bitmaps cannot disappear. So in the
>>> first case you operate with BQL+dirty bitmap mutex.  The result is that
>>> you lock out both the second and the third case while creating and
>>> destroying bitmaps.
>>>
 Why do we do not need them
 on read from the bitmap, only on write?
>>> Indeed, reading the bitmap also requires taking the lock.  So
>>> s/Modifying/Accessing/ in that comment.
>>>
>>> Paolo
>>
>> so,
>>
>>     /* 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.  Accessing a bitmap only requires
>>  * dirty_bitmap_mutex. */
>>     QemuMutex dirty_bitmap_mutex;
>>
>>
>>
>> so, accessing the bitmap needs mutex lock?
>>
>> Then what do you mean under accessing the bitmap? Any touch of
>> BdrvDirtyBitmap fields? Then "reading the list" will require bitmap
>> mutex too.
>> Or accessing the bitmap is accessing any field except
>> BdrvDirtyBitmap.list? Then in (2), what do you mean? For example
>> query-block will go through
>> the list, but it touches other fields too, so it should lock mutex.
>>
> 
> and one more question:
> 
> What about qmp transactions? Should we lock mutex during the whole
> transaction?
> 

For bitmaps?

hmm...

at the moment, Transactions still do the tepid bdrv_drain_all() prior to
the transaction, and then I suspect they rely on the QMP context holding
the big lock to prevent any new IO occurring.

It should be a quiescent point, I think, but I've lost track of how
exactly it behaves presently. Didn't we need a
bdrv_drained_all_begin/_end pair here? Did we not do that? I forget why...

(...is it related to how we don't know how to implement this in a
context where graph changes might occur, which can happen during a
transaction?)

it might not be possible to grab the bitmap locks during .prepare and
release them in .cleanup, because we might want to add two actions to
the same transaction that operate on the same bitmap (full backup +
clear, for instance?) and they'll deadlock against each other.

It might be sufficient to just lock and release per each action, until
the deeper issues with transactions are resolved. If the transaction is
properly quiescent, you shouldn't run into any bitmap inconsistency
problems anyway.

Hoping Kevin and Paolo can chime in to remind me of the details, here.

Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

19.01.2018 17:12, Vladimir Sementsov-Ogievskiy wrote:

18.01.2018 13:09, Paolo Bonzini wrote:

On 18/01/2018 10:55, Vladimir Sementsov-Ogievskiy wrote:

Most functions that looks at the list are "called with BQL taken".
Functions that write to the list are "called with BQL taken" and call
bdrv_dirty_bitmaps_lock/bdrv_dirty_bitmaps_unlock themselves.

Paolo, could you please explain about bitmap locking in more details?
Why do we need mutexes?

We have three cases:

1) monitor creates and destroy bitmaps.

2) monitor also has to read the list.  We know it operates with BQL.

3) users such as mirror.c create a dirty bitmap in the monitor command
(under BQL), but they can operate without BQL in a separate iothread so
we create a separate lock (bitmap->mutex).

While in the second and third case, bitmaps cannot disappear. So in the
first case you operate with BQL+dirty bitmap mutex.  The result is that
you lock out both the second and the third case while creating and
destroying bitmaps.


Why do we do not need them
on read from the bitmap, only on write?

Indeed, reading the bitmap also requires taking the lock.  So
s/Modifying/Accessing/ in that comment.

Paolo


so,

    /* 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.  Accessing a bitmap only requires
 * dirty_bitmap_mutex. */
    QemuMutex dirty_bitmap_mutex;



so, accessing the bitmap needs mutex lock?

Then what do you mean under accessing the bitmap? Any touch of 
BdrvDirtyBitmap fields? Then "reading the list" will require bitmap 
mutex too.
Or accessing the bitmap is accessing any field except 
BdrvDirtyBitmap.list? Then in (2), what do you mean? For example 
query-block will go through

the list, but it touches other fields too, so it should lock mutex.



and one more question:

What about qmp transactions? Should we lock mutex during the whole 
transaction?


--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

18.01.2018 13:09, Paolo Bonzini wrote:

On 18/01/2018 10:55, Vladimir Sementsov-Ogievskiy wrote:

Most functions that looks at the list are "called with BQL taken".
Functions that write to the list are "called with BQL taken" and call
bdrv_dirty_bitmaps_lock/bdrv_dirty_bitmaps_unlock themselves.

Paolo, could you please explain about bitmap locking in more details?
Why do we need mutexes?

We have three cases:

1) monitor creates and destroy bitmaps.

2) monitor also has to read the list.  We know it operates with BQL.

3) users such as mirror.c create a dirty bitmap in the monitor command
(under BQL), but they can operate without BQL in a separate iothread so
we create a separate lock (bitmap->mutex).

While in the second and third case, bitmaps cannot disappear.  So in the
first case you operate with BQL+dirty bitmap mutex.  The result is that
you lock out both the second and the third case while creating and
destroying bitmaps.


Why do we do not need them
on read from the bitmap, only on write?

Indeed, reading the bitmap also requires taking the lock.  So
s/Modifying/Accessing/ in that comment.

Paolo


so,

    /* 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.  Accessing a bitmap only requires
 * dirty_bitmap_mutex. */
    QemuMutex dirty_bitmap_mutex;



so, accessing the bitmap needs mutex lock?

Then what do you mean under accessing the bitmap? Any touch of 
BdrvDirtyBitmap fields? Then "reading the list" will require bitmap 
mutex too.
Or accessing the bitmap is accessing any field except 
BdrvDirtyBitmap.list? Then in (2), what do you mean? For example 
query-block will go through

the list, but it touches other fields too, so it should lock mutex.

--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

[Paolo Bonzini]

On 18/01/2018 10:55, Vladimir Sementsov-Ogievskiy wrote:
>>
>> Most functions that looks at the list are "called with BQL taken".
>> Functions that write to the list are "called with BQL taken" and call
>> bdrv_dirty_bitmaps_lock/bdrv_dirty_bitmaps_unlock themselves.
> 
> Paolo, could you please explain about bitmap locking in more details?
> Why do we need mutexes?

We have three cases:

1) monitor creates and destroy bitmaps.

2) monitor also has to read the list.  We know it operates with BQL.

3) users such as mirror.c create a dirty bitmap in the monitor command
(under BQL), but they can operate without BQL in a separate iothread so
we create a separate lock (bitmap->mutex).

While in the second and third case, bitmaps cannot disappear.  So in the
first case you operate with BQL+dirty bitmap mutex.  The result is that
you lock out both the second and the third case while creating and
destroying bitmaps.

> Why do we do not need them
> on read from the bitmap, only on write?

Indeed, reading the bitmap also requires taking the lock.  So
s/Modifying/Accessing/ in that comment.

Paolo

Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

18.01.2018 11:43, Paolo Bonzini wrote:

On 29/12/2017 02:31, Fam Zheng wrote:

we have the following comment:

     /* 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;

(I don't understand well the logic, why is it so. Paolo introduced the lock,
but didn't update some functions..)

so, actually, here we need both BQL and mutex.

Yes, because of that comment my interpretion has been both "BQL and the mutex"
whereever we say "within bdrv_dirty_bitmap_lock..unlock", as to some other
places in this file.

A bit late, but---no, "within bdrv_dirty_bitmap_lock..unlock" means it's
the "modifying the bitmap" case.

Most functions that looks at the list are "called with BQL taken".
Functions that write to the list are "called with BQL taken" and call
bdrv_dirty_bitmaps_lock/bdrv_dirty_bitmaps_unlock themselves.

Paolo


Paolo, could you please explain about bitmap locking in more details? 
Why do we need mutexes? Why do we do not need them

on read from the bitmap, only on write?

I imaging the following cases for locks:

- mutex + BQL(do aio_context_acquire take it?)
- only mutex
- only BQL
- no locks

and following operation on bitmaps:

- r/w bitmaps list (i.e. change .next fields of bitmaps and head of the 
list)

- r/w BdrvDirtyBitmap fields
- r/w HBitmap fields
- r/w HBitmap levels (set/unset/read bits)

what is the relations between locking cases and operations and why?

Sorry if I'm asking stupid questions :(

--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

[Paolo Bonzini]

On 29/12/2017 02:31, Fam Zheng wrote:
>> we have the following comment:
>>
>>     /* 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;
>>
>> (I don't understand well the logic, why is it so. Paolo introduced the lock,
>> but didn't update some functions..)
>>
>> so, actually, here we need both BQL and mutex.
>
> Yes, because of that comment my interpretion has been both "BQL and the mutex"
> whereever we say "within bdrv_dirty_bitmap_lock..unlock", as to some other
> places in this file.

A bit late, but---no, "within bdrv_dirty_bitmap_lock..unlock" means it's
the "modifying the bitmap" case.

Most functions that looks at the list are "called with BQL taken".
Functions that write to the list are "called with BQL taken" and call
bdrv_dirty_bitmaps_lock/bdrv_dirty_bitmaps_unlock themselves.

Paolo

Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

[Fam Zheng]

On Thu, 12/28 14:16, Vladimir Sementsov-Ogievskiy wrote:
> 28.12.2017 08:24, Fam Zheng wrote:
> > On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy 
> > > ---
> > >   include/block/dirty-bitmap.h |  3 +++
> > >   block/dirty-bitmap.c | 25 -
> > >   2 files changed, 19 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> > > index 93d4336505..caf1f3d861 100644
> > > --- a/include/block/dirty-bitmap.h
> > > +++ b/include/block/dirty-bitmap.h
> > > @@ -92,5 +92,8 @@ bool 
> > > bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
> > >   BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
> > >   BdrvDirtyBitmap *bitmap);
> > >   char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error 
> > > **errp);
> > > +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
> > > +  BdrvDirtyBitmap 
> > > *bitmap,
> > > +  Error **errp);
> > >   #endif
> > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> > > index d83da077d5..fe27ddfb83 100644
> > > --- a/block/dirty-bitmap.c
> > > +++ b/block/dirty-bitmap.c
> > > @@ -327,15 +327,11 @@ BdrvDirtyBitmap 
> > > *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
> > >* The merged parent will be un-frozen, but not explicitly re-enabled.
> > >* Called with BQL taken.
> > Maybe update the comment as
> > 
> > s/Called with BQL taken/Called within bdrv_dirty_bitmap_lock..unlock/
> > 
> > and ...
> 
> we have the following comment:
> 
>     /* 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;
> 
> (I don't understand well the logic, why is it so. Paolo introduced the lock,
> but didn't update some functions..)
> 
> so, actually, here we need both BQL and mutex.

Yes, because of that comment my interpretion has been both "BQL and the mutex"
whereever we say "within bdrv_dirty_bitmap_lock..unlock", as to some other
places in this file.

Fam

Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

[Vladimir Sementsov-Ogievskiy]

28.12.2017 08:24, Fam Zheng wrote:

On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/dirty-bitmap.h |  3 +++
  block/dirty-bitmap.c | 25 -
  2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 93d4336505..caf1f3d861 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -92,5 +92,8 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState 
*bs);
  BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
  BdrvDirtyBitmap *bitmap);
  char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
+  BdrvDirtyBitmap *bitmap,
+  Error **errp);
  
  #endif

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index d83da077d5..fe27ddfb83 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -327,15 +327,11 @@ BdrvDirtyBitmap 
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
   * The merged parent will be un-frozen, but not explicitly re-enabled.
   * Called with BQL taken.

Maybe update the comment as

s/Called with BQL taken/Called within bdrv_dirty_bitmap_lock..unlock/

and ...


we have the following comment:

    /* 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;

(I don't understand well the logic, why is it so. Paolo introduced the 
lock, but didn't update some functions..)


so, actually, here we need both BQL and mutex.




   */
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
-   BdrvDirtyBitmap *parent,
-   Error **errp)
+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
+  BdrvDirtyBitmap *parent,
+  Error **errp)
  {
-BdrvDirtyBitmap *successor;
-
-qemu_mutex_lock(parent->mutex);
-
-successor = parent->successor;
+BdrvDirtyBitmap *successor = parent->successor;
  
  if (!successor) {

  error_setg(errp, "Cannot reclaim a successor when none is present");
@@ -349,9 +345,20 @@ BdrvDirtyBitmap 
*bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
  bdrv_release_dirty_bitmap_locked(bs, successor);
  parent->successor = NULL;
  
+return parent;

+}
+

... move the "Called with BQL taken" comment here?


and here BQL.

Ok, I'll add/fix comments.




+BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
+   BdrvDirtyBitmap *parent,
+   Error **errp)
+{
+BdrvDirtyBitmap *ret;
+
+qemu_mutex_lock(parent->mutex);
+ret = bdrv_reclaim_dirty_bitmap_locked(bs, parent, errp);
  qemu_mutex_unlock(parent->mutex);
  
-return parent;

+return ret;
  }
  
  /**

--
2.11.1




--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

[Fam Zheng]

On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  include/block/dirty-bitmap.h |  3 +++
>  block/dirty-bitmap.c | 25 -
>  2 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 93d4336505..caf1f3d861 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -92,5 +92,8 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState 
> *bs);
>  BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>  BdrvDirtyBitmap *bitmap);
>  char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
> +  BdrvDirtyBitmap *bitmap,
> +  Error **errp);
>  
>  #endif
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index d83da077d5..fe27ddfb83 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -327,15 +327,11 @@ BdrvDirtyBitmap 
> *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>   * The merged parent will be un-frozen, but not explicitly re-enabled.
>   * Called with BQL taken.

Maybe update the comment as

s/Called with BQL taken/Called within bdrv_dirty_bitmap_lock..unlock/

and ...

>   */
> -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> -   BdrvDirtyBitmap *parent,
> -   Error **errp)
> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
> +  BdrvDirtyBitmap *parent,
> +  Error **errp)
>  {
> -BdrvDirtyBitmap *successor;
> -
> -qemu_mutex_lock(parent->mutex);
> -
> -successor = parent->successor;
> +BdrvDirtyBitmap *successor = parent->successor;
>  
>  if (!successor) {
>  error_setg(errp, "Cannot reclaim a successor when none is present");
> @@ -349,9 +345,20 @@ BdrvDirtyBitmap 
> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>  bdrv_release_dirty_bitmap_locked(bs, successor);
>  parent->successor = NULL;
>  
> +return parent;
> +}
> +

... move the "Called with BQL taken" comment here?

> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> +   BdrvDirtyBitmap *parent,
> +   Error **errp)
> +{
> +BdrvDirtyBitmap *ret;
> +
> +qemu_mutex_lock(parent->mutex);
> +ret = bdrv_reclaim_dirty_bitmap_locked(bs, parent, errp);
>  qemu_mutex_unlock(parent->mutex);
>  
> -return parent;
> +return ret;
>  }
>  
>  /**
> -- 
> 2.11.1
>