[Qemu-devel] [PATCH v4 0/2] fix 'qemu-img snapshot -a' operation for sheepdog

[Qemu-devel] [PATCH v4 0/2] fix 'qemu-img snapshot -a' operation for sheepdog

[Liu Yuan]

v4:
 - fix savevm, pass current vdi_id instead of parent_vdi_id

v3:
 - fix sheepdog's loadvm handling, don't rely on the write to create branch

v2:
 - add the comment to make things more clear
 - call sd_create_branch() after s->is_snapshot = true because after calling
   sd_create_branch, it is not snapshot anymore.

Nothing big, just two simple patches to enable this commind for sheepdog.

Cc: qemu-devel@nongnu.org
Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>

Liu Yuan (2):
  sheepdog: fix snapshot tag initialization
  sheepdog: support 'qemu-img snapshot -a'

 block/sheepdog.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 1/2] sheepdog: fix snapshot tag initialization

[Liu Yuan]

This is an old and obvious bug. We should pass snapshot_id to the
tag. Or simple command like 'qemu-img snapshot -a tag sheepdog:image' will fail

Cc: qemu-devel@nongnu.org
Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Liu Yuan <namei.unix@gmail.com>
---
 block/sheepdog.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 21a4edf..94218ac 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2063,7 +2063,7 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     if (snapid) {
         tag[0] = 0;
     } else {
-        pstrcpy(tag, sizeof(tag), s->name);
+        pstrcpy(tag, sizeof(tag), snapshot_id);
     }
 
     ret = reload_inode(s, snapid, tag);
-- 
1.7.9.5

[Qemu-devel] [PATCH v4 2/2] sheepdog: support 'qemu-img snapshot -a'

[Liu Yuan]

Just call sd_create_branch() in the snapshot_goto to rollback the image is good
enough. With this patch, 'loadvm' process for sheepdog is modified:

Suppose we have a snapshot chain A --> B --> C, we do 'loadvm A' so as to get
a new chain,

A --> B
|
V
C1

in the old code:

1 reload inode of A (in snapshot_goto)
2 read vmstate via A's vdi_id (loadvm_state)
3 delete C and create C1, reload inode of C1 (sd_create_branch on write)

with this patch applied:

1 reload inode of A, delete C and create C1  (in snapshot_goto)
2 read vmstate via C1's parent, that is A's vdi_id (loadvm_state)

This will fix the possible bug that QEMU exit between 2 and 3 in the old code

Cc: qemu-devel@nongnu.org
Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Liu Yuan <namei.unix@gmail.com>
---
 block/sheepdog.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 94218ac..1b7c3f1 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2071,14 +2071,11 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
         goto out;
     }
 
-    if (!s->inode.vm_state_size) {
-        error_report("Invalid snapshot");
-        ret = -ENOENT;
+    ret = sd_create_branch(s);
+    if (ret) {
         goto out;
     }
 
-    s->is_snapshot = true;
-
     g_free(old_s);
 
     return 0;
@@ -2196,8 +2193,9 @@ static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data,
     int fd, ret = 0, remaining = size;
     unsigned int data_len;
     uint64_t vmstate_oid;
-    uint32_t vdi_index;
     uint64_t offset;
+    uint32_t vdi_index;
+    uint32_t vdi_id = load ? s->inode.parent_vdi_id : s->inode.vdi_id;
 
     fd = connect_to_sdog(s);
     if (fd < 0) {
@@ -2210,7 +2208,7 @@ static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data,
 
         data_len = MIN(remaining, SD_DATA_OBJ_SIZE - offset);
 
-        vmstate_oid = vid_to_vmstate_oid(s->inode.vdi_id, vdi_index);
+        vmstate_oid = vid_to_vmstate_oid(vdi_id, vdi_index);
 
         create = (offset == 0);
         if (load) {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v4 0/2] fix 'qemu-img snapshot -a' operation for sheepdog

[Kevin Wolf]

Am 07.06.2013 um 19:54 hat Liu Yuan geschrieben:
> v4:
>  - fix savevm, pass current vdi_id instead of parent_vdi_id
> 
> v3:
>  - fix sheepdog's loadvm handling, don't rely on the write to create branch
> 
> v2:
>  - add the comment to make things more clear
>  - call sd_create_branch() after s->is_snapshot = true because after calling
>    sd_create_branch, it is not snapshot anymore.
> 
> Nothing big, just two simple patches to enable this commind for sheepdog.
> 
> Cc: qemu-devel@nongnu.org
> Cc: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Liu Yuan (2):
>   sheepdog: fix snapshot tag initialization
>   sheepdog: support 'qemu-img snapshot -a'
> 
>  block/sheepdog.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Thanks, applied to the block branch.

Kevin