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

[Max Reitz]

On 2018-03-07 17:43, Stefano Panella wrote:
> 
> 
> On Wed, Mar 7, 2018 at 4:16 PM, Stefan Hajnoczi <address@hidden
> <mailto:address@hidden>> wrote:
>>
>> On Wed, Mar 7, 2018 at 1:57 PM, Stefano Panella <address@hidden
> <mailto:address@hidden>> wrote:
>> > On Wed, Mar 7, 2018 at 10:55 AM, Stefan Hajnoczi <address@hidden
> <mailto:address@hidden>> wrote:
>> >>
>> >> On Tue, Mar 6, 2018 at 11:25 PM, Stefano Panella
> <address@hidden <mailto: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.
>> > Thanks, that explains the behaviour
>> >>
>> >> 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.  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 would be nice, I mean to add an extra parameter so it is added
> to the
>> > monitor
>> >>
>> >> 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.
>> >>
>> > Yes, I was not aware of that behaviour, the problem is that many
> examples
>> > refer
>> > to having a device associated with the blockdev-add'd node therefore
> we do
>> > not
>> > see this problem.
>> >> If it is a problem for your use case, please explain what you are
> trying
>> >> to do.
>> >>
>> > It is not strictly a problem for my usecase but it would be nice to
> have the
>> > extra param to
>> > blockdev-snapshot-sync. That would also fix the problem of running
> multiple
>> > snap-sync
>> > after blockdev-add but before there is any user.
>>
>> Max Reitz mentioned that the 'blockdev-snapshot' command is preferred
>> over 'blockdev-snapshot-sync'.  'blockdev-snapshot-sync' is a legacy
>> command that implicitly creates the snapshot node.
>>
>> The difference is that 'blockdev-snapshot' requires that the user
>> first creates the snapshot file (e.g. using qemu-img create), then
>> uses 'blockdev-add' to add the snapshot node, and finally uses
>> 'blockdev-snapshot' to install the snapshot node.
>>
>> When 'blockdev-snapshot' is used, the user must delete snapshot nodes
>> using 'blockdev-del' since they created using 'blockdev-add'.
>>
> That is a very usefull info, I was not aware that blockdev-snapshot-sync
> was not
> recommended.

Yeah, well...  Someone (O:-)) needs to go over all the block QMP
commands and see which are good and which should be deprecated at some
point.  I don't think we have a central list of everything yet...

> I will try to run some examples with blockdev-snapshot.
> In case I want to achieve
> A <- B
> and I do:
> blockdev_add A
> create external snapshot with qemu-img B with A as backing image
> blockdev_add B
> blockdev_snapshot B -> A
> 
> What do I need to do to delete A and B?
> is it fine to just call blockdev_del B ?
> or should I call blockdev_del A as well?

You need to call both.  The basic idea is that you have to pair every
blockdev-add with a blockdev-del.

(You have to delete B first, though, because you cannot delete a node
while it is in use (and A is in use by B as long as B exists).)

Don't forget the '"backing": null" parameter for the blockdev-add B
command, or B will already have A opened as its backing image (which is
not good, you don't want qemu to open the same image twice).

(Or maybe blockdev-add B will not even work without '"backing": null'
because qemu figures out that you are trying to open the same image (A)
twice and prevent that.)

Max

signature.asc
Description: OpenPGP digital signature