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
