Re: [Qemu-devel] [PATCH 03/12] migration: create new section to store global state
"Dr. David Alan Gilbert" wrote:
> * Juan Quintela (quint...@redhat.com) wrote:
>> This includes a new section that for now just stores the current qemu state.
>>
>> Right now, there are only one way to control what is the state of the
>> target after migration.
>>
>> - If you run the target qemu with -S, it would start stopped.
>> - If you run the target qemu without -S, it would run just after migration
>> finishes.
>>
>> The problem here is what happens if we start the target without -S and
>> there happens one error during migration that puts current state as
>> -EIO. Migration would ends (notice that the error happend doing block
>> IO, network IO, i.e. nothing related with migration), and when
>> migration finish, we would just "continue" running on destination,
>> probably hanging the guest/corruption data, whatever.
>>
>> Signed-off-by: Juan Quintela
>> ---
>> include/migration/migration.h | 1 +
>> migration/migration.c | 105
>> +++---
>> trace-events | 3 ++
>> vl.c | 1 +
>> 4 files changed, 103 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/migration/migration.h b/include/migration/migration.h
>> index 9387c8c..1280193 100644
>> --- a/include/migration/migration.h
>> +++ b/include/migration/migration.h
>> @@ -197,4 +197,5 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t
>> block_offset,
>>
>> void ram_mig_init(void);
>> void savevm_skip_section_footers(void);
>> +void register_global_state(void);
>> #endif
>> diff --git a/migration/migration.c b/migration/migration.c
>> index c6ac08a..b4b6d69 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -26,6 +26,7 @@
>> #include "qemu/thread.h"
>> #include "qmp-commands.h"
>> #include "trace.h"
>> +#include "qapi/util.h"
>>
>> #define MAX_THROTTLE (32 << 20) /* Migration speed throttling */
>>
>> @@ -97,6 +98,83 @@ void migration_incoming_state_destroy(void)
>> mis_current = NULL;
>> }
>>
>> +
>> +typedef struct {
>> +uint32_t size;
>> +uint8_t runstate[100];
>> +} GlobalState;
>> +
>> +static GlobalState global_state;
>> +
>> +static int global_state_store(void)
>> +{
>> +if (!runstate_store((char *)global_state.runstate,
>> +sizeof(global_state.runstate))) {
>> +trace_migrate_state_too_big();
>
> Hmm ok, but that should have stayed as the error_report - the only
> problem with that in the previous version was a \n
> (error_report for errors, trace_ for debug)
>
>> +return -EINVAL;
>> +}
>> +return 0;
>> +}
>> +
>> +static char *global_state_get_runstate(void)
>> +{
>> +return (char *)global_state.runstate;
>> +}
>> +
>> +static int global_state_post_load(void *opaque, int version_id)
>> +{
>> +GlobalState *s = opaque;
>> +int ret = 0;
>> +char *runstate = (char *)s->runstate;
>> +
>> +trace_migrate_global_state_post_load(runstate);
>> +
>> +if (strcmp(runstate, "running") != 0) {
>> +Error *local_err = NULL;
>> +int r = qapi_enum_parse(RunState_lookup, runstate, RUN_STATE_MAX,
>> +-1, &local_err);
>> +
>> +if (r == -1) {
>> +if (local_err) {
>> +error_report_err(local_err);
>> +}
>> +return -EINVAL;
>> +}
>> +ret = vm_stop_force_state(r);
>> +}
>> +
>> + return ret;
>> +}
>> +
>> +static void global_state_pre_save(void *opaque)
>> +{
>> +GlobalState *s = opaque;
>> +
>> +trace_migrate_global_state_pre_save((char *)s->runstate);
>> +s->size = strlen((char *)s->runstate) + 1;
>> +printf("saved state: %s\n", s->runstate);
>
> Stray printf
grrr, the removal got somehow on the following patch.
Thanks.
>
>
> Other than that,
>
> Reviewed-by: Dr. David Alan Gilbert
>
>> +}
>> +
>> +static const VMStateDescription vmstate_globalstate = {
>> +.name = "globalstate",
>> +.version_id = 1,
>> +.minimum_version_id = 1,
>> +.post_load = global_state_post_load,
>> +.pre_save = global_state_pre_save,
>> +.fields = (VMStateField[]) {
>> +VMSTATE_UINT32(size, GlobalState),
>> +VMSTATE_BUFFER(runstate, GlobalState),
>> +VMSTATE_END_OF_LIST()
>> +},
>> +};
>> +
>> +void register_global_state(void)
>> +{
>> +/* We would use it independently that we receive it */
>> +strcpy((char *)&global_state.runstate, "");
>> +vmstate_register(NULL, 0, &vmstate_globalstate, &global_state);
>> +}
>> +
>> /*
>> * Called on -incoming with a defer: uri.
>> * The migration can be started later after any parameters have been
>> @@ -164,10 +242,20 @@ static void process_incoming_migration_co(void *opaque)
>> exit(EXIT_FAILURE);
>> }
>>
>> -if (autostart) {
>> +/* runstate == "" means that we haven't received it through the
>> + * wire, so we obey autostart. runstate == runing means that we
>> + * ne
Re: [Qemu-devel] [PATCH 03/12] migration: create new section to store global state
* Juan Quintela (quint...@redhat.com) wrote:
> This includes a new section that for now just stores the current qemu state.
>
> Right now, there are only one way to control what is the state of the
> target after migration.
>
> - If you run the target qemu with -S, it would start stopped.
> - If you run the target qemu without -S, it would run just after migration
> finishes.
>
> The problem here is what happens if we start the target without -S and
> there happens one error during migration that puts current state as
> -EIO. Migration would ends (notice that the error happend doing block
> IO, network IO, i.e. nothing related with migration), and when
> migration finish, we would just "continue" running on destination,
> probably hanging the guest/corruption data, whatever.
>
> Signed-off-by: Juan Quintela
> ---
> include/migration/migration.h | 1 +
> migration/migration.c | 105
> +++---
> trace-events | 3 ++
> vl.c | 1 +
> 4 files changed, 103 insertions(+), 7 deletions(-)
>
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 9387c8c..1280193 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -197,4 +197,5 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t
> block_offset,
>
> void ram_mig_init(void);
> void savevm_skip_section_footers(void);
> +void register_global_state(void);
> #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index c6ac08a..b4b6d69 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -26,6 +26,7 @@
> #include "qemu/thread.h"
> #include "qmp-commands.h"
> #include "trace.h"
> +#include "qapi/util.h"
>
> #define MAX_THROTTLE (32 << 20) /* Migration speed throttling */
>
> @@ -97,6 +98,83 @@ void migration_incoming_state_destroy(void)
> mis_current = NULL;
> }
>
> +
> +typedef struct {
> +uint32_t size;
> +uint8_t runstate[100];
> +} GlobalState;
> +
> +static GlobalState global_state;
> +
> +static int global_state_store(void)
> +{
> +if (!runstate_store((char *)global_state.runstate,
> +sizeof(global_state.runstate))) {
> +trace_migrate_state_too_big();
Hmm ok, but that should have stayed as the error_report - the only
problem with that in the previous version was a \n
(error_report for errors, trace_ for debug)
> +return -EINVAL;
> +}
> +return 0;
> +}
> +
> +static char *global_state_get_runstate(void)
> +{
> +return (char *)global_state.runstate;
> +}
> +
> +static int global_state_post_load(void *opaque, int version_id)
> +{
> +GlobalState *s = opaque;
> +int ret = 0;
> +char *runstate = (char *)s->runstate;
> +
> +trace_migrate_global_state_post_load(runstate);
> +
> +if (strcmp(runstate, "running") != 0) {
> +Error *local_err = NULL;
> +int r = qapi_enum_parse(RunState_lookup, runstate, RUN_STATE_MAX,
> +-1, &local_err);
> +
> +if (r == -1) {
> +if (local_err) {
> +error_report_err(local_err);
> +}
> +return -EINVAL;
> +}
> +ret = vm_stop_force_state(r);
> +}
> +
> + return ret;
> +}
> +
> +static void global_state_pre_save(void *opaque)
> +{
> +GlobalState *s = opaque;
> +
> +trace_migrate_global_state_pre_save((char *)s->runstate);
> +s->size = strlen((char *)s->runstate) + 1;
> +printf("saved state: %s\n", s->runstate);
Stray printf
Other than that,
Reviewed-by: Dr. David Alan Gilbert
> +}
> +
> +static const VMStateDescription vmstate_globalstate = {
> +.name = "globalstate",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.post_load = global_state_post_load,
> +.pre_save = global_state_pre_save,
> +.fields = (VMStateField[]) {
> +VMSTATE_UINT32(size, GlobalState),
> +VMSTATE_BUFFER(runstate, GlobalState),
> +VMSTATE_END_OF_LIST()
> +},
> +};
> +
> +void register_global_state(void)
> +{
> +/* We would use it independently that we receive it */
> +strcpy((char *)&global_state.runstate, "");
> +vmstate_register(NULL, 0, &vmstate_globalstate, &global_state);
> +}
> +
> /*
> * Called on -incoming with a defer: uri.
> * The migration can be started later after any parameters have been
> @@ -164,10 +242,20 @@ static void process_incoming_migration_co(void *opaque)
> exit(EXIT_FAILURE);
> }
>
> -if (autostart) {
> +/* runstate == "" means that we haven't received it through the
> + * wire, so we obey autostart. runstate == runing means that we
> + * need to run it, we need to make sure that we do it after
> + * everything else has finished. Every other state change is done
> + * at the post_load function */
> +
> +if (strcmp(global_state_get_runstate(), "running") == 0) {
> vm_start();
[Qemu-devel] [PATCH 03/12] migration: create new section to store global state
This includes a new section that for now just stores the current qemu state.
Right now, there are only one way to control what is the state of the
target after migration.
- If you run the target qemu with -S, it would start stopped.
- If you run the target qemu without -S, it would run just after migration
finishes.
The problem here is what happens if we start the target without -S and
there happens one error during migration that puts current state as
-EIO. Migration would ends (notice that the error happend doing block
IO, network IO, i.e. nothing related with migration), and when
migration finish, we would just "continue" running on destination,
probably hanging the guest/corruption data, whatever.
Signed-off-by: Juan Quintela
---
include/migration/migration.h | 1 +
migration/migration.c | 105 +++---
trace-events | 3 ++
vl.c | 1 +
4 files changed, 103 insertions(+), 7 deletions(-)
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 9387c8c..1280193 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -197,4 +197,5 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t
block_offset,
void ram_mig_init(void);
void savevm_skip_section_footers(void);
+void register_global_state(void);
#endif
diff --git a/migration/migration.c b/migration/migration.c
index c6ac08a..b4b6d69 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -26,6 +26,7 @@
#include "qemu/thread.h"
#include "qmp-commands.h"
#include "trace.h"
+#include "qapi/util.h"
#define MAX_THROTTLE (32 << 20) /* Migration speed throttling */
@@ -97,6 +98,83 @@ void migration_incoming_state_destroy(void)
mis_current = NULL;
}
+
+typedef struct {
+uint32_t size;
+uint8_t runstate[100];
+} GlobalState;
+
+static GlobalState global_state;
+
+static int global_state_store(void)
+{
+if (!runstate_store((char *)global_state.runstate,
+sizeof(global_state.runstate))) {
+trace_migrate_state_too_big();
+return -EINVAL;
+}
+return 0;
+}
+
+static char *global_state_get_runstate(void)
+{
+return (char *)global_state.runstate;
+}
+
+static int global_state_post_load(void *opaque, int version_id)
+{
+GlobalState *s = opaque;
+int ret = 0;
+char *runstate = (char *)s->runstate;
+
+trace_migrate_global_state_post_load(runstate);
+
+if (strcmp(runstate, "running") != 0) {
+Error *local_err = NULL;
+int r = qapi_enum_parse(RunState_lookup, runstate, RUN_STATE_MAX,
+-1, &local_err);
+
+if (r == -1) {
+if (local_err) {
+error_report_err(local_err);
+}
+return -EINVAL;
+}
+ret = vm_stop_force_state(r);
+}
+
+ return ret;
+}
+
+static void global_state_pre_save(void *opaque)
+{
+GlobalState *s = opaque;
+
+trace_migrate_global_state_pre_save((char *)s->runstate);
+s->size = strlen((char *)s->runstate) + 1;
+printf("saved state: %s\n", s->runstate);
+}
+
+static const VMStateDescription vmstate_globalstate = {
+.name = "globalstate",
+.version_id = 1,
+.minimum_version_id = 1,
+.post_load = global_state_post_load,
+.pre_save = global_state_pre_save,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32(size, GlobalState),
+VMSTATE_BUFFER(runstate, GlobalState),
+VMSTATE_END_OF_LIST()
+},
+};
+
+void register_global_state(void)
+{
+/* We would use it independently that we receive it */
+strcpy((char *)&global_state.runstate, "");
+vmstate_register(NULL, 0, &vmstate_globalstate, &global_state);
+}
+
/*
* Called on -incoming with a defer: uri.
* The migration can be started later after any parameters have been
@@ -164,10 +242,20 @@ static void process_incoming_migration_co(void *opaque)
exit(EXIT_FAILURE);
}
-if (autostart) {
+/* runstate == "" means that we haven't received it through the
+ * wire, so we obey autostart. runstate == runing means that we
+ * need to run it, we need to make sure that we do it after
+ * everything else has finished. Every other state change is done
+ * at the post_load function */
+
+if (strcmp(global_state_get_runstate(), "running") == 0) {
vm_start();
-} else {
-runstate_set(RUN_STATE_PAUSED);
+} else if (strcmp(global_state_get_runstate(), "") == 0) {
+if (autostart) {
+vm_start();
+} else {
+runstate_set(RUN_STATE_PAUSED);
+}
}
migrate_decompress_threads_join();
}
@@ -793,10 +881,13 @@ static void *migration_thread(void *opaque)
qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
old_vm_running = runstate_is_running();
-ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
-
