Re: [Qemu-devel] [PATCH v8 04/14] block/dirty-bitmap: add bdrv_dirty_bitmap_set_frozen

[Vladimir Sementsov-Ogievskiy]

18.11.2017 02:46, John Snow wrote:


On 11/17/2017 12:30 PM, Vladimir Sementsov-Ogievskiy wrote:

17.11.2017 20:20, John Snow wrote:

On 11/17/2017 09:46 AM, Vladimir Sementsov-Ogievskiy wrote:

14.11.2017 02:32, John Snow wrote:

On 10/30/2017 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:

Make it possible to set bitmap 'frozen' without a successor.
This is needed to protect the bitmap during outgoing bitmap postcopy
migration.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
    include/block/dirty-bitmap.h |  1 +
    block/dirty-bitmap.c | 22 --
    2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/include/block/dirty-bitmap.h
b/include/block/dirty-bitmap.h
index a9e2a92e4f..ae6d697850 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -39,6 +39,7 @@ uint32_t
bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
    uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap
*bitmap);
    bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
    bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
+void bdrv_dirty_bitmap_set_frozen(BdrvDirtyBitmap *bitmap, bool
frozen);
    const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap);
    int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap);
    DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap
*bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 7578863aa1..67fc6bd6e0 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -40,6 +40,8 @@ struct BdrvDirtyBitmap {
    QemuMutex *mutex;
    HBitmap *bitmap;    /* Dirty bitmap implementation */
    HBitmap *meta;  /* Meta dirty bitmap */
+    bool frozen;    /* Bitmap is frozen, it can't be
modified
+   through QMP */

I hesitate, because this now means that we have two independent bits of
state we need to update and maintain consistency with.

it was your proposal)))

https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01172.html

"
Your new use case here sounds like Frozen to me, but it simply does not
have an anonymous successor to force it to be recognized as "frozen." We
can add a `bool protected` or `bool frozen` field to force recognition
of this status and adjust the json documentation accordingly.

I think then we'd have four recognized states:

FROZEN: Cannot be reset/deleted. Bitmap is in-use by a block job or
other internal process. Bitmap is otherwise ACTIVE.
DISABLED: Not recording any writes (by choice.)
READONLY: Not able to record any writes (by necessity.)
ACTIVE: Normal bitmap status.

Sound right?
"



I was afraid you'd say that :(

It's okay, anyway. I shouldn't let myself go so long between reviews
like this, because you catch me changing my mind. Anyway, please go
ahead with it. I don't want to delay you on something that works because
I can't make up *my* mind.

Hm, if you remember, reusing "frozen" state was strange for me too. And
it's not
late to move to
1. make a new state: MIGRATION, which disallows qmp operations on bitmap
2. or just make them unnamed, so they can't be touched by qmp

"Migrating" is fine as a state name. You could probably announce this by
having it be "frozen" in the usual way (it has a successor) and a new
bool that lets you do whatever special handling you need to do for it.


create a successor and merge it in before postcopy stage? it is possible 
but I don't like it, looks like cheat..





anything is ok for me as well as leaving it as is. It's all little
things, the core is patch 10.

"frozen" sounds like unchanged, but user will see dirty-count changing
in query-block.

I guess it's a strange misnomer now... or maybe just always was a bad
name, since it's not really "frozen" but rather "locked" in a way that
the QMP user cannot interfere with it -- but it's still a live,
functioning object.


so, may be the best way is to add LOCKED state? which mean:

bitmap is under some operation and is not operable by qmp. dirty-count may
not reflect current state of the bitmap until operation end. and than we 
will be
able to move to 'locked' instead of 'frozen' for backups to, and 
deprecate frozen

state.



I'm remembering what I was talking about, but I think my preference is
illustrably worse. I was trying to avoid boolean bloat by advocating
re-use, but the re-use is kind of confusing...

I think I was hoping that a bitmap on the receiving end could simply be
"frozen" in the usual way, in that it has a successor recording writes.

I think the way you want to handle it though is with different semantics
for cleanup and recovery which makes it not quite the same state, which
needs either a new bool or some such.

Go with what you think is cleanest at this point, including if you want
to leave it alone. I don't want to cause you to respin it over bikeshedding.

--js




--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v8 04/14] block/dirty-bitmap: add bdrv_dirty_bitmap_set_frozen

[John Snow]

On 11/17/2017 12:30 PM, Vladimir Sementsov-Ogievskiy wrote:
> 17.11.2017 20:20, John Snow wrote:
>>
>> On 11/17/2017 09:46 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 14.11.2017 02:32, John Snow wrote:
 On 10/30/2017 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
> Make it possible to set bitmap 'frozen' without a successor.
> This is needed to protect the bitmap during outgoing bitmap postcopy
> migration.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>    include/block/dirty-bitmap.h |  1 +
>    block/dirty-bitmap.c | 22 --
>    2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/include/block/dirty-bitmap.h
> b/include/block/dirty-bitmap.h
> index a9e2a92e4f..ae6d697850 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -39,6 +39,7 @@ uint32_t
> bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
>    uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap
> *bitmap);
>    bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
>    bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
> +void bdrv_dirty_bitmap_set_frozen(BdrvDirtyBitmap *bitmap, bool
> frozen);
>    const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap);
>    int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap);
>    DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap
> *bitmap);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 7578863aa1..67fc6bd6e0 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -40,6 +40,8 @@ struct BdrvDirtyBitmap {
>    QemuMutex *mutex;
>    HBitmap *bitmap;    /* Dirty bitmap implementation */
>    HBitmap *meta;  /* Meta dirty bitmap */
> +    bool frozen;    /* Bitmap is frozen, it can't be
> modified
> +   through QMP */
 I hesitate, because this now means that we have two independent bits of
 state we need to update and maintain consistency with.
>>> it was your proposal)))
>>>
>>> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01172.html
>>>
>>> "
>>> Your new use case here sounds like Frozen to me, but it simply does not
>>> have an anonymous successor to force it to be recognized as "frozen." We
>>> can add a `bool protected` or `bool frozen` field to force recognition
>>> of this status and adjust the json documentation accordingly.
>>>
>>> I think then we'd have four recognized states:
>>>
>>> FROZEN: Cannot be reset/deleted. Bitmap is in-use by a block job or
>>> other internal process. Bitmap is otherwise ACTIVE.
>>> DISABLED: Not recording any writes (by choice.)
>>> READONLY: Not able to record any writes (by necessity.)
>>> ACTIVE: Normal bitmap status.
>>>
>>> Sound right?
>>> "
>>>
>>>
>> I was afraid you'd say that :(
>>
>> It's okay, anyway. I shouldn't let myself go so long between reviews
>> like this, because you catch me changing my mind. Anyway, please go
>> ahead with it. I don't want to delay you on something that works because
>> I can't make up *my* mind.
> 
> Hm, if you remember, reusing "frozen" state was strange for me too. And
> it's not
> late to move to
> 1. make a new state: MIGRATION, which disallows qmp operations on bitmap
> 2. or just make them unnamed, so they can't be touched by qmp

"Migrating" is fine as a state name. You could probably announce this by
having it be "frozen" in the usual way (it has a successor) and a new
bool that lets you do whatever special handling you need to do for it.

> 
> anything is ok for me as well as leaving it as is. It's all little
> things, the core is patch 10.
> 
> "frozen" sounds like unchanged, but user will see dirty-count changing
> in query-block.

I guess it's a strange misnomer now... or maybe just always was a bad
name, since it's not really "frozen" but rather "locked" in a way that
the QMP user cannot interfere with it -- but it's still a live,
functioning object.

> 

I'm remembering what I was talking about, but I think my preference is
illustrably worse. I was trying to avoid boolean bloat by advocating
re-use, but the re-use is kind of confusing...

I think I was hoping that a bitmap on the receiving end could simply be
"frozen" in the usual way, in that it has a successor recording writes.

I think the way you want to handle it though is with different semantics
for cleanup and recovery which makes it not quite the same state, which
needs either a new bool or some such.

Go with what you think is cleanest at this point, including if you want
to leave it alone. I don't want to cause you to respin it over bikeshedding.

--js

Re: [Qemu-devel] [PATCH v8 04/14] block/dirty-bitmap: add bdrv_dirty_bitmap_set_frozen

[Vladimir Sementsov-Ogievskiy]

17.11.2017 20:20, John Snow wrote:


On 11/17/2017 09:46 AM, Vladimir Sementsov-Ogievskiy wrote:

14.11.2017 02:32, John Snow wrote:

On 10/30/2017 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:

Make it possible to set bitmap 'frozen' without a successor.
This is needed to protect the bitmap during outgoing bitmap postcopy
migration.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
   include/block/dirty-bitmap.h |  1 +
   block/dirty-bitmap.c | 22 --
   2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index a9e2a92e4f..ae6d697850 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -39,6 +39,7 @@ uint32_t
bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
   uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap);
   bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
   bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
+void bdrv_dirty_bitmap_set_frozen(BdrvDirtyBitmap *bitmap, bool
frozen);
   const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap);
   int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap);
   DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 7578863aa1..67fc6bd6e0 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -40,6 +40,8 @@ struct BdrvDirtyBitmap {
   QemuMutex *mutex;
   HBitmap *bitmap;    /* Dirty bitmap implementation */
   HBitmap *meta;  /* Meta dirty bitmap */
+    bool frozen;    /* Bitmap is frozen, it can't be
modified
+   through QMP */

I hesitate, because this now means that we have two independent bits of
state we need to update and maintain consistency with.

it was your proposal)))

https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01172.html

"
Your new use case here sounds like Frozen to me, but it simply does not
have an anonymous successor to force it to be recognized as "frozen." We
can add a `bool protected` or `bool frozen` field to force recognition
of this status and adjust the json documentation accordingly.

I think then we'd have four recognized states:

FROZEN: Cannot be reset/deleted. Bitmap is in-use by a block job or
other internal process. Bitmap is otherwise ACTIVE.
DISABLED: Not recording any writes (by choice.)
READONLY: Not able to record any writes (by necessity.)
ACTIVE: Normal bitmap status.

Sound right?
"



I was afraid you'd say that :(

It's okay, anyway. I shouldn't let myself go so long between reviews
like this, because you catch me changing my mind. Anyway, please go
ahead with it. I don't want to delay you on something that works because
I can't make up *my* mind.


Hm, if you remember, reusing "frozen" state was strange for me too. And 
it's not

late to move to
1. make a new state: MIGRATION, which disallows qmp operations on bitmap
2. or just make them unnamed, so they can't be touched by qmp

anything is ok for me as well as leaving it as is. It's all little 
things, the core is patch 10.


"frozen" sounds like unchanged, but user will see dirty-count changing 
in query-block.


--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v8 04/14] block/dirty-bitmap: add bdrv_dirty_bitmap_set_frozen

[John Snow]

On 11/17/2017 09:46 AM, Vladimir Sementsov-Ogievskiy wrote:
> 14.11.2017 02:32, John Snow wrote:
>>
>> On 10/30/2017 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> Make it possible to set bitmap 'frozen' without a successor.
>>> This is needed to protect the bitmap during outgoing bitmap postcopy
>>> migration.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>   include/block/dirty-bitmap.h |  1 +
>>>   block/dirty-bitmap.c | 22 --
>>>   2 files changed, 21 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>>> index a9e2a92e4f..ae6d697850 100644
>>> --- a/include/block/dirty-bitmap.h
>>> +++ b/include/block/dirty-bitmap.h
>>> @@ -39,6 +39,7 @@ uint32_t
>>> bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
>>>   uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap);
>>>   bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
>>>   bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
>>> +void bdrv_dirty_bitmap_set_frozen(BdrvDirtyBitmap *bitmap, bool
>>> frozen);
>>>   const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap);
>>>   int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap);
>>>   DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>> index 7578863aa1..67fc6bd6e0 100644
>>> --- a/block/dirty-bitmap.c
>>> +++ b/block/dirty-bitmap.c
>>> @@ -40,6 +40,8 @@ struct BdrvDirtyBitmap {
>>>   QemuMutex *mutex;
>>>   HBitmap *bitmap;    /* Dirty bitmap implementation */
>>>   HBitmap *meta;  /* Meta dirty bitmap */
>>> +    bool frozen;    /* Bitmap is frozen, it can't be
>>> modified
>>> +   through QMP */
>> I hesitate, because this now means that we have two independent bits of
>> state we need to update and maintain consistency with.
> 
> it was your proposal)))
> 
> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01172.html
> 
> "
> Your new use case here sounds like Frozen to me, but it simply does not
> have an anonymous successor to force it to be recognized as "frozen." We
> can add a `bool protected` or `bool frozen` field to force recognition
> of this status and adjust the json documentation accordingly.
> 
> I think then we'd have four recognized states:
> 
> FROZEN: Cannot be reset/deleted. Bitmap is in-use by a block job or
> other internal process. Bitmap is otherwise ACTIVE.
> DISABLED: Not recording any writes (by choice.)
> READONLY: Not able to record any writes (by necessity.)
> ACTIVE: Normal bitmap status.
> 
> Sound right?
> "
> 
> 

I was afraid you'd say that :(

It's okay, anyway. I shouldn't let myself go so long between reviews
like this, because you catch me changing my mind. Anyway, please go
ahead with it. I don't want to delay you on something that works because
I can't make up *my* mind.

Re: [Qemu-devel] [PATCH v8 04/14] block/dirty-bitmap: add bdrv_dirty_bitmap_set_frozen

[Vladimir Sementsov-Ogievskiy]

14.11.2017 02:32, John Snow wrote:


On 10/30/2017 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:

Make it possible to set bitmap 'frozen' without a successor.
This is needed to protect the bitmap during outgoing bitmap postcopy
migration.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  include/block/dirty-bitmap.h |  1 +
  block/dirty-bitmap.c | 22 --
  2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index a9e2a92e4f..ae6d697850 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -39,6 +39,7 @@ uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState 
*bs);
  uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap);
  bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
  bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
+void bdrv_dirty_bitmap_set_frozen(BdrvDirtyBitmap *bitmap, bool frozen);
  const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap);
  int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap);
  DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 7578863aa1..67fc6bd6e0 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -40,6 +40,8 @@ struct BdrvDirtyBitmap {
  QemuMutex *mutex;
  HBitmap *bitmap;/* Dirty bitmap implementation */
  HBitmap *meta;  /* Meta dirty bitmap */
+bool frozen;/* Bitmap is frozen, it can't be modified
+   through QMP */

I hesitate, because this now means that we have two independent bits of
state we need to update and maintain consistency with.


it was your proposal)))

https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01172.html

"
Your new use case here sounds like Frozen to me, but it simply does not
have an anonymous successor to force it to be recognized as "frozen." We
can add a `bool protected` or `bool frozen` field to force recognition
of this status and adjust the json documentation accordingly.

I think then we'd have four recognized states:

FROZEN: Cannot be reset/deleted. Bitmap is in-use by a block job or
other internal process. Bitmap is otherwise ACTIVE.
DISABLED: Not recording any writes (by choice.)
READONLY: Not able to record any writes (by necessity.)
ACTIVE: Normal bitmap status.

Sound right?
"




Before:

Frozen: "Bitmap has a successor and is no longer itself recording
writes, though we are guaranteed to have a successor doing so on our
behalf."

After:

Frozen: "Bitmap may or may not have a successor, but it is disabled with
an even more limited subset of tasks than a traditionally disabled bitmap."

This changes the meaning of "frozen" a little, and I am not sure that is
a problem as such, but it does make the interface seem a little
"fuzzier" than it did prior.


  BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
  char *name; /* Optional non-empty unique ID */
  int64_t size;   /* Size of the bitmap, in bytes */
@@ -183,13 +185,22 @@ const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap 
*bitmap)
  /* Called with BQL taken.  */
  bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
  {
-return bitmap->successor;
+return bitmap->frozen;
+}

Are there any cases where we will be unfrozen, but actually have a
successor now? If so, under what circumstances does that happen?


+
+/* Called with BQL taken.  */
+void bdrv_dirty_bitmap_set_frozen(BdrvDirtyBitmap *bitmap, bool frozen)
+{
+qemu_mutex_lock(bitmap->mutex);
+assert(bitmap->successor == NULL);
+bitmap->frozen = frozen;
+qemu_mutex_unlock(bitmap->mutex);
  }
  

OK, so we can only "set frozen" (or unset) if we don't have a successor.


  /* Called with BQL taken.  */
  bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
  {
-return !(bitmap->disabled || bitmap->successor);
+return !(bitmap->disabled || (bitmap->successor != NULL));
  }
  

I guess this just makes 'successor' more obviously non-boolean, which is
fine.


  /* Called with BQL taken.  */
@@ -234,6 +245,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
  
  /* Install the successor and freeze the parent */

  bitmap->successor = child;
+bitmap->frozen = true;
  return 0;
  }
  
@@ -266,6 +278,8 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,

  name = bitmap->name;
  bitmap->name = NULL;
  successor->name = name;
+assert(bitmap->frozen);
+bitmap->frozen = false;
  bitmap->successor = NULL;
  successor->persistent = bitmap->persistent;
  bitmap->persistent = false;
@@ -298,6 +312,8 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState 
*bs,
  return NULL;
  }
  bdrv_release_dirty_bitmap(bs, successor);
+

Re: [Qemu-devel] [PATCH v8 04/14] block/dirty-bitmap: add bdrv_dirty_bitmap_set_frozen

[John Snow]

On 10/30/2017 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
> Make it possible to set bitmap 'frozen' without a successor.
> This is needed to protect the bitmap during outgoing bitmap postcopy
> migration.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  include/block/dirty-bitmap.h |  1 +
>  block/dirty-bitmap.c | 22 --
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index a9e2a92e4f..ae6d697850 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -39,6 +39,7 @@ uint32_t 
> bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
>  uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
> +void bdrv_dirty_bitmap_set_frozen(BdrvDirtyBitmap *bitmap, bool frozen);
>  const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap);
>  int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap);
>  DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 7578863aa1..67fc6bd6e0 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -40,6 +40,8 @@ struct BdrvDirtyBitmap {
>  QemuMutex *mutex;
>  HBitmap *bitmap;/* Dirty bitmap implementation */
>  HBitmap *meta;  /* Meta dirty bitmap */
> +bool frozen;/* Bitmap is frozen, it can't be modified
> +   through QMP */

I hesitate, because this now means that we have two independent bits of
state we need to update and maintain consistency with.

Before:

Frozen: "Bitmap has a successor and is no longer itself recording
writes, though we are guaranteed to have a successor doing so on our
behalf."

After:

Frozen: "Bitmap may or may not have a successor, but it is disabled with
an even more limited subset of tasks than a traditionally disabled bitmap."

This changes the meaning of "frozen" a little, and I am not sure that is
a problem as such, but it does make the interface seem a little
"fuzzier" than it did prior.

>  BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
>  char *name; /* Optional non-empty unique ID */
>  int64_t size;   /* Size of the bitmap, in bytes */
> @@ -183,13 +185,22 @@ const char *bdrv_dirty_bitmap_name(const 
> BdrvDirtyBitmap *bitmap)
>  /* Called with BQL taken.  */
>  bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
>  {
> -return bitmap->successor;
> +return bitmap->frozen;
> +}

Are there any cases where we will be unfrozen, but actually have a
successor now? If so, under what circumstances does that happen?

> +
> +/* Called with BQL taken.  */
> +void bdrv_dirty_bitmap_set_frozen(BdrvDirtyBitmap *bitmap, bool frozen)
> +{
> +qemu_mutex_lock(bitmap->mutex);
> +assert(bitmap->successor == NULL);
> +bitmap->frozen = frozen;
> +qemu_mutex_unlock(bitmap->mutex);
>  }
>  

OK, so we can only "set frozen" (or unset) if we don't have a successor.

>  /* Called with BQL taken.  */
>  bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
>  {
> -return !(bitmap->disabled || bitmap->successor);
> +return !(bitmap->disabled || (bitmap->successor != NULL));
>  }
>  

I guess this just makes 'successor' more obviously non-boolean, which is
fine.

>  /* Called with BQL taken.  */
> @@ -234,6 +245,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState 
> *bs,
>  
>  /* Install the successor and freeze the parent */
>  bitmap->successor = child;
> +bitmap->frozen = true;
>  return 0;
>  }
>  
> @@ -266,6 +278,8 @@ BdrvDirtyBitmap 
> *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>  name = bitmap->name;
>  bitmap->name = NULL;
>  successor->name = name;
> +assert(bitmap->frozen);
> +bitmap->frozen = false;
>  bitmap->successor = NULL;
>  successor->persistent = bitmap->persistent;
>  bitmap->persistent = false;
> @@ -298,6 +312,8 @@ BdrvDirtyBitmap 
> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>  return NULL;
>  }
>  bdrv_release_dirty_bitmap(bs, successor);
> +assert(parent->frozen);
> +parent->frozen = false;
>  parent->successor = NULL;
>  
>  return parent;
> @@ -439,6 +455,8 @@ void bdrv_dirty_bitmap_release_successor(BlockDriverState 
> *bs,
>  
>  if (parent->successor) {
>  bdrv_release_dirty_bitmap_locked(bs, parent->successor);
> +assert(parent->frozen);
> +parent->frozen = false;
>  parent->successor = NULL;
>  }
>  
> 

Tentatively:

Reviewed-by: John Snow 

Fam, any thoughts?

--John

[Qemu-devel] [PATCH v8 04/14] block/dirty-bitmap: add bdrv_dirty_bitmap_set_frozen

[Vladimir Sementsov-Ogievskiy]

Make it possible to set bitmap 'frozen' without a successor.
This is needed to protect the bitmap during outgoing bitmap postcopy
migration.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/dirty-bitmap.h |  1 +
 block/dirty-bitmap.c | 22 --
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index a9e2a92e4f..ae6d697850 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -39,6 +39,7 @@ uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState 
*bs);
 uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
+void bdrv_dirty_bitmap_set_frozen(BdrvDirtyBitmap *bitmap, bool frozen);
 const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap);
 int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap);
 DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 7578863aa1..67fc6bd6e0 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -40,6 +40,8 @@ struct BdrvDirtyBitmap {
 QemuMutex *mutex;
 HBitmap *bitmap;/* Dirty bitmap implementation */
 HBitmap *meta;  /* Meta dirty bitmap */
+bool frozen;/* Bitmap is frozen, it can't be modified
+   through QMP */
 BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
 char *name; /* Optional non-empty unique ID */
 int64_t size;   /* Size of the bitmap, in bytes */
@@ -183,13 +185,22 @@ const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap 
*bitmap)
 /* Called with BQL taken.  */
 bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
 {
-return bitmap->successor;
+return bitmap->frozen;
+}
+
+/* Called with BQL taken.  */
+void bdrv_dirty_bitmap_set_frozen(BdrvDirtyBitmap *bitmap, bool frozen)
+{
+qemu_mutex_lock(bitmap->mutex);
+assert(bitmap->successor == NULL);
+bitmap->frozen = frozen;
+qemu_mutex_unlock(bitmap->mutex);
 }
 
 /* Called with BQL taken.  */
 bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
 {
-return !(bitmap->disabled || bitmap->successor);
+return !(bitmap->disabled || (bitmap->successor != NULL));
 }
 
 /* Called with BQL taken.  */
@@ -234,6 +245,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
 
 /* Install the successor and freeze the parent */
 bitmap->successor = child;
+bitmap->frozen = true;
 return 0;
 }
 
@@ -266,6 +278,8 @@ BdrvDirtyBitmap 
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
 name = bitmap->name;
 bitmap->name = NULL;
 successor->name = name;
+assert(bitmap->frozen);
+bitmap->frozen = false;
 bitmap->successor = NULL;
 successor->persistent = bitmap->persistent;
 bitmap->persistent = false;
@@ -298,6 +312,8 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState 
*bs,
 return NULL;
 }
 bdrv_release_dirty_bitmap(bs, successor);
+assert(parent->frozen);
+parent->frozen = false;
 parent->successor = NULL;
 
 return parent;
@@ -439,6 +455,8 @@ void bdrv_dirty_bitmap_release_successor(BlockDriverState 
*bs,
 
 if (parent->successor) {
 bdrv_release_dirty_bitmap_locked(bs, parent->successor);
+assert(parent->frozen);
+parent->frozen = false;
 parent->successor = NULL;
 }
 
-- 
2.11.1