[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snap
Re: [Qemu-block] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case
Wed, 7 Mar 2018 16:16:46 +0000
On Wed, Mar 7, 2018 at 1:57 PM, Stefano Panella <address@hidden> wrote:
> On Wed, Mar 7, 2018 at 10:55 AM, Stefan Hajnoczi <address@hidden> wrote:
>> On Tue, Mar 6, 2018 at 11:25 PM, Stefano Panella <address@hidden>
>> > 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
> that would be nice, I mean to add an extra parameter so it is added to the
>> 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
> to having a device associated with the blockdev-add'd node therefore we do
> 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
> 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'.
Re: [Qemu-block] [Qemu-devel] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case, Eric Blake, 2018/03/09
Re: [Qemu-block] [PATCH 2/2] iotests: add 208 nbd-server + blockdev-snapshot-sync test case, Max Reitz, 2018/03/12
[Qemu-block] [PATCH 1/2] block: let blk_add/remove_aio_context_notifier() tolerate BDS changes, Stefan Hajnoczi, 2018/03/06
Re: [Qemu-block] [PATCH 1/2] block: let blk_add/remove_aio_context_notifier() tolerate BDS changes, Max Reitz, 2018/03/12