Re: [Qemu-devel] [PATCH RFC] block: add block-insert-node QMP command
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
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH RFC] block: add block-insert-node QMP command
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
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
Re: [Qemu-devel] [PATCH RFC] block: add block-insert-node QMP command
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 signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH RFC] block: add block-insert-node QMP command
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? signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH RFC] block: add block-insert-node QMP command
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 signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH RFC] block: add block-insert-node QMP command
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
---
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
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH RFC] block: add block-insert-node QMP command
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
> ---
> 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
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH RFC] block: add block-insert-node QMP command
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
---
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,
+
