[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH v3 10/10] migration: Add a wrapper to cleanup migration files
From: |
Fabiano Rosas |
Subject: |
[PATCH v3 10/10] migration: Add a wrapper to cleanup migration files |
Date: |
Fri, 11 Aug 2023 12:08:36 -0300 |
We currently have a pattern for cleaning up a migration QEMUFile:
qemu_mutex_lock(&s->qemu_file_lock);
file = s->file_name;
s->file_name = NULL;
qemu_mutex_unlock(&s->qemu_file_lock);
migration_ioc_unregister_yank_from_file(file);
qemu_file_shutdown(file);
qemu_fclose(file);
There are some considerations for this sequence:
- we must clear the pointer under the lock, to avoid TOC/TOU bugs;
- the shutdown() and close() expect be given a non-null parameter;
- a close() in one thread should not race with a shutdown() in another;
Create a wrapper function to make sure everything works correctly.
Note: the return path did not used to call
migration_ioc_unregister_yank_from_file(), but I added it
nonetheless for uniformity.
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
migration/migration.c | 92 ++++++++++++-------------------------------
util/yank.c | 2 -
2 files changed, 26 insertions(+), 68 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 4df5ca25c1..3c33e4fae4 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -217,6 +217,27 @@ MigrationIncomingState
*migration_incoming_get_current(void)
return current_incoming;
}
+static void migration_file_release(QEMUFile **file)
+{
+ MigrationState *ms = migrate_get_current();
+ QEMUFile *tmp;
+
+ /*
+ * Reset the pointer before releasing it to avoid holding the lock
+ * for too long.
+ */
+ WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
+ tmp = *file;
+ *file = NULL;
+ }
+
+ if (tmp) {
+ migration_ioc_unregister_yank_from_file(tmp);
+ qemu_file_shutdown(tmp);
+ qemu_fclose(tmp);
+ }
+}
+
void migration_incoming_transport_cleanup(MigrationIncomingState *mis)
{
if (mis->socket_address_list) {
@@ -1155,8 +1176,6 @@ static void migrate_fd_cleanup(MigrationState *s)
qemu_savevm_state_cleanup();
if (s->to_dst_file) {
- QEMUFile *tmp;
-
trace_migrate_fd_cleanup();
qemu_mutex_unlock_iothread();
if (s->migration_thread_running) {
@@ -1166,17 +1185,7 @@ static void migrate_fd_cleanup(MigrationState *s)
qemu_mutex_lock_iothread();
multifd_save_cleanup();
- 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.
- */
- migration_ioc_unregister_yank_from_file(tmp);
- qemu_file_shutdown(tmp);
- qemu_fclose(tmp);
+ migration_file_release(&s->to_dst_file);
}
/*
@@ -1816,39 +1825,6 @@ static int migrate_handle_rp_resume_ack(MigrationState
*s, uint32_t value)
return 0;
}
-/*
- * Release ms->rp_state.from_dst_file (and postcopy_qemufile_src if
- * existed) in a safe way.
- */
-static void migration_release_dst_files(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;
- }
-
- /*
- * Do the same to postcopy fast path socket too if there is. No
- * locking needed because this qemufile should only be managed by
- * return path thread.
- */
- if (ms->postcopy_qemufile_src) {
- migration_ioc_unregister_yank_from_file(ms->postcopy_qemufile_src);
- qemu_file_shutdown(ms->postcopy_qemufile_src);
- qemu_fclose(ms->postcopy_qemufile_src);
- ms->postcopy_qemufile_src = NULL;
- }
-
- qemu_file_shutdown(file);
- qemu_fclose(file);
-}
-
/*
* Handles messages sent on the return path towards the source VM
*
@@ -2048,7 +2024,8 @@ static int
await_return_path_close_on_source(MigrationState *ms)
ret = ms->rp_state.error;
ms->rp_state.error = false;
- migration_release_dst_files(ms);
+ migration_file_release(&ms->rp_state.from_dst_file);
+ migration_file_release(&ms->postcopy_qemufile_src);
trace_migration_return_path_end_after(ret);
return ret;
@@ -2504,26 +2481,9 @@ static MigThrError postcopy_pause(MigrationState *s)
assert(s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE);
while (true) {
- QEMUFile *file;
-
- /*
- * Current channel is possibly broken. Release it. Note that this is
- * guaranteed even without lock because to_dst_file should only be
- * modified by the migration thread. That also guarantees that the
- * unregister of yank is safe too without the lock. It should be safe
- * even to be within the qemu_file_lock, but we didn't do that to avoid
- * taking more mutex (yank_lock) within qemu_file_lock. TL;DR: we make
- * the qemu_file_lock critical section as small as possible.
- */
+ /* Current channel is possibly broken. Release it. */
assert(s->to_dst_file);
- migration_ioc_unregister_yank_from_file(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);
+ migration_file_release(&s->to_dst_file);
/*
* We're already pausing, so ignore any errors on the return
diff --git a/util/yank.c b/util/yank.c
index abf47c346d..4b6afbf589 100644
--- a/util/yank.c
+++ b/util/yank.c
@@ -146,8 +146,6 @@ void yank_unregister_function(const YankInstance *instance,
return;
}
}
-
- abort();
}
void qmp_yank(YankInstanceList *instances,
--
2.35.3
- [PATCH v3 03/10] migration: Fix possible race when checking to_dst_file for errors, (continued)
- [PATCH v3 10/10] migration: Add a wrapper to cleanup migration files,
Fabiano Rosas <=