* Peter Xu ([email protected]) wrote: > Let's introduce a lock for that QEMUFile since we are going to operate > on it in multiple threads. > > Signed-off-by: Peter Xu <[email protected]>
Reviewed-by: Dr. David Alan Gilbert <[email protected]> > --- > migration/migration.h | 6 ++++++ > migration/channel.c | 3 ++- > migration/migration.c | 22 +++++++++++++++++++--- > 3 files changed, 27 insertions(+), 4 deletions(-) > > diff --git a/migration/migration.h b/migration/migration.h > index 012bcd352b..f6b9e774f9 100644 > --- a/migration/migration.h > +++ b/migration/migration.h > @@ -114,6 +114,12 @@ struct MigrationState > QemuThread thread; > QEMUBH *cleanup_bh; > QEMUFile *to_dst_file; > + /* > + * Protects to_dst_file pointer. We need to make sure we won't > + * yield or hang during the critical section, since this lock will > + * be used in OOB command handler. > + */ > + QemuMutex qemu_file_lock; So what are the rules on access to_dst_file? You only seem to be taking the lock when closing or setting the to_dst_file. Which I think given the problem we were trying to fix is OK, but it needs to be commented to say why it's safe. Dave > /* bytes already send at the beggining of current interation */ > uint64_t iteration_initial_bytes; > diff --git a/migration/channel.c b/migration/channel.c > index c5eaf0fa0e..716192bf75 100644 > --- a/migration/channel.c > +++ b/migration/channel.c > @@ -74,8 +74,9 @@ void migration_channel_connect(MigrationState *s, > } else { > QEMUFile *f = qemu_fopen_channel_output(ioc); > > + qemu_mutex_lock(&s->qemu_file_lock); > s->to_dst_file = f; > - > + qemu_mutex_unlock(&s->qemu_file_lock); > } > } > migrate_fd_connect(s, error); > diff --git a/migration/migration.c b/migration/migration.c > index 03d1fc7bc3..25f26052d2 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -1229,6 +1229,7 @@ static void migrate_fd_cleanup(void *opaque) > > if (s->to_dst_file) { > Error *local_err = NULL; > + QEMUFile *tmp; > > trace_migrate_fd_cleanup(); > qemu_mutex_unlock_iothread(); > @@ -1241,8 +1242,15 @@ static void migrate_fd_cleanup(void *opaque) > if (multifd_save_cleanup(&local_err) != 0) { > error_report_err(local_err); > } > - qemu_fclose(s->to_dst_file); > + qemu_mutex_lock(&s->qemu_file_lock); > + tmp = s->to_dst_file; > s->to_dst_file = NULL; > + qemu_mutex_unlock(&s->qemu_file_lock); > + /* > + * Close the file handle without the lock to make sure the > + * critical section won't block for long. > + */ > + qemu_fclose(tmp); > } > > assert((s->state != MIGRATION_STATUS_ACTIVE) && > @@ -2526,14 +2534,20 @@ static MigThrError postcopy_pause(MigrationState *s) > assert(s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE); > > while (true) { > + QEMUFile *file; > + > migrate_set_state(&s->state, s->state, > MIGRATION_STATUS_POSTCOPY_PAUSED); > > /* Current channel is possibly broken. Release it. */ > assert(s->to_dst_file); > - qemu_file_shutdown(s->to_dst_file); > - qemu_fclose(s->to_dst_file); > + qemu_mutex_lock(&s->qemu_file_lock); > + file = s->to_dst_file; > s->to_dst_file = NULL; > + qemu_mutex_unlock(&s->qemu_file_lock); > + > + qemu_file_shutdown(file); > + qemu_fclose(file); > > error_report("Detected IO failure for postcopy. " > "Migration paused."); > @@ -3002,6 +3016,7 @@ static void migration_instance_finalize(Object *obj) > MigrationParameters *params = &ms->parameters; > > qemu_mutex_destroy(&ms->error_mutex); > + qemu_mutex_destroy(&ms->qemu_file_lock); > g_free(params->tls_hostname); > g_free(params->tls_creds); > qemu_sem_destroy(&ms->pause_sem); > @@ -3041,6 +3056,7 @@ static void migration_instance_init(Object *obj) > qemu_sem_init(&ms->postcopy_pause_sem, 0); > qemu_sem_init(&ms->postcopy_pause_rp_sem, 0); > qemu_sem_init(&ms->rp_state.rp_sem, 0); > + qemu_mutex_init(&ms->qemu_file_lock); > } > > /* > -- > 2.14.3 > -- Dr. David Alan Gilbert / [email protected] / Manchester, UK
