[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 22/24] migration: introduce lock for to_dst_f
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH v8 22/24] migration: introduce lock for to_dst_file |
Date: |
Tue, 8 May 2018 12:31:49 +0100 |
User-agent: |
Mutt/1.9.5 (2018-04-13) |
* Peter Xu (address@hidden) 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 <address@hidden>
Reviewed-by: Dr. David Alan Gilbert <address@hidden>
> ---
> 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 / address@hidden / Manchester, UK
- [Qemu-devel] [PATCH v8 14/24] migration: new message MIG_RP_MSG_RESUME_ACK, (continued)
- [Qemu-devel] [PATCH v8 14/24] migration: new message MIG_RP_MSG_RESUME_ACK, Peter Xu, 2018/05/02
- [Qemu-devel] [PATCH v8 15/24] migration: introduce SaveVMHandlers.resume_prepare, Peter Xu, 2018/05/02
- [Qemu-devel] [PATCH v8 16/24] migration: synchronize dirty bitmap for resume, Peter Xu, 2018/05/02
- [Qemu-devel] [PATCH v8 17/24] migration: setup ramstate for resume, Peter Xu, 2018/05/02
- [Qemu-devel] [PATCH v8 18/24] migration: final handshake for the resume, Peter Xu, 2018/05/02
- [Qemu-devel] [PATCH v8 19/24] migration: init dst in migration_object_init too, Peter Xu, 2018/05/02
- [Qemu-devel] [PATCH v8 20/24] qmp/migration: new command migrate-recover, Peter Xu, 2018/05/02
- [Qemu-devel] [PATCH v8 21/24] hmp/migration: add migrate_recover command, Peter Xu, 2018/05/02
- [Qemu-devel] [PATCH v8 22/24] migration: introduce lock for to_dst_file, Peter Xu, 2018/05/02
- Re: [Qemu-devel] [PATCH v8 22/24] migration: introduce lock for to_dst_file,
Dr. David Alan Gilbert <=
- [Qemu-devel] [PATCH v8 23/24] migration/qmp: add command migrate-pause, Peter Xu, 2018/05/02
- [Qemu-devel] [PATCH v8 24/24] migration/hmp: add migrate_pause command, Peter Xu, 2018/05/02