[PATCH 0/5] migrations: Fix potential rare race of migration-test after yank

[PATCH 0/5] migrations: Fix potential rare race of migration-test after yank

[Peter Xu]

Patch 1 fixes a possible race that migration thread can accidentally skip
join() of rp_thread even if the return thread is enabled.  Patch 1 is suspected
to also be the root cause of the recent hard-to-reproduce migration-test
failure here reported by PMM:

https://lore.kernel.org/qemu-devel/YPamXAHwan%2FPPXLf@work-vm/

I didn't reproduce it myself; but after co-debugged with Dave it's suspected
that the race of rp_thread could be the cause.  It's not exposed before because
yank is soo strict on releasing instances, while we're not that strict before,
and didn't join() on rp_thread wasn't so dangerous after all when migration
succeeded before.

Patch 2 fixes another theoretical race on accessing from_dst_file spotted by
Dave.  I don't think there's known issues with it, but may still worth fixing.

Patch 3 should be a cleanup on yank that I think would be nice to have.

Patch 4-5 are further cleanups to remove the ref==1 check in channel_close(),
finally, as I always thought that's a bit hackish.  So I used explicit
unregister of the yank function at necessary places to replace that ref==1 one.

I still think having patch 3-5 altogether would be great, however I think patch
1 should still be the most important to be reviewed.  Also it would be great to
know whether patch 1 could fix the known yank crash.

Please review, thanks.

Peter Xu (5):
  migration: Fix missing join() of rp_thread
  migration: Shutdown src in await_return_path_close_on_source()
  migration: Introduce migration_ioc_[un]register_yank()
  migration: Teach QEMUFile to be QIOChannel-aware
  migration: Move the yank unregister of channel_close out

 migration/channel.c           | 15 ++-----------
 migration/migration.c         | 18 +++++++++++----
 migration/migration.h         |  7 ++++++
 migration/multifd.c           |  8 ++-----
 migration/qemu-file-channel.c | 11 ++-------
 migration/qemu-file.c         | 17 +++++++++++++-
 migration/qemu-file.h         |  4 +++-
 migration/ram.c               |  2 +-
 migration/savevm.c            | 11 +++++++--
 migration/yank_functions.c    | 42 +++++++++++++++++++++++++++++++++++
 migration/yank_functions.h    |  3 +++
 11 files changed, 101 insertions(+), 37 deletions(-)

-- 
2.31.1

[PATCH 1/5] migration: Fix missing join() of rp_thread

[Peter Xu]

It's possible that the migration thread skip the join() of the rp_thread in
below race and crash on src right at finishing migration:

       migration_thread                     rp_thread
       ----------------                     ---------
    migration_completion()
                                        (before rp_thread quits)
                                        from_dst_file=NULL
                                        [thread got scheduled out]
      s->rp_state.from_dst_file==NULL
        (skip join() of rp_thread)
    migrate_fd_cleanup()
      qemu_fclose(s->to_dst_file)
      yank_unregister_instance()
        assert(yank_find_entry())  <------- crash

It could mostly happen with postcopy, but that shouldn't be required, e.g., I
think it could also trigger with MIGRATION_CAPABILITY_RETURN_PATH set.

It's suspected that above race could be the root cause of a recent (but rare)
migration-test break reported by either Dave or PMM:

https://lore.kernel.org/qemu-devel/YPamXAHwan%2FPPXLf@work-vm/

The issue is: from_dst_file is reset in the rp_thread, so if the thread reset
it to NULL fast enough then the migration thread will assume there's no
rp_thread at all.

This could potentially cause more severe issue (e.g. crash) after the yank code.

Fix it by using a boolean to keep "whether we've created rp_thread".

Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 4 +++-
 migration/migration.h | 7 +++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 2d306582eb..21b94f75a3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2867,6 +2867,7 @@ static int open_return_path_on_source(MigrationState *ms,
 
     qemu_thread_create(&ms->rp_state.rp_thread, "return path",
                        source_return_path_thread, ms, QEMU_THREAD_JOINABLE);
+    ms->rp_state.rp_thread_created = true;
 
     trace_open_return_path_on_source_continue();
 
@@ -2891,6 +2892,7 @@ static int await_return_path_close_on_source(MigrationState *ms)
     }
     trace_await_return_path_close_on_source_joining();
     qemu_thread_join(&ms->rp_state.rp_thread);
+    ms->rp_state.rp_thread_created = false;
     trace_await_return_path_close_on_source_close();
     return ms->rp_state.error;
 }
@@ -3170,7 +3172,7 @@ static void migration_completion(MigrationState *s)
      * it will wait for the destination to send it's status in
      * a SHUT command).
      */
-    if (s->rp_state.from_dst_file) {
+    if (s->rp_state.rp_thread_created) {
         int rp_error;
         trace_migration_return_path_end_before();
         rp_error = await_return_path_close_on_source(s);
diff --git a/migration/migration.h b/migration/migration.h
index 2ebb740dfa..c302879fad 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -195,6 +195,13 @@ struct MigrationState {
         QEMUFile     *from_dst_file;
         QemuThread    rp_thread;
         bool          error;
+        /*
+         * We can also check non-zero of rp_thread, but there's no "official"
+         * way to do this, so this bool makes it slightly more elegant.
+         * Checking from_dst_file for this is racy because from_dst_file will
+         * be cleared in the rp_thread!
+         */
+        bool          rp_thread_created;
         QemuSemaphore rp_sem;
     } rp_state;
 
-- 
2.31.1

[PATCH 2/5] migration: Shutdown src in await_return_path_close_on_source()

[Peter Xu]

We have a logic in await_return_path_close_on_source() that we will explicitly
shutdown the socket when migration encounters errors.  However it could be racy
because from_dst_file could have been reset right after checking it but before
passing it to qemu_file_shutdown() by the rp_thread.

Fix it by shutdown() on the src file instead.  Since they must be a pair of
qemu files, shutdown on either of them will work the same.

Since at it, drop the check for from_dst_file directly, which makes the
behavior even more predictable.

Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 21b94f75a3..4f48cde796 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2882,12 +2882,15 @@ static int await_return_path_close_on_source(MigrationState *ms)
      * rp_thread will exit, however if there's an error we need to cause
      * it to exit.
      */
-    if (qemu_file_get_error(ms->to_dst_file) && ms->rp_state.from_dst_file) {
+    if (qemu_file_get_error(ms->to_dst_file)) {
         /*
          * shutdown(2), if we have it, will cause it to unblock if it's stuck
-         * waiting for the destination.
+         * waiting for the destination.  We do shutdown on to_dst_file should
+         * also shutdown the from_dst_file as they're in a pair. We explicilty
+         * don't operate on from_dst_file because it's potentially racy
+         * (rp_thread could have reset it in parallel).
          */
-        qemu_file_shutdown(ms->rp_state.from_dst_file);
+        qemu_file_shutdown(ms->to_dst_file);
         mark_source_rp_bad(ms);
     }
     trace_await_return_path_close_on_source_joining();
-- 
2.31.1

[PATCH 3/5] migration: Introduce migration_ioc_[un]register_yank()

[Peter Xu]

There're plenty of places in migration/* that checks against either socket or
tls typed ioc for yank operations.  Provide two helpers to hide all these
information.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/channel.c           | 15 ++-------------
 migration/multifd.c           |  8 ++------
 migration/qemu-file-channel.c |  8 ++------
 migration/yank_functions.c    | 28 ++++++++++++++++++++++++++++
 migration/yank_functions.h    |  2 ++
 5 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/migration/channel.c b/migration/channel.c
index 01275a9162..c4fc000a1a 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -44,13 +44,7 @@ void migration_channel_process_incoming(QIOChannel *ioc)
                              TYPE_QIO_CHANNEL_TLS)) {
         migration_tls_channel_process_incoming(s, ioc, &local_err);
     } else {
-        if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
-            object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) {
-            yank_register_function(MIGRATION_YANK_INSTANCE,
-                                   migration_yank_iochannel,
-                                   QIO_CHANNEL(ioc));
-        }
-
+        migration_ioc_register_yank(ioc);
         migration_ioc_process_incoming(ioc, &local_err);
     }
 
@@ -94,12 +88,7 @@ void migration_channel_connect(MigrationState *s,
         } else {
             QEMUFile *f = qemu_fopen_channel_output(ioc);
 
-            if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
-                object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) {
-                yank_register_function(MIGRATION_YANK_INSTANCE,
-                                       migration_yank_iochannel,
-                                       QIO_CHANNEL(ioc));
-            }
+            migration_ioc_register_yank(ioc);
 
             qemu_mutex_lock(&s->qemu_file_lock);
             s->to_dst_file = f;
diff --git a/migration/multifd.c b/migration/multifd.c
index ab41590e71..377da78f5b 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -987,12 +987,8 @@ int multifd_load_cleanup(Error **errp)
     for (i = 0; i < migrate_multifd_channels(); i++) {
         MultiFDRecvParams *p = &multifd_recv_state->params[i];
 
-        if ((object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_SOCKET) ||
-             object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_TLS))
-            && OBJECT(p->c)->ref == 1) {
-            yank_unregister_function(MIGRATION_YANK_INSTANCE,
-                                     migration_yank_iochannel,
-                                     QIO_CHANNEL(p->c));
+        if (OBJECT(p->c)->ref == 1) {
+            migration_ioc_unregister_yank(p->c);
         }
 
         object_unref(OBJECT(p->c));
diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
index fad340ea7a..867a5ed0c3 100644
--- a/migration/qemu-file-channel.c
+++ b/migration/qemu-file-channel.c
@@ -107,12 +107,8 @@ static int channel_close(void *opaque, Error **errp)
     int ret;
     QIOChannel *ioc = QIO_CHANNEL(opaque);
     ret = qio_channel_close(ioc, errp);
-    if ((object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
-         object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS))
-        && OBJECT(ioc)->ref == 1) {
-        yank_unregister_function(MIGRATION_YANK_INSTANCE,
-                                 migration_yank_iochannel,
-                                 QIO_CHANNEL(ioc));
+    if (OBJECT(ioc)->ref == 1) {
+        migration_ioc_unregister_yank(ioc);
     }
     object_unref(OBJECT(ioc));
     return ret;
diff --git a/migration/yank_functions.c b/migration/yank_functions.c
index 96c90e17dc..23697173ae 100644
--- a/migration/yank_functions.c
+++ b/migration/yank_functions.c
@@ -11,6 +11,9 @@
 #include "qapi/error.h"
 #include "io/channel.h"
 #include "yank_functions.h"
+#include "qemu/yank.h"
+#include "io/channel-socket.h"
+#include "io/channel-tls.h"
 
 void migration_yank_iochannel(void *opaque)
 {
@@ -18,3 +21,28 @@ void migration_yank_iochannel(void *opaque)
 
     qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
 }
+
+/* Return whether yank is supported on this ioc */
+static bool migration_ioc_yank_supported(QIOChannel *ioc)
+{
+    return object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
+        object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS);
+}
+
+void migration_ioc_register_yank(QIOChannel *ioc)
+{
+    if (migration_ioc_yank_supported(ioc)) {
+        yank_register_function(MIGRATION_YANK_INSTANCE,
+                               migration_yank_iochannel,
+                               QIO_CHANNEL(ioc));
+    }
+}
+
+void migration_ioc_unregister_yank(QIOChannel *ioc)
+{
+    if (migration_ioc_yank_supported(ioc)) {
+        yank_unregister_function(MIGRATION_YANK_INSTANCE,
+                                 migration_yank_iochannel,
+                                 QIO_CHANNEL(ioc));
+    }
+}
diff --git a/migration/yank_functions.h b/migration/yank_functions.h
index 055ea22523..74c7f18c91 100644
--- a/migration/yank_functions.h
+++ b/migration/yank_functions.h
@@ -15,3 +15,5 @@
  * @opaque: QIOChannel to shutdown
  */
 void migration_yank_iochannel(void *opaque);
+void migration_ioc_register_yank(QIOChannel *ioc);
+void migration_ioc_unregister_yank(QIOChannel *ioc);
-- 
2.31.1

[PATCH 4/5] migration: Teach QEMUFile to be QIOChannel-aware

[Peter Xu]

migration uses QIOChannel typed qemufiles.  In follow up patches, we'll need
the capability to identify this fact, so that we can get the backing QIOChannel
from a QEMUFile.

We can also define types for QEMUFile but so far since we only need to be able
to identify QIOChannel, introduce a boolean which is simpler.

No functional change.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/qemu-file-channel.c | 4 ++--
 migration/qemu-file.c         | 5 ++++-
 migration/qemu-file.h         | 2 +-
 migration/ram.c               | 2 +-
 migration/savevm.c            | 4 ++--
 5 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
index 867a5ed0c3..2f8b1fcd46 100644
--- a/migration/qemu-file-channel.c
+++ b/migration/qemu-file-channel.c
@@ -187,11 +187,11 @@ static const QEMUFileOps channel_output_ops = {
 QEMUFile *qemu_fopen_channel_input(QIOChannel *ioc)
 {
     object_ref(OBJECT(ioc));
-    return qemu_fopen_ops(ioc, &channel_input_ops);
+    return qemu_fopen_ops(ioc, &channel_input_ops, true);
 }
 
 QEMUFile *qemu_fopen_channel_output(QIOChannel *ioc)
 {
     object_ref(OBJECT(ioc));
-    return qemu_fopen_ops(ioc, &channel_output_ops);
+    return qemu_fopen_ops(ioc, &channel_output_ops, true);
 }
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 1eacf9e831..ada58c94dd 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -55,6 +55,8 @@ struct QEMUFile {
     Error *last_error_obj;
     /* has the file has been shutdown */
     bool shutdown;
+    /* Whether opaque points to a QIOChannel */
+    bool has_ioc;
 };
 
 /*
@@ -101,7 +103,7 @@ bool qemu_file_mode_is_not_valid(const char *mode)
     return false;
 }
 
-QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops)
+QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops, bool has_ioc)
 {
     QEMUFile *f;
 
@@ -109,6 +111,7 @@ QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops)
 
     f->opaque = opaque;
     f->ops = ops;
+    f->has_ioc = has_ioc;
     return f;
 }
 
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index a9b6d6ccb7..80d0e79fd1 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -119,7 +119,7 @@ typedef struct QEMUFileHooks {
     QEMURamSaveFunc *save_page;
 } QEMUFileHooks;
 
-QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops);
+QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops, bool has_ioc);
 void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks);
 int qemu_get_fd(QEMUFile *f);
 int qemu_fclose(QEMUFile *f);
diff --git a/migration/ram.c b/migration/ram.c
index b5fc454b2f..f2a86f9971 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -550,7 +550,7 @@ static int compress_threads_save_setup(void)
         /* comp_param[i].file is just used as a dummy buffer to save data,
          * set its ops to empty.
          */
-        comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops);
+        comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops, false);
         comp_param[i].done = true;
         comp_param[i].quit = false;
         qemu_mutex_init(&comp_param[i].mutex);
diff --git a/migration/savevm.c b/migration/savevm.c
index 72848b946c..96b5e5d639 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -168,9 +168,9 @@ static const QEMUFileOps bdrv_write_ops = {
 static QEMUFile *qemu_fopen_bdrv(BlockDriverState *bs, int is_writable)
 {
     if (is_writable) {
-        return qemu_fopen_ops(bs, &bdrv_write_ops);
+        return qemu_fopen_ops(bs, &bdrv_write_ops, false);
     }
-    return qemu_fopen_ops(bs, &bdrv_read_ops);
+    return qemu_fopen_ops(bs, &bdrv_read_ops, false);
 }
 
 
-- 
2.31.1

[PATCH 5/5] migration: Move the yank unregister of channel_close out

[Peter Xu]

It's efficient, but hackish to call yank unregister calls in channel_close(),
especially it'll be hard to debug when qemu crashed with some yank function
leaked.

Remove that hack, but instead explicitly unregister yank functions at the
places where needed, they are:

  (on src)
  - migrate_fd_cleanup
  - postcopy_pause

  (on dst)
  - migration_incoming_state_destroy
  - postcopy_pause_incoming

Some small helpers are introduced to achieve this task.  One of them is called
migration_file_get_ioc(), which tries to fetch the ioc out of the qemu file.
It's a bit tricky because qemufile is also used for savevm/loadvm.  We need to
check for NULL to bypass those.  Please see comment above that helper for more
information.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c         |  5 +++++
 migration/qemu-file-channel.c |  3 ---
 migration/qemu-file.c         | 12 ++++++++++++
 migration/qemu-file.h         |  2 ++
 migration/savevm.c            |  7 +++++++
 migration/yank_functions.c    | 14 ++++++++++++++
 migration/yank_functions.h    |  1 +
 7 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 4f48cde796..65b8c2eb52 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -59,6 +59,7 @@
 #include "multifd.h"
 #include "qemu/yank.h"
 #include "sysemu/cpus.h"
+#include "yank_functions.h"
 
 #define MAX_THROTTLE  (128 << 20)      /* Migration transfer speed throttling */
 
@@ -273,6 +274,7 @@ void migration_incoming_state_destroy(void)
     }
 
     if (mis->from_src_file) {
+        migration_ioc_unregister_yank_from_file(mis->from_src_file);
         qemu_fclose(mis->from_src_file);
         mis->from_src_file = NULL;
     }
@@ -1811,6 +1813,7 @@ static void migrate_fd_cleanup(MigrationState *s)
          * 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_fclose(tmp);
     }
 
@@ -3337,6 +3340,8 @@ static MigThrError postcopy_pause(MigrationState *s)
 
         /* Current channel is possibly broken. Release it. */
         assert(s->to_dst_file);
+        /* Unregister yank for current channel */
+        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;
diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
index 2f8b1fcd46..bb5a5752df 100644
--- a/migration/qemu-file-channel.c
+++ b/migration/qemu-file-channel.c
@@ -107,9 +107,6 @@ static int channel_close(void *opaque, Error **errp)
     int ret;
     QIOChannel *ioc = QIO_CHANNEL(opaque);
     ret = qio_channel_close(ioc, errp);
-    if (OBJECT(ioc)->ref == 1) {
-        migration_ioc_unregister_yank(ioc);
-    }
     object_unref(OBJECT(ioc));
     return ret;
 }
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index ada58c94dd..b32ff35e73 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -854,3 +854,15 @@ void qemu_file_set_blocking(QEMUFile *f, bool block)
         f->ops->set_blocking(f->opaque, block, NULL);
     }
 }
+
+/*
+ * Return the ioc object if it's a migration channel.  Note: it can return NULL
+ * for callers passing in a non-migration qemufile.  E.g. see qemu_fopen_bdrv()
+ * and its usage in e.g. load_snapshot().  So we need to check against NULL
+ * before using it.  If without the check, migration_incoming_state_destroy()
+ * could fail for load_snapshot().
+ */
+QIOChannel *migration_file_get_ioc(QEMUFile *file)
+{
+    return file->has_ioc ? QIO_CHANNEL(file->opaque) : NULL;
+}
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index 80d0e79fd1..59f3f78e8b 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -27,6 +27,7 @@
 
 #include <zlib.h>
 #include "exec/cpu-common.h"
+#include "io/channel.h"
 
 /* Read a chunk of data from a file at the given position.  The pos argument
  * can be ignored if the file is only be used for streaming.  The number of
@@ -179,5 +180,6 @@ void ram_control_load_hook(QEMUFile *f, uint64_t flags, void *data);
 size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
                              ram_addr_t offset, size_t size,
                              uint64_t *bytes_sent);
+QIOChannel *migration_file_get_ioc(QEMUFile *file);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 96b5e5d639..7b7b64bd13 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -65,6 +65,7 @@
 #include "qemu/bitmap.h"
 #include "net/announce.h"
 #include "qemu/yank.h"
+#include "yank_functions.h"
 
 const unsigned int postcopy_ram_discard_version;
 
@@ -2568,6 +2569,12 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
     /* Clear the triggered bit to allow one recovery */
     mis->postcopy_recover_triggered = false;
 
+    /*
+     * Unregister yank with either from/to src would work, since ioc behind it
+     * is the same
+     */
+    migration_ioc_unregister_yank_from_file(mis->from_src_file);
+
     assert(mis->from_src_file);
     qemu_file_shutdown(mis->from_src_file);
     qemu_fclose(mis->from_src_file);
diff --git a/migration/yank_functions.c b/migration/yank_functions.c
index 23697173ae..1f35ba3512 100644
--- a/migration/yank_functions.c
+++ b/migration/yank_functions.c
@@ -14,6 +14,7 @@
 #include "qemu/yank.h"
 #include "io/channel-socket.h"
 #include "io/channel-tls.h"
+#include "qemu-file.h"
 
 void migration_yank_iochannel(void *opaque)
 {
@@ -46,3 +47,16 @@ void migration_ioc_unregister_yank(QIOChannel *ioc)
                                  QIO_CHANNEL(ioc));
     }
 }
+
+void migration_ioc_unregister_yank_from_file(QEMUFile *file)
+{
+    QIOChannel *ioc = migration_file_get_ioc(file);
+
+    if (ioc) {
+        /*
+         * For migration qemufiles, we'll always reach here.  Though we'll skip
+         * calls from e.g. savevm/loadvm as they don't use yank.
+         */
+        migration_ioc_unregister_yank(ioc);
+    }
+}
diff --git a/migration/yank_functions.h b/migration/yank_functions.h
index 74c7f18c91..a7577955ed 100644
--- a/migration/yank_functions.h
+++ b/migration/yank_functions.h
@@ -17,3 +17,4 @@
 void migration_yank_iochannel(void *opaque);
 void migration_ioc_register_yank(QIOChannel *ioc);
 void migration_ioc_unregister_yank(QIOChannel *ioc);
+void migration_ioc_unregister_yank_from_file(QEMUFile *file);
-- 
2.31.1

Re: [PATCH 1/5] migration: Fix missing join() of rp_thread

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> It's possible that the migration thread skip the join() of the rp_thread in
> below race and crash on src right at finishing migration:
> 
>        migration_thread                     rp_thread
>        ----------------                     ---------
>     migration_completion()
>                                         (before rp_thread quits)
>                                         from_dst_file=NULL
>                                         [thread got scheduled out]
>       s->rp_state.from_dst_file==NULL
>         (skip join() of rp_thread)
>     migrate_fd_cleanup()
>       qemu_fclose(s->to_dst_file)
>       yank_unregister_instance()
>         assert(yank_find_entry())  <------- crash
> 
> It could mostly happen with postcopy, but that shouldn't be required, e.g., I
> think it could also trigger with MIGRATION_CAPABILITY_RETURN_PATH set.
> 
> It's suspected that above race could be the root cause of a recent (but rare)
> migration-test break reported by either Dave or PMM:
> 
> https://lore.kernel.org/qemu-devel/YPamXAHwan%2FPPXLf@work-vm/
> 
> The issue is: from_dst_file is reset in the rp_thread, so if the thread reset
> it to NULL fast enough then the migration thread will assume there's no
> rp_thread at all.
> 
> This could potentially cause more severe issue (e.g. crash) after the yank code.
> 
> Fix it by using a boolean to keep "whether we've created rp_thread".
> 
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/migration.c | 4 +++-
>  migration/migration.h | 7 +++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 2d306582eb..21b94f75a3 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2867,6 +2867,7 @@ static int open_return_path_on_source(MigrationState *ms,
>  
>      qemu_thread_create(&ms->rp_state.rp_thread, "return path",
>                         source_return_path_thread, ms, QEMU_THREAD_JOINABLE);
> +    ms->rp_state.rp_thread_created = true;
>  
>      trace_open_return_path_on_source_continue();
>  
> @@ -2891,6 +2892,7 @@ static int await_return_path_close_on_source(MigrationState *ms)
>      }
>      trace_await_return_path_close_on_source_joining();
>      qemu_thread_join(&ms->rp_state.rp_thread);
> +    ms->rp_state.rp_thread_created = false;
>      trace_await_return_path_close_on_source_close();
>      return ms->rp_state.error;
>  }
> @@ -3170,7 +3172,7 @@ static void migration_completion(MigrationState *s)
>       * it will wait for the destination to send it's status in
>       * a SHUT command).
>       */
> -    if (s->rp_state.from_dst_file) {
> +    if (s->rp_state.rp_thread_created) {
>          int rp_error;
>          trace_migration_return_path_end_before();
>          rp_error = await_return_path_close_on_source(s);
> diff --git a/migration/migration.h b/migration/migration.h
> index 2ebb740dfa..c302879fad 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -195,6 +195,13 @@ struct MigrationState {
>          QEMUFile     *from_dst_file;
>          QemuThread    rp_thread;
>          bool          error;
> +        /*
> +         * We can also check non-zero of rp_thread, but there's no "official"
> +         * way to do this, so this bool makes it slightly more elegant.
> +         * Checking from_dst_file for this is racy because from_dst_file will
> +         * be cleared in the rp_thread!
> +         */
> +        bool          rp_thread_created;
>          QemuSemaphore rp_sem;
>      } rp_state;
>  
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 2/5] migration: Shutdown src in await_return_path_close_on_source()

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> We have a logic in await_return_path_close_on_source() that we will explicitly
> shutdown the socket when migration encounters errors.  However it could be racy
> because from_dst_file could have been reset right after checking it but before
> passing it to qemu_file_shutdown() by the rp_thread.
> 
> Fix it by shutdown() on the src file instead.  Since they must be a pair of
> qemu files, shutdown on either of them will work the same.
> 
> Since at it, drop the check for from_dst_file directly, which makes the
> behavior even more predictable.

So while the existing code maybe racy, I'm not sure that this change
keeps the semantics; the channel may well have dup()'d the fd's for the
two directions, and I'm not convinced that a shutdown() on one will
necessarily impact the other; and if the shutdown doesn't happen the
rp_thread might not exit, and we might block on the koin.

Why don't we solve this a different way - how about we move the:
    ms->rp_state.from_dst_file = NULL;
    qemu_fclose(rp);

out of the source_return_path_thread and put it in
await_return_path_close_on_source, immediately after the join?
Then we *know* that the the rp thread isn't messing with it.

Dave

> Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 21b94f75a3..4f48cde796 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2882,12 +2882,15 @@ static int await_return_path_close_on_source(MigrationState *ms)
>       * rp_thread will exit, however if there's an error we need to cause
>       * it to exit.
>       */
> -    if (qemu_file_get_error(ms->to_dst_file) && ms->rp_state.from_dst_file) {
> +    if (qemu_file_get_error(ms->to_dst_file)) {
>          /*
>           * shutdown(2), if we have it, will cause it to unblock if it's stuck
> -         * waiting for the destination.
> +         * waiting for the destination.  We do shutdown on to_dst_file should
> +         * also shutdown the from_dst_file as they're in a pair. We explicilty
> +         * don't operate on from_dst_file because it's potentially racy
> +         * (rp_thread could have reset it in parallel).
>           */
> -        qemu_file_shutdown(ms->rp_state.from_dst_file);
> +        qemu_file_shutdown(ms->to_dst_file);
>          mark_source_rp_bad(ms);
>      }
>      trace_await_return_path_close_on_source_joining();
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 3/5] migration: Introduce migration_ioc_[un]register_yank()

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> There're plenty of places in migration/* that checks against either socket or
> tls typed ioc for yank operations.  Provide two helpers to hide all these
> information.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/channel.c           | 15 ++-------------
>  migration/multifd.c           |  8 ++------
>  migration/qemu-file-channel.c |  8 ++------
>  migration/yank_functions.c    | 28 ++++++++++++++++++++++++++++
>  migration/yank_functions.h    |  2 ++
>  5 files changed, 36 insertions(+), 25 deletions(-)
> 
> diff --git a/migration/channel.c b/migration/channel.c
> index 01275a9162..c4fc000a1a 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -44,13 +44,7 @@ void migration_channel_process_incoming(QIOChannel *ioc)
>                               TYPE_QIO_CHANNEL_TLS)) {
>          migration_tls_channel_process_incoming(s, ioc, &local_err);
>      } else {
> -        if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
> -            object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) {
> -            yank_register_function(MIGRATION_YANK_INSTANCE,
> -                                   migration_yank_iochannel,
> -                                   QIO_CHANNEL(ioc));
> -        }
> -
> +        migration_ioc_register_yank(ioc);
>          migration_ioc_process_incoming(ioc, &local_err);
>      }
>  
> @@ -94,12 +88,7 @@ void migration_channel_connect(MigrationState *s,
>          } else {
>              QEMUFile *f = qemu_fopen_channel_output(ioc);
>  
> -            if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
> -                object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) {
> -                yank_register_function(MIGRATION_YANK_INSTANCE,
> -                                       migration_yank_iochannel,
> -                                       QIO_CHANNEL(ioc));
> -            }
> +            migration_ioc_register_yank(ioc);
>  
>              qemu_mutex_lock(&s->qemu_file_lock);
>              s->to_dst_file = f;
> diff --git a/migration/multifd.c b/migration/multifd.c
> index ab41590e71..377da78f5b 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -987,12 +987,8 @@ int multifd_load_cleanup(Error **errp)
>      for (i = 0; i < migrate_multifd_channels(); i++) {
>          MultiFDRecvParams *p = &multifd_recv_state->params[i];
>  
> -        if ((object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_SOCKET) ||
> -             object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_TLS))
> -            && OBJECT(p->c)->ref == 1) {
> -            yank_unregister_function(MIGRATION_YANK_INSTANCE,
> -                                     migration_yank_iochannel,
> -                                     QIO_CHANNEL(p->c));
> +        if (OBJECT(p->c)->ref == 1) {
> +            migration_ioc_unregister_yank(p->c);
>          }
>  
>          object_unref(OBJECT(p->c));
> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
> index fad340ea7a..867a5ed0c3 100644
> --- a/migration/qemu-file-channel.c
> +++ b/migration/qemu-file-channel.c
> @@ -107,12 +107,8 @@ static int channel_close(void *opaque, Error **errp)
>      int ret;
>      QIOChannel *ioc = QIO_CHANNEL(opaque);
>      ret = qio_channel_close(ioc, errp);
> -    if ((object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
> -         object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS))
> -        && OBJECT(ioc)->ref == 1) {
> -        yank_unregister_function(MIGRATION_YANK_INSTANCE,
> -                                 migration_yank_iochannel,
> -                                 QIO_CHANNEL(ioc));
> +    if (OBJECT(ioc)->ref == 1) {
> +        migration_ioc_unregister_yank(ioc);
>      }
>      object_unref(OBJECT(ioc));
>      return ret;
> diff --git a/migration/yank_functions.c b/migration/yank_functions.c
> index 96c90e17dc..23697173ae 100644
> --- a/migration/yank_functions.c
> +++ b/migration/yank_functions.c
> @@ -11,6 +11,9 @@
>  #include "qapi/error.h"
>  #include "io/channel.h"
>  #include "yank_functions.h"
> +#include "qemu/yank.h"
> +#include "io/channel-socket.h"
> +#include "io/channel-tls.h"
>  
>  void migration_yank_iochannel(void *opaque)
>  {
> @@ -18,3 +21,28 @@ void migration_yank_iochannel(void *opaque)
>  
>      qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
>  }
> +
> +/* Return whether yank is supported on this ioc */
> +static bool migration_ioc_yank_supported(QIOChannel *ioc)
> +{
> +    return object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
> +        object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS);
> +}
> +
> +void migration_ioc_register_yank(QIOChannel *ioc)
> +{
> +    if (migration_ioc_yank_supported(ioc)) {
> +        yank_register_function(MIGRATION_YANK_INSTANCE,
> +                               migration_yank_iochannel,
> +                               QIO_CHANNEL(ioc));
> +    }
> +}
> +
> +void migration_ioc_unregister_yank(QIOChannel *ioc)
> +{
> +    if (migration_ioc_yank_supported(ioc)) {
> +        yank_unregister_function(MIGRATION_YANK_INSTANCE,
> +                                 migration_yank_iochannel,
> +                                 QIO_CHANNEL(ioc));
> +    }
> +}
> diff --git a/migration/yank_functions.h b/migration/yank_functions.h
> index 055ea22523..74c7f18c91 100644
> --- a/migration/yank_functions.h
> +++ b/migration/yank_functions.h
> @@ -15,3 +15,5 @@
>   * @opaque: QIOChannel to shutdown
>   */
>  void migration_yank_iochannel(void *opaque);
> +void migration_ioc_register_yank(QIOChannel *ioc);
> +void migration_ioc_unregister_yank(QIOChannel *ioc);
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 4/5] migration: Teach QEMUFile to be QIOChannel-aware

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> migration uses QIOChannel typed qemufiles.  In follow up patches, we'll need
> the capability to identify this fact, so that we can get the backing QIOChannel
> from a QEMUFile.
> 
> We can also define types for QEMUFile but so far since we only need to be able
> to identify QIOChannel, introduce a boolean which is simpler.
> 
> No functional change.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

This is messy but I can't see another quick way; the better way would be
to add an OBJECT or QIOCHannel wrapper for BlockDriverState.


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/qemu-file-channel.c | 4 ++--
>  migration/qemu-file.c         | 5 ++++-
>  migration/qemu-file.h         | 2 +-
>  migration/ram.c               | 2 +-
>  migration/savevm.c            | 4 ++--
>  5 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
> index 867a5ed0c3..2f8b1fcd46 100644
> --- a/migration/qemu-file-channel.c
> +++ b/migration/qemu-file-channel.c
> @@ -187,11 +187,11 @@ static const QEMUFileOps channel_output_ops = {
>  QEMUFile *qemu_fopen_channel_input(QIOChannel *ioc)
>  {
>      object_ref(OBJECT(ioc));
> -    return qemu_fopen_ops(ioc, &channel_input_ops);
> +    return qemu_fopen_ops(ioc, &channel_input_ops, true);
>  }
>  
>  QEMUFile *qemu_fopen_channel_output(QIOChannel *ioc)
>  {
>      object_ref(OBJECT(ioc));
> -    return qemu_fopen_ops(ioc, &channel_output_ops);
> +    return qemu_fopen_ops(ioc, &channel_output_ops, true);
>  }
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 1eacf9e831..ada58c94dd 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -55,6 +55,8 @@ struct QEMUFile {
>      Error *last_error_obj;
>      /* has the file has been shutdown */
>      bool shutdown;
> +    /* Whether opaque points to a QIOChannel */
> +    bool has_ioc;
>  };
>  
>  /*
> @@ -101,7 +103,7 @@ bool qemu_file_mode_is_not_valid(const char *mode)
>      return false;
>  }
>  
> -QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops)
> +QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops, bool has_ioc)
>  {
>      QEMUFile *f;
>  
> @@ -109,6 +111,7 @@ QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops)
>  
>      f->opaque = opaque;
>      f->ops = ops;
> +    f->has_ioc = has_ioc;
>      return f;
>  }
>  
> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index a9b6d6ccb7..80d0e79fd1 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -119,7 +119,7 @@ typedef struct QEMUFileHooks {
>      QEMURamSaveFunc *save_page;
>  } QEMUFileHooks;
>  
> -QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops);
> +QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops, bool has_ioc);
>  void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks);
>  int qemu_get_fd(QEMUFile *f);
>  int qemu_fclose(QEMUFile *f);
> diff --git a/migration/ram.c b/migration/ram.c
> index b5fc454b2f..f2a86f9971 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -550,7 +550,7 @@ static int compress_threads_save_setup(void)
>          /* comp_param[i].file is just used as a dummy buffer to save data,
>           * set its ops to empty.
>           */
> -        comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops);
> +        comp_param[i].file = qemu_fopen_ops(NULL, &empty_ops, false);
>          comp_param[i].done = true;
>          comp_param[i].quit = false;
>          qemu_mutex_init(&comp_param[i].mutex);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 72848b946c..96b5e5d639 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -168,9 +168,9 @@ static const QEMUFileOps bdrv_write_ops = {
>  static QEMUFile *qemu_fopen_bdrv(BlockDriverState *bs, int is_writable)
>  {
>      if (is_writable) {
> -        return qemu_fopen_ops(bs, &bdrv_write_ops);
> +        return qemu_fopen_ops(bs, &bdrv_write_ops, false);
>      }
> -    return qemu_fopen_ops(bs, &bdrv_read_ops);
> +    return qemu_fopen_ops(bs, &bdrv_read_ops, false);
>  }
>  
>  
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 5/5] migration: Move the yank unregister of channel_close out

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> It's efficient, but hackish to call yank unregister calls in channel_close(),
> especially it'll be hard to debug when qemu crashed with some yank function
> leaked.
> 
> Remove that hack, but instead explicitly unregister yank functions at the
> places where needed, they are:
> 
>   (on src)
>   - migrate_fd_cleanup
>   - postcopy_pause
> 
>   (on dst)
>   - migration_incoming_state_destroy
>   - postcopy_pause_incoming
> 
> Some small helpers are introduced to achieve this task.  One of them is called
> migration_file_get_ioc(), which tries to fetch the ioc out of the qemu file.
> It's a bit tricky because qemufile is also used for savevm/loadvm.  We need to
> check for NULL to bypass those.  Please see comment above that helper for more
> information.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Yes

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


although....

> ---
>  migration/migration.c         |  5 +++++
>  migration/qemu-file-channel.c |  3 ---
>  migration/qemu-file.c         | 12 ++++++++++++
>  migration/qemu-file.h         |  2 ++
>  migration/savevm.c            |  7 +++++++
>  migration/yank_functions.c    | 14 ++++++++++++++
>  migration/yank_functions.h    |  1 +
>  7 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 4f48cde796..65b8c2eb52 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -59,6 +59,7 @@
>  #include "multifd.h"
>  #include "qemu/yank.h"
>  #include "sysemu/cpus.h"
> +#include "yank_functions.h"
>  
>  #define MAX_THROTTLE  (128 << 20)      /* Migration transfer speed throttling */
>  
> @@ -273,6 +274,7 @@ void migration_incoming_state_destroy(void)
>      }
>  
>      if (mis->from_src_file) {
> +        migration_ioc_unregister_yank_from_file(mis->from_src_file);
>          qemu_fclose(mis->from_src_file);
>          mis->from_src_file = NULL;
>      }
> @@ -1811,6 +1813,7 @@ static void migrate_fd_cleanup(MigrationState *s)
>           * 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_fclose(tmp);
>      }
>  
> @@ -3337,6 +3340,8 @@ static MigThrError postcopy_pause(MigrationState *s)
>  
>          /* Current channel is possibly broken. Release it. */
>          assert(s->to_dst_file);
> +        /* Unregister yank for current channel */
> +        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;
> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
> index 2f8b1fcd46..bb5a5752df 100644
> --- a/migration/qemu-file-channel.c
> +++ b/migration/qemu-file-channel.c
> @@ -107,9 +107,6 @@ static int channel_close(void *opaque, Error **errp)
>      int ret;
>      QIOChannel *ioc = QIO_CHANNEL(opaque);
>      ret = qio_channel_close(ioc, errp);
> -    if (OBJECT(ioc)->ref == 1) {
> -        migration_ioc_unregister_yank(ioc);
> -    }
>      object_unref(OBJECT(ioc));
>      return ret;
>  }
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index ada58c94dd..b32ff35e73 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -854,3 +854,15 @@ void qemu_file_set_blocking(QEMUFile *f, bool block)
>          f->ops->set_blocking(f->opaque, block, NULL);
>      }
>  }
> +
> +/*
> + * Return the ioc object if it's a migration channel.  Note: it can return NULL
> + * for callers passing in a non-migration qemufile.  E.g. see qemu_fopen_bdrv()
> + * and its usage in e.g. load_snapshot().  So we need to check against NULL
> + * before using it.  If without the check, migration_incoming_state_destroy()
> + * could fail for load_snapshot().
> + */
> +QIOChannel *migration_file_get_ioc(QEMUFile *file)
> +{
> +    return file->has_ioc ? QIO_CHANNEL(file->opaque) : NULL;
> +}

Do you think this should go in the previous patch where you created
has_ioc?  Also the name is weird, qemu_file is probably right for
everything in here.

Dave

> diff --git a/migration/qemu-file.h b/migration/qemu-file.h
> index 80d0e79fd1..59f3f78e8b 100644
> --- a/migration/qemu-file.h
> +++ b/migration/qemu-file.h
> @@ -27,6 +27,7 @@
>  
>  #include <zlib.h>
>  #include "exec/cpu-common.h"
> +#include "io/channel.h"
>  
>  /* Read a chunk of data from a file at the given position.  The pos argument
>   * can be ignored if the file is only be used for streaming.  The number of
> @@ -179,5 +180,6 @@ void ram_control_load_hook(QEMUFile *f, uint64_t flags, void *data);
>  size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
>                               ram_addr_t offset, size_t size,
>                               uint64_t *bytes_sent);
> +QIOChannel *migration_file_get_ioc(QEMUFile *file);
>  
>  #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 96b5e5d639..7b7b64bd13 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -65,6 +65,7 @@
>  #include "qemu/bitmap.h"
>  #include "net/announce.h"
>  #include "qemu/yank.h"
> +#include "yank_functions.h"
>  
>  const unsigned int postcopy_ram_discard_version;
>  
> @@ -2568,6 +2569,12 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
>      /* Clear the triggered bit to allow one recovery */
>      mis->postcopy_recover_triggered = false;
>  
> +    /*
> +     * Unregister yank with either from/to src would work, since ioc behind it
> +     * is the same
> +     */
> +    migration_ioc_unregister_yank_from_file(mis->from_src_file);
> +
>      assert(mis->from_src_file);
>      qemu_file_shutdown(mis->from_src_file);
>      qemu_fclose(mis->from_src_file);
> diff --git a/migration/yank_functions.c b/migration/yank_functions.c
> index 23697173ae..1f35ba3512 100644
> --- a/migration/yank_functions.c
> +++ b/migration/yank_functions.c
> @@ -14,6 +14,7 @@
>  #include "qemu/yank.h"
>  #include "io/channel-socket.h"
>  #include "io/channel-tls.h"
> +#include "qemu-file.h"
>  
>  void migration_yank_iochannel(void *opaque)
>  {
> @@ -46,3 +47,16 @@ void migration_ioc_unregister_yank(QIOChannel *ioc)
>                                   QIO_CHANNEL(ioc));
>      }
>  }
> +
> +void migration_ioc_unregister_yank_from_file(QEMUFile *file)
> +{
> +    QIOChannel *ioc = migration_file_get_ioc(file);
> +
> +    if (ioc) {
> +        /*
> +         * For migration qemufiles, we'll always reach here.  Though we'll skip
> +         * calls from e.g. savevm/loadvm as they don't use yank.
> +         */
> +        migration_ioc_unregister_yank(ioc);
> +    }
> +}
> diff --git a/migration/yank_functions.h b/migration/yank_functions.h
> index 74c7f18c91..a7577955ed 100644
> --- a/migration/yank_functions.h
> +++ b/migration/yank_functions.h
> @@ -17,3 +17,4 @@
>  void migration_yank_iochannel(void *opaque);
>  void migration_ioc_register_yank(QIOChannel *ioc);
>  void migration_ioc_unregister_yank(QIOChannel *ioc);
> +void migration_ioc_unregister_yank_from_file(QEMUFile *file);
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 4/5] migration: Teach QEMUFile to be QIOChannel-aware

[Daniel P. Berrangé]

On Wed, Jul 21, 2021 at 11:27:44AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > migration uses QIOChannel typed qemufiles.  In follow up patches, we'll need
> > the capability to identify this fact, so that we can get the backing QIOChannel
> > from a QEMUFile.
> > 
> > We can also define types for QEMUFile but so far since we only need to be able
> > to identify QIOChannel, introduce a boolean which is simpler.
> > 
> > No functional change.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> This is messy but I can't see another quick way; the better way would be
> to add an OBJECT or QIOCHannel wrapper for BlockDriverState.

I looked at making a QIOChannel for BlockDriverState but it was not
as easy as it might seem.  The problem is that the QEMUFile
get_buffer / write_buffer methods take a offset at which the
I/O operation is required to be applied.

For the existing QIOChannel impl for migration, we simply ignore
the 'pos' argument entirely, since it is irrelevant for the main
migration channel doing streaming.

For a BlockDriverState based impl though I think we need to
honour "pos" in some manner.

I think it ought to be possible to rewrite the savevm code
so that it uses 'seek' in the few places it needs to, and
then we can drop "pos" from get_buffer/write_buffer, but
that requires careful consideration.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/5] migration: Shutdown src in await_return_path_close_on_source()

[Peter Xu]

On Wed, Jul 21, 2021 at 10:55:00AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > We have a logic in await_return_path_close_on_source() that we will explicitly
> > shutdown the socket when migration encounters errors.  However it could be racy
> > because from_dst_file could have been reset right after checking it but before
> > passing it to qemu_file_shutdown() by the rp_thread.
> > 
> > Fix it by shutdown() on the src file instead.  Since they must be a pair of
> > qemu files, shutdown on either of them will work the same.
> > 
> > Since at it, drop the check for from_dst_file directly, which makes the
> > behavior even more predictable.
> 
> So while the existing code maybe racy, I'm not sure that this change
> keeps the semantics; the channel may well have dup()'d the fd's for the
> two directions, and I'm not convinced that a shutdown() on one will
> necessarily impact the other; and if the shutdown doesn't happen the
> rp_thread might not exit, and we might block on the koin.

dup() seems fine as long as the backend file/socket is the same; but I get the
point here, e.g., potentially from/to channels can indeed be different ones.

> 
> Why don't we solve this a different way - how about we move the:
>     ms->rp_state.from_dst_file = NULL;
>     qemu_fclose(rp);
> 
> out of the source_return_path_thread and put it in
> await_return_path_close_on_source, immediately after the join?
> Then we *know* that the the rp thread isn't messing with it.

Yes that looks working for this special case of when rp_thread quits.

It's just that it'll make things a bit more complicated, previously
from_dst_file is only reset in rp_thread (for either paused or completed
migration), now we handle "migration completes" a bit different.

This also reminded me that maybe we can also use the qemu_file_lock mutex as we
use to try protect access to to_dst_file, because I think from_dst_file is
potentially racy in the same way.  Even if we moved the reset to migration
thread, we still have e.g. migrate_fd_cancel() calling:

    if (s->rp_state.from_dst_file) {
        qemu_file_shutdown(s->rp_state.from_dst_file);
    }

I think that is also racy too when running in the main thread, as either the
migration thread or rp_thread could have reset it right after the check.

So.. maybe I start to use the qemu_file_lock to cover from_dst_file cases too?
Then I'll cover at leasd both migrate_fd_cancel() and this case.

Thanks,

-- 
Peter Xu

Re: [PATCH 5/5] migration: Move the yank unregister of channel_close out

[Peter Xu]

On Wed, Jul 21, 2021 at 11:39:37AM +0100, Dr. David Alan Gilbert wrote:
> > +/*
> > + * Return the ioc object if it's a migration channel.  Note: it can return NULL
> > + * for callers passing in a non-migration qemufile.  E.g. see qemu_fopen_bdrv()
> > + * and its usage in e.g. load_snapshot().  So we need to check against NULL
> > + * before using it.  If without the check, migration_incoming_state_destroy()
> > + * could fail for load_snapshot().
> > + */
> > +QIOChannel *migration_file_get_ioc(QEMUFile *file)
> > +{
> > +    return file->has_ioc ? QIO_CHANNEL(file->opaque) : NULL;
> > +}
> 
> Do you think this should go in the previous patch where you created
> has_ioc?  Also the name is weird, qemu_file is probably right for
> everything in here.

Sure; let me just move it over with it renamed.  Thanks,

-- 
Peter Xu

Re: [PATCH 2/5] migration: Shutdown src in await_return_path_close_on_source()

[Daniel P. Berrangé]

On Wed, Jul 21, 2021 at 10:55:00AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > We have a logic in await_return_path_close_on_source() that we will explicitly
> > shutdown the socket when migration encounters errors.  However it could be racy
> > because from_dst_file could have been reset right after checking it but before
> > passing it to qemu_file_shutdown() by the rp_thread.
> > 
> > Fix it by shutdown() on the src file instead.  Since they must be a pair of
> > qemu files, shutdown on either of them will work the same.
> > 
> > Since at it, drop the check for from_dst_file directly, which makes the
> > behavior even more predictable.
> 
> So while the existing code maybe racy, I'm not sure that this change
> keeps the semantics; the channel may well have dup()'d the fd's for the
> two directions, and I'm not convinced that a shutdown() on one will
> necessarily impact the other; and if the shutdown doesn't happen the
> rp_thread might not exit, and we might block on the koin.

My understanding is that 'shutdown' operation affects the state of
the socket connection. An FD is merely the way a socket is exposed
to userspace. Thus if you have multiple FDs all pointing to the same
underlying socket (thanks to dup()), then I expect that the effects
of 'shutdown' will apply equally to all of the FD copies.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/5] migration: Shutdown src in await_return_path_close_on_source()

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Wed, Jul 21, 2021 at 10:55:00AM +0100, Dr. David Alan Gilbert wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
> > > We have a logic in await_return_path_close_on_source() that we will explicitly
> > > shutdown the socket when migration encounters errors.  However it could be racy
> > > because from_dst_file could have been reset right after checking it but before
> > > passing it to qemu_file_shutdown() by the rp_thread.
> > > 
> > > Fix it by shutdown() on the src file instead.  Since they must be a pair of
> > > qemu files, shutdown on either of them will work the same.
> > > 
> > > Since at it, drop the check for from_dst_file directly, which makes the
> > > behavior even more predictable.
> > 
> > So while the existing code maybe racy, I'm not sure that this change
> > keeps the semantics; the channel may well have dup()'d the fd's for the
> > two directions, and I'm not convinced that a shutdown() on one will
> > necessarily impact the other; and if the shutdown doesn't happen the
> > rp_thread might not exit, and we might block on the koin.
> 
> dup() seems fine as long as the backend file/socket is the same; but I get the
> point here, e.g., potentially from/to channels can indeed be different ones.
> 
> > 
> > Why don't we solve this a different way - how about we move the:
> >     ms->rp_state.from_dst_file = NULL;
> >     qemu_fclose(rp);
> > 
> > out of the source_return_path_thread and put it in
> > await_return_path_close_on_source, immediately after the join?
> > Then we *know* that the the rp thread isn't messing with it.
> 
> Yes that looks working for this special case of when rp_thread quits.
> 
> It's just that it'll make things a bit more complicated, previously
> from_dst_file is only reset in rp_thread (for either paused or completed
> migration), now we handle "migration completes" a bit different.
> 
> This also reminded me that maybe we can also use the qemu_file_lock mutex as we
> use to try protect access to to_dst_file, because I think from_dst_file is
> potentially racy in the same way.  Even if we moved the reset to migration
> thread, we still have e.g. migrate_fd_cancel() calling:
> 
>     if (s->rp_state.from_dst_file) {
>         qemu_file_shutdown(s->rp_state.from_dst_file);
>     }
> 
> I think that is also racy too when running in the main thread, as either the
> migration thread or rp_thread could have reset it right after the check.
> 
> So.. maybe I start to use the qemu_file_lock to cover from_dst_file cases too?
> Then I'll cover at leasd both migrate_fd_cancel() and this case.

Yes, I think that's best; but try to do as little as possible with the
lock held.

Dave

> Thanks,
> 
> -- 
> Peter Xu
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 2/5] migration: Shutdown src in await_return_path_close_on_source()

[Peter Xu]

On Wed, Jul 21, 2021 at 07:00:24PM +0100, Dr. David Alan Gilbert wrote:
> Yes, I think that's best; but try to do as little as possible with the
> lock held.

Yep; non-blocking for the OOB thread.  Will do.

-- 
Peter Xu