[Qemu-devel] [PATCH 0/5] dirty-bitmaps: deprecate @status field

[Qemu-devel] [PATCH 0/5] dirty-bitmaps: deprecate @status field

[John Snow]

The current internal meanings of "locked", "user_locked",
"qmp_locked", "frozen", "enabled", and "disabled" are all
a little muddled.

Deprecate the @status field in favor of two new booleans
that carry very specific meanings. Then, rename and rework
some of the internal semantics to help make the API a bit
more clear and easier to read.

Well, in my opinion.

Based on my current bitmaps branch (includes Eric's patch
and my documentation update patch.)

John Snow (5):
  block/dirty-bitmap: add recording and busy properties
  block/dirty-bitmaps: rename frozen predicate helper
  block/dirty-bitmap: change semantics of enabled predicate
  block/dirty-bitmap: explicitly lock bitmaps with successors
  block/dirty-bitmaps: unify qmp_locked and user_locked calls

 block/dirty-bitmap.c           | 74 +++++++++++++++++++---------------
 blockdev.c                     | 18 ++++-----
 include/block/dirty-bitmap.h   |  7 ++--
 migration/block-dirty-bitmap.c |  8 ++--
 nbd/server.c                   |  6 +--
 qapi/block-core.json           |  9 ++++-
 tests/qemu-iotests/236.out     | 28 +++++++++++++
 7 files changed, 96 insertions(+), 54 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH 1/5] block/dirty-bitmap: add recording and busy properties

[John Snow]

The current API allows us to report a single status, which we've defined as:

Frozen: has a successor, treated as qmp_locked, may or may not be enabled.
Locked: no successor, qmp_locked. may or may not be enabled.
Disabled: Not frozen or locked, disabled.
Active: Not frozen, locked, or disabled.

The problem is that both "Frozen" and "Locked" mean nearly the same thing,
and that both of them do not intuit whether they are recording guest writes
or not.

This patch deprecates that status field and introduces two orthogonal
properties instead to replace it.
---
 block/dirty-bitmap.c       |  9 +++++++++
 qapi/block-core.json       |  9 ++++++++-
 tests/qemu-iotests/236.out | 28 ++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index c6d4acebfa..101383b3af 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -226,6 +226,13 @@ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
     }
 }
 
+/* Called with BQL taken.  */
+static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap)
+{
+    return !bitmap->disabled || (bitmap->successor &&
+                                 !bitmap->successor->disabled);
+}
+
 /**
  * Create a successor bitmap destined to replace this bitmap after an operation.
  * Requires that the bitmap is not frozen and has no successor.
@@ -448,6 +455,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
         info->has_name = !!bm->name;
         info->name = g_strdup(bm->name);
         info->status = bdrv_dirty_bitmap_status(bm);
+        info->recording = bdrv_dirty_bitmap_recording(bm);
+        info->busy = bdrv_dirty_bitmap_user_locked(bm);
         info->persistent = bm->persistent;
         entry->value = info;
         *plist = entry;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5a0f5f5a95..275f0d573c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -455,7 +455,13 @@
 #
 # @granularity: granularity of the dirty bitmap in bytes (since 1.4)
 #
-# @status: current status of the dirty bitmap (since 2.4)
+# @status: Deprecated in favor of @recording and @locked. (since 4.0)
+#
+# @recording: true if the bitmap is recording new writes from the guest.
+#             Replaces `active` status. (since 4.0)
+#
+# @busy: true if the bitmap is in-use by some operation (NBD or jobs)
+#        and cannot be modified via QMP right now. (since 4.0)
 #
 # @persistent: true if the bitmap will eventually be flushed to persistent
 #              storage (since 4.0)
@@ -464,6 +470,7 @@
 ##
 { 'struct': 'BlockDirtyInfo',
   'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
+           'recording': 'bool', 'busy': 'bool',
            'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
 
 ##
diff --git a/tests/qemu-iotests/236.out b/tests/qemu-iotests/236.out
index 5006f7bca1..815cd053f0 100644
--- a/tests/qemu-iotests/236.out
+++ b/tests/qemu-iotests/236.out
@@ -22,17 +22,21 @@ write -P0xcd 0x3ff0000 64k
   "bitmaps": {
     "drive0": [
       {
+        "busy": false,
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapB",
         "persistent": false,
+        "recording": true,
         "status": "active"
       },
       {
+        "busy": false,
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapA",
         "persistent": false,
+        "recording": true,
         "status": "active"
       }
     ]
@@ -84,17 +88,21 @@ write -P0xcd 0x3ff0000 64k
   "bitmaps": {
     "drive0": [
       {
+        "busy": false,
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapB",
         "persistent": false,
+        "recording": true,
         "status": "active"
       },
       {
+        "busy": false,
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapA",
         "persistent": false,
+        "recording": true,
         "status": "active"
       }
     ]
@@ -184,24 +192,30 @@ write -P0xea 0x3fe0000 64k
   "bitmaps": {
     "drive0": [
       {
+        "busy": false,
         "count": 393216,
         "granularity": 65536,
         "name": "bitmapC",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       },
       {
+        "busy": false,
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapB",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       },
       {
+        "busy": false,
         "count": 458752,
         "granularity": 65536,
         "name": "bitmapA",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       }
     ]
@@ -251,24 +265,30 @@ write -P0xea 0x3fe0000 64k
   "bitmaps": {
     "drive0": [
       {
+        "busy": false,
         "count": 393216,
         "granularity": 65536,
         "name": "bitmapC",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       },
       {
+        "busy": false,
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapB",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       },
       {
+        "busy": false,
         "count": 458752,
         "granularity": 65536,
         "name": "bitmapA",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       }
     ]
@@ -311,31 +331,39 @@ write -P0xea 0x3fe0000 64k
   "bitmaps": {
     "drive0": [
       {
+        "busy": false,
         "count": 458752,
         "granularity": 65536,
         "name": "bitmapD",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       },
       {
+        "busy": false,
         "count": 393216,
         "granularity": 65536,
         "name": "bitmapC",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       },
       {
+        "busy": false,
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapB",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       },
       {
+        "busy": false,
         "count": 458752,
         "granularity": 65536,
         "name": "bitmapA",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       }
     ]
-- 
2.17.2

[Qemu-devel] [PATCH 2/5] block/dirty-bitmaps: rename frozen predicate helper

[John Snow]

"Frozen" was a good description a long time ago, but it isn't adequate now.
Rename the frozen predicate to has_successor to make the semantics of the
predicate more clear to outside callers.

In the process, remove some calls to frozen() that no longer semantically
make sense. For enabled and disabled in particular, it's actually okay for
the internals to do this but only forbidden for users to invoke them, and
all of the QMP entry uses already check against qmp_locked.

Several other assertions really want to check that the bitmap isn't in-use
by another operation -- use the qmp_locked function for this instead, which
presently also checks for has_successor.
---
 block/dirty-bitmap.c           | 32 +++++++++++++++++---------------
 include/block/dirty-bitmap.h   |  2 +-
 migration/block-dirty-bitmap.c |  2 +-
 3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 101383b3af..dbe2d97d3f 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -50,7 +50,7 @@ struct BdrvDirtyBitmap {
     HBitmap *meta;              /* Meta dirty bitmap */
     bool qmp_locked;            /* Bitmap is locked, it can't be modified
                                    through QMP */
-    BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
+    BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked state */
     char *name;                 /* Optional non-empty unique ID */
     int64_t size;               /* Size of the bitmap, in bytes */
     bool disabled;              /* Bitmap is disabled. It ignores all writes to
@@ -183,14 +183,14 @@ const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap)
 }
 
 /* Called with BQL taken.  */
-bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
+bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap)
 {
     return bitmap->successor;
 }
 
 /* Both conditions disallow user-modification via QMP. */
 bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) {
-    return bdrv_dirty_bitmap_frozen(bitmap) ||
+    return bdrv_dirty_bitmap_has_successor(bitmap) ||
            bdrv_dirty_bitmap_qmp_locked(bitmap);
 }
 
@@ -215,7 +215,7 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
 /* Called with BQL taken.  */
 DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
 {
-    if (bdrv_dirty_bitmap_frozen(bitmap)) {
+    if (bdrv_dirty_bitmap_has_successor(bitmap)) {
         return DIRTY_BITMAP_STATUS_FROZEN;
     } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
         return DIRTY_BITMAP_STATUS_LOCKED;
@@ -235,7 +235,7 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap)
 
 /**
  * Create a successor bitmap destined to replace this bitmap after an operation.
- * Requires that the bitmap is not frozen and has no successor.
+ * Requires that the bitmap is not locked and has no successor.
  * Called with BQL taken.
  */
 int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
@@ -244,12 +244,16 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
     uint64_t granularity;
     BdrvDirtyBitmap *child;
 
-    if (bdrv_dirty_bitmap_frozen(bitmap)) {
-        error_setg(errp, "Cannot create a successor for a bitmap that is "
-                   "currently frozen");
+    if (bdrv_dirty_bitmap_has_successor(bitmap)) {
+        error_setg(errp, "Cannot create a successor for a bitmap that already "
+                   "has one");
+        return -1;
+    }
+    if (bdrv_dirty_bitmap_user_locked(bitmap)) {
+        error_setg(errp, "Cannot create a successor for a bitmap that is in-use "
+                   "by an operation.");
         return -1;
     }
-    assert(!bitmap->successor);
 
     /* Create an anonymous successor */
     granularity = bdrv_dirty_bitmap_granularity(bitmap);
@@ -268,7 +272,6 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
 
 void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
 {
-    assert(!bdrv_dirty_bitmap_frozen(bitmap));
     bitmap->disabled = false;
 }
 
@@ -285,7 +288,7 @@ void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap)
 static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
 {
     assert(!bitmap->active_iterators);
-    assert(!bdrv_dirty_bitmap_frozen(bitmap));
+    assert(!bdrv_dirty_bitmap_user_locked(bitmap));
     assert(!bitmap->meta);
     QLIST_REMOVE(bitmap, list);
     hbitmap_free(bitmap->bitmap);
@@ -325,7 +328,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
 /**
  * In cases of failure where we can no longer safely delete the parent,
  * we may wish to re-join the parent and child/successor.
- * The merged parent will be un-frozen, but not explicitly re-enabled.
+ * The merged parent will not be user_locked, but not explicitly re-enabled.
  * Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken.
  */
 BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
@@ -373,7 +376,7 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes)
 
     bdrv_dirty_bitmaps_lock(bs);
     QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
-        assert(!bdrv_dirty_bitmap_frozen(bitmap));
+        assert(!bdrv_dirty_bitmap_user_locked(bitmap));
         assert(!bitmap->active_iterators);
         hbitmap_truncate(bitmap->bitmap, bytes);
         bitmap->size = bytes;
@@ -391,7 +394,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
 
 /**
  * Release all named dirty bitmaps attached to a BDS (for use in bdrv_close()).
- * There must not be any frozen bitmaps attached.
+ * There must not be any user_locked bitmaps attached.
  * This function does not remove persistent bitmaps from the storage.
  * Called with BQL taken.
  */
@@ -428,7 +431,6 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
 void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
 {
     bdrv_dirty_bitmap_lock(bitmap);
-    assert(!bdrv_dirty_bitmap_frozen(bitmap));
     bitmap->disabled = true;
     bdrv_dirty_bitmap_unlock(bitmap);
 }
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 04a117fc81..cdbb4dfefd 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -36,7 +36,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 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);
+bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap);
 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/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 6426151e4f..ac6954142f 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -542,7 +542,7 @@ static void dirty_bitmap_load_complete(QEMUFile *f, DirtyBitmapLoadState *s)
         }
     }
 
-    if (bdrv_dirty_bitmap_frozen(s->bitmap)) {
+    if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {
         bdrv_dirty_bitmap_lock(s->bitmap);
         if (enabled_bitmaps == NULL) {
             /* in postcopy */
-- 
2.17.2

[Qemu-devel] [PATCH 3/5] block/dirty-bitmap: change semantics of enabled predicate

[John Snow]

Currently, enabled means something like "the status of the bitmap
is ACTIVE." After this patch, it should mean exclusively: "This
bitmap is recording guest writes, and is allowed to do so."

In many places, this is how this predicate was already used.
We'll allow users to call user_locked if they're really curious about
finding out if the bitmap is in use by an operation.

To accommodate this, modify the create_successor routine to now
explicitly disable the parent bitmap at creation time.


Justifications:

1. bdrv_dirty_bitmap_status suffers no change from the lack of
   1:1 parity with the new predicates because of the order in which
   the predicates are checked. This is now only for compatibility.

2. bdrv_set_dirty_bitmap is only used by mirror, which does not use
   disabled bitmaps -- all of these writes are internal usages.
   Therefore, we should allow writes even in the disabled state.
   The condition is removed.

3. bdrv_reset_dirty_bitmap Similarly, this is only used internally by
   mirror and migration. In these contexts it is always enabled anyway,
   but our API does not need to enforce this.

4. bdrv_set_dirty will skip recording writes from the guest here if
   we are disabled OR if we had a successor, which now changes.
   Accommodate the change by explicitly disabling bitmaps with successors.

5. qcow2/dirty-bitmap: This only ever wanted to check if the bitmap
   was enabled or not. Theoretically if we save during an operation,
   this now gets set as enabled instead of disabled.

6. block_dirty_bitmap_enable_prepare only ever cared if about the
   literal bit, and already checked for user_locked beforehand.

7. block_dirty_bitmap_disable_prepare ditto as above.

8. init_dirty_bitmap_migration also already checks user_locked,
   so this call can be a simple enabled/disabled check.
---
 block/dirty-bitmap.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index dbe2d97d3f..1a433130ff 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -209,7 +209,7 @@ bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap)
 /* Called with BQL taken.  */
 bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
 {
-    return !(bitmap->disabled || bitmap->successor);
+    return !bitmap->disabled;
 }
 
 /* Called with BQL taken.  */
@@ -264,6 +264,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
 
     /* Successor will be on or off based on our current state. */
     child->disabled = bitmap->disabled;
+    bitmap->disabled = true;
 
     /* Install the successor and freeze the parent */
     bitmap->successor = child;
@@ -346,6 +347,8 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
         error_setg(errp, "Merging of parent and successor bitmap failed");
         return NULL;
     }
+
+    parent->disabled = successor->disabled;
     bdrv_release_dirty_bitmap_locked(successor);
     parent->successor = NULL;
 
@@ -542,7 +545,6 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
 void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
                                   int64_t offset, int64_t bytes)
 {
-    assert(bdrv_dirty_bitmap_enabled(bitmap));
     assert(!bdrv_dirty_bitmap_readonly(bitmap));
     hbitmap_set(bitmap->bitmap, offset, bytes);
 }
@@ -559,7 +561,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
 void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap,
                                     int64_t offset, int64_t bytes)
 {
-    assert(bdrv_dirty_bitmap_enabled(bitmap));
     assert(!bdrv_dirty_bitmap_readonly(bitmap));
     hbitmap_reset(bitmap->bitmap, offset, bytes);
 }
-- 
2.17.2

[Qemu-devel] [PATCH 4/5] block/dirty-bitmap: explicitly lock bitmaps with successors

[John Snow]

Instead of implying a locked status, make it explicit.
Now, bitmaps in use by migration, NBD or backup operations
are all treated the same way with the same code paths.
---
 block/dirty-bitmap.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 1a433130ff..22c4a9e0fe 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -188,10 +188,8 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap)
     return bitmap->successor;
 }
 
-/* Both conditions disallow user-modification via QMP. */
 bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) {
-    return bdrv_dirty_bitmap_has_successor(bitmap) ||
-           bdrv_dirty_bitmap_qmp_locked(bitmap);
+    return bdrv_dirty_bitmap_qmp_locked(bitmap);
 }
 
 void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked)
@@ -266,8 +264,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
     child->disabled = bitmap->disabled;
     bitmap->disabled = true;
 
-    /* Install the successor and freeze the parent */
+    /* Install the successor and lock the parent */
     bitmap->successor = child;
+    bitmap->qmp_locked = true;
     return 0;
 }
 
@@ -321,6 +320,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
     bitmap->successor = NULL;
     successor->persistent = bitmap->persistent;
     bitmap->persistent = false;
+    bitmap->qmp_locked = false;
     bdrv_release_dirty_bitmap(bs, bitmap);
 
     return successor;
@@ -349,6 +349,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
     }
 
     parent->disabled = successor->disabled;
+    parent->qmp_locked = false;
     bdrv_release_dirty_bitmap_locked(successor);
     parent->successor = NULL;
 
-- 
2.17.2

[Qemu-devel] [PATCH 5/5] block/dirty-bitmaps: unify qmp_locked and user_locked calls

[John Snow]

These mean the same thing now. Unify them and rename the merged call
bdrv_dirty_bitmap_busy to indicate semantically what we are describing,
as well as help disambiguate from the various _locked and _unlocked
versions of bitmap helpers that refer to mutex locks.
---
 block/dirty-bitmap.c           | 41 +++++++++++++++-------------------
 blockdev.c                     | 18 +++++++--------
 include/block/dirty-bitmap.h   |  5 ++---
 migration/block-dirty-bitmap.c |  6 ++---
 nbd/server.c                   |  6 ++---
 5 files changed, 35 insertions(+), 41 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 22c4a9e0fe..43141dc540 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -48,9 +48,9 @@ struct BdrvDirtyBitmap {
     QemuMutex *mutex;
     HBitmap *bitmap;            /* Dirty bitmap implementation */
     HBitmap *meta;              /* Meta dirty bitmap */
-    bool qmp_locked;            /* Bitmap is locked, it can't be modified
-                                   through QMP */
-    BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked state */
+    bool busy;                  /* Bitmap is busy, it can't be modified through
+                                   QMP */
+    BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
     char *name;                 /* Optional non-empty unique ID */
     int64_t size;               /* Size of the bitmap, in bytes */
     bool disabled;              /* Bitmap is disabled. It ignores all writes to
@@ -188,22 +188,17 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap)
     return bitmap->successor;
 }
 
-bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) {
-    return bdrv_dirty_bitmap_qmp_locked(bitmap);
+bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {
+    return bitmap->busy;
 }
 
-void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked)
+void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy)
 {
     qemu_mutex_lock(bitmap->mutex);
-    bitmap->qmp_locked = qmp_locked;
+    bitmap->busy = busy;
     qemu_mutex_unlock(bitmap->mutex);
 }
 
-bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap)
-{
-    return bitmap->qmp_locked;
-}
-
 /* Called with BQL taken.  */
 bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
 {
@@ -215,7 +210,7 @@ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
 {
     if (bdrv_dirty_bitmap_has_successor(bitmap)) {
         return DIRTY_BITMAP_STATUS_FROZEN;
-    } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
+    } else if (bdrv_dirty_bitmap_busy(bitmap)) {
         return DIRTY_BITMAP_STATUS_LOCKED;
     } else if (!bdrv_dirty_bitmap_enabled(bitmap)) {
         return DIRTY_BITMAP_STATUS_DISABLED;
@@ -247,7 +242,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
                    "has one");
         return -1;
     }
-    if (bdrv_dirty_bitmap_user_locked(bitmap)) {
+    if (bdrv_dirty_bitmap_busy(bitmap)) {
         error_setg(errp, "Cannot create a successor for a bitmap that is in-use "
                    "by an operation.");
         return -1;
@@ -266,7 +261,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
 
     /* Install the successor and lock the parent */
     bitmap->successor = child;
-    bitmap->qmp_locked = true;
+    bitmap->busy = true;
     return 0;
 }
 
@@ -288,7 +283,7 @@ void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap)
 static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
 {
     assert(!bitmap->active_iterators);
-    assert(!bdrv_dirty_bitmap_user_locked(bitmap));
+    assert(!bdrv_dirty_bitmap_busy(bitmap));
     assert(!bitmap->meta);
     QLIST_REMOVE(bitmap, list);
     hbitmap_free(bitmap->bitmap);
@@ -320,7 +315,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
     bitmap->successor = NULL;
     successor->persistent = bitmap->persistent;
     bitmap->persistent = false;
-    bitmap->qmp_locked = false;
+    bitmap->busy = false;
     bdrv_release_dirty_bitmap(bs, bitmap);
 
     return successor;
@@ -329,7 +324,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
 /**
  * In cases of failure where we can no longer safely delete the parent,
  * we may wish to re-join the parent and child/successor.
- * The merged parent will not be user_locked, but not explicitly re-enabled.
+ * The merged parent will not be busy, but not explicitly re-enabled.
  * Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken.
  */
 BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
@@ -349,7 +344,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
     }
 
     parent->disabled = successor->disabled;
-    parent->qmp_locked = false;
+    parent->busy = false;
     bdrv_release_dirty_bitmap_locked(successor);
     parent->successor = NULL;
 
@@ -380,7 +375,7 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes)
 
     bdrv_dirty_bitmaps_lock(bs);
     QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
-        assert(!bdrv_dirty_bitmap_user_locked(bitmap));
+        assert(!bdrv_dirty_bitmap_busy(bitmap));
         assert(!bitmap->active_iterators);
         hbitmap_truncate(bitmap->bitmap, bytes);
         bitmap->size = bytes;
@@ -398,7 +393,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
 
 /**
  * Release all named dirty bitmaps attached to a BDS (for use in bdrv_close()).
- * There must not be any user_locked bitmaps attached.
+ * There must not be any busy bitmaps attached.
  * This function does not remove persistent bitmaps from the storage.
  * Called with BQL taken.
  */
@@ -462,7 +457,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
         info->name = g_strdup(bm->name);
         info->status = bdrv_dirty_bitmap_status(bm);
         info->recording = bdrv_dirty_bitmap_recording(bm);
-        info->busy = bdrv_dirty_bitmap_user_locked(bm);
+        info->busy = bdrv_dirty_bitmap_busy(bm);
         info->persistent = bm->persistent;
         entry->value = info;
         *plist = entry;
@@ -770,7 +765,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
 
     qemu_mutex_lock(dest->mutex);
 
-    if (bdrv_dirty_bitmap_user_locked(dest)) {
+    if (bdrv_dirty_bitmap_busy(dest)) {
         error_setg(errp, "Bitmap '%s' is currently in use by another"
         " operation and cannot be modified", dest->name);
         goto out;
diff --git a/blockdev.c b/blockdev.c
index fb18e9c975..23a4bf136e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2008,7 +2008,7 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
         return;
     }
 
-    if (bdrv_dirty_bitmap_user_locked(state->bitmap)) {
+    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
         error_setg(errp, "Cannot modify a bitmap in use by another operation");
         return;
     } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) {
@@ -2057,7 +2057,7 @@ static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
         return;
     }
 
-    if (bdrv_dirty_bitmap_user_locked(state->bitmap)) {
+    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
         error_setg(errp,
                    "Bitmap '%s' is currently in use by another operation"
                    " and cannot be enabled", action->name);
@@ -2098,7 +2098,7 @@ static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
         return;
     }
 
-    if (bdrv_dirty_bitmap_user_locked(state->bitmap)) {
+    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
         error_setg(errp,
                    "Bitmap '%s' is currently in use by another operation"
                    " and cannot be disabled", action->name);
@@ -2884,7 +2884,7 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
         return;
     }
 
-    if (bdrv_dirty_bitmap_user_locked(bitmap)) {
+    if (bdrv_dirty_bitmap_busy(bitmap)) {
         error_setg(errp,
                    "Bitmap '%s' is currently in use by another operation and"
                    " cannot be removed", name);
@@ -2917,7 +2917,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
         return;
     }
 
-    if (bdrv_dirty_bitmap_user_locked(bitmap)) {
+    if (bdrv_dirty_bitmap_busy(bitmap)) {
         error_setg(errp,
                    "Bitmap '%s' is currently in use by another operation"
                    " and cannot be cleared", name);
@@ -2941,7 +2941,7 @@ void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
         return;
     }
 
-    if (bdrv_dirty_bitmap_user_locked(bitmap)) {
+    if (bdrv_dirty_bitmap_busy(bitmap)) {
         error_setg(errp,
                    "Bitmap '%s' is currently in use by another operation"
                    " and cannot be enabled", name);
@@ -2962,7 +2962,7 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
         return;
     }
 
-    if (bdrv_dirty_bitmap_user_locked(bitmap)) {
+    if (bdrv_dirty_bitmap_busy(bitmap)) {
         error_setg(errp,
                    "Bitmap '%s' is currently in use by another operation"
                    " and cannot be disabled", name);
@@ -3538,7 +3538,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
             bdrv_unref(target_bs);
             goto out;
         }
-        if (bdrv_dirty_bitmap_user_locked(bmap)) {
+        if (bdrv_dirty_bitmap_busy(bmap)) {
             error_setg(errp,
                        "Bitmap '%s' is currently in use by another operation"
                        " and cannot be used for backup", backup->bitmap);
@@ -3651,7 +3651,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
             error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);
             goto out;
         }
-        if (bdrv_dirty_bitmap_user_locked(bmap)) {
+        if (bdrv_dirty_bitmap_busy(bmap)) {
             error_setg(errp,
                        "Bitmap '%s' is currently in use by another operation"
                        " and cannot be used for backup", backup->bitmap);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index cdbb4dfefd..ba8477b73f 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -68,7 +68,7 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
 void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
                                        bool persistent);
-void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked);
+void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy);
 void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
                              HBitmap **backup, Error **errp);
 void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration);
@@ -91,8 +91,7 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
 bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
 bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
-bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap);
-bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap);
+bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap);
 bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
 BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
                                         BdrvDirtyBitmap *bitmap);
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index ac6954142f..7057fff242 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -261,7 +261,7 @@ static void dirty_bitmap_mig_cleanup(void)
 
     while ((dbms = QSIMPLEQ_FIRST(&dirty_bitmap_mig_state.dbms_list)) != NULL) {
         QSIMPLEQ_REMOVE_HEAD(&dirty_bitmap_mig_state.dbms_list, entry);
-        bdrv_dirty_bitmap_set_qmp_locked(dbms->bitmap, false);
+        bdrv_dirty_bitmap_set_busy(dbms->bitmap, false);
         bdrv_unref(dbms->bs);
         g_free(dbms);
     }
@@ -301,7 +301,7 @@ static int init_dirty_bitmap_migration(void)
                 goto fail;
             }
 
-            if (bdrv_dirty_bitmap_user_locked(bitmap)) {
+            if (bdrv_dirty_bitmap_busy(bitmap)) {
                 error_report("Can't migrate a bitmap that is in use by another operation: '%s'",
                              bdrv_dirty_bitmap_name(bitmap));
                 goto fail;
@@ -314,7 +314,7 @@ static int init_dirty_bitmap_migration(void)
             }
 
             bdrv_ref(bs);
-            bdrv_dirty_bitmap_set_qmp_locked(bitmap, true);
+            bdrv_dirty_bitmap_set_busy(bitmap, true);
 
             dbms = g_new0(DirtyBitmapMigBitmapState, 1);
             dbms->bs = bs;
diff --git a/nbd/server.c b/nbd/server.c
index 838c150d8c..0bc64de677 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1519,12 +1519,12 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
             goto fail;
         }
 
-        if (bdrv_dirty_bitmap_user_locked(bm)) {
+        if (bdrv_dirty_bitmap_busy(bm)) {
             error_setg(errp, "Bitmap '%s' is in use", bitmap);
             goto fail;
         }
 
-        bdrv_dirty_bitmap_set_qmp_locked(bm, true);
+        bdrv_dirty_bitmap_set_busy(bm, true);
         exp->export_bitmap = bm;
         exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
                                                      bitmap);
@@ -1642,7 +1642,7 @@ void nbd_export_put(NBDExport *exp)
         }
 
         if (exp->export_bitmap) {
-            bdrv_dirty_bitmap_set_qmp_locked(exp->export_bitmap, false);
+            bdrv_dirty_bitmap_set_busy(exp->export_bitmap, false);
             g_free(exp->export_bitmap_context);
         }
 
-- 
2.17.2

Re: [Qemu-devel] [PATCH 0/5] dirty-bitmaps: deprecate @status field

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1816 bytes --]

On 2/11/19 7:02 PM, John Snow wrote:
> The current internal meanings of "locked", "user_locked",
> "qmp_locked", "frozen", "enabled", and "disabled" are all
> a little muddled.
> 
> Deprecate the @status field in favor of two new booleans
> that carry very specific meanings. Then, rename and rework
> some of the internal semantics to help make the API a bit
> more clear and easier to read.

So far, my libvirt patches are not relying on the contents of the status
field, so deprecating things should not hurt libvirt. Using the full
2-cycle process is still probably wise for other users, though, even if
it means for some awkward back-compat computation of status for a while
longer.

> 
> Well, in my opinion.
> 
> Based on my current bitmaps branch (includes Eric's patch
> and my documentation update patch.)
> 
> John Snow (5):
>   block/dirty-bitmap: add recording and busy properties
>   block/dirty-bitmaps: rename frozen predicate helper
>   block/dirty-bitmap: change semantics of enabled predicate
>   block/dirty-bitmap: explicitly lock bitmaps with successors
>   block/dirty-bitmaps: unify qmp_locked and user_locked calls
> 
>  block/dirty-bitmap.c           | 74 +++++++++++++++++++---------------
>  blockdev.c                     | 18 ++++-----
>  include/block/dirty-bitmap.h   |  7 ++--
>  migration/block-dirty-bitmap.c |  8 ++--
>  nbd/server.c                   |  6 +--
>  qapi/block-core.json           |  9 ++++-
>  tests/qemu-iotests/236.out     | 28 +++++++++++++
>  7 files changed, 96 insertions(+), 54 deletions(-)

No change to qemu-deprecated.texi to start the clock ticking, so we can
remove the old field?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 0/5] dirty-bitmaps: deprecate @status field

[John Snow]

On 2/12/19 1:12 PM, Eric Blake wrote:
> On 2/11/19 7:02 PM, John Snow wrote:
>> The current internal meanings of "locked", "user_locked",
>> "qmp_locked", "frozen", "enabled", and "disabled" are all
>> a little muddled.
>>
>> Deprecate the @status field in favor of two new booleans
>> that carry very specific meanings. Then, rename and rework
>> some of the internal semantics to help make the API a bit
>> more clear and easier to read.
> 
> So far, my libvirt patches are not relying on the contents of the status
> field, so deprecating things should not hurt libvirt. Using the full
> 2-cycle process is still probably wise for other users, though, even if
> it means for some awkward back-compat computation of status for a while
> longer.
> 

I didn't fully remove it, either. I will leave it there for the normal
deprecation period -- it doesn't hurt to keep it around.

>>
>> Well, in my opinion.
>>
>> Based on my current bitmaps branch (includes Eric's patch
>> and my documentation update patch.)
>>
>> John Snow (5):
>>   block/dirty-bitmap: add recording and busy properties
>>   block/dirty-bitmaps: rename frozen predicate helper
>>   block/dirty-bitmap: change semantics of enabled predicate
>>   block/dirty-bitmap: explicitly lock bitmaps with successors
>>   block/dirty-bitmaps: unify qmp_locked and user_locked calls
>>
>>  block/dirty-bitmap.c           | 74 +++++++++++++++++++---------------
>>  blockdev.c                     | 18 ++++-----
>>  include/block/dirty-bitmap.h   |  7 ++--
>>  migration/block-dirty-bitmap.c |  8 ++--
>>  nbd/server.c                   |  6 +--
>>  qapi/block-core.json           |  9 ++++-
>>  tests/qemu-iotests/236.out     | 28 +++++++++++++
>>  7 files changed, 96 insertions(+), 54 deletions(-)
> 
> No change to qemu-deprecated.texi to start the clock ticking, so we can
> remove the old field?
> 

I was counting on having to send a v2, I just didn't add the RFC tag
because I wanted to trick people into reviewing it.

If this passes the sniff test for you, I will document it in the .texi.

--js

Re: [Qemu-devel] [PATCH 1/5] block/dirty-bitmap: add recording and busy properties

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2044 bytes --]

On 2/11/19 7:02 PM, John Snow wrote:
> The current API allows us to report a single status, which we've defined as:
> 
> Frozen: has a successor, treated as qmp_locked, may or may not be enabled.
> Locked: no successor, qmp_locked. may or may not be enabled.
> Disabled: Not frozen or locked, disabled.
> Active: Not frozen, locked, or disabled.
> 
> The problem is that both "Frozen" and "Locked" mean nearly the same thing,
> and that both of them do not intuit whether they are recording guest writes
> or not.
> 
> This patch deprecates that status field and introduces two orthogonal
> properties instead to replace it.
> ---

No S-o-b?


> +++ b/qapi/block-core.json
> @@ -455,7 +455,13 @@
>  #
>  # @granularity: granularity of the dirty bitmap in bytes (since 1.4)
>  #
> -# @status: current status of the dirty bitmap (since 2.4)
> +# @status: Deprecated in favor of @recording and @locked. (since 4.0)

I'd leave this one as (since 2.4). The deprecation clock is starting in
4.0, but the field has been present longer, and it is still obvious to
readers...

> +#
> +# @recording: true if the bitmap is recording new writes from the guest.
> +#             Replaces `active` status. (since 4.0)

...that the replacement fields are only usable in 4.0 and newer.

> +#
> +# @busy: true if the bitmap is in-use by some operation (NBD or jobs)
> +#        and cannot be modified via QMP right now. (since 4.0)
>  #
>  # @persistent: true if the bitmap will eventually be flushed to persistent
>  #              storage (since 4.0)
> @@ -464,6 +470,7 @@
>  ##
>  { 'struct': 'BlockDirtyInfo',
>    'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
> +           'recording': 'bool', 'busy': 'bool',
>             'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }

The UI changes look reasonable.

No iotests coverage of "busy":true?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 1/5] block/dirty-bitmap: add recording and busy properties

[John Snow]

On 2/12/19 1:17 PM, Eric Blake wrote:
> On 2/11/19 7:02 PM, John Snow wrote:
>> The current API allows us to report a single status, which we've defined as:
>>
>> Frozen: has a successor, treated as qmp_locked, may or may not be enabled.
>> Locked: no successor, qmp_locked. may or may not be enabled.
>> Disabled: Not frozen or locked, disabled.
>> Active: Not frozen, locked, or disabled.
>>
>> The problem is that both "Frozen" and "Locked" mean nearly the same thing,
>> and that both of them do not intuit whether they are recording guest writes
>> or not.
>>
>> This patch deprecates that status field and introduces two orthogonal
>> properties instead to replace it.
>> ---
> 
> No S-o-b?
> 

Ah, heck, what's wrong with my script that it keeps dropping this? sorry.

> 
>> +++ b/qapi/block-core.json
>> @@ -455,7 +455,13 @@
>>  #
>>  # @granularity: granularity of the dirty bitmap in bytes (since 1.4)
>>  #
>> -# @status: current status of the dirty bitmap (since 2.4)
>> +# @status: Deprecated in favor of @recording and @locked. (since 4.0)
> 
> I'd leave this one as (since 2.4). The deprecation clock is starting in
> 4.0, but the field has been present longer, and it is still obvious to
> readers...
> >> +#
>> +# @recording: true if the bitmap is recording new writes from the guest.
>> +#             Replaces `active` status. (since 4.0)
> 
> ...that the replacement fields are only usable in 4.0 and newer.
> 

OK. I'll just add a small note for what it was deprecated in favor of.

>> +#
>> +# @busy: true if the bitmap is in-use by some operation (NBD or jobs)
>> +#        and cannot be modified via QMP right now. (since 4.0)
>>  #
>>  # @persistent: true if the bitmap will eventually be flushed to persistent
>>  #              storage (since 4.0)
>> @@ -464,6 +470,7 @@
>>  ##
>>  { 'struct': 'BlockDirtyInfo',
>>    'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
>> +           'recording': 'bool', 'busy': 'bool',
>>             'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
> 
> The UI changes look reasonable.
> 
> No iotests coverage of "busy":true?
> 

I'll add it to the list. I wrote test 124 when I was pretty new at
python and it's a dense monster to add things to, but I can probably
work it in without much problem.

Re: [Qemu-devel] [PATCH 2/5] block/dirty-bitmaps: rename frozen predicate helper

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2368 bytes --]

On 2/11/19 7:02 PM, John Snow wrote:
> "Frozen" was a good description a long time ago, but it isn't adequate now.
> Rename the frozen predicate to has_successor to make the semantics of the
> predicate more clear to outside callers.
> 
> In the process, remove some calls to frozen() that no longer semantically
> make sense. For enabled and disabled in particular, it's actually okay for
> the internals to do this but only forbidden for users to invoke them, and
> all of the QMP entry uses already check against qmp_locked.
> 
> Several other assertions really want to check that the bitmap isn't in-use
> by another operation -- use the qmp_locked function for this instead, which
> presently also checks for has_successor.
> ---

Missing S-o-b on entire series, so you have to send v2 anyway :)

> @@ -244,12 +244,16 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>      uint64_t granularity;
>      BdrvDirtyBitmap *child;
>  
> -    if (bdrv_dirty_bitmap_frozen(bitmap)) {
> -        error_setg(errp, "Cannot create a successor for a bitmap that is "
> -                   "currently frozen");
> +    if (bdrv_dirty_bitmap_has_successor(bitmap)) {
> +        error_setg(errp, "Cannot create a successor for a bitmap that already "
> +                   "has one");
> +        return -1;
> +    }
> +    if (bdrv_dirty_bitmap_user_locked(bitmap)) {
> +        error_setg(errp, "Cannot create a successor for a bitmap that is in-use "
> +                   "by an operation.");
>          return -1;

No trailing dot in error_setg().

Should these two errors be swapped (check for locked before
has_successor)?  After all, having a successor is an internal detail,
whereas being in use by something I already triggered is fairly
straightforward to understand.


> @@ -325,7 +328,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>  /**
>   * In cases of failure where we can no longer safely delete the parent,
>   * we may wish to re-join the parent and child/successor.
> - * The merged parent will be un-frozen, but not explicitly re-enabled.
> + * The merged parent will not be user_locked, but not explicitly re-enabled.

s/but not/nor/

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 2/5] block/dirty-bitmaps: rename frozen predicate helper

[John Snow]

On 2/12/19 1:26 PM, Eric Blake wrote:
> On 2/11/19 7:02 PM, John Snow wrote:
>> "Frozen" was a good description a long time ago, but it isn't adequate now.
>> Rename the frozen predicate to has_successor to make the semantics of the
>> predicate more clear to outside callers.
>>
>> In the process, remove some calls to frozen() that no longer semantically
>> make sense. For enabled and disabled in particular, it's actually okay for
>> the internals to do this but only forbidden for users to invoke them, and
>> all of the QMP entry uses already check against qmp_locked.
>>
>> Several other assertions really want to check that the bitmap isn't in-use
>> by another operation -- use the qmp_locked function for this instead, which
>> presently also checks for has_successor.
>> ---
> 
> Missing S-o-b on entire series, so you have to send v2 anyway :)
> 
>> @@ -244,12 +244,16 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>>      uint64_t granularity;
>>      BdrvDirtyBitmap *child;
>>  
>> -    if (bdrv_dirty_bitmap_frozen(bitmap)) {
>> -        error_setg(errp, "Cannot create a successor for a bitmap that is "
>> -                   "currently frozen");
>> +    if (bdrv_dirty_bitmap_has_successor(bitmap)) {
>> +        error_setg(errp, "Cannot create a successor for a bitmap that already "
>> +                   "has one");
>> +        return -1;
>> +    }
>> +    if (bdrv_dirty_bitmap_user_locked(bitmap)) {
>> +        error_setg(errp, "Cannot create a successor for a bitmap that is in-use "
>> +                   "by an operation.");
>>          return -1;
> 
> No trailing dot in error_setg().
> 

D'oh. I need to re-enable checkpatch, obviously.

> Should these two errors be swapped (check for locked before
> has_successor)?  After all, having a successor is an internal detail,
> whereas being in use by something I already triggered is fairly
> straightforward to understand.
> 

Good point. Will do.

> 
>> @@ -325,7 +328,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>>  /**
>>   * In cases of failure where we can no longer safely delete the parent,
>>   * we may wish to re-join the parent and child/successor.
>> - * The merged parent will be un-frozen, but not explicitly re-enabled.
>> + * The merged parent will not be user_locked, but not explicitly re-enabled.
> 
> s/but not/nor/
> 

I was trying to draw a contrast between "We will forcibly set locked =
false, but make no guaranteed about enable/disable."

I guess nor still works in that case.

Re: [Qemu-devel] [PATCH 3/5] block/dirty-bitmap: change semantics of enabled predicate

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3462 bytes --]

On 2/11/19 7:02 PM, John Snow wrote:
> Currently, enabled means something like "the status of the bitmap
> is ACTIVE." After this patch, it should mean exclusively: "This
> bitmap is recording guest writes, and is allowed to do so."
> 
> In many places, this is how this predicate was already used.
> We'll allow users to call user_locked if they're really curious about
> finding out if the bitmap is in use by an operation.
> 
> To accommodate this, modify the create_successor routine to now
> explicitly disable the parent bitmap at creation time.
> 
> 
> Justifications:
> 
> 1. bdrv_dirty_bitmap_status suffers no change from the lack of
>    1:1 parity with the new predicates because of the order in which
>    the predicates are checked. This is now only for compatibility.
> 
> 2. bdrv_set_dirty_bitmap is only used by mirror, which does not use
>    disabled bitmaps -- all of these writes are internal usages.
>    Therefore, we should allow writes even in the disabled state.
>    The condition is removed.
> 
> 3. bdrv_reset_dirty_bitmap Similarly, this is only used internally by
>    mirror and migration. In these contexts it is always enabled anyway,
>    but our API does not need to enforce this.
> 
> 4. bdrv_set_dirty will skip recording writes from the guest here if
>    we are disabled OR if we had a successor, which now changes.
>    Accommodate the change by explicitly disabling bitmaps with successors.

I didn't quite follow this wording.  My try:

The code in bdrv_set_dirty() is unchanged: pre-patch, it was skipping
bitmaps that were disabled or had a successor, while post-patch it is
only skipping bitmaps that are disabled. But we have the same behavior
because the change to create_successor now ensures that any bitmap with
a successor is disabled.

> 
> 5. qcow2/dirty-bitmap: This only ever wanted to check if the bitmap

Did you mean qcow2_store_persistent_dirty_bitmaps()?

>    was enabled or not. Theoretically if we save during an operation,
>    this now gets set as enabled instead of disabled.

I'm not sure I see the theoretical change in behavior (let alone whether
you could write an iotest to expose it).  Pre-patch, persistent bitmaps
that were disabled or which had a successor did not have the AUTO bit
set (although since we currently only write persistent bitmaps out to
file at exit, when there should be no ongoing jobs and thus no
successors); post-patch, only disabled bitmaps do not have the AUTO bit
(but a bitmap with a successor is disabled because of the change to
create_successor).  But I agree that this code did not need a change due
to the new semantics of bdrv_dirty_bitmap_enabled.

> 
> 6. block_dirty_bitmap_enable_prepare only ever cared if about the

s/if //

>    literal bit, and already checked for user_locked beforehand.

That is, the check for user_locked already ruled out the has_successor
clause.

> 
> 7. block_dirty_bitmap_disable_prepare ditto as above.
> 
> 8. init_dirty_bitmap_migration also already checks user_locked,
>    so this call can be a simple enabled/disabled check.

Looks like correct conversions to me.

> ---
>  block/dirty-bitmap.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 3/5] block/dirty-bitmap: change semantics of enabled predicate

[John Snow]

On 2/12/19 1:58 PM, Eric Blake wrote:
> On 2/11/19 7:02 PM, John Snow wrote:
>> Currently, enabled means something like "the status of the bitmap
>> is ACTIVE." After this patch, it should mean exclusively: "This
>> bitmap is recording guest writes, and is allowed to do so."
>>
>> In many places, this is how this predicate was already used.
>> We'll allow users to call user_locked if they're really curious about
>> finding out if the bitmap is in use by an operation.
>>
>> To accommodate this, modify the create_successor routine to now
>> explicitly disable the parent bitmap at creation time.
>>
>>
>> Justifications:
>>
>> 1. bdrv_dirty_bitmap_status suffers no change from the lack of
>>    1:1 parity with the new predicates because of the order in which
>>    the predicates are checked. This is now only for compatibility.
>>
>> 2. bdrv_set_dirty_bitmap is only used by mirror, which does not use
>>    disabled bitmaps -- all of these writes are internal usages.
>>    Therefore, we should allow writes even in the disabled state.
>>    The condition is removed.
>>
>> 3. bdrv_reset_dirty_bitmap Similarly, this is only used internally by
>>    mirror and migration. In these contexts it is always enabled anyway,
>>    but our API does not need to enforce this.
>>
>> 4. bdrv_set_dirty will skip recording writes from the guest here if
>>    we are disabled OR if we had a successor, which now changes.
>>    Accommodate the change by explicitly disabling bitmaps with successors.
> 
> I didn't quite follow this wording.  My try:
> 
> The code in bdrv_set_dirty() is unchanged: pre-patch, it was skipping
> bitmaps that were disabled or had a successor, while post-patch it is
> only skipping bitmaps that are disabled. But we have the same behavior
> because the change to create_successor now ensures that any bitmap with
> a successor is disabled.
> 

Yes, sorry, the final sentence there is doing a lot of heavy lifting.

>>
>> 5. qcow2/dirty-bitmap: This only ever wanted to check if the bitmap
> 
> Did you mean qcow2_store_persistent_dirty_bitmaps()?
> 

Ah, yeah, I skipped the function name. Yes, that function. It's the only
use in this file.

>>    was enabled or not. Theoretically if we save during an operation,
>>    this now gets set as enabled instead of disabled.
> 
> I'm not sure I see the theoretical change in behavior (let alone whether
> you could write an iotest to expose it).  Pre-patch, persistent bitmaps
> that were disabled or which had a successor did not have the AUTO bit
> set (although since we currently only write persistent bitmaps out to
> file at exit, when there should be no ongoing jobs and thus no
> successors); post-patch, only disabled bitmaps do not have the AUTO bit
> (but a bitmap with a successor is disabled because of the change to
> create_successor).  But I agree that this code did not need a change due
> to the new semantics of bdrv_dirty_bitmap_enabled.
> 

Right, the change is extremely subtle and likely should be impossible to
see. If you CAN see it, that's a bug!

>>
>> 6. block_dirty_bitmap_enable_prepare only ever cared if about the
> 
> s/if //
> 
>>    literal bit, and already checked for user_locked beforehand.
> 
> That is, the check for user_locked already ruled out the has_successor
> clause.
> 

Yes.

>>
>> 7. block_dirty_bitmap_disable_prepare ditto as above.
>>
>> 8. init_dirty_bitmap_migration also already checks user_locked,
>>    so this call can be a simple enabled/disabled check.
> 
> Looks like correct conversions to me.
> 
>> ---
>>  block/dirty-bitmap.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks!

Re: [Qemu-devel] [PATCH 4/5] block/dirty-bitmap: explicitly lock bitmaps with successors

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 497 bytes --]

On 2/11/19 7:02 PM, John Snow wrote:
> Instead of implying a locked status, make it explicit.
> Now, bitmaps in use by migration, NBD or backup operations
> are all treated the same way with the same code paths.
> ---
>  block/dirty-bitmap.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 5/5] block/dirty-bitmaps: unify qmp_locked and user_locked calls

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 863 bytes --]

On 2/11/19 7:02 PM, John Snow wrote:
> These mean the same thing now. Unify them and rename the merged call
> bdrv_dirty_bitmap_busy to indicate semantically what we are describing,
> as well as help disambiguate from the various _locked and _unlocked
> versions of bitmap helpers that refer to mutex locks.

Yay, definitely improved naming.

> ---
>  block/dirty-bitmap.c           | 41 +++++++++++++++-------------------
>  blockdev.c                     | 18 +++++++--------
>  include/block/dirty-bitmap.h   |  5 ++---
>  migration/block-dirty-bitmap.c |  6 ++---
>  nbd/server.c                   |  6 ++---
>  5 files changed, 35 insertions(+), 41 deletions(-)
> 
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 5/5] block/dirty-bitmaps: unify qmp_locked and user_locked calls

[John Snow]

On 2/12/19 2:27 PM, Eric Blake wrote:
> On 2/11/19 7:02 PM, John Snow wrote:
>> These mean the same thing now. Unify them and rename the merged call
>> bdrv_dirty_bitmap_busy to indicate semantically what we are describing,
>> as well as help disambiguate from the various _locked and _unlocked
>> versions of bitmap helpers that refer to mutex locks.
> 
> Yay, definitely improved naming.
> 

Glad you agree! Thanks to Vladimir for the suggestion.

>> ---
>>  block/dirty-bitmap.c           | 41 +++++++++++++++-------------------
>>  blockdev.c                     | 18 +++++++--------
>>  include/block/dirty-bitmap.h   |  5 ++---
>>  migration/block-dirty-bitmap.c |  6 ++---
>>  nbd/server.c                   |  6 ++---
>>  5 files changed, 35 insertions(+), 41 deletions(-)
>>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks again! I'll send a V2 with a few peeks and pokes of additional
test coverage and the deprecated texi update.

--js

Re: [Qemu-devel] [PATCH 1/5] block/dirty-bitmap: add recording and busy properties

[Vladimir Sementsov-Ogievskiy]

On 12.02.2019 1:02, John Snow wrote:
> The current API allows us to report a single status, which we've defined as:
> 
> Frozen: has a successor, treated as qmp_locked, may or may not be enabled.
> Locked: no successor, qmp_locked. may or may not be enabled.
> Disabled: Not frozen or locked, disabled.
> Active: Not frozen, locked, or disabled.
> 
> The problem is that both "Frozen" and "Locked" mean nearly the same thing,
> and that both of them do not intuit whether they are recording guest writes
> or not.
> 
> This patch deprecates that status field and introduces two orthogonal
> properties instead to replace it.
> ---
>   block/dirty-bitmap.c       |  9 +++++++++
>   qapi/block-core.json       |  9 ++++++++-
>   tests/qemu-iotests/236.out | 28 ++++++++++++++++++++++++++++
>   3 files changed, 45 insertions(+), 1 deletion(-)
> 

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 5a0f5f5a95..275f0d573c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -455,7 +455,13 @@
>   #
>   # @granularity: granularity of the dirty bitmap in bytes (since 1.4)
>   #
> -# @status: current status of the dirty bitmap (since 2.4)
> +# @status: Deprecated in favor of @recording and @locked. (since 4.0)
> +#
> +# @recording: true if the bitmap is recording new writes from the guest.
> +#             Replaces `active` status. (since 4.0)
> +#
> +# @busy: true if the bitmap is in-use by some operation (NBD or jobs)
> +#        and cannot be modified via QMP right now. (since 4.0)

not only modified, but also cannot be used somehow for any other operation.

>   #
>   # @persistent: true if the bitmap will eventually be flushed to persistent
>   #              storage (since 4.0)
> @@ -464,6 +470,7 @@
>   ##
>   { 'struct': 'BlockDirtyInfo',
>     'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
> +           'recording': 'bool', 'busy': 'bool',
>              'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
>   

Re: [Qemu-devel] [PATCH 0/5] dirty-bitmaps: deprecate @status field

[Vladimir Sementsov-Ogievskiy]

I am ill, I think, will review it next week, if you not push it earlier.

Best regards,
Vladimir