[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/5] migration: Make from_dst_file accesses thread-safe
From: |
Peter Xu |
Subject: |
Re: [PATCH v2 2/5] migration: Make from_dst_file accesses thread-safe |
Date: |
Thu, 22 Jul 2021 10:44:23 -0400 |
On Wed, Jul 21, 2021 at 04:15:27PM -0500, Eric Blake wrote:
> On Wed, Jul 21, 2021 at 03:34:06PM -0400, Peter Xu wrote:
> > Accessing from_dst_file is potentially racy in current code base like below:
> >
> > if (s->from_dst_file)
> > do_something(s->from_dst_file);
> >
> > Because from_dst_file can be reset right after the check in another
> > thread (rp_thread). One example is migrate_fd_cancel().
> >
> > Use the same qemu_file_lock to protect it too, just like to_dst_file.
> >
> > When it's safe to access without lock, comment it.
> >
> > There's one special reference in migration_thread() that can be replaced by
> > the newly introduced rp_thread_created flag.
> >
> > Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > migration/migration.c | 32 +++++++++++++++++++++++++-------
> > migration/migration.h | 8 +++++---
> > migration/ram.c | 1 +
> > 3 files changed, 31 insertions(+), 10 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 21b94f75a3..fa70400f98 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1879,10 +1879,12 @@ static void migrate_fd_cancel(MigrationState *s)
> > QEMUFile *f = migrate_get_current()->to_dst_file;
> > trace_migrate_fd_cancel();
> >
> > + qemu_mutex_lock(&s->qemu_file_lock);
> > if (s->rp_state.from_dst_file) {
> > /* shutdown the rp socket, so causing the rp thread to shutdown */
> > qemu_file_shutdown(s->rp_state.from_dst_file);
> > }
> > + qemu_mutex_unlock(&s->qemu_file_lock);
>
> Worth using WITH_QEMU_LOCK_GUARD?
Sure.
>
> > @@ -2827,11 +2845,13 @@ out:
> > * Maybe there is something we can do: it looks like a
> > * network down issue, and we pause for a recovery.
> > */
> > - qemu_fclose(rp);
> > - ms->rp_state.from_dst_file = NULL;
> > + migration_release_from_dst_file(ms);
> > rp = NULL;
> > if (postcopy_pause_return_path_thread(ms)) {
> > - /* Reload rp, reset the rest */
> > + /*
> > + * Reload rp, reset the rest. Referencing it is save since
>
> s/save/safe/
Will fix.
I'll wait for some more comments before I repost.
Thanks,
--
Peter Xu
- [PATCH v2 0/5] migrations: Fix potential rare race of migration-test after yank, Peter Xu, 2021/07/21
- [PATCH v2 1/5] migration: Fix missing join() of rp_thread, Peter Xu, 2021/07/21
- [PATCH v2 2/5] migration: Make from_dst_file accesses thread-safe, Peter Xu, 2021/07/21
- [PATCH v2 3/5] migration: Introduce migration_ioc_[un]register_yank(), Peter Xu, 2021/07/21
- [PATCH v2 4/5] migration: Teach QEMUFile to be QIOChannel-aware, Peter Xu, 2021/07/21
- [PATCH v2 5/5] migration: Move the yank unregister of channel_close out, Peter Xu, 2021/07/21