Re: [Qemu-devel] [PATCH v8 22/24] migration: introduce lock for to_dst_file

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Peter Xu (peterx@redhat.com) 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 <peterx@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  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 / dgilbert@redhat.com / Manchester, UK