bitmaps: remove x- prefix from QMP api

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 <[email protected]>

---
  blockdev.c | 14 +++++++-------
  1 file changed, 7 insertions(+), 7 deletions(-)

Quoted 7 lines · 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.

Quoted 7 lines

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 <[email protected]>

---
  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 <[email protected]>

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 <[email protected]>

---
  blockdev.c | 14 +++++++-------
  1 file changed, 7 insertions(+), 7 deletions(-)

blockdev.c +7 -7

index 81f95d920b..1ba706df8b 100644
@@ -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);
          }

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 <[email protected]>

---
  blockdev.c           | 64 +++++++++++++++++++++++++++++---------------
  qapi/block-core.json | 22 +++++++--------
  2 files changed, 53 insertions(+), 33 deletions(-)

+++ b/qapi/block-core.json

qapi/block-core.json +0 -1

@@ -1818,14 +1818,14 @@
  #
  # @node: name of device/node which the bitmap is tracking
  #
-# @dst_name: name of the destination dirty bitmap

Quoted 19 lines · 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 <[email protected]>

---
  blockdev.c           | 64 +++++++++++++++++++++++++++++--------------- qapi/block-core.json | 22 +++++++--------

  2 files changed, 53 insertions(+), 33 deletions(-)

+++ b/qapi/block-core.json

qapi/block-core.json +0 -1

@@ -1818,14 +1818,14 @@
   #
   # @node: name of device/node which the bitmap is tracking
   #
-# @dst_name: name of the destination dirty bitmap

qapi/block-core.json line 1828

Quoted 10 lines

qapi/block-core.json +3 -2

+# @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.

qapi/block-core.json line 1944

Quoted 13 lines

qapi/block-core.json +0 -3

     ##
   # @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

qapi/block-core.json +2 -1

-# 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 <[email protected]>

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 <[email protected]>

---

+++ b/tests/qemu-iotests/223

tests/qemu-iotests/223 +0 -0

@@ -112,9 +112,9 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"qmp_capabilities"}' "return"
  _send_qemu_cmd $QEMU_HANDLE '{"execute":"blockdev-add",

Quoted 16 lines · 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 <[email protected]>

---

+++ b/tests/qemu-iotests/223

(quoted diff) +0 -0

@@ -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"

(quoted diff) +1 -1

-_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"

Quoted 5 lines

(quoted diff) +1 -1

-_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 <[email protected]>

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(-)

Patchew URL: https://patchew.org/QEMU/[email protected]/

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: [email protected]

=== 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/[email protected]/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to [email protected]