| Message ID | 20180515234017.2277-17-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/40] migration: fix saving normal page even if it's been compressed | expand |
On 16 May 2018 at 00:39, Juan Quintela <quintela@redhat.com> wrote: > From: Peter Xu <peterx@redhat.com> > > 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. > > One note is that here on ram loading thread we cannot detect the > POSTCOPY_ACTIVE state, but we need to detect the more specific > POSTCOPY_INCOMING_RUNNING state, to make sure we have already loaded all > the device states. > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > Message-Id: <20180502104740.12123-5-peterx@redhat.com> > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- Hi; Coverity (CID 1391289) points out what it thinks is an issue in this commit. I think it's wrong, but it does leave me uncertain whether we have the locking correct here... > +/* 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; In postcopy_pause_incoming() we always set mis->from_src_file to NULL... > + > + assert(mis->to_src_file); > + qemu_file_shutdown(mis->to_src_file); > + qemu_mutex_lock(&mis->rp_mutex); > + qemu_fclose(mis->to_src_file); > + mis->to_src_file = NULL; > + qemu_mutex_unlock(&mis->rp_mutex); > + > + error_report("Detected IO failure for postcopy. " > + "Migration paused."); > + > + 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); > > @@ -2104,6 +2145,24 @@ 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 (after receiving all device data, which > + * must be in POSTCOPY_INCOMING_RUNNING state. Note that > + * POSTCOPY_INCOMING_LISTENING is still not enough, it's > + * still receiving device states). > + * 2. network failure (-EIO) > + * > + * If so, we try to wait for a recovery. > + */ > + if (postcopy_state_get() == POSTCOPY_INCOMING_RUNNING && > + ret == -EIO && postcopy_pause_incoming(mis)) { > + /* Reset f to point to the newly created channel */ > + f = mis->from_src_file; ...but here we set f to mis->from_src_file, which Coverity thinks must be NULL... > + goto retry; ...and then we goto the 'retry' label, which will always dereference the NULL pointer and crash in qemu_get_byte(). > + } > } > return ret; > } Looking at the wider code, I think what Coverity has not spotted is that postcopy_pause_incoming() blocks on the semaphore, and the code that wakes it up in migration_fd_process_incoming() will set mis->from_src_file to something non-NULL before it posts that semaphore. However, I'm not sure about the locking being used here. There's no lock held while postcopy_pause_incoming() does the "set state to paused and then clear mis->from_src_file", so what prevents this interleaving of execution of the two threads? postcopy_pause_incoming() migration_fd_process_incoming() + set state to PAUSED + find that state is PAUSED + mis->from_src_file = f + mis->from_src_file = NULL + wait on semaphore + post semaphare ? There are also a couple of other things Coverity thinks might be data race conditions (CID 1391295 and CID 1391288) that you might want to look at, though I suspect they are false-positives (access occurs before thread create of the thread the mutex is providing protection against). thanks --- PMM
On Mon, Jun 04, 2018 at 02:49:58PM +0100, Peter Maydell wrote: > On 16 May 2018 at 00:39, Juan Quintela <quintela@redhat.com> wrote: > > From: Peter Xu <peterx@redhat.com> > > > > 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. > > > > One note is that here on ram loading thread we cannot detect the > > POSTCOPY_ACTIVE state, but we need to detect the more specific > > POSTCOPY_INCOMING_RUNNING state, to make sure we have already loaded all > > the device states. > > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > Signed-off-by: Peter Xu <peterx@redhat.com> > > Message-Id: <20180502104740.12123-5-peterx@redhat.com> > > Signed-off-by: Juan Quintela <quintela@redhat.com> > > --- > > Hi; Coverity (CID 1391289) points out what it thinks is an issue in > this commit. I think it's wrong, but it does leave me uncertain > whether we have the locking correct here... Hi, Peter, Yeah actually this confused me a bit too when I wanted to fix this... > > > +/* 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; > > In postcopy_pause_incoming() we always set mis->from_src_file to NULL... > > > + > > + assert(mis->to_src_file); > > + qemu_file_shutdown(mis->to_src_file); > > + qemu_mutex_lock(&mis->rp_mutex); > > + qemu_fclose(mis->to_src_file); > > + mis->to_src_file = NULL; > > + qemu_mutex_unlock(&mis->rp_mutex); > > + > > + error_report("Detected IO failure for postcopy. " > > + "Migration paused."); > > + > > + 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); > > > > @@ -2104,6 +2145,24 @@ 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 (after receiving all device data, which > > + * must be in POSTCOPY_INCOMING_RUNNING state. Note that > > + * POSTCOPY_INCOMING_LISTENING is still not enough, it's > > + * still receiving device states). > > + * 2. network failure (-EIO) > > + * > > + * If so, we try to wait for a recovery. > > + */ > > + if (postcopy_state_get() == POSTCOPY_INCOMING_RUNNING && > > + ret == -EIO && postcopy_pause_incoming(mis)) { > > + /* Reset f to point to the newly created channel */ > > + f = mis->from_src_file; > > ...but here we set f to mis->from_src_file, which Coverity > thinks must be NULL... > > > + goto retry; > > ...and then we goto the 'retry' label, which will always > dereference the NULL pointer and crash in qemu_get_byte(). > > > + } > > } > > return ret; > > } > > Looking at the wider code, I think what Coverity has not spotted is > that postcopy_pause_incoming() blocks on the semaphore, and the > code that wakes it up in migration_fd_process_incoming() will > set mis->from_src_file to something non-NULL before it posts that > semaphore. > > However, I'm not sure about the locking being used here. There's > no lock held while postcopy_pause_incoming() does the "set state > to paused and then clear mis->from_src_file", so what prevents > this interleaving of execution of the two threads? > > postcopy_pause_incoming() migration_fd_process_incoming() > + set state to PAUSED > + find that state is PAUSED > + mis->from_src_file = f > + mis->from_src_file = NULL > + wait on semaphore > + post semaphare > > ? Thanks for raising this question up. I suspect here I should postpone the status update in postcopy_pause_incoming() to be after clearing the from_src_file var. Then we'll make sure the migration_fd_process_incoming() will always update the correct new file handle after it was cleared in the other thread. > > There are also a couple of other things Coverity thinks might > be data race conditions (CID 1391295 and CID 1391288) that you > might want to look at, though I suspect they are false-positives > (access occurs before thread create of the thread the mutex > is providing protection against). Could you kindly let me know if there is any way for me to lookup these CIDs? E.g., I searched "CID 1391295 QEMU Coverity" on Google but I can't find any link. Any preferred way to do this? (I think a stupid way is to lookup the CID in every Coverity reports, but I guess there is a faster way) Thanks,
On Tue, Jun 05, 2018 at 03:48:09PM +0800, Peter Xu wrote: [...] > > There are also a couple of other things Coverity thinks might > > be data race conditions (CID 1391295 and CID 1391288) that you > > might want to look at, though I suspect they are false-positives > > (access occurs before thread create of the thread the mutex > > is providing protection against). > > Could you kindly let me know if there is any way for me to lookup > these CIDs? E.g., I searched "CID 1391295 QEMU Coverity" on Google > but I can't find any link. Any preferred way to do this? > > (I think a stupid way is to lookup the CID in every Coverity reports, > but I guess there is a faster way) Dave helped pointed out that there was a CID entry on the top-right corner that I can fill in... I'll have a look on both defects. Thanks,
diff --git a/migration/migration.c b/migration/migration.c index 8392cf467d..4a7c02fd3c 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -159,6 +159,7 @@ MigrationIncomingState *migration_incoming_get_current(void) sizeof(struct PostCopyFD)); 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); init_dirty_bitmap_incoming_migration(); diff --git a/migration/migration.h b/migration/migration.h index 60283c39b2..0ccdcb8ffe 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -73,6 +73,9 @@ struct MigrationIncomingState { * live migration, to calculate vCPU block time * */ struct PostcopyBlocktimeContext *blocktime_ctx; + + /* 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 e2be02afe4..8ad99b1eaa 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1564,8 +1564,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, @@ -1579,6 +1579,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); @@ -1668,7 +1676,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); @@ -2055,11 +2063,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_file_shutdown(mis->to_src_file); + qemu_mutex_lock(&mis->rp_mutex); + qemu_fclose(mis->to_src_file); + mis->to_src_file = NULL; + qemu_mutex_unlock(&mis->rp_mutex); + + error_report("Detected IO failure for postcopy. " + "Migration paused."); + + 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); @@ -2104,6 +2145,24 @@ 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 (after receiving all device data, which + * must be in POSTCOPY_INCOMING_RUNNING state. Note that + * POSTCOPY_INCOMING_LISTENING is still not enough, it's + * still receiving device states). + * 2. network failure (-EIO) + * + * If so, we try to wait for a recovery. + */ + if (postcopy_state_get() == POSTCOPY_INCOMING_RUNNING && + ret == -EIO && postcopy_pause_incoming(mis)) { + /* Reset f to point to the newly created channel */ + f = mis->from_src_file; + goto retry; + } } return ret; } diff --git a/migration/trace-events b/migration/trace-events index 409b4b8be3..e23ec019be 100644 --- a/migration/trace-events +++ b/migration/trace-events @@ -100,6 +100,8 @@ open_return_path_on_source(void) "" open_return_path_on_source_continue(void) "" postcopy_start(void) "" postcopy_pause_continued(void) "" +postcopy_pause_incoming(void) "" +postcopy_pause_incoming_continued(void) "" postcopy_start_set_run(void) "" source_return_path_thread_bad_end(void) "" source_return_path_thread_end(void) ""