On 2018-03-07 11:55, Stefan Hajnoczi wrote: > On Tue, Mar 6, 2018 at 11:25 PM, Stefano Panella <spane...@gmail.com> wrote: >> I have applied this patch and when I run the following qmp commands I I do >> not see the crash anymore but there is still something wrong because only >> /root/a is opened from qemu. It looks like nbd-server-stop is also getting >> rid of the nodes added with blockdev-snapshot-sync, therfore is than not >> possible to do blockdev-del on /root/d because node-name is not found > > Nodes are reference counted. If nothing holds a refcount then the > node is freed. > > The blockdev-add command holds a reference to the node. The node will > stay alive until blockdev-del, which releases that reference. > > blockdev-snapshot-sync does not hold a reference.
I think that's a bug. When you specify a node name for the new node, it should get a reference. > Therefore snapshot > nodes are freed once nothing is using them anymore. When the snapshot > node is created, the users of the parent node are updated to point to > the snapshot node instead. This is why the NBD server switches to the > snapshot mode after blockdev-snapshot-sync. > > This is why the snapshot nodes disappear after the NBD server is > stopped while /root/a stays alive. > > I'm not sure if the current blockdev-snapshot-sync behavior is useful. > Perhaps the presence of the "snapshot-node-name" argument should cause > the snapshot node to be treated as monitor-owned, just like > blockdev-add. This would introduce leaks for existing QMP clients > though, so it may be necessary to add yet another argument for this > behavior. That's true. > Anyway, I hope this explains the current behavior. I don't see a > problem with it, but it's something the API users need to be aware of. Hm, OK. As an explanation: blockdev-snapshot-sync is from before we had node names and blockdev-add. You'd just create something that needs the block layer (like a guest device or an NBD server) and then you'd open the BDS chain you want to go with it (mostly by just specifying the filename of the top image, and maybe its format). Then you'd use blockdev-snapshot-sync to just create an overlay during runtime, and since there weren't any node names it was clear that it would go away if you deleted whatever was using the chain (like the NBD server). Then we introduced node names, and blockdev-snapshot-sync gained the ability to give one to the overlay -- basically as an afterthought. I think we didn't really have a fleshed-out concept of monitor references back then... So we forgot to give the overlay an additional reference in such a case (because we didn't know better). As you pointed to in your other reply, blockdev-snapshot is the "pure" blockdev command that should ideally be used. It allows you much more control over the overlay (because you have to do the blockdev-add yourself), and it doesn't have this reference issue. Max
signature.asc
Description: OpenPGP digital signature
