diff mbox

[Qemu-block,v1,1/1] qcow2 resize with snapshots

Message ID 1E3A05EF-C0F1-48FD-A967-657D9B61C5E2@meituan.com
State New
Headers show

Commit Message

zhangzhiming May 19, 2016, 7:46 a.m. UTC 
hi, i wrote some code for 'qcow2 resize' with snapshot with v3 image and 'qcow2 goto’ too, 
different size of snapshots are supported. 
and i have tested the function and it  seems work well.
there are some code copied from snapshot_delete_blkdev_internal, and qmp_block_resize, 
it feels not very good.

please review it for me. thanks.

zhangzhiming
zhangzhiming02@meituan.com

--


--

Comments

Eric Blake May 19, 2016, 2:29 p.m. UTC | #1 
On 05/19/2016 01:46 AM, zhangzhiming wrote:

First, a disclaimer: thanks for writing a patch, and for trying to
improve the code.  The rest of this mail may sound negative, but take it
as advice for how to make your efforts more likely to succeed.

In the subject line, it looks like you manually added "[Qemu-block]",
but did not actually CC: qemu-block@nongnu.org.  Don't manually add
that, the list does it on your behalf if you actually send to the
intended lists (any patch mail that goes to qemu-block should also be
sent to qemu-devel).

> hi, i wrote some code for 'qcow2 resize' with snapshot with v3 image and 'qcow2 goto’ too, 
> different size of snapshots are supported. 
> and i have tested the function and it  seems work well.
> there are some code copied from snapshot_delete_blkdev_internal, and qmp_block_resize, 
> it feels not very good.
> 
> please review it for me. thanks.

It's implied that any patch sent to the list will be reviewed.  And this
particular sentence doesn't add any value once the patch is applied to
git.  It might be better after the '---' separator (information to the
reviewer that this is one of your first submissions, and needs extra
care in the review) rather than as part of the commit message proper, if
it is needed at all.

> 
> zhangzhiming
> zhangzhiming02@meituan.com

Unfortunately, your patch is missing a Signed-off-by, which is an
absolute must before it can be accepted.  Also, I noticed you sent
several followups; it's better to polish your patch series and send a v2
with all the pieces in one coherent set (perhaps split across multiple
patches) than it is to make reviewers string together multiple emails
into a single patch.

At this point, I'm just doing a high-level stylistic review, not even
caring about content or design, since you have a number of things that
need fixing before the patch can be considered ready.

More tips on proper patch submission:

http://wiki.qemu.org/Contribute/SubmitAPatch

> 
> --
> 
> diff --git a/block.c b/block.c

Your patch is missing a diffstat, which is a very handy tool in giving a
quick summary of what you touch below.

> index 18a497f..047698a 100644
> --- a/block.c
> +++ b/block.c
> @@ -2632,6 +2632,24 @@ int bdrv_truncate(BlockDriverState *bs, int64_t offset)
>  }
>  
>  /**
> + *  goto a snapshot

Not the most informative of comments, and redundant with the function
name. In this case, the function name is self-descriptive enough that
you might be able to get away without a comment, although a comment
probably is better.

> + */
> +int bdrv_apply_snapshot(BlockDriverState *bs, const char *snapshot_id, uint64_t snapshot_size)

Long line. ./scripts/checkpatch.pl should have flagged it as needing
line wrapping.

> +{
> +    int ret = bdrv_snapshot_goto(bs, snapshot_id);
> +    if(ret < 0){

Coding style violations (again, checkpatch is your friend).  Space after
'if', and before '{'.

Since this function can fail, it should probably take an Error **errp
parameter to pass a detailed message about the failure back to the
caller, rather than just an error code with no further information.

> +        return ret;
> +    }
> +
> +    ret = refresh_total_sectors(bs, snapshot_size);
> +    bdrv_dirty_bitmap_truncate(bs);

Is it safe to call this even if ret indicates that
refresh_total_sectors() failed?

> +    if (bs->blk) {
> +        blk_dev_resize_cb(bs->blk);

Can bdrv_dirty_bitmap_truncate() or blk_dev_resize_cb() fail? If so,
what error recovery should you take?

> +++ b/block/qcow2.c
> @@ -2501,15 +2501,17 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
>          return -EINVAL;
>      }
>  
> -    /* cannot proceed if image has snapshots */
> -    if (s->nb_snapshots) {
> -        error_report("Can't resize an image which has snapshots");
> +    bool v3_truncate = (s->qcow_version == 3);
> +
> +    /* cannot proceed if image has snapshots and qcow_version is not 3*/

Space before */

> +    if (!v3_truncate && s->nb_snapshots) {
> +        error_report("Can't resize an image which has snapshots and qcow_version is not 3");

Long line

>          return -ENOTSUP;
>      }
>  
> -    /* shrinking is currently not supported */
> -    if (offset < bs->total_sectors * 512) {
> -        error_report("qcow2 doesn't support shrinking images yet");
> +    /* shrinking is supported from version 3*/
> +    if (!v3_truncate && offset < bs->total_sectors * 512) {
> +        error_report("qcow2 doesn't support shrinking images yet while qcow_version is not 3");

And again

> +++ b/blockdev.c
> @@ -2961,6 +2961,120 @@ out:
>      aio_context_release(aio_context);
>  }
>  
> +SnapshotInfo *qmp_blockdev_snapshot_goto_internal_sync(const char *device,
> +                             bool has_id,

Alignment is off.


> +
> +    if(!has_id){

Multiple style problems throughout the function.

> +++ b/hmp-commands.hx
> @@ -1159,6 +1159,24 @@ Delete an internal snapshot on device if it support
>  ETEXI
>  
>      {
> +        .name       = "snapshot_goto_blkdev_internal",
> +        .args_type  = "device:B,name:s,id:s?",
> +        .params     = "device name [id]",
> +        .help       = "apply an internal snapshot of device.\n\t\t\t"
> +                      "If id is specified, qemu will try apply\n\t\t\t"
> +                      "the snapshot matching both id and name.\n\t\t\t"
> +                      "The format of the image used by device must\n\t\t\t"
> +                      "support it, such as qcow2.\n\t\t\t",
> +        .mhandler.cmd = hmp_snapshot_goto_blkdev_internal,

Why do we need a new command?  Especially one with the name 'internal'
in it?  Why are the existing commands not extensible to call the new
behavior?

> +++ b/pixman
> @@ -1 +1 @@
> -Subproject commit 87eea99e443b389c978cf37efc52788bf03a0ee0
> +Subproject commit 7c6066b700c7cdd4aeb8be426b14b3a5f0de4b6c

Oops, that should not be part of your commit.

Also, adding a new HMP command but no counterpart QMP command is fishy.
diff mbox

Patch

diff --git a/block.c b/block.c
index 18a497f..047698a 100644
--- a/block.c
+++ b/block.c
@@ -2632,6 +2632,24 @@  int bdrv_truncate(BlockDriverState *bs, int64_t offset)
 }
 
 /**
+ *  goto a snapshot
+ */
+int bdrv_apply_snapshot(BlockDriverState *bs, const char *snapshot_id, uint64_t snapshot_size)
+{
+    int ret = bdrv_snapshot_goto(bs, snapshot_id);
+    if(ret < 0){
+        return ret;
+    }
+
+    ret = refresh_total_sectors(bs, snapshot_size);
+    bdrv_dirty_bitmap_truncate(bs);
+    if (bs->blk) {
+        blk_dev_resize_cb(bs->blk);
+    }
+    return ret;
+}
+
+/**
  * Length of a allocated file in bytes. Sparse files are counted by actual
  * allocated space. Return < 0 if error or unknown.
  */
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 5f4a17e..9bc987f 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -477,13 +477,6 @@  int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
     }
     sn = &s->snapshots[snapshot_index];
 
-    if (sn->disk_size != bs->total_sectors * BDRV_SECTOR_SIZE) {
-        error_report("qcow2: Loading snapshots with different disk "
-            "size is not implemented");
-        ret = -ENOTSUP;
-        goto fail;
-    }
-
     /*
      * Make sure that the current L1 table is big enough to contain the whole
      * L1 table of the snapshot. If the snapshot L1 table is smaller, the
@@ -675,6 +668,7 @@  int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
         sn_info->date_sec = sn->date_sec;
         sn_info->date_nsec = sn->date_nsec;
         sn_info->vm_clock_nsec = sn->vm_clock_nsec;
+        sn_info->disk_size = sn->disk_size;
     }
     *psn_tab = sn_tab;
     return s->nb_snapshots;
diff --git a/block/qcow2.c b/block/qcow2.c
index 62febfc..6535f92 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2501,15 +2501,17 @@  static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
         return -EINVAL;
     }
 
-    /* cannot proceed if image has snapshots */
-    if (s->nb_snapshots) {
-        error_report("Can't resize an image which has snapshots");
+    bool v3_truncate = (s->qcow_version == 3);
+
+    /* cannot proceed if image has snapshots and qcow_version is not 3*/
+    if (!v3_truncate && s->nb_snapshots) {
+        error_report("Can't resize an image which has snapshots and qcow_version is not 3");
         return -ENOTSUP;
     }
 
-    /* shrinking is currently not supported */
-    if (offset < bs->total_sectors * 512) {
-        error_report("qcow2 doesn't support shrinking images yet");
+    /* shrinking is supported from version 3*/
+    if (!v3_truncate && offset < bs->total_sectors * 512) {
+        error_report("qcow2 doesn't support shrinking images yet while qcow_version is not 3");
         return -ENOTSUP;
     }
 
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 23fbace..bc12f7b 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2693,6 +2693,7 @@  static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
             sn_tab[found].date_nsec = inode.snap_ctime & 0xffffffff;
             sn_tab[found].vm_state_size = inode.vm_state_size;
             sn_tab[found].vm_clock_nsec = inode.vm_clock_nsec;
+            sn_tab[found].disk_size = inode.vdi_size;
 
             snprintf(sn_tab[found].id_str, sizeof(sn_tab[found].id_str),
                      "%" PRIu32, inode.snap_id);
diff --git a/blockdev.c b/blockdev.c
index 1892b8e..36c66c1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2961,6 +2961,120 @@  out:
     aio_context_release(aio_context);
 }
 
+SnapshotInfo *qmp_blockdev_snapshot_goto_internal_sync(const char *device,
+                             bool has_id,
+                             const char *id,
+                             bool has_name,
+                             const char *name,
+                             Error **errp)
+{
+    BlockDriverState *bs;
+    BlockBackend *blk;
+    AioContext *aio_context;
+    QEMUSnapshotInfo sn;
+    Error *local_err = NULL;
+    SnapshotInfo *info = NULL;
+    int ret;
+
+    blk = blk_by_name(device);
+    if (!blk) {
+        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+                "Device '%s' not found", device);
+        return NULL;
+    }
+    aio_context = blk_get_aio_context(blk);
+    aio_context_acquire(aio_context);
+
+    if(!has_id){
+        id = NULL;
+    }
+
+    if(!has_name){
+        name = NULL;
+    }
+
+    if(!id && !name){
+        error_setg(errp, "Name or id must be provided");
+        goto out_aio_context;
+    }
+
+    if(!blk_is_available(blk)){
+        error_setg(errp, "Device '%s' has no medium", device);
+        goto out_aio_context;
+    }
+
+    bs = blk_bs(blk);
+
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_APPLY, errp)){
+        goto out_aio_context;
+    }
+
+    ret = bdrv_snapshot_find_by_id_and_name(bs, id, name, &sn, &local_err);
+    if(local_err){
+        error_propagate(errp, local_err);
+        goto out_aio_context;
+    }
+    if(!ret){
+        error_setg(errp,
+                   "Snapshot with id '%s' and name '%s' does not exist on "
+                   "device '%s'",
+                   STR_OR_NULL(id), STR_OR_NULL(name), device);
+        goto out_aio_context;
+    }
+    if(!sn.disk_size){
+        error_setg(errp,
+                   "Snapshot with id '%s' and name '%s' does not has a disk size "
+                   "device '%s'",
+                   STR_OR_NULL(id), STR_OR_NULL(name), device);
+        goto out_aio_context;
+    }
+
+    /* complete all in-flight operations before resizing the device */
+    bdrv_drain_all();
+
+    ret = bdrv_apply_snapshot(bs, sn.id_str, sn.disk_size);
+    switch (ret) {
+    case 0:
+        break;
+    case -ENOMEDIUM:
+        error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+        break;
+    case -ENOTSUP:
+        error_setg(errp, QERR_UNSUPPORTED);
+        break;
+    case -EACCES:
+        error_setg(errp, "Device '%s' is read only", device);
+        break;
+    case -EBUSY:
+        error_setg(errp, QERR_DEVICE_IN_USE, device);
+        break;
+    default:
+        error_setg_errno(errp, -ret, "Could not resize");
+        break;
+    }
+
+    if(ret < 0){
+        goto out_aio_context;
+    }
+
+    aio_context_release(aio_context);
+
+    info = g_new0(SnapshotInfo, 1);
+    info->id = g_strdup(sn.id_str);
+    info->name = g_strdup(sn.name);
+    info->date_nsec = sn.date_nsec;
+    info->date_sec = sn.date_sec;
+    info->vm_state_size = sn.vm_state_size;
+    info->vm_clock_nsec = sn.vm_clock_nsec % 1000000000;
+    info->vm_clock_sec = sn.vm_clock_nsec / 1000000000;
+
+    return info;
+
+out_aio_context:
+    aio_context_release(aio_context);
+    return NULL;
+}
+
 static void block_job_cb(void *opaque, int ret)
 {
     /* Note that this function may be executed from another AioContext besides
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 4f4f60a..5848a57 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1159,6 +1159,24 @@  Delete an internal snapshot on device if it support
 ETEXI
 
     {
+        .name       = "snapshot_goto_blkdev_internal",
+        .args_type  = "device:B,name:s,id:s?",
+        .params     = "device name [id]",
+        .help       = "apply an internal snapshot of device.\n\t\t\t"
+                      "If id is specified, qemu will try apply\n\t\t\t"
+                      "the snapshot matching both id and name.\n\t\t\t"
+                      "The format of the image used by device must\n\t\t\t"
+                      "support it, such as qcow2.\n\t\t\t",
+        .mhandler.cmd = hmp_snapshot_goto_blkdev_internal,
+    },
+
+STEXI
+@item snapshot_goto_blkdev_internal
+@findex snapshot_goto_blkdev_internal
+Apply an internal snapshot on device if it support
+ETEXI
+
+    {
         .name       = "drive_mirror",
         .args_type  = "reuse:-n,full:-f,device:B,target:s,format:s?",
         .params     = "[-n] [-f] device target [format]",
diff --git a/hmp.c b/hmp.c
index d510236..3f1d146 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1057,6 +1057,8 @@  void hmp_block_resize(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
+
+
 void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
 {
     const char *device = qdict_get_str(qdict, "device");
@@ -1163,6 +1165,17 @@  void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
+void hmp_snapshot_goto_blkdev_internal(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *name = qdict_get_str(qdict, "name");
+    const char *id = qdict_get_try_str(qdict, "id");
+    Error *err = NULL;
+
+    qmp_blockdev_snapshot_goto_internal_sync(device, !!id, id, true, name, &err);
+    hmp_handle_error(mon, &err);
+}
+
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
 {
     qmp_migrate_cancel(NULL);
diff --git a/hmp.h b/hmp.h
index 093d65f..b1ad2f5 100644
--- a/hmp.h
+++ b/hmp.h
@@ -59,6 +59,7 @@  void hmp_block_resize(Monitor *mon, const QDict *qdict);
 void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict);
 void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict);
 void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict);
+void hmp_snapshot_goto_blkdev_internal(Monitor *mon, const QDict *qdict);
 void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
 void hmp_drive_backup(Monitor *mon, const QDict *qdict);
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
diff --git a/include/block/block.h b/include/block/block.h
index b210832..393ca6b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -173,6 +173,7 @@  typedef enum BlockOpType {
     BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
     BLOCK_OP_TYPE_INTERNAL_SNAPSHOT,
     BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE,
+    BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_APPLY,
     BLOCK_OP_TYPE_MIRROR_SOURCE,
     BLOCK_OP_TYPE_MIRROR_TARGET,
     BLOCK_OP_TYPE_RESIZE,
@@ -266,6 +267,7 @@  BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
 int bdrv_get_backing_file_depth(BlockDriverState *bs);
 void bdrv_refresh_filename(BlockDriverState *bs);
 int bdrv_truncate(BlockDriverState *bs, int64_t offset);
+int bdrv_apply_snapshot(BlockDriverState *bs, const char *snapshot_id, uint64_t snapshot_size);
 int64_t bdrv_nb_sectors(BlockDriverState *bs);
 int64_t bdrv_getlength(BlockDriverState *bs);
 int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index e5c0553..7279c12 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -44,6 +44,7 @@  typedef struct QEMUSnapshotInfo {
     uint32_t date_sec; /* UTC date of the snapshot */
     uint32_t date_nsec;
     uint64_t vm_clock_nsec; /* VM clock relative to boot */
+    uint64_t disk_size;
 } QEMUSnapshotInfo;
 
 int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
diff --git a/pixman b/pixman
index 87eea99..7c6066b 160000
--- a/pixman
+++ b/pixman
@@ -1 +1 @@ 
-Subproject commit 87eea99e443b389c978cf37efc52788bf03a0ee0
+Subproject commit 7c6066b700c7cdd4aeb8be426b14b3a5f0de4b6c