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

[Max Reitz]

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

signature.asc
Description: OpenPGP digital signature