Re: [Qemu-block] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case

[Max Reitz]

On 2018-03-07 11:55, Stefan Hajnoczi wrote:
> On Tue, Mar 6, 2018 at 11:25 PM, Stefano Panella <address@hidden> 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