From b4b076daf324894dd288cbdb67ff1e3c7434df7b Mon Sep 17 00:00:00 2001 From: Juan Quintela Date: Mon, 23 Jan 2017 22:32:06 +0100 Subject: [PATCH 1/6] migration: create Migration Incoming State at init time Signed-off-by: Juan Quintela Reviewed-by: Dr. David Alan Gilbert Message-Id: <1485207141-1941-3-git-send-email-quintela@redhat.com> Signed-off-by: Juan Quintela --- include/migration/migration.h | 1 - migration/migration.c | 38 ++++++++++++++++------------------- migration/savevm.c | 4 ++-- 3 files changed, 19 insertions(+), 24 deletions(-) diff --git a/include/migration/migration.h b/include/migration/migration.h index af9135f0a7..7528cc2fbc 100644 --- a/include/migration/migration.h +++ b/include/migration/migration.h @@ -122,7 +122,6 @@ struct MigrationIncomingState { }; MigrationIncomingState *migration_incoming_get_current(void); -MigrationIncomingState *migration_incoming_state_new(QEMUFile *f); void migration_incoming_state_destroy(void); /* diff --git a/migration/migration.c b/migration/migration.c index 2766d2f586..619ccc4590 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -111,32 +111,28 @@ MigrationState *migrate_get_current(void) return ¤t_migration; } -/* For incoming */ -static MigrationIncomingState *mis_current; - MigrationIncomingState *migration_incoming_get_current(void) { - return mis_current; -} + static bool once; + static MigrationIncomingState mis_current; -MigrationIncomingState *migration_incoming_state_new(QEMUFile* f) -{ - mis_current = g_new0(MigrationIncomingState, 1); - mis_current->from_src_file = f; - mis_current->state = MIGRATION_STATUS_NONE; - QLIST_INIT(&mis_current->loadvm_handlers); - qemu_mutex_init(&mis_current->rp_mutex); - qemu_event_init(&mis_current->main_thread_load_event, false); - - return mis_current; + if (!once) { + mis_current.state = MIGRATION_STATUS_NONE; + memset(&mis_current, 0, sizeof(MigrationIncomingState)); + QLIST_INIT(&mis_current.loadvm_handlers); + qemu_mutex_init(&mis_current.rp_mutex); + qemu_event_init(&mis_current.main_thread_load_event, false); + once = true; + } + return &mis_current; } void migration_incoming_state_destroy(void) { - qemu_event_destroy(&mis_current->main_thread_load_event); - loadvm_free_handlers(mis_current); - g_free(mis_current); - mis_current = NULL; + struct MigrationIncomingState *mis = migration_incoming_get_current(); + + qemu_event_destroy(&mis->main_thread_load_event); + loadvm_free_handlers(mis); } @@ -382,11 +378,11 @@ static void process_incoming_migration_bh(void *opaque) static void process_incoming_migration_co(void *opaque) { QEMUFile *f = opaque; - MigrationIncomingState *mis; + MigrationIncomingState *mis = migration_incoming_get_current(); PostcopyState ps; int ret; - mis = migration_incoming_state_new(f); + mis->from_src_file = f; postcopy_state_set(POSTCOPY_INCOMING_NONE); migrate_set_state(&mis->state, MIGRATION_STATUS_NONE, MIGRATION_STATUS_ACTIVE); diff --git a/migration/savevm.c b/migration/savevm.c index 204012ecef..f3644ca1d7 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2199,7 +2199,6 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp) qio_channel_set_name(QIO_CHANNEL(ioc), "migration-xen-load-state"); f = qemu_fopen_channel_input(QIO_CHANNEL(ioc)); - migration_incoming_state_new(f); ret = qemu_loadvm_state(f); qemu_fclose(f); if (ret < 0) { @@ -2215,6 +2214,7 @@ int load_vmstate(const char *name) QEMUFile *f; int ret; AioContext *aio_context; + MigrationIncomingState *mis = migration_incoming_get_current(); if (!bdrv_all_can_snapshot(&bs)) { error_report("Device '%s' is writable but does not support snapshots.", @@ -2265,7 +2265,7 @@ int load_vmstate(const char *name) } qemu_system_reset(VMRESET_SILENT); - migration_incoming_state_new(f); + mis->from_src_file = f; aio_context_acquire(aio_context); ret = qemu_loadvm_state(f); From bc5c4f21966977c4ff00ae77979b95a2b39142a3 Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Thu, 2 Feb 2017 12:59:54 +0000 Subject: [PATCH 2/6] vmstate_register_with_alias_id: Take an Error ** I'll be adding an error to it in a subsequent patch. Signed-off-by: Dr. David Alan Gilbert Reviewed-by: Juan Quintela Message-Id: <20170202125956.21942-2-dgilbert@redhat.com> Signed-off-by: Juan Quintela --- hw/core/qdev.c | 3 ++- hw/intc/apic_common.c | 2 +- include/migration/vmstate.h | 5 +++-- migration/savevm.c | 3 ++- stubs/vmstate.c | 3 ++- 5 files changed, 10 insertions(+), 6 deletions(-) diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 57834423b9..ea97b15f45 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -935,7 +935,8 @@ static void device_set_realized(Object *obj, bool value, Error **errp) if (qdev_get_vmsd(dev)) { vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev, dev->instance_id_alias, - dev->alias_required_for_version); + dev->alias_required_for_version, + NULL); } QLIST_FOREACH(bus, &dev->child_bus, sibling) { diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c index 17df24c9d0..6ce8ef744a 100644 --- a/hw/intc/apic_common.c +++ b/hw/intc/apic_common.c @@ -329,7 +329,7 @@ static void apic_common_realize(DeviceState *dev, Error **errp) instance_id = -1; } vmstate_register_with_alias_id(NULL, instance_id, &vmstate_apic_common, - s, -1, 0); + s, -1, 0, NULL); } static void apic_common_unrealize(DeviceState *dev, Error **errp) diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index 3bbe3ed984..c38b00a6f1 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -988,14 +988,15 @@ bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque); int vmstate_register_with_alias_id(DeviceState *dev, int instance_id, const VMStateDescription *vmsd, void *base, int alias_id, - int required_for_version); + int required_for_version, + Error **errp); static inline int vmstate_register(DeviceState *dev, int instance_id, const VMStateDescription *vmsd, void *opaque) { return vmstate_register_with_alias_id(dev, instance_id, vmsd, - opaque, -1, 0); + opaque, -1, 0, NULL); } void vmstate_unregister(DeviceState *dev, const VMStateDescription *vmsd, diff --git a/migration/savevm.c b/migration/savevm.c index f3644ca1d7..b11b4a22bf 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -656,7 +656,8 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque) int vmstate_register_with_alias_id(DeviceState *dev, int instance_id, const VMStateDescription *vmsd, void *opaque, int alias_id, - int required_for_version) + int required_for_version, + Error **errp) { SaveStateEntry *se; diff --git a/stubs/vmstate.c b/stubs/vmstate.c index 65906271d2..bbe158fe3b 100644 --- a/stubs/vmstate.c +++ b/stubs/vmstate.c @@ -8,7 +8,8 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id, const VMStateDescription *vmsd, void *base, int alias_id, - int required_for_version) + int required_for_version, + Error **errp) { return 0; } From 581f08bac22bdd5e081ae07f68071a0fc3c5c2c7 Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Thu, 2 Feb 2017 12:59:55 +0000 Subject: [PATCH 3/6] migration: Check for ID length The qdev id of a device can be huge if it's on the end of a chain of bridges; in reality such chains shouldn't occur but they can be made to by chaining PCIe bridges together. The migration format has a number of 256 character long format limits; check we don't hit them (we already use pstrcat/cpy but that just protects us from buffer overruns, we fairly quickly hit an assert). Signed-off-by: Dr. David Alan Gilbert Reviewed-by: Juan Quintela Message-Id: <20170202125956.21942-3-dgilbert@redhat.com> Signed-off-by: Juan Quintela --- include/migration/vmstate.h | 2 ++ migration/savevm.c | 21 ++++++++++++++++----- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index c38b00a6f1..6233fe2e5b 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -985,12 +985,14 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd, bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque); +/* Returns: 0 on success, -1 on failure */ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id, const VMStateDescription *vmsd, void *base, int alias_id, int required_for_version, Error **errp); +/* Returns: 0 on success, -1 on failure */ static inline int vmstate_register(DeviceState *dev, int instance_id, const VMStateDescription *vmsd, void *opaque) diff --git a/migration/savevm.c b/migration/savevm.c index b11b4a22bf..8b8c74dc86 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -590,8 +590,14 @@ int register_savevm_live(DeviceState *dev, if (dev) { char *id = qdev_get_dev_path(dev); if (id) { - pstrcpy(se->idstr, sizeof(se->idstr), id); - pstrcat(se->idstr, sizeof(se->idstr), "/"); + if (snprintf(se->idstr, sizeof(se->idstr), "%s/", id) >= + sizeof(se->idstr)) { + error_report("Path too long for VMState (%s)", id); + g_free(id); + g_free(se); + + return -1; + } g_free(id); se->compat = g_new0(CompatEntry, 1); @@ -674,9 +680,14 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id, if (dev) { char *id = qdev_get_dev_path(dev); if (id) { - pstrcpy(se->idstr, sizeof(se->idstr), id); - pstrcat(se->idstr, sizeof(se->idstr), "/"); - g_free(id); + if (snprintf(se->idstr, sizeof(se->idstr), "%s/", id) >= + sizeof(se->idstr)) { + error_setg(errp, "Path too long for VMState (%s)", id); + g_free(id); + g_free(se); + + return -1; + } se->compat = g_new0(CompatEntry, 1); pstrcpy(se->compat->idstr, sizeof(se->compat->idstr), vmsd->name); From 67980031d234aa90524b83bb80bb5d1601d29076 Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Thu, 2 Feb 2017 12:59:56 +0000 Subject: [PATCH 4/6] vmstate registration: check return values Check qdev's call to vmstate_register_with_alias_id; that gets most of the common uses; there's hundreds of calls via vmstate_register which could get fixed over time. Signed-off-by: Dr. David Alan Gilbert Reviewed-by: Juan Quintela Message-Id: <20170202125956.21942-4-dgilbert@redhat.com> Signed-off-by: Juan Quintela --- hw/core/qdev.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/hw/core/qdev.c b/hw/core/qdev.c index ea97b15f45..06ba02e2a3 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -933,10 +933,12 @@ static void device_set_realized(Object *obj, bool value, Error **errp) } if (qdev_get_vmsd(dev)) { - vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev, - dev->instance_id_alias, - dev->alias_required_for_version, - NULL); + if (vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev, + dev->instance_id_alias, + dev->alias_required_for_version, + &local_err) < 0) { + goto post_realize_fail; + } } QLIST_FOREACH(bus, &dev->child_bus, sibling) { From 328d4d85282e7d62f89f5b0547a493d9cd07cea0 Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Thu, 2 Feb 2017 15:59:08 +0000 Subject: [PATCH 5/6] Postcopy: Reset state to avoid cleanup assert On a destination host with no userfault support an incoming postcopy would cause the state to enter ADVISE before it realised there was no support, and because it was in ADVISE state it would perform a cleanup at the end. Since there was no support the cleanup function should be unreachable, but ends up being called and asserting. Reset the state when we realise we have no support, thus the cleanup doesn't happen. Signed-off-by: Dr. David Alan Gilbert Message-Id: <20170202155909.31784-2-dgilbert@redhat.com> Signed-off-by: Juan Quintela --- migration/savevm.c | 1 + 1 file changed, 1 insertion(+) diff --git a/migration/savevm.c b/migration/savevm.c index 8b8c74dc86..01997687c4 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1355,6 +1355,7 @@ static int loadvm_postcopy_handle_advise(MigrationIncomingState *mis) } if (!postcopy_ram_supported_by_host()) { + postcopy_state_set(POSTCOPY_INCOMING_NONE); return -1; } From ef8d6488d2767fe81bb4bb9bcdc52af5ff718b56 Mon Sep 17 00:00:00 2001 From: "Dr. David Alan Gilbert" Date: Thu, 2 Feb 2017 15:59:09 +0000 Subject: [PATCH 6/6] postcopy: Recover block devices on early failure An early postcopy failure can be recovered from as long as we know we haven't sent the command to run the destination. We have to undo the bdrv_inactivate_all by calling bdrv_invalidate_cache_all Note that I'm not using ms->block_inactive because once we've sent the postcopy package we dont want anything else to try and recover the block storage on the source; the destination might have started writing to it. Signed-off-by: Dr. David Alan Gilbert Message-Id: <20170202155909.31784-3-dgilbert@redhat.com> Signed-off-by: Juan Quintela --- migration/migration.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/migration/migration.c b/migration/migration.c index 619ccc4590..2b179c69fa 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1601,6 +1601,7 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running) QIOChannelBuffer *bioc; QEMUFile *fb; int64_t time_at_stop = qemu_clock_get_ms(QEMU_CLOCK_REALTIME); + bool restart_block = false; migrate_set_state(&ms->state, MIGRATION_STATUS_ACTIVE, MIGRATION_STATUS_POSTCOPY_ACTIVE); @@ -1620,6 +1621,7 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running) if (ret < 0) { goto fail; } + restart_block = true; /* * Cause any non-postcopiable, but iterative devices to @@ -1676,6 +1678,18 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running) /* <><> end of stuff going into the package */ + /* Last point of recovery; as soon as we send the package the destination + * can open devices and potentially start running. + * Lets just check again we've not got any errors. + */ + ret = qemu_file_get_error(ms->to_dst_file); + if (ret) { + error_report("postcopy_start: Migration stream errored (pre package)"); + goto fail_closefb; + } + + restart_block = false; + /* Now send that blob */ if (qemu_savevm_send_packaged(ms->to_dst_file, bioc->data, bioc->usage)) { goto fail_closefb; @@ -1713,6 +1727,17 @@ fail_closefb: fail: migrate_set_state(&ms->state, MIGRATION_STATUS_POSTCOPY_ACTIVE, MIGRATION_STATUS_FAILED); + if (restart_block) { + /* A failure happened early enough that we know the destination hasn't + * accessed block devices, so we're safe to recover. + */ + Error *local_err = NULL; + + bdrv_invalidate_cache_all(&local_err); + if (local_err) { + error_report_err(local_err); + } + } qemu_mutex_unlock_iothread(); return -1; }