* [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level
@ 2013-08-07 3:00 Wenchao Xia
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 1/8] snapshot: new function bdrv_snapshot_find_by_id_and_name() Wenchao Xia
` (11 more replies)
0 siblings, 12 replies; 16+ messages in thread
From: Wenchao Xia @ 2013-08-07 3:00 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, famz, Wenchao Xia, armbru, lcapitulino, stefanha,
pbonzini, dietmar
This series brings internal snapshot support at block devices level, now we
have two three methods to do block snapshot lively: 1) backing chain,
2) internal one and 3) drive-back up approach.
Comparation:
Advantages: Disadvantages:
1) delta data, taken fast, export, size performance, delete slow.
2) taken fast, delete fast, performance, size delta data, format
3) performance, export, format taken slow, delta data, size, host I/O
I think in most case, saving vmstate in an standalone file is better than
saving it inside qcow2, So suggest treat internal snapshot as block level
methods and not encourage user to savevm in qcow2 any more.
Implemention details:
To avoid trouble, this serial have hide ID in create interfaces, this make
sure no chaos of ID and name will be introduced by these interfaces.
There is one patch may be common to Pavel's savvm transaction, patch 1/11,
others are not quite related. Patch 1/11 will not set errp when no snapshot
find, since patch 3/11 need to distinguish real error case.
Next steps to better full VM snapshot:
Improve internal snapshot's export capability.
Better vmstate saving.
Thanks Kevin to give advisement about how add it in qmp_transaction, oldest
version comes drom Dietmar Maurer.
v3:
General:
Rebased after Stenfan's driver-backup patch V6.
Address Eric's comments:
4/9: grammar fix and better doc.
5/9: parameter name is mandatory now. grammar fix.
6/9: redesiged interface: take both id and name as optional parameter, return
the deleted snapshot's info.
Address Stefan's comments:
4/9: add '' around %s in message. drop code comments about vm_clock.
9/9: better doc, refined the code and add more test case.
v4:
Address Stefan's comments:
4/9: use error_setg_errno() to show error reason for bdrv_snapshot_create(),
spell fix and better doc.
5/9: better doc.
6/9: remove spurious ';' in code, spell fix and better doc.
v5:
Address Kevin's comments:
3/8, 4/8, 8/8: remove the limit of numeric snapshot name.
General change:
4/8: use existing type as parameter in qapi schema.
v6:
Address Stefan's comments:
2/8: macro STR_PRINT_CHAR was renamed as STR_OR_NULL, and moved into patch 5,
since implement function in this patch do not printf snapshot id any more, as
Kevin's suggestion.
Address Kevin's comments:
2/8: remove device, id, name info in the error message, use error message in
existing caller. A new function bdrv_snapshot_delete_by_id_or_name() is added
to make the usage clear while keep logic unchanged.
3/8: remove device info in error message when name is empty. Use else if
after call of bdrv_snapshot_find_by_id_and_name().
Other:
2/8: refined the comments in code for bdrv_snapshot_delete().
3/8: in error reporting, change format from "reason is: '%s'" to
"reason is: %s".
v7:
rebased on upstream, target for 1.7.
Wenchao Xia (8):
1 snapshot: new function bdrv_snapshot_find_by_id_and_name()
2 snapshot: distinguish id and name in snapshot delete
3 qmp: add internal snapshot support in qmp_transaction
4 qmp: add interface blockdev-snapshot-internal-sync
5 qmp: add interface blockdev-snapshot-delete-internal-sync
6 hmp: add interface hmp_snapshot_blkdev_internal
7 hmp: add interface hmp_snapshot_delete_blkdev_internal
8 qemu-iotests: add 057 internal snapshot for block device test case
block/qcow2-snapshot.c | 55 +++++++---
block/qcow2.h | 5 +-
block/rbd.c | 21 ++++-
block/sheepdog.c | 5 +-
block/snapshot.c | 131 ++++++++++++++++++++++-
blockdev.c | 190 ++++++++++++++++++++++++++++++++
hmp-commands.hx | 37 ++++++-
hmp.c | 22 ++++
hmp.h | 2 +
include/block/block_int.h | 5 +-
include/block/snapshot.h | 14 +++-
include/qemu-common.h | 3 +
qapi-schema.json | 66 +++++++++++-
qemu-img.c | 11 ++-
qmp-commands.hx | 104 ++++++++++++++++--
savevm.c | 32 +++---
tests/qemu-iotests/057 | 259 ++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/057.out | 5 +
tests/qemu-iotests/group | 1 +
19 files changed, 914 insertions(+), 54 deletions(-)
create mode 100755 tests/qemu-iotests/057
create mode 100644 tests/qemu-iotests/057.out
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH V7 1/8] snapshot: new function bdrv_snapshot_find_by_id_and_name()
2013-08-07 3:00 [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level Wenchao Xia
@ 2013-08-07 3:00 ` Wenchao Xia
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 2/8] snapshot: distinguish id and name in snapshot delete Wenchao Xia
` (10 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Wenchao Xia @ 2013-08-07 3:00 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, famz, Wenchao Xia, armbru, lcapitulino, stefanha,
pbonzini, dietmar
To make it clear about id and name in searching, add this API
to distinguish them. Caller can choose to search by id or name,
*errp will be set only for exception.
Some code are modified based on Pavel's patch.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/snapshot.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++
include/block/snapshot.h | 6 ++++
2 files changed, 79 insertions(+), 0 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index 6c6d9de..481a3ee 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -48,6 +48,79 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
return ret;
}
+/**
+ * Look up an internal snapshot by @id and @name.
+ * @bs: block device to search
+ * @id: unique snapshot ID, or NULL
+ * @name: snapshot name, or NULL
+ * @sn_info: location to store information on the snapshot found
+ * @errp: location to store error, will be set only for exception
+ *
+ * This function will traverse snapshot list in @bs to search the matching
+ * one, @id and @name are the matching condition:
+ * If both @id and @name are specified, find the first one with id @id and
+ * name @name.
+ * If only @id is specified, find the first one with id @id.
+ * If only @name is specified, find the first one with name @name.
+ * if none is specified, abort().
+ *
+ * Returns: true when a snapshot is found and @sn_info will be filled, false
+ * when error or not found. If all operation succeed but no matching one is
+ * found, @errp will NOT be set.
+ */
+bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
+ const char *id,
+ const char *name,
+ QEMUSnapshotInfo *sn_info,
+ Error **errp)
+{
+ QEMUSnapshotInfo *sn_tab, *sn;
+ int nb_sns, i;
+ bool ret = false;
+
+ assert(id || name);
+
+ nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+ if (nb_sns < 0) {
+ error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
+ return false;
+ } else if (nb_sns == 0) {
+ return false;
+ }
+
+ if (id && name) {
+ for (i = 0; i < nb_sns; i++) {
+ sn = &sn_tab[i];
+ if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
+ *sn_info = *sn;
+ ret = true;
+ break;
+ }
+ }
+ } else if (id) {
+ for (i = 0; i < nb_sns; i++) {
+ sn = &sn_tab[i];
+ if (!strcmp(sn->id_str, id)) {
+ *sn_info = *sn;
+ ret = true;
+ break;
+ }
+ }
+ } else if (name) {
+ for (i = 0; i < nb_sns; i++) {
+ sn = &sn_tab[i];
+ if (!strcmp(sn->name, name)) {
+ *sn_info = *sn;
+ ret = true;
+ break;
+ }
+ }
+ }
+
+ g_free(sn_tab);
+ return ret;
+}
+
int bdrv_can_snapshot(BlockDriverState *bs)
{
BlockDriver *drv = bs->drv;
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index eaf61f0..9d06dc1 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -26,6 +26,7 @@
#define SNAPSHOT_H
#include "qemu-common.h"
+#include "qapi/error.h"
typedef struct QEMUSnapshotInfo {
char id_str[128]; /* unique snapshot id */
@@ -40,6 +41,11 @@ typedef struct QEMUSnapshotInfo {
int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
const char *name);
+bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
+ const char *id,
+ const char *name,
+ QEMUSnapshotInfo *sn_info,
+ Error **errp);
int bdrv_can_snapshot(BlockDriverState *bs);
int bdrv_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info);
--
1.7.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH V7 2/8] snapshot: distinguish id and name in snapshot delete
2013-08-07 3:00 [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level Wenchao Xia
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 1/8] snapshot: new function bdrv_snapshot_find_by_id_and_name() Wenchao Xia
@ 2013-08-07 3:00 ` Wenchao Xia
2013-09-10 12:36 ` Kevin Wolf
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 3/8] qmp: add internal snapshot support in qmp_transaction Wenchao Xia
` (9 subsequent siblings)
11 siblings, 1 reply; 16+ messages in thread
From: Wenchao Xia @ 2013-08-07 3:00 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, famz, Wenchao Xia, armbru, lcapitulino, stefanha,
pbonzini, dietmar
Snapshot creation actually already distinguish id and name since it take
a structured parameter *sn, but delete can't. Later an accurate delete
is needed in qmp_transaction abort and blockdev-snapshot-delete-sync,
so change its prototype. Also *errp is added to tip error, but return
value is kepted to let caller check what kind of error happens. Existing
caller for it are savevm, delvm and qemu-img, they are not impacted by
introducing a new function bdrv_snapshot_delete_by_id_or_name(), which
check the return value and do the operation again.
Before this patch:
For qcow2, it search id first then name to find the one to delete.
For rbd, it search name.
For sheepdog, it does nothing.
After this patch:
For qcow2, logic is the same by call it twice in caller.
For rbd, it always fails in delete with id, but still search for name
in second try, no change to user.
Some code for *errp is based on Pavel's patch.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
block/qcow2-snapshot.c | 55 ++++++++++++++++++++++++++++++------------
block/qcow2.h | 5 +++-
block/rbd.c | 21 +++++++++++++++-
block/sheepdog.c | 5 +++-
block/snapshot.c | 58 ++++++++++++++++++++++++++++++++++++++++++--
include/block/block_int.h | 5 +++-
include/block/snapshot.h | 8 +++++-
qemu-img.c | 11 +++++---
savevm.c | 32 +++++++++++++-----------
9 files changed, 157 insertions(+), 43 deletions(-)
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 0caac90..6a0ed6b 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -288,31 +288,47 @@ static void find_new_snapshot_id(BlockDriverState *bs,
snprintf(id_str, id_str_size, "%d", id_max + 1);
}
-static int find_snapshot_by_id(BlockDriverState *bs, const char *id_str)
+static int find_snapshot_by_id_and_name(BlockDriverState *bs,
+ const char *id,
+ const char *name)
{
BDRVQcowState *s = bs->opaque;
int i;
- for(i = 0; i < s->nb_snapshots; i++) {
- if (!strcmp(s->snapshots[i].id_str, id_str))
- return i;
+ if (id && name) {
+ for (i = 0; i < s->nb_snapshots; i++) {
+ if (!strcmp(s->snapshots[i].id_str, id) &&
+ !strcmp(s->snapshots[i].name, name)) {
+ return i;
+ }
+ }
+ } else if (id) {
+ for (i = 0; i < s->nb_snapshots; i++) {
+ if (!strcmp(s->snapshots[i].id_str, id)) {
+ return i;
+ }
+ }
+ } else if (name) {
+ for (i = 0; i < s->nb_snapshots; i++) {
+ if (!strcmp(s->snapshots[i].name, name)) {
+ return i;
+ }
+ }
}
+
return -1;
}
-static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name)
+static int find_snapshot_by_id_or_name(BlockDriverState *bs,
+ const char *id_or_name)
{
- BDRVQcowState *s = bs->opaque;
- int i, ret;
+ int ret;
- ret = find_snapshot_by_id(bs, name);
- if (ret >= 0)
+ ret = find_snapshot_by_id_and_name(bs, id_or_name, NULL);
+ if (ret >= 0) {
return ret;
- for(i = 0; i < s->nb_snapshots; i++) {
- if (!strcmp(s->snapshots[i].name, name))
- return i;
}
- return -1;
+ return find_snapshot_by_id_and_name(bs, NULL, id_or_name);
}
/* if no id is provided, a new one is constructed */
@@ -334,7 +350,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
}
/* Check that the ID is unique */
- if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
+ if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) {
return -EEXIST;
}
@@ -531,15 +547,19 @@ fail:
return ret;
}
-int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+int qcow2_snapshot_delete(BlockDriverState *bs,
+ const char *snapshot_id,
+ const char *name,
+ Error **errp)
{
BDRVQcowState *s = bs->opaque;
QCowSnapshot sn;
int snapshot_index, ret;
/* Search the snapshot */
- snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
+ snapshot_index = find_snapshot_by_id_and_name(bs, snapshot_id, name);
if (snapshot_index < 0) {
+ error_setg(errp, "Can't find the snapshot");
return -ENOENT;
}
sn = s->snapshots[snapshot_index];
@@ -551,6 +571,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
s->nb_snapshots--;
ret = qcow2_write_snapshots(bs);
if (ret < 0) {
+ error_setg(errp, "Failed to remove snapshot from snapshot list");
return ret;
}
@@ -568,6 +589,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
ret = qcow2_update_snapshot_refcount(bs, sn.l1_table_offset,
sn.l1_size, -1);
if (ret < 0) {
+ error_setg(errp, "Failed to free the cluster and L1 table");
return ret;
}
qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t),
@@ -576,6 +598,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
/* must update the copied flag on the current cluster offsets */
ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 0);
if (ret < 0) {
+ error_setg(errp, "Failed to update snapshot status in disk");
return ret;
}
diff --git a/block/qcow2.h b/block/qcow2.h
index dba9771..a9eba9a 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -411,7 +411,10 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors);
/* qcow2-snapshot.c functions */
int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
-int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
+int qcow2_snapshot_delete(BlockDriverState *bs,
+ const char *snapshot_id,
+ const char *name,
+ Error **errp);
int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name);
diff --git a/block/rbd.c b/block/rbd.c
index cb71751..6a98009 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -903,12 +903,31 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
}
static int qemu_rbd_snap_remove(BlockDriverState *bs,
- const char *snapshot_name)
+ const char *snapshot_id,
+ const char *snapshot_name,
+ Error **errp)
{
BDRVRBDState *s = bs->opaque;
int r;
+ if (!snapshot_name) {
+ error_setg(errp, "rbd need a valid snapshot name");
+ return -EINVAL;
+ }
+
+ /* If snapshot_id is specified, it must be equal to name, see
+ qemu_rbd_snap_list() */
+ if (snapshot_id && strcmp(snapshot_id, snapshot_name)) {
+ error_setg(errp,
+ "rbd do not support snapshot id, it should be NULL or "
+ "equal to snapshot name");
+ return -EINVAL;
+ }
+
r = rbd_snap_remove(s->image, snapshot_name);
+ if (r < 0) {
+ error_setg_errno(errp, -r, "Failed to remove the snapshot");
+ }
return r;
}
diff --git a/block/sheepdog.c b/block/sheepdog.c
index a506137..f57f565 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2089,7 +2089,10 @@ out:
return ret;
}
-static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+static int sd_snapshot_delete(BlockDriverState *bs,
+ const char *snapshot_id,
+ const char *name,
+ Error **errp)
{
/* FIXME: Delete specified snapshot id. */
return 0;
diff --git a/block/snapshot.c b/block/snapshot.c
index 481a3ee..eda5137 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -182,21 +182,73 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
return -ENOTSUP;
}
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+/**
+ * Delete an internal snapshot by @snapshot_id and @name.
+ * @bs: block device used in the operation
+ * @snapshot_id: unique snapshot ID, or NULL
+ * @name: snapshot name, or NULL
+ * @errp: location to store error
+ *
+ * If both @snapshot_id and @name are specified, delete the first one with
+ * id @snapshot_id and name @name.
+ * If only @snapshot_id is specified, delete the first one with id
+ * @snapshot_id.
+ * If only @name is specified, delete the first one with name @name.
+ * if none is specified, return -ENINVAL.
+ *
+ * Returns: 0 on success, -errno on failure. If @bs is not inserted, return
+ * -ENOMEDIUM. If @snapshot_id and @name are both NULL, return -EINVAL. If @bs
+ * does not support internal snapshot deletion, return -ENOTSUP. If @bs does
+ * not support parameter @snapshot_id or @name, or one of them is not correctly
+ * specified, return -EINVAL. If @bs can't find one matching @id and @name,
+ * return -ENOENT. If @errp != NULL, it will always be filled with error
+ * message on failure.
+ */
+int bdrv_snapshot_delete(BlockDriverState *bs,
+ const char *snapshot_id,
+ const char *name,
+ Error **errp)
{
BlockDriver *drv = bs->drv;
if (!drv) {
+ error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
return -ENOMEDIUM;
}
+ if (!snapshot_id && !name) {
+ error_setg(errp, "snapshot_id and name are both NULL");
+ return -EINVAL;
+ }
if (drv->bdrv_snapshot_delete) {
- return drv->bdrv_snapshot_delete(bs, snapshot_id);
+ return drv->bdrv_snapshot_delete(bs, snapshot_id, name, errp);
}
if (bs->file) {
- return bdrv_snapshot_delete(bs->file, snapshot_id);
+ return bdrv_snapshot_delete(bs->file, snapshot_id, name, errp);
}
+ error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+ drv->format_name, bdrv_get_device_name(bs),
+ "internal snapshot deletion");
return -ENOTSUP;
}
+void bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
+ const char *id_or_name,
+ Error **errp)
+{
+ int ret;
+ Error *local_err = NULL;
+
+ ret = bdrv_snapshot_delete(bs, id_or_name, NULL, &local_err);
+ if (ret == -ENOENT || ret == -EINVAL) {
+ error_free(local_err);
+ local_err = NULL;
+ ret = bdrv_snapshot_delete(bs, NULL, id_or_name, &local_err);
+ }
+
+ if (ret < 0) {
+ error_propagate(errp, local_err);
+ }
+}
+
int bdrv_snapshot_list(BlockDriverState *bs,
QEMUSnapshotInfo **psn_info)
{
diff --git a/include/block/block_int.h b/include/block/block_int.h
index e45f2a0..323deb0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -166,7 +166,10 @@ struct BlockDriver {
QEMUSnapshotInfo *sn_info);
int (*bdrv_snapshot_goto)(BlockDriverState *bs,
const char *snapshot_id);
- int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);
+ int (*bdrv_snapshot_delete)(BlockDriverState *bs,
+ const char *snapshot_id,
+ const char *name,
+ Error **errp);
int (*bdrv_snapshot_list)(BlockDriverState *bs,
QEMUSnapshotInfo **psn_info);
int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 9d06dc1..012bf22 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -51,7 +51,13 @@ int bdrv_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info);
int bdrv_snapshot_goto(BlockDriverState *bs,
const char *snapshot_id);
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
+int bdrv_snapshot_delete(BlockDriverState *bs,
+ const char *snapshot_id,
+ const char *name,
+ Error **errp);
+void bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
+ const char *id_or_name,
+ Error **errp);
int bdrv_snapshot_list(BlockDriverState *bs,
QEMUSnapshotInfo **psn_info);
int bdrv_snapshot_load_tmp(BlockDriverState *bs,
diff --git a/qemu-img.c b/qemu-img.c
index c55ca5c..4e05c48 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1791,6 +1791,7 @@ static int img_snapshot(int argc, char **argv)
int action = 0;
qemu_timeval tv;
bool quiet = false;
+ Error *err = NULL;
bdrv_oflags = BDRV_O_FLAGS | BDRV_O_RDWR;
/* Parse commandline parameters */
@@ -1883,10 +1884,12 @@ static int img_snapshot(int argc, char **argv)
break;
case SNAPSHOT_DELETE:
- ret = bdrv_snapshot_delete(bs, snapshot_name);
- if (ret) {
- error_report("Could not delete snapshot '%s': %d (%s)",
- snapshot_name, ret, strerror(-ret));
+ bdrv_snapshot_delete_by_id_or_name(bs, snapshot_name, &err);
+ if (error_is_set(&err)) {
+ error_report("Could not delete snapshot '%s': (%s)",
+ snapshot_name, error_get_pretty(err));
+ error_free(err);
+ ret = 1;
}
break;
}
diff --git a/savevm.c b/savevm.c
index 03fc4d9..0808414 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2325,18 +2325,21 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
{
BlockDriverState *bs;
QEMUSnapshotInfo sn1, *snapshot = &sn1;
- int ret;
+ Error *err = NULL;
bs = NULL;
while ((bs = bdrv_next(bs))) {
if (bdrv_can_snapshot(bs) &&
bdrv_snapshot_find(bs, snapshot, name) >= 0)
{
- ret = bdrv_snapshot_delete(bs, name);
- if (ret < 0) {
+ bdrv_snapshot_delete_by_id_or_name(bs, name, &err);
+ if (error_is_set(&err)) {
monitor_printf(mon,
- "Error while deleting snapshot on '%s'\n",
- bdrv_get_device_name(bs));
+ "Error while deleting snapshot on device '%s', "
+ "reason: %s\n",
+ bdrv_get_device_name(bs),
+ error_get_pretty(err));
+ error_free(err);
return -1;
}
}
@@ -2550,7 +2553,7 @@ int load_vmstate(const char *name)
void do_delvm(Monitor *mon, const QDict *qdict)
{
BlockDriverState *bs, *bs1;
- int ret;
+ Error *err = NULL;
const char *name = qdict_get_str(qdict, "name");
bs = find_vmstate_bs();
@@ -2562,15 +2565,14 @@ void do_delvm(Monitor *mon, const QDict *qdict)
bs1 = NULL;
while ((bs1 = bdrv_next(bs1))) {
if (bdrv_can_snapshot(bs1)) {
- ret = bdrv_snapshot_delete(bs1, name);
- if (ret < 0) {
- if (ret == -ENOTSUP)
- monitor_printf(mon,
- "Snapshots not supported on device '%s'\n",
- bdrv_get_device_name(bs1));
- else
- monitor_printf(mon, "Error %d while deleting snapshot on "
- "'%s'\n", ret, bdrv_get_device_name(bs1));
+ bdrv_snapshot_delete_by_id_or_name(bs, name, &err);
+ if (error_is_set(&err)) {
+ monitor_printf(mon,
+ "Error while deleting snapshot on device '%s', "
+ "reason: %s\n",
+ bdrv_get_device_name(bs),
+ error_get_pretty(err));
+ error_free(err);
}
}
}
--
1.7.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH V7 3/8] qmp: add internal snapshot support in qmp_transaction
2013-08-07 3:00 [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level Wenchao Xia
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 1/8] snapshot: new function bdrv_snapshot_find_by_id_and_name() Wenchao Xia
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 2/8] snapshot: distinguish id and name in snapshot delete Wenchao Xia
@ 2013-08-07 3:00 ` Wenchao Xia
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 4/8] qmp: add interface blockdev-snapshot-internal-sync Wenchao Xia
` (8 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Wenchao Xia @ 2013-08-07 3:00 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, famz, Wenchao Xia, armbru, lcapitulino, stefanha,
pbonzini, dietmar
Unlike savevm, the qmp_transaction interface will not generate
snapshot name automatically, saving trouble to return information
of the new created snapshot.
Although qcow2 support storing multiple snapshots with same name
but different ID, here it will fail when an snapshot with that name
already exist before the operation. Format such as rbd do not support
ID at all, and in most case, it means trouble to user when he faces
multiple snapshots with same name, so ban that case. Request with
empty name will be rejected.
Snapshot ID can't be specified in this interface.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
blockdev.c | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
qapi-schema.json | 19 ++++++++-
qmp-commands.hx | 34 ++++++++++++----
3 files changed, 160 insertions(+), 9 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 41b0a49..0d717bc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -887,6 +887,117 @@ struct BlkTransactionState {
QSIMPLEQ_ENTRY(BlkTransactionState) entry;
};
+/* internal snapshot private data */
+typedef struct InternalSnapshotState {
+ BlkTransactionState common;
+ BlockDriverState *bs;
+ QEMUSnapshotInfo sn;
+} InternalSnapshotState;
+
+static void internal_snapshot_prepare(BlkTransactionState *common,
+ Error **errp)
+{
+ const char *device;
+ const char *name;
+ BlockDriverState *bs;
+ QEMUSnapshotInfo old_sn, *sn;
+ bool ret;
+ qemu_timeval tv;
+ BlockdevSnapshotInternal *internal;
+ InternalSnapshotState *state;
+ int ret1;
+
+ g_assert(common->action->kind ==
+ TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC);
+ internal = common->action->blockdev_snapshot_internal_sync;
+ state = DO_UPCAST(InternalSnapshotState, common, common);
+
+ /* 1. parse input */
+ device = internal->device;
+ name = internal->name;
+
+ /* 2. check for validation */
+ bs = bdrv_find(device);
+ if (!bs) {
+ error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+ return;
+ }
+
+ if (!bdrv_is_inserted(bs)) {
+ error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+ return;
+ }
+
+ if (bdrv_is_read_only(bs)) {
+ error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
+ return;
+ }
+
+ if (!bdrv_can_snapshot(bs)) {
+ error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+ bs->drv->format_name, device, "internal snapshot");
+ return;
+ }
+
+ if (!strlen(name)) {
+ error_setg(errp, "Name is empty");
+ return;
+ }
+
+ /* check whether a snapshot with name exist */
+ ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &old_sn, errp);
+ if (error_is_set(errp)) {
+ return;
+ } else if (ret) {
+ error_setg(errp,
+ "Snapshot with name '%s' already exists on device '%s'",
+ name, device);
+ return;
+ }
+
+ /* 3. take the snapshot */
+ sn = &state->sn;
+ pstrcpy(sn->name, sizeof(sn->name), name);
+ qemu_gettimeofday(&tv);
+ sn->date_sec = tv.tv_sec;
+ sn->date_nsec = tv.tv_usec * 1000;
+ sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
+
+ ret1 = bdrv_snapshot_create(bs, sn);
+ if (ret1 < 0) {
+ error_setg_errno(errp, -ret1,
+ "Failed to create snapshot '%s' on device '%s'",
+ name, device);
+ return;
+ }
+
+ /* 4. succeed, mark a snapshot is created */
+ state->bs = bs;
+}
+
+static void internal_snapshot_abort(BlkTransactionState *common)
+{
+ InternalSnapshotState *state =
+ DO_UPCAST(InternalSnapshotState, common, common);
+ BlockDriverState *bs = state->bs;
+ QEMUSnapshotInfo *sn = &state->sn;
+ Error *local_error = NULL;
+
+ if (!bs) {
+ return;
+ }
+
+ if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) {
+ error_report("Failed to delete snapshot with id '%s' and name '%s' on "
+ "device '%s' in abort, reason is: %s",
+ sn->id_str,
+ sn->name,
+ bdrv_get_device_name(bs),
+ error_get_pretty(local_error));
+ error_free(local_error);
+ }
+}
+
/* external snapshot private data */
typedef struct ExternalSnapshotState {
BlkTransactionState common;
@@ -1070,6 +1181,11 @@ static const BdrvActionOps actions[] = {
.prepare = abort_prepare,
.commit = abort_commit,
},
+ [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC] = {
+ .instance_size = sizeof(InternalSnapshotState),
+ .prepare = internal_snapshot_prepare,
+ .abort = internal_snapshot_abort,
+ },
};
/*
diff --git a/qapi-schema.json b/qapi-schema.json
index a51f7d2..38663c5 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1639,6 +1639,22 @@
'*mode': 'NewImageMode' } }
##
+# @BlockdevSnapshotInternal
+#
+# @device: the name of the device to generate the snapshot from
+#
+# @name: the name of the internal snapshot to be created
+#
+# Notes: In transaction, if @name is empty, or any snapshot matching @name
+# exists, the operation will fail. Only some image formats support it,
+# for example, qcow2, rbd, and sheepdog.
+#
+# Since: 1.7
+##
+{ 'type': 'BlockdevSnapshotInternal',
+ 'data': { 'device': 'str', 'name': 'str' } }
+
+##
# @DriveBackup
#
# @device: the name of the device which should be copied.
@@ -1700,7 +1716,8 @@
'data': {
'blockdev-snapshot-sync': 'BlockdevSnapshot',
'drive-backup': 'DriveBackup',
- 'abort': 'Abort'
+ 'abort': 'Abort',
+ 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
} }
##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 2e59b0d..68a1611 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1001,14 +1001,15 @@ SQMP
transaction
-----------
-Atomically operate on one or more block devices. The only supported
-operation for now is snapshotting. If there is any failure performing
-any of the operations, all snapshots for the group are abandoned, and
-the original disks pre-snapshot attempt are used.
+Atomically operate on one or more block devices. The only supported operations
+for now are drive-backup, internal and external snapshotting. A list of
+dictionaries is accepted, that contains the actions to be performed.
+If there is any failure performing any of the operations, all operations
+for the group are abandoned.
-A list of dictionaries is accepted, that contains the actions to be performed.
-For snapshots this is the device, the file to use for the new snapshot,
-and the format. The default format, if not specified, is qcow2.
+For external snapshots, the dictionary contains the device, the file to use for
+the new snapshot, and the format. The default format, if not specified, is
+qcow2.
Each new snapshot defaults to being created by QEMU (wiping any
contents if the file already exists), but it is also possible to reuse
@@ -1017,6 +1018,17 @@ the new image file has the same contents as the current one; QEMU cannot
perform any meaningful check. Typically this is achieved by using the
current image file as the backing file for the new image.
+On failure, the original disks pre-snapshot attempt will be used.
+
+For internal snapshots, the dictionary contains the device and the snapshot's
+name. If an internal snapshot matching name already exists, the request will
+be rejected. Only some image formats support it, for example, qcow2, rbd,
+and sheepdog.
+
+On failure, qemu will try delete the newly created internal snapshot in the
+transaction. When an I/O error occurs during deletion, the user needs to fix
+it later with qemu-img or other command.
+
Arguments:
actions array:
@@ -1029,6 +1041,9 @@ actions array:
- "format": format of new image (json-string, optional)
- "mode": whether and how QEMU should create the snapshot file
(NewImageMode, optional, default "absolute-paths")
+ When "type" is "blockdev-snapshot-internal-sync":
+ - "device": device name to snapshot (json-string)
+ - "name": name of the new snapshot (json-string)
Example:
@@ -1040,7 +1055,10 @@ Example:
{ 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd1",
"snapshot-file": "/some/place/my-image2",
"mode": "existing",
- "format": "qcow2" } } ] } }
+ "format": "qcow2" } },
+ { 'type': 'blockdev-snapshot-internal-sync', 'data' : {
+ "device": "ide-hd2",
+ "name": "snapshot0" } } ] } }
<- { "return": {} }
EQMP
--
1.7.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH V7 4/8] qmp: add interface blockdev-snapshot-internal-sync
2013-08-07 3:00 [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level Wenchao Xia
` (2 preceding siblings ...)
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 3/8] qmp: add internal snapshot support in qmp_transaction Wenchao Xia
@ 2013-08-07 3:00 ` Wenchao Xia
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 5/8] qmp: add interface blockdev-snapshot-delete-internal-sync Wenchao Xia
` (7 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Wenchao Xia @ 2013-08-07 3:00 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, famz, Wenchao Xia, armbru, lcapitulino, stefanha,
pbonzini, dietmar
Snapshot ID can't be specified in this interface.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
blockdev.c | 13 +++++++++++++
qapi-schema.json | 20 ++++++++++++++++++++
qmp-commands.hx | 29 +++++++++++++++++++++++++++++
3 files changed, 62 insertions(+), 0 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 0d717bc..b4754ef 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -856,6 +856,19 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
&snapshot, errp);
}
+void qmp_blockdev_snapshot_internal_sync(const char *device,
+ const char *name,
+ Error **errp)
+{
+ BlockdevSnapshotInternal snapshot = {
+ .device = (char *) device,
+ .name = (char *) name
+ };
+
+ blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC,
+ &snapshot, errp);
+}
+
/* New and old BlockDriverState structs for group snapshots */
diff --git a/qapi-schema.json b/qapi-schema.json
index 38663c5..a236cde 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1758,6 +1758,26 @@
'data': 'BlockdevSnapshot' }
##
+# @blockdev-snapshot-internal-sync
+#
+# Synchronously take an internal snapshot of a block device, when the format
+# of the image used supports it.
+#
+# For the arguments, see the documentation of BlockdevSnapshotInternal.
+#
+# Returns: nothing on success
+# If @device is not a valid block device, DeviceNotFound
+# If any snapshot matching @name exists, or @name is empty,
+# GenericError
+# If the format of the image used does not support it,
+# BlockFormatFeatureNotSupported
+#
+# Since 1.7
+##
+{ 'command': 'blockdev-snapshot-internal-sync',
+ 'data': 'BlockdevSnapshotInternal' }
+
+##
# @human-monitor-command:
#
# Execute a command on the human monitor and return the output.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 68a1611..6097283 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1098,6 +1098,35 @@ Example:
EQMP
{
+ .name = "blockdev-snapshot-internal-sync",
+ .args_type = "device:B,name:s",
+ .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_internal_sync,
+ },
+
+SQMP
+blockdev-snapshot-internal-sync
+-------------------------------
+
+Synchronously take an internal snapshot of a block device when the format of
+image used supports it. If the name is an empty string, or a snapshot with
+name already exists, the operation will fail.
+
+Arguments:
+
+- "device": device name to snapshot (json-string)
+- "name": name of the new snapshot (json-string)
+
+Example:
+
+-> { "execute": "blockdev-snapshot-internal-sync",
+ "arguments": { "device": "ide-hd0",
+ "name": "snapshot0" }
+ }
+<- { "return": {} }
+
+EQMP
+
+ {
.name = "drive-mirror",
.args_type = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
"on-source-error:s?,on-target-error:s?,"
--
1.7.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH V7 5/8] qmp: add interface blockdev-snapshot-delete-internal-sync
2013-08-07 3:00 [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level Wenchao Xia
` (3 preceding siblings ...)
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 4/8] qmp: add interface blockdev-snapshot-internal-sync Wenchao Xia
@ 2013-08-07 3:00 ` Wenchao Xia
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 6/8] hmp: add interface hmp_snapshot_blkdev_internal Wenchao Xia
` (6 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Wenchao Xia @ 2013-08-07 3:00 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, famz, Wenchao Xia, armbru, lcapitulino, stefanha,
pbonzini, dietmar
This interface use id and name as optional parameters, to handle the
case that one image contain multiple snapshots with same name which
may be '', but with different id.
Adding parameter id is for historical compatiability reason, and
that case is not possible in qemu's new interface for internal
snapshot at block device level, but still possible in qemu-img.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
blockdev.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++
include/qemu-common.h | 3 ++
qapi-schema.json | 27 +++++++++++++++++++++
qmp-commands.hx | 41 +++++++++++++++++++++++++++++++++
4 files changed, 132 insertions(+), 0 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index b4754ef..13de395 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -869,6 +869,67 @@ void qmp_blockdev_snapshot_internal_sync(const char *device,
&snapshot, errp);
}
+SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
+ bool has_id,
+ const char *id,
+ bool has_name,
+ const char *name,
+ Error **errp)
+{
+ BlockDriverState *bs = bdrv_find(device);
+ QEMUSnapshotInfo sn;
+ Error *local_err = NULL;
+ SnapshotInfo *info = NULL;
+ int ret;
+
+ if (!bs) {
+ error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+ return NULL;
+ }
+
+ if (!has_id) {
+ id = NULL;
+ }
+
+ if (!has_name) {
+ name = NULL;
+ }
+
+ if (!id && !name) {
+ error_setg(errp, "Name or id must be provided");
+ return NULL;
+ }
+
+ ret = bdrv_snapshot_find_by_id_and_name(bs, id, name, &sn, &local_err);
+ if (error_is_set(&local_err)) {
+ error_propagate(errp, local_err);
+ return NULL;
+ }
+ 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);
+ return NULL;
+ }
+
+ bdrv_snapshot_delete(bs, id, name, &local_err);
+ if (error_is_set(&local_err)) {
+ error_propagate(errp, local_err);
+ return NULL;
+ }
+
+ info = g_malloc0(sizeof(SnapshotInfo));
+ 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;
+}
/* New and old BlockDriverState structs for group snapshots */
diff --git a/include/qemu-common.h b/include/qemu-common.h
index 6948bb9..5054836 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -191,6 +191,9 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix);
int64_t strtosz_suffix_unit(const char *nptr, char **end,
const char default_suffix, int64_t unit);
+/* used to print char* safely */
+#define STR_OR_NULL(str) ((str) ? (str) : "null")
+
/* path.c */
void init_paths(const char *prefix);
const char *path(const char *pathname);
diff --git a/qapi-schema.json b/qapi-schema.json
index a236cde..5a46abe 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1778,6 +1778,33 @@
'data': 'BlockdevSnapshotInternal' }
##
+# @blockdev-snapshot-delete-internal-sync
+#
+# Synchronously delete an internal snapshot of a block device, when the format
+# of the image used support it. The snapshot is identified by name or id or
+# both. One of the name or id is required. Return SnapshotInfo for the
+# successfully deleted snapshot.
+#
+# @device: the name of the device to delete the snapshot from
+#
+# @id: optional the snapshot's ID to be deleted
+#
+# @name: optional the snapshot's name to be deleted
+#
+# Returns: SnapshotInfo on success
+# If @device is not a valid block device, DeviceNotFound
+# If snapshot not found, GenericError
+# If the format of the image used does not support it,
+# BlockFormatFeatureNotSupported
+# If @id and @name are both not specified, GenericError
+#
+# Since 1.7
+##
+{ 'command': 'blockdev-snapshot-delete-internal-sync',
+ 'data': { 'device': 'str', '*id': 'str', '*name': 'str'},
+ 'returns': 'SnapshotInfo' }
+
+##
# @human-monitor-command:
#
# Execute a command on the human monitor and return the output.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6097283..3d34a1c 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1127,6 +1127,47 @@ Example:
EQMP
{
+ .name = "blockdev-snapshot-delete-internal-sync",
+ .args_type = "device:B,id:s?,name:s?",
+ .mhandler.cmd_new =
+ qmp_marshal_input_blockdev_snapshot_delete_internal_sync,
+ },
+
+SQMP
+blockdev-snapshot-delete-internal-sync
+--------------------------------------
+
+Synchronously delete an internal snapshot of a block device when the format of
+image used supports it. The snapshot is identified by name or id or both. One
+of name or id is required. If the snapshot is not found, the operation will
+fail.
+
+Arguments:
+
+- "device": device name (json-string)
+- "id": ID of the snapshot (json-string, optional)
+- "name": name of the snapshot (json-string, optional)
+
+Example:
+
+-> { "execute": "blockdev-snapshot-delete-internal-sync",
+ "arguments": { "device": "ide-hd0",
+ "name": "snapshot0" }
+ }
+<- { "return": {
+ "id": "1",
+ "name": "snapshot0",
+ "vm-state-size": 0,
+ "date-sec": 1000012,
+ "date-nsec": 10,
+ "vm-clock-sec": 100,
+ "vm-clock-nsec": 20
+ }
+ }
+
+EQMP
+
+ {
.name = "drive-mirror",
.args_type = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
"on-source-error:s?,on-target-error:s?,"
--
1.7.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH V7 6/8] hmp: add interface hmp_snapshot_blkdev_internal
2013-08-07 3:00 [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level Wenchao Xia
` (4 preceding siblings ...)
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 5/8] qmp: add interface blockdev-snapshot-delete-internal-sync Wenchao Xia
@ 2013-08-07 3:00 ` Wenchao Xia
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 7/8] hmp: add interface hmp_snapshot_delete_blkdev_internal Wenchao Xia
` (5 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Wenchao Xia @ 2013-08-07 3:00 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, famz, Wenchao Xia, armbru, lcapitulino, stefanha,
pbonzini, dietmar
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
hmp-commands.hx | 19 +++++++++++++++++--
hmp.c | 10 ++++++++++
hmp.h | 1 +
3 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8c6b91a..039c401 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1023,8 +1023,7 @@ ETEXI
"of device. If a new image file is specified, the\n\t\t\t"
"new image file will become the new root image.\n\t\t\t"
"If format is specified, the snapshot file will\n\t\t\t"
- "be created in that format. Otherwise the\n\t\t\t"
- "snapshot will be internal! (currently unsupported).\n\t\t\t"
+ "be created in that format.\n\t\t\t"
"The default format is qcow2. The -n flag requests QEMU\n\t\t\t"
"to reuse the image found in new-image-file, instead of\n\t\t\t"
"recreating it from scratch.",
@@ -1038,6 +1037,22 @@ Snapshot device, using snapshot file as target if provided
ETEXI
{
+ .name = "snapshot_blkdev_internal",
+ .args_type = "device:B,name:s",
+ .params = "device name",
+ .help = "take an internal snapshot of device.\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_blkdev_internal,
+ },
+
+STEXI
+@item snapshot_blkdev_internal
+@findex snapshot_blkdev_internal
+Take 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 c45514b..34763be 100644
--- a/hmp.c
+++ b/hmp.c
@@ -962,6 +962,16 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, &errp);
}
+void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict)
+{
+ const char *device = qdict_get_str(qdict, "device");
+ const char *name = qdict_get_str(qdict, "name");
+ Error *errp = NULL;
+
+ qmp_blockdev_snapshot_internal_sync(device, name, &errp);
+ hmp_handle_error(mon, &errp);
+}
+
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
{
qmp_migrate_cancel(NULL);
diff --git a/hmp.h b/hmp.h
index 6c3bdcd..cc1ca58 100644
--- a/hmp.h
+++ b/hmp.h
@@ -54,6 +54,7 @@ void hmp_block_passwd(Monitor *mon, const QDict *qdict);
void hmp_balloon(Monitor *mon, const QDict *qdict);
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_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);
--
1.7.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH V7 7/8] hmp: add interface hmp_snapshot_delete_blkdev_internal
2013-08-07 3:00 [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level Wenchao Xia
` (5 preceding siblings ...)
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 6/8] hmp: add interface hmp_snapshot_blkdev_internal Wenchao Xia
@ 2013-08-07 3:00 ` Wenchao Xia
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 8/8] qemu-iotests: add 057 internal snapshot for block device test case Wenchao Xia
` (4 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Wenchao Xia @ 2013-08-07 3:00 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, famz, Wenchao Xia, armbru, lcapitulino, stefanha,
pbonzini, dietmar
It is hard to make both id and name optional in hmp console as qmp
interface, so this interface require user to specify name.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
hmp-commands.hx | 18 ++++++++++++++++++
hmp.c | 12 ++++++++++++
hmp.h | 1 +
3 files changed, 31 insertions(+), 0 deletions(-)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 039c401..96b1874 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1053,6 +1053,24 @@ Take an internal snapshot on device if it support
ETEXI
{
+ .name = "snapshot_delete_blkdev_internal",
+ .args_type = "device:B,name:s,id:s?",
+ .params = "device name [id]",
+ .help = "delete an internal snapshot of device.\n\t\t\t"
+ "If id is specified, qemu will try delete\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_delete_blkdev_internal,
+ },
+
+STEXI
+@item snapshot_delete_blkdev_internal
+@findex snapshot_delete_blkdev_internal
+Delete 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 34763be..e2139d0 100644
--- a/hmp.c
+++ b/hmp.c
@@ -972,6 +972,18 @@ void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict)
hmp_handle_error(mon, &errp);
}
+void hmp_snapshot_delete_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 *errp = NULL;
+
+ qmp_blockdev_snapshot_delete_internal_sync(device, !!id, id,
+ true, name, &errp);
+ hmp_handle_error(mon, &errp);
+}
+
void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
{
qmp_migrate_cancel(NULL);
diff --git a/hmp.h b/hmp.h
index cc1ca58..54cf71f 100644
--- a/hmp.h
+++ b/hmp.h
@@ -55,6 +55,7 @@ void hmp_balloon(Monitor *mon, const QDict *qdict);
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_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);
--
1.7.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH V7 8/8] qemu-iotests: add 057 internal snapshot for block device test case
2013-08-07 3:00 [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level Wenchao Xia
` (6 preceding siblings ...)
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 7/8] hmp: add interface hmp_snapshot_delete_blkdev_internal Wenchao Xia
@ 2013-08-07 3:00 ` Wenchao Xia
2013-08-16 2:16 ` [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level Wenchao Xia
` (3 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Wenchao Xia @ 2013-08-07 3:00 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, famz, Wenchao Xia, armbru, lcapitulino, stefanha,
pbonzini, dietmar
Create in transaction and deletion in single command will be tested.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
tests/qemu-iotests/057 | 259 ++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/057.out | 5 +
tests/qemu-iotests/group | 1 +
3 files changed, 265 insertions(+), 0 deletions(-)
create mode 100755 tests/qemu-iotests/057
create mode 100644 tests/qemu-iotests/057.out
diff --git a/tests/qemu-iotests/057 b/tests/qemu-iotests/057
new file mode 100755
index 0000000..9cdd582
--- /dev/null
+++ b/tests/qemu-iotests/057
@@ -0,0 +1,259 @@
+#!/usr/bin/env python
+#
+# Tests for internal snapshot.
+#
+# Copyright (C) 2013 IBM, Inc.
+#
+# Based on 055.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+import time
+import os
+import iotests
+from iotests import qemu_img, qemu_io
+
+test_drv_base_name = 'drive'
+
+class ImageSnapshotTestCase(iotests.QMPTestCase):
+ image_len = 120 * 1024 * 1024 # MB
+
+ def __init__(self, *args):
+ self.expect = []
+ super(ImageSnapshotTestCase, self).__init__(*args)
+
+ def _setUp(self, test_img_base_name, image_num):
+ self.vm = iotests.VM()
+ for i in range(0, image_num):
+ filename = '%s%d' % (test_img_base_name, i)
+ img = os.path.join(iotests.test_dir, filename)
+ device = '%s%d' % (test_drv_base_name, i)
+ qemu_img('create', '-f', iotests.imgfmt, img, str(self.image_len))
+ self.vm.add_drive(img)
+ self.expect.append({'image': img, 'device': device,
+ 'snapshots': [],
+ 'snapshots_name_counter': 0})
+ self.vm.launch()
+
+ def tearDown(self):
+ self.vm.shutdown()
+ for dev_expect in self.expect:
+ os.remove(dev_expect['image'])
+
+ def createSnapshotInTransaction(self, snapshot_num, abort = False):
+ actions = []
+ for dev_expect in self.expect:
+ num = dev_expect['snapshots_name_counter']
+ for j in range(0, snapshot_num):
+ name = '%s_sn%d' % (dev_expect['device'], num)
+ num = num + 1
+ if abort == False:
+ dev_expect['snapshots'].append({'name': name})
+ dev_expect['snapshots_name_counter'] = num
+ actions.append({
+ 'type': 'blockdev-snapshot-internal-sync',
+ 'data': { 'device': dev_expect['device'],
+ 'name': name },
+ })
+
+ if abort == True:
+ actions.append({
+ 'type': 'abort',
+ 'data': {},
+ })
+
+ result = self.vm.qmp('transaction', actions = actions)
+
+ if abort == True:
+ self.assert_qmp(result, 'error/class', 'GenericError')
+ else:
+ self.assert_qmp(result, 'return', {})
+
+ def verifySnapshotInfo(self):
+ result = self.vm.qmp('query-block')
+
+ # Verify each expected result
+ for dev_expect in self.expect:
+ # 1. Find the returned image value and snapshot info
+ image_result = None
+ for device in result['return']:
+ if device['device'] == dev_expect['device']:
+ image_result = device['inserted']['image']
+ break
+ self.assertTrue(image_result != None)
+ # Do not consider zero snapshot case now
+ sn_list_result = image_result['snapshots']
+ sn_list_expect = dev_expect['snapshots']
+
+ # 2. Verify it with expect
+ self.assertTrue(len(sn_list_result) == len(sn_list_expect))
+
+ for sn_expect in sn_list_expect:
+ sn_result = None
+ for sn in sn_list_result:
+ if sn_expect['name'] == sn['name']:
+ sn_result = sn
+ break
+ self.assertTrue(sn_result != None)
+ # Fill in the detail info
+ sn_expect.update(sn_result)
+
+ def deleteSnapshot(self, device, id = None, name = None):
+ sn_list_expect = None
+ sn_expect = None
+
+ self.assertTrue(id != None or name != None)
+
+ # Fill in the detail info include ID
+ self.verifySnapshotInfo()
+
+ #find the expected snapshot list
+ for dev_expect in self.expect:
+ if dev_expect['device'] == device:
+ sn_list_expect = dev_expect['snapshots']
+ break
+ self.assertTrue(sn_list_expect != None)
+
+ if id != None and name != None:
+ for sn in sn_list_expect:
+ if sn['id'] == id and sn['name'] == name:
+ sn_expect = sn
+ result = \
+ self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+ device = device,
+ id = id,
+ name = name)
+ break
+ elif id != None:
+ for sn in sn_list_expect:
+ if sn['id'] == id:
+ sn_expect = sn
+ result = \
+ self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+ device = device,
+ id = id)
+ break
+ else:
+ for sn in sn_list_expect:
+ if sn['name'] == name:
+ sn_expect = sn
+ result = \
+ self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+ device = device,
+ name = name)
+ break
+
+ self.assertTrue(sn_expect != None)
+
+ self.assert_qmp(result, 'return', sn_expect)
+ sn_list_expect.remove(sn_expect)
+
+class TestSingleTransaction(ImageSnapshotTestCase):
+ def setUp(self):
+ self._setUp('test_a.img', 1)
+
+ def test_create(self):
+ self.createSnapshotInTransaction(1)
+ self.verifySnapshotInfo()
+
+ def test_error_name_empty(self):
+ actions = [{'type': 'blockdev-snapshot-internal-sync',
+ 'data': { 'device': self.expect[0]['device'],
+ 'name': '' },
+ }]
+ result = self.vm.qmp('transaction', actions = actions)
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
+ def test_error_device(self):
+ actions = [{'type': 'blockdev-snapshot-internal-sync',
+ 'data': { 'device': 'drive_error',
+ 'name': 'a' },
+ }]
+ result = self.vm.qmp('transaction', actions = actions)
+ self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+ def test_error_exist(self):
+ self.createSnapshotInTransaction(1)
+ self.verifySnapshotInfo()
+ actions = [{'type': 'blockdev-snapshot-internal-sync',
+ 'data': { 'device': self.expect[0]['device'],
+ 'name': self.expect[0]['snapshots'][0] },
+ }]
+ result = self.vm.qmp('transaction', actions = actions)
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
+class TestMultipleTransaction(ImageSnapshotTestCase):
+ def setUp(self):
+ self._setUp('test_b.img', 2)
+
+ def test_create(self):
+ self.createSnapshotInTransaction(3)
+ self.verifySnapshotInfo()
+
+ def test_abort(self):
+ self.createSnapshotInTransaction(2)
+ self.verifySnapshotInfo()
+ self.createSnapshotInTransaction(3, abort = True)
+ self.verifySnapshotInfo()
+
+class TestSnapshotDelete(ImageSnapshotTestCase):
+ def setUp(self):
+ self._setUp('test_c.img', 1)
+
+ def test_delete_with_id(self):
+ self.createSnapshotInTransaction(2)
+ self.verifySnapshotInfo()
+ self.deleteSnapshot(self.expect[0]['device'],
+ id = self.expect[0]['snapshots'][0]['id'])
+ self.verifySnapshotInfo()
+
+ def test_delete_with_name(self):
+ self.createSnapshotInTransaction(3)
+ self.verifySnapshotInfo()
+ self.deleteSnapshot(self.expect[0]['device'],
+ name = self.expect[0]['snapshots'][1]['name'])
+ self.verifySnapshotInfo()
+
+ def test_delete_with_id_and_name(self):
+ self.createSnapshotInTransaction(4)
+ self.verifySnapshotInfo()
+ self.deleteSnapshot(self.expect[0]['device'],
+ id = self.expect[0]['snapshots'][2]['id'],
+ name = self.expect[0]['snapshots'][2]['name'])
+ self.verifySnapshotInfo()
+
+
+ def test_error_device(self):
+ result = self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+ device = 'drive_error',
+ id = '0')
+ self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+ def test_error_no_id_and_name(self):
+ result = self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+ device = self.expect[0]['device'])
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
+ def test_error_snapshot_not_exist(self):
+ self.createSnapshotInTransaction(2)
+ self.verifySnapshotInfo()
+ result = self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+ device = self.expect[0]['device'],
+ id = self.expect[0]['snapshots'][0]['id'],
+ name = self.expect[0]['snapshots'][1]['name'])
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
+if __name__ == '__main__':
+ iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/057.out b/tests/qemu-iotests/057.out
new file mode 100644
index 0000000..281b69e
--- /dev/null
+++ b/tests/qemu-iotests/057.out
@@ -0,0 +1,5 @@
+............
+----------------------------------------------------------------------
+Ran 12 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 69e208c..74aebed 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -63,3 +63,4 @@
054 rw auto
055 rw auto
056 rw auto backing
+057 rw auto
--
1.7.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level
2013-08-07 3:00 [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level Wenchao Xia
` (7 preceding siblings ...)
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 8/8] qemu-iotests: add 057 internal snapshot for block device test case Wenchao Xia
@ 2013-08-16 2:16 ` Wenchao Xia
2013-08-16 15:36 ` Stefan Hajnoczi
` (2 subsequent siblings)
11 siblings, 0 replies; 16+ messages in thread
From: Wenchao Xia @ 2013-08-16 2:16 UTC (permalink / raw)
To: Wenchao Xia
Cc: kwolf, phrdina, famz, armbru, qemu-devel, lcapitulino, stefanha,
pbonzini, dietmar
于 2013-8-7 11:00, Wenchao Xia 写道:
> This series brings internal snapshot support at block devices level, now we
> have two three methods to do block snapshot lively: 1) backing chain,
> 2) internal one and 3) drive-back up approach.
>
> Comparation:
> Advantages: Disadvantages:
> 1) delta data, taken fast, export, size performance, delete slow.
> 2) taken fast, delete fast, performance, size delta data, format
> 3) performance, export, format taken slow, delta data, size, host I/O
>
> I think in most case, saving vmstate in an standalone file is better than
> saving it inside qcow2, So suggest treat internal snapshot as block level
> methods and not encourage user to savevm in qcow2 any more.
>
> Implemention details:
> To avoid trouble, this serial have hide ID in create interfaces, this make
> sure no chaos of ID and name will be introduced by these interfaces.
> There is one patch may be common to Pavel's savvm transaction, patch 1/11,
> others are not quite related. Patch 1/11 will not set errp when no snapshot
> find, since patch 3/11 need to distinguish real error case.
>
> Next steps to better full VM snapshot:
> Improve internal snapshot's export capability.
> Better vmstate saving.
>
> Thanks Kevin to give advisement about how add it in qmp_transaction, oldest
> version comes drom Dietmar Maurer.
>
> v3:
> General:
> Rebased after Stenfan's driver-backup patch V6.
>
> Address Eric's comments:
> 4/9: grammar fix and better doc.
> 5/9: parameter name is mandatory now. grammar fix.
> 6/9: redesiged interface: take both id and name as optional parameter, return
> the deleted snapshot's info.
>
> Address Stefan's comments:
> 4/9: add '' around %s in message. drop code comments about vm_clock.
> 9/9: better doc, refined the code and add more test case.
>
> v4:
> Address Stefan's comments:
> 4/9: use error_setg_errno() to show error reason for bdrv_snapshot_create(),
> spell fix and better doc.
> 5/9: better doc.
> 6/9: remove spurious ';' in code, spell fix and better doc.
>
> v5:
> Address Kevin's comments:
> 3/8, 4/8, 8/8: remove the limit of numeric snapshot name.
> General change:
> 4/8: use existing type as parameter in qapi schema.
>
> v6:
> Address Stefan's comments:
> 2/8: macro STR_PRINT_CHAR was renamed as STR_OR_NULL, and moved into patch 5,
> since implement function in this patch do not printf snapshot id any more, as
> Kevin's suggestion.
> Address Kevin's comments:
> 2/8: remove device, id, name info in the error message, use error message in
> existing caller. A new function bdrv_snapshot_delete_by_id_or_name() is added
> to make the usage clear while keep logic unchanged.
> 3/8: remove device info in error message when name is empty. Use else if
> after call of bdrv_snapshot_find_by_id_and_name().
> Other:
> 2/8: refined the comments in code for bdrv_snapshot_delete().
> 3/8: in error reporting, change format from "reason is: '%s'" to
> "reason is: %s".
>
> v7:
> rebased on upstream, target for 1.7.
>
> Wenchao Xia (8):
> 1 snapshot: new function bdrv_snapshot_find_by_id_and_name()
> 2 snapshot: distinguish id and name in snapshot delete
> 3 qmp: add internal snapshot support in qmp_transaction
> 4 qmp: add interface blockdev-snapshot-internal-sync
> 5 qmp: add interface blockdev-snapshot-delete-internal-sync
> 6 hmp: add interface hmp_snapshot_blkdev_internal
> 7 hmp: add interface hmp_snapshot_delete_blkdev_internal
> 8 qemu-iotests: add 057 internal snapshot for block device test case
>
> block/qcow2-snapshot.c | 55 +++++++---
> block/qcow2.h | 5 +-
> block/rbd.c | 21 ++++-
> block/sheepdog.c | 5 +-
> block/snapshot.c | 131 ++++++++++++++++++++++-
> blockdev.c | 190 ++++++++++++++++++++++++++++++++
> hmp-commands.hx | 37 ++++++-
> hmp.c | 22 ++++
> hmp.h | 2 +
> include/block/block_int.h | 5 +-
> include/block/snapshot.h | 14 +++-
> include/qemu-common.h | 3 +
> qapi-schema.json | 66 +++++++++++-
> qemu-img.c | 11 ++-
> qmp-commands.hx | 104 ++++++++++++++++--
> savevm.c | 32 +++---
> tests/qemu-iotests/057 | 259 ++++++++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/057.out | 5 +
> tests/qemu-iotests/group | 1 +
> 19 files changed, 914 insertions(+), 54 deletions(-)
> create mode 100755 tests/qemu-iotests/057
> create mode 100644 tests/qemu-iotests/057.out
>
>
Any comments for it?
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level
2013-08-07 3:00 [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level Wenchao Xia
` (8 preceding siblings ...)
2013-08-16 2:16 ` [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level Wenchao Xia
@ 2013-08-16 15:36 ` Stefan Hajnoczi
2013-09-02 1:44 ` Wenchao Xia
2013-09-10 12:26 ` Kevin Wolf
2013-09-10 13:09 ` Kevin Wolf
11 siblings, 1 reply; 16+ messages in thread
From: Stefan Hajnoczi @ 2013-08-16 15:36 UTC (permalink / raw)
To: Kevin Wolf
Cc: phrdina, famz, Wenchao Xia, armbru, qemu-devel, lcapitulino,
stefanha, pbonzini, dietmar
On Wed, Aug 07, 2013 at 11:00:11AM +0800, Wenchao Xia wrote:
> v6:
> Address Stefan's comments:
> 2/8: macro STR_PRINT_CHAR was renamed as STR_OR_NULL, and moved into patch 5,
> since implement function in this patch do not printf snapshot id any more, as
> Kevin's suggestion.
Great.
> Address Kevin's comments:
> 2/8: remove device, id, name info in the error message, use error message in
> existing caller. A new function bdrv_snapshot_delete_by_id_or_name() is added
> to make the usage clear while keep logic unchanged.
> 3/8: remove device info in error message when name is empty. Use else if
> after call of bdrv_snapshot_find_by_id_and_name().
> Other:
> 2/8: refined the comments in code for bdrv_snapshot_delete().
> 3/8: in error reporting, change format from "reason is: '%s'" to
> "reason is: %s".
Kevin: do you have time to review these changes?
Stefan
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level
2013-08-16 15:36 ` Stefan Hajnoczi
@ 2013-09-02 1:44 ` Wenchao Xia
0 siblings, 0 replies; 16+ messages in thread
From: Wenchao Xia @ 2013-09-02 1:44 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, phrdina, famz, qemu-devel, armbru, lcapitulino,
stefanha, pbonzini, dietmar
于 2013-8-16 23:36, Stefan Hajnoczi 写道:
> On Wed, Aug 07, 2013 at 11:00:11AM +0800, Wenchao Xia wrote:
>> v6:
>> Address Stefan's comments:
>> 2/8: macro STR_PRINT_CHAR was renamed as STR_OR_NULL, and moved into patch 5,
>> since implement function in this patch do not printf snapshot id any more, as
>> Kevin's suggestion.
>
> Great.
>
>> Address Kevin's comments:
>> 2/8: remove device, id, name info in the error message, use error message in
>> existing caller. A new function bdrv_snapshot_delete_by_id_or_name() is added
>> to make the usage clear while keep logic unchanged.
>> 3/8: remove device info in error message when name is empty. Use else if
>> after call of bdrv_snapshot_find_by_id_and_name().
>> Other:
>> 2/8: refined the comments in code for bdrv_snapshot_delete().
>> 3/8: in error reporting, change format from "reason is: '%s'" to
>> "reason is: %s".
>
> Kevin: do you have time to review these changes?
>
> Stefan
>
Hi, Kevin
Do you have comments for it?
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level
2013-08-07 3:00 [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level Wenchao Xia
` (9 preceding siblings ...)
2013-08-16 15:36 ` Stefan Hajnoczi
@ 2013-09-10 12:26 ` Kevin Wolf
2013-09-10 13:09 ` Kevin Wolf
11 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2013-09-10 12:26 UTC (permalink / raw)
To: Wenchao Xia
Cc: phrdina, famz, armbru, qemu-devel, lcapitulino, stefanha,
pbonzini, dietmar
Am 07.08.2013 um 05:00 hat Wenchao Xia geschrieben:
> This series brings internal snapshot support at block devices level, now we
> have two three methods to do block snapshot lively: 1) backing chain,
> 2) internal one and 3) drive-back up approach.
>
> Comparation:
> Advantages: Disadvantages:
> 1) delta data, taken fast, export, size performance, delete slow.
> 2) taken fast, delete fast, performance, size delta data, format
> 3) performance, export, format taken slow, delta data, size, host I/O
>
> I think in most case, saving vmstate in an standalone file is better than
> saving it inside qcow2, So suggest treat internal snapshot as block level
> methods and not encourage user to savevm in qcow2 any more.
>
> Implemention details:
> To avoid trouble, this serial have hide ID in create interfaces, this make
> sure no chaos of ID and name will be introduced by these interfaces.
> There is one patch may be common to Pavel's savvm transaction, patch 1/11,
> others are not quite related. Patch 1/11 will not set errp when no snapshot
> find, since patch 3/11 need to distinguish real error case.
>
> Next steps to better full VM snapshot:
> Improve internal snapshot's export capability.
> Better vmstate saving.
>
> Thanks Kevin to give advisement about how add it in qmp_transaction, oldest
> version comes drom Dietmar Maurer.
This series needs to be rebased. The last patch (qemu-iotests) doesn't
apply any more because other tests were added in the meantime, and when
applying only the code changes, it fails to compile:
blockdev.c: In function 'internal_snapshot_prepare':
blockdev.c:1040:5: error: implicit declaration of function 'qemu_get_clock_ns' [-Werror=implicit-function-declaration]
blockdev.c:1040:5: error: nested extern declaration of 'qemu_get_clock_ns' [-Werror=nested-externs]
blockdev.c:1040:43: error: 'vm_clock' undeclared (first use in this function)
blockdev.c:1040:43: note: each undeclared identifier is reported only once for each function it appears in
Kevin
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH V7 2/8] snapshot: distinguish id and name in snapshot delete
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 2/8] snapshot: distinguish id and name in snapshot delete Wenchao Xia
@ 2013-09-10 12:36 ` Kevin Wolf
2013-09-11 2:30 ` Wenchao Xia
0 siblings, 1 reply; 16+ messages in thread
From: Kevin Wolf @ 2013-09-10 12:36 UTC (permalink / raw)
To: Wenchao Xia
Cc: phrdina, famz, armbru, qemu-devel, lcapitulino, stefanha,
pbonzini, dietmar
Am 07.08.2013 um 05:00 hat Wenchao Xia geschrieben:
> Snapshot creation actually already distinguish id and name since it take
> a structured parameter *sn, but delete can't. Later an accurate delete
> is needed in qmp_transaction abort and blockdev-snapshot-delete-sync,
> so change its prototype. Also *errp is added to tip error, but return
> value is kepted to let caller check what kind of error happens. Existing
> caller for it are savevm, delvm and qemu-img, they are not impacted by
> introducing a new function bdrv_snapshot_delete_by_id_or_name(), which
> check the return value and do the operation again.
>
> Before this patch:
> For qcow2, it search id first then name to find the one to delete.
> For rbd, it search name.
> For sheepdog, it does nothing.
>
> After this patch:
> For qcow2, logic is the same by call it twice in caller.
> For rbd, it always fails in delete with id, but still search for name
> in second try, no change to user.
>
> Some code for *errp is based on Pavel's patch.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> diff --git a/savevm.c b/savevm.c
> index 03fc4d9..0808414 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2325,18 +2325,21 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
> {
> BlockDriverState *bs;
> QEMUSnapshotInfo sn1, *snapshot = &sn1;
> - int ret;
> + Error *err = NULL;
>
> bs = NULL;
> while ((bs = bdrv_next(bs))) {
> if (bdrv_can_snapshot(bs) &&
> bdrv_snapshot_find(bs, snapshot, name) >= 0)
> {
> - ret = bdrv_snapshot_delete(bs, name);
> - if (ret < 0) {
> + bdrv_snapshot_delete_by_id_or_name(bs, name, &err);
> + if (error_is_set(&err)) {
> monitor_printf(mon,
> - "Error while deleting snapshot on '%s'\n",
> - bdrv_get_device_name(bs));
> + "Error while deleting snapshot on device '%s', "
> + "reason: %s\n",
More commonly, error messages just use a colon before the detailed
error code instead of saying ", reason:"
> + bdrv_get_device_name(bs),
> + error_get_pretty(err));
> + error_free(err);
> return -1;
> }
> }
Kevin
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level
2013-08-07 3:00 [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level Wenchao Xia
` (10 preceding siblings ...)
2013-09-10 12:26 ` Kevin Wolf
@ 2013-09-10 13:09 ` Kevin Wolf
11 siblings, 0 replies; 16+ messages in thread
From: Kevin Wolf @ 2013-09-10 13:09 UTC (permalink / raw)
To: Wenchao Xia
Cc: phrdina, famz, armbru, qemu-devel, lcapitulino, stefanha,
pbonzini, dietmar
Am 07.08.2013 um 05:00 hat Wenchao Xia geschrieben:
> This series brings internal snapshot support at block devices level, now we
> have two three methods to do block snapshot lively: 1) backing chain,
> 2) internal one and 3) drive-back up approach.
>
> Comparation:
> Advantages: Disadvantages:
> 1) delta data, taken fast, export, size performance, delete slow.
> 2) taken fast, delete fast, performance, size delta data, format
> 3) performance, export, format taken slow, delta data, size, host I/O
>
> I think in most case, saving vmstate in an standalone file is better than
> saving it inside qcow2, So suggest treat internal snapshot as block level
> methods and not encourage user to savevm in qcow2 any more.
Looks good and should be mergable after a rebase.
Kevin
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH V7 2/8] snapshot: distinguish id and name in snapshot delete
2013-09-10 12:36 ` Kevin Wolf
@ 2013-09-11 2:30 ` Wenchao Xia
0 siblings, 0 replies; 16+ messages in thread
From: Wenchao Xia @ 2013-09-11 2:30 UTC (permalink / raw)
To: Kevin Wolf
Cc: phrdina, famz, armbru, qemu-devel, lcapitulino, stefanha,
pbonzini, dietmar
于 2013/9/10 20:36, Kevin Wolf 写道:
> Am 07.08.2013 um 05:00 hat Wenchao Xia geschrieben:
>> Snapshot creation actually already distinguish id and name since it take
>> a structured parameter *sn, but delete can't. Later an accurate delete
>> is needed in qmp_transaction abort and blockdev-snapshot-delete-sync,
>> so change its prototype. Also *errp is added to tip error, but return
>> value is kepted to let caller check what kind of error happens. Existing
>> caller for it are savevm, delvm and qemu-img, they are not impacted by
>> introducing a new function bdrv_snapshot_delete_by_id_or_name(), which
>> check the return value and do the operation again.
>>
>> Before this patch:
>> For qcow2, it search id first then name to find the one to delete.
>> For rbd, it search name.
>> For sheepdog, it does nothing.
>>
>> After this patch:
>> For qcow2, logic is the same by call it twice in caller.
>> For rbd, it always fails in delete with id, but still search for name
>> in second try, no change to user.
>>
>> Some code for *errp is based on Pavel's patch.
>>
>> Signed-off-by: Wenchao Xia<xiawenc@linux.vnet.ibm.com>
>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>> diff --git a/savevm.c b/savevm.c
>> index 03fc4d9..0808414 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2325,18 +2325,21 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
>> {
>> BlockDriverState *bs;
>> QEMUSnapshotInfo sn1, *snapshot =&sn1;
>> - int ret;
>> + Error *err = NULL;
>>
>> bs = NULL;
>> while ((bs = bdrv_next(bs))) {
>> if (bdrv_can_snapshot(bs)&&
>> bdrv_snapshot_find(bs, snapshot, name)>= 0)
>> {
>> - ret = bdrv_snapshot_delete(bs, name);
>> - if (ret< 0) {
>> + bdrv_snapshot_delete_by_id_or_name(bs, name,&err);
>> + if (error_is_set(&err)) {
>> monitor_printf(mon,
>> - "Error while deleting snapshot on '%s'\n",
>> - bdrv_get_device_name(bs));
>> + "Error while deleting snapshot on device '%s', "
>> + "reason: %s\n",
> More commonly, error messages just use a colon before the detailed
> error code instead of saying ", reason:"
>
>> + bdrv_get_device_name(bs),
>> + error_get_pretty(err));
>> + error_free(err);
>> return -1;
>> }
>> }
> Kevin
>
Thanks for reviewing, will fix and rebase.
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2013-09-11 2:30 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-07 3:00 [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level Wenchao Xia
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 1/8] snapshot: new function bdrv_snapshot_find_by_id_and_name() Wenchao Xia
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 2/8] snapshot: distinguish id and name in snapshot delete Wenchao Xia
2013-09-10 12:36 ` Kevin Wolf
2013-09-11 2:30 ` Wenchao Xia
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 3/8] qmp: add internal snapshot support in qmp_transaction Wenchao Xia
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 4/8] qmp: add interface blockdev-snapshot-internal-sync Wenchao Xia
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 5/8] qmp: add interface blockdev-snapshot-delete-internal-sync Wenchao Xia
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 6/8] hmp: add interface hmp_snapshot_blkdev_internal Wenchao Xia
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 7/8] hmp: add interface hmp_snapshot_delete_blkdev_internal Wenchao Xia
2013-08-07 3:00 ` [Qemu-devel] [PATCH V7 8/8] qemu-iotests: add 057 internal snapshot for block device test case Wenchao Xia
2013-08-16 2:16 ` [Qemu-devel] [PATCH V7 0/8] add internal snapshot support at block device level Wenchao Xia
2013-08-16 15:36 ` Stefan Hajnoczi
2013-09-02 1:44 ` Wenchao Xia
2013-09-10 12:26 ` Kevin Wolf
2013-09-10 13:09 ` Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.