Re: [Qemu-devel] [PATCH COLO-Frame v12 27/38] COLO failover: Don't do failover during loading VM's state

[Hailiang Zhang]

On 2015/12/15 18:21, Dr. David Alan Gilbert wrote:
> * zhanghailiang (address@hidden) wrote:
> > We should not do failover work while the main thread is loading
> > VM's state, otherwise it will destroy the consistent of VM's memory and
> > device state.
> >
> > Here we add a new failover status 'RELAUNCH' which means we should
> > relaunch the process of failover.
> >
> > Signed-off-by: zhanghailiang <address@hidden>
> > Signed-off-by: Li Zhijian <address@hidden>
> > ---
> >   include/migration/failover.h |  2 ++
> >   migration/colo.c             | 25 +++++++++++++++++++++++++
> >   2 files changed, 27 insertions(+)
> >
> > diff --git a/include/migration/failover.h b/include/migration/failover.h
> > index fba3931..e115d25 100644
> > --- a/include/migration/failover.h
> > +++ b/include/migration/failover.h
> > @@ -20,6 +20,8 @@ typedef enum COLOFailoverStatus {
> >       FAILOVER_STATUS_REQUEST = 1, /* Request but not handled */
> >       FAILOVER_STATUS_HANDLING = 2, /* In the process of handling failover */
> >       FAILOVER_STATUS_COMPLETED = 3, /* Finish the failover process */
> > +    /* Optional, Relaunch the failover process, again 'NONE' -> 'COMPLETED' */
> > +    FAILOVER_STATUS_RELAUNCH = 4,
> >   } COLOFailoverStatus;
> >
> >   void failover_init_state(void);
> > diff --git a/migration/colo.c b/migration/colo.c
> > index 58531e7..f4bb661 100644
> > --- a/migration/colo.c
> > +++ b/migration/colo.c
> > @@ -20,6 +20,8 @@
> >   #include "migration/failover.h"
> >   #include "qapi-event.h"
> >
> > +static bool vmstate_loading;
> > +
> >   /* colo buffer */
> >   #define COLO_BUFFER_BASE_SIZE (4 * 1024 * 1024)
> >
> > @@ -52,6 +54,19 @@ static void secondary_vm_do_failover(void)
> >       int old_state;
> >       MigrationIncomingState *mis = migration_incoming_get_current();
> >
> > +    /* Can not do failover during the process of VM's loading VMstate, Or
> > +      * it will break the secondary VM.
> > +      */
> > +    if (vmstate_loading) {
> > +        old_state = failover_set_state(FAILOVER_STATUS_HANDLING,
> > +                                       FAILOVER_STATUS_RELAUNCH);
> > +        if (old_state != FAILOVER_STATUS_HANDLING) {
> > +            error_report("Unknow error while do failover for secondary VM,"
> > +                         "old_state: %d", old_state);
>
> Typo: 'Unknown' and it would be good to say it was during vmstate_loading.
>
> The state is being loaded from the qemu buffer, not the real file descriptor,
> so we're guaranteed that the vmstate will finish loading; so yes, this is OK.

I will fix it in next version.

Thanks.
Hailiang
> Reviewed-by: Dr. David Alan Gilbert <address@hidden>
>
>
> > +        }
> > +        return;
> > +    }
> > +
> >       migrate_set_state(&mis->state, MIGRATION_STATUS_COLO,
> >                         MIGRATION_STATUS_COMPLETED);
> >
> > @@ -535,13 +550,23 @@ void *colo_process_incoming_thread(void *opaque)
> >
> >           qemu_mutex_lock_iothread();
> >           qemu_system_reset(VMRESET_SILENT);
> > +        vmstate_loading = true;
> >           if (qemu_loadvm_state(fb) < 0) {
> >               error_report("COLO: loadvm failed");
> > +            vmstate_loading = false;
> >               qemu_mutex_unlock_iothread();
> >               goto out;
> >           }
> > +
> > +        vmstate_loading = false;
> >           qemu_mutex_unlock_iothread();
> >
> > +        if (failover_get_state() == FAILOVER_STATUS_RELAUNCH) {
> > +            failover_set_state(FAILOVER_STATUS_RELAUNCH, FAILOVER_STATUS_NONE);
> > +            failover_request_active(NULL);
> > +            goto out;
> > +        }
> > +
> >           ret = colo_put_cmd(mis->to_src_file, COLO_COMMAND_VMSTATE_LOADED);
> >           if (ret < 0) {
> >               goto out;
> > --
> > 1.8.3.1
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
>
> .