From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43790) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fb0Mk-0005Qq-4f for qemu-devel@nongnu.org; Thu, 05 Jul 2018 05:15:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fb0Mg-0006EK-PC for qemu-devel@nongnu.org; Thu, 05 Jul 2018 05:15:53 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:38568 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fb0Mg-0006DS-JJ for qemu-devel@nongnu.org; Thu, 05 Jul 2018 05:15:50 -0400 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3C12987928 for ; Thu, 5 Jul 2018 09:15:50 +0000 (UTC) Date: Thu, 5 Jul 2018 10:15:47 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20180705091546.GC2538@work-vm> References: <20180705031755.3254-1-peterx@redhat.com> <20180705031755.3254-3-peterx@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180705031755.3254-3-peterx@redhat.com> Subject: Re: [Qemu-devel] [PATCH for-3.0 2/9] migration: loosen recovery check when load vm List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Xu Cc: qemu-devel@nongnu.org, Juan Quintela * Peter Xu (peterx@redhat.com) wrote: > We were checking against -EIO, assuming that it will cover all IO > failures. But actually it is not. One example is that in > qemu_loadvm_section_start_full() we can have tons of places that will > return -EINVAL even if the error is caused by IO failures on the > network. I suspect we should fix those; but OK; I think the only cases in there that could hit that are the get_counted_string and the check_section_footer; they could both be fixed to return an errno like the other cases. > Let's loosen the recovery check logic here to cover all the error cases > happened by removing the explicit check against -EIO. After all we > won't lose anything here if any other failure happened. > > Signed-off-by: Peter Xu > --- > migration/savevm.c | 16 ++++++---------- > 1 file changed, 6 insertions(+), 10 deletions(-) > > diff --git a/migration/savevm.c b/migration/savevm.c > index 851d74e8b6..efcc795071 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -2276,18 +2276,14 @@ out: > qemu_file_set_error(f, ret); > > /* > - * Detect whether it is: > - * > - * 1. postcopy running (after receiving all device data, which > - * must be in POSTCOPY_INCOMING_RUNNING state. Note that > - * POSTCOPY_INCOMING_LISTENING is still not enough, it's > - * still receiving device states). > - * 2. network failure (-EIO) > - * > - * If so, we try to wait for a recovery. > + * If we are during an active postcopy, then we pause instead > + * of bail out to at least keep the VM's dirty data. Note > + * that POSTCOPY_INCOMING_LISTENING stage is still not enough, > + * during which we're still receiving device states and we > + * still haven't yet started the VM on destination. > */ > if (postcopy_state_get() == POSTCOPY_INCOMING_RUNNING && > - ret == -EIO && postcopy_pause_incoming(mis)) { > + postcopy_pause_incoming(mis)) { Hmm, OK; It does make me a bit nervous we might trigger this too often, but I see the other cases where we're missing stuff we should trigger it. We might find we need to go the other way and blacklist some errors. Reviewed-by: Dr. David Alan Gilbert > /* Reset f to point to the newly created channel */ > f = mis->from_src_file; > goto retry; > -- > 2.17.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK