[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH] savevm: Really verify if a drive supports snaps
From: |
Kevin Wolf |
Subject: |
[Qemu-devel] Re: [PATCH] savevm: Really verify if a drive supports snapshots |
Date: |
Fri, 28 May 2010 20:50:30 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9) Gecko/20100430 Fedora/3.0.4-2.fc12 Thunderbird/3.0.4 |
Am 28.05.2010 20:18, 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() and
> bdrv_has_snapshot() actually do what they must do and enforces better tests to
> avoid errors in the middle of do_savevm().
>
> The functions were moved from savevm.c to block.c. It makes more sense to me.
>
> The bdrv_has_snapshot() is not beaultiful, but it does the job. I think having
> this function avaible in the BlockDriver would be the best option.
>
> 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>
Markus, I think this implements mostly what we discussed the other day.
Not sure if you already have a patch for doing this - if so, maybe could
compare the patches and give it a review this way?
I seem to remember that we came to the conclusion that
bdrv_has_snapshot() isn't needed at all and should be dropped. Any user
should be using bdrv_can_snapshot() instead as this is what they really
want.
> ---
> block.c | 47 ++++++++++++++++++++++++++++++++++++++++++-----
> block.h | 2 ++
> savevm.c | 48 +++++++++++++++++++++++++-----------------------
> 3 files changed, 69 insertions(+), 28 deletions(-)
>
> diff --git a/block.c b/block.c
> index cd70730..7eddc15 100644
> --- a/block.c
> +++ b/block.c
> @@ -1720,15 +1720,52 @@ void bdrv_debug_event(BlockDriverState *bs,
> BlkDebugEvent event)
> /**************************************************************/
> /* handling of snapshots */
>
> -int bdrv_snapshot_create(BlockDriverState *bs,
> - QEMUSnapshotInfo *sn_info)
> +int bdrv_can_snapshot(BlockDriverState *bs)
> {
> BlockDriver *drv = bs->drv;
> - if (!drv)
> + if (!drv) {
> + return -ENOMEDIUM;
> + }
> +
> + if (!drv->bdrv_snapshot_create || bdrv_is_removable(bs) ||
> + bdrv_is_read_only(bs)) {
> + return -ENOTSUP;
> + }
> +
> + return 1;
> +}
Returning either 1 or -errno is a strange interface. I'm not sure which
of 1/0 or 0/-errno is better in this case, but I'd suggest to take one
of these.
> +int bdrv_has_snapshot(BlockDriverState *bs)
> +{
> + int ret;
> + QEMUSnapshotInfo *sn_tab;
> + BlockDriver *drv = bs->drv;
> + if (!drv) {
> return -ENOMEDIUM;
> - if (!drv->bdrv_snapshot_create)
> + }
> +
> + if (!drv->bdrv_snapshot_list) {
> return -ENOTSUP;
> - return drv->bdrv_snapshot_create(bs, sn_info);
> + }
> +
> + ret = drv->bdrv_snapshot_list(bs, &sn_tab);
> +
> + if (sn_tab) {
> + qemu_free(sn_tab);
> + }
> +
> + return ret;
> +}
> +
> +int bdrv_snapshot_create(BlockDriverState *bs,
> + QEMUSnapshotInfo *sn_info)
> +{
> + BlockDriver *drv = bs->drv;
> + if (bdrv_can_snapshot(bs) > 0) {
> + return drv->bdrv_snapshot_create(bs, sn_info);
> + }
> +
> + return -1;
> }
>
> int bdrv_snapshot_goto(BlockDriverState *bs,
> diff --git a/block.h b/block.h
> index 24efeb6..c536f1c 100644
> --- a/block.h
> +++ b/block.h
> @@ -173,6 +173,8 @@ 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_has_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 dc20390..9bc232f 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1574,22 +1574,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;
> @@ -1599,7 +1583,7 @@ 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) > 0)
> goto ok;
> }
> return NULL;
> @@ -1642,7 +1626,7 @@ static int del_existing_snapshots(Monitor *mon, const
> char *name)
>
> QTAILQ_FOREACH(dinfo, &drives, next) {
> bs = dinfo->bdrv;
> - if (bdrv_can_snapshot(bs) &&
> + if ((bdrv_can_snapshot(bs) > 0) &&
> bdrv_snapshot_find(bs, snapshot, name) >= 0)
> {
> ret = bdrv_snapshot_delete(bs, name);
> @@ -1674,12 +1658,30 @@ void do_savevm(Monitor *mon, const QDict *qdict)
> #endif
> const char *name = qdict_get_try_str(qdict, "name");
>
> + /* Verify if there is have a device that doesn't support snapshots and
> is writable*/
> + ret = 0;
> + QTAILQ_FOREACH(dinfo, &drives, next) {
> + bs = dinfo->bdrv;
> +
> + if (bdrv_is_removable(bs) || bdrv_is_read_only(bs)) {
> + continue;
> + }
> +
> + if ((ret = bdrv_can_snapshot(bs)) < 0) {
This may overwrite the error of the previous loop iteration with a
success return value. It's probably not what you want.
Other than that it looks good to me.
Kevin