* [Qemu-devel] [PATCH v5] savevm: Really verify if a drive supports snapshots
@ 2010-06-08 13:40 Miguel Di Ciurcio Filho
2010-06-08 14:09 ` [Qemu-devel] " Kevin Wolf
0 siblings, 1 reply; 2+ messages in thread
From: Miguel Di Ciurcio Filho @ 2010-06-08 13:40 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, Miguel Di Ciurcio Filho
Both bdrv_can_snapshot() and bdrv_has_snapshot() does not work as advertized.
First issue: Their names implies different porpouses, but they do the same thing
and have exactly the same code. Maybe copied and pasted and forgotten?
bdrv_has_snapshot() is called in various places for actually checking if there
is snapshots or not.
Second issue: the way bdrv_can_snapshot() verifies if a block driver supports or
not snapshots does not catch all cases. E.g.: a raw image.
So when do_savevm() is called, first thing it does is to set a global
BlockDriverState to save the VM memory state calling get_bs_snapshots().
static BlockDriverState *get_bs_snapshots(void)
{
BlockDriverState *bs;
DriveInfo *dinfo;
if (bs_snapshots)
return bs_snapshots;
QTAILQ_FOREACH(dinfo, &drives, next) {
bs = dinfo->bdrv;
if (bdrv_can_snapshot(bs))
goto ok;
}
return NULL;
ok:
bs_snapshots = bs;
return bs;
}
bdrv_can_snapshot() may return a BlockDriverState that does not support
snapshots and do_savevm() goes on.
Later on in do_savevm(), we find:
QTAILQ_FOREACH(dinfo, &drives, next) {
bs1 = dinfo->bdrv;
if (bdrv_has_snapshot(bs1)) {
/* Write VM state size only to the image that contains the state */
sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
ret = bdrv_snapshot_create(bs1, sn);
if (ret < 0) {
monitor_printf(mon, "Error while creating snapshot on '%s'\n",
bdrv_get_device_name(bs1));
}
}
}
bdrv_has_snapshot(bs1) is not checking if the device does support or has
snapshots as explained above. Only in bdrv_snapshot_create() the device is
actually checked for snapshot support.
So, in cases where the first device supports snapshots, and the second does not,
the snapshot on the first will happen anyways. I believe this is not a good
behavior. It should be an all or nothing process.
This patch addresses these issues by making bdrv_can_snapshot() actually do
what it must do and enforces better tests to avoid errors in the middle of
do_savevm(). bdrv_has_snapshot() is removed and replaced by bdrv_can_snapshot()
where appropriate.
bdrv_can_snapshot() was moved from savevm.c to block.c. It makes more sense to me.
The loadvm_state() function was updated too to enforce that when loading a VM at
least all writable devices must support snapshots too.
Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
---
block.c | 17 +++++++++++++++++
block.h | 1 +
savevm.c | 58 ++++++++++++++++++++++++++++++++++++----------------------
3 files changed, 54 insertions(+), 22 deletions(-)
diff --git a/block.c b/block.c
index 39724c1..139feff 100644
--- a/block.c
+++ b/block.c
@@ -1652,6 +1652,23 @@ void bdrv_debug_event(BlockDriverState *bs, BlkDebugEvent event)
/**************************************************************/
/* handling of snapshots */
+int bdrv_can_snapshot(BlockDriverState *bs)
+{
+ BlockDriver *drv = bs->drv;
+ if (!drv || bdrv_is_removable(bs) || bdrv_is_read_only(bs)) {
+ return 0;
+ }
+
+ if (!drv->bdrv_snapshot_create) {
+ if (bs->file != NULL) {
+ return bdrv_can_snapshot(bs->file);
+ }
+ return 0;
+ }
+
+ return 1;
+}
+
int bdrv_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info)
{
diff --git a/block.h b/block.h
index 756670d..acb327f 100644
--- a/block.h
+++ b/block.h
@@ -174,6 +174,7 @@ int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
void bdrv_get_backing_filename(BlockDriverState *bs,
char *filename, int filename_size);
+int bdrv_can_snapshot(BlockDriverState *bs);
int bdrv_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info);
int bdrv_snapshot_goto(BlockDriverState *bs,
diff --git a/savevm.c b/savevm.c
index af92ba2..9b41eff 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1575,22 +1575,6 @@ out:
return ret;
}
-/* device can contain snapshots */
-static int bdrv_can_snapshot(BlockDriverState *bs)
-{
- return (bs &&
- !bdrv_is_removable(bs) &&
- !bdrv_is_read_only(bs));
-}
-
-/* device must be snapshots in order to have a reliable snapshot */
-static int bdrv_has_snapshot(BlockDriverState *bs)
-{
- return (bs &&
- !bdrv_is_removable(bs) &&
- !bdrv_is_read_only(bs));
-}
-
static BlockDriverState *get_bs_snapshots(void)
{
BlockDriverState *bs;
@@ -1600,8 +1584,9 @@ static BlockDriverState *get_bs_snapshots(void)
return bs_snapshots;
QTAILQ_FOREACH(dinfo, &drives, next) {
bs = dinfo->bdrv;
- if (bdrv_can_snapshot(bs))
+ if (bdrv_can_snapshot(bs)) {
goto ok;
+ }
}
return NULL;
ok:
@@ -1675,12 +1660,26 @@ void do_savevm(Monitor *mon, const QDict *qdict)
#endif
const char *name = qdict_get_try_str(qdict, "name");
+ /* Verify if there is a device that doesn't support snapshots and is writable */
+ QTAILQ_FOREACH(dinfo, &drives, next) {
+ bs = dinfo->bdrv;
+
+ if (bdrv_is_removable(bs) || bdrv_is_read_only(bs)) {
+ continue;
+ }
+
+ if (!bdrv_can_snapshot(bs)) {
+ monitor_printf(mon, "Device '%s' is writable but does not support snapshots.\n",
+ bdrv_get_device_name(bs));
+ return;
+ }
+ }
+
bs = get_bs_snapshots();
if (!bs) {
monitor_printf(mon, "No block device can accept snapshots\n");
return;
}
-
/* ??? Should this occur after vm_stop? */
qemu_aio_flush();
@@ -1733,7 +1732,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
QTAILQ_FOREACH(dinfo, &drives, next) {
bs1 = dinfo->bdrv;
- if (bdrv_has_snapshot(bs1)) {
+ if (bdrv_can_snapshot(bs1)) {
/* Write VM state size only to the image that contains the state */
sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
ret = bdrv_snapshot_create(bs1, sn);
@@ -1757,6 +1756,21 @@ int load_vmstate(const char *name)
QEMUFile *f;
int ret;
+ /* Verify if there is a device that doesn't support snapshots and is writable */
+ QTAILQ_FOREACH(dinfo, &drives, next) {
+ bs = dinfo->bdrv;
+
+ if (bdrv_is_removable(bs) || bdrv_is_read_only(bs)) {
+ continue;
+ }
+
+ if (!bdrv_can_snapshot(bs)) {
+ error_report("Device '%s' is writable but does not support snapshots.",
+ bdrv_get_device_name(bs));
+ return -ENOTSUP;
+ }
+ }
+
bs = get_bs_snapshots();
if (!bs) {
error_report("No block device supports snapshots");
@@ -1768,7 +1782,7 @@ int load_vmstate(const char *name)
QTAILQ_FOREACH(dinfo, &drives, next) {
bs1 = dinfo->bdrv;
- if (bdrv_has_snapshot(bs1)) {
+ if (bdrv_can_snapshot(bs1)) {
ret = bdrv_snapshot_goto(bs1, name);
if (ret < 0) {
switch(ret) {
@@ -1830,7 +1844,7 @@ void do_delvm(Monitor *mon, const QDict *qdict)
QTAILQ_FOREACH(dinfo, &drives, next) {
bs1 = dinfo->bdrv;
- if (bdrv_has_snapshot(bs1)) {
+ if (bdrv_can_snapshot(bs1)) {
ret = bdrv_snapshot_delete(bs1, name);
if (ret < 0) {
if (ret == -ENOTSUP)
@@ -1861,7 +1875,7 @@ void do_info_snapshots(Monitor *mon)
monitor_printf(mon, "Snapshot devices:");
QTAILQ_FOREACH(dinfo, &drives, next) {
bs1 = dinfo->bdrv;
- if (bdrv_has_snapshot(bs1)) {
+ if (bdrv_can_snapshot(bs1)) {
if (bs == bs1)
monitor_printf(mon, " %s", bdrv_get_device_name(bs1));
}
--
1.7.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* [Qemu-devel] Re: [PATCH v5] savevm: Really verify if a drive supports snapshots
2010-06-08 13:40 [Qemu-devel] [PATCH v5] savevm: Really verify if a drive supports snapshots Miguel Di Ciurcio Filho
@ 2010-06-08 14:09 ` Kevin Wolf
0 siblings, 0 replies; 2+ messages in thread
From: Kevin Wolf @ 2010-06-08 14:09 UTC (permalink / raw)
To: Miguel Di Ciurcio Filho; +Cc: qemu-devel
Am 08.06.2010 15:40, schrieb Miguel Di Ciurcio Filho:
> Both bdrv_can_snapshot() and bdrv_has_snapshot() does not work as advertized.
>
> First issue: Their names implies different porpouses, but they do the same thing
> and have exactly the same code. Maybe copied and pasted and forgotten?
> bdrv_has_snapshot() is called in various places for actually checking if there
> is snapshots or not.
>
> Second issue: the way bdrv_can_snapshot() verifies if a block driver supports or
> not snapshots does not catch all cases. E.g.: a raw image.
>
> So when do_savevm() is called, first thing it does is to set a global
> BlockDriverState to save the VM memory state calling get_bs_snapshots().
>
> static BlockDriverState *get_bs_snapshots(void)
> {
> BlockDriverState *bs;
> DriveInfo *dinfo;
>
> if (bs_snapshots)
> return bs_snapshots;
> QTAILQ_FOREACH(dinfo, &drives, next) {
> bs = dinfo->bdrv;
> if (bdrv_can_snapshot(bs))
> goto ok;
> }
> return NULL;
> ok:
> bs_snapshots = bs;
> return bs;
> }
>
> bdrv_can_snapshot() may return a BlockDriverState that does not support
> snapshots and do_savevm() goes on.
>
> Later on in do_savevm(), we find:
>
> QTAILQ_FOREACH(dinfo, &drives, next) {
> bs1 = dinfo->bdrv;
> if (bdrv_has_snapshot(bs1)) {
> /* Write VM state size only to the image that contains the state */
> sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
> ret = bdrv_snapshot_create(bs1, sn);
> if (ret < 0) {
> monitor_printf(mon, "Error while creating snapshot on '%s'\n",
> bdrv_get_device_name(bs1));
> }
> }
> }
>
> bdrv_has_snapshot(bs1) is not checking if the device does support or has
> snapshots as explained above. Only in bdrv_snapshot_create() the device is
> actually checked for snapshot support.
>
> So, in cases where the first device supports snapshots, and the second does not,
> the snapshot on the first will happen anyways. I believe this is not a good
> behavior. It should be an all or nothing process.
>
> This patch addresses these issues by making bdrv_can_snapshot() actually do
> what it must do and enforces better tests to avoid errors in the middle of
> do_savevm(). bdrv_has_snapshot() is removed and replaced by bdrv_can_snapshot()
> where appropriate.
>
> bdrv_can_snapshot() was moved from savevm.c to block.c. It makes more sense to me.
>
> The loadvm_state() function was updated too to enforce that when loading a VM at
> least all writable devices must support snapshots too.
>
> Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
Thanks, I updated the patch in the block branch. Let's hope we've got it
correct this time.
Kevin
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2010-06-08 14:09 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-06-08 13:40 [Qemu-devel] [PATCH v5] savevm: Really verify if a drive supports snapshots Miguel Di Ciurcio Filho
2010-06-08 14:09 ` [Qemu-devel] " Kevin Wolf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).