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

[Manos Pitsidianakis <el13635@mail.ntua.gr>]

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

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?

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