From patchwork Tue Jun 8 13:40:55 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Miguel Di Ciurcio Filho X-Patchwork-Id: 54996 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id DB024B7D59 for ; Wed, 9 Jun 2010 00:35:01 +1000 (EST) Received: from localhost ([127.0.0.1]:60920 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OLzcH-0007Xk-DI for incoming@patchwork.ozlabs.org; Tue, 08 Jun 2010 10:17:21 -0400 Received: from [140.186.70.92] (port=47629 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OLzEK-0002m3-Bu for qemu-devel@nongnu.org; Tue, 08 Jun 2010 09:52:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OLz3r-0007Fj-Is for qemu-devel@nongnu.org; Tue, 08 Jun 2010 09:41:48 -0400 Received: from mail-pv0-f173.google.com ([74.125.83.173]:51512) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OLz3r-0007FJ-Ah for qemu-devel@nongnu.org; Tue, 08 Jun 2010 09:41:47 -0400 Received: by pvg3 with SMTP id 3so405592pvg.4 for ; Tue, 08 Jun 2010 06:41:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:from:to:cc:subject:date :message-id:x-mailer; bh=r/GJ3BbUCLjl0sUdhfM9XLO2OvlhrgBL5UIb0wJbKok=; b=c9UfQ+Q0ByPZidd+Xgjs0X5CGx2RdlR8xVvA5WGrhB3ad/e6c5p8mPT4scmkhYoVQ+ 4pOybgAjx4PLY3a+0+4P1w/P4DwQmrX8AJcvNVzqw7bJ+vHA3SkRrzr15DodBtMb//Zo HcQbnbmK2n5RQvdIq1YFgn3nuoAr8FNLaAKR8= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=from:to:cc:subject:date:message-id:x-mailer; b=frCtBlps/2a6zEK3lIi+2vB96kjAAhZJb4rQop6lB+yhjEEVTDMhwNneK0cFzERm4K Q6CyTD3wkicloUT4PPzxKw5AJm13YArwB5fufs4W0Ou8QZx+P3/7TpFg0DpsXnt4yWU/ Yu+vOu03O8mj2YRiqY1tHR8Gfi9KKamZfYTIc= Received: by 10.141.213.27 with SMTP id p27mr1498693rvq.155.1276004505561; Tue, 08 Jun 2010 06:41:45 -0700 (PDT) Received: from localhost.localdomain ([189.61.230.202]) by mx.google.com with ESMTPS id k17sm5766320rvh.5.2010.06.08.06.41.41 (version=TLSv1/SSLv3 cipher=RC4-MD5); Tue, 08 Jun 2010 06:41:43 -0700 (PDT) From: Miguel Di Ciurcio Filho To: qemu-devel@nongnu.org Date: Tue, 8 Jun 2010 10:40:55 -0300 Message-Id: <1276004455-5803-1-git-send-email-miguel.filho@gmail.com> X-Mailer: git-send-email 1.7.1 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) Cc: kwolf@redhat.com, Miguel Di Ciurcio Filho Subject: [Qemu-devel] [PATCH v5] savevm: Really verify if a drive supports snapshots X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 --- 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)); }