[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-devel] [PATCH v8 04/14] block/dirty-bitmap: add bdrv_dirty_bitmap_set_frozen
Date: Fri, 17 Nov 2017 17:46:58 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0

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

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
  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 
  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)))


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?


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


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

  /* 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 
          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 
if (parent->successor) {
          bdrv_release_dirty_bitmap_locked(bs, parent->successor);
+        assert(parent->frozen);
+        parent->frozen = false;
          parent->successor = NULL;

Reviewed-by: John Snow <address@hidden>

Fam, any thoughts?


Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]