[Qemu-devel] [PATCH] monitor: Really show snapshot information about all devices

[Qemu-devel] [PATCH] monitor: Really show snapshot information about all devices

[Miguel Di Ciurcio Filho]

The 'info snapshots' monitor command does not show snapshot information from all
available block devices.

Usage example:
$ qemu -hda disk1.qcow2 -hdb disk2.qcow2

(qemu) info snapshots
Snapshot devices: ide0-hd0
Snapshot list (from ide0-hd0):
ID        TAG                 VM SIZE                DATE       VM CLOCK
1                                1.5M 2010-05-26 21:51:02   00:00:03.263
2                                1.5M 2010-05-26 21:51:09   00:00:08.844
3                                1.5M 2010-05-26 21:51:24   00:00:23.274
4                                1.5M 2010-05-26 21:53:17   00:00:03.595

In the above case, disk2.qcow2 has snapshot information, but it is not being
shown. Only the first device is always shown.

This patch updates the do_info_snapshots() function do correctly show snapshot
information about all available block devices.

New output:
(qemu) info snapshots
Snapshot list from ide0-hd0:
ID        TAG                 VM SIZE                DATE       VM CLOCK
1                                1.5M 2010-05-26 21:51:02   00:00:03.263
2                                1.5M 2010-05-26 21:51:09   00:00:08.844
3                                1.5M 2010-05-26 21:51:24   00:00:23.274
4                                1.5M 2010-05-26 21:53:17   00:00:03.595

Snapshot list from ide0-hd1:
ID        TAG                 VM SIZE                DATE       VM CLOCK
1                                   0 2010-05-26 21:51:02   00:00:03.263
2                                   0 2010-05-26 21:51:09   00:00:08.844
3                                   0 2010-05-26 21:51:24   00:00:23.274
4                                   0 2010-05-26 21:53:17   00:00:03.595

Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
---
 savevm.c |   46 +++++++++++++++++++++++-----------------------
 1 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/savevm.c b/savevm.c
index 20354a8..0eacb9f 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1858,8 +1858,8 @@ void do_delvm(Monitor *mon, const QDict *qdict)
 
 void do_info_snapshots(Monitor *mon)
 {
-    BlockDriverState *bs, *bs1;
-    QEMUSnapshotInfo *sn_tab, *sn;
+    BlockDriverState *bs;
+    QEMUSnapshotInfo *sn_tab;
     int nb_sns, i;
     char buf[256];
 
@@ -1868,27 +1868,27 @@ void do_info_snapshots(Monitor *mon)
         monitor_printf(mon, "No available block device supports snapshots\n");
         return;
     }
-    monitor_printf(mon, "Snapshot devices:");
-    bs1 = NULL;
-    while ((bs1 = bdrv_next(bs1))) {
-        if (bdrv_can_snapshot(bs1)) {
-            if (bs == bs1)
-                monitor_printf(mon, " %s", bdrv_get_device_name(bs1));
-        }
-    }
-    monitor_printf(mon, "\n");
 
-    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
-    if (nb_sns < 0) {
-        monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
-        return;
-    }
-    monitor_printf(mon, "Snapshot list (from %s):\n",
-                   bdrv_get_device_name(bs));
-    monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
-    for(i = 0; i < nb_sns; i++) {
-        sn = &sn_tab[i];
-        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
+    bs = NULL;
+    while ((bs = bdrv_next(bs))) {
+        if (bdrv_can_snapshot(bs)) {
+            monitor_printf(mon, "Snapshot list from %s:\n",
+                           bdrv_get_device_name(bs));
+            monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+
+            nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+            if (nb_sns < 0) {
+                monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
+                continue;
+            }
+
+            for (i = 0; i < nb_sns; i++) {
+                monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf),
+                    &sn_tab[i]));
+            }
+
+            qemu_free(sn_tab);
+            monitor_printf(mon, "\n");
+        }
     }
-    qemu_free(sn_tab);
 }
-- 
1.7.1

[Qemu-devel] Re: [PATCH] monitor: Really show snapshot information about all devices

[Kevin Wolf]

Am 16.06.2010 03:53, schrieb Miguel Di Ciurcio Filho:
> The 'info snapshots' monitor command does not show snapshot information from all
> available block devices.
> 
> Usage example:
> $ qemu -hda disk1.qcow2 -hdb disk2.qcow2
> 
> (qemu) info snapshots
> Snapshot devices: ide0-hd0
> Snapshot list (from ide0-hd0):
> ID        TAG                 VM SIZE                DATE       VM CLOCK
> 1                                1.5M 2010-05-26 21:51:02   00:00:03.263
> 2                                1.5M 2010-05-26 21:51:09   00:00:08.844
> 3                                1.5M 2010-05-26 21:51:24   00:00:23.274
> 4                                1.5M 2010-05-26 21:53:17   00:00:03.595
> 
> In the above case, disk2.qcow2 has snapshot information, but it is not being
> shown. Only the first device is always shown.
> 
> This patch updates the do_info_snapshots() function do correctly show snapshot
> information about all available block devices.
> 
> New output:
> (qemu) info snapshots
> Snapshot list from ide0-hd0:
> ID        TAG                 VM SIZE                DATE       VM CLOCK
> 1                                1.5M 2010-05-26 21:51:02   00:00:03.263
> 2                                1.5M 2010-05-26 21:51:09   00:00:08.844
> 3                                1.5M 2010-05-26 21:51:24   00:00:23.274
> 4                                1.5M 2010-05-26 21:53:17   00:00:03.595
> 
> Snapshot list from ide0-hd1:
> ID        TAG                 VM SIZE                DATE       VM CLOCK
> 1                                   0 2010-05-26 21:51:02   00:00:03.263
> 2                                   0 2010-05-26 21:51:09   00:00:08.844
> 3                                   0 2010-05-26 21:51:24   00:00:23.274
> 4                                   0 2010-05-26 21:53:17   00:00:03.595
> 
> Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>

If the human monitor was exactly what its name says, I'd happily apply
this one (though I think it should be made clear from which image the VM
state would be loaded). However, it isn't and I'm not sure if this
wouldn't break libvirt. Dan, can you help?

If we can't change the info snapshots output, a possible alternative
would be to introduce another info command for this.

Kevin

[Qemu-devel] Re: [PATCH] monitor: Really show snapshot information about all devices

[Miguel Di Ciurcio Filho]

On Wed, Jun 16, 2010 at 9:40 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>
> If the human monitor was exactly what its name says, I'd happily apply
> this one (though I think it should be made clear from which image the VM
> state would be loaded). However, it isn't and I'm not sure if this
> wouldn't break libvirt. Dan, can you help?
>

I didn't mention in the commit, but I've looked at libvirt's source
and it is not using 'info snapshots' AFAIK.

At the present time, the VM state is always saved in the first block
device that supports snapshots. I could update the patch to make it
clear on the output somehow. From savevm.c:get_bs_snapshot():

static BlockDriverState *get_bs_snapshots(void)
{
    BlockDriverState *bs;

    if (bs_snapshots)
        return bs_snapshots;
    /* FIXME what if bs_snapshots gets hot-unplugged? */

    bs = NULL;
    while ((bs = bdrv_next(bs))) {
        if (bdrv_can_snapshot(bs)) {
            goto ok;
        }
    }
    return NULL;
 ok:
    bs_snapshots = bs;
    return bs;
}

Regards,

Miguel

[Qemu-devel] Re: [PATCH] monitor: Really show snapshot information about all devices

[Kevin Wolf]

Am 16.06.2010 14:59, schrieb Miguel Di Ciurcio Filho:
> On Wed, Jun 16, 2010 at 9:40 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>>
>> If the human monitor was exactly what its name says, I'd happily apply
>> this one (though I think it should be made clear from which image the VM
>> state would be loaded). However, it isn't and I'm not sure if this
>> wouldn't break libvirt. Dan, can you help?
>>
> 
> I didn't mention in the commit, but I've looked at libvirt's source
> and it is not using 'info snapshots' AFAIK.

Anthony, Dan, are you okay with the change then?

> At the present time, the VM state is always saved in the first block
> device that supports snapshots. I could update the patch to make it
> clear on the output somehow.

Would be nice to make it clear. Something like this maybe:

    Snapshot list from ide0-hd0 (VM state image):
    [...]

Kevin

Re: [Qemu-devel] Re: [PATCH] monitor: Really show snapshot information about all devices

[Kevin Wolf]

Am 16.06.2010 17:22, schrieb Chris Lalancette:
> On 06/16/10 - 03:15:11PM, Kevin Wolf wrote:
>> Am 16.06.2010 14:59, schrieb Miguel Di Ciurcio Filho:
>>> On Wed, Jun 16, 2010 at 9:40 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>>>>
>>>> If the human monitor was exactly what its name says, I'd happily apply
>>>> this one (though I think it should be made clear from which image the VM
>>>> state would be loaded). However, it isn't and I'm not sure if this
>>>> wouldn't break libvirt. Dan, can you help?
>>>>
>>>
>>> I didn't mention in the commit, but I've looked at libvirt's source
>>> and it is not using 'info snapshots' AFAIK.
>>
>> Anthony, Dan, are you okay with the change then?
> 
> Right, exactly as Miguel said, libvirt doesn't use "info snapshots" at all
> at the moment.  One of the reasons we don't use it at present is precisely
> because it doesn't give us information about all disks in-use.
> 
> The other reason that we can't use "info snapshots" is that we need to know
> parent information about snapshots. That is, if you take a sequence of
> snapshots:
> 
> A -> B -> C
> 
> And then you delete B, the disk changes from B will be merged automatically
> into C to keep C a valid snapshot.  However, there is currently no way to
> discover this parent/child relationship, so we can't use "info snapshots"
> for that reason as well.

Well, there is no parent/child relation in qcow2, so exposing this is
going to be really hard. We also don't really need it anywhere in qemu.
What would libvirt use this information for?

Kevin

Re: [Qemu-devel] Re: [PATCH] monitor: Really show snapshot information about all devices

[Kevin Wolf]

Am 16.06.2010 17:57, schrieb Chris Lalancette:
> On 06/16/10 - 05:32:58PM, Kevin Wolf wrote:
>> Am 16.06.2010 17:22, schrieb Chris Lalancette:
>>> On 06/16/10 - 03:15:11PM, Kevin Wolf wrote:
>>>> Am 16.06.2010 14:59, schrieb Miguel Di Ciurcio Filho:
>>>>> On Wed, Jun 16, 2010 at 9:40 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>>>>>>
>>>>>> If the human monitor was exactly what its name says, I'd happily apply
>>>>>> this one (though I think it should be made clear from which image the VM
>>>>>> state would be loaded). However, it isn't and I'm not sure if this
>>>>>> wouldn't break libvirt. Dan, can you help?
>>>>>>
>>>>>
>>>>> I didn't mention in the commit, but I've looked at libvirt's source
>>>>> and it is not using 'info snapshots' AFAIK.
>>>>
>>>> Anthony, Dan, are you okay with the change then?
>>>
>>> Right, exactly as Miguel said, libvirt doesn't use "info snapshots" at all
>>> at the moment.  One of the reasons we don't use it at present is precisely
>>> because it doesn't give us information about all disks in-use.
>>>
>>> The other reason that we can't use "info snapshots" is that we need to know
>>> parent information about snapshots. That is, if you take a sequence of
>>> snapshots:
>>>
>>> A -> B -> C
>>>
>>> And then you delete B, the disk changes from B will be merged automatically
>>> into C to keep C a valid snapshot.  However, there is currently no way to
>>> discover this parent/child relationship, so we can't use "info snapshots"
>>> for that reason as well.
>>
>> Well, there is no parent/child relation in qcow2, so exposing this is
>> going to be really hard. We also don't really need it anywhere in qemu.
>> What would libvirt use this information for?
> 
> I keep being told this, and I don't really understand how this is.  I know
> when I was heavily playing with this, the scenario above worked; that is, the
> deletion of snapshot B maintained a valid C snapshot.  If nothing is tracking
> the parent/child relationship, how does this work?

Clusters are refcounted. When you save a snapshot, the refcount for all
clusters in the current state is increased. When you delete it, the
refcount is decreased and only if it's zero the cluster is freed.

> As for how libvirt uses it, it's mostly to provide the ability for the user
> to keep track of a "tree" of snapshots.  So the user could do something like
> install their base OS, and take a snapshot S1.  Then they could install one set
> of applications, and take a snapshot S2.  Now they can go back to the base
> image, install a different set of applications, and take a snapshot S3.
> Now both S2 and S3 are children of S1, and libvirt wants to be able to
> represent this relationship.

qemu doesn't even remember which snapshot you have loaded. Basically you
have one L1 table for active cluster allocations and you have another
one for each snapshot. When you load a snapshot, it just copies the L1
table to the active one (and adjusts refcounts).

So technically the concept of a snapshot tree doesn't exist with
internal snapshots. It's something that management introduces.

Kevin

Re: [Qemu-devel] Re: [PATCH] monitor: Really show snapshot information about all devices

[Miguel Di Ciurcio Filho]

On Wed, Jun 16, 2010 at 12:22 PM, Chris Lalancette <clalance@redhat.com> wrote:
>> > I didn't mention in the commit, but I've looked at libvirt's source
>> > and it is not using 'info snapshots' AFAIK.
>>
>> Anthony, Dan, are you okay with the change then?
>
> Right, exactly as Miguel said, libvirt doesn't use "info snapshots" at all
> at the moment.  One of the reasons we don't use it at present is precisely
> because it doesn't give us information about all disks in-use.
>
> The other reason that we can't use "info snapshots" is that we need to know
> parent information about snapshots. That is, if you take a sequence of
> snapshots:
>

So lets get there, we need to start from somewhere :-D


>>
>> > At the present time, the VM state is always saved in the first block
>> > device that supports snapshots. I could update the patch to make it
>> > clear on the output somehow.
>>
>> Would be nice to make it clear. Something like this maybe:
>>
>>     Snapshot list from ide0-hd0 (VM state image):
>>     [...]
>
> Yes, I also agree this would be a good idea.  When the QMP version of this
> finally gets implemented, we'll want some way to distinguish where the
> VM state is stored as well.
>

I will update the patch as proposed by Kevin and resend.

Regards,

Miguel