Re: [Qemu-devel] [PATCH 5/7] migration: create now section to store global state

2014-10-20 Thread Dr. David Alan Gilbert 
* 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.

A couple of questions:
   1) Does the ordering of loading this state matter - lets say that the source
 was in an error state, then all the other device states get loaded and then
 it loads your global_state which tells the destination is in error - is 
that
 too late ? Could the device emulation have already started doing some IO
 or something to the devices which it wouldn't have done if it knew there 
was
 already a problem?

   2) What's the advantage of the optional section over using the 'command' 
sections
  I use in postcopy; 
http://lists.gnu.org/archive/html/qemu-devel/2014-10/msg00337.html ?

Dave

 Signed-off-by: Juan Quintela quint...@redhat.com
 ---
  include/migration/migration.h |  4 ++
  migration.c   | 88 
 +--
  vl.c  |  1 +
  3 files changed, 90 insertions(+), 3 deletions(-)
 
 diff --git a/include/migration/migration.h b/include/migration/migration.h
 index 3cb5ba8..bc1069b 100644
 --- a/include/migration/migration.h
 +++ b/include/migration/migration.h
 @@ -174,4 +174,8 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t 
 block_offset,
   ram_addr_t offset, size_t size,
   int *bytes_sent);
 
 +void register_global_state(void);
 +void global_state_store(void);
 +char *global_state_get_runstate(void);
 +
  #endif
 diff --git a/migration.c b/migration.c
 index 8d675b3..6f7e50e 100644
 --- a/migration.c
 +++ b/migration.c
 @@ -112,10 +112,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);
 +}
  }
  }
 
 @@ -608,6 +618,7 @@ static void *migration_thread(void *opaque)
  qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
  old_vm_running = runstate_is_running();
 
 +global_state_store();
  ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
  if (ret = 0) {
  qemu_file_set_rate_limit(s-file, INT64_MAX);
 @@ -699,3 +710,74 @@ void migrate_fd_connect(MigrationState *s)
  qemu_thread_create(s-thread, migration, migration_thread, s,
 QEMU_THREAD_JOINABLE);
  }
 +
 +typedef struct {
 +int32_t size;
 +uint8_t runstate[100];
 +} GlobalState;
 +
 +static GlobalState global_state;
 +
 +void global_state_store(void)
 +{
 +if (runstate_store((char*)global_state.runstate,
 +   sizeof(global_state.runstate)) == -1) {
 +printf(Runstate is too big\n);
 +exit(-1);
 +}
 +}
 +
 +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;
 +
 +printf(loaded state: %s\n, runstate);
 +
 +if (strcmp(runstate, running) != 0) {
 +
 +RunState r = runstate_index(runstate);
 +
 +if (r == -1) {
 +printf(Unknown received state %s\n, runstate);
 +return -1;
 +}
 +ret = vm_stop_force_state(r);
 +}
 +
 +   return ret;
 +}
 +
 +static void global_state_pre_save(void *opaque)
 +{
 +GlobalState *s = opaque;
 +
 +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,

Re: [Qemu-devel] [PATCH 5/7] migration: create now section to store global state

2014-10-20 Thread Kevin Wolf 
Am 15.10.2014 um 09:55 hat Juan Quintela geschrieben:
 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 quint...@redhat.com
 ---
  include/migration/migration.h |  4 ++
  migration.c   | 88 
 +--
  vl.c  |  1 +
  3 files changed, 90 insertions(+), 3 deletions(-)
 
 diff --git a/include/migration/migration.h b/include/migration/migration.h
 index 3cb5ba8..bc1069b 100644
 --- a/include/migration/migration.h
 +++ b/include/migration/migration.h
 @@ -174,4 +174,8 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t 
 block_offset,
   ram_addr_t offset, size_t size,
   int *bytes_sent);
 
 +void register_global_state(void);
 +void global_state_store(void);
 +char *global_state_get_runstate(void);
 +
  #endif
 diff --git a/migration.c b/migration.c
 index 8d675b3..6f7e50e 100644
 --- a/migration.c
 +++ b/migration.c
 @@ -112,10 +112,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();

Does this mean that -S is now ignored in the common case? Wouldn't it be
better to change only the case without -S? Otherwise I guess libvirt
will get quite confused.

 -} else {
 -runstate_set(RUN_STATE_PAUSED);
 +} else if (strcmp(global_state_get_runstate(), ) == 0 ) {
 +if (autostart) {
 +vm_start();
 +} else {
 +runstate_set(RUN_STATE_PAUSED);
 +}
  }
  }
 
 @@ -608,6 +618,7 @@ static void *migration_thread(void *opaque)
  qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
  old_vm_running = runstate_is_running();
 
 +global_state_store();
  ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
  if (ret = 0) {
  qemu_file_set_rate_limit(s-file, INT64_MAX);
 @@ -699,3 +710,74 @@ void migrate_fd_connect(MigrationState *s)
  qemu_thread_create(s-thread, migration, migration_thread, s,
 QEMU_THREAD_JOINABLE);
  }
 +
 +typedef struct {
 +int32_t size;
 +uint8_t runstate[100];
 +} GlobalState;
 +
 +static GlobalState global_state;
 +
 +void global_state_store(void)
 +{
 +if (runstate_store((char*)global_state.runstate,
 +   sizeof(global_state.runstate)) == -1) {
 +printf(Runstate is too big\n);
 +exit(-1);
 +}
 +}

Not sure if the concept of a single GlobalStore that calls all the
individual handlers for each piece of global state is optimal.

Can't we use something like vmstate_register()? Perhaps even the same
function, just with dev == NULL? (Actually, you even do this below, to
register the global state. So I guess I'm only disagreeing on the
granularity of having only a single section with a single handler
function for the whole global state.)

 +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;
 +
 +printf(loaded state: %s\n, runstate);
 +
 +if (strcmp(runstate, running) != 0) {
 +
 +RunState r = runstate_index(runstate);
 +
 +if (r == -1) {
 +printf(Unknown received state %s\n, runstate);
 +return -1;
 +}
 +ret = vm_stop_force_state(r);
 +}
 +
 +   return ret;
 +}
 +
 +static void global_state_pre_save(void *opaque)
 +{
 +GlobalState *s = opaque;
 +
 +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 =

[Qemu-devel] [PATCH 5/7] migration: create now section to store global state

2014-10-15 Thread Juan Quintela 
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 quint...@redhat.com
---
 include/migration/migration.h |  4 ++
 migration.c   | 88 +--
 vl.c  |  1 +
 3 files changed, 90 insertions(+), 3 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 3cb5ba8..bc1069b 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -174,4 +174,8 @@ size_t ram_control_save_page(QEMUFile *f, ram_addr_t 
block_offset,
  ram_addr_t offset, size_t size,
  int *bytes_sent);

+void register_global_state(void);
+void global_state_store(void);
+char *global_state_get_runstate(void);
+
 #endif
diff --git a/migration.c b/migration.c
index 8d675b3..6f7e50e 100644
--- a/migration.c
+++ b/migration.c
@@ -112,10 +112,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);
+}
 }
 }

@@ -608,6 +618,7 @@ static void *migration_thread(void *opaque)
 qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
 old_vm_running = runstate_is_running();

+global_state_store();
 ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
 if (ret = 0) {
 qemu_file_set_rate_limit(s-file, INT64_MAX);
@@ -699,3 +710,74 @@ void migrate_fd_connect(MigrationState *s)
 qemu_thread_create(s-thread, migration, migration_thread, s,
QEMU_THREAD_JOINABLE);
 }
+
+typedef struct {
+int32_t size;
+uint8_t runstate[100];
+} GlobalState;
+
+static GlobalState global_state;
+
+void global_state_store(void)
+{
+if (runstate_store((char*)global_state.runstate,
+   sizeof(global_state.runstate)) == -1) {
+printf(Runstate is too big\n);
+exit(-1);
+}
+}
+
+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;
+
+printf(loaded state: %s\n, runstate);
+
+if (strcmp(runstate, running) != 0) {
+
+RunState r = runstate_index(runstate);
+
+if (r == -1) {
+printf(Unknown received state %s\n, runstate);
+return -1;
+}
+ret = vm_stop_force_state(r);
+}
+
+   return ret;
+}
+
+static void global_state_pre_save(void *opaque)
+{
+GlobalState *s = opaque;
+
+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_INT32(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);
+}
diff --git a/vl.c b/vl.c
index 1788b6a..75e855e 100644
--- a/vl.c
+++ b/vl.c
@@ -4511,6 +4511,7 @@ int main(int argc, char **argv, char **envp)
 return 0;
 }

+register_global_state();
 if (incoming) {
 Error *local_err = NULL;
 qemu_start_incoming_migration(incoming, local_err);
-- 
2.1.0