From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55621) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ukr98-0000V4-Vj for qemu-devel@nongnu.org; Fri, 07 Jun 2013 03:31:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ukr94-0001fU-AT for qemu-devel@nongnu.org; Fri, 07 Jun 2013 03:31:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58558) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ukr94-0001fP-3V for qemu-devel@nongnu.org; Fri, 07 Jun 2013 03:31:34 -0400 Date: Fri, 7 Jun 2013 09:31:18 +0200 From: Kevin Wolf Message-ID: <20130607073118.GA3658@dhcp-200-207.str.redhat.com> References: <1370519849-10620-1-git-send-email-namei.unix@gmail.com> <1370519849-10620-3-git-send-email-namei.unix@gmail.com> <20130606124629.GE2586@dhcp-200-207.str.redhat.com> <51B08A25.30604@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <51B08A25.30604@gmail.com> Subject: Re: [Qemu-devel] [PATCH 2/2] sheepdog: support 'qemu-img snapshot -a' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Liu Yuan Cc: Stefan Hajnoczi , sheepdog@lists.wpkg.org, qemu-devel@nongnu.org, MORITA Kazutaka Am 06.06.2013 um 15:09 hat Liu Yuan geschrieben: > On 06/06/2013 08:46 PM, Kevin Wolf wrote: > > Am 06.06.2013 um 13:57 hat Liu Yuan geschrieben: > >> Just call sd_create_branch() to rollback the image is good enough > >> > >> Cc: qemu-devel@nongnu.org > >> Cc: MORITA Kazutaka > >> Cc: Kevin Wolf > >> Cc: Stefan Hajnoczi > >> Signed-off-by: Liu Yuan > >> --- > >> block/sheepdog.c | 8 +++++--- > >> 1 file changed, 5 insertions(+), 3 deletions(-) > >> > >> diff --git a/block/sheepdog.c b/block/sheepdog.c > >> index 94218ac..cb5ca4a 100644 > >> --- a/block/sheepdog.c > >> +++ b/block/sheepdog.c > >> @@ -2072,9 +2072,11 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id) > >> } > >> > >> if (!s->inode.vm_state_size) { > >> - error_report("Invalid snapshot"); > >> - ret = -ENOENT; > >> - goto out; > >> + /* qemu-img asks us to rollback, we need to do it right now */ > >> + ret = sd_create_branch(s); > >> + if (ret) { > >> + goto out; > >> + } > >> } > > > > I'm not sure how snapshots work internally for Sheepdog, but it seems > > odd to me that you need to do this only for disk-only snapshots, but not > > when the snapshot has VM state. (Also, note that 'qemu-img snapshot -a' > > works on images with a VM state, so the comment doesn't seem to be > > completely accurate) > > > > Would you mind explaining to me how this works in detail? > > > > Hmm, the original code isn't written by me and this snapshot mechanism > exists since day 0. I just hacks it to work now. So I'll try to explain > on my understanding. > > When we do a snapshot such as 'savedvm' or 'qemu-img snapshot', the > active vdi is snapshotted and marked as snapshot and a new vdi is > created as copy-on-write on the previous active vdi, then this new vdi > becomes active vdi. For e.g, > > As1 --> As2 --> A > > We take snapshot of vdi A twice, tagged s1 and s2 respectively. I guess > this is quit similar to qcow2 snapshots, only inode object with a bitmap > is created. > > So when we 'loadvm' or 'qemu-img snapshot -a' to A, current logic just > handle 'loadvm', that .bdrv_snapshot_goto only reloads inode object, > that is, for e.g, we 'savevm s1', and mark it as snapshot, the chain > would like > > As1 --> As2 --> A > | > v > just reload As1's inode object > > Only when the write comes from VM, we do the following stuff > - delete active vdi A > - created a new inode based on the previously reloaded As1's inode Thanks, this is the part that I missed. I'm not sure however why the actual switch is delayed until the first write. This seems inconsistent with qcow2 snapshots. Do you know if there is a reason why we can't always do this already during bdrv_snapshot_goto()? > The chain will look like: > > As1 --> As2 > | > V > A > > This is how sheepdog handles savevm/loadvm. > > So for 'qemu-img snapshot -a', we should create the branch in the > .bdrv_snapshot_goto. > > As you pointed out, we need to consider vm state even for 'snapshot -a', > so I need to rework the patch 2/2. Yes, the presence of VM state is independent from whether you're using qemu-img or loadvm. And it actually goes both ways: qemu-img can revert to snapshots that have a VM state, and loadvm can be used with images that don't have a VM state (if you have multiple images, only one of them has the VM state). Kevin