block.c | 192 ++++++++ blockdev.c | 44 ++ include/block/block.h | 6 + qapi/block-core.json | 60 +++ tests/qemu-iotests/193 | 241 ++++++++++ tests/qemu-iotests/193.out | 1116 ++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/group | 1 + 7 files changed, 1660 insertions(+) create mode 100755 tests/qemu-iotests/193 create mode 100644 tests/qemu-iotests/193.out
block-insert-node and its pair command block-remove-node provide runtime
insertion and removal of filter nodes.
block-insert-node takes a (parent, child) and (node, child) pair of
edges and unrefs the (parent, child) BdrvChild relationship and creates
a new (parent, node) child with the same BdrvChildRole.
This is a different approach than x-blockdev-change which uses the driver
methods bdrv_add_child() and bdrv_del_child(),
Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
block.c | 192 ++++++++
blockdev.c | 44 ++
include/block/block.h | 6 +
qapi/block-core.json | 60 +++
tests/qemu-iotests/193 | 241 ++++++++++
tests/qemu-iotests/193.out | 1116 ++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/group | 1 +
7 files changed, 1660 insertions(+)
create mode 100755 tests/qemu-iotests/193
create mode 100644 tests/qemu-iotests/193.out
diff --git a/block.c b/block.c
index 81bd51b670..f874aabbfb 100644
--- a/block.c
+++ b/block.c
@@ -930,6 +930,9 @@ static void bdrv_backing_attach(BdrvChild *c)
parent->backing_blocker);
bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_STREAM,
parent->backing_blocker);
+ /* Unblock filter node insertion */
+ bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_EDGE_MODIFICATION,
+ parent->backing_blocker);
/*
* We do backup in 3 ways:
* 1. drive backup
@@ -5036,3 +5039,192 @@ BlockDriverState *bdrv_get_first_explicit(BlockDriverState *bs)
}
return bs;
}
+
+
+static inline BdrvChild *bdrv_find_child(BlockDriverState *parent_bs,
+ const char *child_name)
+{
+ BdrvChild *child;
+ assert(child_name);
+
+ QLIST_FOREACH(child, &parent_bs->children, next) {
+ if (child->bs && !g_strcmp0(child->bs->node_name, child_name)) {
+ return child;
+ }
+ }
+
+ return NULL;
+}
+
+static int check_node_edge(const char *parent, const char *child, Error **errp)
+{
+ BlockDriverState *parent_bs, *child_bs;
+ parent_bs = bdrv_find_node(parent);
+ if (!parent_bs) {
+ error_setg(errp, "'%s' not a node name", parent);
+ return 1;
+ }
+ child_bs = bdrv_find_node(child);
+ if (!child_bs) {
+ error_setg(errp, "'%s' not a node name", child);
+ return 1;
+ }
+ if (!bdrv_find_child(parent_bs, child)) {
+ error_setg(errp, "'%s' not a child of '%s'", child, parent);
+ return 1;
+ }
+ if (bdrv_op_is_blocked(parent_bs, BLOCK_OP_TYPE_EDGE_MODIFICATION, errp) ||
+ bdrv_op_is_blocked(child_bs, BLOCK_OP_TYPE_EDGE_MODIFICATION, errp)) {
+ return 1;
+ }
+ return 0;
+}
+
+void bdrv_insert_node(const char *parent, const char *child,
+ const char *node, Error **errp)
+{
+ BlockBackend *blk;
+ BlockDriverState *parent_bs, *node_bs, *child_bs;
+ BdrvChild *c;
+ const BdrvChildRole *role;
+
+ if (check_node_edge(node, child, errp)) {
+ return;
+ }
+ node_bs = bdrv_find_node(node);
+ child_bs = bdrv_find_node(child);
+ blk = blk_by_name(parent);
+ if (blk) {
+ /* insert 'node' as root bs of 'parent' device */
+ if (!blk_bs(blk)) {
+ error_setg(errp, "Device '%s' has no medium", parent);
+ return;
+ }
+ if (blk_bs(blk) != child_bs) {
+ error_setg(errp, "'%s' not a child of device '%s'", child, parent);
+ return;
+ }
+ bdrv_drained_begin(child_bs);
+ blk_remove_bs(blk);
+ blk_insert_bs(blk, node_bs, errp);
+ if (!blk_bs(blk)) {
+ blk_insert_bs(blk, child_bs, &error_abort);
+ }
+ bdrv_drained_end(child_bs);
+ return;
+ }
+
+ /* insert 'node' as child bs of 'parent' node */
+ if (check_node_edge(parent, child, errp)) {
+ return;
+ }
+ parent_bs = bdrv_find_node(parent);
+ c = bdrv_find_child(parent_bs, child);
+ role = c->role;
+ assert(role == &child_file || role == &child_backing);
+
+ bdrv_ref(node_bs);
+
+ bdrv_drained_begin(parent_bs);
+ bdrv_unref_child(parent_bs, c);
+ if (role == &child_file) {
+ parent_bs->file = bdrv_attach_child(parent_bs, node_bs, "file",
+ &child_file, errp);
+ if (!parent_bs->file) {
+ parent_bs->file = bdrv_attach_child(parent_bs, child_bs, "file",
+ &child_file, &error_abort);
+ goto out;
+ }
+ } else if (role == &child_backing) {
+ parent_bs->backing = bdrv_attach_child(parent_bs, node_bs, "backing",
+ &child_backing, errp);
+ if (!parent_bs->backing) {
+ parent_bs->backing = bdrv_attach_child(parent_bs, child_bs,
+ "backing", &child_backing,
+ &error_abort);
+ goto out;
+ }
+ }
+ bdrv_refresh_filename(parent_bs);
+ bdrv_refresh_limits(parent_bs, NULL);
+
+out:
+ bdrv_drained_end(parent_bs);
+}
+
+void bdrv_remove_node(const char *parent, const char *child,
+ const char *node, Error **errp)
+{
+ BlockBackend *blk;
+ BlockDriverState *node_bs, *parent_bs, *child_bs;
+ BdrvChild *c;
+ const BdrvChildRole *role;
+
+ if (check_node_edge(node, child, errp)) {
+ return;
+ }
+ node_bs = bdrv_find_node(node);
+ child_bs = bdrv_find_node(child);
+ blk = blk_by_name(parent);
+ if (blk) {
+ /* remove 'node' as root bs of 'parent' device */
+ if (!blk_bs(blk)) {
+ error_setg(errp, "Device '%s' has no medium", parent);
+ return;
+ }
+ if (blk_bs(blk) != node_bs) {
+ error_setg(errp, "'%s' not a child of device '%s'", node, parent);
+ return;
+ }
+ bdrv_drained_begin(node_bs);
+ blk_remove_bs(blk);
+ blk_insert_bs(blk, child_bs, errp);
+ if (!blk_bs(blk)) {
+ blk_insert_bs(blk, node_bs, &error_abort);
+ }
+ bdrv_drained_end(node_bs);
+ return;
+ }
+
+ if (check_node_edge(parent, node, errp)) {
+ return;
+ }
+ parent_bs = bdrv_find_node(parent);
+ c = bdrv_find_child(node_bs, child);
+ role = c->role;
+ assert(role == &child_file || role == &child_backing);
+
+
+ bdrv_ref(child_bs);
+ bdrv_ref(node_bs);
+
+ bdrv_drained_begin(parent_bs);
+ bdrv_unref_child(parent_bs, bdrv_find_child(parent_bs, node));
+ if (role == &child_file) {
+ parent_bs->file = bdrv_attach_child(parent_bs, child_bs, "file",
+ &child_file, errp);
+ if (!parent_bs->file) {
+ parent_bs->file = bdrv_attach_child(parent_bs, node_bs, "file",
+ &child_file, &error_abort);
+ node_bs->file = bdrv_attach_child(node_bs, child_bs, "file",
+ &child_file, &error_abort);
+ goto out;
+ }
+ } else if (role == &child_backing) {
+ parent_bs->backing = bdrv_attach_child(parent_bs, child_bs, "backing",
+ &child_backing, errp);
+ if (!parent_bs->backing) {
+ parent_bs->backing = bdrv_attach_child(parent_bs, node_bs,
+ "backing", &child_backing,
+ &error_abort);
+ node_bs->backing = bdrv_attach_child(node_bs, child_bs, "backing",
+ &child_backing, &error_abort);
+ goto out;
+ }
+ }
+ bdrv_refresh_filename(parent_bs);
+ bdrv_refresh_limits(parent_bs, NULL);
+ bdrv_unref(node_bs);
+out:
+ bdrv_drained_end(parent_bs);
+}
diff --git a/blockdev.c b/blockdev.c
index 8e2fc6e64c..5195ec1b61 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4238,3 +4238,47 @@ QemuOptsList qemu_drive_opts = {
{ /* end of list */ }
},
};
+
+void qmp_block_insert_node(const char *parent, const char *child,
+ const char *node, Error **errp)
+{
+ BlockDriverState *bs = bdrv_find_node(node);
+ if (!bs) {
+ error_setg(errp, "Node '%s' not found", node);
+ return;
+ }
+ if (!bs->monitor_list.tqe_prev) {
+ error_setg(errp, "Node '%s' is not owned by the monitor",
+ bs->node_name);
+ return;
+ }
+ if (!bs->drv->is_filter) {
+ error_setg(errp, "Block format '%s' used by node '%s' does not support"
+ "insertion", bs->drv->format_name, bs->node_name);
+ return;
+ }
+
+ bdrv_insert_node(parent, child, node, errp);
+}
+
+void qmp_block_remove_node(const char *parent, const char *child,
+ const char *node, Error **errp)
+{
+ BlockDriverState *bs = bdrv_find_node(node);
+ if (!bs) {
+ error_setg(errp, "Node '%s' not found", node);
+ return;
+ }
+ if (!bs->monitor_list.tqe_prev) {
+ error_setg(errp, "Node %s is not owned by the monitor",
+ bs->node_name);
+ return;
+ }
+ if (!bs->drv->is_filter) {
+ error_setg(errp, "Block format '%s' used by node '%s' does not support"
+ "removal", bs->drv->format_name, bs->node_name);
+ return;
+ }
+
+ bdrv_remove_node(parent, child, node, errp);
+}
diff --git a/include/block/block.h b/include/block/block.h
index 744b50e734..9e1120af1b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -191,6 +191,8 @@ typedef enum BlockOpType {
BLOCK_OP_TYPE_RESIZE,
BLOCK_OP_TYPE_STREAM,
BLOCK_OP_TYPE_REPLACE,
+ BLOCK_OP_TYPE_EDGE_MODIFICATION, /* block user modification of graph edges
+ including this node */
BLOCK_OP_TYPE_MAX,
} BlockOpType;
@@ -627,4 +629,8 @@ void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error **errp);
bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
uint32_t granularity, Error **errp);
+void bdrv_insert_node(const char *parent, const char *child,
+ const char *node, Error **errp);
+void bdrv_remove_node(const char *parent, const char *child,
+ const char *node, Error **errp);
#endif
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4d6ba1baef..16e19cb648 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3947,3 +3947,63 @@
'data' : { 'parent': 'str',
'*child': 'str',
'*node': 'str' } }
+
+##
+# @block-insert-node:
+#
+# Insert a filter node between a specific edge in the block driver state graph.
+# @parent: the name of the parent node or device
+# @node: the name of the node to insert under parent
+# @child: the name of the child of both node and parent
+#
+# Example:
+# Insert and remove a throttle filter on top of a device chain, between the
+# device 'ide0-hd0' and node 'node-A'
+#
+# -> {'execute': 'object-add',
+# "arguments": {
+# "qom-type": "throttle-group",
+# "id": "group0",
+# "props" : { "limits": { "iops-total": 300 } }
+# }
+# }
+# <- { 'return': {} }
+# -> {'execute': 'blockdev-add',
+# 'arguments': {
+# 'driver': 'throttle',
+# 'node-name': 'throttle0',
+# 'throttle-group': 'group0',
+# 'file': 'node-A'
+# }
+# }
+# <- { 'return': {} }
+# -> { 'execute': 'block-insert-node',
+# 'arguments': { 'parent': 'ide0-hd0', 'child': 'node-A', 'node': 'throttle0' }
+# }
+# <- { 'return': {} }
+# -> { 'execute': 'block-remove-node',
+# 'arguments': { 'parent': 'ide0-hd0', 'child': 'node-A', 'node': 'throttle0' }
+# }
+# <- { 'return': {} }
+# -> { 'execute': 'blockdev-del',
+# 'arguments': { 'node-name': 'throttle0' }
+# }
+# <- { 'return': {} }
+#
+##
+{ 'command': 'block-insert-node',
+ 'data': { 'parent': 'str',
+ 'child': 'str',
+ 'node': 'str'} }
+##
+# @block-remove-node:
+#
+# Remove a filter node between two other nodes in the block driver state graph.
+# @parent: the name of the parent node or device
+# @node: the name of the node to remove from parent
+# @child: the name of the child of node which will go under parent
+##
+{ 'command': 'block-remove-node',
+ 'data': { 'parent': 'str',
+ 'child': 'str',
+ 'node': 'str'} }
diff --git a/tests/qemu-iotests/193 b/tests/qemu-iotests/193
new file mode 100755
index 0000000000..2936eba95e
--- /dev/null
+++ b/tests/qemu-iotests/193
@@ -0,0 +1,241 @@
+#!/bin/bash
+#
+# Test block-insert-node & block-remove-node commands
+#
+# Copyright (C) 2017 Manos Pitsidianakis
+#
+# 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="Manos Pitsidianakis"
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1 # failure is the default!
+
+_cleanup()
+{
+ _cleanup_test_img
+ rm -rf "$TEST_IMG".s
+ rm -rf "$TEST_IMG".e
+
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+ echo Testing: "$@" | _filter_imgfmt
+ $QEMU -nographic -qmp-pretty stdio -serial none "$@"
+ echo
+}
+
+function run_qemu()
+{
+ do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_qmp\
+ | _filter_qemu_io | _filter_generated_node_ids
+}
+
+_make_test_img 64M
+test_throttle=$($QEMU_IMG --help|grep throttle)
+[ "$test_throttle" = "" ] && _supported_fmt throttle
+
+echo
+echo "== inserting node =="
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+
+{ "execute": "blockdev-add",
+ "arguments": {
+ "node-name": "disk",
+ "driver": "$IMGFMT",
+ "file": {
+ "driver": "file",
+ "filename": "$TEST_IMG"
+ }
+ }
+}
+{ "execute": "device_add",
+ "arguments": { "driver": "virtio-blk", "drive": "disk",
+ "id": "virtio0" } }
+{
+ "execute": "blockdev-snapshot-sync",
+ "arguments": {
+ "node-name": "disk",
+ "snapshot-file": "$TEST_IMG.s",
+ "format": "$IMGFMT",
+ "snapshot-node-name": "node-B"
+ }
+}
+{ "execute": "blockdev-add",
+ "arguments": {
+ "node-name": "throttle0",
+ "driver": "throttle",
+ "file": "disk" }
+}
+
+{ "execute": "block-insert-node",
+ "arguments": {
+ "parent": "node-B",
+ "child": "disk",
+ "node": "throttle0" }
+}
+{ "execute": "query-block" }
+{ "execute": "query-named-block-nodes" }
+{ "execute": "quit" }
+EOF
+
+echo
+echo "== inserting and removing node =="
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+
+{ "execute": "blockdev-add",
+ "arguments": {
+ "node-name": "disk",
+ "driver": "$IMGFMT",
+ "file": {
+ "driver": "file",
+ "filename": "$TEST_IMG"
+ }
+ }
+}
+{"execute": "blockdev-add",
+"arguments": {
+"node-name": "throttle0", "driver": "throttle", "file": "disk", "throttle-group": "foo", "limits" : { "iops-total": 100 } } }
+{ "execute": "device_add",
+ "arguments": { "driver": "virtio-blk", "drive": "throttle0",
+ "id": "virtio0" } }
+{"execute": "blockdev-add",
+"arguments": {
+"node-name": "throttle1", "driver": "throttle", "file": "disk", "throttle-group": "bar", "limits" : { "iops-total": 200 } } }
+{"execute": "block-insert-node",
+ "arguments": {
+ "parent": "throttle0",
+ "child": "disk",
+ "node": "throttle1"}
+}
+{ "execute": "query-block" }
+{ "execute": "query-named-block-nodes" }
+{"execute": "block-remove-node",
+ "arguments": {
+ "parent": "throttle0",
+ "child": "disk",
+ "node": "throttle1"}
+}
+{"execute": "blockdev-del",
+ "arguments": { "node-name": "throttle1" }
+}
+{ "execute": "query-block" }
+{ "execute": "query-named-block-nodes" }
+{ "execute": "quit" }
+EOF
+
+echo
+echo "== inserting node below BB =="
+
+run_qemu -drive file=$TEST_IMG,format=$IMGFMT,if=none,id=disk,node-name=disk0 <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "blockdev-add",
+ "arguments": {
+ "node-name": "throttle0",
+ "driver": "throttle",
+ "file": "disk0" }
+}
+{ "execute": "block-insert-node",
+ "arguments": {
+ "parent": "disk",
+ "child": "disk0",
+ "node": "throttle0" }
+}
+{ "execute": "query-block" }
+{ "execute": "query-named-block-nodes" }
+{"execute": "block-remove-node",
+ "arguments": {
+ "parent": "disk",
+ "child": "disk0",
+ "node": "throttle0"}
+}
+{"execute": "blockdev-del",
+ "arguments": { "node-name": "throttle0" }
+}
+{ "execute": "query-block" }
+{ "execute": "query-named-block-nodes" }
+{ "execute": "quit" }
+EOF
+
+echo
+echo "== try insert with active block job =="
+
+# this should error because of the hidden mirror filter on top
+run_qemu -drive file=$TEST_IMG,format=$IMGFMT,if=none,id=disk,node-name=disk0 <<EOF
+{ "execute": "qmp_capabilities" }
+{
+ "execute": "blockdev-snapshot-sync",
+ "arguments": {
+ "node-name": "disk0",
+ "snapshot-file": "$TEST_IMG.s",
+ "format": "$IMGFMT",
+ "snapshot-node-name": "overlay0"
+ }
+}
+{
+ "execute": "drive-mirror",
+ "arguments": {
+ "device": "overlay0",
+ "job-id": "job0",
+ "target": "$TEST_IMG.e",
+ "sync": "full"
+ }
+}
+{
+ "execute": "query-block-jobs",
+ "arguments": {}
+}
+{ "execute": "blockdev-add",
+ "arguments": {
+ "node-name": "throttle0",
+ "driver": "throttle",
+ "file": "overlay0" }
+}
+{ "execute": "block-insert-node",
+ "arguments": {
+ "parent": "disk",
+ "child": "overlay0",
+ "node": "throttle0" }
+}
+{ "execute": "query-block" }
+{"execute": "blockdev-del",
+ "arguments": { "node-name": "throttle0" }
+}
+{ "execute": "quit" }
+EOF
+
+echo
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/193.out b/tests/qemu-iotests/193.out
new file mode 100644
index 0000000000..01a63c7ccb
--- /dev/null
+++ b/tests/qemu-iotests/193.out
@@ -0,0 +1,1116 @@
+QA output created by 192
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+
+== inserting node ==
+Testing:
+{
+ QMP_VERSION
+}
+{
+ "return": {
+ }
+}
+{
+ "return": {
+ }
+}
+{
+ "return": {
+ }
+}
+Formatting 'TEST_DIR/t.qcow2.s', fmt=qcow2 size=67108864 backing_file=TEST_DIR/t.qcow2 backing_fmt=qcow2 cluster_size=65536 lazy_refcounts=off refcount_bits=16
+{
+ "return": {
+ }
+}
+{
+ "return": {
+ }
+}
+{
+ "return": {
+ }
+}
+{
+ "return": [
+ {
+ "device": "",
+ "locked": false,
+ "removable": false,
+ "inserted": {
+ "iops_rd": 0,
+ "detect_zeroes": "off",
+ "image": {
+ "backing-image": {
+ "virtual-size": 67108864,
+ "filename": "TEST_DIR/t.qcow2",
+ "format": "throttle",
+ "actual-size": 200704
+ },
+ "backing-filename-format": "throttle",
+ "virtual-size": 67108864,
+ "filename": "TEST_DIR/t.qcow2.s",
+ "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",
+ "backing-filename": "TEST_DIR/t.qcow2",
+ "dirty-flag": false
+ },
+ "iops_wr": 0,
+ "ro": false,
+ "node-name": "node-B",
+ "backing_file_depth": 1,
+ "drv": "qcow2",
+ "iops": 0,
+ "bps_wr": 0,
+ "write_threshold": 0,
+ "backing_file": "TEST_DIR/t.qcow2",
+ "encrypted": false,
+ "bps": 0,
+ "bps_rd": 0,
+ "cache": {
+ "no-flush": false,
+ "direct": false,
+ "writeback": true
+ },
+ "file": "TEST_DIR/t.qcow2.s",
+ "encryption_key_missing": false
+ },
+ "qdev": "/machine/peripheral/virtio0/virtio-backend",
+ "type": "unknown"
+ }
+ ]
+}
+{
+ "return": [
+ {
+ "iops_rd": 0,
+ "detect_zeroes": "off",
+ "image": {
+ "virtual-size": 67108864,
+ "filename": "TEST_DIR/t.qcow2",
+ "format": "throttle",
+ "actual-size": 200704
+ },
+ "iops_wr": 0,
+ "ro": false,
+ "node-name": "throttle0",
+ "backing_file_depth": 0,
+ "drv": "throttle",
+ "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",
+ "format": "throttle",
+ "actual-size": 200704
+ },
+ "backing-filename-format": "throttle",
+ "virtual-size": 67108864,
+ "filename": "TEST_DIR/t.qcow2.s",
+ "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",
+ "backing-filename": "TEST_DIR/t.qcow2",
+ "dirty-flag": false
+ },
+ "iops_wr": 0,
+ "ro": false,
+ "node-name": "node-B",
+ "backing_file_depth": 1,
+ "drv": "qcow2",
+ "iops": 0,
+ "bps_wr": 0,
+ "write_threshold": 0,
+ "backing_file": "TEST_DIR/t.qcow2",
+ "encrypted": false,
+ "bps": 0,
+ "bps_rd": 0,
+ "cache": {
+ "no-flush": false,
+ "direct": false,
+ "writeback": true
+ },
+ "file": "TEST_DIR/t.qcow2.s",
+ "encryption_key_missing": false
+ },
+ {
+ "iops_rd": 0,
+ "detect_zeroes": "off",
+ "image": {
+ "virtual-size": 197120,
+ "filename": "TEST_DIR/t.qcow2.s",
+ "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.s",
+ "encryption_key_missing": false
+ },
+ {
+ "iops_rd": 0,
+ "detect_zeroes": "off",
+ "image": {
+ "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
+ }
+ },
+ "dirty-flag": false
+ },
+ "iops_wr": 0,
+ "ro": true,
+ "node-name": "disk",
+ "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",
+ "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": 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",
+ "encryption_key_missing": false
+ }
+ ]
+}
+{
+ "return": {
+ }
+}
+{
+ "timestamp": {
+ "seconds": TIMESTAMP,
+ "microseconds": TIMESTAMP
+ },
+ "event": "SHUTDOWN",
+ "data": {
+ "guest": false
+ }
+}
+
+
+== inserting and removing node ==
+Testing:
+{
+ QMP_VERSION
+}
+{
+ "return": {
+ }
+}
+{
+ "return": {
+ }
+}
+{
+ "return": {
+ }
+}
+{
+ "return": {
+ }
+}
+{
+ "return": {
+ }
+}
+{
+ "return": {
+ }
+}
+{
+ "return": [
+ {
+ "device": "",
+ "locked": false,
+ "removable": false,
+ "inserted": {
+ "iops_rd": 0,
+ "detect_zeroes": "off",
+ "image": {
+ "virtual-size": 67108864,
+ "filename": "json:{\"throttle-group\": \"foo\", \"driver\": \"throttle\", \"limits.iops-total\": 100, \"file\": {\"throttle-group\": \"bar\", \"driver\": \"throttle\", \"limits.iops-total\": 200, \"file\": {\"driver\": \"qcow2\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/t.qcow2\"}}}}",
+ "format": "throttle",
+ "actual-size": 200704
+ },
+ "iops_wr": 0,
+ "ro": false,
+ "node-name": "throttle0",
+ "backing_file_depth": 0,
+ "drv": "throttle",
+ "iops": 0,
+ "bps_wr": 0,
+ "write_threshold": 0,
+ "encrypted": false,
+ "bps": 0,
+ "bps_rd": 0,
+ "cache": {
+ "no-flush": false,
+ "direct": false,
+ "writeback": true
+ },
+ "file": "json:{\"throttle-group\": \"foo\", \"driver\": \"throttle\", \"limits.iops-total\": 100, \"file\": {\"throttle-group\": \"bar\", \"driver\": \"throttle\", \"limits.iops-total\": 200, \"file\": {\"driver\": \"qcow2\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/t.qcow2\"}}}}",
+ "encryption_key_missing": false
+ },
+ "qdev": "/machine/peripheral/virtio0/virtio-backend",
+ "type": "unknown"
+ }
+ ]
+}
+{
+ "return": [
+ {
+ "iops_rd": 0,
+ "detect_zeroes": "off",
+ "image": {
+ "virtual-size": 67108864,
+ "filename": "json:{\"throttle-group\": \"bar\", \"driver\": \"throttle\", \"limits.iops-total\": 200, \"file\": {\"driver\": \"qcow2\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/t.qcow2\"}}}",
+ "format": "throttle",
+ "actual-size": 200704
+ },
+ "iops_wr": 0,
+ "ro": false,
+ "node-name": "throttle1",
+ "backing_file_depth": 0,
+ "drv": "throttle",
+ "iops": 0,
+ "bps_wr": 0,
+ "write_threshold": 0,
+ "encrypted": false,
+ "bps": 0,
+ "bps_rd": 0,
+ "cache": {
+ "no-flush": false,
+ "direct": false,
+ "writeback": true
+ },
+ "file": "json:{\"throttle-group\": \"bar\", \"driver\": \"throttle\", \"limits.iops-total\": 200, \"file\": {\"driver\": \"qcow2\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/t.qcow2\"}}}",
+ "encryption_key_missing": false
+ },
+ {
+ "iops_rd": 0,
+ "detect_zeroes": "off",
+ "image": {
+ "virtual-size": 67108864,
+ "filename": "json:{\"throttle-group\": \"foo\", \"driver\": \"throttle\", \"limits.iops-total\": 100, \"file\": {\"throttle-group\": \"bar\", \"driver\": \"throttle\", \"limits.iops-total\": 200, \"file\": {\"driver\": \"qcow2\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/t.qcow2\"}}}}",
+ "format": "throttle",
+ "actual-size": 200704
+ },
+ "iops_wr": 0,
+ "ro": false,
+ "node-name": "throttle0",
+ "backing_file_depth": 0,
+ "drv": "throttle",
+ "iops": 0,
+ "bps_wr": 0,
+ "write_threshold": 0,
+ "encrypted": false,
+ "bps": 0,
+ "bps_rd": 0,
+ "cache": {
+ "no-flush": false,
+ "direct": false,
+ "writeback": true
+ },
+ "file": "json:{\"throttle-group\": \"foo\", \"driver\": \"throttle\", \"limits.iops-total\": 100, \"file\": {\"throttle-group\": \"bar\", \"driver\": \"throttle\", \"limits.iops-total\": 200, \"file\": {\"driver\": \"qcow2\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/t.qcow2\"}}}}",
+ "encryption_key_missing": false
+ },
+ {
+ "iops_rd": 0,
+ "detect_zeroes": "off",
+ "image": {
+ "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
+ }
+ },
+ "dirty-flag": false
+ },
+ "iops_wr": 0,
+ "ro": false,
+ "node-name": "disk",
+ "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",
+ "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": {
+ }
+}
+{
+ "return": {
+ }
+}
+{
+ "return": [
+ {
+ "device": "",
+ "locked": false,
+ "removable": false,
+ "inserted": {
+ "iops_rd": 0,
+ "detect_zeroes": "off",
+ "image": {
+ "virtual-size": 67108864,
+ "filename": "json:{\"throttle-group\": \"foo\", \"driver\": \"throttle\", \"limits.iops-total\": 100, \"file\": {\"driver\": \"qcow2\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/t.qcow2\"}}}",
+ "format": "throttle",
+ "actual-size": 200704
+ },
+ "iops_wr": 0,
+ "ro": false,
+ "node-name": "throttle0",
+ "backing_file_depth": 0,
+ "drv": "throttle",
+ "iops": 0,
+ "bps_wr": 0,
+ "write_threshold": 0,
+ "encrypted": false,
+ "bps": 0,
+ "bps_rd": 0,
+ "cache": {
+ "no-flush": false,
+ "direct": false,
+ "writeback": true
+ },
+ "file": "json:{\"throttle-group\": \"foo\", \"driver\": \"throttle\", \"limits.iops-total\": 100, \"file\": {\"driver\": \"qcow2\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/t.qcow2\"}}}",
+ "encryption_key_missing": false
+ },
+ "qdev": "/machine/peripheral/virtio0/virtio-backend",
+ "type": "unknown"
+ }
+ ]
+}
+{
+ "return": [
+ {
+ "iops_rd": 0,
+ "detect_zeroes": "off",
+ "image": {
+ "virtual-size": 67108864,
+ "filename": "json:{\"throttle-group\": \"foo\", \"driver\": \"throttle\", \"limits.iops-total\": 100, \"file\": {\"driver\": \"qcow2\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/t.qcow2\"}}}",
+ "format": "throttle",
+ "actual-size": 200704
+ },
+ "iops_wr": 0,
+ "ro": false,
+ "node-name": "throttle0",
+ "backing_file_depth": 0,
+ "drv": "throttle",
+ "iops": 0,
+ "bps_wr": 0,
+ "write_threshold": 0,
+ "encrypted": false,
+ "bps": 0,
+ "bps_rd": 0,
+ "cache": {
+ "no-flush": false,
+ "direct": false,
+ "writeback": true
+ },
+ "file": "json:{\"throttle-group\": \"foo\", \"driver\": \"throttle\", \"limits.iops-total\": 100, \"file\": {\"driver\": \"qcow2\", \"file\": {\"driver\": \"file\", \"filename\": \"TEST_DIR/t.qcow2\"}}}",
+ "encryption_key_missing": false
+ },
+ {
+ "iops_rd": 0,
+ "detect_zeroes": "off",
+ "image": {
+ "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
+ }
+ },
+ "dirty-flag": false
+ },
+ "iops_wr": 0,
+ "ro": false,
+ "node-name": "disk",
+ "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",
+ "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
+ }
+}
+
+
+== inserting node below BB ==
+Testing: -drive file=TEST_DIR/t.IMGFMT,format=IMGFMT,if=none,id=disk,node-name=disk0
+{
+ QMP_VERSION
+}
+{
+ "return": {
+ }
+}
+{
+ "return": {
+ }
+}
+{
+ "return": {
+ }
+}
+{
+ "return": [
+ {
+ "device": "disk",
+ "locked": false,
+ "removable": true,
+ "inserted": {
+ "iops_rd": 0,
+ "detect_zeroes": "off",
+ "image": {
+ "virtual-size": 67108864,
+ "filename": "TEST_DIR/t.qcow2",
+ "format": "throttle",
+ "actual-size": 200704
+ },
+ "iops_wr": 0,
+ "ro": false,
+ "node-name": "throttle0",
+ "backing_file_depth": 0,
+ "drv": "throttle",
+ "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
+ },
+ "type": "unknown"
+ }
+ ]
+}
+{
+ "return": [
+ {
+ "iops_rd": 0,
+ "detect_zeroes": "off",
+ "image": {
+ "virtual-size": 67108864,
+ "filename": "TEST_DIR/t.qcow2",
+ "format": "throttle",
+ "actual-size": 200704
+ },
+ "iops_wr": 0,
+ "ro": false,
+ "node-name": "throttle0",
+ "backing_file_depth": 0,
+ "drv": "throttle",
+ "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": {
+ "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
+ }
+ },
+ "dirty-flag": false
+ },
+ "iops_wr": 0,
+ "ro": false,
+ "node-name": "disk0",
+ "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",
+ "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": {
+ }
+}
+{
+ "return": {
+ }
+}
+{
+ "return": [
+ {
+ "device": "disk",
+ "locked": false,
+ "removable": true,
+ "inserted": {
+ "iops_rd": 0,
+ "detect_zeroes": "off",
+ "image": {
+ "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
+ }
+ },
+ "dirty-flag": false
+ },
+ "iops_wr": 0,
+ "ro": false,
+ "node-name": "disk0",
+ "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",
+ "encryption_key_missing": false
+ },
+ "type": "unknown"
+ }
+ ]
+}
+{
+ "return": [
+ {
+ "iops_rd": 0,
+ "detect_zeroes": "off",
+ "image": {
+ "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
+ }
+ },
+ "dirty-flag": false
+ },
+ "iops_wr": 0,
+ "ro": false,
+ "node-name": "disk0",
+ "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",
+ "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
+ }
+}
+
+
+== try insert with active block job ==
+Testing: -drive file=TEST_DIR/t.IMGFMT,format=IMGFMT,if=none,id=disk,node-name=disk0
+{
+ QMP_VERSION
+}
+{
+ "return": {
+ }
+}
+Formatting 'TEST_DIR/t.qcow2.s', fmt=qcow2 size=67108864 backing_file=TEST_DIR/t.qcow2 backing_fmt=qcow2 cluster_size=65536 lazy_refcounts=off refcount_bits=16
+{
+ "return": {
+ }
+}
+Formatting 'TEST_DIR/t.qcow2.e', fmt=qcow2 size=67108864 cluster_size=65536 lazy_refcounts=off refcount_bits=16
+{
+ "timestamp": {
+ "seconds": TIMESTAMP,
+ "microseconds": TIMESTAMP
+ },
+ "event": "BLOCK_JOB_READY",
+ "data": {
+ "device": "job0",
+ "len": 0,
+ "offset": 0,
+ "speed": 0,
+ "type": "mirror"
+ }
+}
+{
+ "return": {
+ }
+}
+{
+ "return": [
+ {
+ "io-status": "ok",
+ "device": "job0",
+ "busy": false,
+ "len": 0,
+ "offset": 0,
+ "paused": false,
+ "speed": 0,
+ "ready": true,
+ "type": "mirror"
+ }
+ ]
+}
+{
+ "return": {
+ }
+}
+{
+ "error": {
+ "class": "GenericError",
+ "desc": "'overlay0' not a child of device 'disk'"
+ }
+}
+{
+ "return": [
+ {
+ "device": "disk",
+ "locked": false,
+ "removable": true,
+ "inserted": {
+ "iops_rd": 0,
+ "detect_zeroes": "off",
+ "image": {
+ "backing-image": {
+ "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
+ }
+ },
+ "dirty-flag": false
+ },
+ "backing-filename-format": "qcow2",
+ "virtual-size": 67108864,
+ "filename": "TEST_DIR/t.qcow2.s",
+ "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",
+ "backing-filename": "TEST_DIR/t.qcow2",
+ "dirty-flag": false
+ },
+ "iops_wr": 0,
+ "ro": false,
+ "node-name": "overlay0",
+ "backing_file_depth": 1,
+ "drv": "qcow2",
+ "iops": 0,
+ "bps_wr": 0,
+ "write_threshold": 0,
+ "backing_file": "TEST_DIR/t.qcow2",
+ "encrypted": false,
+ "bps": 0,
+ "bps_rd": 0,
+ "cache": {
+ "no-flush": false,
+ "direct": false,
+ "writeback": true
+ },
+ "file": "TEST_DIR/t.qcow2.s",
+ "encryption_key_missing": false
+ },
+ "dirty-bitmaps": [
+ {
+ "status": "active",
+ "granularity": 65536,
+ "count": 0
+ }
+ ],
+ "type": "unknown"
+ }
+ ]
+}
+{
+ "return": {
+ }
+}
+{
+ "return": {
+ }
+}
+{
+ "timestamp": {
+ "seconds": TIMESTAMP,
+ "microseconds": TIMESTAMP
+ },
+ "event": "SHUTDOWN",
+ "data": {
+ "guest": false
+ }
+}
+{
+ "timestamp": {
+ "seconds": TIMESTAMP,
+ "microseconds": TIMESTAMP
+ },
+ "event": "BLOCK_JOB_COMPLETED",
+ "data": {
+ "device": "job0",
+ "len": 0,
+ "offset": 0,
+ "speed": 0,
+ "type": "mirror"
+ }
+}
+
+
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 1b4221f26d..9ad3d3d8cb 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -188,3 +188,4 @@
189 rw auto
190 rw auto quick
191 rw auto quick
+193 rw auto quick
--
2.11.0
On 08/15/2017 02:45 AM, Manos Pitsidianakis wrote:
> block-insert-node and its pair command block-remove-node provide runtime
> insertion and removal of filter nodes.
>
> block-insert-node takes a (parent, child) and (node, child) pair of
> edges and unrefs the (parent, child) BdrvChild relationship and creates
> a new (parent, node) child with the same BdrvChildRole.
>
> This is a different approach than x-blockdev-change which uses the driver
> methods bdrv_add_child() and bdrv_del_child(),
>
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
> block.c | 192 ++++++++
> blockdev.c | 44 ++
> include/block/block.h | 6 +
> qapi/block-core.json | 60 +++
> tests/qemu-iotests/193 | 241 ++++++++++
> tests/qemu-iotests/193.out | 1116 ++++++++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/group | 1 +
> 7 files changed, 1660 insertions(+)
> create mode 100755 tests/qemu-iotests/193
> create mode 100644 tests/qemu-iotests/193.out
You may want to look at using scripts/git.orderfile, to rearrange your
patch so that interface changes (.json, .h) occur before implementation
(.c). For now, I'm just focusing on the interface:
> +++ b/qapi/block-core.json
> @@ -3947,3 +3947,63 @@
> 'data' : { 'parent': 'str',
> '*child': 'str',
> '*node': 'str' } }
> +
> +##
> +# @block-insert-node:
> +#
> +# Insert a filter node between a specific edge in the block driver state graph.
> +# @parent: the name of the parent node or device
> +# @node: the name of the node to insert under parent
> +# @child: the name of the child of both node and parent
Is this always going to be between two existing nodes, or can this
command also be used to insert at the end of the chain (for example, if
parent or child is omitted)?
> +# }
> +# <- { 'return': {} }
> +#
> +##
Missing 'Since: 2.11'.
> +{ 'command': 'block-insert-node',
> + 'data': { 'parent': 'str',
> + 'child': 'str',
> + 'node': 'str'} }
For now, it looks like you require all arguments, and therefore this is
always insertion in the middle.
> +##
> +# @block-remove-node:
> +#
> +# Remove a filter node between two other nodes in the block driver state graph.
> +# @parent: the name of the parent node or device
> +# @node: the name of the node to remove from parent
> +# @child: the name of the child of node which will go under parent
> +##
> +{ 'command': 'block-remove-node',
> + 'data': { 'parent': 'str',
> + 'child': 'str',
> + 'node': 'str'} }
Likewise missing 2.11.
Overall I'm not seeing problems with the interface from the UI
perspective, but I have not been paying close attention to your larger
efforts on throttling nodes, so I hope other reviewers will chime in.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
On Tue, Aug 15, 2017 at 05:12:42PM -0500, Eric Blake wrote:
>On 08/15/2017 02:45 AM, Manos Pitsidianakis wrote:
>> block-insert-node and its pair command block-remove-node provide runtime
>> insertion and removal of filter nodes.
>>
>> block-insert-node takes a (parent, child) and (node, child) pair of
>> edges and unrefs the (parent, child) BdrvChild relationship and creates
>> a new (parent, node) child with the same BdrvChildRole.
>>
>> This is a different approach than x-blockdev-change which uses the driver
>> methods bdrv_add_child() and bdrv_del_child(),
>>
>> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
>> ---
>> block.c | 192 ++++++++
>> blockdev.c | 44 ++
>> include/block/block.h | 6 +
>> qapi/block-core.json | 60 +++
>> tests/qemu-iotests/193 | 241 ++++++++++
>> tests/qemu-iotests/193.out | 1116 ++++++++++++++++++++++++++++++++++++++++++++
>> tests/qemu-iotests/group | 1 +
>> 7 files changed, 1660 insertions(+)
>> create mode 100755 tests/qemu-iotests/193
>> create mode 100644 tests/qemu-iotests/193.out
>
>You may want to look at using scripts/git.orderfile, to rearrange your
>patch so that interface changes (.json, .h) occur before implementation
>(.c). For now, I'm just focusing on the interface:
Thanks for the tip, I will use it from now on!
>
>
>> +++ b/qapi/block-core.json
>> @@ -3947,3 +3947,63 @@
>> 'data' : { 'parent': 'str',
>> '*child': 'str',
>> '*node': 'str' } }
>> +
>> +##
>> +# @block-insert-node:
>> +#
>> +# Insert a filter node between a specific edge in the block driver state graph.
>> +# @parent: the name of the parent node or device
>> +# @node: the name of the node to insert under parent
>> +# @child: the name of the child of both node and parent
>
>Is this always going to be between two existing nodes, or can this
>command also be used to insert at the end of the chain (for example, if
>parent or child is omitted)?
If this is used for filter nodes, I suppose only between would make
sense (for now). Is there a use case for the latter?
>
>
>> +# }
>> +# <- { 'return': {} }
>> +#
>> +##
>
>Missing 'Since: 2.11'.
>
>> +{ 'command': 'block-insert-node',
>> + 'data': { 'parent': 'str',
>> + 'child': 'str',
>> + 'node': 'str'} }
>
>For now, it looks like you require all arguments, and therefore this is
>always insertion in the middle.
>
>> +##
>> +# @block-remove-node:
>> +#
>> +# Remove a filter node between two other nodes in the block driver state graph.
>> +# @parent: the name of the parent node or device
>> +# @node: the name of the node to remove from parent
>> +# @child: the name of the child of node which will go under parent
>> +##
>> +{ 'command': 'block-remove-node',
>> + 'data': { 'parent': 'str',
>> + 'child': 'str',
>> + 'node': 'str'} }
>
>Likewise missing 2.11.
>
>Overall I'm not seeing problems with the interface from the UI
>perspective, but I have not been paying close attention to your larger
>efforts on throttling nodes, so I hope other reviewers will chime in.
>
>--
>Eric Blake, Principal Software Engineer
>Red Hat, Inc. +1-919-301-3266
>Virtualization: qemu.org | libvirt.org
>
On 08/16/2017 04:41 AM, Manos Pitsidianakis wrote: >>> +## >>> +# @block-insert-node: >>> +# >>> +# Insert a filter node between a specific edge in the block driver >>> state graph. >>> +# @parent: the name of the parent node or device >>> +# @node: the name of the node to insert under parent >>> +# @child: the name of the child of both node and parent >> >> Is this always going to be between two existing nodes, or can this >> command also be used to insert at the end of the chain (for example, if >> parent or child is omitted)? > > If this is used for filter nodes, I suppose only between would make > sense (for now). Is there a use case for the latter? Perhaps. Given a qcow2 image backing chain: base <- active there are four BDS (2 format, 2 protocol). Ideally, I could add filtering to any one of those four nodes (a filter on the base protocol level restricts how much guest data can be used from the backing image, but with no limits on the qcow2 metadata; a filter on the base format level restricts metadata reads as well; similarly for filters on the active protocol and format layers). But adding a filter on 'active' at the format level has no pre-existing parent (I'm adding the filter as the new top-level). Or am I missing something? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Wed, Aug 16, 2017 at 06:59:25AM -0500, Eric Blake wrote: >On 08/16/2017 04:41 AM, Manos Pitsidianakis wrote: > >>>> +## >>>> +# @block-insert-node: >>>> +# >>>> +# Insert a filter node between a specific edge in the block driver >>>> state graph. >>>> +# @parent: the name of the parent node or device >>>> +# @node: the name of the node to insert under parent >>>> +# @child: the name of the child of both node and parent >>> >>> Is this always going to be between two existing nodes, or can this >>> command also be used to insert at the end of the chain (for example, if >>> parent or child is omitted)? >> >> If this is used for filter nodes, I suppose only between would make >> sense (for now). Is there a use case for the latter? > >Perhaps. > >Given a qcow2 image backing chain: > >base <- active > >there are four BDS (2 format, 2 protocol). Ideally, I could add >filtering to any one of those four nodes (a filter on the base protocol >level restricts how much guest data can be used from the backing image, >but with no limits on the qcow2 metadata; a filter on the base format >level restricts metadata reads as well; similarly for filters on the >active protocol and format layers). > >But adding a filter on 'active' at the format level has no pre-existing >parent (I'm adding the filter as the new top-level). Or am I missing >something? The parent in this case is the storage device (disk / cdrom), whose name is specified as the parent. The first example in the qapi/block-core.json is such a case. In code I check blk_by_name(parent) and if that doesn't exist, I try with bdrv_find_node(parent). Perhaps I should reword the documentation or did I misunderstand what you wrote?
On 08/16/2017 07:11 AM, Manos Pitsidianakis wrote: >> >> Given a qcow2 image backing chain: >> >> base <- active >> >> there are four BDS (2 format, 2 protocol). Ideally, I could add >> filtering to any one of those four nodes (a filter on the base protocol >> level restricts how much guest data can be used from the backing image, >> but with no limits on the qcow2 metadata; a filter on the base format >> level restricts metadata reads as well; similarly for filters on the >> active protocol and format layers). >> >> But adding a filter on 'active' at the format level has no pre-existing >> parent (I'm adding the filter as the new top-level). Or am I missing >> something? > > The parent in this case is the storage device (disk / cdrom), whose name > is specified as the parent. The first example in the > qapi/block-core.json is such a case. In code I check blk_by_name(parent) > and if that doesn't exist, I try with bdrv_find_node(parent). Perhaps I > should reword the documentation or did I misunderstand what you wrote? Ah, so you are including both BB and BDS in your set of names for parent objects. Yes, clarifying the documentation that the parent can be either a device name (for the top of the chain) or a node name (for the middle of the chain) would help; the child name is always a node name (since we are filtering how the child will be used when accessed from the parent). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Am 15.08.2017 um 09:45 hat Manos Pitsidianakis geschrieben:
> block-insert-node and its pair command block-remove-node provide runtime
> insertion and removal of filter nodes.
>
> block-insert-node takes a (parent, child) and (node, child) pair of
> edges and unrefs the (parent, child) BdrvChild relationship and creates
> a new (parent, node) child with the same BdrvChildRole.
>
> This is a different approach than x-blockdev-change which uses the driver
> methods bdrv_add_child() and bdrv_del_child(),
>
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 4d6ba1baef..16e19cb648 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3947,3 +3947,63 @@
> 'data' : { 'parent': 'str',
> '*child': 'str',
> '*node': 'str' } }
> +
> +##
> +# @block-insert-node:
> +#
> +# Insert a filter node between a specific edge in the block driver state graph.
> +# @parent: the name of the parent node or device
> +# @node: the name of the node to insert under parent
> +# @child: the name of the child of both node and parent
> +#
> +# Example:
> +# Insert and remove a throttle filter on top of a device chain, between the
> +# device 'ide0-hd0' and node 'node-A'
> +#
> +# -> {'execute': 'object-add',
> +# "arguments": {
> +# "qom-type": "throttle-group",
> +# "id": "group0",
> +# "props" : { "limits": { "iops-total": 300 } }
> +# }
> +# }
> +# <- { 'return': {} }
> +# -> {'execute': 'blockdev-add',
> +# 'arguments': {
> +# 'driver': 'throttle',
> +# 'node-name': 'throttle0',
> +# 'throttle-group': 'group0',
> +# 'file': 'node-A'
> +# }
> +# }
> +# <- { 'return': {} }
> +# -> { 'execute': 'block-insert-node',
> +# 'arguments': { 'parent': 'ide0-hd0', 'child': 'node-A', 'node': 'throttle0' }
> +# }
> +# <- { 'return': {} }
> +# -> { 'execute': 'block-remove-node',
> +# 'arguments': { 'parent': 'ide0-hd0', 'child': 'node-A', 'node': 'throttle0' }
> +# }
> +# <- { 'return': {} }
> +# -> { 'execute': 'blockdev-del',
> +# 'arguments': { 'node-name': 'throttle0' }
> +# }
> +# <- { 'return': {} }
> +#
> +##
> +{ 'command': 'block-insert-node',
> + 'data': { 'parent': 'str',
> + 'child': 'str',
> + 'node': 'str'} }
I would suggest a change to the meaning of @child: Instead of using the
node-name of the child BDS, I would use the name of the BdrvChild that
represents the link.
The reason for this is that the node-name could be ambiguous, if you
have two edges between the same two nodes.
The only use of the node-name of the child that I can remember was for
checking that the graph still looks like what the user expects. But I
think we came to the conclusion that there are no race conditions to
check for if we have manual block job deletion instead of automatic
completion which can involve surprise changes to the graph. So probably
we don't need the node-name even for this.
> +##
> +# @block-remove-node:
> +#
> +# Remove a filter node between two other nodes in the block driver state graph.
> +# @parent: the name of the parent node or device
> +# @node: the name of the node to remove from parent
> +# @child: the name of the child of node which will go under parent
> +##
> +{ 'command': 'block-remove-node',
> + 'data': { 'parent': 'str',
> + 'child': 'str',
> + 'node': 'str'} }
Same thing here.
> diff --git a/block.c b/block.c
> index 81bd51b670..f874aabbfb 100644
> --- a/block.c
> +++ b/block.c
> + /* insert 'node' as child bs of 'parent' node */
> + if (check_node_edge(parent, child, errp)) {
> + return;
> + }
> + parent_bs = bdrv_find_node(parent);
> + c = bdrv_find_child(parent_bs, child);
> + role = c->role;
> + assert(role == &child_file || role == &child_backing);
> +
> + bdrv_ref(node_bs);
> +
> + bdrv_drained_begin(parent_bs);
> + bdrv_unref_child(parent_bs, c);
> + if (role == &child_file) {
> + parent_bs->file = bdrv_attach_child(parent_bs, node_bs, "file",
> + &child_file, errp);
> + if (!parent_bs->file) {
> + parent_bs->file = bdrv_attach_child(parent_bs, child_bs, "file",
> + &child_file, &error_abort);
> + goto out;
> + }
> + } else if (role == &child_backing) {
> + parent_bs->backing = bdrv_attach_child(parent_bs, node_bs, "backing",
> + &child_backing, errp);
> + if (!parent_bs->backing) {
> + parent_bs->backing = bdrv_attach_child(parent_bs, child_bs,
> + "backing", &child_backing,
> + &error_abort);
> + goto out;
> + }
> + }
I would prefer if we could find a solution to avoid requiring a specific
role. I'm not even sure that your assertion above is correct; can you
explain why c couldn't have any other role?
Instead of bdrv_unref_child/bdrv_attach_child, could we just change
where the child points to using bdrv_replace_child()? Then
parent_bs->file and parent_bs->backing (or whatever other variable
contains the BdrvChild pointer) can stay unchanged and just keep
working.
> + bdrv_refresh_filename(parent_bs);
> + bdrv_refresh_limits(parent_bs, NULL);
> +
> +out:
> + bdrv_drained_end(parent_bs);
> +}
> diff --git a/blockdev.c b/blockdev.c
> index 8e2fc6e64c..5195ec1b61 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -4238,3 +4238,47 @@ QemuOptsList qemu_drive_opts = {
> { /* end of list */ }
> },
> };
> +
> +void qmp_block_insert_node(const char *parent, const char *child,
> + const char *node, Error **errp)
> +{
> + BlockDriverState *bs = bdrv_find_node(node);
> + if (!bs) {
> + error_setg(errp, "Node '%s' not found", node);
> + return;
> + }
> + if (!bs->monitor_list.tqe_prev) {
> + error_setg(errp, "Node '%s' is not owned by the monitor",
> + bs->node_name);
> + return;
> + }
> + if (!bs->drv->is_filter) {
> + error_setg(errp, "Block format '%s' used by node '%s' does not support"
> + "insertion", bs->drv->format_name, bs->node_name);
> + return;
> + }
> +
> + bdrv_insert_node(parent, child, node, errp);
> +}
Do we need to acquire an AioContext lock somewhere?
Kevin
On Fri, Sep 29, 2017 at 07:52:35PM +0200, Kevin Wolf wrote:
>Am 15.08.2017 um 09:45 hat Manos Pitsidianakis geschrieben:
>> block-insert-node and its pair command block-remove-node provide runtime
>> insertion and removal of filter nodes.
>>
>> block-insert-node takes a (parent, child) and (node, child) pair of
>> edges and unrefs the (parent, child) BdrvChild relationship and creates
>> a new (parent, node) child with the same BdrvChildRole.
>>
>> This is a different approach than x-blockdev-change which uses the driver
>> methods bdrv_add_child() and bdrv_del_child(),
>>
>> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 4d6ba1baef..16e19cb648 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -3947,3 +3947,63 @@
>> 'data' : { 'parent': 'str',
>> '*child': 'str',
>> '*node': 'str' } }
>> +
>> +##
>> +# @block-insert-node:
>> +#
>> +# Insert a filter node between a specific edge in the block driver state graph.
>> +# @parent: the name of the parent node or device
>> +# @node: the name of the node to insert under parent
>> +# @child: the name of the child of both node and parent
>> +#
>> +# Example:
>> +# Insert and remove a throttle filter on top of a device chain, between the
>> +# device 'ide0-hd0' and node 'node-A'
>> +#
>> +# -> {'execute': 'object-add',
>> +# "arguments": {
>> +# "qom-type": "throttle-group",
>> +# "id": "group0",
>> +# "props" : { "limits": { "iops-total": 300 } }
>> +# }
>> +# }
>> +# <- { 'return': {} }
>> +# -> {'execute': 'blockdev-add',
>> +# 'arguments': {
>> +# 'driver': 'throttle',
>> +# 'node-name': 'throttle0',
>> +# 'throttle-group': 'group0',
>> +# 'file': 'node-A'
>> +# }
>> +# }
>> +# <- { 'return': {} }
>> +# -> { 'execute': 'block-insert-node',
>> +# 'arguments': { 'parent': 'ide0-hd0', 'child': 'node-A', 'node': 'throttle0' }
>> +# }
>> +# <- { 'return': {} }
>> +# -> { 'execute': 'block-remove-node',
>> +# 'arguments': { 'parent': 'ide0-hd0', 'child': 'node-A', 'node': 'throttle0' }
>> +# }
>> +# <- { 'return': {} }
>> +# -> { 'execute': 'blockdev-del',
>> +# 'arguments': { 'node-name': 'throttle0' }
>> +# }
>> +# <- { 'return': {} }
>> +#
>> +##
>> +{ 'command': 'block-insert-node',
>> + 'data': { 'parent': 'str',
>> + 'child': 'str',
>> + 'node': 'str'} }
>
>I would suggest a change to the meaning of @child: Instead of using the
>node-name of the child BDS, I would use the name of the BdrvChild that
>represents the link.
>
>The reason for this is that the node-name could be ambiguous, if you
>have two edges between the same two nodes.
>
>The only use of the node-name of the child that I can remember was for
>checking that the graph still looks like what the user expects. But I
>think we came to the conclusion that there are no race conditions to
>check for if we have manual block job deletion instead of automatic
>completion which can involve surprise changes to the graph. So probably
>we don't need the node-name even for this.
>
>> +##
>> +# @block-remove-node:
>> +#
>> +# Remove a filter node between two other nodes in the block driver state graph.
>> +# @parent: the name of the parent node or device
>> +# @node: the name of the node to remove from parent
>> +# @child: the name of the child of node which will go under parent
>> +##
>> +{ 'command': 'block-remove-node',
>> + 'data': { 'parent': 'str',
>> + 'child': 'str',
>> + 'node': 'str'} }
>
>Same thing here.
>
>> diff --git a/block.c b/block.c
>> index 81bd51b670..f874aabbfb 100644
>> --- a/block.c
>> +++ b/block.c
>> + /* insert 'node' as child bs of 'parent' node */
>> + if (check_node_edge(parent, child, errp)) {
>> + return;
>> + }
>> + parent_bs = bdrv_find_node(parent);
>> + c = bdrv_find_child(parent_bs, child);
>> + role = c->role;
>> + assert(role == &child_file || role == &child_backing);
>> +
>> + bdrv_ref(node_bs);
>> +
>> + bdrv_drained_begin(parent_bs);
>> + bdrv_unref_child(parent_bs, c);
>> + if (role == &child_file) {
>> + parent_bs->file = bdrv_attach_child(parent_bs, node_bs, "file",
>> + &child_file, errp);
>> + if (!parent_bs->file) {
>> + parent_bs->file = bdrv_attach_child(parent_bs, child_bs, "file",
>> + &child_file, &error_abort);
>> + goto out;
>> + }
>> + } else if (role == &child_backing) {
>> + parent_bs->backing = bdrv_attach_child(parent_bs, node_bs, "backing",
>> + &child_backing, errp);
>> + if (!parent_bs->backing) {
>> + parent_bs->backing = bdrv_attach_child(parent_bs, child_bs,
>> + "backing", &child_backing,
>> + &error_abort);
>> + goto out;
>> + }
>> + }
>
>I would prefer if we could find a solution to avoid requiring a specific
>role. I'm not even sure that your assertion above is correct; can you
>explain why c couldn't have any other role?
>
>Instead of bdrv_unref_child/bdrv_attach_child, could we just change
>where the child points to using bdrv_replace_child()? Then
bdrv_replace_child() uses bdrv_set_perm() and co. When I tried it at
first I got errors like "Conflicts with use by ****** as 'backing',
which does not allow 'write' on disk". Presumably the permissions do not
need to change but can we do bdrv_set_perm without bdrv_check_perm?
>parent_bs->file and parent_bs->backing (or whatever other variable
>contains the BdrvChild pointer) can stay unchanged and just keep
>working.
>
>> + bdrv_refresh_filename(parent_bs);
>> + bdrv_refresh_limits(parent_bs, NULL);
>> +
>> +out:
>> + bdrv_drained_end(parent_bs);
>> +}
>
>> diff --git a/blockdev.c b/blockdev.c
>> index 8e2fc6e64c..5195ec1b61 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -4238,3 +4238,47 @@ QemuOptsList qemu_drive_opts = {
>> { /* end of list */ }
>> },
>> };
>> +
>> +void qmp_block_insert_node(const char *parent, const char *child,
>> + const char *node, Error **errp)
>> +{
>> + BlockDriverState *bs = bdrv_find_node(node);
>> + if (!bs) {
>> + error_setg(errp, "Node '%s' not found", node);
>> + return;
>> + }
>> + if (!bs->monitor_list.tqe_prev) {
>> + error_setg(errp, "Node '%s' is not owned by the monitor",
>> + bs->node_name);
>> + return;
>> + }
>> + if (!bs->drv->is_filter) {
>> + error_setg(errp, "Block format '%s' used by node '%s' does not support"
>> + "insertion", bs->drv->format_name, bs->node_name);
>> + return;
>> + }
>> +
>> + bdrv_insert_node(parent, child, node, errp);
>> +}
>
>Do we need to acquire an AioContext lock somewhere?
>
>Kevin
>
the *_child() functions call drained_begin/end which I think might cover
this case?
Am 04.10.2017 um 14:23 hat Manos Pitsidianakis geschrieben:
> > > diff --git a/block.c b/block.c
> > > index 81bd51b670..f874aabbfb 100644
> > > --- a/block.c
> > > +++ b/block.c
> > > + /* insert 'node' as child bs of 'parent' node */
> > > + if (check_node_edge(parent, child, errp)) {
> > > + return;
> > > + }
> > > + parent_bs = bdrv_find_node(parent);
> > > + c = bdrv_find_child(parent_bs, child);
> > > + role = c->role;
> > > + assert(role == &child_file || role == &child_backing);
> > > +
> > > + bdrv_ref(node_bs);
> > > +
> > > + bdrv_drained_begin(parent_bs);
> > > + bdrv_unref_child(parent_bs, c);
> > > + if (role == &child_file) {
> > > + parent_bs->file = bdrv_attach_child(parent_bs, node_bs, "file",
> > > + &child_file, errp);
> > > + if (!parent_bs->file) {
> > > + parent_bs->file = bdrv_attach_child(parent_bs, child_bs, "file",
> > > + &child_file, &error_abort);
> > > + goto out;
> > > + }
> > > + } else if (role == &child_backing) {
> > > + parent_bs->backing = bdrv_attach_child(parent_bs, node_bs, "backing",
> > > + &child_backing, errp);
> > > + if (!parent_bs->backing) {
> > > + parent_bs->backing = bdrv_attach_child(parent_bs, child_bs,
> > > + "backing", &child_backing,
> > > + &error_abort);
> > > + goto out;
> > > + }
> > > + }
> >
> > I would prefer if we could find a solution to avoid requiring a specific
> > role. I'm not even sure that your assertion above is correct; can you
> > explain why c couldn't have any other role?
> >
> > Instead of bdrv_unref_child/bdrv_attach_child, could we just change
> > where the child points to using bdrv_replace_child()? Then
>
> bdrv_replace_child() uses bdrv_set_perm() and co. When I tried it at first I
> got errors like "Conflicts with use by ****** as 'backing', which does not
> allow 'write' on disk". Presumably the permissions do not need to change but
> can we do bdrv_set_perm without bdrv_check_perm?
Which child is conflicting with which other child? Is c conflicting with
itself or something?
If unref_child/attach_child works without any other action in between,
there is no reason why replace_child shouldn't work, too. Maybe this is
a bug in bdrv_
> > parent_bs->file and parent_bs->backing (or whatever other variable
> > contains the BdrvChild pointer) can stay unchanged and just keep
> > working.
> >
> > > + bdrv_refresh_filename(parent_bs);
> > > + bdrv_refresh_limits(parent_bs, NULL);
> > > +
> > > +out:
> > > + bdrv_drained_end(parent_bs);
> > > +}
> >
> > > diff --git a/blockdev.c b/blockdev.c
> > > index 8e2fc6e64c..5195ec1b61 100644
> > > --- a/blockdev.c
> > > +++ b/blockdev.c
> > > @@ -4238,3 +4238,47 @@ QemuOptsList qemu_drive_opts = {
> > > { /* end of list */ }
> > > },
> > > };
> > > +
> > > +void qmp_block_insert_node(const char *parent, const char *child,
> > > + const char *node, Error **errp)
> > > +{
> > > + BlockDriverState *bs = bdrv_find_node(node);
> > > + if (!bs) {
> > > + error_setg(errp, "Node '%s' not found", node);
> > > + return;
> > > + }
> > > + if (!bs->monitor_list.tqe_prev) {
> > > + error_setg(errp, "Node '%s' is not owned by the monitor",
> > > + bs->node_name);
> > > + return;
> > > + }
> > > + if (!bs->drv->is_filter) {
> > > + error_setg(errp, "Block format '%s' used by node '%s' does not support"
> > > + "insertion", bs->drv->format_name, bs->node_name);
> > > + return;
> > > + }
> > > +
> > > + bdrv_insert_node(parent, child, node, errp);
> > > +}
> >
> > Do we need to acquire an AioContext lock somewhere?
>
> the *_child() functions call drained_begin/end which I think might cover
> this case?
I don't think it's enough when you don't own the AioContext lock.
Kevin
On 2017-08-15 09:45, Manos Pitsidianakis wrote:
> block-insert-node and its pair command block-remove-node provide runtime
> insertion and removal of filter nodes.
>
> block-insert-node takes a (parent, child) and (node, child) pair of
> edges and unrefs the (parent, child) BdrvChild relationship and creates
> a new (parent, node) child with the same BdrvChildRole.
>
> This is a different approach than x-blockdev-change which uses the driver
> methods bdrv_add_child() and bdrv_del_child(),
Why? :-)
Can't we reuse x-blockdev-change? As far as I'm concerned, this was one
of its roles, and at least I don't want to have both x-blockdev-change
and these new commands.
I think it would be a good idea just to change bdrv_add_child() and
bdrv_del_child(): If the driver has a bdrv_{add,del}_child() callback,
invoke that. If it doesn't, then just attach the child.
Of course, it may turn out that x-blockdev-change is lacking something
(e.g. a way to specify as what kind of child a new node is to be
attached). Then we should either extend it so that it covers what it's
lacking, or replace it completely by these new commands. In the latter
case, however, they would need to cover the existing use case which is
reconfiguring the quorum driver. (And that would mean it would have to
invoke bdrv_add_child()/bdrv_del_child() when the driver has them.)
Max
On Wed, Oct 04, 2017 at 02:49:27PM +0200, Max Reitz wrote:
>On 2017-08-15 09:45, Manos Pitsidianakis wrote:
>> block-insert-node and its pair command block-remove-node provide runtime
>> insertion and removal of filter nodes.
>>
>> block-insert-node takes a (parent, child) and (node, child) pair of
>> edges and unrefs the (parent, child) BdrvChild relationship and
>> creates
>> a new (parent, node) child with the same BdrvChildRole.
>>
>> This is a different approach than x-blockdev-change which uses the driver
>> methods bdrv_add_child() and bdrv_del_child(),
>
>Why? :-)
>
>Can't we reuse x-blockdev-change? As far as I'm concerned, this was one
>of its roles, and at least I don't want to have both x-blockdev-change
>and these new commands.
>
>I think it would be a good idea just to change bdrv_add_child() and
>bdrv_del_child(): If the driver has a bdrv_{add,del}_child() callback,
>invoke that. If it doesn't, then just attach the child.
>
>Of course, it may turn out that x-blockdev-change is lacking something
>(e.g. a way to specify as what kind of child a new node is to be
>attached). Then we should either extend it so that it covers what it's
>lacking, or replace it completely by these new commands. In the latter
>case, however, they would need to cover the existing use case which is
>reconfiguring the quorum driver. (And that would mean it would have to
>invoke bdrv_add_child()/bdrv_del_child() when the driver has them.)
>
>Max
>
I think the two use cases are this:
a) Adding/removing a replication child to an existing quorum node
b) Insert a filter between two existing nodes.
These are not directly compatible semantically, but as you said
x-blockdev-change can perform b) if bdrv_add_child()/bdrv_del_child()
are not implemented. Wouldn't that be unnecessary overloading?
On 2017-10-04 19:05, Manos Pitsidianakis wrote:
> On Wed, Oct 04, 2017 at 02:49:27PM +0200, Max Reitz wrote:
>> On 2017-08-15 09:45, Manos Pitsidianakis wrote:
>>> block-insert-node and its pair command block-remove-node provide runtime
>>> insertion and removal of filter nodes.
>>>
>>> block-insert-node takes a (parent, child) and (node, child) pair of
>>> edges and unrefs the (parent, child) BdrvChild relationship and creates
>>> a new (parent, node) child with the same BdrvChildRole.
>>>
>>> This is a different approach than x-blockdev-change which uses the
>>> driver
>>> methods bdrv_add_child() and bdrv_del_child(),
>>
>> Why? :-)
>>
>> Can't we reuse x-blockdev-change? As far as I'm concerned, this was one
>> of its roles, and at least I don't want to have both x-blockdev-change
>> and these new commands.
>>
>> I think it would be a good idea just to change bdrv_add_child() and
>> bdrv_del_child(): If the driver has a bdrv_{add,del}_child() callback,
>> invoke that. If it doesn't, then just attach the child.
>>
>> Of course, it may turn out that x-blockdev-change is lacking something
>> (e.g. a way to specify as what kind of child a new node is to be
>> attached). Then we should either extend it so that it covers what it's
>> lacking, or replace it completely by these new commands. In the latter
>> case, however, they would need to cover the existing use case which is
>> reconfiguring the quorum driver. (And that would mean it would have to
>> invoke bdrv_add_child()/bdrv_del_child() when the driver has them.)
>>
>> Max
>>
>
> I think the two use cases are this:
>
> a) Adding/removing a replication child to an existing quorum node
> b) Insert a filter between two existing nodes.
For me both are the same: Adding/removing nodes into/from the graph.
The difference is how those children are added (or removed, but it's the
same in reverse): In case of quorum, you can simply attach a new child
because it supports a variable number of children. Another case where
this would work are all block drivers which support backing files (you
can freely attach/detach those).
In this series, it's not about adding or removing new children, but
instead "injecting" them into an edge: An existing child is replaced,
but it now serves as some child of the new one.
(I guess writing too much trying to get my point across, sorry...)
The gist is that for me it's not so much about "quorum" or "filter
nodes". It's about adding a new child to a node vs. replacing an
existing one.
> These are not directly compatible semantically, but as you said
> x-blockdev-change can perform b) if bdrv_add_child()/bdrv_del_child()
> are not implemented. Wouldn't that be unnecessary overloading?
Yes, I think it would be. :-)
So say we have these two trees in our graph:
[ Parent BDS -> Child BDS ]
[ Filter BDS -> Child BDS ]
So here's what you can do with x-blockdev-change (in theory; in practice
it can only do that for quorum):
- Remove a child, so
[ Parent BDS -> Child BDS ]
is split into two trees
[ Parent BDS ] and
[ Child BDS ]
- Add a child, so we can merge
[ Parent BDS ] and
[ Filter BDS -> Child BDS ]
into
[ Parent BDS -> Filter BDS -> Child BDS ]
However, this is only possible with quorum because usually block drivers
don't support detaching children.
And here's what you can do with your commands (from what I can see):
- Replace a child (you call it insertion, but it really is just
replacement of an existing child with the constraint that both nodes
involved must have the same child):
[ Parent BDS -> Child BDS ]
[ Filter BDS -> Child BDS ]
to
[ Parent BDS -> Filter BDS -> Child BDS ]
- Replace a child (you call it removal, but it really is just
replacement of a child with its child):
[ Parent BDS -> Filter BDS -> Child BDS ]
to
[ Parent BDS -> Child BDS ]
This works on all BDSs because you don't change the number of children.
The interesting thing of course is that the "change" command can
actually add and remove children; where as the "insert" and "remove"
commands can only replace children. So that's already a bit funny (one
command does two things; two commands do one thing).
And then of course you can simply modify x-blockdev-change so it can do
the same thing block-insert-node and block-remove-node can do: It just
needs another mode which can be used to replace a child (and its
description already states that it is supposed to be usable for that at
some point in the future).
So after I've written all of this, I feel like the new insert-node and
remove-node commands are exactly what x-blockdev-change should do when
asked to replace a node. The only difference is that x-blockdev-change
would allow you to replace any node with anything, without the
constraints that block-insert-node and block-remove-node exact.
(And node replacement with x-blockdev-change would work by specifying
all three parameters.)
Not sure if that makes sense, I hope it does. :-)
Max
On Wed, Oct 04, 2017 at 08:09:24PM +0200, Max Reitz wrote:
>On 2017-10-04 19:05, Manos Pitsidianakis wrote:
>> On Wed, Oct 04, 2017 at 02:49:27PM +0200, Max Reitz wrote:
>>> On 2017-08-15 09:45, Manos Pitsidianakis wrote:
>>>> block-insert-node and its pair command block-remove-node provide runtime
>>>> insertion and removal of filter nodes.
>>>>
>>>> block-insert-node takes a (parent, child) and (node, child) pair of
>>>> edges and unrefs the (parent, child) BdrvChild relationship and creates
>>>> a new (parent, node) child with the same BdrvChildRole.
>>>>
>>>> This is a different approach than x-blockdev-change which uses the
>>>> driver
>>>> methods bdrv_add_child() and bdrv_del_child(),
>>>
>>> Why? :-)
>>>
>>> Can't we reuse x-blockdev-change? As far as I'm concerned, this was one
>>> of its roles, and at least I don't want to have both x-blockdev-change
>>> and these new commands.
>>>
>>> I think it would be a good idea just to change bdrv_add_child() and
>>> bdrv_del_child(): If the driver has a bdrv_{add,del}_child() callback,
>>> invoke that. If it doesn't, then just attach the child.
>>>
>>> Of course, it may turn out that x-blockdev-change is lacking something
>>> (e.g. a way to specify as what kind of child a new node is to be
>>> attached). Then we should either extend it so that it covers what it's
>>> lacking, or replace it completely by these new commands. In the latter
>>> case, however, they would need to cover the existing use case which is
>>> reconfiguring the quorum driver. (And that would mean it would have to
>>> invoke bdrv_add_child()/bdrv_del_child() when the driver has them.)
>>>
>>> Max
>>>
>>
>> I think the two use cases are this:
>>
>> a) Adding/removing a replication child to an existing quorum node
>> b) Insert a filter between two existing nodes.
>
>For me both are the same: Adding/removing nodes into/from the graph.
>
>The difference is how those children are added (or removed, but it's the
>same in reverse): In case of quorum, you can simply attach a new child
>because it supports a variable number of children. Another case where
>this would work are all block drivers which support backing files (you
>can freely attach/detach those).
Doesn't blockdev-snapshot-sync cover this? (I may be missing something).
Now that we're on this topic, quorum might be a good candidate for
*_reopen when and if it lands on QMP: Reconfiguring the children could
be done by reopening the BDS with new options.
>
>In this series, it's not about adding or removing new children, but
>instead "injecting" them into an edge: An existing child is replaced,
>but it now serves as some child of the new one.
>
>(I guess writing too much trying to get my point across, sorry...)
>
>The gist is that for me it's not so much about "quorum" or "filter
>nodes". It's about adding a new child to a node vs. replacing an
>existing one.
>
>> These are not directly compatible semantically, but as you said
>> x-blockdev-change can perform b) if bdrv_add_child()/bdrv_del_child()
>> are not implemented. Wouldn't that be unnecessary overloading?
>
>Yes, I think it would be. :-)
>
>So say we have these two trees in our graph:
>
>[ Parent BDS -> Child BDS ]
>[ Filter BDS -> Child BDS ]
>
>So here's what you can do with x-blockdev-change (in theory; in practice
>it can only do that for quorum):
>- Remove a child, so
> [ Parent BDS -> Child BDS ]
> is split into two trees
> [ Parent BDS ] and
> [ Child BDS ]
>- Add a child, so we can merge
> [ Parent BDS ] and
> [ Filter BDS -> Child BDS ]
> into
> [ Parent BDS -> Filter BDS -> Child BDS ]
>
Yes, of course this would have to be done in one transaction.
>However, this is only possible with quorum because usually block drivers
>don't support detaching children.
>
>And here's what you can do with your commands (from what I can see):
>- Replace a child (you call it insertion, but it really is just
> replacement of an existing child with the constraint that both nodes
> involved must have the same child):
> [ Parent BDS -> Child BDS ]
> [ Filter BDS -> Child BDS ]
> to
> [ Parent BDS -> Filter BDS -> Child BDS ]
>- Replace a child (you call it removal, but it really is just
> replacement of a child with its child):
> [ Parent BDS -> Filter BDS -> Child BDS ]
> to
> [ Parent BDS -> Child BDS ]
>
>This works on all BDSs because you don't change the number of children.
>
>
>The interesting thing of course is that the "change" command can
>actually add and remove children; where as the "insert" and "remove"
>commands can only replace children. So that's already a bit funny (one
>command does two things; two commands do one thing).
That is true, but the replacing is more in terms of inserting and
removing a node in a BDS chain.
>
>And then of course you can simply modify x-blockdev-change so it can do
>the same thing block-insert-node and block-remove-node can do: It just
>needs another mode which can be used to replace a child (and its
>description already states that it is supposed to be usable for that at
>some point in the future).
>
>
>So after I've written all of this, I feel like the new insert-node and
>remove-node commands are exactly what x-blockdev-change should do when
>asked to replace a node. The only difference is that x-blockdev-change
>would allow you to replace any node with anything, without the
>constraints that block-insert-node and block-remove-node exact.
>
>(And node replacement with x-blockdev-change would work by specifying
>all three parameters.)
>
>Not sure if that makes sense, I hope it does. :-)
>
Hm, I can't think of a way to fit that into x-blockdev-change *and* keep
the bdrv_add_child/bdrv_del_child functionality into consideration
(since we'd have to keep both). This is what the current doco is:
If @node is specified, it will be inserted under @parent. @child
may not be specified in this case. If both @parent and @child are
specified but @node is not, @child will be detached from @parent.
The simplest thing would be to add a flag as to what operation you want
to perform (add/del child versus filter insertion/removal from edges)
but this is what I was thinking about overloading it, it feels
convoluted yet the insert and remove commands felt intuitive enough to
me after experimenting with it a little. What do you think?
On 2017-10-04 23:04, Manos Pitsidianakis wrote:
> On Wed, Oct 04, 2017 at 08:09:24PM +0200, Max Reitz wrote:
>> On 2017-10-04 19:05, Manos Pitsidianakis wrote:
>>> On Wed, Oct 04, 2017 at 02:49:27PM +0200, Max Reitz wrote:
>>>> On 2017-08-15 09:45, Manos Pitsidianakis wrote:
>>>>> block-insert-node and its pair command block-remove-node provide
>>>>> runtime
>>>>> insertion and removal of filter nodes.
>>>>>
>>>>> block-insert-node takes a (parent, child) and (node, child) pair of
>>>>> edges and unrefs the (parent, child) BdrvChild relationship and
>>>>> creates
>>>>> a new (parent, node) child with the same BdrvChildRole.
>>>>>
>>>>> This is a different approach than x-blockdev-change which uses the
>>>>> driver
>>>>> methods bdrv_add_child() and bdrv_del_child(),
>>>>
>>>> Why? :-)
>>>>
>>>> Can't we reuse x-blockdev-change? As far as I'm concerned, this was one
>>>> of its roles, and at least I don't want to have both x-blockdev-change
>>>> and these new commands.
>>>>
>>>> I think it would be a good idea just to change bdrv_add_child() and
>>>> bdrv_del_child(): If the driver has a bdrv_{add,del}_child() callback,
>>>> invoke that. If it doesn't, then just attach the child.
>>>>
>>>> Of course, it may turn out that x-blockdev-change is lacking something
>>>> (e.g. a way to specify as what kind of child a new node is to be
>>>> attached). Then we should either extend it so that it covers what it's
>>>> lacking, or replace it completely by these new commands. In the latter
>>>> case, however, they would need to cover the existing use case which is
>>>> reconfiguring the quorum driver. (And that would mean it would have to
>>>> invoke bdrv_add_child()/bdrv_del_child() when the driver has them.)
>>>>
>>>> Max
>>>>
>>>
>>> I think the two use cases are this:
>>>
>>> a) Adding/removing a replication child to an existing quorum node
>>> b) Insert a filter between two existing nodes.
>>
>> For me both are the same: Adding/removing nodes into/from the graph.
>>
>> The difference is how those children are added (or removed, but it's the
>> same in reverse): In case of quorum, you can simply attach a new child
>> because it supports a variable number of children. Another case where
>> this would work are all block drivers which support backing files (you
>> can freely attach/detach those).
>
> Doesn't blockdev-snapshot-sync cover this? (I may be missing something).
Not really, because it doesn't add a new child. But it's still a good
point, because your block-insert-node command would make it obsolete,
actually.
blockdev-snapshot literally is a special-cased block-insert-node. And
blockdev-snapshot-sync then is qemu-img create + blockdev-add +
blockdev-snapshot.
> Now that we're on this topic, quorum might be a good candidate for
> *_reopen when and if it lands on QMP: Reconfiguring the children could
> be done by reopening the BDS with new options.
Hmmm... I guess that would work, too. But intuitively, that seems a
bit heavy-weight to me
>> In this series, it's not about adding or removing new children, but
>> instead "injecting" them into an edge: An existing child is replaced,
>> but it now serves as some child of the new one.
>>
>> (I guess writing too much trying to get my point across, sorry...)
>>
>> The gist is that for me it's not so much about "quorum" or "filter
>> nodes". It's about adding a new child to a node vs. replacing an
>> existing one.
>>
>>> These are not directly compatible semantically, but as you said
>>> x-blockdev-change can perform b) if bdrv_add_child()/bdrv_del_child()
>>> are not implemented. Wouldn't that be unnecessary overloading?
>>
>> Yes, I think it would be. :-)
>>
>> So say we have these two trees in our graph:
>>
>> [ Parent BDS -> Child BDS ]
>> [ Filter BDS -> Child BDS ]
>>
>> So here's what you can do with x-blockdev-change (in theory; in practice
>> it can only do that for quorum):
>> - Remove a child, so
>> [ Parent BDS -> Child BDS ]
>> is split into two trees
>> [ Parent BDS ] and
>> [ Child BDS ]
>> - Add a child, so we can merge
>> [ Parent BDS ] and
>> [ Filter BDS -> Child BDS ]
>> into
>> [ Parent BDS -> Filter BDS -> Child BDS ]
>>
>
> Yes, of course this would have to be done in one transaction.
Would it? If you want to put Filter BDS into the chain, you can just
create [ Filter BDS ] without any child, and then use a single
x-blockdev-change invocation to inject it.
The only thing that's necessary is that the filter BDS would have to be
able to handle having no child.
[...]
>> So after I've written all of this, I feel like the new insert-node and
>> remove-node commands are exactly what x-blockdev-change should do when
>> asked to replace a node. The only difference is that x-blockdev-change
>> would allow you to replace any node with anything, without the
>> constraints that block-insert-node and block-remove-node exact.
>>
>> (And node replacement with x-blockdev-change would work by specifying
>> all three parameters.)
>>
>> Not sure if that makes sense, I hope it does. :-)
>>
>
> Hm, I can't think of a way to fit that into x-blockdev-change *and* keep
> the bdrv_add_child/bdrv_del_child functionality into consideration
> (since we'd have to keep both). This is what the current doco is:
>
> If @node is specified, it will be inserted under @parent. @child
> may not be specified in this case. If both @parent and @child are
> specified but @node is not, @child will be detached from @parent.
>
> The simplest thing would be to add a flag as to what operation you want
> to perform (add/del child versus filter insertion/removal from edges)
> but this is what I was thinking about overloading it, it feels
> convoluted yet the insert and remove commands felt intuitive enough to
> me after experimenting with it a little. What do you think?
I agree that adding a flag is rather pointless, because then you can
simply have multiple commands.
But the idea was that if you specify both @node and @child, @child gets
replaced by @node. Currently, that combination is not allowed.
Max
On Fri, Oct 06, 2017 at 02:59:55PM +0200, Max Reitz wrote:
>On 2017-10-04 23:04, Manos Pitsidianakis wrote:
>> On Wed, Oct 04, 2017 at 08:09:24PM +0200, Max Reitz wrote:
>>> On 2017-10-04 19:05, Manos Pitsidianakis wrote:
>>>> On Wed, Oct 04, 2017 at 02:49:27PM +0200, Max Reitz wrote:
>>>>> On 2017-08-15 09:45, Manos Pitsidianakis wrote:
>>>>>> block-insert-node and its pair command block-remove-node provide
>>>>>> runtime
>>>>>> insertion and removal of filter nodes.
>>>>>>
>>>>>> block-insert-node takes a (parent, child) and (node, child) pair of
>>>>>> edges and unrefs the (parent, child) BdrvChild relationship and
>>>>>> creates
>>>>>> a new (parent, node) child with the same BdrvChildRole.
>>>>>>
>>>>>> This is a different approach than x-blockdev-change which uses the
>>>>>> driver
>>>>>> methods bdrv_add_child() and bdrv_del_child(),
>>>>>
>>>>> Why? :-)
>>>>>
>>>>> Can't we reuse x-blockdev-change? As far as I'm concerned, this was one
>>>>> of its roles, and at least I don't want to have both x-blockdev-change
>>>>> and these new commands.
>>>>>
>>>>> I think it would be a good idea just to change bdrv_add_child() and
>>>>> bdrv_del_child(): If the driver has a bdrv_{add,del}_child() callback,
>>>>> invoke that. If it doesn't, then just attach the child.
>>>>>
>>>>> Of course, it may turn out that x-blockdev-change is lacking something
>>>>> (e.g. a way to specify as what kind of child a new node is to be
>>>>> attached). Then we should either extend it so that it covers what it's
>>>>> lacking, or replace it completely by these new commands. In the latter
>>>>> case, however, they would need to cover the existing use case which is
>>>>> reconfiguring the quorum driver. (And that would mean it would have to
>>>>> invoke bdrv_add_child()/bdrv_del_child() when the driver has them.)
>>>>>
>>>>> Max
>>>>>
>>>>
>>>> I think the two use cases are this:
>>>>
>>>> a) Adding/removing a replication child to an existing quorum node
>>>> b) Insert a filter between two existing nodes.
>>>
>>> For me both are the same: Adding/removing nodes into/from the graph.
>>>
>>> The difference is how those children are added (or removed, but it's the
>>> same in reverse): In case of quorum, you can simply attach a new child
>>> because it supports a variable number of children. Another case where
>>> this would work are all block drivers which support backing files (you
>>> can freely attach/detach those).
>>
>> Doesn't blockdev-snapshot-sync cover this? (I may be missing something).
>
>Not really, because it doesn't add a new child. But it's still a good
>point, because your block-insert-node command would make it obsolete,
>actually.
>
>blockdev-snapshot literally is a special-cased block-insert-node. And
>blockdev-snapshot-sync then is qemu-img create + blockdev-add +
>blockdev-snapshot.
>
>> Now that we're on this topic, quorum might be a good candidate for
>> *_reopen when and if it lands on QMP: Reconfiguring the children could
>> be done by reopening the BDS with new options.
>
>Hmmm... I guess that would work, too. But intuitively, that seems a
>bit heavy-weight to me
>
>>> In this series, it's not about adding or removing new children, but
>>> instead "injecting" them into an edge: An existing child is replaced,
>>> but it now serves as some child of the new one.
>>>
>>> (I guess writing too much trying to get my point across, sorry...)
>>>
>>> The gist is that for me it's not so much about "quorum" or "filter
>>> nodes". It's about adding a new child to a node vs. replacing an
>>> existing one.
>>>
>>>> These are not directly compatible semantically, but as you said
>>>> x-blockdev-change can perform b) if bdrv_add_child()/bdrv_del_child()
>>>> are not implemented. Wouldn't that be unnecessary overloading?
>>>
>>> Yes, I think it would be. :-)
>>>
>>> So say we have these two trees in our graph:
>>>
>>> [ Parent BDS -> Child BDS ]
>>> [ Filter BDS -> Child BDS ]
>>>
>>> So here's what you can do with x-blockdev-change (in theory; in practice
>>> it can only do that for quorum):
>>> - Remove a child, so
>>> [ Parent BDS -> Child BDS ]
>>> is split into two trees
>>> [ Parent BDS ] and
>>> [ Child BDS ]
>>> - Add a child, so we can merge
>>> [ Parent BDS ] and
>>> [ Filter BDS -> Child BDS ]
>>> into
>>> [ Parent BDS -> Filter BDS -> Child BDS ]
>>>
>>
>> Yes, of course this would have to be done in one transaction.
>
>Would it? If you want to put Filter BDS into the chain, you can just
>create [ Filter BDS ] without any child, and then use a single
>x-blockdev-change invocation to inject it.
>
>The only thing that's necessary is that the filter BDS would have to be
>able to handle having no child.
Yeah that was what I had in mind.
>
>[...]
>
>>> So after I've written all of this, I feel like the new insert-node and
>>> remove-node commands are exactly what x-blockdev-change should do when
>>> asked to replace a node. The only difference is that x-blockdev-change
>>> would allow you to replace any node with anything, without the
>>> constraints that block-insert-node and block-remove-node exact.
>>>
>>> (And node replacement with x-blockdev-change would work by specifying
>>> all three parameters.)
>>>
>>> Not sure if that makes sense, I hope it does. :-)
>>>
>>
>> Hm, I can't think of a way to fit that into x-blockdev-change *and* keep
>> the bdrv_add_child/bdrv_del_child functionality into consideration
>> (since we'd have to keep both). This is what the current doco is:
>>
>> If @node is specified, it will be inserted under @parent. @child
>> may not be specified in this case. If both @parent and @child are
>> specified but @node is not, @child will be detached from @parent.
>>
>> The simplest thing would be to add a flag as to what operation you want
>> to perform (add/del child versus filter insertion/removal from edges)
>> but this is what I was thinking about overloading it, it feels
>> convoluted yet the insert and remove commands felt intuitive enough to
>> me after experimenting with it a little. What do you think?
>
>I agree that adding a flag is rather pointless, because then you can
>simply have multiple commands.
>
>But the idea was that if you specify both @node and @child, @child gets
>replaced by @node. Currently, that combination is not allowed.
And removal is to swap @node and @child and leave @node an orphan, but
with the constraint that @child is a child of @parent and @node is a
child of @parent.
Okay, I don't have any more arguments!. If there are no objections I can
look into a new RFC patch for x-blockdev-change.
© 2016 - 2024 Red Hat, Inc.