[Qemu-devel] [PATCH 0/3] bitmaps: remove x- prefix from QMP api

[Qemu-devel] [PATCH 0/3] bitmaps: remove x- prefix from QMP api

[John Snow]

Touch up a few last things and remove the x- prefix.

Test on the way, thoughts?

John Snow (3):
  blockdev: abort transactions in reverse order
  blockdev: n-ary bitmap merge
  block: remove 'x' prefix from experimental bitmap APIs

 blockdev.c             | 96 +++++++++++++++++++++++++-----------------
 qapi/block-core.json   | 56 ++++++++++++------------
 qapi/transaction.json  | 12 +++---
 tests/qemu-iotests/223 |  4 +-
 4 files changed, 94 insertions(+), 74 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH 1/3] blockdev: abort transactions in reverse order

[John Snow]

Presently, we abort transactions in the same order they were processed in.
Bitmap commands, though, attempt to restore backup data structures on abort.
To that end, though, they need to be aborted in reverse chronological order.

Replace the QSIMPLEQ data structure with a QTAILQ one, so we can iterate
in reverse for the abort phase of the transaction.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 81f95d920b..1ba706df8b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1341,7 +1341,7 @@ struct BlkActionState {
     const BlkActionOps *ops;
     JobTxn *block_job_txn;
     TransactionProperties *txn_props;
-    QSIMPLEQ_ENTRY(BlkActionState) entry;
+    QTAILQ_ENTRY(BlkActionState) entry;
 };
 
 /* internal snapshot private data */
@@ -2269,8 +2269,8 @@ void qmp_transaction(TransactionActionList *dev_list,
     BlkActionState *state, *next;
     Error *local_err = NULL;
 
-    QSIMPLEQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states;
-    QSIMPLEQ_INIT(&snap_bdrv_states);
+    QTAILQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states;
+    QTAILQ_INIT(&snap_bdrv_states);
 
     /* Does this transaction get canceled as a group on failure?
      * If not, we don't really need to make a JobTxn.
@@ -2301,7 +2301,7 @@ void qmp_transaction(TransactionActionList *dev_list,
         state->action = dev_info;
         state->block_job_txn = block_job_txn;
         state->txn_props = props;
-        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
+        QTAILQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
 
         state->ops->prepare(state, &local_err);
         if (local_err) {
@@ -2310,7 +2310,7 @@ void qmp_transaction(TransactionActionList *dev_list,
         }
     }
 
-    QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
+    QTAILQ_FOREACH(state, &snap_bdrv_states, entry) {
         if (state->ops->commit) {
             state->ops->commit(state);
         }
@@ -2321,13 +2321,13 @@ void qmp_transaction(TransactionActionList *dev_list,
 
 delete_and_fail:
     /* failure, and it is all-or-none; roll back all operations */
-    QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
+    QTAILQ_FOREACH_REVERSE(state, &snap_bdrv_states, snap_bdrv_states, entry) {
         if (state->ops->abort) {
             state->ops->abort(state);
         }
     }
 exit:
-    QSIMPLEQ_FOREACH_SAFE(state, &snap_bdrv_states, entry, next) {
+    QTAILQ_FOREACH_SAFE(state, &snap_bdrv_states, entry, next) {
         if (state->ops->clean) {
             state->ops->clean(state);
         }
-- 
2.17.2

[Qemu-devel] [PATCH 2/3] blockdev: n-ary bitmap merge

[John Snow]

Especially outside of transactions, it is helpful to provide
all-or-nothing semantics for bitmap merges. This facilitates
the coalescing of multiple bitmaps into a single target for
the "checkpoint" interpretation when assembling bitmaps that
represent arbitrary points in time from component bitmaps.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c           | 64 +++++++++++++++++++++++++++++---------------
 qapi/block-core.json | 22 +++++++--------
 2 files changed, 53 insertions(+), 33 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 1ba706df8b..0f740fd964 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2122,33 +2122,26 @@ static void block_dirty_bitmap_disable_abort(BlkActionState *common)
     }
 }
 
+void do_block_dirty_bitmap_merge(const char *node, const char *target,
+                                 strList *bitmaps, HBitmap **backup,
+                                 Error **errp);
+
 static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
                                              Error **errp)
 {
     BlockDirtyBitmapMerge *action;
     BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
                                              common, common);
-    BdrvDirtyBitmap *merge_source;
 
     if (action_check_completion_mode(common, errp) < 0) {
         return;
     }
 
     action = common->action->u.x_block_dirty_bitmap_merge.data;
-    state->bitmap = block_dirty_bitmap_lookup(action->node,
-                                              action->dst_name,
-                                              &state->bs,
-                                              errp);
-    if (!state->bitmap) {
-        return;
-    }
 
-    merge_source = bdrv_find_dirty_bitmap(state->bs, action->src_name);
-    if (!merge_source) {
-        return;
-    }
-
-    bdrv_merge_dirty_bitmap(state->bitmap, merge_source, &state->backup, errp);
+    do_block_dirty_bitmap_merge(action->node, action->target,
+                                action->bitmaps, &state->backup,
+                                errp);
 }
 
 static void abort_prepare(BlkActionState *common, Error **errp)
@@ -2980,24 +2973,51 @@ void qmp_x_block_dirty_bitmap_disable(const char *node, const char *name,
     bdrv_disable_dirty_bitmap(bitmap);
 }
 
-void qmp_x_block_dirty_bitmap_merge(const char *node, const char *dst_name,
-                                    const char *src_name, Error **errp)
+void do_block_dirty_bitmap_merge(const char *node, const char *target,
+                                 strList *bitmaps, HBitmap **backup,
+                                 Error **errp)
 {
     BlockDriverState *bs;
-    BdrvDirtyBitmap *dst, *src;
+    BdrvDirtyBitmap *dst, *src, *anon;
+    strList *lst;
+    Error *local_err = NULL;
 
-    dst = block_dirty_bitmap_lookup(node, dst_name, &bs, errp);
+    dst = block_dirty_bitmap_lookup(node, target, &bs, errp);
     if (!dst) {
         return;
     }
 
-    src = bdrv_find_dirty_bitmap(bs, src_name);
-    if (!src) {
-        error_setg(errp, "Dirty bitmap '%s' not found", src_name);
+    anon = bdrv_create_dirty_bitmap(bs, bdrv_dirty_bitmap_granularity(dst),
+                                    NULL, errp);
+    if (!anon) {
         return;
     }
 
-    bdrv_merge_dirty_bitmap(dst, src, NULL, errp);
+    for (lst = bitmaps; lst; lst = lst->next) {
+        src = bdrv_find_dirty_bitmap(bs, lst->value);
+        if (!src) {
+            error_setg(errp, "Dirty bitmap '%s' not found", lst->value);
+            goto out;
+        }
+
+        bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            goto out;
+        }
+    }
+
+    /* Merge into dst; dst is unchanged on failure. */
+    bdrv_merge_dirty_bitmap(dst, anon, backup, errp);
+
+ out:
+    bdrv_release_dirty_bitmap(bs, anon);
+}
+
+void qmp_x_block_dirty_bitmap_merge(const char *node, const char *target,
+                                    strList *bitmaps, Error **errp)
+{
+    do_block_dirty_bitmap_merge(node, target, bitmaps, NULL, errp);
 }
 
 BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d4fe710836..320d74ef34 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1818,14 +1818,14 @@
 #
 # @node: name of device/node which the bitmap is tracking
 #
-# @dst_name: name of the destination dirty bitmap
+# @target: name of the destination dirty bitmap
 #
-# @src_name: name of the source dirty bitmap
+# @bitmaps: name(s) of the source dirty bitmap(s)
 #
 # Since: 3.0
 ##
 { 'struct': 'BlockDirtyBitmapMerge',
-  'data': { 'node': 'str', 'dst_name': 'str', 'src_name': 'str' } }
+  'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } }
 
 ##
 # @block-dirty-bitmap-add:
@@ -1940,23 +1940,23 @@
 ##
 # @x-block-dirty-bitmap-merge:
 #
-# FIXME: Rename @src_name and @dst_name to src-name and dst-name.
-#
-# Merge @src_name dirty bitmap to @dst_name dirty bitmap. @src_name dirty
-# bitmap is unchanged. On error, @dst_name is unchanged.
+# Merge dirty bitmaps listed in @bitmaps to the @target dirty bitmap.
+# The @bitmaps dirty bitmaps are unchanged.
+# On error, @target is unchanged.
 #
 # Returns: nothing on success
 #          If @node is not a valid block device, DeviceNotFound
-#          If @dst_name or @src_name is not found, GenericError
-#          If bitmaps has different sizes or granularities, GenericError
+#          If any bitmap in @bitmaps or @target is not found, GenericError
+#          If any of the bitmaps have different sizes or granularities,
+#              GenericError
 #
 # Since: 3.0
 #
 # Example:
 #
 # -> { "execute": "x-block-dirty-bitmap-merge",
-#      "arguments": { "node": "drive0", "dst_name": "bitmap0",
-#                     "src_name": "bitmap1" } }
+#      "arguments": { "node": "drive0", "target": "bitmap0",
+#                     "bitmaps": ["bitmap1"] } }
 # <- { "return": {} }
 #
 ##
-- 
2.17.2

[Qemu-devel] [PATCH 3/3] block: remove 'x' prefix from experimental bitmap APIs

[John Snow]

The 'x' prefix was added because we were uncertain of the direction we'd
take for the libvirt API. With the general approach solidified, I feel
comfortable committing to this API for 4.0.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c             | 22 +++++++++++-----------
 qapi/block-core.json   | 34 +++++++++++++++++-----------------
 qapi/transaction.json  | 12 ++++++------
 tests/qemu-iotests/223 |  4 ++--
 4 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 0f740fd964..da87aae5cf 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1966,7 +1966,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState *common,
                                action->has_granularity, action->granularity,
                                action->has_persistent, action->persistent,
                                action->has_autoload, action->autoload,
-                               action->has_x_disabled, action->x_disabled,
+                               action->has_disabled, action->disabled,
                                &local_err);
 
     if (!local_err) {
@@ -2051,7 +2051,7 @@ static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
         return;
     }
 
-    action = common->action->u.x_block_dirty_bitmap_enable.data;
+    action = common->action->u.block_dirty_bitmap_enable.data;
     state->bitmap = block_dirty_bitmap_lookup(action->node,
                                               action->name,
                                               NULL,
@@ -2092,7 +2092,7 @@ static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
         return;
     }
 
-    action = common->action->u.x_block_dirty_bitmap_disable.data;
+    action = common->action->u.block_dirty_bitmap_disable.data;
     state->bitmap = block_dirty_bitmap_lookup(action->node,
                                               action->name,
                                               NULL,
@@ -2137,7 +2137,7 @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
         return;
     }
 
-    action = common->action->u.x_block_dirty_bitmap_merge.data;
+    action = common->action->u.block_dirty_bitmap_merge.data;
 
     do_block_dirty_bitmap_merge(action->node, action->target,
                                 action->bitmaps, &state->backup,
@@ -2205,17 +2205,17 @@ static const BlkActionOps actions[] = {
         .commit = block_dirty_bitmap_free_backup,
         .abort = block_dirty_bitmap_restore,
     },
-    [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_ENABLE] = {
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
         .instance_size = sizeof(BlockDirtyBitmapState),
         .prepare = block_dirty_bitmap_enable_prepare,
         .abort = block_dirty_bitmap_enable_abort,
     },
-    [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_DISABLE] = {
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
         .instance_size = sizeof(BlockDirtyBitmapState),
         .prepare = block_dirty_bitmap_disable_prepare,
         .abort = block_dirty_bitmap_disable_abort,
     },
-    [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_MERGE] = {
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_MERGE] = {
         .instance_size = sizeof(BlockDirtyBitmapState),
         .prepare = block_dirty_bitmap_merge_prepare,
         .commit = block_dirty_bitmap_free_backup,
@@ -2931,7 +2931,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
     bdrv_clear_dirty_bitmap(bitmap, NULL);
 }
 
-void qmp_x_block_dirty_bitmap_enable(const char *node, const char *name,
+void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
                                    Error **errp)
 {
     BlockDriverState *bs;
@@ -2952,7 +2952,7 @@ void qmp_x_block_dirty_bitmap_enable(const char *node, const char *name,
     bdrv_enable_dirty_bitmap(bitmap);
 }
 
-void qmp_x_block_dirty_bitmap_disable(const char *node, const char *name,
+void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
                                     Error **errp)
 {
     BlockDriverState *bs;
@@ -3014,8 +3014,8 @@ void do_block_dirty_bitmap_merge(const char *node, const char *target,
     bdrv_release_dirty_bitmap(bs, anon);
 }
 
-void qmp_x_block_dirty_bitmap_merge(const char *node, const char *target,
-                                    strList *bitmaps, Error **errp)
+void qmp_block_dirty_bitmap_merge(const char *node, const char *target,
+                                  strList *bitmaps, Error **errp)
 {
     do_block_dirty_bitmap_merge(node, target, bitmaps, NULL, errp);
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 320d74ef34..fde96fdb50 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1803,15 +1803,15 @@
 #            Currently, all dirty tracking bitmaps are loaded from Qcow2 on
 #            open.
 #
-# @x-disabled: the bitmap is created in the disabled state, which means that
-#              it will not track drive changes. The bitmap may be enabled with
-#              x-block-dirty-bitmap-enable. Default is false. (Since: 3.0)
+# @disabled: the bitmap is created in the disabled state, which means that
+#            it will not track drive changes. The bitmap may be enabled with
+#            block-dirty-bitmap-enable. Default is false. (Since: 4.0)
 #
 # Since: 2.4
 ##
 { 'struct': 'BlockDirtyBitmapAdd',
   'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
-            '*persistent': 'bool', '*autoload': 'bool', '*x-disabled': 'bool' } }
+            '*persistent': 'bool', '*autoload': 'bool', '*disabled': 'bool' } }
 
 ##
 # @BlockDirtyBitmapMerge:
@@ -1822,7 +1822,7 @@
 #
 # @bitmaps: name(s) of the source dirty bitmap(s)
 #
-# Since: 3.0
+# Since: 4.0
 ##
 { 'struct': 'BlockDirtyBitmapMerge',
   'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } }
@@ -1896,7 +1896,7 @@
   'data': 'BlockDirtyBitmap' }
 
 ##
-# @x-block-dirty-bitmap-enable:
+# @block-dirty-bitmap-enable:
 #
 # Enables a dirty bitmap so that it will begin tracking disk changes.
 #
@@ -1904,20 +1904,20 @@
 #          If @node is not a valid block device, DeviceNotFound
 #          If @name is not found, GenericError with an explanation
 #
-# Since: 3.0
+# Since: 4.0
 #
 # Example:
 #
-# -> { "execute": "x-block-dirty-bitmap-enable",
+# -> { "execute": "block-dirty-bitmap-enable",
 #      "arguments": { "node": "drive0", "name": "bitmap0" } }
 # <- { "return": {} }
 #
 ##
-  { 'command': 'x-block-dirty-bitmap-enable',
+  { 'command': 'block-dirty-bitmap-enable',
     'data': 'BlockDirtyBitmap' }
 
 ##
-# @x-block-dirty-bitmap-disable:
+# @block-dirty-bitmap-disable:
 #
 # Disables a dirty bitmap so that it will stop tracking disk changes.
 #
@@ -1925,20 +1925,20 @@
 #          If @node is not a valid block device, DeviceNotFound
 #          If @name is not found, GenericError with an explanation
 #
-# Since: 3.0
+# Since: 4.0
 #
 # Example:
 #
-# -> { "execute": "x-block-dirty-bitmap-disable",
+# -> { "execute": "block-dirty-bitmap-disable",
 #      "arguments": { "node": "drive0", "name": "bitmap0" } }
 # <- { "return": {} }
 #
 ##
-    { 'command': 'x-block-dirty-bitmap-disable',
+    { 'command': 'block-dirty-bitmap-disable',
       'data': 'BlockDirtyBitmap' }
 
 ##
-# @x-block-dirty-bitmap-merge:
+# @block-dirty-bitmap-merge:
 #
 # Merge dirty bitmaps listed in @bitmaps to the @target dirty bitmap.
 # The @bitmaps dirty bitmaps are unchanged.
@@ -1950,17 +1950,17 @@
 #          If any of the bitmaps have different sizes or granularities,
 #              GenericError
 #
-# Since: 3.0
+# Since: 4.0
 #
 # Example:
 #
-# -> { "execute": "x-block-dirty-bitmap-merge",
+# -> { "execute": "block-dirty-bitmap-merge",
 #      "arguments": { "node": "drive0", "target": "bitmap0",
 #                     "bitmaps": ["bitmap1"] } }
 # <- { "return": {} }
 #
 ##
-      { 'command': 'x-block-dirty-bitmap-merge',
+      { 'command': 'block-dirty-bitmap-merge',
         'data': 'BlockDirtyBitmapMerge' }
 
 ##
diff --git a/qapi/transaction.json b/qapi/transaction.json
index 5875cdb16c..95edb78227 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -46,9 +46,9 @@
 # - @abort: since 1.6
 # - @block-dirty-bitmap-add: since 2.5
 # - @block-dirty-bitmap-clear: since 2.5
-# - @x-block-dirty-bitmap-enable: since 3.0
-# - @x-block-dirty-bitmap-disable: since 3.0
-# - @x-block-dirty-bitmap-merge: since 3.1
+# - @block-dirty-bitmap-enable: since 4.0
+# - @block-dirty-bitmap-disable: since 4.0
+# - @block-dirty-bitmap-merge: since 4.0
 # - @blockdev-backup: since 2.3
 # - @blockdev-snapshot: since 2.5
 # - @blockdev-snapshot-internal-sync: since 1.7
@@ -62,9 +62,9 @@
        'abort': 'Abort',
        'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
        'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
-       'x-block-dirty-bitmap-enable': 'BlockDirtyBitmap',
-       'x-block-dirty-bitmap-disable': 'BlockDirtyBitmap',
-       'x-block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge',
+       'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
+       'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
+       'block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge',
        'blockdev-backup': 'BlockdevBackup',
        'blockdev-snapshot': 'BlockdevSnapshot',
        'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index 397b865d34..5513dc6215 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -112,9 +112,9 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"qmp_capabilities"}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"blockdev-add",
   "arguments":{"driver":"qcow2", "node-name":"n",
     "file":{"driver":"file", "filename":"'"$TEST_IMG"'"}}}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-block-dirty-bitmap-disable",
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
   "arguments":{"node":"n", "name":"b"}}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-block-dirty-bitmap-disable",
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
   "arguments":{"node":"n", "name":"b2"}}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
   "arguments":{"addr":{"type":"unix",
-- 
2.17.2

Re: [Qemu-devel] [PATCH 1/3] blockdev: abort transactions in reverse order

[Eric Blake]

On 12/6/18 1:25 PM, John Snow wrote:
> Presently, we abort transactions in the same order they were processed in.
> Bitmap commands, though, attempt to restore backup data structures on abort.
> To that end, though, they need to be aborted in reverse chronological order.
> 
> Replace the QSIMPLEQ data structure with a QTAILQ one, so we can iterate
> in reverse for the abort phase of the transaction.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   blockdev.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)

Does this need to cc qemu-stable? I'm trying to figure out if it affects 
any of the transactions issued by my libvirt code demo'd at KVM Forum.

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

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

Re: [Qemu-devel] [PATCH 0/3] bitmaps: remove x- prefix from QMP api

[John Snow]

On 12/6/18 2:25 PM, John Snow wrote:
> Touch up a few last things and remove the x- prefix.
> 
> Test on the way, thoughts?
> 
> John Snow (3):
>   blockdev: abort transactions in reverse order
>   blockdev: n-ary bitmap merge
>   block: remove 'x' prefix from experimental bitmap APIs
> 
>  blockdev.c             | 96 +++++++++++++++++++++++++-----------------
>  qapi/block-core.json   | 56 ++++++++++++------------
>  qapi/transaction.json  | 12 +++---
>  tests/qemu-iotests/223 |  4 +-
>  4 files changed, 94 insertions(+), 74 deletions(-)
> 

Sorry, at some point I bookmarked a typo for Vladimir's email :(

Correcting his address here in the cover letter.

Re: [Qemu-devel] [PATCH 1/3] blockdev: abort transactions in reverse order

[John Snow]

On 12/6/18 3:37 PM, Eric Blake wrote:
> On 12/6/18 1:25 PM, John Snow wrote:
>> Presently, we abort transactions in the same order they were processed
>> in.
>> Bitmap commands, though, attempt to restore backup data structures on
>> abort.
>> To that end, though, they need to be aborted in reverse chronological
>> order.
>>
>> Replace the QSIMPLEQ data structure with a QTAILQ one, so we can iterate
>> in reverse for the abort phase of the transaction.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   blockdev.c | 14 +++++++-------
>>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> Does this need to cc qemu-stable? I'm trying to figure out if it affects
> any of the transactions issued by my libvirt code demo'd at KVM Forum.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Hmm, possibly. merge is of course an x-prefix, but "clear" uses the same
restoration technique. I suppose if you cleared the same bitmap multiple
times and tried to roll it back you could see trouble where we restore
an already cleared bitmap.

I'll say that patch 01/03 here should be considered a bugfix, yes, but I
wanted to see if anyone thought this had consequences I hadn't
considered yet.

--js

Re: [Qemu-devel] [PATCH 0/3] bitmaps: remove x- prefix from QMP api

[no-reply]

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



Hi,

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

Type: series
Subject: [Qemu-devel] [PATCH 0/3] bitmaps: remove x- prefix from QMP api
Message-id: 20181206192544.3987-1-jsnow@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
c972673 block: remove 'x' prefix from experimental bitmap APIs
ebd1e6c blockdev: n-ary bitmap merge
68d3c0c blockdev: abort transactions in reverse order

=== OUTPUT BEGIN ===
Checking PATCH 1/3: blockdev: abort transactions in reverse order...
Checking PATCH 2/3: blockdev: n-ary bitmap merge...
ERROR: externs should be avoided in .c files
#23: FILE: blockdev.c:2125:
+void do_block_dirty_bitmap_merge(const char *node, const char *target,

total: 1 errors, 0 warnings, 147 lines checked

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

Checking PATCH 3/3: block: remove 'x' prefix from experimental bitmap APIs...
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20181206192544.3987-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

Re: [Qemu-devel] [PATCH 1/3] blockdev: abort transactions in reverse order

[Vladimir Sementsov-Ogievskiy]

06.12.2018 22:25, John Snow wrote:
> Presently, we abort transactions in the same order they were processed in.
> Bitmap commands, though, attempt to restore backup data structures on abort.
> To that end, though, they need to be aborted in reverse chronological order.
> 
> Replace the QSIMPLEQ data structure with a QTAILQ one, so we can iterate
> in reverse for the abort phase of the transaction.

aha, so, abort for
   add
   disable
should be
   enable
   del
and not visa-versa

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

> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   blockdev.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 81f95d920b..1ba706df8b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1341,7 +1341,7 @@ struct BlkActionState {
>       const BlkActionOps *ops;
>       JobTxn *block_job_txn;
>       TransactionProperties *txn_props;
> -    QSIMPLEQ_ENTRY(BlkActionState) entry;
> +    QTAILQ_ENTRY(BlkActionState) entry;
>   };
>   
>   /* internal snapshot private data */
> @@ -2269,8 +2269,8 @@ void qmp_transaction(TransactionActionList *dev_list,
>       BlkActionState *state, *next;
>       Error *local_err = NULL;
>   
> -    QSIMPLEQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states;
> -    QSIMPLEQ_INIT(&snap_bdrv_states);
> +    QTAILQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states;
> +    QTAILQ_INIT(&snap_bdrv_states);
>   
>       /* Does this transaction get canceled as a group on failure?
>        * If not, we don't really need to make a JobTxn.
> @@ -2301,7 +2301,7 @@ void qmp_transaction(TransactionActionList *dev_list,
>           state->action = dev_info;
>           state->block_job_txn = block_job_txn;
>           state->txn_props = props;
> -        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
> +        QTAILQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
>   
>           state->ops->prepare(state, &local_err);
>           if (local_err) {
> @@ -2310,7 +2310,7 @@ void qmp_transaction(TransactionActionList *dev_list,
>           }
>       }
>   
> -    QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
> +    QTAILQ_FOREACH(state, &snap_bdrv_states, entry) {
>           if (state->ops->commit) {
>               state->ops->commit(state);
>           }
> @@ -2321,13 +2321,13 @@ void qmp_transaction(TransactionActionList *dev_list,
>   
>   delete_and_fail:
>       /* failure, and it is all-or-none; roll back all operations */
> -    QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
> +    QTAILQ_FOREACH_REVERSE(state, &snap_bdrv_states, snap_bdrv_states, entry) {
>           if (state->ops->abort) {
>               state->ops->abort(state);
>           }
>       }
>   exit:
> -    QSIMPLEQ_FOREACH_SAFE(state, &snap_bdrv_states, entry, next) {
> +    QTAILQ_FOREACH_SAFE(state, &snap_bdrv_states, entry, next) {
>           if (state->ops->clean) {
>               state->ops->clean(state);
>           }
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/3] blockdev: n-ary bitmap merge

[Eric Blake]

On 12/6/18 1:25 PM, John Snow wrote:
> Especially outside of transactions, it is helpful to provide
> all-or-nothing semantics for bitmap merges. This facilitates
> the coalescing of multiple bitmaps into a single target for
> the "checkpoint" interpretation when assembling bitmaps that
> represent arbitrary points in time from component bitmaps.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   blockdev.c           | 64 +++++++++++++++++++++++++++++---------------
>   qapi/block-core.json | 22 +++++++--------
>   2 files changed, 53 insertions(+), 33 deletions(-)
> 

> +++ b/qapi/block-core.json
> @@ -1818,14 +1818,14 @@
>   #
>   # @node: name of device/node which the bitmap is tracking
>   #
> -# @dst_name: name of the destination dirty bitmap
> +# @target: name of the destination dirty bitmap
>   #
> -# @src_name: name of the source dirty bitmap
> +# @bitmaps: name(s) of the source dirty bitmap(s)
>   #
>   # Since: 3.0
>   ##
>   { 'struct': 'BlockDirtyBitmapMerge',
> -  'data': { 'node': 'str', 'dst_name': 'str', 'src_name': 'str' } }
> +  'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } }

Definitely worthwhile!

I'll update my pending libvirt patches to use this.

>   
>   ##
>   # @block-dirty-bitmap-add:
> @@ -1940,23 +1940,23 @@
>   ##
>   # @x-block-dirty-bitmap-merge:
>   #
> -# FIXME: Rename @src_name and @dst_name to src-name and dst-name.
> -#
> -# Merge @src_name dirty bitmap to @dst_name dirty bitmap. @src_name dirty
> -# bitmap is unchanged. On error, @dst_name is unchanged.
> +# Merge dirty bitmaps listed in @bitmaps to the @target dirty bitmap.
> +# The @bitmaps dirty bitmaps are unchanged.

Well, except in the corner case of when @bitmaps also lists the 
destination (I presume that merging a bitmap into itself silently 
succeeds with no further changes, but therefore the inclusion of the 
destination in the list of sources means that that particular source is 
changing due to merging in the other sources).  Not worth rewording this 
  sentence, but does make you want to consider ensuring that the 
testsuite covers merging a bitmap into itself.

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

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

Re: [Qemu-devel] [PATCH 3/3] block: remove 'x' prefix from experimental bitmap APIs

[Eric Blake]

On 12/6/18 1:25 PM, John Snow wrote:
> The 'x' prefix was added because we were uncertain of the direction we'd
> take for the libvirt API. With the general approach solidified, I feel
> comfortable committing to this API for 4.0.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---

> +++ b/tests/qemu-iotests/223
> @@ -112,9 +112,9 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"qmp_capabilities"}' "return"
>   _send_qemu_cmd $QEMU_HANDLE '{"execute":"blockdev-add",
>     "arguments":{"driver":"qcow2", "node-name":"n",
>       "file":{"driver":"file", "filename":"'"$TEST_IMG"'"}}}' "return"
> -_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-block-dirty-bitmap-disable",
> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
>     "arguments":{"node":"n", "name":"b"}}' "return"
> -_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-block-dirty-bitmap-disable",
> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
>     "arguments":{"node":"n", "name":"b2"}}' "return"
>   _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
>     "arguments":{"addr":{"type":"unix",

No iotests coverage of block-dirty-bitmap-merge.  We should fix that as 
part of this series; separate patch is fine.

I'm glad you remembered to renumber all the 'since' tags to 4.0 (as the 
new spelling is indeed new to 4.0, not when we introduced the older x- 
variant).

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

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

Re: [Qemu-devel] [PATCH 3/3] block: remove 'x' prefix from experimental bitmap APIs

[John Snow]

On 12/7/18 11:28 AM, Eric Blake wrote:
> On 12/6/18 1:25 PM, John Snow wrote:
>> The 'x' prefix was added because we were uncertain of the direction we'd
>> take for the libvirt API. With the general approach solidified, I feel
>> comfortable committing to this API for 4.0.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
> 
>> +++ b/tests/qemu-iotests/223
>> @@ -112,9 +112,9 @@ _send_qemu_cmd $QEMU_HANDLE
>> '{"execute":"qmp_capabilities"}' "return"
>>   _send_qemu_cmd $QEMU_HANDLE '{"execute":"blockdev-add",
>>     "arguments":{"driver":"qcow2", "node-name":"n",
>>       "file":{"driver":"file", "filename":"'"$TEST_IMG"'"}}}' "return"
>> -_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-block-dirty-bitmap-disable",
>> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
>>     "arguments":{"node":"n", "name":"b"}}' "return"
>> -_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-block-dirty-bitmap-disable",
>> +_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
>>     "arguments":{"node":"n", "name":"b2"}}' "return"
>>   _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
>>     "arguments":{"addr":{"type":"unix",
> 
> No iotests coverage of block-dirty-bitmap-merge.  We should fix that as
> part of this series; separate patch is fine.
> 

you're not wrong :(

> I'm glad you remembered to renumber all the 'since' tags to 4.0 (as the
> new spelling is indeed new to 4.0, not when we introduced the older x-
> variant).
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks!

Re: [Qemu-devel] [PATCH 2/3] blockdev: n-ary bitmap merge

[John Snow]

On 12/7/18 11:25 AM, Eric Blake wrote:
> On 12/6/18 1:25 PM, John Snow wrote:
>> Especially outside of transactions, it is helpful to provide
>> all-or-nothing semantics for bitmap merges. This facilitates
>> the coalescing of multiple bitmaps into a single target for
>> the "checkpoint" interpretation when assembling bitmaps that
>> represent arbitrary points in time from component bitmaps.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   blockdev.c           | 64 +++++++++++++++++++++++++++++---------------
>>   qapi/block-core.json | 22 +++++++--------
>>   2 files changed, 53 insertions(+), 33 deletions(-)
>>
> 
>> +++ b/qapi/block-core.json
>> @@ -1818,14 +1818,14 @@
>>   #
>>   # @node: name of device/node which the bitmap is tracking
>>   #
>> -# @dst_name: name of the destination dirty bitmap
>> +# @target: name of the destination dirty bitmap
>>   #
>> -# @src_name: name of the source dirty bitmap
>> +# @bitmaps: name(s) of the source dirty bitmap(s)
>>   #
>>   # Since: 3.0
>>   ##
>>   { 'struct': 'BlockDirtyBitmapMerge',
>> -  'data': { 'node': 'str', 'dst_name': 'str', 'src_name': 'str' } }
>> +  'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } }
> 
> Definitely worthwhile!
> 
> I'll update my pending libvirt patches to use this.
> 
>>     ##
>>   # @block-dirty-bitmap-add:
>> @@ -1940,23 +1940,23 @@
>>   ##
>>   # @x-block-dirty-bitmap-merge:
>>   #
>> -# FIXME: Rename @src_name and @dst_name to src-name and dst-name.
>> -#
>> -# Merge @src_name dirty bitmap to @dst_name dirty bitmap. @src_name
>> dirty
>> -# bitmap is unchanged. On error, @dst_name is unchanged.
>> +# Merge dirty bitmaps listed in @bitmaps to the @target dirty bitmap.
>> +# The @bitmaps dirty bitmaps are unchanged.
> 
> Well, except in the corner case of when @bitmaps also lists the
> destination (I presume that merging a bitmap into itself silently
> succeeds with no further changes, but therefore the inclusion of the
> destination in the list of sources means that that particular source is
> changing due to merging in the other sources).  Not worth rewording this
>  sentence, but does make you want to consider ensuring that the
> testsuite covers merging a bitmap into itself.
> 

OK, noted.

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

Re: [Qemu-devel] [PATCH 0/3] bitmaps: remove x- prefix from QMP api

[John Snow]

On 12/6/18 7:08 PM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20181206192544.3987-1-jsnow@redhat.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Type: series
> Subject: [Qemu-devel] [PATCH 0/3] bitmaps: remove x- prefix from QMP api
> Message-id: 20181206192544.3987-1-jsnow@redhat.com
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> Switched to a new branch 'test'
> c972673 block: remove 'x' prefix from experimental bitmap APIs
> ebd1e6c blockdev: n-ary bitmap merge
> 68d3c0c blockdev: abort transactions in reverse order
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/3: blockdev: abort transactions in reverse order...
> Checking PATCH 2/3: blockdev: n-ary bitmap merge...
> ERROR: externs should be avoided in .c files
> #23: FILE: blockdev.c:2125:
> +void do_block_dirty_bitmap_merge(const char *node, const char *target,
> 

Oh, I guess I'm missing `static` here for this forward declaration.

Also, we need to edit patchew to not CC famz@redhat.com anymore :(

> total: 1 errors, 0 warnings, 147 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 3/3: block: remove 'x' prefix from experimental bitmap APIs...
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> The full log is available at
> http://patchew.org/logs/20181206192544.3987-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
>