From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33547) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UkZx8-0004Uk-1i for qemu-devel@nongnu.org; Thu, 06 Jun 2013 09:10:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UkZx6-0001ia-HZ for qemu-devel@nongnu.org; Thu, 06 Jun 2013 09:10:05 -0400 Received: from mail-pd0-f182.google.com ([209.85.192.182]:54704) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UkZx6-0001fs-Br for qemu-devel@nongnu.org; Thu, 06 Jun 2013 09:10:04 -0400 Received: by mail-pd0-f182.google.com with SMTP id g10so3314473pdj.41 for ; Thu, 06 Jun 2013 06:10:03 -0700 (PDT) Message-ID: <51B08A25.30604@gmail.com> Date: Thu, 06 Jun 2013 21:09:57 +0800 From: Liu Yuan MIME-Version: 1.0 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> In-Reply-To: <20130606124629.GE2586@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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: Kevin Wolf Cc: Stefan Hajnoczi , sheepdog@lists.wpkg.org, qemu-devel@nongnu.org, MORITA Kazutaka 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 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. Thanks, Yuan