[Qemu-devel] [PATCH v3 00/10] dirty-bitmaps: deprecate @status field

[Qemu-devel] [PATCH v3 00/10] 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.

V3:

[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/10:[0002] [FC] 'block/dirty-bitmap: add recording and busy properties'
002/10:[0002] [FC] 'block/dirty-bitmaps: rename frozen predicate helper'
003/10:[down] 'block/dirty-bitmap: remove set/reset assertions against enabled bit'
004/10:[0006] [FC] 'block/dirty-bitmap: change semantics of enabled predicate'
005/10:[down] 'nbd: change error checking order for bitmaps'
006/10:[0002] [FC] 'block/dirty-bitmap: explicitly lock bitmaps with successors'
007/10:[0011] [FC] 'block/dirty-bitmaps: unify qmp_locked and user_locked calls'
008/10:[0002] [FC] 'block/dirty-bitmaps: move comment block'
009/10:[down] 'blockdev: remove unused paio parameter documentation'
010/10:[down] 'iotests: add busy/recording bit test to 124'

V3: (Following Vladimir's feedback)

001: Changed texi phrasing, parameter --> field
002: Commit message adjustment
     comment edit: "not locked" --> "not user_locked"
003: pulled forward out of patch 004
004: Commit message rewrite
     create_successor and abdicate doc adjustments
005: New.
006: Change successor doc comment
     Commit message adjustment
007: BdrvDirtyBitmap struct comment adjustments
     Comment change to create_successor (lock --> busy)
     Incidental changes from 004's changes
008: Grammar fix (Eric)
009: New, trivial, unrelated fix tagging along.
010: iotest 124 added.

John Snow (10):
  block/dirty-bitmap: add recording and busy properties
  block/dirty-bitmaps: rename frozen predicate helper
  block/dirty-bitmap: remove set/reset assertions against enabled bit
  block/dirty-bitmap: change semantics of enabled predicate
  nbd: change error checking order for bitmaps
  block/dirty-bitmap: explicitly lock bitmaps with successors
  block/dirty-bitmaps: unify qmp_locked and user_locked calls
  block/dirty-bitmaps: move comment block
  blockdev: remove unused paio parameter documentation
  iotests: add busy/recording bit test to 124

 block/dirty-bitmap.c           | 111 ++++++++++++++++++---------------
 blockdev.c                     |  19 +++---
 include/block/dirty-bitmap.h   |   7 +--
 migration/block-dirty-bitmap.c |   8 +--
 nbd/server.c                   |  14 ++---
 qapi/block-core.json           |  10 ++-
 qemu-deprecated.texi           |   6 ++
 tests/qemu-iotests/124         | 110 ++++++++++++++++++++++++++++++++
 tests/qemu-iotests/124.out     |   4 +-
 tests/qemu-iotests/236.out     |  28 +++++++++
 10 files changed, 239 insertions(+), 78 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH v3 01/10] 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.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/dirty-bitmap.c       |  9 +++++++++
 qapi/block-core.json       | 10 +++++++++-
 qemu-deprecated.texi       |  6 ++++++
 tests/qemu-iotests/236.out | 28 ++++++++++++++++++++++++++++
 4 files changed, 52 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 2b8afbb924..6e543594b3 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -458,7 +458,14 @@
 #
 # @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 2.4)
+#
+# @recording: true if the bitmap is recording new writes from the guest.
+#             Replaces `active` and `disabled` statuses. (since 4.0)
+#
+# @busy: true if the bitmap is in-use by some operation (NBD or jobs)
+#        and cannot be modified via QMP or used by another operation.
+#        Replaces `locked` and `frozen` statuses. (since 4.0)
 #
 # @persistent: true if the bitmap will eventually be flushed to persistent
 #              storage (since 4.0)
@@ -467,6 +474,7 @@
 ##
 { 'struct': 'BlockDirtyInfo',
   'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
+           'recording': 'bool', 'busy': 'bool',
            'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
 
 ##
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 45c57952da..4c7ae8180f 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -67,6 +67,12 @@ topologies described with -smp include all possible cpus, i.e.
 "autoload" parameter is now ignored. All bitmaps are automatically loaded
 from qcow2 images.
 
+@subsection query-block result field(s) dirty-bitmaps[i].status (since 4.0)
+
+The ``status'' field of the ``BlockDirtyInfo'' structure, returned by
+the query-block command is deprecated. Two new boolean fields,
+``recording'' and ``busy'' effectively replace it.
+
 @subsection query-cpus (since 2.12.0)
 
 The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
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 v3 02/10] 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 bdrv_enable_dirty_bitmap_locked and
bdrv_disable_dirty_bitmap_locked, it doesn't make sense to prohibit QEMU
internals from performing this action when we only wished to prohibit QMP
users from issuing these commands. All of the QMP API commands for bitmap
manipulation already check against user_locked() to prohibit these actions.

Several other assertions really want to check that the bitmap isn't in-use
by another operation -- use the bitmap_user_locked function for this instead,
which presently also checks for has_successor. This leaves some redundant
checks of has_sucessor through different helpers that are addressed in
forthcoming patches.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 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..aa3f86bb73 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 user_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_user_locked(bitmap)) {
+        error_setg(errp, "Cannot create a successor for a bitmap that is in-use "
+                   "by an operation");
+        return -1;
+    }
+    if (bdrv_dirty_bitmap_has_successor(bitmap)) {
+        error_setg(errp, "Cannot create a successor for a bitmap that already "
+                   "has one");
         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, nor 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 v3 03/10] block/dirty-bitmap: remove set/reset assertions against enabled bit

[John Snow]

bdrv_set_dirty_bitmap and bdrv_reset_dirty_bitmap are only used as an
internal API by the mirror and migration areas of our code. These
calls modify the bitmap, but do so at the behest of QEMU and not the
guest.

Presently, these bitmaps are always "enabled" anyway, but there's no
reason they have to be.

Modify these internal APIs to drop this assertion.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/dirty-bitmap.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index aa3f86bb73..9ea5738420 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -542,7 +542,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 +558,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 v3 04/10] block/dirty-bitmap: change semantics of enabled predicate

[John Snow]

Currently, the enabled predicate means something like:
"the QAPI 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.
Internal usages of the bitmap QPI can call user_locked to find 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() 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. To accommodate this, create_successor now ensures that
   any bitmap with a successor is explicitly disabled.

3. qcow2_store_persistent_dirty_bitmaps: No functional change. This function
   cares only about the literal enabled bit, and makes no effort to check if
   the bitmap is in-use or not. After this patch there are still no ways to
   produce an enabled bitmap with a successor.

4. block_dirty_bitmap_enable_prepare
   block_dirty_bitmap_disable_prepare
   init_dirty_bitmap_migration
   nbd_export_new

   These functions care about the literal enabled bit,
   and already check user_locked separately.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/dirty-bitmap.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 9ea5738420..976831e765 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.  */
@@ -236,6 +236,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 user_locked and has no successor.
+ * The successor will be enabled if the parent bitmap was.
  * Called with BQL taken.
  */
 int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
@@ -264,6 +265,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;
@@ -328,7 +330,8 @@ 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, nor explicitly re-enabled.
+ * The merged parent will not be user_locked.
+ * The marged parent will be enabled if and only if the successor was enabled.
  * Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken.
  */
 BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
@@ -346,6 +349,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;
 
-- 
2.17.2

[Qemu-devel] [PATCH v3 05/10] nbd: change error checking order for bitmaps

[John Snow]

Check that the bitmap is not in use prior to it checking if it is
not enabled/recording guest writes. The bitmap being busy was likely
at the behest of the user, so this error has a greater chance of being
understood by the user.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 nbd/server.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 0910d09a6d..de21c64ce3 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1510,6 +1510,11 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
             goto fail;
         }
 
+        if (bdrv_dirty_bitmap_user_locked(bm)) {
+            error_setg(errp, "Bitmap '%s' is in use", bitmap);
+            goto fail;
+        }
+
         if ((nbdflags & NBD_FLAG_READ_ONLY) && bdrv_is_writable(bs) &&
             bdrv_dirty_bitmap_enabled(bm)) {
             error_setg(errp,
@@ -1518,11 +1523,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
             goto fail;
         }
 
-        if (bdrv_dirty_bitmap_user_locked(bm)) {
-            error_setg(errp, "Bitmap '%s' is in use", bitmap);
-            goto fail;
-        }
-
         bdrv_dirty_bitmap_set_qmp_locked(bm, true);
         exp->export_bitmap = bm;
         exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s",
-- 
2.17.2

[Qemu-devel] [PATCH v3 06/10] block/dirty-bitmap: explicitly lock bitmaps with successors

[John Snow]

Instead of implying a user_locked/busy 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.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/dirty-bitmap.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 976831e765..d92a269753 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 user_locked state */
+    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,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)
@@ -267,8 +265,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;
 }
 
@@ -322,6 +321,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;
@@ -351,6 +351,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 v3 07/10] 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.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/dirty-bitmap.c           | 40 +++++++++++++++-------------------
 blockdev.c                     | 18 +++++++--------
 include/block/dirty-bitmap.h   |  5 ++---
 migration/block-dirty-bitmap.c |  6 ++---
 nbd/server.c                   |  6 ++---
 5 files changed, 34 insertions(+), 41 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index d92a269753..b3627b0d8c 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -48,8 +48,7 @@ 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 */
+    bool busy;                  /* Bitmap is busy, it can't be used via QMP */
     BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
     char *name;                 /* Optional non-empty unique ID */
     int64_t size;               /* Size of the bitmap, in bytes */
@@ -188,22 +187,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 +209,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;
@@ -243,7 +237,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
     uint64_t granularity;
     BdrvDirtyBitmap *child;
 
-    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;
@@ -265,9 +259,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
     child->disabled = bitmap->disabled;
     bitmap->disabled = true;
 
-    /* Install the successor and lock the parent */
+    /* Install the successor and mark the parent as busy */
     bitmap->successor = child;
-    bitmap->qmp_locked = true;
+    bitmap->busy = true;
     return 0;
 }
 
@@ -289,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);
@@ -321,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;
@@ -330,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.
+ * The merged parent will be marked as not busy.
  * The marged parent will be enabled if and only if the successor was enabled.
  * Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken.
  */
@@ -351,7 +345,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;
 
@@ -382,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_user_locked(bitmap));
+        assert(!bdrv_dirty_bitmap_busy(bitmap));
         assert(!bitmap->active_iterators);
         hbitmap_truncate(bitmap->bitmap, bytes);
         bitmap->size = bytes;
@@ -400,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 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.
  */
@@ -464,7 +458,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;
@@ -772,7 +766,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 8714ad2702..1aaadb6128 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);
@@ -2892,7 +2892,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);
@@ -2931,7 +2931,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);
@@ -2955,7 +2955,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);
@@ -2976,7 +2976,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);
@@ -3552,7 +3552,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);
@@ -3665,7 +3665,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 de21c64ce3..02773e2836 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1510,7 +1510,7 @@ 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;
         }
@@ -1523,7 +1523,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
             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);
@@ -1641,7 +1641,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

[Qemu-devel] [PATCH v3 08/10] block/dirty-bitmaps: move comment block

[John Snow]

Simply move the big status enum comment block to above the status
function, and document it as being deprecated. The whole confusing
block can get deleted in three releases time.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/dirty-bitmap.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index b3627b0d8c..86c3b87ab9 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -28,22 +28,6 @@
 #include "block/block_int.h"
 #include "block/blockjob.h"
 
-/**
- * A BdrvDirtyBitmap can be in four possible user-visible states:
- * (1) Active:   successor is NULL, and disabled is false: full r/w mode
- * (2) Disabled: successor is NULL, and disabled is true: qualified r/w mode,
- *               guest writes are dropped, but monitor writes are possible,
- *               through commands like merge and clear.
- * (3) Frozen:   successor is not NULL.
- *               A frozen bitmap cannot be renamed, deleted, cleared, set,
- *               enabled, merged to, etc. A frozen bitmap can only abdicate()
- *               or reclaim().
- *               In this state, the anonymous successor bitmap may be either
- *               Active and recording writes from the guest (e.g. backup jobs),
- *               but it can be Disabled and not recording writes.
- * (4) Locked:   Whether Active or Disabled, the user cannot modify this bitmap
- *               in any way from the monitor.
- */
 struct BdrvDirtyBitmap {
     QemuMutex *mutex;
     HBitmap *bitmap;            /* Dirty bitmap implementation */
@@ -204,7 +188,25 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
     return !bitmap->disabled;
 }
 
-/* Called with BQL taken.  */
+/**
+ * bdrv_dirty_bitmap_status: This API is now deprecated.
+ * Called with BQL taken.
+ *
+ * A BdrvDirtyBitmap can be in four possible user-visible states:
+ * (1) Active:   successor is NULL, and disabled is false: full r/w mode
+ * (2) Disabled: successor is NULL, and disabled is true: qualified r/w mode,
+ *               guest writes are dropped, but monitor writes are possible,
+ *               through commands like merge and clear.
+ * (3) Frozen:   successor is not NULL.
+ *               A frozen bitmap cannot be renamed, deleted, cleared, set,
+ *               enabled, merged to, etc. A frozen bitmap can only abdicate()
+ *               or reclaim().
+ *               In this state, the anonymous successor bitmap may be either
+ *               Active and recording writes from the guest (e.g. backup jobs),
+ *               or it can be Disabled and not recording writes.
+ * (4) Locked:   Whether Active or Disabled, the user cannot modify this bitmap
+ *               in any way from the monitor.
+ */
 DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
 {
     if (bdrv_dirty_bitmap_has_successor(bitmap)) {
-- 
2.17.2

[Qemu-devel] [PATCH v3 09/10] blockdev: remove unused paio parameter documentation

[John Snow]

This field isn't present anymore.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 1aaadb6128..cbce44877d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1255,7 +1255,6 @@ out_aio_context:
  * @node: The name of the BDS node to search for bitmaps
  * @name: The name of the bitmap to search for
  * @pbs: Output pointer for BDS lookup, if desired. Can be NULL.
- * @paio: Output pointer for aio_context acquisition, if desired. Can be NULL.
  * @errp: Output pointer for error information. Can be NULL.
  *
  * @return: A bitmap object on success, or NULL on failure.
-- 
2.17.2

[Qemu-devel] [PATCH v3 10/10] iotests: add busy/recording bit test to 124

[John Snow]

This adds a simple test that ensures the busy bit works for push backups,
as well as doubling as bonus test for incremental backups that get interrupted
by EIO errors.

Recording bit tests are already handled sufficiently by 236.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/124     | 110 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/124.out |   4 +-
 2 files changed, 112 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 5aa1bf1bd6..30f12a2202 100755
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -634,6 +634,116 @@ class TestIncrementalBackupBlkdebug(TestIncrementalBackupBase):
         self.vm.shutdown()
         self.check_backups()
 
+    def test_incremental_pause(self):
+        """
+        Test an incremental backup that errors into a pause and is resumed.
+        """
+
+        drive0 = self.drives[0]
+        result = self.vm.qmp('blockdev-add',
+                             node_name=drive0['id'],
+                             driver=drive0['fmt'],
+                             file={
+                                 'driver': 'blkdebug',
+                                 'image': {
+                                     'driver': 'file',
+                                     'filename': drive0['file']
+                                 },
+                                 'set-state': [{
+                                     'event': 'flush_to_disk',
+                                     'state': 1,
+                                     'new_state': 2
+                                 },{
+                                     'event': 'read_aio',
+                                     'state': 2,
+                                     'new_state': 3
+                                 }],
+                                 'inject-error': [{
+                                     'event': 'read_aio',
+                                     'errno': 5,
+                                     'state': 3,
+                                     'immediately': False,
+                                     'once': True
+                                 }],
+                             })
+        self.assert_qmp(result, 'return', {})
+        self.create_anchor_backup(drive0)
+        bitmap = self.add_bitmap('bitmap0', drive0)
+
+        # Emulate guest activity
+        self.hmp_io_writes(drive0['id'], (('0xab', 0, 512),
+                                          ('0xfe', '16M', '256k'),
+                                          ('0x64', '32736k', '64k')))
+
+        # For the purposes of query-block visibility of bitmaps, add a drive
+        # frontend after we've written data; otherwise we can't use hmp-io
+        result = self.vm.qmp("device_add",
+                             id="device0",
+                             drive=drive0['id'],
+                             driver="virtio-blk")
+        self.assert_qmp(result, 'return', {})
+
+        # Bitmap Status Check
+        query = self.vm.qmp('query-block')
+        ret = [bmap for bmap in query['return'][0]['dirty-bitmaps']
+               if bmap.get('name') == bitmap.name][0]
+        self.assert_qmp(ret, 'count', 458752)
+        self.assert_qmp(ret, 'granularity', 65536)
+        self.assert_qmp(ret, 'status', 'active')
+        self.assert_qmp(ret, 'busy', False)
+        self.assert_qmp(ret, 'recording', True)
+
+        # Start backup
+        parent, _ = bitmap.last_target()
+        target = self.prepare_backup(bitmap, parent)
+        res = self.vm.qmp('drive-backup',
+                          job_id=bitmap.drive['id'],
+                          device=bitmap.drive['id'],
+                          sync='incremental',
+                          bitmap=bitmap.name,
+                          format=bitmap.drive['fmt'],
+                          target=target,
+                          mode='existing',
+                          on_source_error='stop')
+        self.assert_qmp(res, 'return', {})
+
+        # Wait for the error
+        event = self.vm.event_wait(name="BLOCK_JOB_ERROR",
+                                   match={"data":{"device":bitmap.drive['id']}})
+        self.assert_qmp(event, 'data', {'device': bitmap.drive['id'],
+                                        'action': 'stop',
+                                        'operation': 'read'})
+
+        # Bitmap Status Check
+        query = self.vm.qmp('query-block')
+        ret = [bmap for bmap in query['return'][0]['dirty-bitmaps']
+               if bmap.get('name') == bitmap.name][0]
+        self.assert_qmp(ret, 'count', 458752)
+        self.assert_qmp(ret, 'granularity', 65536)
+        self.assert_qmp(ret, 'status', 'frozen')
+        self.assert_qmp(ret, 'busy', True)
+        self.assert_qmp(ret, 'recording', True)
+
+        # Resume and check incremental backup for consistency
+        res = self.vm.qmp('block-job-resume', device=bitmap.drive['id'])
+        self.assert_qmp(res, 'return', {})
+        self.wait_qmp_backup(bitmap.drive['id'])
+
+        # Bitmap Status Check
+        query = self.vm.qmp('query-block')
+        ret = [bmap for bmap in query['return'][0]['dirty-bitmaps']
+               if bmap.get('name') == bitmap.name][0]
+        self.assert_qmp(ret, 'count', 0)
+        self.assert_qmp(ret, 'granularity', 65536)
+        self.assert_qmp(ret, 'status', 'active')
+        self.assert_qmp(ret, 'busy', False)
+        self.assert_qmp(ret, 'recording', True)
+
+        # Finalize / Cleanup
+        self.make_reference_backup(bitmap)
+        self.vm.shutdown()
+        self.check_backups()
+
 
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out
index e56cae021b..281b69efea 100644
--- a/tests/qemu-iotests/124.out
+++ b/tests/qemu-iotests/124.out
@@ -1,5 +1,5 @@
-...........
+............
 ----------------------------------------------------------------------
-Ran 11 tests
+Ran 12 tests
 
 OK
-- 
2.17.2

Re: [Qemu-devel] [PATCH v3 01/10] block/dirty-bitmap: add recording and busy properties

[Eric Blake]

On 2/22/19 6:06 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.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---

> +++ b/qemu-deprecated.texi
> @@ -67,6 +67,12 @@ topologies described with -smp include all possible cpus, i.e.
>  "autoload" parameter is now ignored. All bitmaps are automatically loaded
>  from qcow2 images.
>  
> +@subsection query-block result field(s) dirty-bitmaps[i].status (since 4.0)

I suspect you wrote "field(s)" since there can be more than one
dirty-bitmaps[i]. But it still sound funny; within a single
dirty-bitmaps[i], there is only one field being deprecated. Dropping the
'(s)' makes this subsection read better to me.

That's minor enough that a maintainer could fix it, if you don't have
any other reason to spin v4.

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

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

Re: [Qemu-devel] [PATCH v3 02/10] block/dirty-bitmaps: rename frozen predicate helper

[Eric Blake]

On 2/22/19 6:06 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 bdrv_enable_dirty_bitmap_locked and
> bdrv_disable_dirty_bitmap_locked, it doesn't make sense to prohibit QEMU
> internals from performing this action when we only wished to prohibit QMP
> users from issuing these commands. All of the QMP API commands for bitmap
> manipulation already check against user_locked() to prohibit these actions.
> 
> Several other assertions really want to check that the bitmap isn't in-use
> by another operation -- use the bitmap_user_locked function for this instead,
> which presently also checks for has_successor. This leaves some redundant
> checks of has_sucessor through different helpers that are addressed in

successor (maintainer can fix, if we don't need v4)

> forthcoming patches.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/dirty-bitmap.c           | 32 +++++++++++++++++---------------
>  include/block/dirty-bitmap.h   |  2 +-
>  migration/block-dirty-bitmap.c |  2 +-
>  3 files changed, 19 insertions(+), 17 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

Re: [Qemu-devel] [PATCH v3 03/10] block/dirty-bitmap: remove set/reset assertions against enabled bit

[Eric Blake]

On 2/22/19 6:06 PM, John Snow wrote:
> bdrv_set_dirty_bitmap and bdrv_reset_dirty_bitmap are only used as an
> internal API by the mirror and migration areas of our code. These
> calls modify the bitmap, but do so at the behest of QEMU and not the
> guest.
> 
> Presently, these bitmaps are always "enabled" anyway, but there's no
> reason they have to be.
> 
> Modify these internal APIs to drop this assertion.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/dirty-bitmap.c | 2 --
>  1 file changed, 2 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

Re: [Qemu-devel] [PATCH v3 04/10] block/dirty-bitmap: change semantics of enabled predicate

[Eric Blake]

On 2/22/19 6:06 PM, John Snow wrote:
> Currently, the enabled predicate means something like:
> "the QAPI 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.
> Internal usages of the bitmap QPI can call user_locked to find 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.
> 

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

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

Re: [Qemu-devel] [PATCH v3 05/10] nbd: change error checking order for bitmaps

[Eric Blake]

On 2/22/19 6:06 PM, John Snow wrote:
> Check that the bitmap is not in use prior to it checking if it is
> not enabled/recording guest writes. The bitmap being busy was likely
> at the behest of the user, so this error has a greater chance of being
> understood by the user.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  nbd/server.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 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

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

[Eric Blake]

On 2/22/19 6:06 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.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---

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

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

Re: [Qemu-devel] [PATCH v3 08/10] block/dirty-bitmaps: move comment block

[Eric Blake]

On 2/22/19 6:06 PM, John Snow wrote:
> Simply move the big status enum comment block to above the status
> function, and document it as being deprecated. The whole confusing
> block can get deleted in three releases time.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/dirty-bitmap.c | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 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

Re: [Qemu-devel] [PATCH v3 09/10] blockdev: remove unused paio parameter documentation

[Eric Blake]

On 2/22/19 6:06 PM, John Snow wrote:
> This field isn't present anymore.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  blockdev.c | 1 -
>  1 file changed, 1 deletion(-)

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

> 
> diff --git a/blockdev.c b/blockdev.c
> index 1aaadb6128..cbce44877d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1255,7 +1255,6 @@ out_aio_context:
>   * @node: The name of the BDS node to search for bitmaps
>   * @name: The name of the bitmap to search for
>   * @pbs: Output pointer for BDS lookup, if desired. Can be NULL.
> - * @paio: Output pointer for aio_context acquisition, if desired. Can be NULL.
>   * @errp: Output pointer for error information. Can be NULL.
>   *
>   * @return: A bitmap object on success, or NULL on failure.
> 

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

Re: [Qemu-devel] [PATCH v3 10/10] iotests: add busy/recording bit test to 124

[Eric Blake]

On 2/22/19 6:06 PM, John Snow wrote:
> This adds a simple test that ensures the busy bit works for push backups,
> as well as doubling as bonus test for incremental backups that get interrupted
> by EIO errors.

This makes 124 longer to run, but it is not in the 'quick' group, so
that's okay.

The easy part: I compiled the series, and validated that ./check still
passes with this applied, so:

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

Now for the fun part (I know from IRC that you had some interesting
challenges coming up with scenarios to even provoke the states you
wanted shown in the output):

> 
> Recording bit tests are already handled sufficiently by 236.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/124     | 110 +++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/124.out |   4 +-
>  2 files changed, 112 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
> index 5aa1bf1bd6..30f12a2202 100755
> --- a/tests/qemu-iotests/124
> +++ b/tests/qemu-iotests/124
> @@ -634,6 +634,116 @@ class TestIncrementalBackupBlkdebug(TestIncrementalBackupBase):
>          self.vm.shutdown()
>          self.check_backups()
>  
> +    def test_incremental_pause(self):
> +        """
> +        Test an incremental backup that errors into a pause and is resumed.
> +        """
> +
> +        drive0 = self.drives[0]
> +        result = self.vm.qmp('blockdev-add',
> +                             node_name=drive0['id'],
> +                             driver=drive0['fmt'],
> +                             file={
> +                                 'driver': 'blkdebug',
> +                                 'image': {
> +                                     'driver': 'file',
> +                                     'filename': drive0['file']
> +                                 },
> +                                 'set-state': [{
> +                                     'event': 'flush_to_disk',
> +                                     'state': 1,
> +                                     'new_state': 2
> +                                 },{
> +                                     'event': 'read_aio',
> +                                     'state': 2,
> +                                     'new_state': 3
> +                                 }],
> +                                 'inject-error': [{
> +                                     'event': 'read_aio',
> +                                     'errno': 5,
> +                                     'state': 3,
> +                                     'immediately': False,
> +                                     'once': True
> +                                 }],

Yeah, it's hairy to come up with sequences that will pause at the right
place, and this may be sensitive enough that we have to revisit it in
the future, but for now it works and I don't have any better suggestions.

> +                             })
> +        self.assert_qmp(result, 'return', {})
> +        self.create_anchor_backup(drive0)
> +        bitmap = self.add_bitmap('bitmap0', drive0)
> +
> +        # Emulate guest activity
> +        self.hmp_io_writes(drive0['id'], (('0xab', 0, 512),
> +                                          ('0xfe', '16M', '256k'),
> +                                          ('0x64', '32736k', '64k')))
> +
> +        # For the purposes of query-block visibility of bitmaps, add a drive
> +        # frontend after we've written data; otherwise we can't use hmp-io
> +        result = self.vm.qmp("device_add",
> +                             id="device0",
> +                             drive=drive0['id'],
> +                             driver="virtio-blk")
> +        self.assert_qmp(result, 'return', {})
> +
> +        # Bitmap Status Check
> +        query = self.vm.qmp('query-block')
> +        ret = [bmap for bmap in query['return'][0]['dirty-bitmaps']
> +               if bmap.get('name') == bitmap.name][0]
> +        self.assert_qmp(ret, 'count', 458752)
> +        self.assert_qmp(ret, 'granularity', 65536)
> +        self.assert_qmp(ret, 'status', 'active')
> +        self.assert_qmp(ret, 'busy', False)
> +        self.assert_qmp(ret, 'recording', True)

So far, nothing too different from we've seen elsewhere, but then we get
to the fun part:

> +
> +        # Start backup
> +        parent, _ = bitmap.last_target()
> +        target = self.prepare_backup(bitmap, parent)
> +        res = self.vm.qmp('drive-backup',
> +                          job_id=bitmap.drive['id'],
> +                          device=bitmap.drive['id'],
> +                          sync='incremental',
> +                          bitmap=bitmap.name,
> +                          format=bitmap.drive['fmt'],
> +                          target=target,
> +                          mode='existing',
> +                          on_source_error='stop')
> +        self.assert_qmp(res, 'return', {})
> +
> +        # Wait for the error
> +        event = self.vm.event_wait(name="BLOCK_JOB_ERROR",
> +                                   match={"data":{"device":bitmap.drive['id']}})
> +        self.assert_qmp(event, 'data', {'device': bitmap.drive['id'],
> +                                        'action': 'stop',
> +                                        'operation': 'read'})
> +
> +        # Bitmap Status Check
> +        query = self.vm.qmp('query-block')
> +        ret = [bmap for bmap in query['return'][0]['dirty-bitmaps']
> +               if bmap.get('name') == bitmap.name][0]
> +        self.assert_qmp(ret, 'count', 458752)
> +        self.assert_qmp(ret, 'granularity', 65536)
> +        self.assert_qmp(ret, 'status', 'frozen')
> +        self.assert_qmp(ret, 'busy', True)
> +        self.assert_qmp(ret, 'recording', True)

"status":"frozen", "busy":true - yay, you provoked the other QMP states
that we have not seen elsewhere in the testsuite.

> +
> +        # Resume and check incremental backup for consistency
> +        res = self.vm.qmp('block-job-resume', device=bitmap.drive['id'])
> +        self.assert_qmp(res, 'return', {})
> +        self.wait_qmp_backup(bitmap.drive['id'])
> +
> +        # Bitmap Status Check
> +        query = self.vm.qmp('query-block')
> +        ret = [bmap for bmap in query['return'][0]['dirty-bitmaps']
> +               if bmap.get('name') == bitmap.name][0]
> +        self.assert_qmp(ret, 'count', 0)
> +        self.assert_qmp(ret, 'granularity', 65536)
> +        self.assert_qmp(ret, 'status', 'active')
> +        self.assert_qmp(ret, 'busy', False)
> +        self.assert_qmp(ret, 'recording', True)

and even nicer, after the transient error goes away, it does not get in
the way of further bitmap edits.

> +
> +        # Finalize / Cleanup
> +        self.make_reference_backup(bitmap)
> +        self.vm.shutdown()
> +        self.check_backups()
> +
>  
>  if __name__ == '__main__':
>      iotests.main(supported_fmts=['qcow2'])
> diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out
> index e56cae021b..281b69efea 100644
> --- a/tests/qemu-iotests/124.out
> +++ b/tests/qemu-iotests/124.out
> @@ -1,5 +1,5 @@
> -...........
> +............
>  ----------------------------------------------------------------------
> -Ran 11 tests
> +Ran 12 tests

Pre-existing, but this output is a bear to debug if the test ever starts
failing.  Not something you have to worry about today, though.

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

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

Re: [Qemu-devel] [PATCH v3 01/10] block/dirty-bitmap: add recording and busy properties

[Vladimir Sementsov-Ogievskiy]

23.02.2019 3:06, 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.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 02/10] block/dirty-bitmaps: rename frozen predicate helper

[Vladimir Sementsov-Ogievskiy]

23.02.2019 3:06, 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 bdrv_enable_dirty_bitmap_locked and
> bdrv_disable_dirty_bitmap_locked, it doesn't make sense to prohibit QEMU
> internals from performing this action when we only wished to prohibit QMP
> users from issuing these commands. All of the QMP API commands for bitmap
> manipulation already check against user_locked() to prohibit these actions.
> 
> Several other assertions really want to check that the bitmap isn't in-use
> by another operation -- use the bitmap_user_locked function for this instead,
> which presently also checks for has_successor. This leaves some redundant
> checks of has_sucessor through different helpers that are addressed in
> forthcoming patches.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   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..aa3f86bb73 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c

[..]

> @@ -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));

hmm. I'd prefere to keep a separate assertion for successor: we don't free it here,
so we really depend on absence of this object.

>       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, nor 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));

and here too. As we don't truncate successors.

>           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

Could you please add scripts/git.orderfile to your .git/config, like

[diff]
     orderFile = /path/to/scripts/git.orderfile

?

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


With my suggestions or without:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

And thank you for rewriting commit message, it's more clear now!

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 03/10] block/dirty-bitmap: remove set/reset assertions against enabled bit

[Vladimir Sementsov-Ogievskiy]

about subject: shouldn't it be "against disabled bit" instead?

23.02.2019 3:06, John Snow wrote:
> bdrv_set_dirty_bitmap and bdrv_reset_dirty_bitmap are only used as an
> internal API by the mirror and migration areas of our code. These
> calls modify the bitmap, but do so at the behest of QEMU and not the
> guest.
> 
> Presently, these bitmaps are always "enabled" anyway, but there's no
> reason they have to be.
> 
> Modify these internal APIs to drop this assertion.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   block/dirty-bitmap.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index aa3f86bb73..9ea5738420 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -542,7 +542,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 +558,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);
>   }
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 04/10] block/dirty-bitmap: change semantics of enabled predicate

[Vladimir Sementsov-Ogievskiy]

23.02.2019 3:06, John Snow wrote:
> Currently, the enabled predicate means something like:
> "the QAPI 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.
> Internal usages of the bitmap QPI can call user_locked to find 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() 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. To accommodate this, create_successor now ensures that
>     any bitmap with a successor is explicitly disabled.
> 
> 3. qcow2_store_persistent_dirty_bitmaps: No functional change. This function
>     cares only about the literal enabled bit, and makes no effort to check if
>     the bitmap is in-use or not. After this patch there are still no ways to
>     produce an enabled bitmap with a successor.
> 
> 4. block_dirty_bitmap_enable_prepare
>     block_dirty_bitmap_disable_prepare
>     init_dirty_bitmap_migration
>     nbd_export_new
> 
>     These functions care about the literal enabled bit,
>     and already check user_locked separately.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 05/10] nbd: change error checking order for bitmaps

[Vladimir Sementsov-Ogievskiy]

23.02.2019 3:06, John Snow wrote:
> Check that the bitmap is not in use prior to it checking if it is
> not enabled/recording guest writes. The bitmap being busy was likely
> at the behest of the user, so this error has a greater chance of being
> understood by the user.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 06/10] block/dirty-bitmap: explicitly lock bitmaps with successors

[Vladimir Sementsov-Ogievskiy]

23.02.2019 3:06, John Snow wrote:
> Instead of implying a user_locked/busy 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.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>
> Reviewed-by: Eric Blake<eblake@redhat.com>

hmm, you forget my r-b:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

23.02.2019 3:06, 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.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Hmm, and here, you directly fixed what I've noticed, so my r-b,
of course, still applies.

Ha, but I noticed funny thing:

> ---
>   block/dirty-bitmap.c           | 40 +++++++++++++++-------------------
>   blockdev.c                     | 18 +++++++--------
>   include/block/dirty-bitmap.h   |  5 ++---
>   migration/block-dirty-bitmap.c |  6 ++---
>   nbd/server.c                   |  6 ++---
>   5 files changed, 34 insertions(+), 41 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index d92a269753..b3627b0d8c 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c

[..]

> @@ -265,9 +259,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>       child->disabled = bitmap->disabled;
>       bitmap->disabled = true;
>   
> -    /* Install the successor and lock the parent */
> +    /* Install the successor and mark the parent as busy */

I asked you to fix comment for bdrv_dirty_bitmap_create_successor(), but I didn't
noticed this one, I meant the comment to the whole function, placed above it, which
now have
  "Requires that the bitmap is not user_locked and has no successor."

So, it should be updated too.

and with it:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

>       bitmap->successor = child;
> -    bitmap->qmp_locked = true;
> +    bitmap->busy = true;
>       return 0;
>   }
>   



-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 08/10] block/dirty-bitmaps: move comment block

[Vladimir Sementsov-Ogievskiy]

23.02.2019 3:06, John Snow wrote:
> Simply move the big status enum comment block to above the status
> function, and document it as being deprecated. The whole confusing
> block can get deleted in three releases time.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 09/10] blockdev: remove unused paio parameter documentation

[Vladimir Sementsov-Ogievskiy]

23.02.2019 3:06, John Snow wrote:
> This field isn't present anymore.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   blockdev.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 1aaadb6128..cbce44877d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1255,7 +1255,6 @@ out_aio_context:
>    * @node: The name of the BDS node to search for bitmaps
>    * @name: The name of the bitmap to search for
>    * @pbs: Output pointer for BDS lookup, if desired. Can be NULL.
> - * @paio: Output pointer for aio_context acquisition, if desired. Can be NULL.
>    * @errp: Output pointer for error information. Can be NULL.
>    *
>    * @return: A bitmap object on success, or NULL on failure.
> 

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 01/10] block/dirty-bitmap: add recording and busy properties

[Vladimir Sementsov-Ogievskiy]

23.02.2019 3:06, 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.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block/dirty-bitmap.c       |  9 +++++++++
>   qapi/block-core.json       | 10 +++++++++-
>   qemu-deprecated.texi       |  6 ++++++
>   tests/qemu-iotests/236.out | 28 ++++++++++++++++++++++++++++
>   4 files changed, 52 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 2b8afbb924..6e543594b3 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -458,7 +458,14 @@
>   #
>   # @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 2.4)
> +#
> +# @recording: true if the bitmap is recording new writes from the guest.
> +#             Replaces `active` and `disabled` statuses. (since 4.0)
> +#
> +# @busy: true if the bitmap is in-use by some operation (NBD or jobs)
> +#        and cannot be modified via QMP or used by another operation.
> +#        Replaces `locked` and `frozen` statuses. (since 4.0)


Don't we want instead an array of flags? Which will include also persistent and
inconsistent?


>   #
>   # @persistent: true if the bitmap will eventually be flushed to persistent
>   #              storage (since 4.0)
> @@ -467,6 +474,7 @@
>   ##
>   { 'struct': 'BlockDirtyInfo',
>     'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
> +           'recording': 'bool', 'busy': 'bool',
>              'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
>   
>   ##
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 45c57952da..4c7ae8180f 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -67,6 +67,12 @@ topologies described with -smp include all possible cpus, i.e.
>   "autoload" parameter is now ignored. All bitmaps are automatically loaded
>   from qcow2 images.
>   
> +@subsection query-block result field(s) dirty-bitmaps[i].status (since 4.0)
> +
> +The ``status'' field of the ``BlockDirtyInfo'' structure, returned by
> +the query-block command is deprecated. Two new boolean fields,
> +``recording'' and ``busy'' effectively replace it.
> +
>   @subsection query-cpus (since 2.12.0)
>   
>   The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
> 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"
>         }
>       ]
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v3 01/10] block/dirty-bitmap: add recording and busy properties

[Eric Blake]

On 2/25/19 9:01 AM, Vladimir Sementsov-Ogievskiy wrote:
> 23.02.2019 3:06, 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.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---

>> +++ b/qapi/block-core.json
>> @@ -458,7 +458,14 @@
>>   #
>>   # @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 2.4)
>> +#
>> +# @recording: true if the bitmap is recording new writes from the guest.
>> +#             Replaces `active` and `disabled` statuses. (since 4.0)
>> +#
>> +# @busy: true if the bitmap is in-use by some operation (NBD or jobs)
>> +#        and cannot be modified via QMP or used by another operation.
>> +#        Replaces `locked` and `frozen` statuses. (since 4.0)
> 
> 
> Don't we want instead an array of flags? Which will include also persistent and
> inconsistent?

No, I don't think an array of flags is worth the extra complications in
generation and parsing of the JSON.

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

Re: [Qemu-devel] [PATCH v3 02/10] block/dirty-bitmaps: rename frozen predicate helper

[John Snow]

On 2/25/19 2:01 AM, Vladimir Sementsov-Ogievskiy wrote:
> 23.02.2019 3:06, 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 bdrv_enable_dirty_bitmap_locked and
>> bdrv_disable_dirty_bitmap_locked, it doesn't make sense to prohibit QEMU
>> internals from performing this action when we only wished to prohibit QMP
>> users from issuing these commands. All of the QMP API commands for bitmap
>> manipulation already check against user_locked() to prohibit these actions.
>>
>> Several other assertions really want to check that the bitmap isn't in-use
>> by another operation -- use the bitmap_user_locked function for this instead,
>> which presently also checks for has_successor. This leaves some redundant
>> checks of has_sucessor through different helpers that are addressed in
>> forthcoming patches.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   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..aa3f86bb73 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
> 
> [..]
> 
>> @@ -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));
> 
> hmm. I'd prefere to keep a separate assertion for successor: we don't free it here,
> so we really depend on absence of this object.
> 

Reasonable.

>>       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, nor 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));
> 
> and here too. As we don't truncate successors.
> 
>>           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
> 
> Could you please add scripts/git.orderfile to your .git/config, like
> 
> [diff]
>      orderFile = /path/to/scripts/git.orderfile
> 
> ?
> 

Hm, we should add this to the .gitpublish profile, but yes, let me
re-add this.

>> @@ -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 */
>>
> 
> 
> With my suggestions or without:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> And thank you for rewriting commit message, it's more clear now!
> 

Thanks!

Re: [Qemu-devel] [PATCH v3 10/10] iotests: add busy/recording bit test to 124

[John Snow]

On 2/23/19 5:06 PM, Eric Blake wrote:
> On 2/22/19 6:06 PM, John Snow wrote:
>> This adds a simple test that ensures the busy bit works for push backups,
>> as well as doubling as bonus test for incremental backups that get interrupted
>> by EIO errors.
> 
> This makes 124 longer to run, but it is not in the 'quick' group, so
> that's okay.
> 
> The easy part: I compiled the series, and validated that ./check still
> passes with this applied, so:
> 
> Tested-by: Eric Blake <eblake@redhat.com>
> 
> Now for the fun part (I know from IRC that you had some interesting
> challenges coming up with scenarios to even provoke the states you
> wanted shown in the output):
> 
>>
>> Recording bit tests are already handled sufficiently by 236.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  tests/qemu-iotests/124     | 110 +++++++++++++++++++++++++++++++++++++
>>  tests/qemu-iotests/124.out |   4 +-
>>  2 files changed, 112 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
>> index 5aa1bf1bd6..30f12a2202 100755
>> --- a/tests/qemu-iotests/124
>> +++ b/tests/qemu-iotests/124
>> @@ -634,6 +634,116 @@ class TestIncrementalBackupBlkdebug(TestIncrementalBackupBase):
>>          self.vm.shutdown()
>>          self.check_backups()
>>  
>> +    def test_incremental_pause(self):
>> +        """
>> +        Test an incremental backup that errors into a pause and is resumed.
>> +        """
>> +
>> +        drive0 = self.drives[0]
>> +        result = self.vm.qmp('blockdev-add',
>> +                             node_name=drive0['id'],
>> +                             driver=drive0['fmt'],
>> +                             file={
>> +                                 'driver': 'blkdebug',
>> +                                 'image': {
>> +                                     'driver': 'file',
>> +                                     'filename': drive0['file']
>> +                                 },
>> +                                 'set-state': [{
>> +                                     'event': 'flush_to_disk',
>> +                                     'state': 1,
>> +                                     'new_state': 2
>> +                                 },{
>> +                                     'event': 'read_aio',
>> +                                     'state': 2,
>> +                                     'new_state': 3
>> +                                 }],
>> +                                 'inject-error': [{
>> +                                     'event': 'read_aio',
>> +                                     'errno': 5,
>> +                                     'state': 3,
>> +                                     'immediately': False,
>> +                                     'once': True
>> +                                 }],
> 
> Yeah, it's hairy to come up with sequences that will pause at the right
> place, and this may be sensitive enough that we have to revisit it in
> the future, but for now it works and I don't have any better suggestions.
> 

The problem in this case was that the drive_add itself provokes a
read_aio. I didn't dig too far to see why, but I needed an extra
read_aio state in the sequence there so that the error would occur
during the actual backup job.

I think I'll document this in the test, with at least a comment.

I wonder if there's a syntactical sugar we could write that makes this
easier to read, like:

'inject-error': [{ 'event': 'flush_to_disk->read_aio->read_aio' }] or
some such, but that's probably at odds with the existing interface and
not worth my time to go on a crusade about.

>> +                             })
>> +        self.assert_qmp(result, 'return', {})
>> +        self.create_anchor_backup(drive0)
>> +        bitmap = self.add_bitmap('bitmap0', drive0)
>> +
>> +        # Emulate guest activity
>> +        self.hmp_io_writes(drive0['id'], (('0xab', 0, 512),
>> +                                          ('0xfe', '16M', '256k'),
>> +                                          ('0x64', '32736k', '64k')))
>> +
>> +        # For the purposes of query-block visibility of bitmaps, add a drive
>> +        # frontend after we've written data; otherwise we can't use hmp-io
>> +        result = self.vm.qmp("device_add",
>> +                             id="device0",
>> +                             drive=drive0['id'],
>> +                             driver="virtio-blk")
>> +        self.assert_qmp(result, 'return', {})
>> +
>> +        # Bitmap Status Check
>> +        query = self.vm.qmp('query-block')
>> +        ret = [bmap for bmap in query['return'][0]['dirty-bitmaps']
>> +               if bmap.get('name') == bitmap.name][0]
>> +        self.assert_qmp(ret, 'count', 458752)
>> +        self.assert_qmp(ret, 'granularity', 65536)
>> +        self.assert_qmp(ret, 'status', 'active')
>> +        self.assert_qmp(ret, 'busy', False)
>> +        self.assert_qmp(ret, 'recording', True)
> 
> So far, nothing too different from we've seen elsewhere, but then we get
> to the fun part:
> 
>> +
>> +        # Start backup
>> +        parent, _ = bitmap.last_target()
>> +        target = self.prepare_backup(bitmap, parent)
>> +        res = self.vm.qmp('drive-backup',
>> +                          job_id=bitmap.drive['id'],
>> +                          device=bitmap.drive['id'],
>> +                          sync='incremental',
>> +                          bitmap=bitmap.name,
>> +                          format=bitmap.drive['fmt'],
>> +                          target=target,
>> +                          mode='existing',
>> +                          on_source_error='stop')
>> +        self.assert_qmp(res, 'return', {})
>> +
>> +        # Wait for the error
>> +        event = self.vm.event_wait(name="BLOCK_JOB_ERROR",
>> +                                   match={"data":{"device":bitmap.drive['id']}})
>> +        self.assert_qmp(event, 'data', {'device': bitmap.drive['id'],
>> +                                        'action': 'stop',
>> +                                        'operation': 'read'})
>> +
>> +        # Bitmap Status Check
>> +        query = self.vm.qmp('query-block')
>> +        ret = [bmap for bmap in query['return'][0]['dirty-bitmaps']
>> +               if bmap.get('name') == bitmap.name][0]
>> +        self.assert_qmp(ret, 'count', 458752)
>> +        self.assert_qmp(ret, 'granularity', 65536)
>> +        self.assert_qmp(ret, 'status', 'frozen')
>> +        self.assert_qmp(ret, 'busy', True)
>> +        self.assert_qmp(ret, 'recording', True)
> 
> "status":"frozen", "busy":true - yay, you provoked the other QMP states
> that we have not seen elsewhere in the testsuite.
> 

It also tests that you can query a bitmap during an error-paused backup
job which is new.

>> +
>> +        # Resume and check incremental backup for consistency
>> +        res = self.vm.qmp('block-job-resume', device=bitmap.drive['id'])
>> +        self.assert_qmp(res, 'return', {})
>> +        self.wait_qmp_backup(bitmap.drive['id'])
>> +
>> +        # Bitmap Status Check
>> +        query = self.vm.qmp('query-block')
>> +        ret = [bmap for bmap in query['return'][0]['dirty-bitmaps']
>> +               if bmap.get('name') == bitmap.name][0]
>> +        self.assert_qmp(ret, 'count', 0)
>> +        self.assert_qmp(ret, 'granularity', 65536)
>> +        self.assert_qmp(ret, 'status', 'active')
>> +        self.assert_qmp(ret, 'busy', False)
>> +        self.assert_qmp(ret, 'recording', True)
> 
> and even nicer, after the transient error goes away, it does not get in
> the way of further bitmap edits.
> 

And that the job can successfully finish, the bitmap is correct, etc. It
does not, sadly, test further guest writes because of the thorny issue
with the drive frontend prohibiting writes from HMP.

>> +
>> +        # Finalize / Cleanup
>> +        self.make_reference_backup(bitmap)
>> +        self.vm.shutdown()
>> +        self.check_backups()
>> +
>>  
>>  if __name__ == '__main__':
>>      iotests.main(supported_fmts=['qcow2'])
>> diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out
>> index e56cae021b..281b69efea 100644
>> --- a/tests/qemu-iotests/124.out
>> +++ b/tests/qemu-iotests/124.out
>> @@ -1,5 +1,5 @@
>> -...........
>> +............
>>  ----------------------------------------------------------------------
>> -Ran 11 tests
>> +Ran 12 tests
> 
> Pre-existing, but this output is a bear to debug if the test ever starts
> failing.  Not something you have to worry about today, though.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks!

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

[John Snow]

On 2/25/19 7:03 AM, Vladimir Sementsov-Ogievskiy wrote:
> 23.02.2019 3:06, 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.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> Hmm, and here, you directly fixed what I've noticed, so my r-b,
> of course, still applies.
> 

Sorry for not adding them. I didn't plan to merge the series until you
got a chance to review it, so I wasn't worried about missing it in the
long run. Sorry if it makes it harder to see which ones you'd like to
look at.

> Ha, but I noticed funny thing:
> 
>> ---
>>   block/dirty-bitmap.c           | 40 +++++++++++++++-------------------
>>   blockdev.c                     | 18 +++++++--------
>>   include/block/dirty-bitmap.h   |  5 ++---
>>   migration/block-dirty-bitmap.c |  6 ++---
>>   nbd/server.c                   |  6 ++---
>>   5 files changed, 34 insertions(+), 41 deletions(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index d92a269753..b3627b0d8c 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
> 
> [..]
> 
>> @@ -265,9 +259,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
>>       child->disabled = bitmap->disabled;
>>       bitmap->disabled = true;
>>   
>> -    /* Install the successor and lock the parent */
>> +    /* Install the successor and mark the parent as busy */
> 
> I asked you to fix comment for bdrv_dirty_bitmap_create_successor(), but I didn't
> noticed this one, I meant the comment to the whole function, placed above it, which
> now have
>   "Requires that the bitmap is not user_locked and has no successor."
> 
> So, it should be updated too.
> 
> and with it:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 

Ha. OK, updated to "is not marked busy"

Thanks for the reviews!

>>       bitmap->successor = child;
>> -    bitmap->qmp_locked = true;
>> +    bitmap->busy = true;
>>       return 0;
>>   }
>>   
> 

Re: [Qemu-devel] [PATCH v3 00/10] dirty-bitmaps: deprecate @status field

[John Snow]

On 2/22/19 7:06 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.
> 
> Well, in my opinion.
> 
> Based on my current bitmaps branch.
> 
> V3:
> 
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/10:[0002] [FC] 'block/dirty-bitmap: add recording and busy properties'
> 002/10:[0002] [FC] 'block/dirty-bitmaps: rename frozen predicate helper'
> 003/10:[down] 'block/dirty-bitmap: remove set/reset assertions against enabled bit'
> 004/10:[0006] [FC] 'block/dirty-bitmap: change semantics of enabled predicate'
> 005/10:[down] 'nbd: change error checking order for bitmaps'
> 006/10:[0002] [FC] 'block/dirty-bitmap: explicitly lock bitmaps with successors'
> 007/10:[0011] [FC] 'block/dirty-bitmaps: unify qmp_locked and user_locked calls'
> 008/10:[0002] [FC] 'block/dirty-bitmaps: move comment block'
> 009/10:[down] 'blockdev: remove unused paio parameter documentation'
> 010/10:[down] 'iotests: add busy/recording bit test to 124'
> 
> V3: (Following Vladimir's feedback)
> 
> 001: Changed texi phrasing, parameter --> field
> 002: Commit message adjustment
>      comment edit: "not locked" --> "not user_locked"
> 003: pulled forward out of patch 004
> 004: Commit message rewrite
>      create_successor and abdicate doc adjustments
> 005: New.
> 006: Change successor doc comment
>      Commit message adjustment
> 007: BdrvDirtyBitmap struct comment adjustments
>      Comment change to create_successor (lock --> busy)
>      Incidental changes from 004's changes
> 008: Grammar fix (Eric)
> 009: New, trivial, unrelated fix tagging along.
> 010: iotest 124 added.
> 
> John Snow (10):
>   block/dirty-bitmap: add recording and busy properties
>   block/dirty-bitmaps: rename frozen predicate helper
>   block/dirty-bitmap: remove set/reset assertions against enabled bit
>   block/dirty-bitmap: change semantics of enabled predicate
>   nbd: change error checking order for bitmaps
>   block/dirty-bitmap: explicitly lock bitmaps with successors
>   block/dirty-bitmaps: unify qmp_locked and user_locked calls
>   block/dirty-bitmaps: move comment block
>   blockdev: remove unused paio parameter documentation
>   iotests: add busy/recording bit test to 124
> 
>  block/dirty-bitmap.c           | 111 ++++++++++++++++++---------------
>  blockdev.c                     |  19 +++---
>  include/block/dirty-bitmap.h   |   7 +--
>  migration/block-dirty-bitmap.c |   8 +--
>  nbd/server.c                   |  14 ++---
>  qapi/block-core.json           |  10 ++-
>  qemu-deprecated.texi           |   6 ++
>  tests/qemu-iotests/124         | 110 ++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/124.out     |   4 +-
>  tests/qemu-iotests/236.out     |  28 +++++++++
>  10 files changed, 239 insertions(+), 78 deletions(-)
> 

Pushed to my bitmaps branch with the following minor changes:

001/10:[0002] [FC] 'block/dirty-bitmap: add recording and busy properties'
002/10:[0002] [FC] 'block/dirty-bitmaps: rename frozen predicate helper'
003/10:[----] [--] 'block/dirty-bitmap: remove set/reset assertions
against enabled bit'
004/10:[----] [--] 'block/dirty-bitmap: change semantics of enabled
predicate'
005/10:[----] [--] 'nbd: change error checking order for bitmaps'
006/10:[----] [--] 'block/dirty-bitmap: explicitly lock bitmaps with
successors'
007/10:[0002] [FC] 'block/dirty-bitmaps: unify qmp_locked and
user_locked calls'
008/10:[----] [--] 'block/dirty-bitmaps: move comment block'
009/10:[----] [--] 'blockdev: remove unused paio parameter documentation'
010/10:[0003] [FC] 'iotests: add busy/recording bit test to 124'


001: Texi doc spelling (Eric)
002: Commit message spelling (Eric)
     Additional asserts (Vladimir)
007: Comment phrasing (Vladimir)
010: Extra test comment

Re: [Qemu-devel] [PATCH v3 00/10] dirty-bitmaps: deprecate @status field

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190223000614.13894-1-jsnow@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20190223000614.13894-1-jsnow@redhat.com
Subject: [Qemu-devel] [PATCH v3 00/10] dirty-bitmaps: deprecate @status field
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20190220160628.6555-1-marcandre.lureau@redhat.com -> patchew/20190220160628.6555-1-marcandre.lureau@redhat.com
Switched to a new branch 'test'
5615f81566 iotests: add busy/recording bit test to 124
7c4f722b89 blockdev: remove unused paio parameter documentation
30e0427368 block/dirty-bitmaps: move comment block
a29ece1728 block/dirty-bitmaps: unify qmp_locked and user_locked calls
3d0665404c block/dirty-bitmap: explicitly lock bitmaps with successors
d932d4e9c4 nbd: change error checking order for bitmaps
2ee1f03bae block/dirty-bitmap: change semantics of enabled predicate
4fca6cb44e block/dirty-bitmap: remove set/reset assertions against enabled bit
7d032c41b8 block/dirty-bitmaps: rename frozen predicate helper
9d17322444 block/dirty-bitmap: add recording and busy properties

=== OUTPUT BEGIN ===
1/10 Checking commit 9d1732244435 (block/dirty-bitmap: add recording and busy properties)
2/10 Checking commit 7d032c41b8ab (block/dirty-bitmaps: rename frozen predicate helper)
WARNING: line over 80 characters
#85: FILE: block/dirty-bitmap.c:248:
+        error_setg(errp, "Cannot create a successor for a bitmap that is in-use "

total: 0 errors, 1 warnings, 122 lines checked

Patch 2/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/10 Checking commit 4fca6cb44e9b (block/dirty-bitmap: remove set/reset assertions against enabled bit)
4/10 Checking commit 2ee1f03baedb (block/dirty-bitmap: change semantics of enabled predicate)
5/10 Checking commit d932d4e9c476 (nbd: change error checking order for bitmaps)
6/10 Checking commit 3d0665404c60 (block/dirty-bitmap: explicitly lock bitmaps with successors)
7/10 Checking commit a29ece1728f0 (block/dirty-bitmaps: unify qmp_locked and user_locked calls)
ERROR: open brace '{' following function declarations go on the next line
#37: FILE: block/dirty-bitmap.c:190:
+bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {

total: 1 errors, 0 warnings, 263 lines checked

Patch 7/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

8/10 Checking commit 30e042736827 (block/dirty-bitmaps: move comment block)
9/10 Checking commit 7c4f722b8967 (blockdev: remove unused paio parameter documentation)
10/10 Checking commit 5615f815663d (iotests: add busy/recording bit test to 124)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190223000614.13894-1-jsnow@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com