Re: [Qemu-devel] [PATCH COLO-Frame v12 05/38] migration: Add state records for migration incoming

[Hailiang Zhang]

On 2015/12/16 1:36, Dr. David Alan Gilbert wrote:
> * zhanghailiang (address@hidden) wrote:
> > For migration destination, we also need to know its state,
> > we will use it in COLO.
> >
> > Here we add a new member 'state' for MigrationIncomingState,
> > and also use migrate_set_state() to modify its value.
> >
> > Signed-off-by: zhanghailiang <address@hidden>
> > Reviewed-by: Dr. David Alan Gilbert <address@hidden>
>
> Actually note there is a bug here; see below
>
> > ---
> > v11:
> > - Split exporting migrate_set_state() part into a new patch (Juan's suggestion)
> >
> > Signed-off-by: zhanghailiang <address@hidden>
> > ---
> >   include/migration/migration.h |  1 +
> >   migration/migration.c         | 14 +++++++++-----
> >   2 files changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > index 4b19e80..99dfa92 100644
> > --- a/include/migration/migration.h
> > +++ b/include/migration/migration.h
> > @@ -105,6 +105,7 @@ struct MigrationIncomingState {
> >       QemuMutex rp_mutex;    /* We send replies from multiple threads */
> >       void     *postcopy_tmp_page;
> >
> > +    int state;
> >       /* See savevm.c */
> >       LoadStateEntry_Head loadvm_handlers;
> >   };
> > diff --git a/migration/migration.c b/migration/migration.c
> > index c9cd80d..d58ce98 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -112,6 +112,7 @@ MigrationIncomingState 
> > *migration_incoming_state_new(QEMUFile* f)
> >   {
> >       mis_current = g_new0(MigrationIncomingState, 1);
> >       mis_current->from_src_file = f;
> > +    mis_current->state = MIGRATION_STATUS_NONE;
> >       QLIST_INIT(&mis_current->loadvm_handlers);
> >       qemu_mutex_init(&mis_current->rp_mutex);
> >       qemu_event_init(&mis_current->main_thread_load_event, false);
> > @@ -332,8 +333,8 @@ static void process_incoming_migration_co(void *opaque)
> >
> >       mis = migration_incoming_state_new(f);
> >       postcopy_state_set(POSTCOPY_INCOMING_NONE);
> > -    migrate_generate_event(MIGRATION_STATUS_ACTIVE);
> > -
> > +    migrate_set_state(&mis->state, MIGRATION_STATUS_NONE,
> > +                      MIGRATION_STATUS_ACTIVE);
> >       ret = qemu_loadvm_state(f);
> >
> >       ps = postcopy_state_get();
> > @@ -362,7 +363,8 @@ static void process_incoming_migration_co(void *opaque)
> >       migration_incoming_state_destroy();
>
> We're freeing mis now - we can't use the state later!
>
> >       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);
> > @@ -371,7 +373,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);
> > @@ -403,7 +406,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);
>
> So I moved the migration_incoming_state_destroy()  to here in my world.

Yes, it is bug, thank you for fixing it.

> Dave
>
> >   }
> >
> >   void process_incoming_migration(QEMUFile *f)
> > --
> > 1.8.3.1
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
>
> .