[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4] savevm: Really verify if a drive supports sn
From: |
MORITA Kazutaka |
Subject: |
Re: [Qemu-devel] [PATCH v4] savevm: Really verify if a drive supports snapshots |
Date: |
Tue, 08 Jun 2010 13:39:51 +0900 |
User-agent: |
Wanderlust/2.14.0 (Africa) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (Gojō) APEL/10.7 Emacs/22.2 (x86_64-pc-linux-gnu) MULE/5.0 (SAKAKI) |
At Fri, 4 Jun 2010 16:35:59 -0300,
Miguel Di Ciurcio Filho wrote:
>
> 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 <address@hidden>
> ---
> block.c | 11 +++++++++++
> block.h | 1 +
> savevm.c | 58 ++++++++++++++++++++++++++++++++++++----------------------
> 3 files changed, 48 insertions(+), 22 deletions(-)
>
> diff --git a/block.c b/block.c
> index cd70730..ace3cdb 100644
> --- a/block.c
> +++ b/block.c
> @@ -1720,6 +1720,17 @@ void bdrv_debug_event(BlockDriverState *bs,
> BlkDebugEvent event)
> /**************************************************************/
> /* handling of snapshots */
>
> +int bdrv_can_snapshot(BlockDriverState *bs)
> +{
> + BlockDriver *drv = bs->drv;
> + if (!drv || !drv->bdrv_snapshot_create || bdrv_is_removable(bs) ||
> + bdrv_is_read_only(bs)) {
> + return 0;
> + }
> +
> + return 1;
> +}
> +
The underlying protocol could support snapshots, so I think we should
check against bs->file too.
--- a/block.c
+++ b/block.c
@@ -1671,6 +1671,9 @@ int bdrv_can_snapshot(BlockDriverState *bs)
BlockDriver *drv = bs->drv;
if (!drv || !drv->bdrv_snapshot_create || bdrv_is_removable(bs) ||
bdrv_is_read_only(bs)) {
+ if (bs->file) {
+ return bdrv_can_snapshot(bs->file);
+ }
return 0;
}
Regards,
Kazutaka