From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36917) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dFyDR-0007Lv-SI for qemu-devel@nongnu.org; Wed, 31 May 2017 03:38:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dFyDO-0005CE-J4 for qemu-devel@nongnu.org; Wed, 31 May 2017 03:38:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38620) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dFyDO-0005By-A5 for qemu-devel@nongnu.org; Wed, 31 May 2017 03:38:46 -0400 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 575AF81241 for ; Wed, 31 May 2017 07:38:45 +0000 (UTC) Date: Wed, 31 May 2017 15:38:41 +0800 From: Peter Xu Message-ID: <20170531073841.GH14845@pxdev.xzpeter.org> References: <1495176212-14446-1-git-send-email-peterx@redhat.com> <1495176212-14446-7-git-send-email-peterx@redhat.com> <87shjme3ox.fsf@secure.mitica> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <87shjme3ox.fsf@secure.mitica> Subject: Re: [Qemu-devel] [PATCH RFC 6/6] migration: enable return path for precopy List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: qemu-devel@nongnu.org, "Dr . David Alan Gilbert" On Tue, May 30, 2017 at 05:59:10PM +0200, Juan Quintela wrote: > Peter Xu 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 > > --- > > 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