Re: [Qemu-devel] [RFC v2 09/33] migration: implement "postcopy-pause" src logic

[Peter Xu <peterx@redhat.com>]

On Thu, Sep 21, 2017 at 08:21:45PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > Now when network down for postcopy, the source side will not fail the
> > migration. Instead we convert the status into this new paused state, and
> > we will try to wait for a rescue in the future.
> > 
> > If a recovery is detected, migration_thread() will reset its local
> > variables to prepare for that.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/migration.c  | 98 +++++++++++++++++++++++++++++++++++++++++++++++---
> >  migration/migration.h  |  3 ++
> >  migration/trace-events |  1 +
> >  3 files changed, 98 insertions(+), 4 deletions(-)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index f6130db..8d26ea8 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -993,6 +993,8 @@ static void migrate_fd_cleanup(void *opaque)
> >  
> >      notifier_list_notify(&migration_state_notifiers, s);
> >      block_cleanup_parameters(s);
> > +
> > +    qemu_sem_destroy(&s->postcopy_pause_sem);
> >  }
> >  
> >  void migrate_fd_error(MigrationState *s, const Error *error)
> > @@ -1136,6 +1138,7 @@ MigrationState *migrate_init(void)
> >      s->migration_thread_running = false;
> >      error_free(s->error);
> >      s->error = NULL;
> > +    qemu_sem_init(&s->postcopy_pause_sem, 0);
> >  
> >      migrate_set_state(&s->state, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP);
> >  
> > @@ -1938,6 +1941,80 @@ bool migrate_colo_enabled(void)
> >      return s->enabled_capabilities[MIGRATION_CAPABILITY_X_COLO];
> >  }
> >  
> > +typedef enum MigThrError {
> > +    /* No error detected */
> > +    MIG_THR_ERR_NONE = 0,
> > +    /* Detected error, but resumed successfully */
> > +    MIG_THR_ERR_RECOVERED = 1,
> > +    /* Detected fatal error, need to exit */
> > +    MIG_THR_ERR_FATAL = 2,
> 
> I don't think it's necessary to assign the values there, but it's OK.
> 
> > +} MigThrError;
> > +
> > +/*
> > + * We don't return until we are in a safe state to continue current
> > + * postcopy migration.  Returns MIG_THR_ERR_RECOVERED if recovered, or
> > + * MIG_THR_ERR_FATAL if unrecovery failure happened.
> > + */
> > +static MigThrError postcopy_pause(MigrationState *s)
> > +{
> > +    assert(s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE);
> > +    migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > +                      MIGRATION_STATUS_POSTCOPY_PAUSED);
> > +
> > +    /* Current channel is possibly broken. Release it. */
> > +    assert(s->to_dst_file);
> > +    qemu_file_shutdown(s->to_dst_file);
> > +    qemu_fclose(s->to_dst_file);
> > +    s->to_dst_file = NULL;
> > +
> > +    error_report("Detected IO failure for postcopy. "
> > +                 "Migration paused.");
> > +
> > +    /*
> > +     * We wait until things fixed up. Then someone will setup the
> > +     * status back for us.
> > +     */
> > +    while (s->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> > +        qemu_sem_wait(&s->postcopy_pause_sem);
> > +    }
> > +
> > +    trace_postcopy_pause_continued();
> > +
> > +    return MIG_THR_ERR_RECOVERED;
> > +}
> > +
> > +static MigThrError migration_detect_error(MigrationState *s)
> > +{
> > +    int ret;
> > +
> > +    /* Try to detect any file errors */
> > +    ret = qemu_file_get_error(s->to_dst_file);
> > +
> > +    if (!ret) {
> > +        /* Everything is fine */
> > +        return MIG_THR_ERR_NONE;
> > +    }
> > +
> > +    if (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE && ret == -EIO) {
> 
> We do need to make sure that whenever we hit a failure in migration
> due to a device that we pass that up rather than calling
> qemu_file_set_error - e.g. an EIO in a block device or network.
> 
> However,
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

I'll take the R-b first. :)

Regarding to above - aren't we currently detecting these kind of
errors using -EIO?  And network down should be only one of such case?

For now I still cannot distinguish network down out of something worse
that cannot even be recovered.  No matter what, current code will go
into PAUSED state as long as EIO is got.  I thought about it, and for
now I don't think it is a problem, since even if it is a critical
failure and unable to recover in any way, we still won't lose anything
if we stop the VM at once (that's what paused state do - VM is just
stopped).  For the critical failures, we will just find out that the
recovery will fail again rather than success.

-- 
Peter Xu