[Qemu-devel] [PATCH 0/5] commit: Support multiple roots above top node

[Qemu-devel] [PATCH 0/5] commit: Support multiple roots above top node

[Kevin Wolf]

This is a step towards making the commit job flexible enough that it can
work with any kind of block graph. Currently, it requires that not only
the top and base node of the commit operation are specified, but also
the active layer of the backing file chain. Of course, the assumption
that a single active layer exists is invalid.

This series makes the commit job consider other roots as well so that
all parent nodes of the top node get their backing file updated and stay
valid after the commit job completes.

With this, we should have all of the prerequisites for a follow-up
series that adds a new and clean blockdev-commit QMP command which
doesn't require an option for the active layer and which accepts node
names instead of file names for base and top.

Kevin Wolf (5):
  block: Introduce BdrvChildRole.update_filename
  commit: Support multiple roots above top node
  qemu-iotests: Allow QMP pretty printing in common.qemu
  qemu-iotests: Test commit block job where top has two parents
  commit: Remove overlay_bs

 include/block/block.h          |   3 +-
 include/block/block_int.h      |   6 +
 block.c                        |  91 +++--
 block/commit.c                 |  64 +---
 tests/qemu-iotests/030         |   4 -
 tests/qemu-iotests/191         | 152 ++++++++
 tests/qemu-iotests/191.out     | 827 +++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/common.qemu |  14 +-
 tests/qemu-iotests/group       |   1 +
 9 files changed, 1078 insertions(+), 84 deletions(-)
 create mode 100755 tests/qemu-iotests/191
 create mode 100644 tests/qemu-iotests/191.out

-- 
2.13.5

[Qemu-devel] [PATCH 1/5] block: Introduce BdrvChildRole.update_filename

[Kevin Wolf]

There is no good reason for bdrv_drop_intermediate() to know the active
layer above the subchain it is operating on - even more so, because
the assumption that there is a single active layer above it is not
generally true.

In order to prepare removal of the active parameter, use a BdrvChildRole
callback to update the backing file string in the overlay image instead
of directly calling bdrv_change_backing_file().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block_int.h |  6 ++++++
 block.c                   | 33 ++++++++++++++++++++++++++++-----
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 99abe2ce74..e6d53cc15e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -544,6 +544,12 @@ struct BdrvChildRole {
 
     void (*attach)(BdrvChild *child);
     void (*detach)(BdrvChild *child);
+
+    /* Notifies the parent that the filename of its child has changed (e.g.
+     * because the direct child was removed from the backing chain), so that it
+     * can update its reference. */
+    int (*update_filename)(BdrvChild *child, BlockDriverState *new_base,
+                           const char *filename, Error **errp);
 };
 
 extern const BdrvChildRole child_file;
diff --git a/block.c b/block.c
index 5c65fac672..7ac8cd521b 100644
--- a/block.c
+++ b/block.c
@@ -981,6 +981,21 @@ static void bdrv_backing_options(int *child_flags, QDict *child_options,
     *child_flags = flags;
 }
 
+static int bdrv_backing_update_filename(BdrvChild *c, BlockDriverState *base,
+                                        const char *filename, Error **errp)
+{
+    BlockDriverState *parent = c->opaque;
+    int ret;
+
+    ret = bdrv_change_backing_file(parent, filename,
+                                   base->drv ? base->drv->format_name : "");
+    if (ret < 0) {
+        error_setg_errno(errp, ret, "Could not update backing file link");
+    }
+
+    return ret;
+}
+
 const BdrvChildRole child_backing = {
     .get_parent_desc = bdrv_child_get_parent_desc,
     .attach          = bdrv_backing_attach,
@@ -989,6 +1004,7 @@ const BdrvChildRole child_backing = {
     .drained_begin   = bdrv_child_cb_drained_begin,
     .drained_end     = bdrv_child_cb_drained_end,
     .inactivate      = bdrv_child_cb_inactivate,
+    .update_filename = bdrv_backing_update_filename,
 };
 
 static int bdrv_open_flags(BlockDriverState *bs, int flags)
@@ -3470,6 +3486,8 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
     Error *local_err = NULL;
     int ret = -EIO;
 
+    bdrv_ref(top);
+
     if (!top->drv || !base->drv) {
         goto exit;
     }
@@ -3494,11 +3512,15 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
     }
 
     /* success - we can delete the intermediate states, and link top->base */
-    backing_file_str = backing_file_str ? backing_file_str : base->filename;
-    ret = bdrv_change_backing_file(new_top_bs, backing_file_str,
-                                   base->drv ? base->drv->format_name : "");
-    if (ret) {
-        goto exit;
+    if (new_top_bs->backing->role->update_filename) {
+        backing_file_str = backing_file_str ? backing_file_str : base->filename;
+        ret = new_top_bs->backing->role->update_filename(new_top_bs->backing,
+                                                         base, backing_file_str,
+                                                         &local_err);
+        if (ret < 0) {
+            bdrv_set_backing_hd(new_top_bs, top, &error_abort);
+            goto exit;
+        }
     }
 
     bdrv_set_backing_hd(new_top_bs, base, &local_err);
@@ -3510,6 +3532,7 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
 
     ret = 0;
 exit:
+    bdrv_unref(top);
     return ret;
 }
 
-- 
2.13.5

[Qemu-devel] [PATCH 2/5] commit: Support multiple roots above top node

[Kevin Wolf]

This changes the commit block job to support operation in a graph where
there is more than a single active layer that references the top node.

This involves inserting the commit filter node not only on the path
between the given active node and the top node, but between the top node
and all of its parents.

On completion, bdrv_drop_intermediate() must consider all parents for
updating the backing file link. These parents may be backing files
themselves and such read-only; reopen them temporarily if necessary.
Previously this was achieved by the bdrv_reopen() calls in the commit
block job that made overlay_bs read-write for the whole duration of the
block job, even though write access is only needed on completion.

Now that we consider all parents, overlay_bs is meaningless. It is left
in place in this commit, but we'll remove it soon.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c        | 68 ++++++++++++++++++++++++++++++++++------------------------
 block/commit.c |  2 +-
 2 files changed, 41 insertions(+), 29 deletions(-)

diff --git a/block.c b/block.c
index 7ac8cd521b..e57fab501d 100644
--- a/block.c
+++ b/block.c
@@ -985,14 +985,26 @@ static int bdrv_backing_update_filename(BdrvChild *c, BlockDriverState *base,
                                         const char *filename, Error **errp)
 {
     BlockDriverState *parent = c->opaque;
+    int orig_flags = bdrv_get_flags(parent);
     int ret;
 
+    if (!(orig_flags & BDRV_O_RDWR)) {
+        ret = bdrv_reopen(parent, orig_flags | BDRV_O_RDWR, errp);
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
     ret = bdrv_change_backing_file(parent, filename,
                                    base->drv ? base->drv->format_name : "");
     if (ret < 0) {
         error_setg_errno(errp, ret, "Could not update backing file link");
     }
 
+    if (!(orig_flags & BDRV_O_RDWR)) {
+        bdrv_reopen(parent, orig_flags, NULL);
+    }
+
     return ret;
 }
 
@@ -3482,7 +3494,7 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs)
 int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
                            BlockDriverState *base, const char *backing_file_str)
 {
-    BlockDriverState *new_top_bs = NULL;
+    BdrvChild *c, *next;
     Error *local_err = NULL;
     int ret = -EIO;
 
@@ -3492,42 +3504,42 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
         goto exit;
     }
 
-    new_top_bs = bdrv_find_overlay(active, top);
-
-    if (new_top_bs == NULL) {
-        /* we could not find the image above 'top', this is an error */
-        goto exit;
-    }
-
-    /* special case of new_top_bs->backing->bs already pointing to base - nothing
-     * to do, no intermediate images */
-    if (backing_bs(new_top_bs) == base) {
-        ret = 0;
-        goto exit;
-    }
-
     /* Make sure that base is in the backing chain of top */
     if (!bdrv_chain_contains(top, base)) {
         goto exit;
     }
 
     /* success - we can delete the intermediate states, and link top->base */
-    if (new_top_bs->backing->role->update_filename) {
-        backing_file_str = backing_file_str ? backing_file_str : base->filename;
-        ret = new_top_bs->backing->role->update_filename(new_top_bs->backing,
-                                                         base, backing_file_str,
-                                                         &local_err);
-        if (ret < 0) {
-            bdrv_set_backing_hd(new_top_bs, top, &error_abort);
+    backing_file_str = backing_file_str ? backing_file_str : base->filename;
+
+    QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
+        /* Check whether we are allowed to switch c from top to base */
+        GSList *ignore_children = g_slist_prepend(NULL, c);
+        bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,
+                               ignore_children, &local_err);
+        if (local_err) {
+            ret = -EPERM;
+            error_report_err(local_err);
             goto exit;
         }
-    }
+        g_slist_free(ignore_children);
 
-    bdrv_set_backing_hd(new_top_bs, base, &local_err);
-    if (local_err) {
-        ret = -EPERM;
-        error_report_err(local_err);
-        goto exit;
+        /* If so, update the backing file path in the image file */
+        if (c->role->update_filename) {
+            ret = c->role->update_filename(c, base, backing_file_str,
+                                           &local_err);
+            if (ret < 0) {
+                bdrv_abort_perm_update(base);
+                error_report_err(local_err);
+                goto exit;
+            }
+        }
+
+        /* Do the actual switch in the in-memory graph.
+         * Completes bdrv_check_update_perm() transaction internally. */
+        bdrv_ref(base);
+        bdrv_replace_child(c, base);
+        bdrv_unref(top);
     }
 
     ret = 0;
diff --git a/block/commit.c b/block/commit.c
index 8f0e83578a..610e1cd8f5 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -350,7 +350,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
         error_propagate(errp, local_err);
         goto fail;
     }
-    bdrv_set_backing_hd(overlay_bs, commit_top_bs, &local_err);
+    bdrv_replace_node(top, commit_top_bs, &local_err);
     if (local_err) {
         bdrv_unref(commit_top_bs);
         commit_top_bs = NULL;
-- 
2.13.5

[Qemu-devel] [PATCH 3/5] qemu-iotests: Allow QMP pretty printing in common.qemu

[Kevin Wolf]

QMP responses to certain commands can become quite long, which doesn't
only make reading them hard, but also means that the maximum line length
in patch emails can be exceeded. Allow tests to switch to QMP pretty
printing, which results in more, but shorter lines.

We also need to make sure to keep indentation in the response for this
to work as expected.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/common.qemu | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index 7645f1dc72..92739a1eac 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -55,13 +55,13 @@ function _timed_wait_for()
     shift
 
     QEMU_STATUS[$h]=0
-    while read -t ${QEMU_COMM_TIMEOUT} resp <&${QEMU_OUT[$h]}
+    while IFS= read -t ${QEMU_COMM_TIMEOUT} resp <&${QEMU_OUT[$h]}
     do
         if [ -z "${silent}" ]; then
             echo "${resp}" | _filter_testdir | _filter_qemu \
                            | _filter_qemu_io | _filter_qmp | _filter_hmp
         fi
-        grep -q "${*}" < <(echo ${resp})
+        grep -q "${*}" < <(echo "${resp}")
         if [ $? -eq 0 ]; then
             return
         fi
@@ -129,6 +129,7 @@ function _send_qemu_cmd()
 # $qemu_comm_method: set this variable to 'monitor' (case insensitive)
 #                    to use the QEMU HMP monitor for communication.
 #                    Otherwise, the default of QMP is used.
+# $qmp_pretty: Set this variable to 'y' to enable QMP pretty printing.
 # $keep_stderr: Set this variable to 'y' to keep QEMU's stderr output on stderr.
 #               If this variable is empty, stderr will be redirected to stdout.
 # Returns:
@@ -145,7 +146,11 @@ function _launch_qemu()
         comm="-monitor stdio"
     else
         local qemu_comm_method="qmp"
-        comm="-monitor none -qmp stdio"
+        if [ "$qmp_pretty" = "y" ]; then
+            comm="-monitor none -qmp-pretty stdio"
+        else
+            comm="-monitor none -qmp stdio"
+        fi
     fi
 
     fifo_out=${QEMU_FIFO_OUT}_${_QEMU_HANDLE}
@@ -192,6 +197,9 @@ function _launch_qemu()
     then
         # Don't print response, since it has version information in it
         silent=yes _timed_wait_for ${_QEMU_HANDLE} "capabilities"
+        if [ "$qmp_pretty" = "y" ]; then
+            silent=yes _timed_wait_for ${_QEMU_HANDLE} "^}"
+        fi
     fi
     QEMU_HANDLE=${_QEMU_HANDLE}
     let _QEMU_HANDLE++
-- 
2.13.5

[Qemu-devel] [PATCH 4/5] qemu-iotests: Test commit block job where top has two parents

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/191     | 152 +++++++++
 tests/qemu-iotests/191.out | 827 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 980 insertions(+)
 create mode 100755 tests/qemu-iotests/191
 create mode 100644 tests/qemu-iotests/191.out

diff --git a/tests/qemu-iotests/191 b/tests/qemu-iotests/191
new file mode 100755
index 0000000000..a4e0d691fa
--- /dev/null
+++ b/tests/qemu-iotests/191
@@ -0,0 +1,152 @@
+#!/bin/bash
+#
+# Test commit block job where top has two parents
+#
+# Copyright (C) 2017 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=kwolf@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1 # failure is the default!
+
+MIG_SOCKET="${TEST_DIR}/migrate"
+
+_cleanup()
+{
+    rm -f "${TEST_IMG}.mid"
+    rm -f "${TEST_IMG}.ovl2"
+    rm -f "${TEST_IMG}.ovl3"
+    _cleanup_test_img
+    _cleanup_qemu
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+size=64M
+
+echo
+echo === Preparing and starting VM ===
+echo
+
+TEST_IMG="${TEST_IMG}.base" _make_test_img $size
+TEST_IMG="${TEST_IMG}.mid" _make_test_img -b "${TEST_IMG}.base"
+_make_test_img -b "${TEST_IMG}.mid"
+TEST_IMG="${TEST_IMG}.ovl2" _make_test_img -b "${TEST_IMG}.mid"
+
+$QEMU_IO -c 'write -P 0x55 1M 64k' "${TEST_IMG}.mid" | _filter_qemu_io
+
+qemu_comm_method="qmp"
+qmp_pretty="y"
+
+_launch_qemu \
+    -blockdev "driver=${IMGFMT},file.driver=file,file.filename=${TEST_IMG}.base,node-name=base" \
+    -blockdev "driver=${IMGFMT},file.driver=file,file.filename=${TEST_IMG}.mid,node-name=mid,backing=base" \
+    -blockdev "driver=${IMGFMT},file.driver=file,file.filename=${TEST_IMG},node-name=top,backing=mid" \
+    -blockdev "driver=${IMGFMT},file.driver=file,file.filename=${TEST_IMG}.ovl2,node-name=top2,backing=mid"
+h=$QEMU_HANDLE
+_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" '^}'
+
+echo
+echo === Perform commit job ===
+echo
+
+_send_qemu_cmd $h \
+    "{ 'execute': 'block-commit',
+       'arguments': { 'job-id': 'commit0',
+                      'device': 'top',
+                      'base':'$TEST_IMG.base',
+                      'top': '$TEST_IMG.mid' } }" \
+    "BLOCK_JOB_COMPLETED"
+_send_qemu_cmd $h "" "^}"
+
+echo
+echo === Check that both top and top2 point to base now ===
+echo
+
+_send_qemu_cmd $h "{ 'execute': 'query-named-block-nodes' }" "^}" |
+    _filter_generated_node_ids
+
+_send_qemu_cmd $h "{ 'execute': 'quit' }" "^}"
+wait=1 _cleanup_qemu
+
+_img_info
+TEST_IMG="$TEST_IMG.ovl2" _img_info
+
+
+echo
+echo === Preparing and starting VM with -drive ===
+echo
+
+TEST_IMG="${TEST_IMG}.base" _make_test_img $size
+TEST_IMG="${TEST_IMG}.mid" _make_test_img -b "${TEST_IMG}.base"
+_make_test_img -b "${TEST_IMG}.mid"
+TEST_IMG="${TEST_IMG}.ovl2" _make_test_img -b "${TEST_IMG}.mid"
+TEST_IMG="${TEST_IMG}.ovl3" _make_test_img -b "${TEST_IMG}.ovl2"
+
+$QEMU_IO -c 'write -P 0x55 1M 64k' "${TEST_IMG}.mid" | _filter_qemu_io
+
+qemu_comm_method="qmp"
+qmp_pretty="y"
+
+_launch_qemu \
+    -drive "driver=${IMGFMT},file=${TEST_IMG},node-name=top,backing.node-name=mid" \
+    -drive "driver=${IMGFMT},file=${TEST_IMG}.ovl3,node-name=top2,backing.backing=mid"
+h=$QEMU_HANDLE
+_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" '^}'
+
+echo
+echo === Perform commit job ===
+echo
+
+_send_qemu_cmd $h \
+    "{ 'execute': 'block-commit',
+       'arguments': { 'job-id': 'commit0',
+                      'device': 'top',
+                      'base':'$TEST_IMG.base',
+                      'top': '$TEST_IMG.mid' } }" \
+    "BLOCK_JOB_COMPLETED"
+_send_qemu_cmd $h "" "^}"
+
+echo
+echo === Check that both top and top2 point to base now ===
+echo
+
+_send_qemu_cmd $h "{ 'execute': 'query-named-block-nodes' }" "^}" |
+    _filter_generated_node_ids
+
+_send_qemu_cmd $h "{ 'execute': 'quit' }" "^}"
+wait=1 _cleanup_qemu
+
+_img_info
+TEST_IMG="$TEST_IMG.ovl2" _img_info
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/191.out b/tests/qemu-iotests/191.out
new file mode 100644
index 0000000000..7bfcd2d5d8
--- /dev/null
+++ b/tests/qemu-iotests/191.out
@@ -0,0 +1,827 @@
+QA output created by 191
+
+=== Preparing and starting VM ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT.mid', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.mid
+Formatting 'TEST_DIR/t.IMGFMT.ovl2', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.mid
+wrote 65536/65536 bytes at offset 1048576
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{
+    "return": {
+    }
+}
+
+=== Perform commit job ===
+
+{
+    "return": {
+    }
+}
+{
+    "timestamp": {
+        "seconds":  TIMESTAMP,
+        "microseconds":  TIMESTAMP
+    },
+    "event": "BLOCK_JOB_COMPLETED",
+    "data": {
+        "device": "commit0",
+        "len": 67108864,
+        "offset": 67108864,
+        "speed": 0,
+        "type": "commit"
+    }
+}
+
+=== Check that both top and top2 point to base now ===
+
+{
+    "return": [
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "backing-image": {
+                    "virtual-size": 67108864,
+                    "filename": "TEST_DIR/t.qcow2.base",
+                    "cluster-size": 65536,
+                    "format": "qcow2",
+                    "actual-size": 397312,
+                    "format-specific": {
+                        "type": "qcow2",
+                        "data": {
+                            "compat": "1.1",
+                            "lazy-refcounts": false,
+                            "refcount-bits": 16,
+                            "corrupt": false
+                        }
+                    },
+                    "dirty-flag": false
+                },
+                "backing-filename-format": "qcow2",
+                "virtual-size": 67108864,
+                "filename": "TEST_DIR/t.qcow2.ovl2",
+                "cluster-size": 65536,
+                "format": "qcow2",
+                "actual-size": 200704,
+                "format-specific": {
+                    "type": "qcow2",
+                    "data": {
+                        "compat": "1.1",
+                        "lazy-refcounts": false,
+                        "refcount-bits": 16,
+                        "corrupt": false
+                    }
+                },
+                "full-backing-filename": "TEST_DIR/t.qcow2.base",
+                "backing-filename": "TEST_DIR/t.qcow2.base",
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": false,
+            "node-name": "top2",
+            "backing_file_depth": 1,
+            "drv": "qcow2",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "backing_file": "TEST_DIR/t.qcow2.base",
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.qcow2.ovl2",
+            "encryption_key_missing": false
+        },
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "virtual-size": 197120,
+                "filename": "TEST_DIR/t.qcow2.ovl2",
+                "format": "file",
+                "actual-size": 200704,
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": false,
+            "node-name": "NODE_NAME",
+            "backing_file_depth": 0,
+            "drv": "file",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.qcow2.ovl2",
+            "encryption_key_missing": false
+        },
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "backing-image": {
+                    "virtual-size": 67108864,
+                    "filename": "TEST_DIR/t.qcow2.base",
+                    "cluster-size": 65536,
+                    "format": "qcow2",
+                    "actual-size": 397312,
+                    "format-specific": {
+                        "type": "qcow2",
+                        "data": {
+                            "compat": "1.1",
+                            "lazy-refcounts": false,
+                            "refcount-bits": 16,
+                            "corrupt": false
+                        }
+                    },
+                    "dirty-flag": false
+                },
+                "backing-filename-format": "qcow2",
+                "virtual-size": 67108864,
+                "filename": "TEST_DIR/t.qcow2",
+                "cluster-size": 65536,
+                "format": "qcow2",
+                "actual-size": 200704,
+                "format-specific": {
+                    "type": "qcow2",
+                    "data": {
+                        "compat": "1.1",
+                        "lazy-refcounts": false,
+                        "refcount-bits": 16,
+                        "corrupt": false
+                    }
+                },
+                "full-backing-filename": "TEST_DIR/t.qcow2.base",
+                "backing-filename": "TEST_DIR/t.qcow2.base",
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": false,
+            "node-name": "top",
+            "backing_file_depth": 1,
+            "drv": "qcow2",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "backing_file": "TEST_DIR/t.qcow2.base",
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.qcow2",
+            "encryption_key_missing": false
+        },
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "virtual-size": 197120,
+                "filename": "TEST_DIR/t.qcow2",
+                "format": "file",
+                "actual-size": 200704,
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": false,
+            "node-name": "NODE_NAME",
+            "backing_file_depth": 0,
+            "drv": "file",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.qcow2",
+            "encryption_key_missing": false
+        },
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "backing-image": {
+                    "virtual-size": 67108864,
+                    "filename": "TEST_DIR/t.qcow2.base",
+                    "cluster-size": 65536,
+                    "format": "qcow2",
+                    "actual-size": 397312,
+                    "format-specific": {
+                        "type": "qcow2",
+                        "data": {
+                            "compat": "1.1",
+                            "lazy-refcounts": false,
+                            "refcount-bits": 16,
+                            "corrupt": false
+                        }
+                    },
+                    "dirty-flag": false
+                },
+                "backing-filename-format": "qcow2",
+                "virtual-size": 67108864,
+                "filename": "TEST_DIR/t.qcow2.mid",
+                "cluster-size": 65536,
+                "format": "qcow2",
+                "actual-size": 397312,
+                "format-specific": {
+                    "type": "qcow2",
+                    "data": {
+                        "compat": "1.1",
+                        "lazy-refcounts": false,
+                        "refcount-bits": 16,
+                        "corrupt": false
+                    }
+                },
+                "full-backing-filename": "TEST_DIR/t.qcow2.base",
+                "backing-filename": "TEST_DIR/t.qcow2.base",
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": false,
+            "node-name": "mid",
+            "backing_file_depth": 1,
+            "drv": "qcow2",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "backing_file": "TEST_DIR/t.qcow2.base",
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.qcow2.mid",
+            "encryption_key_missing": false
+        },
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "virtual-size": 393216,
+                "filename": "TEST_DIR/t.qcow2.mid",
+                "format": "file",
+                "actual-size": 397312,
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": false,
+            "node-name": "NODE_NAME",
+            "backing_file_depth": 0,
+            "drv": "file",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.qcow2.mid",
+            "encryption_key_missing": false
+        },
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "virtual-size": 67108864,
+                "filename": "TEST_DIR/t.qcow2.base",
+                "cluster-size": 65536,
+                "format": "qcow2",
+                "actual-size": 397312,
+                "format-specific": {
+                    "type": "qcow2",
+                    "data": {
+                        "compat": "1.1",
+                        "lazy-refcounts": false,
+                        "refcount-bits": 16,
+                        "corrupt": false
+                    }
+                },
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": false,
+            "node-name": "base",
+            "backing_file_depth": 0,
+            "drv": "qcow2",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.qcow2.base",
+            "encryption_key_missing": false
+        },
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "virtual-size": 393216,
+                "filename": "TEST_DIR/t.qcow2.base",
+                "format": "file",
+                "actual-size": 397312,
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": false,
+            "node-name": "NODE_NAME",
+            "backing_file_depth": 0,
+            "drv": "file",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.qcow2.base",
+            "encryption_key_missing": false
+        }
+    ]
+}
+{
+    "return": {
+    }
+}
+{
+    "timestamp": {
+        "seconds":  TIMESTAMP,
+        "microseconds":  TIMESTAMP
+    },
+    "event": "SHUTDOWN",
+    "data": {
+        "guest": false
+    }
+}
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/t.IMGFMT.base
+backing file format: IMGFMT
+image: TEST_DIR/t.IMGFMT.ovl2
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/t.IMGFMT.base
+backing file format: IMGFMT
+
+=== Preparing and starting VM with -drive ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT.mid', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.base
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.mid
+Formatting 'TEST_DIR/t.IMGFMT.ovl2', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.mid
+Formatting 'TEST_DIR/t.IMGFMT.ovl3', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.ovl2
+wrote 65536/65536 bytes at offset 1048576
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{
+    "return": {
+    }
+}
+
+=== Perform commit job ===
+
+{
+    "return": {
+    }
+}
+{
+    "timestamp": {
+        "seconds":  TIMESTAMP,
+        "microseconds":  TIMESTAMP
+    },
+    "event": "BLOCK_JOB_COMPLETED",
+    "data": {
+        "device": "commit0",
+        "len": 67108864,
+        "offset": 67108864,
+        "speed": 0,
+        "type": "commit"
+    }
+}
+
+=== Check that both top and top2 point to base now ===
+
+{
+    "return": [
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "backing-image": {
+                    "virtual-size": 67108864,
+                    "filename": "TEST_DIR/t.qcow2.base",
+                    "cluster-size": 65536,
+                    "format": "qcow2",
+                    "actual-size": 397312,
+                    "format-specific": {
+                        "type": "qcow2",
+                        "data": {
+                            "compat": "1.1",
+                            "lazy-refcounts": false,
+                            "refcount-bits": 16,
+                            "corrupt": false
+                        }
+                    },
+                    "dirty-flag": false
+                },
+                "backing-filename-format": "qcow2",
+                "virtual-size": 67108864,
+                "filename": "TEST_DIR/t.qcow2.ovl2",
+                "cluster-size": 65536,
+                "format": "qcow2",
+                "actual-size": 200704,
+                "format-specific": {
+                    "type": "qcow2",
+                    "data": {
+                        "compat": "1.1",
+                        "lazy-refcounts": false,
+                        "refcount-bits": 16,
+                        "corrupt": false
+                    }
+                },
+                "full-backing-filename": "TEST_DIR/t.qcow2.base",
+                "backing-filename": "TEST_DIR/t.qcow2.base",
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": true,
+            "node-name": "NODE_NAME",
+            "backing_file_depth": 1,
+            "drv": "qcow2",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "backing_file": "TEST_DIR/t.qcow2.base",
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.qcow2.ovl2",
+            "encryption_key_missing": false
+        },
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "virtual-size": 197120,
+                "filename": "TEST_DIR/t.qcow2.ovl2",
+                "format": "file",
+                "actual-size": 200704,
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": true,
+            "node-name": "NODE_NAME",
+            "backing_file_depth": 0,
+            "drv": "file",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.qcow2.ovl2",
+            "encryption_key_missing": false
+        },
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "backing-image": {
+                    "backing-image": {
+                        "virtual-size": 67108864,
+                        "filename": "TEST_DIR/t.qcow2.base",
+                        "cluster-size": 65536,
+                        "format": "qcow2",
+                        "actual-size": 397312,
+                        "format-specific": {
+                            "type": "qcow2",
+                            "data": {
+                                "compat": "1.1",
+                                "lazy-refcounts": false,
+                                "refcount-bits": 16,
+                                "corrupt": false
+                            }
+                        },
+                        "dirty-flag": false
+                    },
+                    "backing-filename-format": "qcow2",
+                    "virtual-size": 67108864,
+                    "filename": "TEST_DIR/t.qcow2.ovl2",
+                    "cluster-size": 65536,
+                    "format": "qcow2",
+                    "actual-size": 200704,
+                    "format-specific": {
+                        "type": "qcow2",
+                        "data": {
+                            "compat": "1.1",
+                            "lazy-refcounts": false,
+                            "refcount-bits": 16,
+                            "corrupt": false
+                        }
+                    },
+                    "full-backing-filename": "TEST_DIR/t.qcow2.base",
+                    "backing-filename": "TEST_DIR/t.qcow2.base",
+                    "dirty-flag": false
+                },
+                "backing-filename-format": "qcow2",
+                "virtual-size": 67108864,
+                "filename": "TEST_DIR/t.qcow2.ovl3",
+                "cluster-size": 65536,
+                "format": "qcow2",
+                "actual-size": 200704,
+                "format-specific": {
+                    "type": "qcow2",
+                    "data": {
+                        "compat": "1.1",
+                        "lazy-refcounts": false,
+                        "refcount-bits": 16,
+                        "corrupt": false
+                    }
+                },
+                "full-backing-filename": "TEST_DIR/t.qcow2.ovl2",
+                "backing-filename": "TEST_DIR/t.qcow2.ovl2",
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": false,
+            "node-name": "top2",
+            "backing_file_depth": 2,
+            "drv": "qcow2",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "backing_file": "TEST_DIR/t.qcow2.ovl2",
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.qcow2.ovl3",
+            "encryption_key_missing": false
+        },
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "virtual-size": 197120,
+                "filename": "TEST_DIR/t.qcow2.ovl3",
+                "format": "file",
+                "actual-size": 200704,
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": false,
+            "node-name": "NODE_NAME",
+            "backing_file_depth": 0,
+            "drv": "file",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.qcow2.ovl3",
+            "encryption_key_missing": false
+        },
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "virtual-size": 67108864,
+                "filename": "TEST_DIR/t.qcow2.base",
+                "cluster-size": 65536,
+                "format": "qcow2",
+                "actual-size": 397312,
+                "format-specific": {
+                    "type": "qcow2",
+                    "data": {
+                        "compat": "1.1",
+                        "lazy-refcounts": false,
+                        "refcount-bits": 16,
+                        "corrupt": false
+                    }
+                },
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": true,
+            "node-name": "NODE_NAME",
+            "backing_file_depth": 0,
+            "drv": "qcow2",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.qcow2.base",
+            "encryption_key_missing": false
+        },
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "virtual-size": 393216,
+                "filename": "TEST_DIR/t.qcow2.base",
+                "format": "file",
+                "actual-size": 397312,
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": true,
+            "node-name": "NODE_NAME",
+            "backing_file_depth": 0,
+            "drv": "file",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.qcow2.base",
+            "encryption_key_missing": false
+        },
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "backing-image": {
+                    "virtual-size": 67108864,
+                    "filename": "TEST_DIR/t.qcow2.base",
+                    "cluster-size": 65536,
+                    "format": "qcow2",
+                    "actual-size": 397312,
+                    "format-specific": {
+                        "type": "qcow2",
+                        "data": {
+                            "compat": "1.1",
+                            "lazy-refcounts": false,
+                            "refcount-bits": 16,
+                            "corrupt": false
+                        }
+                    },
+                    "dirty-flag": false
+                },
+                "backing-filename-format": "qcow2",
+                "virtual-size": 67108864,
+                "filename": "TEST_DIR/t.qcow2",
+                "cluster-size": 65536,
+                "format": "qcow2",
+                "actual-size": 200704,
+                "format-specific": {
+                    "type": "qcow2",
+                    "data": {
+                        "compat": "1.1",
+                        "lazy-refcounts": false,
+                        "refcount-bits": 16,
+                        "corrupt": false
+                    }
+                },
+                "full-backing-filename": "TEST_DIR/t.qcow2.base",
+                "backing-filename": "TEST_DIR/t.qcow2.base",
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": false,
+            "node-name": "top",
+            "backing_file_depth": 1,
+            "drv": "qcow2",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "backing_file": "TEST_DIR/t.qcow2.base",
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.qcow2",
+            "encryption_key_missing": false
+        },
+        {
+            "iops_rd": 0,
+            "detect_zeroes": "off",
+            "image": {
+                "virtual-size": 197120,
+                "filename": "TEST_DIR/t.qcow2",
+                "format": "file",
+                "actual-size": 200704,
+                "dirty-flag": false
+            },
+            "iops_wr": 0,
+            "ro": false,
+            "node-name": "NODE_NAME",
+            "backing_file_depth": 0,
+            "drv": "file",
+            "iops": 0,
+            "bps_wr": 0,
+            "write_threshold": 0,
+            "encrypted": false,
+            "bps": 0,
+            "bps_rd": 0,
+            "cache": {
+                "no-flush": false,
+                "direct": false,
+                "writeback": true
+            },
+            "file": "TEST_DIR/t.qcow2",
+            "encryption_key_missing": false
+        }
+    ]
+}
+{
+    "return": {
+    }
+}
+{
+    "timestamp": {
+        "seconds":  TIMESTAMP,
+        "microseconds":  TIMESTAMP
+    },
+    "event": "SHUTDOWN",
+    "data": {
+        "guest": false
+    }
+}
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/t.IMGFMT.base
+backing file format: IMGFMT
+image: TEST_DIR/t.IMGFMT.ovl2
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/t.IMGFMT.base
+backing file format: IMGFMT
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 108339cd03..63d4ee052c 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -187,6 +187,7 @@
 188 rw auto quick
 189 rw auto
 190 rw auto quick
+191 rw auto
 192 rw auto quick
 194 rw auto migration quick
 195 rw auto quick
-- 
2.13.5

[Qemu-devel] [PATCH 5/5] commit: Remove overlay_bs

[Kevin Wolf]

We don't need to make any assumptions about the graph layout above the
top node of the commit operation any more. Remove the use of
bdrv_find_overlay() and related variables from the commit job code.

bdrv_drop_intermediate() doesn't use the 'active' parameter any more, so
we can just drop it.

The overlay node was previously added to the block job to get a
BLK_PERM_GRAPH_MOD. We really need to respect those permissions in
bdrv_drop_intermediate() now, but as long as we haven't figured out yet
how BLK_PERM_GRAPH_MOD is actually supposed to work, just leave a TODO
comment there.

With this change, it is now possible to perform another block job on an
overlay node without conflicts. qemu-iotests 030 is changed accordingly.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block.h  |  3 +--
 block.c                |  6 +++--
 block/commit.c         | 62 ++++++++++++--------------------------------------
 tests/qemu-iotests/030 |  4 ----
 4 files changed, 20 insertions(+), 55 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 3c3af462e4..d5c2731a03 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -315,8 +315,7 @@ int bdrv_commit(BlockDriverState *bs);
 int bdrv_change_backing_file(BlockDriverState *bs,
     const char *backing_file, const char *backing_fmt);
 void bdrv_register(BlockDriver *bdrv);
-int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
-                           BlockDriverState *base,
+int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
                            const char *backing_file_str);
 BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
                                     BlockDriverState *bs);
diff --git a/block.c b/block.c
index e57fab501d..5f2d7222a5 100644
--- a/block.c
+++ b/block.c
@@ -3491,8 +3491,8 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs)
  *  if active == top, that is considered an error
  *
  */
-int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
-                           BlockDriverState *base, const char *backing_file_str)
+int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
+                           const char *backing_file_str)
 {
     BdrvChild *c, *next;
     Error *local_err = NULL;
@@ -3510,6 +3510,8 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
     }
 
     /* success - we can delete the intermediate states, and link top->base */
+    /* TODO Check graph modification op blockers (BLK_PERM_GRAPH_MOD) once
+     * we've figured out how they should work. */
     backing_file_str = backing_file_str ? backing_file_str : base->filename;
 
     QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
diff --git a/block/commit.c b/block/commit.c
index 610e1cd8f5..5036eec434 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -36,13 +36,11 @@ enum {
 typedef struct CommitBlockJob {
     BlockJob common;
     RateLimit limit;
-    BlockDriverState *active;
     BlockDriverState *commit_top_bs;
     BlockBackend *top;
     BlockBackend *base;
     BlockdevOnError on_error;
     int base_flags;
-    int orig_overlay_flags;
     char *backing_file_str;
 } CommitBlockJob;
 
@@ -81,18 +79,15 @@ static void commit_complete(BlockJob *job, void *opaque)
 {
     CommitBlockJob *s = container_of(job, CommitBlockJob, common);
     CommitCompleteData *data = opaque;
-    BlockDriverState *active = s->active;
     BlockDriverState *top = blk_bs(s->top);
     BlockDriverState *base = blk_bs(s->base);
-    BlockDriverState *overlay_bs = bdrv_find_overlay(active, s->commit_top_bs);
+    BlockDriverState *commit_top_bs = s->commit_top_bs;
     int ret = data->ret;
     bool remove_commit_top_bs = false;
 
-    /* Make sure overlay_bs and top stay around until bdrv_set_backing_hd() */
+    /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
     bdrv_ref(top);
-    if (overlay_bs) {
-        bdrv_ref(overlay_bs);
-    }
+    bdrv_ref(commit_top_bs);
 
     /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
      * the normal backing chain can be restored. */
@@ -100,9 +95,9 @@ static void commit_complete(BlockJob *job, void *opaque)
 
     if (!block_job_is_cancelled(&s->common) && ret == 0) {
         /* success */
-        ret = bdrv_drop_intermediate(active, s->commit_top_bs, base,
+        ret = bdrv_drop_intermediate(s->commit_top_bs, base,
                                      s->backing_file_str);
-    } else if (overlay_bs) {
+    } else {
         /* XXX Can (or should) we somehow keep 'consistent read' blocked even
          * after the failed/cancelled commit job is gone? If we already wrote
          * something to base, the intermediate images aren't valid any more. */
@@ -115,9 +110,6 @@ static void commit_complete(BlockJob *job, void *opaque)
     if (s->base_flags != bdrv_get_flags(base)) {
         bdrv_reopen(base, s->base_flags, NULL);
     }
-    if (overlay_bs && s->orig_overlay_flags != bdrv_get_flags(overlay_bs)) {
-        bdrv_reopen(overlay_bs, s->orig_overlay_flags, NULL);
-    }
     g_free(s->backing_file_str);
     blk_unref(s->top);
 
@@ -134,10 +126,13 @@ static void commit_complete(BlockJob *job, void *opaque)
      * filter driver from the backing chain. Do this as the final step so that
      * the 'consistent read' permission can be granted.  */
     if (remove_commit_top_bs) {
-        bdrv_set_backing_hd(overlay_bs, top, &error_abort);
+        bdrv_child_try_set_perm(commit_top_bs->backing, 0, BLK_PERM_ALL,
+                                &error_abort);
+        bdrv_replace_node(commit_top_bs, backing_bs(commit_top_bs),
+                          &error_abort);
     }
 
-    bdrv_unref(overlay_bs);
+    bdrv_unref(commit_top_bs);
     bdrv_unref(top);
 }
 
@@ -283,10 +278,8 @@ void commit_start(const char *job_id, BlockDriverState *bs,
 {
     CommitBlockJob *s;
     BlockReopenQueue *reopen_queue = NULL;
-    int orig_overlay_flags;
     int orig_base_flags;
     BlockDriverState *iter;
-    BlockDriverState *overlay_bs;
     BlockDriverState *commit_top_bs = NULL;
     Error *local_err = NULL;
     int ret;
@@ -297,31 +290,19 @@ void commit_start(const char *job_id, BlockDriverState *bs,
         return;
     }
 
-    overlay_bs = bdrv_find_overlay(bs, top);
-
-    if (overlay_bs == NULL) {
-        error_setg(errp, "Could not find overlay image for %s:", top->filename);
-        return;
-    }
-
     s = block_job_create(job_id, &commit_job_driver, bs, 0, BLK_PERM_ALL,
                          speed, BLOCK_JOB_DEFAULT, NULL, NULL, errp);
     if (!s) {
         return;
     }
 
-    orig_base_flags    = bdrv_get_flags(base);
-    orig_overlay_flags = bdrv_get_flags(overlay_bs);
-
-    /* convert base & overlay_bs to r/w, if necessary */
+    /* convert base to r/w, if necessary */
+    orig_base_flags = bdrv_get_flags(base);
     if (!(orig_base_flags & BDRV_O_RDWR)) {
         reopen_queue = bdrv_reopen_queue(reopen_queue, base, NULL,
                                          orig_base_flags | BDRV_O_RDWR);
     }
-    if (!(orig_overlay_flags & BDRV_O_RDWR)) {
-        reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, NULL,
-                                         orig_overlay_flags | BDRV_O_RDWR);
-    }
+
     if (reopen_queue) {
         bdrv_reopen_multiple(bdrv_get_aio_context(bs), reopen_queue, &local_err);
         if (local_err != NULL) {
@@ -382,14 +363,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
         goto fail;
     }
 
-    /* overlay_bs must be blocked because it needs to be modified to
-     * update the backing image string. */
-    ret = block_job_add_bdrv(&s->common, "overlay of top", overlay_bs,
-                             BLK_PERM_GRAPH_MOD, BLK_PERM_ALL, errp);
-    if (ret < 0) {
-        goto fail;
-    }
-
     s->base = blk_new(BLK_PERM_CONSISTENT_READ
                       | BLK_PERM_WRITE
                       | BLK_PERM_RESIZE,
@@ -408,13 +381,8 @@ void commit_start(const char *job_id, BlockDriverState *bs,
         goto fail;
     }
 
-    s->active = bs;
-
-    s->base_flags          = orig_base_flags;
-    s->orig_overlay_flags  = orig_overlay_flags;
-
+    s->base_flags = orig_base_flags;
     s->backing_file_str = g_strdup(backing_file_str);
-
     s->on_error = on_error;
 
     trace_commit_start(bs, base, top, s);
@@ -429,7 +397,7 @@ fail:
         blk_unref(s->top);
     }
     if (commit_top_bs) {
-        bdrv_set_backing_hd(overlay_bs, top, &error_abort);
+        bdrv_replace_node(commit_top_bs, top, &error_abort);
     }
     block_job_early_fail(&s->common);
 }
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index d745cb4cde..18838948fa 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -287,10 +287,6 @@ class TestParallelOps(iotests.QMPTestCase):
         result = self.vm.qmp('block-stream', device='node6', base=self.imgs[4], job_id='stream-node6-v2')
         self.assert_qmp(result, 'error/class', 'GenericError')
 
-        # This fails because block-commit needs to block node6, the overlay of the 'top' image
-        result = self.vm.qmp('block-stream', device='node7', base=self.imgs[5], job_id='stream-node6-v3')
-        self.assert_qmp(result, 'error/class', 'GenericError')
-
         # This fails because block-commit currently blocks the active layer even if it's not used
         result = self.vm.qmp('block-stream', device='drive0', base=self.imgs[5], job_id='stream-drive0')
         self.assert_qmp(result, 'error/class', 'GenericError')
-- 
2.13.5

Re: [Qemu-devel] [PATCH 1/5] block: Introduce BdrvChildRole.update_filename

[Eric Blake]

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

On 09/25/2017 07:28 AM, Kevin Wolf wrote:
> There is no good reason for bdrv_drop_intermediate() to know the active
> layer above the subchain it is operating on - even more so, because
> the assumption that there is a single active layer above it is not
> generally true.
> 
> In order to prepare removal of the active parameter, use a BdrvChildRole
> callback to update the backing file string in the overlay image instead
> of directly calling bdrv_change_backing_file().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block_int.h |  6 ++++++
>  block.c                   | 33 ++++++++++++++++++++++++++++-----
>  2 files changed, 34 insertions(+), 5 deletions(-)
> 

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

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


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

Re: [Qemu-devel] [PATCH 2/5] commit: Support multiple roots above top node

[Eric Blake]

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

On 09/25/2017 07:28 AM, Kevin Wolf wrote:
> This changes the commit block job to support operation in a graph where
> there is more than a single active layer that references the top node.
> 
> This involves inserting the commit filter node not only on the path
> between the given active node and the top node, but between the top node
> and all of its parents.
> 
> On completion, bdrv_drop_intermediate() must consider all parents for
> updating the backing file link. These parents may be backing files
> themselves and such read-only; reopen them temporarily if necessary.

s/such/as such/

> Previously this was achieved by the bdrv_reopen() calls in the commit
> block job that made overlay_bs read-write for the whole duration of the
> block job, even though write access is only needed on completion.

Do we know, at the start of the operation, enough to reject attempts
that will eventually fail because we cannot obtain write access at
completion, without having to wait for all the intermediate work before
the completion phase is reached?  If not, that might be a bit of a
regression (where a command used to fail up front, we now waste a lot of
effort, and possibly modify the backing chain irreversably, before
finding out we still fail because we lack write access).

> 
> Now that we consider all parents, overlay_bs is meaningless. It is left
> in place in this commit, but we'll remove it soon.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c        | 68 ++++++++++++++++++++++++++++++++++------------------------
>  block/commit.c |  2 +-
>  2 files changed, 41 insertions(+), 29 deletions(-)
>  
>      /* success - we can delete the intermediate states, and link top->base */
> -    if (new_top_bs->backing->role->update_filename) {
> -        backing_file_str = backing_file_str ? backing_file_str : base->filename;
> -        ret = new_top_bs->backing->role->update_filename(new_top_bs->backing,
> -                                                         base, backing_file_str,
> -                                                         &local_err);
> -        if (ret < 0) {
> -            bdrv_set_backing_hd(new_top_bs, top, &error_abort);
> +    backing_file_str = backing_file_str ? backing_file_str : base->filename;
> +
> +    QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
> +        /* Check whether we are allowed to switch c from top to base */
> +        GSList *ignore_children = g_slist_prepend(NULL, c);
> +        bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,
> +                               ignore_children, &local_err);
> +        if (local_err) {
> +            ret = -EPERM;
> +            error_report_err(local_err);
>              goto exit;
>          }
> -    }
> +        g_slist_free(ignore_children);
>  

And it looks like you DO check up front that permissions are sufficient
for later on.


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


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

Re: [Qemu-devel] [PATCH 3/5] qemu-iotests: Allow QMP pretty printing in common.qemu

[Eric Blake]

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

On 09/25/2017 07:28 AM, Kevin Wolf wrote:
> QMP responses to certain commands can become quite long, which doesn't
> only make reading them hard, but also means that the maximum line length
> in patch emails can be exceeded. Allow tests to switch to QMP pretty
> printing, which results in more, but shorter lines.
> 
> We also need to make sure to keep indentation in the response for this
> to work as expected.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/common.qemu | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)

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

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


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

Re: [Qemu-devel] [Qemu-block] [PATCH 0/5] commit: Support multiple roots above top node

[John Snow]

On 09/25/2017 08:28 AM, Kevin Wolf wrote:
> This is a step towards making the commit job flexible enough that it can
> work with any kind of block graph. Currently, it requires that not only
> the top and base node of the commit operation are specified, but also
> the active layer of the backing file chain. Of course, the assumption
> that a single active layer exists is invalid.
> 
> This series makes the commit job consider other roots as well so that
> all parent nodes of the top node get their backing file updated and stay
> valid after the commit job completes.
> 
> With this, we should have all of the prerequisites for a follow-up
> series that adds a new and clean blockdev-commit QMP command which
> doesn't require an option for the active layer and which accepts node
> names instead of file names for base and top.
> 
> Kevin Wolf (5):
>   block: Introduce BdrvChildRole.update_filename
>   commit: Support multiple roots above top node
>   qemu-iotests: Allow QMP pretty printing in common.qemu
>   qemu-iotests: Test commit block job where top has two parents
>   commit: Remove overlay_bs
> 
>  include/block/block.h          |   3 +-
>  include/block/block_int.h      |   6 +
>  block.c                        |  91 +++--
>  block/commit.c                 |  64 +---
>  tests/qemu-iotests/030         |   4 -
>  tests/qemu-iotests/191         | 152 ++++++++
>  tests/qemu-iotests/191.out     | 827 +++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/common.qemu |  14 +-
>  tests/qemu-iotests/group       |   1 +
>  9 files changed, 1078 insertions(+), 84 deletions(-)
>  create mode 100755 tests/qemu-iotests/191
>  create mode 100644 tests/qemu-iotests/191.out
> 

Does this depend on another series?

Re: [Qemu-devel] [PATCH 4/5] qemu-iotests: Test commit block job where top has two parents

[Eric Blake]

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

On 09/25/2017 07:28 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/191     | 152 +++++++++
>  tests/qemu-iotests/191.out | 827 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 980 insertions(+)
>  create mode 100755 tests/qemu-iotests/191
>  create mode 100644 tests/qemu-iotests/191.out
> 

> +
> +_cleanup()
> +{
> +    rm -f "${TEST_IMG}.mid"
> +    rm -f "${TEST_IMG}.ovl2"
> +    rm -f "${TEST_IMG}.ovl3"
> +    _cleanup_test_img
> +    _cleanup_qemu
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15

Semantic conflict with Jeff's work to allow preservation of intermediate
files via './check -s'.

> +++ b/tests/qemu-iotests/191.out

> +{
> +    "timestamp": {
> +        "seconds":  TIMESTAMP,
> +        "microseconds":  TIMESTAMP
> +    },
> +    "event": "BLOCK_JOB_COMPLETED",
> +    "data": {
> +        "device": "commit0",
> +        "len": 67108864,
> +        "offset": 67108864,
> +        "speed": 0,
> +        "type": "commit"
> +    }
> +}

This may be sensitive to ordering if I ever finish my patches that allow
for QAPI->JSON conversion without having to first go through QObject (as
it is, it's dependent on QDict's hash being stable, whereas my code
switches things to be stable according to QAPI ordering).  But that's
not a problem for your patch.


> +                        "data": {
> +                            "compat": "1.1",

You should make the test specifically exclude compat=0.10 images, or
else have further filtering in place if we can still support this on old
images.


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


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

Re: [Qemu-devel] [PATCH 5/5] commit: Remove overlay_bs

[Eric Blake]

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

On 09/25/2017 07:28 AM, Kevin Wolf wrote:
> We don't need to make any assumptions about the graph layout above the
> top node of the commit operation any more. Remove the use of
> bdrv_find_overlay() and related variables from the commit job code.
> 
> bdrv_drop_intermediate() doesn't use the 'active' parameter any more, so
> we can just drop it.
> 
> The overlay node was previously added to the block job to get a
> BLK_PERM_GRAPH_MOD. We really need to respect those permissions in
> bdrv_drop_intermediate() now, but as long as we haven't figured out yet
> how BLK_PERM_GRAPH_MOD is actually supposed to work, just leave a TODO
> comment there.
> 
> With this change, it is now possible to perform another block job on an
> overlay node without conflicts. qemu-iotests 030 is changed accordingly.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block.h  |  3 +--
>  block.c                |  6 +++--
>  block/commit.c         | 62 ++++++++++++--------------------------------------
>  tests/qemu-iotests/030 |  4 ----
>  4 files changed, 20 insertions(+), 55 deletions(-)
> 

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

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


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

Re: [Qemu-devel] [Qemu-block] [PATCH 0/5] commit: Support multiple roots above top node

[Kevin Wolf]

Am 25.09.2017 um 22:02 hat John Snow geschrieben:
> On 09/25/2017 08:28 AM, Kevin Wolf wrote:
> > This is a step towards making the commit job flexible enough that it can
> > work with any kind of block graph. Currently, it requires that not only
> > the top and base node of the commit operation are specified, but also
> > the active layer of the backing file chain. Of course, the assumption
> > that a single active layer exists is invalid.
> > 
> > This series makes the commit job consider other roots as well so that
> > all parent nodes of the top node get their backing file updated and stay
> > valid after the commit job completes.
> > 
> > With this, we should have all of the prerequisites for a follow-up
> > series that adds a new and clean blockdev-commit QMP command which
> > doesn't require an option for the active layer and which accepts node
> > names instead of file names for base and top.
> > 
> > Kevin Wolf (5):
> >   block: Introduce BdrvChildRole.update_filename
> >   commit: Support multiple roots above top node
> >   qemu-iotests: Allow QMP pretty printing in common.qemu
> >   qemu-iotests: Test commit block job where top has two parents
> >   commit: Remove overlay_bs
> 
> Does this depend on another series?

It is based on my block branch. I think specifically the series "block:
Fix permissions after ro/rw reopen" might be needed for the patches to
work correctly.

Kevin

Re: [Qemu-devel] [PATCH 2/5] commit: Support multiple roots above top node

[Kevin Wolf]

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

Am 25.09.2017 um 21:38 hat Eric Blake geschrieben:
> On 09/25/2017 07:28 AM, Kevin Wolf wrote:
> > This changes the commit block job to support operation in a graph where
> > there is more than a single active layer that references the top node.
> > 
> > This involves inserting the commit filter node not only on the path
> > between the given active node and the top node, but between the top node
> > and all of its parents.
> > 
> > On completion, bdrv_drop_intermediate() must consider all parents for
> > updating the backing file link. These parents may be backing files
> > themselves and such read-only; reopen them temporarily if necessary.
> 
> s/such/as such/

Yes, thanks.

> > Previously this was achieved by the bdrv_reopen() calls in the commit
> > block job that made overlay_bs read-write for the whole duration of the
> > block job, even though write access is only needed on completion.
> 
> Do we know, at the start of the operation, enough to reject attempts
> that will eventually fail because we cannot obtain write access at
> completion, without having to wait for all the intermediate work before
> the completion phase is reached?  If not, that might be a bit of a
> regression (where a command used to fail up front, we now waste a lot of
> effort, and possibly modify the backing chain irreversably, before
> finding out we still fail because we lack write access).

It is kind of a change in behaviour, but I wouldn't call it a
regression. The reason is that today I'm looking at it with a completely
different perspective than when the command was added.

Originally, we assumed a more or less static block node graph, which in
fact was only a mostly degenerated tree - a backing file chain. Block
jobs were kind of an exceptional thing and we allowed only one per
chain. In this restricted setting, checking at the start of the
operation made sense because we wouldn't allow things to change while
the job was running anyway.

A big part of the recent blockdev work, however, is about getting rid of
arbitrary restrictions like this. We want to allow running two commit
operations in the same backing chain as long as they don't conflict. We
don't want to prevent adding new users to an existing node unless there
is a good reason.

If you consider this, the set of nodes that will get their backing file
rewritten at the end of the block job isn't known yet when the job
starts; and even if we knew it, keeping the nodes writable all the time
while the job runs feels like it could easily conflict with other
callers of bdrv_reopen().

> > Now that we consider all parents, overlay_bs is meaningless. It is left
> > in place in this commit, but we'll remove it soon.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block.c        | 68 ++++++++++++++++++++++++++++++++++------------------------
> >  block/commit.c |  2 +-
> >  2 files changed, 41 insertions(+), 29 deletions(-)
> >  
> >      /* success - we can delete the intermediate states, and link top->base */
> > -    if (new_top_bs->backing->role->update_filename) {
> > -        backing_file_str = backing_file_str ? backing_file_str : base->filename;
> > -        ret = new_top_bs->backing->role->update_filename(new_top_bs->backing,
> > -                                                         base, backing_file_str,
> > -                                                         &local_err);
> > -        if (ret < 0) {
> > -            bdrv_set_backing_hd(new_top_bs, top, &error_abort);
> > +    backing_file_str = backing_file_str ? backing_file_str : base->filename;
> > +
> > +    QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
> > +        /* Check whether we are allowed to switch c from top to base */
> > +        GSList *ignore_children = g_slist_prepend(NULL, c);
> > +        bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,
> > +                               ignore_children, &local_err);
> > +        if (local_err) {
> > +            ret = -EPERM;
> > +            error_report_err(local_err);
> >              goto exit;
> >          }
> > -    }
> > +        g_slist_free(ignore_children);
> >  
> 
> And it looks like you DO check up front that permissions are sufficient
> for later on.

This is already during completion. The completion code uses the
transactional nature of permission updates to make sure that the
in-memory graph stays consistent with the on-disk backing file links
even in case of errors, but it's not really upfront in the sense of
checking when the block job is started. (You also need to hold the BQL
between starting and completing a permission change transaction, so just
moving this code wouldn't work.)

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [PATCH 4/5] qemu-iotests: Test commit block job where top has two parents

[Kevin Wolf]

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

Am 25.09.2017 um 22:19 hat Eric Blake geschrieben:
> On 09/25/2017 07:28 AM, Kevin Wolf wrote:
> > +                        "data": {
> > +                            "compat": "1.1",
> 
> You should make the test specifically exclude compat=0.10 images, or
> else have further filtering in place if we can still support this on old
> images.

Squashed in a line to this effect:

    _unsupported_imgopts compat=0.10

And applied the series to the block branch.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]