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

[Max Reitz]

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

signature.asc
Description: OpenPGP digital signature