Re: [Qemu-devel] [PATCH] migration: invalidate cache before source start

[Dr. David Alan Gilbert]

* Vladimir Sementsov-Ogievskiy (address@hidden) wrote:
> 15.06.2018 15:06, Dr. David Alan Gilbert wrote:
> > * Vladimir Sementsov-Ogievskiy (address@hidden) wrote:
> > > Invalidate cache before source start in case of failed migration.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > Why doesn't the code at the bottom of migration_completion,
> > fail_invalidate:   and the code in migrate_fd_cancel   handle this?
> > 
> > What case did you see it in that those didn't handle?
> > (Also I guess it probably should set s->block_inactive = false)
> 
> on source I see:
> 
> address@hidden:migrate_set_state new state 7
> address@hidden:migration_thread_file_err
> address@hidden:migration_thread_after_loop
> 
> so, we are leaving loop on
>         if (qemu_file_get_error(s->to_dst_file)) {
>             migrate_set_state(&s->state, current_active_state,
> MIGRATION_STATUS_FAILED);
> trace_migration_thread_file_err();
> break;
>         }
> 
> and skip migration_completion()

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 up
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 'cancelling' for
a while in migrate_fd_cancel and then at the bottom of migrate_fd_cancel
it does the recovery, but because it's going to failed instead, then
it's jumping over that recovery.

Dave

> 
> > 
> > Dave
> > 
> > > ---
> > > 
> > >   migration/migration.c | 9 ++++++++-
> > >   1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > 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(MigrationState *s)
> > >       case MIGRATION_STATUS_FAILED:
> > >       case MIGRATION_STATUS_CANCELLED:
> > >           if (s->vm_was_running) {
> > > -            vm_start();
> > > +            Error *local_err = NULL;
> > > +            bdrv_invalidate_cache_all(&local_err);
> > > +            if (local_err) {
> > > +                error_reportf_err(local_err, "Can't invalidate disks 
> > > before "
> > > +                                  "source vm start");
> > > +            } else {
> > > +                vm_start();
> > > +            }
> > >           } else {
> > >               if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
> > >                   runstate_set(RUN_STATE_POSTMIGRATE);
> > > -- 
> > > 2.11.1
> > > 
> > --
> > Dr. David Alan Gilbert / address@hidden / Manchester, UK
> 
> 
> -- 
> Best regards,
> Vladimir
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK