From 84e7b80a05c0c44b90533c6cd2f1db5c932ccf77 Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Wed, 18 Nov 2015 11:48:39 +0000 Subject: [PATCH 01/14] Set last_sent_block In a82d593b61054b3dea43 I accidentally removed the setting of last_sent_block, put it back. Symptoms: Multithreaded compression only uses one thread. Migration is a bit less efficient since it won't use 'cont' flags. Signed-off-by: Dr. David Alan Gilbert Fixes: a82d593b61054b3dea43 Signed-off-by: Juan Quintela --- migration/ram.c | 1 + 1 file changed, 1 insertion(+) diff --git a/migration/ram.c b/migration/ram.c index 7f32696d79..1eb155aedd 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -1249,6 +1249,7 @@ static int ram_save_target_page(MigrationState *ms, QEMUFile *f, if (unsentmap) { clear_bit(dirty_ram_abs >> TARGET_PAGE_BITS, unsentmap); } + last_sent_block = block; } return res; From 95a7788b2faa81ff95675f1e46a3272a612b35de Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Wed, 18 Nov 2015 11:48:40 +0000 Subject: [PATCH 02/14] migration: Dead assignment of current_time I set current_time before the postcopy test but never use it; (I think this was from the original version where it was time based). Spotted by coverity, CID 1339208 Signed-off-by: Dr. David Alan Gilbert Signed-off-by: Juan Quintela --- migration/migration.c | 1 - 1 file changed, 1 deletion(-) diff --git a/migration/migration.c b/migration/migration.c index 7e4e27b57d..265d13a74f 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1643,7 +1643,6 @@ static void *migration_thread(void *opaque) if (pending_size && pending_size >= max_size) { /* Still a significant amount to transfer */ - current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); if (migrate_postcopy_ram() && s->state != MIGRATION_STATUS_POSTCOPY_ACTIVE && pend_nonpost <= max_size && From 5df5416e639cd75bd85d243af41387c2418fa580 Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Wed, 18 Nov 2015 11:48:41 +0000 Subject: [PATCH 03/14] Unneeded NULL check The check is unneccesary, we read the value at the start of the thread, use it, and never change it. The value is checked to be non-NULL before thread creation. Spotted by coverity, CID 1339211 Signed-off-by: Dr. David Alan Gilbert Signed-off-by: Juan Quintela --- migration/migration.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/migration/migration.c b/migration/migration.c index 265d13a74f..1a42aee412 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1345,7 +1345,7 @@ static void *source_return_path_thread(void *opaque) break; } } - if (rp && qemu_file_get_error(rp)) { + if (qemu_file_get_error(rp)) { trace_source_return_path_thread_bad_end(); mark_source_rp_bad(ms); } From e9ff957ac26e0e11869a3568cfa7423ae33c51e7 Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Thu, 19 Nov 2015 09:42:01 +0300 Subject: [PATCH 04/14] snapshot: create helper to test that block drivers supports snapshots The patch enforces proper locking for this operation. Signed-off-by: Denis V. Lunev Reviewed-by: Greg Kurz Reviewed-by: Stefan Hajnoczi Reviewed-by: Fam Zheng Reviewed-by: Juan Quintela CC: Kevin Wolf Tested-by: Greg Kurz Signed-off-by: Juan Quintela --- 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 89500f2f18..d929d08913 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 770d9bbc8c..6195c9c8be 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 d90e228568..4646aa1178 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(); From 25af925fffc29f2e4c05aee10c61c823c4cdf398 Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Thu, 19 Nov 2015 09:42:02 +0300 Subject: [PATCH 05/14] snapshot: return error code from bdrv_snapshot_delete_by_id_or_name this will make code better in the next patch Signed-off-by: Denis V. Lunev Reviewed-by: Stefan Hajnoczi Reviewed-by: Fam Zheng Reviewed-by: Juan Quintela CC: Kevin Wolf Tested-by: Greg Kurz Signed-off-by: Juan Quintela --- 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 d929d08913..ed0422df9f 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 6195c9c8be..9ddfd42d89 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, From 9b00ea376d42e543feb12d7ce5435366d01aab1b Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Thu, 19 Nov 2015 09:42:03 +0300 Subject: [PATCH 06/14] snapshot: create bdrv_all_delete_snapshot helper to delete snapshots from all loaded block drivers. The patch also ensures proper locking. Signed-off-by: Denis V. Lunev Reviewed-by: Stefan Hajnoczi Reviewed-by: Fam Zheng Reviewed-by: Juan Quintela CC: Kevin Wolf Tested-by: Greg Kurz Signed-off-by: Juan Quintela --- 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 ed0422df9f..61a6ad12dc 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 9ddfd42d89..d02d2b1c09 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 4646aa1178..c52cbbeb3d 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); } } From 4c1cdbaad07d067f3d156687d79014ab44387e2c Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Thu, 19 Nov 2015 09:42:04 +0300 Subject: [PATCH 07/14] snapshot: create bdrv_all_goto_snapshot helper to switch to snapshot on all loaded block drivers. The patch also ensures proper locking. Signed-off-by: Denis V. Lunev Reviewed-by: Greg Kurz Reviewed-by: Stefan Hajnoczi Reviewed-by: Fam Zheng Reviewed-by: Juan Quintela CC: Kevin Wolf Tested-by: Greg Kurz Signed-off-by: Juan Quintela --- 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 61a6ad12dc..9f07a63e6d 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 d02d2b1c09..0a176c7e50 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 c52cbbeb3d..254e51de34 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 */ From 849f96e2f71b52444516a0880fd9d12691b63d20 Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Thu, 19 Nov 2015 09:42:05 +0300 Subject: [PATCH 08/14] migration: factor our snapshottability check in load_vmstate 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 Reviewed-by: Juan Quintela Reviewed-by: Fam Zheng CC: Stefan Hajnoczi CC: Kevin Wolf Tested-by: Greg Kurz Signed-off-by: Juan Quintela --- migration/savevm.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/migration/savevm.c b/migration/savevm.c index 254e51de34..2ecc1b3e09 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); From 723ccda1a0eecece8e70dbcdd35a603f6c41a475 Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Thu, 19 Nov 2015 09:42:06 +0300 Subject: [PATCH 09/14] snapshot: create bdrv_all_find_snapshot helper 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 Reviewed-by: Fam Zheng Reviewed-by: Juan Quintela CC: Stefan Hajnoczi CC: Kevin Wolf Tested-by: Greg Kurz Signed-off-by: Juan Quintela --- 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 9f07a63e6d..eae4730fc6 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 0a176c7e50..10ee582f2c 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 2ecc1b3e09..4e6d578841 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++; } From c6258b04f19bc690b576b089f621cb5333c533d7 Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Thu, 19 Nov 2015 09:42:07 +0300 Subject: [PATCH 10/14] migration: drop find_vmstate_bs check in hmp_delvm There is no much sense to do the check and write warning. Signed-off-by: Denis V. Lunev Reviewed-by: Stefan Hajnoczi Reviewed-by: Fam Zheng Reviewed-by: Juan Quintela Tested-by: Greg Kurz Signed-off-by: Juan Quintela --- migration/savevm.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/migration/savevm.c b/migration/savevm.c index 4e6d578841..63c07e1b9a 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", From a9085f9b5583ba7a02b412ba08f929555112c244 Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Thu, 19 Nov 2015 09:42:08 +0300 Subject: [PATCH 11/14] snapshot: create bdrv_all_create_snapshot helper to create snapshot for all loaded block drivers. The patch also ensures proper locking. Signed-off-by: Denis V. Lunev Reviewed-by: Stefan Hajnoczi Reviewed-by: Fam Zheng Reviewed-by: Juan Quintela CC: Kevin Wolf Tested-by: Greg Kurz Signed-off-by: Juan Quintela --- 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 eae4730fc6..75d0b968f6 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 10ee582f2c..55e995a3fe 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 63c07e1b9a..80107e32b1 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: From 0b46160521ab72744da94988583a45d4d45e2986 Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Thu, 19 Nov 2015 09:42:09 +0300 Subject: [PATCH 12/14] migration: reorder processing in hmp_savevm 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 Reviewed-by: Stefan Hajnoczi Reviewed-by: Fam Zheng Reviewed-by: Juan Quintela Tested-by: Greg Kurz Signed-off-by: Juan Quintela --- migration/savevm.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/migration/savevm.c b/migration/savevm.c index 80107e32b1..98f9a8c694 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) { From 7cb14481498e7acd969a76b53be0535cd90f7d53 Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Thu, 19 Nov 2015 09:42:10 +0300 Subject: [PATCH 13/14] migration: implement bdrv_all_find_vmstate_bs helper The patch also ensures proper locking for the operation. Signed-off-by: Denis V. Lunev Reviewed-by: Stefan Hajnoczi Reviewed-by: Fam Zheng Reviewed-by: Juan Quintela CC: Kevin Wolf Tested-by: Greg Kurz Signed-off-by: Juan Quintela --- 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 75d0b968f6..6e9fa8da98 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 55e995a3fe..c6910da63a 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 98f9a8c694..a845e69db1 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; From 79b3c12ac5714e036a16d1a163a3517d74504f87 Mon Sep 17 00:00:00 2001 From: "Denis V. Lunev" Date: Thu, 19 Nov 2015 09:42:11 +0300 Subject: [PATCH 14/14] migration: normalize locking in migration/savevm.c basically all bdrv_* operations must be called under aio_context_acquire except ones with bdrv_all prefix. Signed-off-by: Denis V. Lunev Reviewed-by: Stefan Hajnoczi Reviewed-by: Fam Zheng CC: Juan Quintela CC: Kevin Wolf Tested-by: Greg Kurz Signed-off-by: Juan Quintela --- migration/savevm.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/migration/savevm.c b/migration/savevm.c index a845e69db1..0ad1b93a8b 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_vm_state); /* 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;