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.) MaxI 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 meIn 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.
signature.asc
Description: PGP signature
