On Thu, Sep 21, 2017 at 08:29:03PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:
> > When there is IO error on the incoming channel (e.g., network down),
> > instead of bailing out immediately, we allow the dst vm to switch to the
> > new POSTCOPY_PAUSE state. Currently it is still simple - it waits the
> > new semaphore, until someone poke it for another attempt.
> >
> > Signed-off-by: Peter Xu <pet...@redhat.com>
> > ---
> > migration/migration.c | 1 +
> > migration/migration.h | 3 +++
> > migration/savevm.c | 60
> > ++++++++++++++++++++++++++++++++++++++++++++++++--
> > migration/trace-events | 2 ++
> > 4 files changed, 64 insertions(+), 2 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 8d26ea8..80de212 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -146,6 +146,7 @@ MigrationIncomingState
> > *migration_incoming_get_current(void)
> > memset(&mis_current, 0, sizeof(MigrationIncomingState));
> > qemu_mutex_init(&mis_current.rp_mutex);
> > qemu_event_init(&mis_current.main_thread_load_event, false);
> > + qemu_sem_init(&mis_current.postcopy_pause_sem_dst, 0);
> > once = true;
> > }
> > return &mis_current;
> > diff --git a/migration/migration.h b/migration/migration.h
> > index 0c957c9..c423682 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -60,6 +60,9 @@ struct MigrationIncomingState {
> > /* The coroutine we should enter (back) after failover */
> > Coroutine *migration_incoming_co;
> > QemuSemaphore colo_incoming_sem;
> > +
> > + /* notify PAUSED postcopy incoming migrations to try to continue */
> > + QemuSemaphore postcopy_pause_sem_dst;
> > };
> >
> > MigrationIncomingState *migration_incoming_get_current(void);
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 7172f14..3777124 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -1488,8 +1488,8 @@ static int
> > loadvm_postcopy_ram_handle_discard(MigrationIncomingState *mis,
> > */
> > static void *postcopy_ram_listen_thread(void *opaque)
> > {
> > - QEMUFile *f = opaque;
> > MigrationIncomingState *mis = migration_incoming_get_current();
> > + QEMUFile *f = mis->from_src_file;
> > int load_res;
> >
> > migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
> > @@ -1503,6 +1503,14 @@ static void *postcopy_ram_listen_thread(void *opaque)
> > */
> > qemu_file_set_blocking(f, true);
> > load_res = qemu_loadvm_state_main(f, mis);
> > +
> > + /*
> > + * This is tricky, but, mis->from_src_file can change after it
> > + * returns, when postcopy recovery happened. In the future, we may
> > + * want a wrapper for the QEMUFile handle.
> > + */
> > + f = mis->from_src_file;
> > +
> > /* And non-blocking again so we don't block in any cleanup */
> > qemu_file_set_blocking(f, false);
> >
> > @@ -1581,7 +1589,7 @@ static int
> > loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
> > /* Start up the listening thread and wait for it to signal ready */
> > qemu_sem_init(&mis->listen_thread_sem, 0);
> > qemu_thread_create(&mis->listen_thread, "postcopy/listen",
> > - postcopy_ram_listen_thread, mis->from_src_file,
> > + postcopy_ram_listen_thread, NULL,
> > QEMU_THREAD_DETACHED);
> > qemu_sem_wait(&mis->listen_thread_sem);
> > qemu_sem_destroy(&mis->listen_thread_sem);
> > @@ -1966,11 +1974,44 @@ void qemu_loadvm_state_cleanup(void)
> > }
> > }
> >
> > +/* 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);
>
> Should you not do the shutdown() before the lock?
> For example if the other thread is stuck, with rp_mutex
> held, trying to write to to_src_file, then you'll block
> waiting for the mutex. If you call shutdown and then take
> the lock, the other thread will error and release the lock.
The problem is that IMHO QEMUFile is not yet thread-safe itself. So
if we operate on it (even to shut it down) logically we need to have
the lock, right?
Then, IMHO the question would be: when will the send() be stuck in the
other thread?
Normally the only case I can think of is that source didn't recv()
fast enough, and we even consumed all the write buffer in dst side (I
don't really know how kernel manages the buffers though, and e.g. how
the size of buffer is defined...).
But when reach here, the channel (say, from_src_file and to_src_file,
since both of them are using the same channel behind the QEMUFile
interface) should already be broken in some way, then IIUC even there
is a send() in the other thread, it should return at some point with a
failure as well, just like how we reached here (possibly due to a
read() failure).
>
> I'm not quite sure what will happen if we end up calling this
> before the main thread has been returned from postcopy and the
> device loading is complete.
IIUC you mean the time starts from when we got MIG_CMD_PACKAGED until
main thread finishes handling that package?
Normally I think that should not matter much since during handling the
package it should hardly fail (we were reading from a buffer QIO
channel, no real IOs there)... But I agree about the reasoning. How
about one more patch to postpone the "active" to "postcopy-active"
state change after the package is handled correctly? Like:
--------------
diff --git a/migration/savevm.c b/migration/savevm.c
index b5c3214034..8317b2a7e2 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1573,8 +1573,6 @@ static void *postcopy_ram_listen_thread(void *opaque)
QEMUFile *f = mis->from_src_file;
int load_res;
- migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
- MIGRATION_STATUS_POSTCOPY_ACTIVE);
qemu_sem_post(&mis->listen_thread_sem);
trace_postcopy_ram_listen_thread_start();
@@ -1817,6 +1815,9 @@ static int
loadvm_handle_cmd_packaged(MigrationIncomingState *mis)
qemu_fclose(packf);
object_unref(OBJECT(bioc));
+ migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
+ MIGRATION_STATUS_POSTCOPY_ACTIVE);
+
return ret;
}
--------------
This function will only be called with "postcopy-active" state.
>
> Also, at this point have we guaranteed no one else is about
> to do an op on mis->to_src_file and will seg?
I think no? Since IMHO the main thread is playing with the buffer QIO
channel, rather than the real one?
(btw, could I ask what's "seg"? :)
--
Peter Xu
