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: 6777 bytes --]

On Fri, Oct 06, 2017 at 02:59:55PM +0200, Max Reitz wrote:
>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.

Yeah that was what I had in mind.
>
>[...]
>
>>> 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.

And removal is to swap @node and @child and leave @node an orphan, but 
with the constraint that @child is a child of @parent and @node is a 
child of @parent.

Okay, I don't have any more arguments!. If there are no objections I can 
look into a new RFC patch for x-blockdev-change.

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