[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PULL 3/7] migration: Make from_dst_file accesses thread-safe
From: |
Dr. David Alan Gilbert (git) |
Subject: |
[PULL 3/7] migration: Make from_dst_file accesses thread-safe |
Date: |
Mon, 26 Jul 2021 13:43:27 +0100 |
From: Peter Xu <peterx@redhat.com>
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>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Lukas Straub <lukasstraub2@web.de>
Message-Id: <20210722175841.938739-3-peterx@redhat.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
with Peter's fixup
---
migration/migration.c | 39 +++++++++++++++++++++++++++++----------
migration/migration.h | 8 +++++---
migration/ram.c | 1 +
3 files changed, 35 insertions(+), 13 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 21b94f75a3..62dbcb7d52 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1879,9 +1879,11 @@ static void migrate_fd_cancel(MigrationState *s)
QEMUFile *f = migrate_get_current()->to_dst_file;
trace_migrate_fd_cancel();
- 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);
+ WITH_QEMU_LOCK_GUARD(&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);
+ }
}
do {
@@ -2686,6 +2688,23 @@ static int migrate_handle_rp_resume_ack(MigrationState
*s, uint32_t value)
return 0;
}
+/* Release ms->rp_state.from_dst_file in a safe way */
+static void migration_release_from_dst_file(MigrationState *ms)
+{
+ QEMUFile *file;
+
+ WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
+ /*
+ * Reset the from_dst_file pointer first before releasing it, as we
+ * can't block within lock section
+ */
+ file = ms->rp_state.from_dst_file;
+ ms->rp_state.from_dst_file = NULL;
+ }
+
+ qemu_fclose(file);
+}
+
/*
* Handles messages sent on the return path towards the source VM
*
@@ -2827,11 +2846,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 safe since
+ * it's reset only by us above, or when migration completes
+ */
rp = ms->rp_state.from_dst_file;
ms->rp_state.error = false;
goto retry;
@@ -2843,8 +2864,7 @@ out:
}
trace_source_return_path_thread_end();
- ms->rp_state.from_dst_file = NULL;
- qemu_fclose(rp);
+ migration_release_from_dst_file(ms);
rcu_unregister_thread();
return NULL;
}
@@ -2852,7 +2872,6 @@ out:
static int open_return_path_on_source(MigrationState *ms,
bool create_thread)
{
-
ms->rp_state.from_dst_file = qemu_file_get_return_path(ms->to_dst_file);
if (!ms->rp_state.from_dst_file) {
return -1;
@@ -3746,7 +3765,7 @@ static void *migration_thread(void *opaque)
* If we opened the return path, we need to make sure dst has it
* opened as well.
*/
- if (s->rp_state.from_dst_file) {
+ if (s->rp_state.rp_thread_created) {
/* Now tell the dest that it should open its end so it can reply */
qemu_savevm_send_open_return_path(s->to_dst_file);
diff --git a/migration/migration.h b/migration/migration.h
index c302879fad..7a5aa8c2fd 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -154,12 +154,13 @@ struct MigrationState {
QemuThread thread;
QEMUBH *vm_start_bh;
QEMUBH *cleanup_bh;
+ /* Protected by qemu_file_lock */
QEMUFile *to_dst_file;
QIOChannelBuffer *bioc;
/*
- * 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.
+ * Protects to_dst_file/from_dst_file pointers. 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;
@@ -192,6 +193,7 @@ struct MigrationState {
/* State related to return path */
struct {
+ /* Protected by qemu_file_lock */
QEMUFile *from_dst_file;
QemuThread rp_thread;
bool error;
diff --git a/migration/ram.c b/migration/ram.c
index b5fc454b2f..f728f5072f 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4012,6 +4012,7 @@ static void ram_dirty_bitmap_reload_notify(MigrationState
*s)
int ram_dirty_bitmap_reload(MigrationState *s, RAMBlock *block)
{
int ret = -EINVAL;
+ /* from_dst_file is always valid because we're within rp_thread */
QEMUFile *file = s->rp_state.from_dst_file;
unsigned long *le_bitmap, nbits = block->used_length >> TARGET_PAGE_BITS;
uint64_t local_size = DIV_ROUND_UP(nbits, 8);
--
2.31.1
- [PULL 0/7] migration queue, Dr. David Alan Gilbert (git), 2021/07/26
- [PULL 1/7] tests/qtest/migration-test.c: use 127.0.0.1 instead of 0, Dr. David Alan Gilbert (git), 2021/07/26
- [PULL 2/7] migration: Fix missing join() of rp_thread, Dr. David Alan Gilbert (git), 2021/07/26
- [PULL 3/7] migration: Make from_dst_file accesses thread-safe,
Dr. David Alan Gilbert (git) <=
- [PULL 4/7] migration: Introduce migration_ioc_[un]register_yank(), Dr. David Alan Gilbert (git), 2021/07/26
- [PULL 5/7] migration: Teach QEMUFile to be QIOChannel-aware, Dr. David Alan Gilbert (git), 2021/07/26
- [PULL 6/7] migration: Move the yank unregister of channel_close out, Dr. David Alan Gilbert (git), 2021/07/26
- [PULL 7/7] migration: clear the memory region dirty bitmap when skipping free pages, Dr. David Alan Gilbert (git), 2021/07/26
- Re: [PULL 0/7] migration queue, Peter Maydell, 2021/07/27