* [Qemu-devel] [PATCH 01/10] snapshot: create helper to test that block drivers supports snapshots
2015-11-10 14:25 [Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes Denis V. Lunev
@ 2015-11-10 14:25 ` Denis V. Lunev
2015-11-10 14:25 ` [Qemu-devel] [PATCH 02/10] snapshot: return error code from bdrv_snapshot_delete_by_id_or_name Denis V. Lunev
` (10 subsequent siblings)
11 siblings, 0 replies; 20+ messages in thread
From: Denis V. Lunev @ 2015-11-10 14:25 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi, 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>
CC: Juan Quintela <quintela@redhat.com>
CC: Stefan Hajnoczi <stefanha@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 9f2230f..c212288 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1290,19 +1290,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] 20+ messages in thread
* [Qemu-devel] [PATCH 02/10] snapshot: return error code from bdrv_snapshot_delete_by_id_or_name
2015-11-10 14:25 [Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes Denis V. Lunev
2015-11-10 14:25 ` [Qemu-devel] [PATCH 01/10] snapshot: create helper to test that block drivers supports snapshots Denis V. Lunev
@ 2015-11-10 14:25 ` Denis V. Lunev
2015-11-10 14:25 ` [Qemu-devel] [PATCH 03/10] snapshot: create bdrv_all_delete_snapshot helper Denis V. Lunev
` (9 subsequent siblings)
11 siblings, 0 replies; 20+ messages in thread
From: Denis V. Lunev @ 2015-11-10 14:25 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi, Juan Quintela
this will make code better in the next patch
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 | 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] 20+ messages in thread
* [Qemu-devel] [PATCH 03/10] snapshot: create bdrv_all_delete_snapshot helper
2015-11-10 14:25 [Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes Denis V. Lunev
2015-11-10 14:25 ` [Qemu-devel] [PATCH 01/10] snapshot: create helper to test that block drivers supports snapshots Denis V. Lunev
2015-11-10 14:25 ` [Qemu-devel] [PATCH 02/10] snapshot: return error code from bdrv_snapshot_delete_by_id_or_name Denis V. Lunev
@ 2015-11-10 14:25 ` Denis V. Lunev
2015-11-10 14:25 ` [Qemu-devel] [PATCH 04/10] snapshot: create bdrv_all_goto_snapshot helper Denis V. Lunev
` (8 subsequent siblings)
11 siblings, 0 replies; 20+ messages in thread
From: Denis V. Lunev @ 2015-11-10 14:25 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi, 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>
CC: Juan Quintela <quintela@redhat.com>
CC: Stefan Hajnoczi <stefanha@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 c212288..1157a6f 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1248,35 +1248,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;
@@ -1334,7 +1305,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;
}
@@ -1494,20 +1469,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] 20+ messages in thread
* [Qemu-devel] [PATCH 04/10] snapshot: create bdrv_all_goto_snapshot helper
2015-11-10 14:25 [Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes Denis V. Lunev
` (2 preceding siblings ...)
2015-11-10 14:25 ` [Qemu-devel] [PATCH 03/10] snapshot: create bdrv_all_delete_snapshot helper Denis V. Lunev
@ 2015-11-10 14:25 ` Denis V. Lunev
2015-11-12 8:38 ` Fam Zheng
2015-11-10 14:25 ` [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper Denis V. Lunev
` (7 subsequent siblings)
11 siblings, 1 reply; 20+ messages in thread
From: Denis V. Lunev @ 2015-11-10 14:25 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi, 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>
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 | 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 1157a6f..d18ff13 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1425,16 +1425,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] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 04/10] snapshot: create bdrv_all_goto_snapshot helper
2015-11-10 14:25 ` [Qemu-devel] [PATCH 04/10] snapshot: create bdrv_all_goto_snapshot helper Denis V. Lunev
@ 2015-11-12 8:38 ` Fam Zheng
2015-11-12 9:48 ` Denis V. Lunev
0 siblings, 1 reply; 20+ messages in thread
From: Fam Zheng @ 2015-11-12 8:38 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, Juan Quintela
On Tue, 11/10 17:25, Denis V. Lunev 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>
> 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 | 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 1157a6f..d18ff13 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1425,16 +1425,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;
Maybe more friendlily strerror(ret)?
> }
>
> /* restore the VM state */
> --
> 2.5.0
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 04/10] snapshot: create bdrv_all_goto_snapshot helper
2015-11-12 8:38 ` Fam Zheng
@ 2015-11-12 9:48 ` Denis V. Lunev
0 siblings, 0 replies; 20+ messages in thread
From: Denis V. Lunev @ 2015-11-12 9:48 UTC (permalink / raw)
To: Fam Zheng; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, Juan Quintela
On 11/12/2015 11:38 AM, Fam Zheng wrote:
> On Tue, 11/10 17:25, Denis V. Lunev 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>
>> 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 | 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 1157a6f..d18ff13 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -1425,16 +1425,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;
> Maybe more friendlily strerror(ret)?
>
>> }
>>
>> /* restore the VM state */
>> --
>> 2.5.0
>>
>>
we can. I think that this could be done for all such functions
as follow up patch. I'll do that separately and will send
with the next iteration if applicable.
Den
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper
2015-11-10 14:25 [Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes Denis V. Lunev
` (3 preceding siblings ...)
2015-11-10 14:25 ` [Qemu-devel] [PATCH 04/10] snapshot: create bdrv_all_goto_snapshot helper Denis V. Lunev
@ 2015-11-10 14:25 ` Denis V. Lunev
2015-11-16 9:31 ` Stefan Hajnoczi
2015-11-10 14:25 ` [Qemu-devel] [PATCH 06/10] migration: drop find_vmstate_bs check in hmp_delvm Denis V. Lunev
` (6 subsequent siblings)
11 siblings, 1 reply; 20+ messages in thread
From: Denis V. Lunev @ 2015-11-10 14:25 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi, Juan Quintela
to check that snapshot is available for all loaded block drivers. The
ability to switch to snapshot is verified separately using
bdrv_all_can_snapshot.
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 | 21 ++++++++++++++++++
include/block/snapshot.h | 2 ++
migration/savevm.c | 55 +++++++++++++-----------------------------------
3 files changed, 38 insertions(+), 40 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index 9f07a63..97dc315 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -423,3 +423,24 @@ 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, bool read_only,
+ 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 (read_only || (bdrv_is_inserted(bs) && !bdrv_is_read_only(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..0fae32b 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -85,5 +85,7 @@ 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, bool read_only,
+ BlockDriverState **first_bad_bs);
#endif
diff --git a/migration/savevm.c b/migration/savevm.c
index d18ff13..90aa565 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1383,6 +1383,18 @@ 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;
+ }
+ ret = bdrv_all_find_snapshot(name, false, &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) {
error_report("No block device supports snapshots");
@@ -1399,29 +1411,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_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;
- }
-
- 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();
@@ -1475,8 +1464,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;
@@ -1500,21 +1489,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, true, &bs1) == 0) {
available_snapshots[total] = i;
total++;
}
--
2.5.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper
2015-11-10 14:25 ` [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper Denis V. Lunev
@ 2015-11-16 9:31 ` Stefan Hajnoczi
2015-11-16 9:49 ` Denis V. Lunev
2015-11-16 10:28 ` Denis V. Lunev
0 siblings, 2 replies; 20+ messages in thread
From: Stefan Hajnoczi @ 2015-11-16 9:31 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, Juan Quintela
[-- Attachment #1: Type: text/plain, Size: 1706 bytes --]
On Tue, Nov 10, 2015 at 05:25:30PM +0300, Denis V. Lunev wrote:
> +int bdrv_all_find_snapshot(const char *name, bool read_only,
> + 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 (read_only || (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs))) {
> + err = bdrv_snapshot_find(bs, &sn, name);
> + }
> + aio_context_release(ctx);
> + }
> +
> + *first_bad_bs = bs;
> + return err;
> +}
It's difficult to see how bdrv_all_find_snapshot(read_only=true) is
equivalent to what you replaced below:
> @@ -1500,21 +1489,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, true, &bs1) == 0) {
The original loop skips drives that cannot snapshot and the vmstate
drive. The new code tries to find a snapshot all devices.
To me it seems the semantics are changed. Can you explain why this
change is safe?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper
2015-11-16 9:31 ` Stefan Hajnoczi
@ 2015-11-16 9:49 ` Denis V. Lunev
2015-11-16 10:28 ` Denis V. Lunev
1 sibling, 0 replies; 20+ messages in thread
From: Denis V. Lunev @ 2015-11-16 9:49 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, Juan Quintela
On 11/16/2015 12:31 PM, Stefan Hajnoczi wrote:
> On Tue, Nov 10, 2015 at 05:25:30PM +0300, Denis V. Lunev wrote:
>> +int bdrv_all_find_snapshot(const char *name, bool read_only,
>> + 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 (read_only || (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs))) {
>> + err = bdrv_snapshot_find(bs, &sn, name);
>> + }
>> + aio_context_release(ctx);
>> + }
>> +
>> + *first_bad_bs = bs;
>> + return err;
>> +}
> It's difficult to see how bdrv_all_find_snapshot(read_only=true) is
> equivalent to what you replaced below:
>
>> @@ -1500,21 +1489,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, true, &bs1) == 0) {
> The original loop skips drives that cannot snapshot and the vmstate
> drive. The new code tries to find a snapshot all devices.
>
> To me it seems the semantics are changed. Can you explain why this
> change is safe?
yep. you are once again right... OK.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper
2015-11-16 9:31 ` Stefan Hajnoczi
2015-11-16 9:49 ` Denis V. Lunev
@ 2015-11-16 10:28 ` Denis V. Lunev
1 sibling, 0 replies; 20+ messages in thread
From: Denis V. Lunev @ 2015-11-16 10:28 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, Juan Quintela
On 11/16/2015 12:31 PM, Stefan Hajnoczi wrote:
> On Tue, Nov 10, 2015 at 05:25:30PM +0300, Denis V. Lunev wrote:
>> +int bdrv_all_find_snapshot(const char *name, bool read_only,
>> + 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 (read_only || (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs))) {
>> + err = bdrv_snapshot_find(bs, &sn, name);
>> + }
>> + aio_context_release(ctx);
>> + }
>> +
>> + *first_bad_bs = bs;
>> + return err;
>> +}
> It's difficult to see how bdrv_all_find_snapshot(read_only=true) is
> equivalent to what you replaced below:
>
>> @@ -1500,21 +1489,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, true, &bs1) == 0) {
> The original loop skips drives that cannot snapshot and the vmstate
> drive. The new code tries to find a snapshot all devices.
>
> To me it seems the semantics are changed. Can you explain why this
> change is safe?
argh... This code is used for 'virsh qemu-monitor-command --hmp rhel7
info snapshots'
and NOT used for 'virsh snapshot-list rhel7' or used in a very different
way.
This is the root of the problem.
You are right, the command is broken completely. Some refactoring is
necessary
here.
Den
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 06/10] migration: drop find_vmstate_bs check in hmp_delvm
2015-11-10 14:25 [Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes Denis V. Lunev
` (4 preceding siblings ...)
2015-11-10 14:25 ` [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper Denis V. Lunev
@ 2015-11-10 14:25 ` Denis V. Lunev
2015-11-10 14:25 ` [Qemu-devel] [PATCH 07/10] snapshot: create bdrv_all_create_snapshot helper Denis V. Lunev
` (5 subsequent siblings)
11 siblings, 0 replies; 20+ messages in thread
From: Denis V. Lunev @ 2015-11-10 14:25 UTC (permalink / raw)
Cc: Denis V. Lunev, qemu-devel, Juan Quintela
There is no much sense to do the check and write warning.
Signed-off-by: Denis V. Lunev <den@openvz.org>
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 90aa565..4c652f3 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1448,11 +1448,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] 20+ messages in thread
* [Qemu-devel] [PATCH 07/10] snapshot: create bdrv_all_create_snapshot helper
2015-11-10 14:25 [Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes Denis V. Lunev
` (5 preceding siblings ...)
2015-11-10 14:25 ` [Qemu-devel] [PATCH 06/10] migration: drop find_vmstate_bs check in hmp_delvm Denis V. Lunev
@ 2015-11-10 14:25 ` Denis V. Lunev
2015-11-10 14:25 ` [Qemu-devel] [PATCH 08/10] migration: reorder processing in hmp_savevm Denis V. Lunev
` (4 subsequent siblings)
11 siblings, 0 replies; 20+ messages in thread
From: Denis V. Lunev @ 2015-11-10 14:25 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi, 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>
CC: Juan Quintela <quintela@redhat.com>
CC: Stefan Hajnoczi <stefanha@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 97dc315..6de53cb 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -444,3 +444,29 @@ int bdrv_all_find_snapshot(const char *name, bool read_only,
*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 0fae32b..5f43c0b 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -87,5 +87,9 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
int bdrv_all_find_snapshot(const char *name, bool read_only,
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 4c652f3..c2d677d 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1328,19 +1328,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] 20+ messages in thread
* [Qemu-devel] [PATCH 08/10] migration: reorder processing in hmp_savevm
2015-11-10 14:25 [Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes Denis V. Lunev
` (6 preceding siblings ...)
2015-11-10 14:25 ` [Qemu-devel] [PATCH 07/10] snapshot: create bdrv_all_create_snapshot helper Denis V. Lunev
@ 2015-11-10 14:25 ` Denis V. Lunev
2015-11-10 14:25 ` [Qemu-devel] [PATCH 09/10] migration: implement bdrv_all_find_vmstate_bs helper Denis V. Lunev
` (3 subsequent siblings)
11 siblings, 0 replies; 20+ messages in thread
From: Denis V. Lunev @ 2015-11-10 14:25 UTC (permalink / raw)
Cc: Denis V. Lunev, qemu-devel, 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>
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 c2d677d..f4da064 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1267,6 +1267,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");
@@ -1304,15 +1313,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] 20+ messages in thread
* [Qemu-devel] [PATCH 09/10] migration: implement bdrv_all_find_vmstate_bs helper
2015-11-10 14:25 [Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes Denis V. Lunev
` (7 preceding siblings ...)
2015-11-10 14:25 ` [Qemu-devel] [PATCH 08/10] migration: reorder processing in hmp_savevm Denis V. Lunev
@ 2015-11-10 14:25 ` Denis V. Lunev
2015-11-10 14:25 ` [Qemu-devel] [PATCH 10/10] migration: normalize locking in migration/savevm.c Denis V. Lunev
` (2 subsequent siblings)
11 siblings, 0 replies; 20+ messages in thread
From: Denis V. Lunev @ 2015-11-10 14:25 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi, Juan Quintela
The patch also ensures proper locking for the operation.
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 | 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 6de53cb..addb7c9 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -470,3 +470,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 5f43c0b..a42a694 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -92,4 +92,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 f4da064..20c95b2 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1237,17 +1237,6 @@ out:
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;
@@ -1276,8 +1265,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;
}
@@ -1386,7 +1375,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;
@@ -1455,7 +1444,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] 20+ messages in thread
* [Qemu-devel] [PATCH 10/10] migration: normalize locking in migration/savevm.c
2015-11-10 14:25 [Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes Denis V. Lunev
` (8 preceding siblings ...)
2015-11-10 14:25 ` [Qemu-devel] [PATCH 09/10] migration: implement bdrv_all_find_vmstate_bs helper Denis V. Lunev
@ 2015-11-10 14:25 ` Denis V. Lunev
2015-11-12 9:21 ` [Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes Fam Zheng
2015-11-16 9:32 ` Stefan Hajnoczi
11 siblings, 0 replies; 20+ messages in thread
From: Denis V. Lunev @ 2015-11-10 14:25 UTC (permalink / raw)
Cc: Kevin Wolf, Denis V. Lunev, qemu-devel, Stefan Hajnoczi, 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>
CC: Juan Quintela <quintela@redhat.com>
CC: Stefan Hajnoczi <stefanha@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 20c95b2..01110a4 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1249,6 +1249,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 "
@@ -1270,6 +1271,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();
@@ -1280,6 +1282,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 */
@@ -1324,6 +1328,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
}
the_end:
+ aio_context_release(aio_context);
if (saved_vm_running) {
vm_start();
}
@@ -1362,6 +1367,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.",
@@ -1380,9 +1386,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) {
@@ -1410,9 +1419,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);
@@ -1443,14 +1455,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] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes
2015-11-10 14:25 [Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes Denis V. Lunev
` (9 preceding siblings ...)
2015-11-10 14:25 ` [Qemu-devel] [PATCH 10/10] migration: normalize locking in migration/savevm.c Denis V. Lunev
@ 2015-11-12 9:21 ` Fam Zheng
2015-11-16 9:32 ` Stefan Hajnoczi
11 siblings, 0 replies; 20+ messages in thread
From: Fam Zheng @ 2015-11-12 9:21 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, Juan Quintela
On Tue, 11/10 17:25, Denis V. Lunev wrote:
> 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(-)
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes
2015-11-10 14:25 [Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes Denis V. Lunev
` (10 preceding siblings ...)
2015-11-12 9:21 ` [Qemu-devel] [PATCH for 2.5 v7 0/10] dataplane snapshot fixes Fam Zheng
@ 2015-11-16 9:32 ` Stefan Hajnoczi
11 siblings, 0 replies; 20+ messages in thread
From: Stefan Hajnoczi @ 2015-11-16 9:32 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, Juan Quintela
[-- Attachment #1: Type: text/plain, Size: 2617 bytes --]
On Tue, Nov 10, 2015 at 05:25:25PM +0300, Denis V. Lunev 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.
>
> Anyway, I think that the construction like
> assert(aio_context_is_locked(aio_context));
> should be widely used to ensure proper locking.
>
> 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(-)
All patches except Patch 6:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread