[Qemu-devel] [PATCH] migration: Add state records for migration incoming

[Qemu-devel] [PATCH] migration: Add state records for migration incoming

[zhanghailiang]

For migration destination, sometimes we need to know its state,
and it is also useful for tracing migration incoming process.

Here we add a new member 'state' for MigrationIncomingState,
and also use migrate_set_state() to modify its value.
We fix the first parameter of migrate_set_state(), and make it
public.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
Hi,

This is picked from COLO frame series, Dave suggests me
to submit it by itself.

I fixed a little for the commit message. and keep Dave's reviewed-by tag.

Thanks.
---
 include/migration/migration.h |  3 +++
 migration/migration.c         | 43 +++++++++++++++++++++++++++----------------
 2 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 8334621..4435dee 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -50,6 +50,7 @@ typedef QLIST_HEAD(, LoadStateEntry) LoadStateEntry_Head;
 struct MigrationIncomingState {
     QEMUFile *file;
 
+    int state;
     /* See savevm.c */
     LoadStateEntry_Head loadvm_handlers;
 };
@@ -82,6 +83,8 @@ struct MigrationState
     int64_t dirty_sync_count;
 };
 
+void migrate_set_state(int *state, int old_state, int new_state);
+
 void process_incoming_migration(QEMUFile *f);
 
 void qemu_start_incoming_migration(const char *uri, Error **errp);
diff --git a/migration/migration.c b/migration/migration.c
index b092f38..85ac850 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -96,6 +96,7 @@ MigrationIncomingState *migration_incoming_state_new(QEMUFile* f)
 {
     mis_current = g_new0(MigrationIncomingState, 1);
     mis_current->file = f;
+    mis_current->state = MIGRATION_STATUS_NONE;
     QLIST_INIT(&mis_current->loadvm_handlers);
 
     return mis_current;
@@ -277,11 +278,13 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
 static void process_incoming_migration_co(void *opaque)
 {
     QEMUFile *f = opaque;
+    MigrationIncomingState *mis;
     Error *local_err = NULL;
     int ret;
 
-    migration_incoming_state_new(f);
-    migrate_generate_event(MIGRATION_STATUS_ACTIVE);
+    mis = migration_incoming_state_new(f);
+    migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
+                      MIGRATION_STATUS_ACTIVE);
     ret = qemu_loadvm_state(f);
 
     qemu_fclose(f);
@@ -289,7 +292,8 @@ static void process_incoming_migration_co(void *opaque)
     migration_incoming_state_destroy();
 
     if (ret < 0) {
-        migrate_generate_event(MIGRATION_STATUS_FAILED);
+        migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
+                          MIGRATION_STATUS_FAILED);
         error_report("load of migration failed: %s", strerror(-ret));
         migrate_decompress_threads_join();
         exit(EXIT_FAILURE);
@@ -298,7 +302,8 @@ static void process_incoming_migration_co(void *opaque)
     /* Make sure all file formats flush their mutable metadata */
     bdrv_invalidate_cache_all(&local_err);
     if (local_err) {
-        migrate_generate_event(MIGRATION_STATUS_FAILED);
+        migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
+                          MIGRATION_STATUS_FAILED);
         error_report_err(local_err);
         migrate_decompress_threads_join();
         exit(EXIT_FAILURE);
@@ -330,7 +335,8 @@ static void process_incoming_migration_co(void *opaque)
      * observer sees this event they might start to prod at the VM assuming
      * it's ready to use.
      */
-    migrate_generate_event(MIGRATION_STATUS_COMPLETED);
+    migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
+                      MIGRATION_STATUS_COMPLETED);
 }
 
 void process_incoming_migration(QEMUFile *f)
@@ -585,9 +591,9 @@ void qmp_migrate_set_parameters(bool has_compress_level,
 
 /* shared migration helpers */
 
-static void migrate_set_state(MigrationState *s, int old_state, int new_state)
+void migrate_set_state(int *state, int old_state, int new_state)
 {
-    if (atomic_cmpxchg(&s->state, old_state, new_state) == old_state) {
+    if (atomic_cmpxchg(state, old_state, new_state) == old_state) {
         trace_migrate_set_state(new_state);
         migrate_generate_event(new_state);
     }
@@ -616,7 +622,7 @@ static void migrate_fd_cleanup(void *opaque)
     if (s->state != MIGRATION_STATUS_COMPLETED) {
         qemu_savevm_state_cancel();
         if (s->state == MIGRATION_STATUS_CANCELLING) {
-            migrate_set_state(s, MIGRATION_STATUS_CANCELLING,
+            migrate_set_state(&s->state, MIGRATION_STATUS_CANCELLING,
                               MIGRATION_STATUS_CANCELLED);
         }
     }
@@ -628,7 +634,8 @@ void migrate_fd_error(MigrationState *s)
 {
     trace_migrate_fd_error();
     assert(s->file == NULL);
-    migrate_set_state(s, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_FAILED);
+    migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
+                      MIGRATION_STATUS_FAILED);
     notifier_list_notify(&migration_state_notifiers, s);
 }
 
@@ -644,7 +651,7 @@ static void migrate_fd_cancel(MigrationState *s)
             old_state != MIGRATION_STATUS_ACTIVE) {
             break;
         }
-        migrate_set_state(s, old_state, MIGRATION_STATUS_CANCELLING);
+        migrate_set_state(&s->state, old_state, MIGRATION_STATUS_CANCELLING);
     } while (s->state != MIGRATION_STATUS_CANCELLING);
 
     /*
@@ -720,7 +727,7 @@ static MigrationState *migrate_init(const MigrationParams *params)
     s->parameters[MIGRATION_PARAMETER_X_CPU_THROTTLE_INCREMENT] =
                 x_cpu_throttle_increment;
     s->bandwidth_limit = bandwidth_limit;
-    migrate_set_state(s, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP);
+    migrate_set_state(&s->state, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP);
 
     s->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
     return s;
@@ -818,7 +825,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     } else {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri",
                    "a valid migration protocol");
-        migrate_set_state(s, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_FAILED);
+        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
+                          MIGRATION_STATUS_FAILED);
         return;
     }
 
@@ -1011,11 +1019,13 @@ static void migration_completion(MigrationState *s, bool *old_vm_running,
         goto fail;
     }
 
-    migrate_set_state(s, MIGRATION_STATUS_ACTIVE, MIGRATION_STATUS_COMPLETED);
+    migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
+                      MIGRATION_STATUS_COMPLETED);
     return;
 
 fail:
-    migrate_set_state(s, MIGRATION_STATUS_ACTIVE, MIGRATION_STATUS_FAILED);
+    migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
+                      MIGRATION_STATUS_FAILED);
 }
 
 /* migration thread support */
@@ -1036,7 +1046,8 @@ static void *migration_thread(void *opaque)
     qemu_savevm_state_begin(s->file, &s->params);
 
     s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
-    migrate_set_state(s, MIGRATION_STATUS_SETUP, MIGRATION_STATUS_ACTIVE);
+    migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
+                      MIGRATION_STATUS_ACTIVE);
 
     while (s->state == MIGRATION_STATUS_ACTIVE) {
         int64_t current_time;
@@ -1055,7 +1066,7 @@ static void *migration_thread(void *opaque)
         }
 
         if (qemu_file_get_error(s->file)) {
-            migrate_set_state(s, MIGRATION_STATUS_ACTIVE,
+            migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
                               MIGRATION_STATUS_FAILED);
             break;
         }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] migration: Add state records for migration incoming

[Juan Quintela]

zhanghailiang <zhang.zhanghailiang@huawei.com> wrote:
> For migration destination, sometimes we need to know its state,
> and it is also useful for tracing migration incoming process.
>
> Here we add a new member 'state' for MigrationIncomingState,
> and also use migrate_set_state() to modify its value.
> We fix the first parameter of migrate_set_state(), and make it
> public.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

1st: split the patch about the change in migrate_set_state prototype,
and the rest.

Once there, if we are going to do this, at least show it on info migrate
on destination?

If we are going this way, I think it is going to be better to reuse
MigrationState?  That way, we could make the info migrate statistics
easier to understand?

Thanks, Juan.

Re: [Qemu-devel] [PATCH] migration: Add state records for migration incoming

[zhanghailiang]

On 2015/11/18 18:51, Juan Quintela wrote:
> zhanghailiang <zhang.zhanghailiang@huawei.com> wrote:
>> For migration destination, sometimes we need to know its state,
>> and it is also useful for tracing migration incoming process.
>>
>> Here we add a new member 'state' for MigrationIncomingState,
>> and also use migrate_set_state() to modify its value.
>> We fix the first parameter of migrate_set_state(), and make it
>> public.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
Hi Juan,

Thanks for your reply.

> 1st: split the patch about the change in migrate_set_state prototype,
> and the rest.
>

OK, i will do it in next version.

> Once there, if we are going to do this, at least show it on info migrate
> on destination?
>

I don't know if there is any sense to export migration info in destination.
In actual application, we use libvirt to issue migration command and get
migration info in source side. So it seems unnecessary to export them.

For COLO, we only use the state internally, to indicate whether we are in COLO state.

Thanks,
zhanghailiang

> If we are going this way, I think it is going to be better to reuse
> MigrationState?  That way, we could make the info migrate statistics
> easier to understand?
>
> Thanks, Juan.
>
> .
>