Re: [Qemu-devel] [PATCH] migration: isolate return path on src

[Peter Xu <peterx@redhat.com>]

On Wed, May 31, 2017 at 08:33:01PM +0200, Juan Quintela wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
> >> There are some places that binded "return path" with postcopy. Let's be
> >> prepared for its usage even without postcopy. This patch mainly did this
> >> on source side.
> >> 
> >> Signed-off-by: Peter Xu <peterx@redhat.com>
> >> ---
> >> standalone patch isolated from the return path series. ok to be picked
> >> up in case one day we'll re-face the return path enablement.
> >> With it, we are ready on source side. The dst side change has been queued.
> >> ---
> >>  migration/migration.c  | 11 ++++++-----
> >>  migration/trace-events |  4 ++--
> >>  2 files changed, 8 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/migration/migration.c b/migration/migration.c
> >> index ad29e53..96e549e 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -1850,13 +1850,12 @@ static void migration_completion(MigrationState *s, int current_active_state,
> >>       * cleaning everything else up (since if there are no failures
> >>       * it will wait for the destination to send it's status in
> >>       * a SHUT command).
> >> -     * Postcopy opens rp if enabled (even if it's not avtivated)
> >>       */
> >> -    if (migrate_postcopy_ram()) {
> >> +    if (s->rp_state.from_dst_file) {
> >>          int rp_error;
> >> -        trace_migration_completion_postcopy_end_before_rp();
> >> +        trace_migration_return_path_end_before();
> >>          rp_error = await_return_path_close_on_source(s);
> >> -        trace_migration_completion_postcopy_end_after_rp(rp_error);
> >> +        trace_migration_return_path_end_after(rp_error);
> >>          if (rp_error) {
> >>              goto fail_invalidate;
> >>          }
> >> @@ -1931,13 +1930,15 @@ static void *migration_thread(void *opaque)
> >>  
> >>      qemu_savevm_state_header(s->to_dst_file);
> >>  
> >> -    if (migrate_postcopy_ram()) {
> >> +    if (s->to_dst_file) {
> >>          /* Now tell the dest that it should open its end so it can reply */
> >>          qemu_savevm_send_open_return_path(s->to_dst_file);
> >>  
> >>          /* And do a ping that will make stuff easier to debug */
> >>          qemu_savevm_send_ping(s->to_dst_file, 1);
> >
> > I'm confused.
> > The migration_thread on the source will always have a s->to_dst_file
> > there so why test?
> >
> > But also, we can't send the 'open return path' and 'send ping' messages
> > without a guard; sending them to old QEMUs will cause them to fail when
> > they don't know what the message is.   I suspect sending them to
> > slightly-old QEMUs will cause them to fail when they try and send a 
> > return message if the destination can't send it.
> >
> > I can see a  if (s->rp_state.from_dst_file) making some sense if it's
> > done at the right point.
> 
> You are right.
> 
> Second chunk makes no sense.  I told Peter to split the
> "objectification" of migration state and this patch.  This made more
> sense when the user *had* required a return path.

Oops! I believe it should be what Dave mentioned
(s->rp_state.from_dst_file rather than s->to_dst_file). Sorry!

I'll repost to see whether we'd like the new one. Thanks,

-- 
Peter Xu