Re: [Qemu-devel] [PATCH RFC 6/6] migration: enable return path for precopy

[Peter Xu]

On Tue, May 30, 2017 at 05:59:10PM +0200, Juan Quintela wrote:
> Peter Xu <address@hidden> wrote:
> > Let this be a flag, default to on. Turn it off for <=2.9 versions.
> >
> > After this patch, return path will be on even for pre-copy migration as
> > long as the transport support, e.g., for socket typed transport
> > including "tcp|udp|unix" typed.
> >
> > This will naturally fix the bug mentioned below, when destination failed
> > on migration but source assumed it was successful - since now even for
> > precopy, source will wait for destination's MIG_RP_MSG_SHUT signal,
> > which will carry the final migration status of destination. Then, when
> > destination failed at any point of migration, source will know it, and
> > it'll resume the VM instead of a data lost.
> >
> > Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1439147
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  include/hw/compat.h           |  4 ++++
> >  include/migration/migration.h |  3 +++
> >  migration/migration.c         | 15 ++++++++++++++-
> >  3 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > index 55b1765..049457b 100644
> > --- a/include/hw/compat.h
> > +++ b/include/hw/compat.h
> > @@ -6,6 +6,10 @@
> >          .driver   = "pci-bridge",\
> >          .property = "shpc",\
> >          .value    = "off",\
> > +    },{\
> > +        .driver   = "migration",\
> > +        .property = "return-path",\
> > +        .value    = "off",\
> >      },
> >  
> >  #define HW_COMPAT_2_8 \
> > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > index 70710de..e44119c 100644
> > --- a/include/migration/migration.h
> > +++ b/include/migration/migration.h
> > @@ -169,6 +169,9 @@ typedef struct MigrationState {
> >      int64_t colo_checkpoint_time;
> >      QEMUTimer *colo_delay_timer;
> >  
> > +    /* Whether to try to enable return-path even for pre-copy */
> > +    bool enable_return_path;
> > +
> >      /* The last error that occurred */
> >      Error *error;
> >  } MigrationState ;
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 6df3483..16a856a 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -2046,7 +2046,7 @@ static void *migration_thread(void *opaque)
> >  static bool migrate_return_path_create(MigrationState *s)
> >  {
> >      /* Whether we should enable return path */
> > -    bool enable_return_path = false;
> > +    bool enable_return_path = s->enable_return_path;
> 
> As you can see on my suggestion for this piece of code, just add the
> ()s->enable_return_path &&) to the right place on the call?
> 
> Thanks, Juan.

Do you mean this?

    /*
     * Open the return path
     */
    if (migrate_postcopy_ram() || s->enable_return_path) {
        if (!migrate_return_path_create(s)) {
            migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
                              MIGRATION_STATUS_FAILED);
            migrate_fd_cleanup(s);
            return;
        }
    }

Here what I wanted to achieve is that:

a. for postcopy, we should try to enable return path, and it must
   succeed

b. for the case when enable_return_path is set, we try to enable return
   path, but even if it failed, we can still continue

Could we really achieve (b) if with above code? Or anything I missed?

Thanks,

-- 
Peter Xu