[Qemu-devel] [PATCH 0/2] Fix global state with savevm

[Qemu-devel] [PATCH 0/2] Fix global state with savevm

[Juan Quintela]

Hi

Store global state for both savevm & migration in a single place.
Register globalstate save handler before loadvm happens.

Please, review.

Juan Quintela (2):
  migration: Register global state section before loadvm
  migration: store globalstate in pre_safe

 migration/migration.c | 30 ++++++++++++------------------
 vl.c                  |  2 +-
 2 files changed, 13 insertions(+), 19 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/2] migration: Register global state section before loadvm

[Juan Quintela]

Otherwise, it is not found

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 vl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 3f269dc..5856396 100644
--- a/vl.c
+++ b/vl.c
@@ -4615,6 +4615,7 @@ int main(int argc, char **argv, char **envp)
     }

     qemu_system_reset(VMRESET_SILENT);
+    register_global_state();
     if (loadvm) {
         if (load_vmstate(loadvm) < 0) {
             autostart = 0;
@@ -4628,7 +4629,6 @@ 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.4.3

[Qemu-devel] [PATCH 2/2] migration: store globalstate in pre_safe

[Juan Quintela]

We use global state in both savevm & migration.  The easiest way is to
put the setup in a single place.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index ba82ff6..d1421fe 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -110,17 +110,6 @@ typedef struct {

 static GlobalState global_state;

-static int global_state_store(void)
-{
-    if (!runstate_store((char *)global_state.runstate,
-                        sizeof(global_state.runstate))) {
-        error_report("runstate name too big: %s", global_state.runstate);
-        trace_migrate_state_too_big();
-        return -EINVAL;
-    }
-    return 0;
-}
-
 static bool global_state_received(void)
 {
     return global_state.received;
@@ -187,6 +176,14 @@ static void global_state_pre_save(void *opaque)
     GlobalState *s = opaque;

     trace_migrate_global_state_pre_save((char *)s->runstate);
+
+    if (!runstate_store((char *)global_state.runstate,
+                        sizeof(global_state.runstate))) {
+        error_report("runstate name too big: %s", global_state.runstate);
+        trace_migrate_state_too_big();
+        exit(EXIT_FAILURE);
+    }
+
     s->size = strlen((char *)s->runstate) + 1;
 }

@@ -940,13 +937,10 @@ static void *migration_thread(void *opaque)
                 qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
                 old_vm_running = runstate_is_running();

-                ret = global_state_store();
-                if (!ret) {
-                    ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
-                    if (ret >= 0) {
-                        qemu_file_set_rate_limit(s->file, INT64_MAX);
-                        qemu_savevm_state_complete(s->file);
-                    }
+                ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+                if (ret >= 0) {
+                    qemu_file_set_rate_limit(s->file, INT64_MAX);
+                    qemu_savevm_state_complete(s->file);
                 }
                 qemu_mutex_unlock_iothread();

-- 
2.4.3

Re: [Qemu-devel] [PATCH 2/2] migration: store globalstate in pre_safe

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> We use global state in both savevm & migration.  The easiest way is to
> put the setup in a single place.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

I don't think this works;  I think pre-save is called after the migration
code has changed the runstate, so you end up saving the wrong runstate,
and that explains the error Christian sees.

Dave

> ---
>  migration/migration.c | 30 ++++++++++++------------------
>  1 file changed, 12 insertions(+), 18 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index ba82ff6..d1421fe 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -110,17 +110,6 @@ typedef struct {
> 
>  static GlobalState global_state;
> 
> -static int global_state_store(void)
> -{
> -    if (!runstate_store((char *)global_state.runstate,
> -                        sizeof(global_state.runstate))) {
> -        error_report("runstate name too big: %s", global_state.runstate);
> -        trace_migrate_state_too_big();
> -        return -EINVAL;
> -    }
> -    return 0;
> -}
> -
>  static bool global_state_received(void)
>  {
>      return global_state.received;
> @@ -187,6 +176,14 @@ static void global_state_pre_save(void *opaque)
>      GlobalState *s = opaque;
> 
>      trace_migrate_global_state_pre_save((char *)s->runstate);
> +
> +    if (!runstate_store((char *)global_state.runstate,
> +                        sizeof(global_state.runstate))) {
> +        error_report("runstate name too big: %s", global_state.runstate);
> +        trace_migrate_state_too_big();
> +        exit(EXIT_FAILURE);
> +    }
> +
>      s->size = strlen((char *)s->runstate) + 1;
>  }
> 
> @@ -940,13 +937,10 @@ static void *migration_thread(void *opaque)
>                  qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
>                  old_vm_running = runstate_is_running();
> 
> -                ret = global_state_store();
> -                if (!ret) {
> -                    ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> -                    if (ret >= 0) {
> -                        qemu_file_set_rate_limit(s->file, INT64_MAX);
> -                        qemu_savevm_state_complete(s->file);
> -                    }
> +                ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> +                if (ret >= 0) {
> +                    qemu_file_set_rate_limit(s->file, INT64_MAX);
> +                    qemu_savevm_state_complete(s->file);
>                  }
>                  qemu_mutex_unlock_iothread();
> 
> -- 
> 2.4.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/2] migration: store globalstate in pre_safe

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> We use global state in both savevm & migration.  The easiest way is to
>> put the setup in a single place.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> I don't think this works;  I think pre-save is called after the migration
> code has changed the runstate, so you end up saving the wrong runstate,
> and that explains the error Christian sees.

Grrrr, it works for savevm.  Just to make clear how things went
completely wrong:

I add this code to global_state_pre_save with a guard of:

    if (strcmp(global_State_runstate, "") != 0) {

    }

And it worked for savevm.  So, if it works for savevm, it will also work
for migration, right?  Code paths are almost identical.  And have the
code in a single place.

It appears that answer was not.

Sorry, Juan.


Adding back to both places.


>
> Dave
>
>> ---
>>  migration/migration.c | 30 ++++++++++++------------------
>>  1 file changed, 12 insertions(+), 18 deletions(-)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index ba82ff6..d1421fe 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -110,17 +110,6 @@ typedef struct {
>> 
>>  static GlobalState global_state;
>> 
>> -static int global_state_store(void)
>> -{
>> -    if (!runstate_store((char *)global_state.runstate,
>> -                        sizeof(global_state.runstate))) {
>> -        error_report("runstate name too big: %s", global_state.runstate);
>> -        trace_migrate_state_too_big();
>> -        return -EINVAL;
>> -    }
>> -    return 0;
>> -}
>> -
>>  static bool global_state_received(void)
>>  {
>>      return global_state.received;
>> @@ -187,6 +176,14 @@ static void global_state_pre_save(void *opaque)
>>      GlobalState *s = opaque;
>> 
>>      trace_migrate_global_state_pre_save((char *)s->runstate);
>> +
>> +    if (!runstate_store((char *)global_state.runstate,
>> +                        sizeof(global_state.runstate))) {
>> +        error_report("runstate name too big: %s", global_state.runstate);
>> +        trace_migrate_state_too_big();
>> +        exit(EXIT_FAILURE);
>> +    }
>> +
>>      s->size = strlen((char *)s->runstate) + 1;
>>  }
>> 
>> @@ -940,13 +937,10 @@ static void *migration_thread(void *opaque)
>>                  qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
>>                  old_vm_running = runstate_is_running();
>> 
>> -                ret = global_state_store();
>> -                if (!ret) {
>> -                    ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
>> -                    if (ret >= 0) {
>> -                        qemu_file_set_rate_limit(s->file, INT64_MAX);
>> -                        qemu_savevm_state_complete(s->file);
>> -                    }
>> +                ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
>> +                if (ret >= 0) {
>> +                    qemu_file_set_rate_limit(s->file, INT64_MAX);
>> +                    qemu_savevm_state_complete(s->file);
>>                  }
>>                  qemu_mutex_unlock_iothread();
>> 
>> -- 
>> 2.4.3
>> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/2] Fix global state with savevm

[Christian Borntraeger]

Am 10.07.2015 um 14:58 schrieb Juan Quintela:
> Hi
> 
> Store global state for both savevm & migration in a single place.
> Register globalstate save handler before loadvm happens.
> 
> Please, review.
> 
> Juan Quintela (2):
>   migration: Register global state section before loadvm
>   migration: store globalstate in pre_safe
> 
>  migration/migration.c | 30 ++++++++++++------------------
>  vl.c                  |  2 +-
>  2 files changed, 13 insertions(+), 19 deletions(-)
> 

Patch 2 does not seem to fit on 2.4.0-rc0 without fixing up a bit.

I fixed up but a 2.4.0-rc0 + patches  managesave/start on s390 gives me
ERROR: invalid runstate transition: 'inmigrate' -> 'finish-migrate'

Christian