From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=60276 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OLtyH-0007Zp-SK for qemu-devel@nongnu.org; Tue, 08 Jun 2010 04:15:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OLtyG-0006C3-HC for qemu-devel@nongnu.org; Tue, 08 Jun 2010 04:15:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47698) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OLtyG-0006Bt-AD for qemu-devel@nongnu.org; Tue, 08 Jun 2010 04:15:40 -0400 Message-ID: <4C0DFC16.2040601@redhat.com> Date: Tue, 08 Jun 2010 10:15:18 +0200 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH v4] savevm: Really verify if a drive supports snapshots References: <1275680159-30396-1-git-send-email-miguel.filho@gmail.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: MORITA Kazutaka Cc: Miguel Di Ciurcio Filho , qemu-devel@nongnu.org Am 08.06.2010 06:39, schrieb MORITA Kazutaka: > 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 >> --- >> 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; > } You're right that we need to consider protocols, but I think your fix isn't completely correct either. We should check bs->file only if !drv->bdrv_snapshot_create, the other options still mean that we can't snapshot. Miguel, looks like this needs a v5... Kevin