Re: [Qemu-devel] [RFC 12/29] migration: allow dst vm pause on postcopy

[Peter Xu]

On Tue, Aug 01, 2017 at 10:47:16AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (address@hidden) wrote:

[...]

> > +/* Return true if we should continue the migration, or false. */
> > +static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> > +{
> > +    trace_postcopy_pause_incoming();
> > +
> > +    migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > +                      MIGRATION_STATUS_POSTCOPY_PAUSED);
> > +
> > +    assert(mis->from_src_file);
> > +    qemu_file_shutdown(mis->from_src_file);
> > +    qemu_fclose(mis->from_src_file);
> > +    mis->from_src_file = NULL;
> > +
> > +    assert(mis->to_src_file);
> > +    qemu_mutex_lock(&mis->rp_mutex);
> > +    qemu_file_shutdown(mis->to_src_file);
> > +    qemu_fclose(mis->to_src_file);
> > +    mis->to_src_file = NULL;
> > +    qemu_mutex_unlock(&mis->rp_mutex);
> 
> Hmm is that safe?  If we look at migrate_send_rp_message we have:
> 
>     static void migrate_send_rp_message(MigrationIncomingState *mis,
>                                         enum mig_rp_message_type message_type,
>                                         uint16_t len, void *data)
>     {
>         trace_migrate_send_rp_message((int)message_type, len);
>         qemu_mutex_lock(&mis->rp_mutex);
>         qemu_put_be16(mis->to_src_file, (unsigned int)message_type);
>         qemu_put_be16(mis->to_src_file, len);
>         qemu_put_buffer(mis->to_src_file, data, len);
>         qemu_fflush(mis->to_src_file);
>         qemu_mutex_unlock(&mis->rp_mutex);
>     }
> 
> If we came into postcopy_pause_incoming at about the same time
> migrate_send_rp_message was being called and pause_incoming took the
> lock first, then once it release the lock, send_rp_message carries on
> and uses mis->to_src_file that's now NULL.
> 
> One solution here is to just call qemu_file_shutdown() but leave the
> files open at this point, but clean the files up sometime later.

I see the commnent on patch 14 as well - yeah, we need patch 14 to
co-op here, and as long as we are with patch 14, we should be ok.

> 
> > +
> > +    while (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> > +        qemu_sem_wait(&mis->postcopy_pause_sem_dst);
> > +    }
> > +
> > +    trace_postcopy_pause_incoming_continued();
> > +
> > +    return true;
> > +}
> > +
> >  static int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> >  {
> >      uint8_t section_type;
> >      int ret = 0;
> >  
> > +retry:
> >      while (true) {
> >          section_type = qemu_get_byte(f);
> >  
> > @@ -2004,6 +2034,21 @@ static int qemu_loadvm_state_main(QEMUFile *f, 
> > MigrationIncomingState *mis)
> >  out:
> >      if (ret < 0) {
> >          qemu_file_set_error(f, ret);
> > +
> > +        /*
> > +         * Detect whether it is:
> > +         *
> > +         * 1. postcopy running
> > +         * 2. network failure (-EIO)
> > +         *
> > +         * If so, we try to wait for a recovery.
> > +         */
> > +        if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
> > +            ret == -EIO && postcopy_pause_incoming(mis)) {
> > +            /* Reset f to point to the newly created channel */
> > +            f = mis->from_src_file;
> > +            goto retry;
> > +        }
> 
> I wonder if:
> 
>            if (mis->state == MIGRATION_STATUS_POSTCOPY_ACTIVE &&
>                ret == -EIO && postcopy_pause_incoming(mis)) {
>                /* Try again after postcopy recovery */
>                return qemu_loadvm_state_main(mis->from_src_file, mis);
>            }
> would be nicer; it avoids the goto loop.

I agree we should avoid using goto loops. However I do see vast usages
of goto like this one when we want to redo part of the procedures of a
function (or, of course, when handling errors in "C-style").

Calling qemu_loadvm_state_main() inside itself is ok as well, but it
also has defect: stack usage would be out of control, or even, it can
be controled by malicious users. E.g., if someone used program to
periodically stop/start any network endpoint along the migration
network, QEMU may go into a paused -> recovery -> active -> paused ...
loop, and stack usage will just grow with time. I'd say it's an
extreme example though...

(Another way besides above two: maybe we can just return in
 qemu_loadvm_state_main with something like -EAGAIN, then the caller
 of qemu_loadvm_state_main can re-call it when necessary, though I
 would prefer "goto is okay here"... :)

-- 
Peter Xu