On Tue, May 30, 2017 at 05:59:10PM +0200, Juan Quintela wrote:
> Peter Xu <pet...@redhat.com> 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 <pet...@redhat.com>
> > ---
> > 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
