On 11/20/2023 1:13 PM, Fabiano Rosas wrote:
> Steve Sistare <steven.sist...@oracle.com> writes:
>
>> Restoring a snapshot can break a suspended guest. Snapshots suffer from
>> the same suspended-state issues that affect live migration, plus they must
>> handle an additional problematic scenario, which is that a running vm must
>> remain running if it loads a suspended snapshot.
>>
>> To save, call vm_stop_force_state to completely stop a vm in the suspended
>> state, and restore the suspended state using runstate_restore. This
>> produces a correct vmstate file and leaves the vm in the state it had prior
>> to the save.
>>
>> To load, if the snapshot is not suspended, then vm_stop_force_state +
>> runstate_restore correctly handles all states, and leaves the vm in the
>> state it had prior to the load. However, if the snapshot is suspended,
>> restoration is trickier. First restore the state to suspended so the
>> current state matches the saved state. Then, if the pre-load state is
>> running, wakeup to resume running.
>>
>> Prior to these changes, the vm_stop to RUN_STATE_SAVE_VM and
>> RUN_STATE_RESTORE_VM did not change runstate if the current state was
>> paused, suspended, or prelaunch, but now vm_stop_force_state forces these
>> transitions, so allow them.
>>
>> Signed-off-by: Steve Sistare <steven.sist...@oracle.com>
>> ---
>> include/migration/snapshot.h | 7 +++++++
>> migration/migration-hmp-cmds.c | 12 ++++++++----
>> migration/savevm.c | 33 +++++++++++++++++++++------------
>> system/runstate.c | 10 ++++++++++
>> system/vl.c | 2 ++
>> 5 files changed, 48 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/migration/snapshot.h b/include/migration/snapshot.h
>> index e72083b..9e4dcaa 100644
>> --- a/include/migration/snapshot.h
>> +++ b/include/migration/snapshot.h
>> @@ -16,6 +16,7 @@
>> #define QEMU_MIGRATION_SNAPSHOT_H
>>
>> #include "qapi/qapi-builtin-types.h"
>> +#include "qapi/qapi-types-run-state.h"
>>
>> /**
>> * save_snapshot: Save an internal snapshot.
>> @@ -61,4 +62,10 @@ bool delete_snapshot(const char *name,
>> bool has_devices, strList *devices,
>> Error **errp);
>>
>> +/**
>> + * load_snapshot_resume: Restore runstate after loading snapshot.
>> + * @state: state to restore
>> + */
>> +void load_snapshot_resume(RunState state);
>> +
>> #endif
>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>> index 86ae832..c31cdc7 100644
>> --- a/migration/migration-hmp-cmds.c
>> +++ b/migration/migration-hmp-cmds.c
>> @@ -399,15 +399,19 @@ void hmp_info_migrate_parameters(Monitor *mon, const
>> QDict *qdict)
>>
>> void hmp_loadvm(Monitor *mon, const QDict *qdict)
>> {
>> - int saved_vm_running = runstate_is_running();
>> + RunState saved_state = runstate_get();
>> +
>> const char *name = qdict_get_str(qdict, "name");
>> Error *err = NULL;
>>
>> - vm_stop(RUN_STATE_RESTORE_VM);
>> + vm_stop_force_state(RUN_STATE_RESTORE_VM);
>>
>> - if (load_snapshot(name, NULL, false, NULL, &err) && saved_vm_running) {
>> - vm_start();
>> + if (load_snapshot(name, NULL, false, NULL, &err)) {
>> + load_snapshot_resume(saved_state);
>> + } else {
>> + vm_resume(saved_state);
>
> Here we're starting the VM if load_snapshot() fails. Is that
> intentional?
My bad, good catch, I will delete the else clause.
>> }
>> +
>> hmp_handle_error(mon, err);
>> }
>>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 78ac2bd..b4b49bb 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -3040,7 +3040,7 @@ bool save_snapshot(const char *name, bool overwrite,
>> const char *vmstate,
>> QEMUSnapshotInfo sn1, *sn = &sn1;
>> int ret = -1, ret2;
>> QEMUFile *f;
>> - int saved_vm_running;
>> + RunState saved_state = runstate_get();
>> uint64_t vm_state_size;
>> g_autoptr(GDateTime) now = g_date_time_new_now_local();
>> AioContext *aio_context;
>> @@ -3088,10 +3088,8 @@ bool save_snapshot(const char *name, bool overwrite,
>> const char *vmstate,
>> }
>> aio_context = bdrv_get_aio_context(bs);
>>
>> - saved_vm_running = runstate_is_running();
>> -
>> global_state_store();
>> - vm_stop(RUN_STATE_SAVE_VM);
>> + vm_stop_force_state(RUN_STATE_SAVE_VM);
>>
>> bdrv_drain_all_begin();
>>
>> @@ -3157,9 +3155,7 @@ bool save_snapshot(const char *name, bool overwrite,
>> const char *vmstate,
>>
>> bdrv_drain_all_end();
>>
>> - if (saved_vm_running) {
>> - vm_start();
>> - }
>> + vm_resume(saved_state);
>> return ret == 0;
>> }
>>
>> @@ -3333,6 +3329,20 @@ err_drain:
>> return false;
>> }
>>
>> +void load_snapshot_resume(RunState state)
>> +{
>> + if (global_state_received() &&
>> + global_state_get_runstate() == RUN_STATE_SUSPENDED) {
>> +
>> + vm_resume(RUN_STATE_SUSPENDED);
>> + if (state == RUN_STATE_RUNNING) {
>> + qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, NULL);
>> + }
>> + } else {
>> + vm_resume(state);
>> + }
>> +}
>> +
>> bool delete_snapshot(const char *name, bool has_devices,
>> strList *devices, Error **errp)
>> {
>> @@ -3397,16 +3407,15 @@ static void snapshot_load_job_bh(void *opaque)
>> {
>> Job *job = opaque;
>> SnapshotJob *s = container_of(job, SnapshotJob, common);
>> - int orig_vm_running;
>> + RunState orig_state = runstate_get();
>>
>> job_progress_set_remaining(&s->common, 1);
>>
>> - orig_vm_running = runstate_is_running();
>> - vm_stop(RUN_STATE_RESTORE_VM);
>> + vm_stop_force_state(RUN_STATE_RESTORE_VM);
>>
>> s->ret = load_snapshot(s->tag, s->vmstate, true, s->devices, s->errp);
>> - if (s->ret && orig_vm_running) {
>> - vm_start();
>> + if (s->ret) {
>> + load_snapshot_resume(orig_state);
>
> Same here, we used to not start the VM if load_snapshot() failed.
Here the behavior is the same as the old code. ret=1 means success.
That inverted return code has misled us both :)
>> }
>>
>> job_progress_update(&s->common, 1);
>> diff --git a/system/runstate.c b/system/runstate.c
>> index ea9d6c2..f1d4bc7 100644
>> --- a/system/runstate.c
>> +++ b/system/runstate.c
>> @@ -77,6 +77,8 @@ typedef struct {
>>
>> static const RunStateTransition runstate_transitions_def[] = {
>> { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
>> + { RUN_STATE_PRELAUNCH, RUN_STATE_PAUSED },
>> + { RUN_STATE_PRELAUNCH, RUN_STATE_SUSPENDED },
>>
>> { RUN_STATE_DEBUG, RUN_STATE_RUNNING },
>> { RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
>> @@ -108,6 +110,8 @@ static const RunStateTransition
>> runstate_transitions_def[] = {
>> { RUN_STATE_PAUSED, RUN_STATE_POSTMIGRATE },
>> { RUN_STATE_PAUSED, RUN_STATE_PRELAUNCH },
>> { RUN_STATE_PAUSED, RUN_STATE_COLO},
>> + { RUN_STATE_PAUSED, RUN_STATE_SAVE_VM},
>> + { RUN_STATE_PAUSED, RUN_STATE_RESTORE_VM},
>>
>> { RUN_STATE_POSTMIGRATE, RUN_STATE_RUNNING },
>> { RUN_STATE_POSTMIGRATE, RUN_STATE_FINISH_MIGRATE },
>> @@ -131,6 +135,8 @@ static const RunStateTransition
>> runstate_transitions_def[] = {
>>
>> { RUN_STATE_RESTORE_VM, RUN_STATE_RUNNING },
>> { RUN_STATE_RESTORE_VM, RUN_STATE_PRELAUNCH },
>> + { RUN_STATE_RESTORE_VM, RUN_STATE_PAUSED },
>> + { RUN_STATE_RESTORE_VM, RUN_STATE_SUSPENDED },
>>
>> { RUN_STATE_COLO, RUN_STATE_RUNNING },
>> { RUN_STATE_COLO, RUN_STATE_PRELAUNCH },
>> @@ -149,6 +155,8 @@ static const RunStateTransition
>> runstate_transitions_def[] = {
>> { RUN_STATE_RUNNING, RUN_STATE_COLO},
>>
>> { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
>> + { RUN_STATE_SAVE_VM, RUN_STATE_PAUSED },
>> + { RUN_STATE_SAVE_VM, RUN_STATE_SUSPENDED },
>>
>> { RUN_STATE_SHUTDOWN, RUN_STATE_PAUSED },
>> { RUN_STATE_SHUTDOWN, RUN_STATE_FINISH_MIGRATE },
>> @@ -161,6 +169,8 @@ static const RunStateTransition
>> runstate_transitions_def[] = {
>> { RUN_STATE_SUSPENDED, RUN_STATE_FINISH_MIGRATE },
>> { RUN_STATE_SUSPENDED, RUN_STATE_PRELAUNCH },
>> { RUN_STATE_SUSPENDED, RUN_STATE_COLO},
>> + { RUN_STATE_SUSPENDED, RUN_STATE_SAVE_VM },
>> + { RUN_STATE_SUSPENDED, RUN_STATE_RESTORE_VM },
>>
>> { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
>> { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
>> diff --git a/system/vl.c b/system/vl.c
>> index bd7fad7..082a45a 100644
>> --- a/system/vl.c
>> +++ b/system/vl.c
>> @@ -2702,7 +2702,9 @@ void qmp_x_exit_preconfig(Error **errp)
>> qemu_machine_creation_done();
>>
>> if (loadvm) {
>> + RunState state = autostart ? RUN_STATE_RUNNING : runstate_get();
>> load_snapshot(loadvm, NULL, false, NULL, &error_fatal);
>> + load_snapshot_resume(state);
>
> Here it's using error_fatal, so it won't start the VM.
Yes, same as the old code.
- Steve
