Re: [Qemu-block] [PATCH RFC] block: add block-insert-node QMP command

[Manos Pitsidianakis]

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?

signature.asc
Description: PGP signature