diff mbox series

[v4,08/10] migration: process_incoming_migration_co(): move colo part to colo

Message ID 20230428194928.1426370-9-vsementsov@yandex-team.ru (mailing list archive)
State New, archived
Headers show
Series COLO: improve build options | expand

Commit Message

Vladimir Sementsov-Ogievskiy April 28, 2023, 7:49 p.m. UTC 
Let's make better public interface for COLO: instead of
colo_process_incoming_thread and not trivial logic around creating the
thread let's make simple colo_incoming_co(), hiding implementation from
generic code.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 include/migration/colo.h |  9 ++++++++-
 migration/colo.c         | 39 ++++++++++++++++++++++++++++++++++++++-
 migration/migration.c    | 28 ++--------------------------
 stubs/colo.c             |  6 ++----
 4 files changed, 50 insertions(+), 32 deletions(-)

Comments

Peter Xu May 2, 2023, 8:54 p.m. UTC | #1 
On Fri, Apr 28, 2023 at 10:49:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> +int coroutine_fn colo_incoming_co(void)
> +{
> +    MigrationIncomingState *mis = migration_incoming_get_current();
> +    Error *local_err = NULL;
> +    QemuThread th;
> +
> +    assert(!qemu_mutex_iothread_locked());

Is this assert reverted?  I assume it wants to guarantee BQL held rather
than not..

> +
> +    if (!migration_incoming_colo_enabled()) {
> +        return 0;
> +    }
> +
> +    /* Make sure all file formats throw away their mutable metadata */
> +    bdrv_activate_all(&local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +        return -EINVAL;
> +    }
> +
> +    qemu_thread_create(&th, "COLO incoming", colo_process_incoming_thread,
> +                       mis, QEMU_THREAD_JOINABLE);
> +
> +    mis->colo_incoming_co = qemu_coroutine_self();
> +    qemu_coroutine_yield();
> +    mis->colo_incoming_co = NULL;
> +
> +    qemu_mutex_unlock_iothread();
> +    /* Wait checkpoint incoming thread exit before free resource */
> +    qemu_thread_join(&th);
> +    qemu_mutex_lock_iothread();
> +
> +    /* We hold the global iothread lock, so it is safe here */
> +    colo_release_ram_cache();
> +
> +    return 0;
> +}
Vladimir Sementsov-Ogievskiy May 3, 2023, 9:15 a.m. UTC | #2 
On 02.05.23 23:54, Peter Xu wrote:
> On Fri, Apr 28, 2023 at 10:49:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> +int coroutine_fn colo_incoming_co(void)
>> +{
>> +    MigrationIncomingState *mis = migration_incoming_get_current();
>> +    Error *local_err = NULL;
>> +    QemuThread th;
>> +
>> +    assert(!qemu_mutex_iothread_locked());
> Is this assert reverted?  I assume it wants to guarantee BQL held rather
> than not..
> 

Oops, right.
diff mbox series

Patch

diff --git a/include/migration/colo.h b/include/migration/colo.h
index 7ef315473e..eaac07f26d 100644
--- a/include/migration/colo.h
+++ b/include/migration/colo.h
@@ -28,7 +28,6 @@  bool migration_in_colo_state(void);
 int migration_incoming_enable_colo(void);
 void migration_incoming_disable_colo(void);
 bool migration_incoming_colo_enabled(void);
-void *colo_process_incoming_thread(void *opaque);
 bool migration_incoming_in_colo_state(void);
 
 COLOMode get_colo_mode(void);
@@ -44,5 +43,13 @@  void colo_do_failover(void);
  */
 void colo_checkpoint_delay_set(void);
 
+/*
+ * Starts COLO incoming process. Called from process_incoming_migration_co()
+ * after loading the state.
+ *
+ * Called with BQL locked, may temporary release BQL.
+ */
+int coroutine_fn colo_incoming_co(void);
+
 void colo_shutdown(void);
 #endif
diff --git a/migration/colo.c b/migration/colo.c
index a688ac553a..d775b442d8 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -817,7 +817,7 @@  void colo_shutdown(void)
     }
 }
 
-void *colo_process_incoming_thread(void *opaque)
+static void *colo_process_incoming_thread(void *opaque)
 {
     MigrationIncomingState *mis = opaque;
     QEMUFile *fb = NULL;
@@ -918,3 +918,40 @@  out:
     rcu_unregister_thread();
     return NULL;
 }
+
+int coroutine_fn colo_incoming_co(void)
+{
+    MigrationIncomingState *mis = migration_incoming_get_current();
+    Error *local_err = NULL;
+    QemuThread th;
+
+    assert(!qemu_mutex_iothread_locked());
+
+    if (!migration_incoming_colo_enabled()) {
+        return 0;
+    }
+
+    /* Make sure all file formats throw away their mutable metadata */
+    bdrv_activate_all(&local_err);
+    if (local_err) {
+        error_report_err(local_err);
+        return -EINVAL;
+    }
+
+    qemu_thread_create(&th, "COLO incoming", colo_process_incoming_thread,
+                       mis, QEMU_THREAD_JOINABLE);
+
+    mis->colo_incoming_co = qemu_coroutine_self();
+    qemu_coroutine_yield();
+    mis->colo_incoming_co = NULL;
+
+    qemu_mutex_unlock_iothread();
+    /* Wait checkpoint incoming thread exit before free resource */
+    qemu_thread_join(&th);
+    qemu_mutex_lock_iothread();
+
+    /* We hold the global iothread lock, so it is safe here */
+    colo_release_ram_cache();
+
+    return 0;
+}
diff --git a/migration/migration.c b/migration/migration.c
index 23b2d187de..0d912ee0d7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -502,7 +502,6 @@  process_incoming_migration_co(void *opaque)
     MigrationIncomingState *mis = migration_incoming_get_current();
     PostcopyState ps;
     int ret;
-    Error *local_err = NULL;
 
     assert(mis->from_src_file);
     mis->largest_page_size = qemu_ram_pagesize_largest();
@@ -540,37 +539,14 @@  process_incoming_migration_co(void *opaque)
         goto fail;
     }
 
-    /* we get COLO info, and know if we are in COLO mode */
-    if (migration_incoming_colo_enabled()) {
-        QemuThread colo_incoming_thread;
-
-        /* Make sure all file formats throw away their mutable metadata */
-        bdrv_activate_all(&local_err);
-        if (local_err) {
-            error_report_err(local_err);
-            goto fail;
-        }
-
-        qemu_thread_create(&colo_incoming_thread, "COLO incoming",
-             colo_process_incoming_thread, mis, QEMU_THREAD_JOINABLE);
-
-        mis->colo_incoming_co = qemu_coroutine_self();
-        qemu_coroutine_yield();
-        mis->colo_incoming_co = NULL;
-
-        qemu_mutex_unlock_iothread();
-        /* Wait checkpoint incoming thread exit before free resource */
-        qemu_thread_join(&colo_incoming_thread);
-        qemu_mutex_lock_iothread();
-        /* We hold the global iothread lock, so it is safe here */
-        colo_release_ram_cache();
+    if (colo_incoming_co() < 0) {
+        goto fail;
     }
 
     mis->bh = qemu_bh_new(process_incoming_migration_bh, mis);
     qemu_bh_schedule(mis->bh);
     return;
 fail:
-    local_err = NULL;
     migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
                       MIGRATION_STATUS_FAILED);
     qemu_fclose(mis->from_src_file);
diff --git a/stubs/colo.c b/stubs/colo.c
index cf9816d368..f33379d0fd 100644
--- a/stubs/colo.c
+++ b/stubs/colo.c
@@ -10,11 +10,9 @@  void colo_shutdown(void)
 {
 }
 
-void *colo_process_incoming_thread(void *opaque)
+int coroutine_fn colo_incoming_co(void)
 {
-    error_report("Impossible happend: trying to start COLO thread when COLO "
-                 "module is not built in");
-    abort();
+    return 0;
 }
 
 void colo_checkpoint_delay_set(void)