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). 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. > >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. >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). That is true, but the replacing is more in terms of inserting and removing a node in a BDS chain. > >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. :-) > 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?