From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57304) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fXVdW-0005Aj-LH for qemu-devel@nongnu.org; Mon, 25 Jun 2018 13:50:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fXVdT-0006Du-T4 for qemu-devel@nongnu.org; Mon, 25 Jun 2018 13:50:46 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:40892 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 1fXVdT-0006De-MG for qemu-devel@nongnu.org; Mon, 25 Jun 2018 13:50:43 -0400 Date: Mon, 25 Jun 2018 18:50:39 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20180625175039.GK2390@work-vm> References: <20180609141457.6283-1-vsementsov@virtuozzo.com> <20180615120649.GC2615@work-vm> <20180625171447.GJ2390@work-vm> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20180625171447.GJ2390@work-vm> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] migration: invalidate cache before source start List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Sementsov-Ogievskiy Cc: qemu-devel@nongnu.org, quintela@redhat.com, jsnow@redhat.com * Dr. David Alan Gilbert (dgilbert@redhat.com) wrote: > * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote: > > 15.06.2018 15:06, Dr. David Alan Gilbert wrote: > > > * Vladimir Sementsov-Ogievskiy (vsementsov@virtuozzo.com) wrote: > > > > Invalidate cache before source start in case of failed migration. > > > >=20 > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy > > > Why doesn't the code at the bottom of migration_completion, > > > fail_invalidate: and the code in migrate_fd_cancel handle this? > > >=20 > > > What case did you see it in that those didn't handle? > > > (Also I guess it probably should set s->block_inactive =3D false) > >=20 > > on source I see: > >=20 > > 81392@1529065750.766289:migrate_set_state new state 7 > > 81392@1529065750.766330:migration_thread_file_err > > 81392@1529065750.766332:migration_thread_after_loop > >=20 > > so, we are leaving loop on > > =A0=A0=A0=A0=A0=A0=A0 if (qemu_file_get_error(s->to_dst_file)) { > > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 migrate_set_state(&s->state, curren= t_active_state, > > MIGRATION_STATUS_FAILED); > > trace_migration_thread_file_err(); > > break; > > =A0=A0=A0=A0=A0=A0=A0 } > >=20 > > and skip migration_completion() >=20 > Yeh, OK; I'd seen soemthing else a few days ago, where a cancellation > test that had previously ended with a 'cancelled' state has now ended u= p > in 'failed' (which is the state 7 you have above). > I suspect there's something else going on as well; I think what is > supposed to happen in the case of 'cancel' is that it spins in 'cancell= ing' for > a while in migrate_fd_cancel and then at the bottom of migrate_fd_cance= l > it does the recovery, but because it's going to failed instead, then > it's jumping over that recovery. Going back and actually looking at the patch again; can I ask for 1 small change; Can you set s->block_inactive =3D false in the case where you don't get the local_err (Like we do at the bottom of migrate_fd_cancel) Does that make sense? Thanks, Dave > Dave >=20 > >=20 > > >=20 > > > Dave > > >=20 > > > > --- > > > >=20 > > > > migration/migration.c | 9 ++++++++- > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > >=20 > > > > diff --git a/migration/migration.c b/migration/migration.c > > > > index 1e99ec9b7e..8f39e0dc02 100644 > > > > --- a/migration/migration.c > > > > +++ b/migration/migration.c > > > > @@ -2806,7 +2806,14 @@ static void migration_iteration_finish(Mig= rationState *s) > > > > case MIGRATION_STATUS_FAILED: > > > > case MIGRATION_STATUS_CANCELLED: > > > > if (s->vm_was_running) { > > > > - vm_start(); > > > > + Error *local_err =3D NULL; > > > > + bdrv_invalidate_cache_all(&local_err); > > > > + if (local_err) { > > > > + error_reportf_err(local_err, "Can't invalidate d= isks before " > > > > + "source vm start"); > > > > + } else { > > > > + vm_start(); > > > > + } > > > > } else { > > > > if (runstate_check(RUN_STATE_FINISH_MIGRATE)) { > > > > runstate_set(RUN_STATE_POSTMIGRATE); > > > > --=20 > > > > 2.11.1 > > > >=20 > > > -- > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > >=20 > >=20 > > --=20 > > Best regards, > > Vladimir > >=20 > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK