* [Qemu-devel] [PATCH 01/11] snapshot: create helper to test that block drivers supports snapshots
2015-11-17 9:08 [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes Denis V. Lunev
@ 2015-11-17 9:08 ` Denis V. Lunev
2015-11-18 10:57 ` Juan Quintela
2015-11-17 9:08 ` [Qemu-devel] [PATCH 02/11] snapshot: return error code from bdrv_snapshot_delete_by_id_or_name Denis V. Lunev
` (12 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Denis V. Lunev @ 2015-11-17 9:08 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, stefanha, Juan Quintela
The patch enforces proper locking for this operation.
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
block/snapshot.c | 24 ++++++++++++++++++++++++
include/block/snapshot.h | 8 ++++++++
migration/savevm.c | 17 ++++-------------
3 files changed, 36 insertions(+), 13 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index 89500f2..d929d08 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -356,3 +356,27 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
return ret;
}
+
+
+/* Group operations. All block drivers are involved.
+ * These functions will properly handle dataplane (take aio_context_acquire
+ * when appropriate for appropriate block drivers) */
+
+bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
+{
+ bool ok = true;
+ BlockDriverState *bs = NULL;
+
+ while (ok && (bs = bdrv_next(bs))) {
+ AioContext *ctx = bdrv_get_aio_context(bs);
+
+ aio_context_acquire(ctx);
+ if (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs)) {
+ ok = bdrv_can_snapshot(bs);
+ }
+ aio_context_release(ctx);
+ }
+
+ *first_bad_bs = bs;
+ return ok;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 770d9bb..6195c9c 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -75,4 +75,12 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
const char *id_or_name,
Error **errp);
+
+
+/* Group operations. All block drivers are involved.
+ * These functions will properly handle dataplane (take aio_context_acquire
+ * when appropriate for appropriate block drivers */
+
+bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
+
#endif
diff --git a/migration/savevm.c b/migration/savevm.c
index d90e228..4646aa1 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1958,19 +1958,10 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
const char *name = qdict_get_try_str(qdict, "name");
Error *local_err = NULL;
- /* Verify if there is a device that doesn't support snapshots and is writable */
- bs = NULL;
- while ((bs = bdrv_next(bs))) {
-
- if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
- continue;
- }
-
- if (!bdrv_can_snapshot(bs)) {
- monitor_printf(mon, "Device '%s' is writable but does not support snapshots.\n",
- bdrv_get_device_name(bs));
- return;
- }
+ if (!bdrv_all_can_snapshot(&bs)) {
+ monitor_printf(mon, "Device '%s' is writable but does not "
+ "support snapshots.\n", bdrv_get_device_name(bs));
+ return;
}
bs = find_vmstate_bs();
--
2.5.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* [Qemu-devel] [PATCH 02/11] snapshot: return error code from bdrv_snapshot_delete_by_id_or_name
2015-11-17 9:08 [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes Denis V. Lunev
2015-11-17 9:08 ` [Qemu-devel] [PATCH 01/11] snapshot: create helper to test that block drivers supports snapshots Denis V. Lunev
@ 2015-11-17 9:08 ` Denis V. Lunev
2015-11-18 10:59 ` Juan Quintela
2015-11-17 9:08 ` [Qemu-devel] [PATCH 03/11] snapshot: create bdrv_all_delete_snapshot helper Denis V. Lunev
` (11 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Denis V. Lunev @ 2015-11-17 9:08 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, stefanha, Juan Quintela
this will make code better in the next patch
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
block/snapshot.c | 7 ++++---
include/block/snapshot.h | 6 +++---
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index d929d08..ed0422d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -253,9 +253,9 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
return -ENOTSUP;
}
-void bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
- const char *id_or_name,
- Error **errp)
+int bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
+ const char *id_or_name,
+ Error **errp)
{
int ret;
Error *local_err = NULL;
@@ -270,6 +270,7 @@ void bdrv_snapshot_delete_by_id_or_name(BlockDriverState *bs,
if (ret < 0) {
error_propagate(errp, local_err);
}
+ return ret;
}
int bdrv_snapshot_list(BlockDriverState *bs,
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 6195c9c..9ddfd42 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -63,9 +63,9 @@ 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_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,
--
2.5.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* [Qemu-devel] [PATCH 03/11] snapshot: create bdrv_all_delete_snapshot helper
2015-11-17 9:08 [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes Denis V. Lunev
2015-11-17 9:08 ` [Qemu-devel] [PATCH 01/11] snapshot: create helper to test that block drivers supports snapshots Denis V. Lunev
2015-11-17 9:08 ` [Qemu-devel] [PATCH 02/11] snapshot: return error code from bdrv_snapshot_delete_by_id_or_name Denis V. Lunev
@ 2015-11-17 9:08 ` Denis V. Lunev
2015-11-18 11:01 ` Juan Quintela
2015-11-17 9:08 ` [Qemu-devel] [PATCH 04/11] snapshot: create bdrv_all_goto_snapshot helper Denis V. Lunev
` (10 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Denis V. Lunev @ 2015-11-17 9:08 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, stefanha, Juan Quintela
to delete snapshots from all loaded block drivers.
The patch also ensures proper locking.
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
block/snapshot.c | 22 ++++++++++++++++++++
include/block/snapshot.h | 2 ++
migration/savevm.c | 54 +++++++++---------------------------------------
3 files changed, 34 insertions(+), 44 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index ed0422d..61a6ad1 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -381,3 +381,25 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
*first_bad_bs = bs;
return ok;
}
+
+int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
+ Error **err)
+{
+ int ret = 0;
+ BlockDriverState *bs = NULL;
+ QEMUSnapshotInfo sn1, *snapshot = &sn1;
+
+ while (ret == 0 && (bs = bdrv_next(bs))) {
+ AioContext *ctx = bdrv_get_aio_context(bs);
+
+ aio_context_acquire(ctx);
+ if (bdrv_can_snapshot(bs) &&
+ bdrv_snapshot_find(bs, snapshot, name) >= 0) {
+ ret = bdrv_snapshot_delete_by_id_or_name(bs, name, err);
+ }
+ aio_context_release(ctx);
+ }
+
+ *first_bad_bs = bs;
+ return ret;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 9ddfd42..d02d2b1 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -82,5 +82,7 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
* when appropriate for appropriate block drivers */
bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
+int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
+ Error **err);
#endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 4646aa1..c52cbbe 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1916,35 +1916,6 @@ static BlockDriverState *find_vmstate_bs(void)
return NULL;
}
-/*
- * Deletes snapshots of a given name in all opened images.
- */
-static int del_existing_snapshots(Monitor *mon, const char *name)
-{
- BlockDriverState *bs;
- QEMUSnapshotInfo sn1, *snapshot = &sn1;
- Error *err = NULL;
-
- bs = NULL;
- while ((bs = bdrv_next(bs))) {
- if (bdrv_can_snapshot(bs) &&
- bdrv_snapshot_find(bs, snapshot, name) >= 0) {
- bdrv_snapshot_delete_by_id_or_name(bs, name, &err);
- if (err) {
- monitor_printf(mon,
- "Error while deleting snapshot on device '%s':"
- " %s\n",
- bdrv_get_device_name(bs),
- error_get_pretty(err));
- error_free(err);
- return -1;
- }
- }
- }
-
- return 0;
-}
-
void hmp_savevm(Monitor *mon, const QDict *qdict)
{
BlockDriverState *bs, *bs1;
@@ -2002,7 +1973,11 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
}
/* Delete old snapshots of the same name */
- if (name && del_existing_snapshots(mon, name) < 0) {
+ if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
+ monitor_printf(mon,
+ "Error while deleting snapshot on device '%s': %s\n",
+ bdrv_get_device_name(bs1), error_get_pretty(local_err));
+ error_free(local_err);
goto the_end;
}
@@ -2162,20 +2137,11 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
return;
}
- bs = NULL;
- while ((bs = bdrv_next(bs))) {
- if (bdrv_can_snapshot(bs)) {
- err = NULL;
- bdrv_snapshot_delete_by_id_or_name(bs, name, &err);
- if (err) {
- monitor_printf(mon,
- "Error while deleting snapshot on device '%s':"
- " %s\n",
- bdrv_get_device_name(bs),
- error_get_pretty(err));
- error_free(err);
- }
- }
+ if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
+ monitor_printf(mon,
+ "Error while deleting snapshot on device '%s': %s\n",
+ bdrv_get_device_name(bs), error_get_pretty(err));
+ error_free(err);
}
}
--
2.5.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH 03/11] snapshot: create bdrv_all_delete_snapshot helper
2015-11-17 9:08 ` [Qemu-devel] [PATCH 03/11] snapshot: create bdrv_all_delete_snapshot helper Denis V. Lunev
@ 2015-11-18 11:01 ` Juan Quintela
0 siblings, 0 replies; 36+ messages in thread
From: Juan Quintela @ 2015-11-18 11:01 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, stefanha
"Denis V. Lunev" <den@openvz.org> wrote:
> to delete snapshots from all loaded block drivers.
>
> The patch also ensures proper locking.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
I will still suggest to rename it to
brdv_delete_all_snapshots()
or
bdrv_delete_snapshot_everywhere()
that makes is more clear what it does.
A big THANKS for removing this from the migration side of the equation
O:-)
Later, Juan.
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 04/11] snapshot: create bdrv_all_goto_snapshot helper
2015-11-17 9:08 [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes Denis V. Lunev
` (2 preceding siblings ...)
2015-11-17 9:08 ` [Qemu-devel] [PATCH 03/11] snapshot: create bdrv_all_delete_snapshot helper Denis V. Lunev
@ 2015-11-17 9:08 ` Denis V. Lunev
2015-11-18 11:02 ` Juan Quintela
2015-11-17 9:08 ` [Qemu-devel] [PATCH 05/11] migration: factor our snapshottability check in load_vmstate Denis V. Lunev
` (9 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Denis V. Lunev @ 2015-11-17 9:08 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, stefanha, Juan Quintela
to switch to snapshot on all loaded block drivers.
The patch also ensures proper locking.
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
block/snapshot.c | 20 ++++++++++++++++++++
include/block/snapshot.h | 1 +
migration/savevm.c | 15 +++++----------
3 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index 61a6ad1..9f07a63 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -403,3 +403,23 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
*first_bad_bs = bs;
return ret;
}
+
+
+int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
+{
+ int err = 0;
+ BlockDriverState *bs = NULL;
+
+ while (err == 0 && (bs = bdrv_next(bs))) {
+ AioContext *ctx = bdrv_get_aio_context(bs);
+
+ aio_context_acquire(ctx);
+ if (bdrv_can_snapshot(bs)) {
+ err = bdrv_snapshot_goto(bs, name);
+ }
+ aio_context_release(ctx);
+ }
+
+ *first_bad_bs = bs;
+ return err;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index d02d2b1..0a176c7 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -84,5 +84,6 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
Error **err);
+int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
#endif
diff --git a/migration/savevm.c b/migration/savevm.c
index c52cbbe..254e51d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2093,16 +2093,11 @@ int load_vmstate(const char *name)
/* Flush all IO requests so they don't interfere with the new state. */
bdrv_drain_all();
- bs = NULL;
- while ((bs = bdrv_next(bs))) {
- if (bdrv_can_snapshot(bs)) {
- ret = bdrv_snapshot_goto(bs, name);
- if (ret < 0) {
- error_report("Error %d while activating snapshot '%s' on '%s'",
- ret, name, bdrv_get_device_name(bs));
- return ret;
- }
- }
+ ret = bdrv_all_goto_snapshot(name, &bs);
+ if (ret < 0) {
+ error_report("Error %d while activating snapshot '%s' on '%s'",
+ ret, name, bdrv_get_device_name(bs));
+ return ret;
}
/* restore the VM state */
--
2.5.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH 04/11] snapshot: create bdrv_all_goto_snapshot helper
2015-11-17 9:08 ` [Qemu-devel] [PATCH 04/11] snapshot: create bdrv_all_goto_snapshot helper Denis V. Lunev
@ 2015-11-18 11:02 ` Juan Quintela
0 siblings, 0 replies; 36+ messages in thread
From: Juan Quintela @ 2015-11-18 11:02 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, stefanha
"Denis V. Lunev" <den@openvz.org> wrote:
> to switch to snapshot on all loaded block drivers.
>
> The patch also ensures proper locking.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
suggestion:
un-export bdrv_snapshot_goto()?
I haven't looked if it is used anywhere else, but looks like one idea.
> ---
> block/snapshot.c | 20 ++++++++++++++++++++
> include/block/snapshot.h | 1 +
> migration/savevm.c | 15 +++++----------
> 3 files changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/block/snapshot.c b/block/snapshot.c
> index 61a6ad1..9f07a63 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -403,3 +403,23 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
> *first_bad_bs = bs;
> return ret;
> }
> +
> +
> +int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
> +{
> + int err = 0;
> + BlockDriverState *bs = NULL;
> +
> + while (err == 0 && (bs = bdrv_next(bs))) {
> + AioContext *ctx = bdrv_get_aio_context(bs);
> +
> + aio_context_acquire(ctx);
> + if (bdrv_can_snapshot(bs)) {
> + err = bdrv_snapshot_goto(bs, name);
> + }
> + aio_context_release(ctx);
> + }
> +
> + *first_bad_bs = bs;
> + return err;
> +}
> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> index d02d2b1..0a176c7 100644
> --- a/include/block/snapshot.h
> +++ b/include/block/snapshot.h
> @@ -84,5 +84,6 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
> bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
> int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
> Error **err);
> +int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
>
> #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index c52cbbe..254e51d 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2093,16 +2093,11 @@ int load_vmstate(const char *name)
> /* Flush all IO requests so they don't interfere with the new state. */
> bdrv_drain_all();
>
> - bs = NULL;
> - while ((bs = bdrv_next(bs))) {
> - if (bdrv_can_snapshot(bs)) {
> - ret = bdrv_snapshot_goto(bs, name);
> - if (ret < 0) {
> - error_report("Error %d while activating snapshot '%s' on '%s'",
> - ret, name, bdrv_get_device_name(bs));
> - return ret;
> - }
> - }
> + ret = bdrv_all_goto_snapshot(name, &bs);
> + if (ret < 0) {
> + error_report("Error %d while activating snapshot '%s' on '%s'",
> + ret, name, bdrv_get_device_name(bs));
> + return ret;
> }
>
> /* restore the VM state */
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 05/11] migration: factor our snapshottability check in load_vmstate
2015-11-17 9:08 [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes Denis V. Lunev
` (3 preceding siblings ...)
2015-11-17 9:08 ` [Qemu-devel] [PATCH 04/11] snapshot: create bdrv_all_goto_snapshot helper Denis V. Lunev
@ 2015-11-17 9:08 ` Denis V. Lunev
2015-11-17 11:51 ` Fam Zheng
2015-11-18 11:05 ` Juan Quintela
2015-11-17 9:08 ` [Qemu-devel] [PATCH 06/11] snapshot: create bdrv_all_find_snapshot helper Denis V. Lunev
` (8 subsequent siblings)
13 siblings, 2 replies; 36+ messages in thread
From: Denis V. Lunev @ 2015-11-17 9:08 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, stefanha, Juan Quintela
We should check that all inserted and not read-only images support
snapshotting. This could be made using already invented helper
bdrv_all_can_snapshot().
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
migration/savevm.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 254e51d..2ecc1b3 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2051,6 +2051,12 @@ int load_vmstate(const char *name)
QEMUFile *f;
int ret;
+ if (!bdrv_all_can_snapshot(&bs)) {
+ error_report("Device '%s' is writable but does not support snapshots.",
+ bdrv_get_device_name(bs));
+ return -ENOTSUP;
+ }
+
bs_vm_state = find_vmstate_bs();
if (!bs_vm_state) {
error_report("No block device supports snapshots");
@@ -2071,15 +2077,8 @@ int load_vmstate(const char *name)
writable and check if the requested snapshot is available too. */
bs = NULL;
while ((bs = bdrv_next(bs))) {
-
- if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
- continue;
- }
-
if (!bdrv_can_snapshot(bs)) {
- error_report("Device '%s' is writable but does not support snapshots.",
- bdrv_get_device_name(bs));
- return -ENOTSUP;
+ continue;
}
ret = bdrv_snapshot_find(bs, &sn, name);
--
2.5.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH 05/11] migration: factor our snapshottability check in load_vmstate
2015-11-17 9:08 ` [Qemu-devel] [PATCH 05/11] migration: factor our snapshottability check in load_vmstate Denis V. Lunev
@ 2015-11-17 11:51 ` Fam Zheng
2015-11-18 11:05 ` Juan Quintela
1 sibling, 0 replies; 36+ messages in thread
From: Fam Zheng @ 2015-11-17 11:51 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, stefanha, Juan Quintela
On Tue, 11/17 12:08, Denis V. Lunev wrote:
> We should check that all inserted and not read-only images support
> snapshotting. This could be made using already invented helper
> bdrv_all_can_snapshot().
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
> migration/savevm.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 254e51d..2ecc1b3 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2051,6 +2051,12 @@ int load_vmstate(const char *name)
> QEMUFile *f;
> int ret;
>
> + if (!bdrv_all_can_snapshot(&bs)) {
> + error_report("Device '%s' is writable but does not support snapshots.",
> + bdrv_get_device_name(bs));
> + return -ENOTSUP;
> + }
> +
> bs_vm_state = find_vmstate_bs();
> if (!bs_vm_state) {
> error_report("No block device supports snapshots");
> @@ -2071,15 +2077,8 @@ int load_vmstate(const char *name)
> writable and check if the requested snapshot is available too. */
> bs = NULL;
> while ((bs = bdrv_next(bs))) {
> -
> - if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
> - continue;
> - }
> -
> if (!bdrv_can_snapshot(bs)) {
> - error_report("Device '%s' is writable but does not support snapshots.",
> - bdrv_get_device_name(bs));
> - return -ENOTSUP;
> + continue;
> }
>
> ret = bdrv_snapshot_find(bs, &sn, name);
> --
> 2.5.0
>
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH 05/11] migration: factor our snapshottability check in load_vmstate
2015-11-17 9:08 ` [Qemu-devel] [PATCH 05/11] migration: factor our snapshottability check in load_vmstate Denis V. Lunev
2015-11-17 11:51 ` Fam Zheng
@ 2015-11-18 11:05 ` Juan Quintela
1 sibling, 0 replies; 36+ messages in thread
From: Juan Quintela @ 2015-11-18 11:05 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, stefanha
"Denis V. Lunev" <den@openvz.org> wrote:
> We should check that all inserted and not read-only images support
> snapshotting. This could be made using already invented helper
> bdrv_all_can_snapshot().
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Juan Quintela <quintela@redhat.com>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 06/11] snapshot: create bdrv_all_find_snapshot helper
2015-11-17 9:08 [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes Denis V. Lunev
` (4 preceding siblings ...)
2015-11-17 9:08 ` [Qemu-devel] [PATCH 05/11] migration: factor our snapshottability check in load_vmstate Denis V. Lunev
@ 2015-11-17 9:08 ` Denis V. Lunev
2015-11-17 11:55 ` Fam Zheng
2015-11-18 11:09 ` Juan Quintela
2015-11-17 9:08 ` [Qemu-devel] [PATCH 07/11] migration: drop find_vmstate_bs check in hmp_delvm Denis V. Lunev
` (7 subsequent siblings)
13 siblings, 2 replies; 36+ messages in thread
From: Denis V. Lunev @ 2015-11-17 9:08 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, stefanha, Juan Quintela
to check that snapshot is available for all loaded block drivers.
The check bs != bs1 in hmp_info_snapshots is an optimization. The check
for availability of this snapshot will return always true as the list
of snapshots was collected from that image.
The patch also ensures proper locking.
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
block/snapshot.c | 20 ++++++++++++++++++++
include/block/snapshot.h | 1 +
migration/savevm.c | 42 +++++++++---------------------------------
3 files changed, 30 insertions(+), 33 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index 9f07a63..eae4730 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -423,3 +423,23 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
*first_bad_bs = bs;
return err;
}
+
+int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
+{
+ QEMUSnapshotInfo sn;
+ int err = 0;
+ BlockDriverState *bs = NULL;
+
+ while (err == 0 && (bs = bdrv_next(bs))) {
+ AioContext *ctx = bdrv_get_aio_context(bs);
+
+ aio_context_acquire(ctx);
+ if (bdrv_can_snapshot(bs)) {
+ err = bdrv_snapshot_find(bs, &sn, name);
+ }
+ aio_context_release(ctx);
+ }
+
+ *first_bad_bs = bs;
+ return err;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 0a176c7..10ee582 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -85,5 +85,6 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
Error **err);
int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
+int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
#endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 2ecc1b3..4e6d578 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2056,6 +2056,12 @@ int load_vmstate(const char *name)
bdrv_get_device_name(bs));
return -ENOTSUP;
}
+ ret = bdrv_all_find_snapshot(name, &bs);
+ if (ret < 0) {
+ error_report("Device '%s' does not have the requested snapshot '%s'",
+ bdrv_get_device_name(bs), name);
+ return ret;
+ }
bs_vm_state = find_vmstate_bs();
if (!bs_vm_state) {
@@ -2073,22 +2079,6 @@ int load_vmstate(const char *name)
return -EINVAL;
}
- /* Verify if there is any device that doesn't support snapshots and is
- writable and check if the requested snapshot is available too. */
- bs = NULL;
- while ((bs = bdrv_next(bs))) {
- if (!bdrv_can_snapshot(bs)) {
- continue;
- }
-
- ret = bdrv_snapshot_find(bs, &sn, name);
- if (ret < 0) {
- error_report("Device '%s' does not have the requested snapshot '%s'",
- bdrv_get_device_name(bs), name);
- return ret;
- }
- }
-
/* Flush all IO requests so they don't interfere with the new state. */
bdrv_drain_all();
@@ -2142,8 +2132,8 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
{
BlockDriverState *bs, *bs1;
- QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
- int nb_sns, i, ret, available;
+ QEMUSnapshotInfo *sn_tab, *sn;
+ int nb_sns, i;
int total;
int *available_snapshots;
@@ -2167,21 +2157,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
available_snapshots = g_new0(int, nb_sns);
total = 0;
for (i = 0; i < nb_sns; i++) {
- sn = &sn_tab[i];
- available = 1;
- bs1 = NULL;
-
- while ((bs1 = bdrv_next(bs1))) {
- if (bdrv_can_snapshot(bs1) && bs1 != bs) {
- ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
- if (ret < 0) {
- available = 0;
- break;
- }
- }
- }
-
- if (available) {
+ if (bdrv_all_find_snapshot(sn_tab[i].id_str, &bs1) == 0) {
available_snapshots[total] = i;
total++;
}
--
2.5.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH 06/11] snapshot: create bdrv_all_find_snapshot helper
2015-11-17 9:08 ` [Qemu-devel] [PATCH 06/11] snapshot: create bdrv_all_find_snapshot helper Denis V. Lunev
@ 2015-11-17 11:55 ` Fam Zheng
2015-11-18 11:09 ` Juan Quintela
1 sibling, 0 replies; 36+ messages in thread
From: Fam Zheng @ 2015-11-17 11:55 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, stefanha, Juan Quintela
On Tue, 11/17 12:08, Denis V. Lunev wrote:
> to check that snapshot is available for all loaded block drivers.
> The check bs != bs1 in hmp_info_snapshots is an optimization. The check
> for availability of this snapshot will return always true as the list
> of snapshots was collected from that image.
>
> The patch also ensures proper locking.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
> block/snapshot.c | 20 ++++++++++++++++++++
> include/block/snapshot.h | 1 +
> migration/savevm.c | 42 +++++++++---------------------------------
> 3 files changed, 30 insertions(+), 33 deletions(-)
>
> diff --git a/block/snapshot.c b/block/snapshot.c
> index 9f07a63..eae4730 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -423,3 +423,23 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
> *first_bad_bs = bs;
> return err;
> }
> +
> +int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
> +{
> + QEMUSnapshotInfo sn;
> + int err = 0;
> + BlockDriverState *bs = NULL;
> +
> + while (err == 0 && (bs = bdrv_next(bs))) {
> + AioContext *ctx = bdrv_get_aio_context(bs);
> +
> + aio_context_acquire(ctx);
> + if (bdrv_can_snapshot(bs)) {
> + err = bdrv_snapshot_find(bs, &sn, name);
> + }
> + aio_context_release(ctx);
> + }
> +
> + *first_bad_bs = bs;
> + return err;
> +}
> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> index 0a176c7..10ee582 100644
> --- a/include/block/snapshot.h
> +++ b/include/block/snapshot.h
> @@ -85,5 +85,6 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
> int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
> Error **err);
> int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
> +int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
>
> #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 2ecc1b3..4e6d578 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2056,6 +2056,12 @@ int load_vmstate(const char *name)
> bdrv_get_device_name(bs));
> return -ENOTSUP;
> }
> + ret = bdrv_all_find_snapshot(name, &bs);
> + if (ret < 0) {
> + error_report("Device '%s' does not have the requested snapshot '%s'",
> + bdrv_get_device_name(bs), name);
> + return ret;
> + }
>
> bs_vm_state = find_vmstate_bs();
> if (!bs_vm_state) {
> @@ -2073,22 +2079,6 @@ int load_vmstate(const char *name)
> return -EINVAL;
> }
>
> - /* Verify if there is any device that doesn't support snapshots and is
> - writable and check if the requested snapshot is available too. */
> - bs = NULL;
> - while ((bs = bdrv_next(bs))) {
> - if (!bdrv_can_snapshot(bs)) {
> - continue;
> - }
> -
> - ret = bdrv_snapshot_find(bs, &sn, name);
> - if (ret < 0) {
> - error_report("Device '%s' does not have the requested snapshot '%s'",
> - bdrv_get_device_name(bs), name);
> - return ret;
> - }
> - }
> -
> /* Flush all IO requests so they don't interfere with the new state. */
> bdrv_drain_all();
>
> @@ -2142,8 +2132,8 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
> void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
> {
> BlockDriverState *bs, *bs1;
> - QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
> - int nb_sns, i, ret, available;
> + QEMUSnapshotInfo *sn_tab, *sn;
> + int nb_sns, i;
> int total;
> int *available_snapshots;
>
> @@ -2167,21 +2157,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
> available_snapshots = g_new0(int, nb_sns);
> total = 0;
> for (i = 0; i < nb_sns; i++) {
> - sn = &sn_tab[i];
> - available = 1;
> - bs1 = NULL;
> -
> - while ((bs1 = bdrv_next(bs1))) {
> - if (bdrv_can_snapshot(bs1) && bs1 != bs) {
> - ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
> - if (ret < 0) {
> - available = 0;
> - break;
> - }
> - }
> - }
> -
> - if (available) {
> + if (bdrv_all_find_snapshot(sn_tab[i].id_str, &bs1) == 0) {
> available_snapshots[total] = i;
> total++;
> }
> --
> 2.5.0
>
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH 06/11] snapshot: create bdrv_all_find_snapshot helper
2015-11-17 9:08 ` [Qemu-devel] [PATCH 06/11] snapshot: create bdrv_all_find_snapshot helper Denis V. Lunev
2015-11-17 11:55 ` Fam Zheng
@ 2015-11-18 11:09 ` Juan Quintela
1 sibling, 0 replies; 36+ messages in thread
From: Juan Quintela @ 2015-11-18 11:09 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, stefanha
"Denis V. Lunev" <den@openvz.org> wrote:
> to check that snapshot is available for all loaded block drivers.
> The check bs != bs1 in hmp_info_snapshots is an optimization. The check
> for availability of this snapshot will return always true as the list
> of snapshots was collected from that image.
>
> The patch also ensures proper locking.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Qemu-devel] [PATCH 07/11] migration: drop find_vmstate_bs check in hmp_delvm
2015-11-17 9:08 [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes Denis V. Lunev
` (5 preceding siblings ...)
2015-11-17 9:08 ` [Qemu-devel] [PATCH 06/11] snapshot: create bdrv_all_find_snapshot helper Denis V. Lunev
@ 2015-11-17 9:08 ` Denis V. Lunev
2015-11-18 11:08 ` Juan Quintela
2015-11-17 9:08 ` [Qemu-devel] [PATCH 08/11] snapshot: create bdrv_all_create_snapshot helper Denis V. Lunev
` (6 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Denis V. Lunev @ 2015-11-17 9:08 UTC (permalink / raw)
Cc: Denis V. Lunev, qemu-devel, stefanha, Juan Quintela
There is no much sense to do the check and write warning.
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
---
migration/savevm.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 4e6d578..63c07e1 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2116,11 +2116,6 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
Error *err;
const char *name = qdict_get_str(qdict, "name");
- if (!find_vmstate_bs()) {
- monitor_printf(mon, "No block device supports snapshots\n");
- return;
- }
-
if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
monitor_printf(mon,
"Error while deleting snapshot on device '%s': %s\n",
--
2.5.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* [Qemu-devel] [PATCH 08/11] snapshot: create bdrv_all_create_snapshot helper
2015-11-17 9:08 [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes Denis V. Lunev
` (6 preceding siblings ...)
2015-11-17 9:08 ` [Qemu-devel] [PATCH 07/11] migration: drop find_vmstate_bs check in hmp_delvm Denis V. Lunev
@ 2015-11-17 9:08 ` Denis V. Lunev
2015-11-18 11:10 ` Juan Quintela
2015-11-17 9:08 ` [Qemu-devel] [PATCH 09/11] migration: reorder processing in hmp_savevm Denis V. Lunev
` (5 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Denis V. Lunev @ 2015-11-17 9:08 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, stefanha, Juan Quintela
to create snapshot for all loaded block drivers.
The patch also ensures proper locking.
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
block/snapshot.c | 26 ++++++++++++++++++++++++++
include/block/snapshot.h | 4 ++++
migration/savevm.c | 17 ++++-------------
3 files changed, 34 insertions(+), 13 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index eae4730..75d0b96 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -443,3 +443,29 @@ int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
*first_bad_bs = bs;
return err;
}
+
+int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
+ BlockDriverState *vm_state_bs,
+ uint64_t vm_state_size,
+ BlockDriverState **first_bad_bs)
+{
+ int err = 0;
+ BlockDriverState *bs = NULL;
+
+ while (err == 0 && (bs = bdrv_next(bs))) {
+ AioContext *ctx = bdrv_get_aio_context(bs);
+
+ aio_context_acquire(ctx);
+ if (bs == vm_state_bs) {
+ sn->vm_state_size = vm_state_size;
+ err = bdrv_snapshot_create(bs, sn);
+ } else if (bdrv_can_snapshot(bs)) {
+ sn->vm_state_size = 0;
+ err = bdrv_snapshot_create(bs, sn);
+ }
+ aio_context_release(ctx);
+ }
+
+ *first_bad_bs = bs;
+ return err;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 10ee582..55e995a 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -86,5 +86,9 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
Error **err);
int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
+int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
+ BlockDriverState *vm_state_bs,
+ uint64_t vm_state_size,
+ BlockDriverState **first_bad_bs);
#endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 63c07e1..80107e3 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1996,19 +1996,10 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
goto the_end;
}
- /* create the snapshots */
-
- bs1 = NULL;
- while ((bs1 = bdrv_next(bs1))) {
- if (bdrv_can_snapshot(bs1)) {
- /* Write VM state size only to the image that contains the state */
- sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
- ret = bdrv_snapshot_create(bs1, sn);
- if (ret < 0) {
- monitor_printf(mon, "Error while creating snapshot on '%s'\n",
- bdrv_get_device_name(bs1));
- }
- }
+ ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
+ if (ret < 0) {
+ monitor_printf(mon, "Error while creating snapshot on '%s'\n",
+ bdrv_get_device_name(bs));
}
the_end:
--
2.5.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* [Qemu-devel] [PATCH 09/11] migration: reorder processing in hmp_savevm
2015-11-17 9:08 [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes Denis V. Lunev
` (7 preceding siblings ...)
2015-11-17 9:08 ` [Qemu-devel] [PATCH 08/11] snapshot: create bdrv_all_create_snapshot helper Denis V. Lunev
@ 2015-11-17 9:08 ` Denis V. Lunev
2015-11-18 11:15 ` Juan Quintela
2015-11-17 9:08 ` [Qemu-devel] [PATCH 10/11] migration: implement bdrv_all_find_vmstate_bs helper Denis V. Lunev
` (4 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Denis V. Lunev @ 2015-11-17 9:08 UTC (permalink / raw)
Cc: Denis V. Lunev, qemu-devel, stefanha, Juan Quintela
State deletion can be performed on running VM which reduces VM downtime
This approach looks a bit more natural.
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
---
migration/savevm.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 80107e3..98f9a8c 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1935,6 +1935,15 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
return;
}
+ /* Delete old snapshots of the same name */
+ if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
+ monitor_printf(mon,
+ "Error while deleting snapshot on device '%s': %s\n",
+ bdrv_get_device_name(bs1), error_get_pretty(local_err));
+ error_free(local_err);
+ return;
+ }
+
bs = find_vmstate_bs();
if (!bs) {
monitor_printf(mon, "No block device can accept snapshots\n");
@@ -1972,15 +1981,6 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
}
- /* Delete old snapshots of the same name */
- if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
- monitor_printf(mon,
- "Error while deleting snapshot on device '%s': %s\n",
- bdrv_get_device_name(bs1), error_get_pretty(local_err));
- error_free(local_err);
- goto the_end;
- }
-
/* save the VM state */
f = qemu_fopen_bdrv(bs, 1);
if (!f) {
--
2.5.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* [Qemu-devel] [PATCH 10/11] migration: implement bdrv_all_find_vmstate_bs helper
2015-11-17 9:08 [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes Denis V. Lunev
` (8 preceding siblings ...)
2015-11-17 9:08 ` [Qemu-devel] [PATCH 09/11] migration: reorder processing in hmp_savevm Denis V. Lunev
@ 2015-11-17 9:08 ` Denis V. Lunev
2015-11-18 11:12 ` Juan Quintela
2015-11-17 9:08 ` [Qemu-devel] [PATCH 11/11] migration: normalize locking in migration/savevm.c Denis V. Lunev
` (3 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Denis V. Lunev @ 2015-11-17 9:08 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, stefanha, Juan Quintela
The patch also ensures proper locking for the operation.
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
block/snapshot.c | 15 +++++++++++++++
include/block/snapshot.h | 2 ++
migration/savevm.c | 19 ++++---------------
3 files changed, 21 insertions(+), 15 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index 75d0b96..6e9fa8d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -469,3 +469,18 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
*first_bad_bs = bs;
return err;
}
+
+BlockDriverState *bdrv_all_find_vmstate_bs(void)
+{
+ bool not_found = true;
+ BlockDriverState *bs = NULL;
+
+ while (not_found && (bs = bdrv_next(bs))) {
+ AioContext *ctx = bdrv_get_aio_context(bs);
+
+ aio_context_acquire(ctx);
+ not_found = !bdrv_can_snapshot(bs);
+ aio_context_release(ctx);
+ }
+ return bs;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 55e995a..c6910da6 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -91,4 +91,6 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
uint64_t vm_state_size,
BlockDriverState **first_bad_bs);
+BlockDriverState *bdrv_all_find_vmstate_bs(void);
+
#endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 98f9a8c..a845e69 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1905,17 +1905,6 @@ int qemu_loadvm_state(QEMUFile *f)
return ret;
}
-static BlockDriverState *find_vmstate_bs(void)
-{
- BlockDriverState *bs = NULL;
- while ((bs = bdrv_next(bs))) {
- if (bdrv_can_snapshot(bs)) {
- return bs;
- }
- }
- return NULL;
-}
-
void hmp_savevm(Monitor *mon, const QDict *qdict)
{
BlockDriverState *bs, *bs1;
@@ -1944,8 +1933,8 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
return;
}
- bs = find_vmstate_bs();
- if (!bs) {
+ bs = bdrv_all_find_vmstate_bs();
+ if (bs == NULL) {
monitor_printf(mon, "No block device can accept snapshots\n");
return;
}
@@ -2054,7 +2043,7 @@ int load_vmstate(const char *name)
return ret;
}
- bs_vm_state = find_vmstate_bs();
+ bs_vm_state = bdrv_all_find_vmstate_bs();
if (!bs_vm_state) {
error_report("No block device supports snapshots");
return -ENOTSUP;
@@ -2123,7 +2112,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
int total;
int *available_snapshots;
- bs = find_vmstate_bs();
+ bs = bdrv_all_find_vmstate_bs();
if (!bs) {
monitor_printf(mon, "No available block device supports snapshots\n");
return;
--
2.5.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* [Qemu-devel] [PATCH 11/11] migration: normalize locking in migration/savevm.c
2015-11-17 9:08 [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes Denis V. Lunev
` (9 preceding siblings ...)
2015-11-17 9:08 ` [Qemu-devel] [PATCH 10/11] migration: implement bdrv_all_find_vmstate_bs helper Denis V. Lunev
@ 2015-11-17 9:08 ` Denis V. Lunev
2015-11-18 11:25 ` Juan Quintela
2015-11-18 13:59 ` Greg Kurz
2015-11-18 10:56 ` [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes Juan Quintela
` (2 subsequent siblings)
13 siblings, 2 replies; 36+ messages in thread
From: Denis V. Lunev @ 2015-11-17 9:08 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, stefanha, Juan Quintela
basically all bdrv_* operations must be called under aio_context_acquire
except ones with bdrv_all prefix.
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
migration/savevm.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index a845e69..09e6a43 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1917,6 +1917,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
struct tm tm;
const char *name = qdict_get_try_str(qdict, "name");
Error *local_err = NULL;
+ AioContext *aio_context;
if (!bdrv_all_can_snapshot(&bs)) {
monitor_printf(mon, "Device '%s' is writable but does not "
@@ -1938,6 +1939,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
monitor_printf(mon, "No block device can accept snapshots\n");
return;
}
+ aio_context = bdrv_get_aio_context(bs);
saved_vm_running = runstate_is_running();
@@ -1948,6 +1950,8 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
}
vm_stop(RUN_STATE_SAVE_VM);
+ aio_context_acquire(aio_context);
+
memset(sn, 0, sizeof(*sn));
/* fill auxiliary fields */
@@ -1992,6 +1996,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
}
the_end:
+ aio_context_release(aio_context);
if (saved_vm_running) {
vm_start();
}
@@ -2030,6 +2035,7 @@ int load_vmstate(const char *name)
QEMUSnapshotInfo sn;
QEMUFile *f;
int ret;
+ AioContext *aio_context;
if (!bdrv_all_can_snapshot(&bs)) {
error_report("Device '%s' is writable but does not support snapshots.",
@@ -2048,9 +2054,12 @@ int load_vmstate(const char *name)
error_report("No block device supports snapshots");
return -ENOTSUP;
}
+ aio_context = bdrv_get_aio_context(bs);
/* Don't even try to load empty VM states */
+ aio_context_acquire(aio_context);
ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
+ aio_context_release(aio_context);
if (ret < 0) {
return ret;
} else if (sn.vm_state_size == 0) {
@@ -2078,9 +2087,12 @@ int load_vmstate(const char *name)
qemu_system_reset(VMRESET_SILENT);
migration_incoming_state_new(f);
- ret = qemu_loadvm_state(f);
+ aio_context_acquire(aio_context);
+ ret = qemu_loadvm_state(f);
qemu_fclose(f);
+ aio_context_release(aio_context);
+
migration_incoming_state_destroy();
if (ret < 0) {
error_report("Error %d while loading VM state", ret);
@@ -2111,14 +2123,19 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
int nb_sns, i;
int total;
int *available_snapshots;
+ AioContext *aio_context;
bs = bdrv_all_find_vmstate_bs();
if (!bs) {
monitor_printf(mon, "No available block device supports snapshots\n");
return;
}
+ aio_context = bdrv_get_aio_context(bs);
+ aio_context_acquire(aio_context);
nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+ aio_context_release(aio_context);
+
if (nb_sns < 0) {
monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
return;
--
2.5.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH 11/11] migration: normalize locking in migration/savevm.c
2015-11-17 9:08 ` [Qemu-devel] [PATCH 11/11] migration: normalize locking in migration/savevm.c Denis V. Lunev
@ 2015-11-18 11:25 ` Juan Quintela
2015-11-18 14:26 ` Denis V. Lunev
2015-11-18 13:59 ` Greg Kurz
1 sibling, 1 reply; 36+ messages in thread
From: Juan Quintela @ 2015-11-18 11:25 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, stefanha
"Denis V. Lunev" <den@openvz.org> wrote:
> basically all bdrv_* operations must be called under aio_context_acquire
> except ones with bdrv_all prefix.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
I will preffer that migration code don't know about aiocontexts.
> @@ -2048,9 +2054,12 @@ int load_vmstate(const char *name)
> error_report("No block device supports snapshots");
> return -ENOTSUP;
> }
> + aio_context = bdrv_get_aio_context(bs);
>
> /* Don't even try to load empty VM states */
> + aio_context_acquire(aio_context);
> ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
> + aio_context_release(aio_context);
Why are we dropping it here?
I can understand doing it on the error case.
> if (ret < 0) {
> return ret;
> } else if (sn.vm_state_size == 0) {
> @@ -2078,9 +2087,12 @@ int load_vmstate(const char *name)
>
> qemu_system_reset(VMRESET_SILENT);
> migration_incoming_state_new(f);
> - ret = qemu_loadvm_state(f);
>
> + aio_context_acquire(aio_context);
We have done a qemu_fopen_bdrv() without acquiring the context, not sure
if we really need it (or not). My understanding of locking is that we
should get the context on first use and maintain it until last use.
Does it work in a different way?
> + ret = qemu_loadvm_state(f);
> qemu_fclose(f);
> + aio_context_release(aio_context);
> +
> migration_incoming_state_destroy();
> if (ret < 0) {
> error_report("Error %d while loading VM state", ret);
> @@ -2111,14 +2123,19 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
> int nb_sns, i;
> int total;
> int *available_snapshots;
> + AioContext *aio_context;
>
> bs = bdrv_all_find_vmstate_bs();
> if (!bs) {
> monitor_printf(mon, "No available block device supports snapshots\n");
> return;
> }
> + aio_context = bdrv_get_aio_context(bs);
>
> + aio_context_acquire(aio_context);
> nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> + aio_context_release(aio_context);
> +
> if (nb_sns < 0) {
> monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
> return;
I will very much preffer to have a:
bdrv_snapshot_list_full_whatever()
That does the two operations and don't make me know about aio_contexts.
What I need there really is just the block layer to fill the sn_tab and
to told me if there is a problem, no real need of knowing about
contexts, no?
Thanks, Juan.
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH 11/11] migration: normalize locking in migration/savevm.c
2015-11-18 11:25 ` Juan Quintela
@ 2015-11-18 14:26 ` Denis V. Lunev
0 siblings, 0 replies; 36+ messages in thread
From: Denis V. Lunev @ 2015-11-18 14:26 UTC (permalink / raw)
To: quintela; +Cc: Kevin Wolf, qemu-devel, stefanha
On 11/18/2015 02:25 PM, Juan Quintela wrote:
> "Denis V. Lunev" <den@openvz.org> wrote:
>> basically all bdrv_* operations must be called under aio_context_acquire
>> except ones with bdrv_all prefix.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Reviewed-by: Fam Zheng <famz@redhat.com>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
> I will preffer that migration code don't know about aiocontexts.
>
>
>> @@ -2048,9 +2054,12 @@ int load_vmstate(const char *name)
>> error_report("No block device supports snapshots");
>> return -ENOTSUP;
>> }
>> + aio_context = bdrv_get_aio_context(bs);
>>
>> /* Don't even try to load empty VM states */
>> + aio_context_acquire(aio_context);
>> ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
>> + aio_context_release(aio_context);
> Why are we dropping it here?
> I can understand doing it on the error case.
acquire == lock
release == unlock
The lock should be dropped....
>> if (ret < 0) {
>> return ret;
>> } else if (sn.vm_state_size == 0) {
>> @@ -2078,9 +2087,12 @@ int load_vmstate(const char *name)
>>
>> qemu_system_reset(VMRESET_SILENT);
>> migration_incoming_state_new(f);
>> - ret = qemu_loadvm_state(f);
>>
>> + aio_context_acquire(aio_context);
> We have done a qemu_fopen_bdrv() without acquiring the context, not sure
> if we really need it (or not). My understanding of locking is that we
> should get the context on first use and maintain it until last use.
>
> Does it work in a different way?
>
I was requested to drop that code in one of the versions.
Stefan, Juan, can you pls come into agreement.
>> + ret = qemu_loadvm_state(f);
>> qemu_fclose(f);
>> + aio_context_release(aio_context);
>> +
>> migration_incoming_state_destroy();
>> if (ret < 0) {
>> error_report("Error %d while loading VM state", ret);
>> @@ -2111,14 +2123,19 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
>> int nb_sns, i;
>> int total;
>> int *available_snapshots;
>> + AioContext *aio_context;
>>
>> bs = bdrv_all_find_vmstate_bs();
>> if (!bs) {
>> monitor_printf(mon, "No available block device supports snapshots\n");
>> return;
>> }
>> + aio_context = bdrv_get_aio_context(bs);
>>
>> + aio_context_acquire(aio_context);
>> nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>> + aio_context_release(aio_context);
>> +
>> if (nb_sns < 0) {
>> monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
>> return;
>
> I will very much preffer to have a:
>
>
> bdrv_snapshot_list_full_whatever()
>
> That does the two operations and don't make me know about aio_contexts.
> What I need there really is just the block layer to fill the sn_tab and
> to told me if there is a problem, no real need of knowing about
> contexts, no?
>
> Thanks, Juan.
>
I know :( This is the most thing I can do at the moment keeping
agreement from Stefan :(
Den
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH 11/11] migration: normalize locking in migration/savevm.c
2015-11-17 9:08 ` [Qemu-devel] [PATCH 11/11] migration: normalize locking in migration/savevm.c Denis V. Lunev
2015-11-18 11:25 ` Juan Quintela
@ 2015-11-18 13:59 ` Greg Kurz
1 sibling, 0 replies; 36+ messages in thread
From: Greg Kurz @ 2015-11-18 13:59 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, stefanha, Juan Quintela
On Tue, 17 Nov 2015 12:08:31 +0300
"Denis V. Lunev" <den@openvz.org> wrote:
> basically all bdrv_* operations must be called under aio_context_acquire
> except ones with bdrv_all prefix.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
> migration/savevm.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index a845e69..09e6a43 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1917,6 +1917,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
> struct tm tm;
> const char *name = qdict_get_try_str(qdict, "name");
> Error *local_err = NULL;
> + AioContext *aio_context;
>
> if (!bdrv_all_can_snapshot(&bs)) {
> monitor_printf(mon, "Device '%s' is writable but does not "
> @@ -1938,6 +1939,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
> monitor_printf(mon, "No block device can accept snapshots\n");
> return;
> }
> + aio_context = bdrv_get_aio_context(bs);
>
> saved_vm_running = runstate_is_running();
>
> @@ -1948,6 +1950,8 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
> }
> vm_stop(RUN_STATE_SAVE_VM);
>
> + aio_context_acquire(aio_context);
> +
> memset(sn, 0, sizeof(*sn));
>
> /* fill auxiliary fields */
> @@ -1992,6 +1996,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
> }
>
> the_end:
> + aio_context_release(aio_context);
> if (saved_vm_running) {
> vm_start();
> }
> @@ -2030,6 +2035,7 @@ int load_vmstate(const char *name)
> QEMUSnapshotInfo sn;
> QEMUFile *f;
> int ret;
> + AioContext *aio_context;
>
> if (!bdrv_all_can_snapshot(&bs)) {
> error_report("Device '%s' is writable but does not support snapshots.",
> @@ -2048,9 +2054,12 @@ int load_vmstate(const char *name)
> error_report("No block device supports snapshots");
> return -ENOTSUP;
> }
> + aio_context = bdrv_get_aio_context(bs);
>
bs is used for error reporting earlier in this function.
bs_vm_state is the one to be used here.
> /* Don't even try to load empty VM states */
> + aio_context_acquire(aio_context);
> ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
> + aio_context_release(aio_context);
> if (ret < 0) {
> return ret;
> } else if (sn.vm_state_size == 0) {
> @@ -2078,9 +2087,12 @@ int load_vmstate(const char *name)
>
> qemu_system_reset(VMRESET_SILENT);
> migration_incoming_state_new(f);
> - ret = qemu_loadvm_state(f);
>
> + aio_context_acquire(aio_context);
> + ret = qemu_loadvm_state(f);
> qemu_fclose(f);
> + aio_context_release(aio_context);
> +
> migration_incoming_state_destroy();
> if (ret < 0) {
> error_report("Error %d while loading VM state", ret);
> @@ -2111,14 +2123,19 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
> int nb_sns, i;
> int total;
> int *available_snapshots;
> + AioContext *aio_context;
>
> bs = bdrv_all_find_vmstate_bs();
> if (!bs) {
> monitor_printf(mon, "No available block device supports snapshots\n");
> return;
> }
> + aio_context = bdrv_get_aio_context(bs);
>
> + aio_context_acquire(aio_context);
> nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> + aio_context_release(aio_context);
> +
> if (nb_sns < 0) {
> monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
> return;
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes
2015-11-17 9:08 [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes Denis V. Lunev
` (10 preceding siblings ...)
2015-11-17 9:08 ` [Qemu-devel] [PATCH 11/11] migration: normalize locking in migration/savevm.c Denis V. Lunev
@ 2015-11-18 10:56 ` Juan Quintela
2015-11-18 11:09 ` Denis V. Lunev
2015-11-18 11:26 ` Juan Quintela
2015-11-18 14:15 ` Greg Kurz
13 siblings, 1 reply; 36+ messages in thread
From: Juan Quintela @ 2015-11-18 10:56 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, stefanha
"Denis V. Lunev" <den@openvz.org> wrote:
for 2.9? You really plane very well in advance? O:-)
As you asked for review from migration side ....
> with test
> while /bin/true ; do
> virsh snapshot-create rhel7
> sleep 10
> virsh snapshot-delete rhel7 --current
> done
> with enabled iothreads on a running VM leads to a lot of troubles: hangs,
> asserts, errors.
>
> Anyway, I think that the construction like
> assert(aio_context_is_locked(aio_context));
> should be widely used to ensure proper locking.
>
> Changes from v8:
> - split patch 5 into 2 separate patches making changes better to understand and
> review
>
> Changes from v7:
> - patch 5: fixes for bdrv_all_find_snapshot. Changed parameter name to
> skip_read_only which better reflects the meaning of the value and
> fixed checks inside to conform the note from Stefan
>
> Changes from v6:
> - tricky part dropped from patch 7
> - patch 5 reworked to process snapshot list differently in info commands
> and on savevm
>
> Changes from v5:
> - dropped already merged patch 11
> - fixed spelling in patch 1
> - changed order of condition in loops in all patches. Thank you Stefan
> - dropped patch 9
> - aio_context is not acquired any more in bdrv_all_find_vmstate_bs by request
> of Stefan
> - patch 10 is implemented in completely different way
>
> Changes from v4:
> - only migration/savevm.c code and monitor is affected now. Generic block
> layer stuff will be sent separately to speedup merging. The approach
> in general was negotiated with Juan and Stefan.
>
> Changes from v3:
> - more places found
> - new aio_poll concept, see patch 10
>
> Changes from v2:
> - droppped patch 5 as already merged
> - changed locking scheme in patch 4 by suggestion of Juan
>
> Changes from v1:
> - aio-context locking added
> - comment is rewritten
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
>
> Denis V. Lunev (10):
> snapshot: create helper to test that block drivers supports snapshots
> snapshot: return error code from bdrv_snapshot_delete_by_id_or_name
> snapshot: create bdrv_all_delete_snapshot helper
> snapshot: create bdrv_all_goto_snapshot helper
> snapshot: create bdrv_all_find_snapshot helper
> migration: drop find_vmstate_bs check in hmp_delvm
> snapshot: create bdrv_all_create_snapshot helper
> migration: reorder processing in hmp_savevm
> migration: implement bdrv_all_find_vmstate_bs helper
> migration: normalize locking in migration/savevm.c
>
> block/snapshot.c | 135 ++++++++++++++++++++++++++++++-
> include/block/snapshot.h | 25 +++++-
> migration/savevm.c | 207 +++++++++++++++--------------------------------
> 3 files changed, 217 insertions(+), 150 deletions(-)
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes
2015-11-17 9:08 [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes Denis V. Lunev
` (11 preceding siblings ...)
2015-11-18 10:56 ` [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes Juan Quintela
@ 2015-11-18 11:26 ` Juan Quintela
2015-11-18 14:15 ` Greg Kurz
13 siblings, 0 replies; 36+ messages in thread
From: Juan Quintela @ 2015-11-18 11:26 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, stefanha
"Denis V. Lunev" <den@openvz.org> wrote:
Hi
Kevin, Stefan
> with test
> while /bin/true ; do
> virsh snapshot-create rhel7
> sleep 10
> virsh snapshot-delete rhel7 --current
> done
> with enabled iothreads on a running VM leads to a lot of troubles: hangs,
> asserts, errors.
>
> Anyway, I think that the construction like
> assert(aio_context_is_locked(aio_context));
> should be widely used to ensure proper locking.
>
> Changes from v8:
> - split patch 5 into 2 separate patches making changes better to understand and
> review
>
> Changes from v7:
> - patch 5: fixes for bdrv_all_find_snapshot. Changed parameter name to
> skip_read_only which better reflects the meaning of the value and
> fixed checks inside to conform the note from Stefan
>
> Changes from v6:
> - tricky part dropped from patch 7
> - patch 5 reworked to process snapshot list differently in info commands
> and on savevm
>
> Changes from v5:
> - dropped already merged patch 11
> - fixed spelling in patch 1
> - changed order of condition in loops in all patches. Thank you Stefan
> - dropped patch 9
> - aio_context is not acquired any more in bdrv_all_find_vmstate_bs by request
> of Stefan
> - patch 10 is implemented in completely different way
>
> Changes from v4:
> - only migration/savevm.c code and monitor is affected now. Generic block
> layer stuff will be sent separately to speedup merging. The approach
> in general was negotiated with Juan and Stefan.
>
> Changes from v3:
> - more places found
> - new aio_poll concept, see patch 10
>
> Changes from v2:
> - droppped patch 5 as already merged
> - changed locking scheme in patch 4 by suggestion of Juan
>
> Changes from v1:
> - aio-context locking added
> - comment is rewritten
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
>
> Denis V. Lunev (10):
> snapshot: create helper to test that block drivers supports snapshots
> snapshot: return error code from bdrv_snapshot_delete_by_id_or_name
> snapshot: create bdrv_all_delete_snapshot helper
> snapshot: create bdrv_all_goto_snapshot helper
> snapshot: create bdrv_all_find_snapshot helper
> migration: drop find_vmstate_bs check in hmp_delvm
> snapshot: create bdrv_all_create_snapshot helper
> migration: reorder processing in hmp_savevm
> migration: implement bdrv_all_find_vmstate_bs helper
> migration: normalize locking in migration/savevm.c
>
> block/snapshot.c | 135 ++++++++++++++++++++++++++++++-
> include/block/snapshot.h | 25 +++++-
> migration/savevm.c | 207 +++++++++++++++--------------------------------
> 3 files changed, 217 insertions(+), 150 deletions(-)
There are more changes on migration/* that on block/*, do you want to
take the changes yourselves or should I take them?
Should they go for 2.5 or 2.6?
Both options are ok with me.
Later, Juan.
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes
2015-11-17 9:08 [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes Denis V. Lunev
` (12 preceding siblings ...)
2015-11-18 11:26 ` Juan Quintela
@ 2015-11-18 14:15 ` Greg Kurz
2015-11-18 14:31 ` Juan Quintela
13 siblings, 1 reply; 36+ messages in thread
From: Greg Kurz @ 2015-11-18 14:15 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, stefanha, Juan Quintela
On Tue, 17 Nov 2015 12:08:20 +0300
"Denis V. Lunev" <den@openvz.org> wrote:
> with test
> while /bin/true ; do
> virsh snapshot-create rhel7
> sleep 10
> virsh snapshot-delete rhel7 --current
> done
> with enabled iothreads on a running VM leads to a lot of troubles: hangs,
> asserts, errors.
>
In my case, when using a virtio-blk-dataplane device, calling savevm *always*
result in a QEMU hang.
With this series (plus the s/bs/bs_vm_state/ change in patch 11), savevm/loadvm
now works like a charm.
I saw that Juan does not like aio_context being used in migration code, but
in case this series gets applied anyway:
Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Anyway, I think that the construction like
> assert(aio_context_is_locked(aio_context));
> should be widely used to ensure proper locking.
>
> Changes from v8:
> - split patch 5 into 2 separate patches making changes better to understand and
> review
>
> Changes from v7:
> - patch 5: fixes for bdrv_all_find_snapshot. Changed parameter name to
> skip_read_only which better reflects the meaning of the value and
> fixed checks inside to conform the note from Stefan
>
> Changes from v6:
> - tricky part dropped from patch 7
> - patch 5 reworked to process snapshot list differently in info commands
> and on savevm
>
> Changes from v5:
> - dropped already merged patch 11
> - fixed spelling in patch 1
> - changed order of condition in loops in all patches. Thank you Stefan
> - dropped patch 9
> - aio_context is not acquired any more in bdrv_all_find_vmstate_bs by request
> of Stefan
> - patch 10 is implemented in completely different way
>
> Changes from v4:
> - only migration/savevm.c code and monitor is affected now. Generic block
> layer stuff will be sent separately to speedup merging. The approach
> in general was negotiated with Juan and Stefan.
>
> Changes from v3:
> - more places found
> - new aio_poll concept, see patch 10
>
> Changes from v2:
> - droppped patch 5 as already merged
> - changed locking scheme in patch 4 by suggestion of Juan
>
> Changes from v1:
> - aio-context locking added
> - comment is rewritten
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
>
> Denis V. Lunev (10):
> snapshot: create helper to test that block drivers supports snapshots
> snapshot: return error code from bdrv_snapshot_delete_by_id_or_name
> snapshot: create bdrv_all_delete_snapshot helper
> snapshot: create bdrv_all_goto_snapshot helper
> snapshot: create bdrv_all_find_snapshot helper
> migration: drop find_vmstate_bs check in hmp_delvm
> snapshot: create bdrv_all_create_snapshot helper
> migration: reorder processing in hmp_savevm
> migration: implement bdrv_all_find_vmstate_bs helper
> migration: normalize locking in migration/savevm.c
>
> block/snapshot.c | 135 ++++++++++++++++++++++++++++++-
> include/block/snapshot.h | 25 +++++-
> migration/savevm.c | 207 +++++++++++++++--------------------------------
> 3 files changed, 217 insertions(+), 150 deletions(-)
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes
2015-11-18 14:15 ` Greg Kurz
@ 2015-11-18 14:31 ` Juan Quintela
2015-11-18 14:54 ` Denis V. Lunev
0 siblings, 1 reply; 36+ messages in thread
From: Juan Quintela @ 2015-11-18 14:31 UTC (permalink / raw)
To: Greg Kurz; +Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, stefanha
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> On Tue, 17 Nov 2015 12:08:20 +0300
> "Denis V. Lunev" <den@openvz.org> wrote:
>
>> with test
>> while /bin/true ; do
>> virsh snapshot-create rhel7
>> sleep 10
>> virsh snapshot-delete rhel7 --current
>> done
>> with enabled iothreads on a running VM leads to a lot of troubles: hangs,
>> asserts, errors.
>>
>
> In my case, when using a virtio-blk-dataplane device, calling savevm *always*
> result in a QEMU hang.
Oops
> With this series (plus the s/bs/bs_vm_state/ change in patch 11), savevm/loadvm
> now works like a charm.
Nice, thanks for the testing.
> I saw that Juan does not like aio_context being used in migration code, but
> in case this series gets applied anyway:
>
> Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
I *think* that we should get better API's exported from block layer, but
*at least* we will get this series in.
Thanks, Juan.
>
>> Anyway, I think that the construction like
>> assert(aio_context_is_locked(aio_context));
>> should be widely used to ensure proper locking.
>>
>> Changes from v8:
>> - split patch 5 into 2 separate patches making changes better to understand and
>> review
>>
>> Changes from v7:
>> - patch 5: fixes for bdrv_all_find_snapshot. Changed parameter name to
>> skip_read_only which better reflects the meaning of the value and
>> fixed checks inside to conform the note from Stefan
>>
>> Changes from v6:
>> - tricky part dropped from patch 7
>> - patch 5 reworked to process snapshot list differently in info commands
>> and on savevm
>>
>> Changes from v5:
>> - dropped already merged patch 11
>> - fixed spelling in patch 1
>> - changed order of condition in loops in all patches. Thank you Stefan
>> - dropped patch 9
>> - aio_context is not acquired any more in bdrv_all_find_vmstate_bs by request
>> of Stefan
>> - patch 10 is implemented in completely different way
>>
>> Changes from v4:
>> - only migration/savevm.c code and monitor is affected now. Generic block
>> layer stuff will be sent separately to speedup merging. The approach
>> in general was negotiated with Juan and Stefan.
>>
>> Changes from v3:
>> - more places found
>> - new aio_poll concept, see patch 10
>>
>> Changes from v2:
>> - droppped patch 5 as already merged
>> - changed locking scheme in patch 4 by suggestion of Juan
>>
>> Changes from v1:
>> - aio-context locking added
>> - comment is rewritten
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>>
>> Denis V. Lunev (10):
>> snapshot: create helper to test that block drivers supports snapshots
>> snapshot: return error code from bdrv_snapshot_delete_by_id_or_name
>> snapshot: create bdrv_all_delete_snapshot helper
>> snapshot: create bdrv_all_goto_snapshot helper
>> snapshot: create bdrv_all_find_snapshot helper
>> migration: drop find_vmstate_bs check in hmp_delvm
>> snapshot: create bdrv_all_create_snapshot helper
>> migration: reorder processing in hmp_savevm
>> migration: implement bdrv_all_find_vmstate_bs helper
>> migration: normalize locking in migration/savevm.c
>>
>> block/snapshot.c | 135 ++++++++++++++++++++++++++++++-
>> include/block/snapshot.h | 25 +++++-
>> migration/savevm.c | 207 +++++++++++++++--------------------------------
>> 3 files changed, 217 insertions(+), 150 deletions(-)
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes
2015-11-18 14:31 ` Juan Quintela
@ 2015-11-18 14:54 ` Denis V. Lunev
2015-11-18 15:24 ` Juan Quintela
0 siblings, 1 reply; 36+ messages in thread
From: Denis V. Lunev @ 2015-11-18 14:54 UTC (permalink / raw)
To: quintela, Greg Kurz; +Cc: Kevin Wolf, qemu-devel, stefanha
On 11/18/2015 05:31 PM, Juan Quintela wrote:
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
>> On Tue, 17 Nov 2015 12:08:20 +0300
>> "Denis V. Lunev" <den@openvz.org> wrote:
>>
>>> with test
>>> while /bin/true ; do
>>> virsh snapshot-create rhel7
>>> sleep 10
>>> virsh snapshot-delete rhel7 --current
>>> done
>>> with enabled iothreads on a running VM leads to a lot of troubles: hangs,
>>> asserts, errors.
>>>
>> In my case, when using a virtio-blk-dataplane device, calling savevm *always*
>> result in a QEMU hang.
> Oops
>
>> With this series (plus the s/bs/bs_vm_state/ change in patch 11), savevm/loadvm
>> now works like a charm.
> Nice, thanks for the testing.
>
>> I saw that Juan does not like aio_context being used in migration code, but
>> in case this series gets applied anyway:
>>
>> Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> I *think* that we should get better API's exported from block layer, but
> *at least* we will get this series in.
>
> Thanks, Juan.
that is good to me. Current block level API is terrible and unclear.
Greg is correct there.
Should I resubmit the last patch or you will change this yourself?
Den
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes
2015-11-18 14:54 ` Denis V. Lunev
@ 2015-11-18 15:24 ` Juan Quintela
2015-11-18 15:27 ` Denis V. Lunev
0 siblings, 1 reply; 36+ messages in thread
From: Juan Quintela @ 2015-11-18 15:24 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, stefanha, Greg Kurz
"Denis V. Lunev" <den@openvz.org> wrote:
> On 11/18/2015 05:31 PM, Juan Quintela wrote:
>> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
>>> On Tue, 17 Nov 2015 12:08:20 +0300
>>> "Denis V. Lunev" <den@openvz.org> wrote:
>>>
>>>> with test
>>>> while /bin/true ; do
>>>> virsh snapshot-create rhel7
>>>> sleep 10
>>>> virsh snapshot-delete rhel7 --current
>>>> done
>>>> with enabled iothreads on a running VM leads to a lot of troubles: hangs,
>>>> asserts, errors.
>>>>
>>> In my case, when using a virtio-blk-dataplane device, calling savevm *always*
>>> result in a QEMU hang.
>> Oops
>>
>>> With this series (plus the s/bs/bs_vm_state/ change in patch 11), savevm/loadvm
>>> now works like a charm.
>> Nice, thanks for the testing.
>>
>>> I saw that Juan does not like aio_context being used in migration code, but
>>> in case this series gets applied anyway:
>>>
>>> Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>> I *think* that we should get better API's exported from block layer, but
>> *at least* we will get this series in.
>>
>> Thanks, Juan.
>
> that is good to me. Current block level API is terrible and unclear.
>
> Greg is correct there.
>
> Should I resubmit the last patch or you will change this yourself?
Please, test and resend.
I am not have lots of experience testing that.
Thanks, Juan.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [PATCH for 2.9 v8 0/10] dataplane snapshot fixes
2015-11-18 15:24 ` Juan Quintela
@ 2015-11-18 15:27 ` Denis V. Lunev
0 siblings, 0 replies; 36+ messages in thread
From: Denis V. Lunev @ 2015-11-18 15:27 UTC (permalink / raw)
To: quintela; +Cc: Kevin Wolf, qemu-devel, stefanha, Greg Kurz
On 11/18/2015 06:24 PM, Juan Quintela wrote:
> "Denis V. Lunev" <den@openvz.org> wrote:
>> On 11/18/2015 05:31 PM, Juan Quintela wrote:
>>> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
>>>> On Tue, 17 Nov 2015 12:08:20 +0300
>>>> "Denis V. Lunev" <den@openvz.org> wrote:
>>>>
>>>>> with test
>>>>> while /bin/true ; do
>>>>> virsh snapshot-create rhel7
>>>>> sleep 10
>>>>> virsh snapshot-delete rhel7 --current
>>>>> done
>>>>> with enabled iothreads on a running VM leads to a lot of troubles: hangs,
>>>>> asserts, errors.
>>>>>
>>>> In my case, when using a virtio-blk-dataplane device, calling savevm *always*
>>>> result in a QEMU hang.
>>> Oops
>>>
>>>> With this series (plus the s/bs/bs_vm_state/ change in patch 11), savevm/loadvm
>>>> now works like a charm.
>>> Nice, thanks for the testing.
>>>
>>>> I saw that Juan does not like aio_context being used in migration code, but
>>>> in case this series gets applied anyway:
>>>>
>>>> Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>>> I *think* that we should get better API's exported from block layer, but
>>> *at least* we will get this series in.
>>>
>>> Thanks, Juan.
>> that is good to me. Current block level API is terrible and unclear.
>>
>> Greg is correct there.
>>
>> Should I resubmit the last patch or you will change this yourself?
> Please, test and resend.
>
> I am not have lots of experience testing that.
>
> Thanks, Juan.
perfect! Give me 2-3 hours pls.
^ permalink raw reply [flat|nested] 36+ messages in thread