[Qemu-devel] [PATCH RFC 0/6] migration: enable return-path for precopy

[Qemu-devel] [PATCH RFC 0/6] migration: enable return-path for precopy

[Peter Xu]

This idea derives from the bug reported:

  https://bugzilla.redhat.com/show_bug.cgi?id=1439147

It is not a extremely critical bug, since as long as the user uses
libvirt to migrate, we won't have such an issue at all (we'll have
identical command line parameters for QEMU). However it can be an
severe one, since it caused data loss (source VM will quit assuming
that destination VM is good, while it is not). So it would be good we
fix it. Meanwhile, imho the return path may be used for other things
as well in the future.

This series proposed a solution for the bug - let's enable return path
even for precopy when possible, then source can know whether dest is
good or not, then it can decide to do something better than quit.

The series is marked as RFC for two reasons:

One point for RFC is the idea in general that whether we would like to
enable return path for precopy. I see it okay since after all it's
optional (so it greatly reduces the chance that QEMU command line user
will lose the data, and this feature will be automatically off when we
are using e.g. "exec:" to migrate to a file).

The other one point is that I converted MigrationState into a QObject
(actually a QDevice) to let migration codes benefits from some general
framework advantages.

I got at least one positive feedback on each of the RFC elements
mentioned above, so dare I post this to public. Though I guess some
migration reviewers may be on PTO recently (I just know it :), I still
decided to post this out for a broader rfc review.

Smoke test done, and of course I verified this series to fix the bug
mentioned.

Please review. Thanks.

Peter Xu (6):
  io: only allow return path for socket typed
  migration: isolate return path on src
  migration: fix leak of src file on dst
  migration: shut src return path unconditionally
  migration: let MigrationState be an QObject
  migration: enable return path for precopy

 include/hw/compat.h           |   4 ++
 include/io/channel.h          |   1 +
 include/migration/migration.h |  27 ++++++++-
 include/qemu/typedefs.h       |   1 -
 io/channel-socket.c           |   1 +
 migration/migration.c         | 133 +++++++++++++++++++++++++++++++-----------
 migration/postcopy-ram.c      |   1 -
 migration/qemu-file-channel.c |   9 +++
 migration/trace-events        |   4 +-
 9 files changed, 140 insertions(+), 41 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH RFC 1/6] io: only allow return path for socket typed

[Peter Xu]

We don't really have a return path for the other types yet. Let's check
this when .get_return_path() is called.

For this, we introduce a new feature bit, and set it up only for socket
typed IO channels.

This will help detect earlier failure for postcopy, e.g., logically
speaking postcopy cannot work with "exec:". Before this patch, when we
try to migrate with "migrate -d exec:cat>out", we'll hang the system.
With this patch, we'll get:

(qemu) migrate -d exec:cat>out
Unable to open return-path for postcopy

CC: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/io/channel.h          | 1 +
 io/channel-socket.c           | 1 +
 migration/qemu-file-channel.c | 9 +++++++++
 3 files changed, 11 insertions(+)

diff --git a/include/io/channel.h b/include/io/channel.h
index db9bb02..7876534 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -45,6 +45,7 @@ enum QIOChannelFeature {
     QIO_CHANNEL_FEATURE_FD_PASS,
     QIO_CHANNEL_FEATURE_SHUTDOWN,
     QIO_CHANNEL_FEATURE_LISTEN,
+    QIO_CHANNEL_FEATURE_RETURN_PATH,
 };
 
 
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 53386b7..ee81b2d 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -56,6 +56,7 @@ qio_channel_socket_new(void)
 
     ioc = QIO_CHANNEL(sioc);
     qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
+    qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_RETURN_PATH);
 
 #ifdef WIN32
     ioc->event = CreateEvent(NULL, FALSE, FALSE, NULL);
diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
index 45c13f1..3bd7940 100644
--- a/migration/qemu-file-channel.c
+++ b/migration/qemu-file-channel.c
@@ -23,6 +23,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qom/object.h"
 #include "migration/qemu-file.h"
 #include "io/channel-socket.h"
 #include "qemu/iov.h"
@@ -139,6 +140,10 @@ static QEMUFile *channel_get_input_return_path(void *opaque)
 {
     QIOChannel *ioc = QIO_CHANNEL(opaque);
 
+    if (!qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_RETURN_PATH)) {
+        return NULL;
+    }
+
     return qemu_fopen_channel_output(ioc);
 }
 
@@ -146,6 +151,10 @@ static QEMUFile *channel_get_output_return_path(void *opaque)
 {
     QIOChannel *ioc = QIO_CHANNEL(opaque);
 
+    if (!qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_RETURN_PATH)) {
+        return NULL;
+    }
+
     return qemu_fopen_channel_input(ioc);
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH RFC 2/6] migration: isolate return path on src

[Peter Xu]

There are some places that binded "return path" with postcopy. Let's be
prepared for its usage even without postcopy. This patch mainly did this
on source side.

This has no functional change. But it'll simplify further patches.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c  | 50 +++++++++++++++++++++++++++++++++++---------------
 migration/trace-events |  4 ++--
 2 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 0304c01..e4f4c8c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1817,13 +1817,12 @@ static void migration_completion(MigrationState *s, int current_active_state,
      * cleaning everything else up (since if there are no failures
      * it will wait for the destination to send it's status in
      * a SHUT command).
-     * Postcopy opens rp if enabled (even if it's not avtivated)
      */
-    if (migrate_postcopy_ram()) {
+    if (s->rp_state.from_dst_file) {
         int rp_error;
-        trace_migration_completion_postcopy_end_before_rp();
+        trace_migration_return_path_end_before();
         rp_error = await_return_path_close_on_source(s);
-        trace_migration_completion_postcopy_end_after_rp(rp_error);
+        trace_migration_return_path_end_after(rp_error);
         if (rp_error) {
             goto fail_invalidate;
         }
@@ -1898,13 +1897,15 @@ static void *migration_thread(void *opaque)
 
     qemu_savevm_state_header(s->to_dst_file);
 
-    if (migrate_postcopy_ram()) {
+    if (s->to_dst_file) {
         /* Now tell the dest that it should open its end so it can reply */
         qemu_savevm_send_open_return_path(s->to_dst_file);
 
         /* And do a ping that will make stuff easier to debug */
         qemu_savevm_send_ping(s->to_dst_file, 1);
+    }
 
+    if (migrate_postcopy_ram()) {
         /*
          * Tell the destination that we *might* want to do postcopy later;
          * if the other end can't do postcopy it should fail now, nice and
@@ -2044,6 +2045,29 @@ static void *migration_thread(void *opaque)
     return NULL;
 }
 
+/* Return true if success, otherwise false. */
+static bool migrate_return_path_create(MigrationState *s)
+{
+    /* Whether we should enable return path */
+    bool enable_return_path = false;
+    /* Whether we should force its success */
+    bool force_return_path = false;
+
+    if (migrate_postcopy_ram()) {
+        enable_return_path = true;
+        force_return_path = true;
+    }
+
+    if (enable_return_path) {
+        if (open_return_path_on_source(s) && force_return_path) {
+            error_report("Unable to open return-path");
+            return false;
+        }
+    }
+
+    return true;
+}
+
 void migrate_fd_connect(MigrationState *s)
 {
     s->expected_downtime = s->parameters.downtime_limit;
@@ -2057,17 +2081,13 @@ void migrate_fd_connect(MigrationState *s)
     notifier_list_notify(&migration_state_notifiers, s);
 
     /*
-     * Open the return path; currently for postcopy but other things might
-     * also want it.
+     * Open the return path.
      */
-    if (migrate_postcopy_ram()) {
-        if (open_return_path_on_source(s)) {
-            error_report("Unable to open return-path for postcopy");
-            migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
-                              MIGRATION_STATUS_FAILED);
-            migrate_fd_cleanup(s);
-            return;
-        }
+    if (!migrate_return_path_create(s)) {
+        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
+                          MIGRATION_STATUS_FAILED);
+        migrate_fd_cleanup(s);
+        return;
     }
 
     migrate_compress_threads_create();
diff --git a/migration/trace-events b/migration/trace-events
index 5b8ccf3..38345be 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -88,8 +88,8 @@ migrate_send_rp_message(int msg_type, uint16_t len) "%d: len %d"
 migration_completion_file_err(void) ""
 migration_completion_postcopy_end(void) ""
 migration_completion_postcopy_end_after_complete(void) ""
-migration_completion_postcopy_end_before_rp(void) ""
-migration_completion_postcopy_end_after_rp(int rp_error) "%d"
+migration_return_path_end_before(void) ""
+migration_return_path_end_after(int rp_error) "%d"
 migration_thread_after_loop(void) ""
 migration_thread_file_err(void) ""
 migration_thread_setup_complete(void) ""
-- 
2.7.4

[Qemu-devel] [PATCH RFC 3/6] migration: fix leak of src file on dst

[Peter Xu]

The return path channel is possibly leaked. Fix it.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index e4f4c8c..92617fc 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -130,6 +130,11 @@ void migration_incoming_state_destroy(void)
 {
     struct MigrationIncomingState *mis = migration_incoming_get_current();
 
+    if (mis->to_src_file) {
+        qemu_fclose(mis->to_src_file);
+        mis->to_src_file = NULL;
+    }
+
     qemu_event_destroy(&mis->main_thread_load_event);
     loadvm_free_handlers(mis);
 }
-- 
2.7.4

[Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally

[Peter Xu]

We were do the shutting off only for postcopy. Now we do this as long as
the source return path is there.

Moving the cleanup of from_src_file there too.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c    | 8 +++++++-
 migration/postcopy-ram.c | 1 -
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 92617fc..a4006b4 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -131,10 +131,17 @@ void migration_incoming_state_destroy(void)
     struct MigrationIncomingState *mis = migration_incoming_get_current();
 
     if (mis->to_src_file) {
+        /* Tell source that we are done */
+        migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 0);
         qemu_fclose(mis->to_src_file);
         mis->to_src_file = NULL;
     }
 
+    if (mis->from_src_file) {
+        qemu_fclose(mis->from_src_file);
+        mis->from_src_file = NULL;
+    }
+
     qemu_event_destroy(&mis->main_thread_load_event);
     loadvm_free_handlers(mis);
 }
@@ -433,7 +440,6 @@ static void process_incoming_migration_co(void *opaque)
         exit(EXIT_FAILURE);
     }
 
-    qemu_fclose(f);
     free_xbzrle_decoded_buf();
 
     mis->bh = qemu_bh_new(process_incoming_migration_bh, mis);
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index a0489f6..57aa208 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -320,7 +320,6 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
     }
 
     postcopy_state_set(POSTCOPY_INCOMING_END);
-    migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 0);
 
     if (mis->postcopy_tmp_page) {
         munmap(mis->postcopy_tmp_page, mis->largest_page_size);
-- 
2.7.4

[Qemu-devel] [PATCH RFC 5/6] migration: let MigrationState be an QObject

[Peter Xu]

Let the old man "MigrationState" join the object family. Direct benefit
is that we can start to use all the property features derived from
current QObject, like: HW_COMPAT_* bits, command line setup for
migration parameters (so will never need to set them up each time using
HMP/QMP, this is really, really attractive for test writters), etc.

I see no reason to disallow this happen yet. So let's start from this
one, to see whether it would be anything good.

No functional change at all.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/migration.h | 24 +++++++++++++++---
 include/qemu/typedefs.h       |  1 -
 migration/migration.c         | 57 +++++++++++++++++++++++++++++--------------
 3 files changed, 60 insertions(+), 22 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 49ec501..70710de 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -22,6 +22,7 @@
 #include "qapi-types.h"
 #include "exec/cpu-common.h"
 #include "qemu/coroutine_int.h"
+#include "hw/qdev.h"
 
 #define QEMU_VM_FILE_MAGIC           0x5145564d
 #define QEMU_VM_FILE_VERSION_COMPAT  0x00000002
@@ -102,8 +103,25 @@ struct MigrationIncomingState {
 MigrationIncomingState *migration_incoming_get_current(void);
 void migration_incoming_state_destroy(void);
 
-struct MigrationState
-{
+#define TYPE_MIGRATION "migration"
+
+#define MIGRATION_CLASS(klass) \
+    OBJECT_CLASS_CHECK(MigrationClass, (klass), TYPE_MIGRATION)
+#define MIGRATION_OBJ(obj) \
+    OBJECT_CHECK(MigrationState, (obj), TYPE_MIGRATION)
+#define MIGRATION_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(MigrationClass, (obj), TYPE_MIGRATION)
+
+typedef struct MigrationClass {
+    /*< private >*/
+    DeviceClass parent_class;
+} MigrationClass;
+
+typedef struct MigrationState {
+    /*< private >*/
+    DeviceState parent_obj;
+
+    /*< public >*/
     size_t bytes_xfer;
     size_t xfer_limit;
     QemuThread thread;
@@ -153,7 +171,7 @@ struct MigrationState
 
     /* The last error that occurred */
     Error *error;
-};
+} MigrationState ;
 
 void migrate_set_state(int *state, int old_state, int new_state);
 
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 7d85057..95a382e 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -50,7 +50,6 @@ typedef struct MemoryRegionCache MemoryRegionCache;
 typedef struct MemoryRegionSection MemoryRegionSection;
 typedef struct MigrationIncomingState MigrationIncomingState;
 typedef struct MigrationParams MigrationParams;
-typedef struct MigrationState MigrationState;
 typedef struct Monitor Monitor;
 typedef struct MonitorDef MonitorDef;
 typedef struct MouseTransformInfo MouseTransformInfo;
diff --git a/migration/migration.c b/migration/migration.c
index a4006b4..6df3483 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -86,28 +86,14 @@ static bool deferred_incoming;
 MigrationState *migrate_get_current(void)
 {
     static bool once;
-    static MigrationState current_migration = {
-        .state = MIGRATION_STATUS_NONE,
-        .xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE,
-        .mbps = -1,
-        .parameters = {
-            .compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,
-            .compress_threads = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
-            .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
-            .cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
-            .cpu_throttle_increment = DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
-            .max_bandwidth = MAX_THROTTLE,
-            .downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
-            .x_checkpoint_delay = DEFAULT_MIGRATE_X_CHECKPOINT_DELAY,
-        },
-    };
+    static MigrationState *current_migration;
 
     if (!once) {
-        current_migration.parameters.tls_creds = g_strdup("");
-        current_migration.parameters.tls_hostname = g_strdup("");
+        current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
         once = true;
     }
-    return &current_migration;
+
+    return current_migration;
 }
 
 MigrationIncomingState *migration_incoming_get_current(void)
@@ -2107,3 +2093,38 @@ void migrate_fd_connect(MigrationState *s)
     s->migration_thread_running = true;
 }
 
+static void migration_instance_init(Object *obj)
+{
+    MigrationState *ms = MIGRATION_OBJ(obj);
+
+    ms->state = MIGRATION_STATUS_NONE;
+    ms->xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE;
+    ms->mbps = -1;
+    ms->parameters = (MigrationParameters) {
+        .compress_level = DEFAULT_MIGRATE_COMPRESS_LEVEL,
+        .compress_threads = DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT,
+        .decompress_threads = DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT,
+        .cpu_throttle_initial = DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL,
+        .cpu_throttle_increment = DEFAULT_MIGRATE_CPU_THROTTLE_INCREMENT,
+        .max_bandwidth = MAX_THROTTLE,
+        .downtime_limit = DEFAULT_MIGRATE_SET_DOWNTIME,
+        .x_checkpoint_delay = DEFAULT_MIGRATE_X_CHECKPOINT_DELAY,
+    };
+    ms->parameters.tls_creds = g_strdup("");
+    ms->parameters.tls_hostname = g_strdup("");
+}
+
+static const TypeInfo migration_type = {
+    .name = TYPE_MIGRATION,
+    .parent = TYPE_DEVICE,
+    .class_size = sizeof(MigrationClass),
+    .instance_size = sizeof(MigrationState),
+    .instance_init = migration_instance_init,
+};
+
+static void register_migration_types(void)
+{
+    type_register_static(&migration_type);
+}
+
+type_init(register_migration_types);
-- 
2.7.4

[Qemu-devel] [PATCH RFC 6/6] migration: enable return path for precopy

[Peter Xu]

Let this be a flag, default to on. Turn it off for <=2.9 versions.

After this patch, return path will be on even for pre-copy migration as
long as the transport support, e.g., for socket typed transport
including "tcp|udp|unix" typed.

This will naturally fix the bug mentioned below, when destination failed
on migration but source assumed it was successful - since now even for
precopy, source will wait for destination's MIG_RP_MSG_SHUT signal,
which will carry the final migration status of destination. Then, when
destination failed at any point of migration, source will know it, and
it'll resume the VM instead of a data lost.

Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1439147
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/hw/compat.h           |  4 ++++
 include/migration/migration.h |  3 +++
 migration/migration.c         | 15 ++++++++++++++-
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/include/hw/compat.h b/include/hw/compat.h
index 55b1765..049457b 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -6,6 +6,10 @@
         .driver   = "pci-bridge",\
         .property = "shpc",\
         .value    = "off",\
+    },{\
+        .driver   = "migration",\
+        .property = "return-path",\
+        .value    = "off",\
     },
 
 #define HW_COMPAT_2_8 \
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 70710de..e44119c 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -169,6 +169,9 @@ typedef struct MigrationState {
     int64_t colo_checkpoint_time;
     QEMUTimer *colo_delay_timer;
 
+    /* Whether to try to enable return-path even for pre-copy */
+    bool enable_return_path;
+
     /* The last error that occurred */
     Error *error;
 } MigrationState ;
diff --git a/migration/migration.c b/migration/migration.c
index 6df3483..16a856a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2046,7 +2046,7 @@ static void *migration_thread(void *opaque)
 static bool migrate_return_path_create(MigrationState *s)
 {
     /* Whether we should enable return path */
-    bool enable_return_path = false;
+    bool enable_return_path = s->enable_return_path;
     /* Whether we should force its success */
     bool force_return_path = false;
 
@@ -2114,9 +2114,22 @@ static void migration_instance_init(Object *obj)
     ms->parameters.tls_hostname = g_strdup("");
 }
 
+static Property migration_properties[] = {
+    DEFINE_PROP_BOOL("return-path", MigrationState, enable_return_path, true),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void migration_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->props = migration_properties;
+}
+
 static const TypeInfo migration_type = {
     .name = TYPE_MIGRATION,
     .parent = TYPE_DEVICE,
+    .class_init = migration_class_init,
     .class_size = sizeof(MigrationClass),
     .instance_size = sizeof(MigrationState),
     .instance_init = migration_instance_init,
-- 
2.7.4

Re: [Qemu-devel] [PATCH RFC 0/6] migration: enable return-path for precopy

[Peter Xu]

On Fri, May 19, 2017 at 02:43:26PM +0800, Peter Xu wrote:
> This idea derives from the bug reported:
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=1439147
> 
> It is not a extremely critical bug, since as long as the user uses
> libvirt to migrate, we won't have such an issue at all (we'll have
> identical command line parameters for QEMU). However it can be an
> severe one, since it caused data loss (source VM will quit assuming
> that destination VM is good, while it is not). So it would be good we
> fix it. Meanwhile, imho the return path may be used for other things
> as well in the future.
> 
> This series proposed a solution for the bug - let's enable return path
> even for precopy when possible, then source can know whether dest is
> good or not, then it can decide to do something better than quit.
> 
> The series is marked as RFC for two reasons:
> 
> One point for RFC is the idea in general that whether we would like to
> enable return path for precopy. I see it okay since after all it's
> optional (so it greatly reduces the chance that QEMU command line user
> will lose the data, and this feature will be automatically off when we
> are using e.g. "exec:" to migrate to a file).
> 
> The other one point is that I converted MigrationState into a QObject
> (actually a QDevice) to let migration codes benefits from some general
> framework advantages.
> 
> I got at least one positive feedback on each of the RFC elements
> mentioned above, so dare I post this to public. Though I guess some
> migration reviewers may be on PTO recently (I just know it :), I still
> decided to post this out for a broader rfc review.
> 
> Smoke test done, and of course I verified this series to fix the bug
> mentioned.
> 
> Please review. Thanks.
> 
> Peter Xu (6):
>   io: only allow return path for socket typed
>   migration: isolate return path on src
>   migration: fix leak of src file on dst
>   migration: shut src return path unconditionally
>   migration: let MigrationState be an QObject
>   migration: enable return path for precopy
> 
>  include/hw/compat.h           |   4 ++
>  include/io/channel.h          |   1 +
>  include/migration/migration.h |  27 ++++++++-
>  include/qemu/typedefs.h       |   1 -
>  io/channel-socket.c           |   1 +
>  migration/migration.c         | 133 +++++++++++++++++++++++++++++++-----------
>  migration/postcopy-ram.c      |   1 -
>  migration/qemu-file-channel.c |   9 +++
>  migration/trace-events        |   4 +-
>  9 files changed, 140 insertions(+), 41 deletions(-)
> 
> -- 
> 2.7.4
> 

CC Laurent as well. Sorry!

-- 
Peter Xu

Re: [Qemu-devel] [PATCH RFC 1/6] io: only allow return path for socket typed

[Daniel P. Berrange]

On Fri, May 19, 2017 at 02:43:27PM +0800, Peter Xu wrote:
> We don't really have a return path for the other types yet. Let's check
> this when .get_return_path() is called.
> 
> For this, we introduce a new feature bit, and set it up only for socket
> typed IO channels.
> 
> This will help detect earlier failure for postcopy, e.g., logically
> speaking postcopy cannot work with "exec:". Before this patch, when we
> try to migrate with "migrate -d exec:cat>out", we'll hang the system.
> With this patch, we'll get:
> 
> (qemu) migrate -d exec:cat>out
> Unable to open return-path for postcopy

This is wrong - post-copy migration *can* work with exec: - it just entirely
depends on what command you are running. Your example ran a command which is
unidirectional, but if you ran 'exec:socat ...' you would have a fully
bidirectional channel. Actually the channel is always bi-directional, but
'cat' simply won't ever send data back to QEMU.

If QEMU hangs when the other end doesn't send data back, that actually seems
like a potentially serious bug in migration code. Even if using the normal
'tcp' migration protocol, if the target QEMU server hangs and fails to
send data to QEMU on the return path, the source QEMU must never hang.

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: [Qemu-devel] [PATCH RFC 1/6] io: only allow return path for socket typed

[Daniel P. Berrange]

On Fri, May 19, 2017 at 09:25:38AM +0100, Daniel P. Berrange wrote:
> On Fri, May 19, 2017 at 02:43:27PM +0800, Peter Xu wrote:
> > We don't really have a return path for the other types yet. Let's check
> > this when .get_return_path() is called.
> > 
> > For this, we introduce a new feature bit, and set it up only for socket
> > typed IO channels.
> > 
> > This will help detect earlier failure for postcopy, e.g., logically
> > speaking postcopy cannot work with "exec:". Before this patch, when we
> > try to migrate with "migrate -d exec:cat>out", we'll hang the system.
> > With this patch, we'll get:
> > 
> > (qemu) migrate -d exec:cat>out
> > Unable to open return-path for postcopy
> 
> This is wrong - post-copy migration *can* work with exec: - it just entirely
> depends on what command you are running. Your example ran a command which is
> unidirectional, but if you ran 'exec:socat ...' you would have a fully
> bidirectional channel. Actually the channel is always bi-directional, but
> 'cat' simply won't ever send data back to QEMU.
> 
> If QEMU hangs when the other end doesn't send data back, that actually seems
> like a potentially serious bug in migration code. Even if using the normal
> 'tcp' migration protocol, if the target QEMU server hangs and fails to
> send data to QEMU on the return path, the source QEMU must never hang.

BTW, if you want to simplify the code in this area at all, then arguably
we should get rid of the "get_return_path" helper method entirely. We're
not actually opening any new connections - we're just creating a second
QEMUFile that uses the same underlying QIOChannel object. All we would
need is for the QEMUFile to have a separate 'buf' field management in
QEMUFile for the read & write directions.  Then all the code would be
able to just use the single QEMUFile for read & write getting rid of this
concept of "opening a return path" which doens't actually do anything at
the underlying data transport level.

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: [Qemu-devel] [PATCH RFC 1/6] io: only allow return path for socket typed

[Peter Xu]

On Fri, May 19, 2017 at 09:25:38AM +0100, Daniel P. Berrange wrote:
> On Fri, May 19, 2017 at 02:43:27PM +0800, Peter Xu wrote:
> > We don't really have a return path for the other types yet. Let's check
> > this when .get_return_path() is called.
> > 
> > For this, we introduce a new feature bit, and set it up only for socket
> > typed IO channels.
> > 
> > This will help detect earlier failure for postcopy, e.g., logically
> > speaking postcopy cannot work with "exec:". Before this patch, when we
> > try to migrate with "migrate -d exec:cat>out", we'll hang the system.
> > With this patch, we'll get:
> > 
> > (qemu) migrate -d exec:cat>out
> > Unable to open return-path for postcopy
> 
> This is wrong - post-copy migration *can* work with exec: - it just entirely
> depends on what command you are running. Your example ran a command which is
> unidirectional, but if you ran 'exec:socat ...' you would have a fully
> bidirectional channel. Actually the channel is always bi-directional, but
> 'cat' simply won't ever send data back to QEMU.

Indeed. I should not block postcopy if the user used a TCP tunnel
between the source and destination in some way, using this exec: way.
Thanks for pointing that out.

However I still think the idea is needed here. Say, we'd better know
whether the transport would be able to respond (though current
approach of "assuming sockets are the only ones that can reply" is not
a good solution...). Please see below.

> 
> If QEMU hangs when the other end doesn't send data back, that actually seems
> like a potentially serious bug in migration code. Even if using the normal
> 'tcp' migration protocol, if the target QEMU server hangs and fails to
> send data to QEMU on the return path, the source QEMU must never hang.

Firstly I should not say it's a hang - it's actually by-design here
imho - migration thread is in the last phase now, waiting for a SHUT
message from destination (which I think is wise). But from the
behavior, indeed src VM is not usable during the time, just like what
happened for most postcopy cases on the source side. So, we can see
that postcopy "assumes" that destination side can reply now.

Meanwhile, I see it reasonable for postcopy to have such an
assumption. After all, postcopy means "start VM on destination before
pages are moved over completely", then there must be someone to reply
to source, no matter whether it'll be via some kind of io channel.

That's why I think we still need the general idea here, that we need
to know whether destination end is able to reply.

But, I still have no good idea (after knowing this patch won't work)
on how we can do this... Any further suggestions would be greatly
welcomed.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH RFC 1/6] io: only allow return path for socket typed

[Peter Xu]

On Fri, May 19, 2017 at 09:30:10AM +0100, Daniel P. Berrange wrote:
> On Fri, May 19, 2017 at 09:25:38AM +0100, Daniel P. Berrange wrote:
> > On Fri, May 19, 2017 at 02:43:27PM +0800, Peter Xu wrote:
> > > We don't really have a return path for the other types yet. Let's check
> > > this when .get_return_path() is called.
> > > 
> > > For this, we introduce a new feature bit, and set it up only for socket
> > > typed IO channels.
> > > 
> > > This will help detect earlier failure for postcopy, e.g., logically
> > > speaking postcopy cannot work with "exec:". Before this patch, when we
> > > try to migrate with "migrate -d exec:cat>out", we'll hang the system.
> > > With this patch, we'll get:
> > > 
> > > (qemu) migrate -d exec:cat>out
> > > Unable to open return-path for postcopy
> > 
> > This is wrong - post-copy migration *can* work with exec: - it just entirely
> > depends on what command you are running. Your example ran a command which is
> > unidirectional, but if you ran 'exec:socat ...' you would have a fully
> > bidirectional channel. Actually the channel is always bi-directional, but
> > 'cat' simply won't ever send data back to QEMU.
> > 
> > If QEMU hangs when the other end doesn't send data back, that actually seems
> > like a potentially serious bug in migration code. Even if using the normal
> > 'tcp' migration protocol, if the target QEMU server hangs and fails to
> > send data to QEMU on the return path, the source QEMU must never hang.
> 
> BTW, if you want to simplify the code in this area at all, then arguably
> we should get rid of the "get_return_path" helper method entirely. We're
> not actually opening any new connections - we're just creating a second
> QEMUFile that uses the same underlying QIOChannel object. All we would
> need is for the QEMUFile to have a separate 'buf' field management in
> QEMUFile for the read & write directions.  Then all the code would be
> able to just use the single QEMUFile for read & write getting rid of this
> concept of "opening a return path" which doens't actually do anything at
> the underlying data transport level.

Makes sense. Noted. Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH RFC 1/6] io: only allow return path for socket typed

[Daniel P. Berrange]

On Fri, May 19, 2017 at 05:51:43PM +0800, Peter Xu wrote:
> On Fri, May 19, 2017 at 09:25:38AM +0100, Daniel P. Berrange wrote:
> > On Fri, May 19, 2017 at 02:43:27PM +0800, Peter Xu wrote:
> > > We don't really have a return path for the other types yet. Let's check
> > > this when .get_return_path() is called.
> > > 
> > > For this, we introduce a new feature bit, and set it up only for socket
> > > typed IO channels.
> > > 
> > > This will help detect earlier failure for postcopy, e.g., logically
> > > speaking postcopy cannot work with "exec:". Before this patch, when we
> > > try to migrate with "migrate -d exec:cat>out", we'll hang the system.
> > > With this patch, we'll get:
> > > 
> > > (qemu) migrate -d exec:cat>out
> > > Unable to open return-path for postcopy
> > 
> > This is wrong - post-copy migration *can* work with exec: - it just entirely
> > depends on what command you are running. Your example ran a command which is
> > unidirectional, but if you ran 'exec:socat ...' you would have a fully
> > bidirectional channel. Actually the channel is always bi-directional, but
> > 'cat' simply won't ever send data back to QEMU.
> 
> Indeed. I should not block postcopy if the user used a TCP tunnel
> between the source and destination in some way, using this exec: way.
> Thanks for pointing that out.
> 
> However I still think the idea is needed here. Say, we'd better know
> whether the transport would be able to respond (though current
> approach of "assuming sockets are the only ones that can reply" is not
> a good solution...). Please see below.
> 
> > 
> > If QEMU hangs when the other end doesn't send data back, that actually seems
> > like a potentially serious bug in migration code. Even if using the normal
> > 'tcp' migration protocol, if the target QEMU server hangs and fails to
> > send data to QEMU on the return path, the source QEMU must never hang.
> 
> Firstly I should not say it's a hang - it's actually by-design here
> imho - migration thread is in the last phase now, waiting for a SHUT
> message from destination (which I think is wise). But from the
> behavior, indeed src VM is not usable during the time, just like what
> happened for most postcopy cases on the source side. So, we can see
> that postcopy "assumes" that destination side can reply now.
> 
> Meanwhile, I see it reasonable for postcopy to have such an
> assumption. After all, postcopy means "start VM on destination before
> pages are moved over completely", then there must be someone to reply
> to source, no matter whether it'll be via some kind of io channel.
> 
> That's why I think we still need the general idea here, that we need
> to know whether destination end is able to reply.
> 
> But, I still have no good idea (after knowing this patch won't work)
> on how we can do this... Any further suggestions would be greatly
> welcomed.

IMHO this is nothing more than a documentation issue for the 'exec'
protocol. ie, document that you should provide a bi-directional
transport for live migration.

A uni-directional transport is arguably only valid if you're using
migrate to save/restore the VM state to a file.

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: [Qemu-devel] [PATCH RFC 1/6] io: only allow return path for socket typed

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berrange@redhat.com) wrote:
> On Fri, May 19, 2017 at 02:43:27PM +0800, Peter Xu wrote:
> > We don't really have a return path for the other types yet. Let's check
> > this when .get_return_path() is called.
> > 
> > For this, we introduce a new feature bit, and set it up only for socket
> > typed IO channels.
> > 
> > This will help detect earlier failure for postcopy, e.g., logically
> > speaking postcopy cannot work with "exec:". Before this patch, when we
> > try to migrate with "migrate -d exec:cat>out", we'll hang the system.
> > With this patch, we'll get:
> > 
> > (qemu) migrate -d exec:cat>out
> > Unable to open return-path for postcopy
> 
> This is wrong - post-copy migration *can* work with exec: - it just entirely
> depends on what command you are running. Your example ran a command which is
> unidirectional, but if you ran 'exec:socat ...' you would have a fully
> bidirectional channel. Actually the channel is always bi-directional, but
> 'cat' simply won't ever send data back to QEMU.

The thing is it didn't used to be able to; prior to your conversion to
channel, postcopy would reject being started with exec: because it
couldn't open a return path, so it was safe.

> If QEMU hangs when the other end doesn't send data back, that actually seems
> like a potentially serious bug in migration code. Even if using the normal
> 'tcp' migration protocol, if the target QEMU server hangs and fails to
> send data to QEMU on the return path, the source QEMU must never hang.

Hmm, we shouldn't get a 'hang' with a postcopy on a link without a
return path; but it does depend on how the exec: behaves on the
destination.
If the destination discards data written to it, then I think the
behaviour would be:
   a) Page requests would just get dropped, they'd eventually get
fulfilled by the background page transmissions, but that could mean that
a page request would wait for minutes for the page.
   b) The qemu main thread on the destination can be blocked by that, so
the monitor might not respond until the page request is fulfilled.
   c) I'm not quite sure what would happen to the source return-path
thread

The behaviour seems to have changed between 2.9.0 (f26 package) and my
reasonably recent head build.

2.9.0 gives me:
(qemu) migrate_set_speed 1B
(qemu) migrate_set_capability postcopy-ram on
(qemu) migrate -d "exec:cat > out"
RP: Received invalid message 0x0000 length 0x0000
(qemu) info migrate
capabilities: xbzrle: off rdma-pin-all: off auto-converge: off zero-blocks: off compress: off events: off postcopy-ram: on x-colo: off release-ram: off 
Migration status: failed
total time: 0 milliseconds

So that's the return path thread trying to read from the exec: not
getting anything and failing.

On head-ish it doesn't fail, the source qemu doesn't hang, however
the migration never completes - possibly because it's waiting for
the MIG_RP_MSG_SHUT from the destination.
A migration_cancel also doesn't work for 'exec' because it doesn't
support shutdown() - it just sticks in 'cancelling'.
On a socket that was broken like this the cancel would work because
it issues a shutdown() which causes the socket to cleanup.

Personally I'd rather fix this by still not supporting exec:,
making shutdown() work on exec (by kill'ing the child process)
means at least cancel would work, but it still wouldn't be pretty
for a postcopy, and still doesn't help Peter solve this problem
which is a nasty problem QEMU has had for ages.

Dave

> 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 :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH RFC 1/6] io: only allow return path for socket typed

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berrange@redhat.com) wrote:
> On Fri, May 19, 2017 at 09:25:38AM +0100, Daniel P. Berrange wrote:
> > On Fri, May 19, 2017 at 02:43:27PM +0800, Peter Xu wrote:
> > > We don't really have a return path for the other types yet. Let's check
> > > this when .get_return_path() is called.
> > > 
> > > For this, we introduce a new feature bit, and set it up only for socket
> > > typed IO channels.
> > > 
> > > This will help detect earlier failure for postcopy, e.g., logically
> > > speaking postcopy cannot work with "exec:". Before this patch, when we
> > > try to migrate with "migrate -d exec:cat>out", we'll hang the system.
> > > With this patch, we'll get:
> > > 
> > > (qemu) migrate -d exec:cat>out
> > > Unable to open return-path for postcopy
> > 
> > This is wrong - post-copy migration *can* work with exec: - it just entirely
> > depends on what command you are running. Your example ran a command which is
> > unidirectional, but if you ran 'exec:socat ...' you would have a fully
> > bidirectional channel. Actually the channel is always bi-directional, but
> > 'cat' simply won't ever send data back to QEMU.
> > 
> > If QEMU hangs when the other end doesn't send data back, that actually seems
> > like a potentially serious bug in migration code. Even if using the normal
> > 'tcp' migration protocol, if the target QEMU server hangs and fails to
> > send data to QEMU on the return path, the source QEMU must never hang.
> 
> BTW, if you want to simplify the code in this area at all, then arguably
> we should get rid of the "get_return_path" helper method entirely. We're
> not actually opening any new connections - we're just creating a second
> QEMUFile that uses the same underlying QIOChannel object. All we would
> need is for the QEMUFile to have a separate 'buf' field management in
> QEMUFile for the read & write directions.  Then all the code would be
> able to just use the single QEMUFile for read & write getting rid of this
> concept of "opening a return path" which doens't actually do anything at
> the underlying data transport level.

No, I'd rather keep the get_return_path;  we should keep each direction
separate.

Dave

> 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 :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH RFC 1/6] io: only allow return path for socket typed

[Daniel P. Berrange]

On Fri, May 19, 2017 at 01:51:43PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berrange@redhat.com) wrote:
> > On Fri, May 19, 2017 at 02:43:27PM +0800, Peter Xu wrote:
> > > We don't really have a return path for the other types yet. Let's check
> > > this when .get_return_path() is called.
> > > 
> > > For this, we introduce a new feature bit, and set it up only for socket
> > > typed IO channels.
> > > 
> > > This will help detect earlier failure for postcopy, e.g., logically
> > > speaking postcopy cannot work with "exec:". Before this patch, when we
> > > try to migrate with "migrate -d exec:cat>out", we'll hang the system.
> > > With this patch, we'll get:
> > > 
> > > (qemu) migrate -d exec:cat>out
> > > Unable to open return-path for postcopy
> > 
> > This is wrong - post-copy migration *can* work with exec: - it just entirely
> > depends on what command you are running. Your example ran a command which is
> > unidirectional, but if you ran 'exec:socat ...' you would have a fully
> > bidirectional channel. Actually the channel is always bi-directional, but
> > 'cat' simply won't ever send data back to QEMU.
> 
> The thing is it didn't used to be able to; prior to your conversion to
> channel, postcopy would reject being started with exec: because it
> couldn't open a return path, so it was safe.

It safe but functionally broken because it is valid to want to use
exec migration with post-copy.

> > If QEMU hangs when the other end doesn't send data back, that actually seems
> > like a potentially serious bug in migration code. Even if using the normal
> > 'tcp' migration protocol, if the target QEMU server hangs and fails to
> > send data to QEMU on the return path, the source QEMU must never hang.
> 
> Hmm, we shouldn't get a 'hang' with a postcopy on a link without a
> return path; but it does depend on how the exec: behaves on the
> destination.
> If the destination discards data written to it, then I think the
> behaviour would be:
>    a) Page requests would just get dropped, they'd eventually get
> fulfilled by the background page transmissions, but that could mean that
> a page request would wait for minutes for the page.
>    b) The qemu main thread on the destination can be blocked by that, so
> the monitor might not respond until the page request is fulfilled.
>    c) I'm not quite sure what would happen to the source return-path
> thread
> 
> The behaviour seems to have changed between 2.9.0 (f26 package) and my
> reasonably recent head build.

That's due to the bug we just fixed where we mistakenly didn't
allow bi-directional I/O for exec

  commit 062d81f0e968fe1597474735f3ea038065027372
  Author: Daniel P. Berrange <berrange@redhat.com>
  Date:   Fri Apr 21 12:12:20 2017 +0100

    migration: setup bi-directional I/O channel for exec: protocol
    
    Historically the migration data channel has only needed to be
    unidirectional. Thus the 'exec:' protocol was requesting an
    I/O channel with O_RDONLY on incoming side, and O_WRONLY on
    the outgoing side.
    
    This is fine for classic migration, but if you then try to run
    TLS over it, this fails because the TLS handshake requires a
    bi-directional channel.
    
    Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
    Reviewed-by: Juan Quintela <quintela@redhat.com>
    Signed-off-by: Juan Quintela <quintela@redhat.com>


> A migration_cancel also doesn't work for 'exec' because it doesn't
> support shutdown() - it just sticks in 'cancelling'.
> On a socket that was broken like this the cancel would work because
> it issues a shutdown() which causes the socket to cleanup.

I'm curious why migration_cancel requires shutdown() to work ? Why
isn't it sufficient from the source POV to just close the socket
entirely straight away.

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: [Qemu-devel] [PATCH RFC 1/6] io: only allow return path for socket typed

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berrange@redhat.com) wrote:
> On Fri, May 19, 2017 at 01:51:43PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrange (berrange@redhat.com) wrote:
> > > On Fri, May 19, 2017 at 02:43:27PM +0800, Peter Xu wrote:
> > > > We don't really have a return path for the other types yet. Let's check
> > > > this when .get_return_path() is called.
> > > > 
> > > > For this, we introduce a new feature bit, and set it up only for socket
> > > > typed IO channels.
> > > > 
> > > > This will help detect earlier failure for postcopy, e.g., logically
> > > > speaking postcopy cannot work with "exec:". Before this patch, when we
> > > > try to migrate with "migrate -d exec:cat>out", we'll hang the system.
> > > > With this patch, we'll get:
> > > > 
> > > > (qemu) migrate -d exec:cat>out
> > > > Unable to open return-path for postcopy
> > > 
> > > This is wrong - post-copy migration *can* work with exec: - it just entirely
> > > depends on what command you are running. Your example ran a command which is
> > > unidirectional, but if you ran 'exec:socat ...' you would have a fully
> > > bidirectional channel. Actually the channel is always bi-directional, but
> > > 'cat' simply won't ever send data back to QEMU.
> > 
> > The thing is it didn't used to be able to; prior to your conversion to
> > channel, postcopy would reject being started with exec: because it
> > couldn't open a return path, so it was safe.
> 
> It safe but functionally broken because it is valid to want to use
> exec migration with post-copy.
> 
> > > If QEMU hangs when the other end doesn't send data back, that actually seems
> > > like a potentially serious bug in migration code. Even if using the normal
> > > 'tcp' migration protocol, if the target QEMU server hangs and fails to
> > > send data to QEMU on the return path, the source QEMU must never hang.
> > 
> > Hmm, we shouldn't get a 'hang' with a postcopy on a link without a
> > return path; but it does depend on how the exec: behaves on the
> > destination.
> > If the destination discards data written to it, then I think the
> > behaviour would be:
> >    a) Page requests would just get dropped, they'd eventually get
> > fulfilled by the background page transmissions, but that could mean that
> > a page request would wait for minutes for the page.
> >    b) The qemu main thread on the destination can be blocked by that, so
> > the monitor might not respond until the page request is fulfilled.
> >    c) I'm not quite sure what would happen to the source return-path
> > thread
> > 
> > The behaviour seems to have changed between 2.9.0 (f26 package) and my
> > reasonably recent head build.
> 
> That's due to the bug we just fixed where we mistakenly didn't
> allow bi-directional I/O for exec
> 
>   commit 062d81f0e968fe1597474735f3ea038065027372
>   Author: Daniel P. Berrange <berrange@redhat.com>
>   Date:   Fri Apr 21 12:12:20 2017 +0100
> 
>     migration: setup bi-directional I/O channel for exec: protocol
>     
>     Historically the migration data channel has only needed to be
>     unidirectional. Thus the 'exec:' protocol was requesting an
>     I/O channel with O_RDONLY on incoming side, and O_WRONLY on
>     the outgoing side.
>     
>     This is fine for classic migration, but if you then try to run
>     TLS over it, this fails because the TLS handshake requires a
>     bi-directional channel.
>     
>     Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>     Reviewed-by: Juan Quintela <quintela@redhat.com>
>     Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> 
> > A migration_cancel also doesn't work for 'exec' because it doesn't
> > support shutdown() - it just sticks in 'cancelling'.
> > On a socket that was broken like this the cancel would work because
> > it issues a shutdown() which causes the socket to cleanup.
> 
> I'm curious why migration_cancel requires shutdown() to work ? Why
> isn't it sufficient from the source POV to just close the socket
> entirely straight away.

close() closes the fd so that any other uses of the fd get an
error and you're at risk of the fd getting reallocated by something
else.  So consider if the main thread calls close(), the migration
thread and the return thread both carry on using that fd, which might
have been reallocated to something different.  Worse I think we came to the
consolution that on some OSs a read()/write() blocked in the use of an fd didn't
get kicked out by a close.

shutdown() is safe, in that it stops any other threads accessing the fd
but doesn't allow it's reallocation until the close;  We perform the
close only when we've joined all other threads that were using the fd.
Any of the threads that do new calls on the fd get an error and quickly
fall down their error paths.

Dave

> 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 :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH RFC 1/6] io: only allow return path for socket typed

[Daniel P. Berrange]

On Fri, May 19, 2017 at 02:02:00PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berrange@redhat.com) wrote:
> > On Fri, May 19, 2017 at 01:51:43PM +0100, Dr. David Alan Gilbert wrote:
> > > * Daniel P. Berrange (berrange@redhat.com) wrote:
> > > > On Fri, May 19, 2017 at 02:43:27PM +0800, Peter Xu wrote:
> > > > > We don't really have a return path for the other types yet. Let's check
> > > > > this when .get_return_path() is called.
> > > > > 
> > > > > For this, we introduce a new feature bit, and set it up only for socket
> > > > > typed IO channels.
> > > > > 
> > > > > This will help detect earlier failure for postcopy, e.g., logically
> > > > > speaking postcopy cannot work with "exec:". Before this patch, when we
> > > > > try to migrate with "migrate -d exec:cat>out", we'll hang the system.
> > > > > With this patch, we'll get:
> > > > > 
> > > > > (qemu) migrate -d exec:cat>out
> > > > > Unable to open return-path for postcopy
> > > > 
> > > > This is wrong - post-copy migration *can* work with exec: - it just entirely
> > > > depends on what command you are running. Your example ran a command which is
> > > > unidirectional, but if you ran 'exec:socat ...' you would have a fully
> > > > bidirectional channel. Actually the channel is always bi-directional, but
> > > > 'cat' simply won't ever send data back to QEMU.
> > > 
> > > The thing is it didn't used to be able to; prior to your conversion to
> > > channel, postcopy would reject being started with exec: because it
> > > couldn't open a return path, so it was safe.
> > 
> > It safe but functionally broken because it is valid to want to use
> > exec migration with post-copy.
> > 
> > > > If QEMU hangs when the other end doesn't send data back, that actually seems
> > > > like a potentially serious bug in migration code. Even if using the normal
> > > > 'tcp' migration protocol, if the target QEMU server hangs and fails to
> > > > send data to QEMU on the return path, the source QEMU must never hang.
> > > 
> > > Hmm, we shouldn't get a 'hang' with a postcopy on a link without a
> > > return path; but it does depend on how the exec: behaves on the
> > > destination.
> > > If the destination discards data written to it, then I think the
> > > behaviour would be:
> > >    a) Page requests would just get dropped, they'd eventually get
> > > fulfilled by the background page transmissions, but that could mean that
> > > a page request would wait for minutes for the page.
> > >    b) The qemu main thread on the destination can be blocked by that, so
> > > the monitor might not respond until the page request is fulfilled.
> > >    c) I'm not quite sure what would happen to the source return-path
> > > thread
> > > 
> > > The behaviour seems to have changed between 2.9.0 (f26 package) and my
> > > reasonably recent head build.
> > 
> > That's due to the bug we just fixed where we mistakenly didn't
> > allow bi-directional I/O for exec
> > 
> >   commit 062d81f0e968fe1597474735f3ea038065027372
> >   Author: Daniel P. Berrange <berrange@redhat.com>
> >   Date:   Fri Apr 21 12:12:20 2017 +0100
> > 
> >     migration: setup bi-directional I/O channel for exec: protocol
> >     
> >     Historically the migration data channel has only needed to be
> >     unidirectional. Thus the 'exec:' protocol was requesting an
> >     I/O channel with O_RDONLY on incoming side, and O_WRONLY on
> >     the outgoing side.
> >     
> >     This is fine for classic migration, but if you then try to run
> >     TLS over it, this fails because the TLS handshake requires a
> >     bi-directional channel.
> >     
> >     Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> >     Reviewed-by: Juan Quintela <quintela@redhat.com>
> >     Signed-off-by: Juan Quintela <quintela@redhat.com>
> > 
> > 
> > > A migration_cancel also doesn't work for 'exec' because it doesn't
> > > support shutdown() - it just sticks in 'cancelling'.
> > > On a socket that was broken like this the cancel would work because
> > > it issues a shutdown() which causes the socket to cleanup.
> > 
> > I'm curious why migration_cancel requires shutdown() to work ? Why
> > isn't it sufficient from the source POV to just close the socket
> > entirely straight away.
> 
> close() closes the fd so that any other uses of the fd get an
> error and you're at risk of the fd getting reallocated by something
> else.  So consider if the main thread calls close(), the migration
> thread and the return thread both carry on using that fd, which might
> have been reallocated to something different.  Worse I think we came to the
> consolution that on some OSs a read()/write() blocked in the use of an fd
> didn't get kicked out by a close.

I'd be very surprised if close() didn't terminate any other threads doing
read/write, and even more surprised if it they handed out the same FD
to another thread while something was still in the read.

> shutdown() is safe, in that it stops any other threads accessing the fd
> but doesn't allow it's reallocation until the close;  We perform the
> close only when we've joined all other threads that were using the fd.
> Any of the threads that do new calls on the fd get an error and quickly
> fall down their error paths.

Ahh that's certainly an interesting scenario. That would certainly be
a problem with the migration code when this was originally written.
It had two QEMUFile structs each with an 'int fd' field, so when you
close 'fd' on one QEMUFile struct, it wouldn't update the other QEMUFile
used by another thread.

Since we switched over to use QIOChannel though, I think the thread
scenario you describe should be avoided entirely. When you have multiple
QEMUFile objects, they each have a reference counted pointer to the same
underlying QIOChannel object instance. So when QEMUFile triggers a call
to qio_channel_close() in one thread, that'll set fd=-1 in the QIOChannel.
Since the other threads have a reference to the same QIOChannel object,
they'll now see this fd == -1 straightaway.

So, IIUC, this should make the need for shutdown() redundant (at least
for the thread race conditions you describe).

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: [Qemu-devel] [PATCH RFC 1/6] io: only allow return path for socket typed

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berrange@redhat.com) wrote:
> On Fri, May 19, 2017 at 02:02:00PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrange (berrange@redhat.com) wrote:
> > > On Fri, May 19, 2017 at 01:51:43PM +0100, Dr. David Alan Gilbert wrote:
> > > > * Daniel P. Berrange (berrange@redhat.com) wrote:
> > > > > On Fri, May 19, 2017 at 02:43:27PM +0800, Peter Xu wrote:
> > > > > > We don't really have a return path for the other types yet. Let's check
> > > > > > this when .get_return_path() is called.
> > > > > > 
> > > > > > For this, we introduce a new feature bit, and set it up only for socket
> > > > > > typed IO channels.
> > > > > > 
> > > > > > This will help detect earlier failure for postcopy, e.g., logically
> > > > > > speaking postcopy cannot work with "exec:". Before this patch, when we
> > > > > > try to migrate with "migrate -d exec:cat>out", we'll hang the system.
> > > > > > With this patch, we'll get:
> > > > > > 
> > > > > > (qemu) migrate -d exec:cat>out
> > > > > > Unable to open return-path for postcopy
> > > > > 
> > > > > This is wrong - post-copy migration *can* work with exec: - it just entirely
> > > > > depends on what command you are running. Your example ran a command which is
> > > > > unidirectional, but if you ran 'exec:socat ...' you would have a fully
> > > > > bidirectional channel. Actually the channel is always bi-directional, but
> > > > > 'cat' simply won't ever send data back to QEMU.
> > > > 
> > > > The thing is it didn't used to be able to; prior to your conversion to
> > > > channel, postcopy would reject being started with exec: because it
> > > > couldn't open a return path, so it was safe.
> > > 
> > > It safe but functionally broken because it is valid to want to use
> > > exec migration with post-copy.
> > > 
> > > > > If QEMU hangs when the other end doesn't send data back, that actually seems
> > > > > like a potentially serious bug in migration code. Even if using the normal
> > > > > 'tcp' migration protocol, if the target QEMU server hangs and fails to
> > > > > send data to QEMU on the return path, the source QEMU must never hang.
> > > > 
> > > > Hmm, we shouldn't get a 'hang' with a postcopy on a link without a
> > > > return path; but it does depend on how the exec: behaves on the
> > > > destination.
> > > > If the destination discards data written to it, then I think the
> > > > behaviour would be:
> > > >    a) Page requests would just get dropped, they'd eventually get
> > > > fulfilled by the background page transmissions, but that could mean that
> > > > a page request would wait for minutes for the page.
> > > >    b) The qemu main thread on the destination can be blocked by that, so
> > > > the monitor might not respond until the page request is fulfilled.
> > > >    c) I'm not quite sure what would happen to the source return-path
> > > > thread
> > > > 
> > > > The behaviour seems to have changed between 2.9.0 (f26 package) and my
> > > > reasonably recent head build.
> > > 
> > > That's due to the bug we just fixed where we mistakenly didn't
> > > allow bi-directional I/O for exec
> > > 
> > >   commit 062d81f0e968fe1597474735f3ea038065027372
> > >   Author: Daniel P. Berrange <berrange@redhat.com>
> > >   Date:   Fri Apr 21 12:12:20 2017 +0100
> > > 
> > >     migration: setup bi-directional I/O channel for exec: protocol
> > >     
> > >     Historically the migration data channel has only needed to be
> > >     unidirectional. Thus the 'exec:' protocol was requesting an
> > >     I/O channel with O_RDONLY on incoming side, and O_WRONLY on
> > >     the outgoing side.
> > >     
> > >     This is fine for classic migration, but if you then try to run
> > >     TLS over it, this fails because the TLS handshake requires a
> > >     bi-directional channel.
> > >     
> > >     Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > >     Reviewed-by: Juan Quintela <quintela@redhat.com>
> > >     Signed-off-by: Juan Quintela <quintela@redhat.com>
> > > 
> > > 
> > > > A migration_cancel also doesn't work for 'exec' because it doesn't
> > > > support shutdown() - it just sticks in 'cancelling'.
> > > > On a socket that was broken like this the cancel would work because
> > > > it issues a shutdown() which causes the socket to cleanup.
> > > 
> > > I'm curious why migration_cancel requires shutdown() to work ? Why
> > > isn't it sufficient from the source POV to just close the socket
> > > entirely straight away.
> > 
> > close() closes the fd so that any other uses of the fd get an
> > error and you're at risk of the fd getting reallocated by something
> > else.  So consider if the main thread calls close(), the migration
> > thread and the return thread both carry on using that fd, which might
> > have been reallocated to something different.  Worse I think we came to the
> > consolution that on some OSs a read()/write() blocked in the use of an fd
> > didn't get kicked out by a close.
> 
> I'd be very surprised if close() didn't terminate any other threads doing
> read/write

Prepare to be surprised then - that's the behaviour I found when I tried
it out in 2014.
(Also at the time we found cases where the close() could hang)

> and even more surprised if it they handed out the same FD
> to another thread while something was still in the read.

Right, I don't think that will happen.

> 
> > shutdown() is safe, in that it stops any other threads accessing the fd
> > but doesn't allow it's reallocation until the close;  We perform the
> > close only when we've joined all other threads that were using the fd.
> > Any of the threads that do new calls on the fd get an error and quickly
> > fall down their error paths.
> 
> Ahh that's certainly an interesting scenario. That would certainly be
> a problem with the migration code when this was originally written.
> It had two QEMUFile structs each with an 'int fd' field, so when you
> close 'fd' on one QEMUFile struct, it wouldn't update the other QEMUFile
> used by another thread.
> 
> Since we switched over to use QIOChannel though, I think the thread
> scenario you describe should be avoided entirely. When you have multiple
> QEMUFile objects, they each have a reference counted pointer to the same
> underlying QIOChannel object instance. So when QEMUFile triggers a call
> to qio_channel_close() in one thread, that'll set fd=-1 in the QIOChannel.
> Since the other threads have a reference to the same QIOChannel object,
> they'll now see this fd == -1 straightaway.
> 
> So, IIUC, this should make the need for shutdown() redundant (at least
> for the thread race conditions you describe).

That's not thread safe unless you're doing some very careful locking.
Consider:
  T1                      T2       
     oldfd=fd               tmp=fd
     fd=-1
     close(oldfd)
     unrelated open()
                            read(tmp,...

In practice every use of fd will be a copy into a tmp and then the
syscall; the unrelated open() could happen in another thread.
(OK, the gap between the tmp and the read is tiny, although if we're
doing multiple operations chances are the compiler will optimise
it to the top of a loop).

There's no way to make that code safe.

Dave

> 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 :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH RFC 1/6] io: only allow return path for socket typed

[Daniel P. Berrange]

On Fri, May 19, 2017 at 03:33:12PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berrange@redhat.com) wrote:
> > > shutdown() is safe, in that it stops any other threads accessing the fd
> > > but doesn't allow it's reallocation until the close;  We perform the
> > > close only when we've joined all other threads that were using the fd.
> > > Any of the threads that do new calls on the fd get an error and quickly
> > > fall down their error paths.
> > 
> > Ahh that's certainly an interesting scenario. That would certainly be
> > a problem with the migration code when this was originally written.
> > It had two QEMUFile structs each with an 'int fd' field, so when you
> > close 'fd' on one QEMUFile struct, it wouldn't update the other QEMUFile
> > used by another thread.
> > 
> > Since we switched over to use QIOChannel though, I think the thread
> > scenario you describe should be avoided entirely. When you have multiple
> > QEMUFile objects, they each have a reference counted pointer to the same
> > underlying QIOChannel object instance. So when QEMUFile triggers a call
> > to qio_channel_close() in one thread, that'll set fd=-1 in the QIOChannel.
> > Since the other threads have a reference to the same QIOChannel object,
> > they'll now see this fd == -1 straightaway.
> > 
> > So, IIUC, this should make the need for shutdown() redundant (at least
> > for the thread race conditions you describe).
> 
> That's not thread safe unless you're doing some very careful locking.
> Consider:
>   T1                      T2       
>      oldfd=fd               tmp=fd
>      fd=-1
>      close(oldfd)
>      unrelated open()
>                             read(tmp,...
> 
> In practice every use of fd will be a copy into a tmp and then the
> syscall; the unrelated open() could happen in another thread.
> (OK, the gap between the tmp and the read is tiny, although if we're
> doing multiple operations chances are the compiler will optimise
> it to the top of a loop).
> 
> There's no way to make that code safe.

Urgh, yes, I see what you mean.

Currently the QIOChannelCommand implementation, uses a pair of anonymous
pipes for stdin/out to the child process. I wonder if we could switch
that to use socketpair() instead, thus letting us shutdown() on it too.

Though I guess it would be sufficient for qio_channel_shutdown() to
merely kill the child PID, while leaving the FDs open, as then you'd
get EOF and/or EPIPE on the read/writes.

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: [Qemu-devel] [PATCH RFC 1/6] io: only allow return path for socket typed

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berrange@redhat.com) wrote:
> On Fri, May 19, 2017 at 03:33:12PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrange (berrange@redhat.com) wrote:
> > > > shutdown() is safe, in that it stops any other threads accessing the fd
> > > > but doesn't allow it's reallocation until the close;  We perform the
> > > > close only when we've joined all other threads that were using the fd.
> > > > Any of the threads that do new calls on the fd get an error and quickly
> > > > fall down their error paths.
> > > 
> > > Ahh that's certainly an interesting scenario. That would certainly be
> > > a problem with the migration code when this was originally written.
> > > It had two QEMUFile structs each with an 'int fd' field, so when you
> > > close 'fd' on one QEMUFile struct, it wouldn't update the other QEMUFile
> > > used by another thread.
> > > 
> > > Since we switched over to use QIOChannel though, I think the thread
> > > scenario you describe should be avoided entirely. When you have multiple
> > > QEMUFile objects, they each have a reference counted pointer to the same
> > > underlying QIOChannel object instance. So when QEMUFile triggers a call
> > > to qio_channel_close() in one thread, that'll set fd=-1 in the QIOChannel.
> > > Since the other threads have a reference to the same QIOChannel object,
> > > they'll now see this fd == -1 straightaway.
> > > 
> > > So, IIUC, this should make the need for shutdown() redundant (at least
> > > for the thread race conditions you describe).
> > 
> > That's not thread safe unless you're doing some very careful locking.
> > Consider:
> >   T1                      T2       
> >      oldfd=fd               tmp=fd
> >      fd=-1
> >      close(oldfd)
> >      unrelated open()
> >                             read(tmp,...
> > 
> > In practice every use of fd will be a copy into a tmp and then the
> > syscall; the unrelated open() could happen in another thread.
> > (OK, the gap between the tmp and the read is tiny, although if we're
> > doing multiple operations chances are the compiler will optimise
> > it to the top of a loop).
> > 
> > There's no way to make that code safe.
> 
> Urgh, yes, I see what you mean.
> 
> Currently the QIOChannelCommand implementation, uses a pair of anonymous
> pipes for stdin/out to the child process. I wonder if we could switch
> that to use socketpair() instead, thus letting us shutdown() on it too.
> 
> Though I guess it would be sufficient for qio_channel_shutdown() to
> merely kill the child PID, while leaving the FDs open, as then you'd
> get EOF and/or EPIPE on the read/writes.

Yes, I guess it's a question of which one is more likely to actually
kill the exec child off; the socketpair is more likely to cause the
source side migration code to cancel cleanly, although a kill -9 
should sort out a wayward exec child.

Dave

> 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 :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> We were do the shutting off only for postcopy. Now we do this as long as
> the source return path is there.
> 
> Moving the cleanup of from_src_file there too.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

> ---
>  migration/migration.c    | 8 +++++++-
>  migration/postcopy-ram.c | 1 -
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 92617fc..a4006b4 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -131,10 +131,17 @@ void migration_incoming_state_destroy(void)
>      struct MigrationIncomingState *mis = migration_incoming_get_current();
>  
>      if (mis->to_src_file) {
> +        /* Tell source that we are done */
> +        migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 0);
>          qemu_fclose(mis->to_src_file);
>          mis->to_src_file = NULL;
>      }
>  
> +    if (mis->from_src_file) {
> +        qemu_fclose(mis->from_src_file);
> +        mis->from_src_file = NULL;
> +    }
> +
>      qemu_event_destroy(&mis->main_thread_load_event);
>      loadvm_free_handlers(mis);
>  }
> @@ -433,7 +440,6 @@ static void process_incoming_migration_co(void *opaque)
>          exit(EXIT_FAILURE);
>      }
>  
> -    qemu_fclose(f);
>      free_xbzrle_decoded_buf();
>  
>      mis->bh = qemu_bh_new(process_incoming_migration_bh, mis);
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index a0489f6..57aa208 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -320,7 +320,6 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
>      }
>  
>      postcopy_state_set(POSTCOPY_INCOMING_END);
> -    migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 0);
>  
>      if (mis->postcopy_tmp_page) {
>          munmap(mis->postcopy_tmp_page, mis->largest_page_size);
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH RFC 1/6] io: only allow return path for socket typed

[Daniel P. Berrange]

On Fri, May 19, 2017 at 07:41:34PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berrange@redhat.com) wrote:
> > On Fri, May 19, 2017 at 03:33:12PM +0100, Dr. David Alan Gilbert wrote:
> > > * Daniel P. Berrange (berrange@redhat.com) wrote:
> > > > > shutdown() is safe, in that it stops any other threads accessing the fd
> > > > > but doesn't allow it's reallocation until the close;  We perform the
> > > > > close only when we've joined all other threads that were using the fd.
> > > > > Any of the threads that do new calls on the fd get an error and quickly
> > > > > fall down their error paths.
> > > > 
> > > > Ahh that's certainly an interesting scenario. That would certainly be
> > > > a problem with the migration code when this was originally written.
> > > > It had two QEMUFile structs each with an 'int fd' field, so when you
> > > > close 'fd' on one QEMUFile struct, it wouldn't update the other QEMUFile
> > > > used by another thread.
> > > > 
> > > > Since we switched over to use QIOChannel though, I think the thread
> > > > scenario you describe should be avoided entirely. When you have multiple
> > > > QEMUFile objects, they each have a reference counted pointer to the same
> > > > underlying QIOChannel object instance. So when QEMUFile triggers a call
> > > > to qio_channel_close() in one thread, that'll set fd=-1 in the QIOChannel.
> > > > Since the other threads have a reference to the same QIOChannel object,
> > > > they'll now see this fd == -1 straightaway.
> > > > 
> > > > So, IIUC, this should make the need for shutdown() redundant (at least
> > > > for the thread race conditions you describe).
> > > 
> > > That's not thread safe unless you're doing some very careful locking.
> > > Consider:
> > >   T1                      T2       
> > >      oldfd=fd               tmp=fd
> > >      fd=-1
> > >      close(oldfd)
> > >      unrelated open()
> > >                             read(tmp,...
> > > 
> > > In practice every use of fd will be a copy into a tmp and then the
> > > syscall; the unrelated open() could happen in another thread.
> > > (OK, the gap between the tmp and the read is tiny, although if we're
> > > doing multiple operations chances are the compiler will optimise
> > > it to the top of a loop).
> > > 
> > > There's no way to make that code safe.
> > 
> > Urgh, yes, I see what you mean.
> > 
> > Currently the QIOChannelCommand implementation, uses a pair of anonymous
> > pipes for stdin/out to the child process. I wonder if we could switch
> > that to use socketpair() instead, thus letting us shutdown() on it too.
> > 
> > Though I guess it would be sufficient for qio_channel_shutdown() to
> > merely kill the child PID, while leaving the FDs open, as then you'd
> > get EOF and/or EPIPE on the read/writes.
> 
> Yes, I guess it's a question of which one is more likely to actually
> kill the exec child off; the socketpair is more likely to cause the
> source side migration code to cancel cleanly, although a kill -9 
> should sort out a wayward exec child.

I'm also curious if there's any real world performance difference between
using a pipe vs socketpair to communicate with a child process

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: [Qemu-devel] [PATCH RFC 2/6] migration: isolate return path on src

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> There are some places that binded "return path" with postcopy. Let's be
> prepared for its usage even without postcopy. This patch mainly did this
> on source side.
>
> This has no functional change. But it'll simplify further patches.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c  | 50 +++++++++++++++++++++++++++++++++++---------------
>  migration/trace-events |  4 ++--
>  2 files changed, 37 insertions(+), 17 deletions(-)




>  
> +/* Return true if success, otherwise false. */
> +static bool migrate_return_path_create(MigrationState *s)
> +{
> +    /* Whether we should enable return path */
> +    bool enable_return_path = false;
> +    /* Whether we should force its success */
> +    bool force_return_path = false;
> +
> +    if (migrate_postcopy_ram()) {
> +        enable_return_path = true;
> +        force_return_path = true;
> +    }
> +
> +    if (enable_return_path) {
> +        if (open_return_path_on_source(s) && force_return_path) {
> +            error_report("Unable to open return-path");
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +

what about this 

static bool migrate_return_patch_create(MigrationState *s)
{
    if (open_return_path_on_source(s)) {
        error_report("Unable to open return-path");
        return false;
    }

    return true;
}

>  void migrate_fd_connect(MigrationState *s)
>  {
>      s->expected_downtime = s->parameters.downtime_limit;
> @@ -2057,17 +2081,13 @@ void migrate_fd_connect(MigrationState *s)
>      notifier_list_notify(&migration_state_notifiers, s);
>  
>      /*
> -     * Open the return path; currently for postcopy but other things might
> -     * also want it.
> +     * Open the return path.
>       */
> -    if (migrate_postcopy_ram()) {
> -        if (open_return_path_on_source(s)) {
> -            error_report("Unable to open return-path for postcopy");
> -            migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> -                              MIGRATION_STATUS_FAILED);
> -            migrate_fd_cleanup(s);
> -            return;
> -        }
> +    if (!migrate_return_path_create(s)) {
> +        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> +                          MIGRATION_STATUS_FAILED);
> +        migrate_fd_cleanup(s);
> +        return;
>      }


And this?

>      /*
> -     * Open the return path; currently for postcopy but other things might
> -     * also want it.
> +     * Open the return path.
>       */
> -    if (migrate_postcopy_ram()) {
> -        if (open_return_path_on_source(s)) {
> -            error_report("Unable to open return-path for postcopy");
> -            migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> -                              MIGRATION_STATUS_FAILED);
> -            migrate_fd_cleanup(s);
> -            return;
> -        }
> +    if (!migrate_return_path_create(s)) {
> +        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> +                          MIGRATION_STATUS_FAILED);
> +        migrate_fd_cleanup(s);
> +        return;
>      }

    /*
     * Open the return path
     */

    if (migrate_postcopy_ram()) {
        if (!migrate_return_path_create(s)) {
            migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
                              MIGRATION_STATUS_FAILED);
            migrate_fd_cleanup(s);
            return;
        }
    }

Two less booleans and same behaviour.  It is also shorter, but that was
not the idea.

Later, Juan.

Re: [Qemu-devel] [PATCH RFC 3/6] migration: fix leak of src file on dst

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> The return path channel is possibly leaked. Fix it.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index e4f4c8c..92617fc 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -130,6 +130,11 @@ void migration_incoming_state_destroy(void)
>  {
>      struct MigrationIncomingState *mis = migration_incoming_get_current();
>  
> +    if (mis->to_src_file) {
> +        qemu_fclose(mis->to_src_file);
> +        mis->to_src_file = NULL;
> +    }
> +
>      qemu_event_destroy(&mis->main_thread_load_event);
>      loadvm_free_handlers(mis);
>  }

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> We were do the shutting off only for postcopy. Now we do this as long as
> the source return path is there.
>
> Moving the cleanup of from_src_file there too.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c    | 8 +++++++-
>  migration/postcopy-ram.c | 1 -
>  2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 92617fc..a4006b4 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -131,10 +131,17 @@ void migration_incoming_state_destroy(void)
>      struct MigrationIncomingState *mis = migration_incoming_get_current();
>  
>      if (mis->to_src_file) {
> +        /* Tell source that we are done */
> +        migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 0);

Reviewed-by: Juan Quintela <quintela@redhat.com>


I think this one belongs to previous patch (with accompaining line from below).
But just if you want to change it.

>          qemu_fclose(mis->to_src_file);
>          mis->to_src_file = NULL;
>      }
>  
> +    if (mis->from_src_file) {
> +        qemu_fclose(mis->from_src_file);
> +        mis->from_src_file = NULL;
> +    }
> +
>      qemu_event_destroy(&mis->main_thread_load_event);
>      loadvm_free_handlers(mis);
>  }
> @@ -433,7 +440,6 @@ static void process_incoming_migration_co(void *opaque)
>          exit(EXIT_FAILURE);
>      }
>  
> -    qemu_fclose(f);
>      free_xbzrle_decoded_buf();
>  
>      mis->bh = qemu_bh_new(process_incoming_migration_bh, mis);
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index a0489f6..57aa208 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -320,7 +320,6 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
>      }
>  
>      postcopy_state_set(POSTCOPY_INCOMING_END);
> -    migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 0);
>  
>      if (mis->postcopy_tmp_page) {
>          munmap(mis->postcopy_tmp_page, mis->largest_page_size);

Re: [Qemu-devel] [PATCH RFC 5/6] migration: let MigrationState be an QObject

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Let the old man "MigrationState" join the object family. Direct benefit
> is that we can start to use all the property features derived from
> current QObject, like: HW_COMPAT_* bits, command line setup for
> migration parameters (so will never need to set them up each time using
> HMP/QMP, this is really, really attractive for test writters), etc.
>
> I see no reason to disallow this happen yet. So let's start from this
> one, to see whether it would be anything good.
>
> No functional change at all.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>


Please, send this patch in toplevel.

Once there, as for following patch, could you incorporate as properties:
- only_migratable: And we don't have to have the global variable in vl.c
- global_state_set_optional()
- savevm_skip_configuration()
- savevm_skip_section_footers()

So we don't have to export functions from them?  they are just
properties of this?

Thanks, Juan.

Re: [Qemu-devel] [PATCH RFC 6/6] migration: enable return path for precopy

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Let this be a flag, default to on. Turn it off for <=2.9 versions.
>
> After this patch, return path will be on even for pre-copy migration as
> long as the transport support, e.g., for socket typed transport
> including "tcp|udp|unix" typed.
>
> This will naturally fix the bug mentioned below, when destination failed
> on migration but source assumed it was successful - since now even for
> precopy, source will wait for destination's MIG_RP_MSG_SHUT signal,
> which will carry the final migration status of destination. Then, when
> destination failed at any point of migration, source will know it, and
> it'll resume the VM instead of a data lost.
>
> Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1439147
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/hw/compat.h           |  4 ++++
>  include/migration/migration.h |  3 +++
>  migration/migration.c         | 15 ++++++++++++++-
>  3 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 55b1765..049457b 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -6,6 +6,10 @@
>          .driver   = "pci-bridge",\
>          .property = "shpc",\
>          .value    = "off",\
> +    },{\
> +        .driver   = "migration",\
> +        .property = "return-path",\
> +        .value    = "off",\
>      },
>  
>  #define HW_COMPAT_2_8 \
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 70710de..e44119c 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -169,6 +169,9 @@ typedef struct MigrationState {
>      int64_t colo_checkpoint_time;
>      QEMUTimer *colo_delay_timer;
>  
> +    /* Whether to try to enable return-path even for pre-copy */
> +    bool enable_return_path;
> +
>      /* The last error that occurred */
>      Error *error;
>  } MigrationState ;
> diff --git a/migration/migration.c b/migration/migration.c
> index 6df3483..16a856a 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2046,7 +2046,7 @@ static void *migration_thread(void *opaque)
>  static bool migrate_return_path_create(MigrationState *s)
>  {
>      /* Whether we should enable return path */
> -    bool enable_return_path = false;
> +    bool enable_return_path = s->enable_return_path;

As you can see on my suggestion for this piece of code, just add the
()s->enable_return_path &&) to the right place on the call?

Thanks, Juan.

Re: [Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> We were do the shutting off only for postcopy. Now we do this as long as
> the source return path is there.
>
> Moving the cleanup of from_src_file there too.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

You can also submmit this and previous patch alone and I will integrate
them.

thanks, Juan.

Re: [Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally

[Peter Xu]

On Tue, May 30, 2017 at 05:50:27PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > We were do the shutting off only for postcopy. Now we do this as long as
> > the source return path is there.
> >
> > Moving the cleanup of from_src_file there too.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/migration.c    | 8 +++++++-
> >  migration/postcopy-ram.c | 1 -
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 92617fc..a4006b4 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -131,10 +131,17 @@ void migration_incoming_state_destroy(void)
> >      struct MigrationIncomingState *mis = migration_incoming_get_current();
> >  
> >      if (mis->to_src_file) {
> > +        /* Tell source that we are done */
> > +        migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 0);
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> 
> I think this one belongs to previous patch (with accompaining line from below).
> But just if you want to change it.

I separated it since these two patches were actually doing different
things:

- previous patch fixed one possible leak, while

- this patch postponed MIG_RP_MSG_SHUT a bit to the end, and let it
  not depending on postcopy, but the return path itself (so that we
  can enable the return path even without postcopy then)

Meanwhile, there might be problem if we just put this single line into
previous patch, since this line depends on below change [1]
(from_src_file should better be closed after this
qemu_file_get_error() call). So... I would still prefer to separate
them using current way. Even if we really want to merge them, I would
prefer directly squashing current patch into previous one.

Thanks,

> 
> >          qemu_fclose(mis->to_src_file);
> >          mis->to_src_file = NULL;
> >      }
> >  
> > +    if (mis->from_src_file) {
> > +        qemu_fclose(mis->from_src_file);
> > +        mis->from_src_file = NULL;
> > +    }
> > +

[1]

> >      qemu_event_destroy(&mis->main_thread_load_event);
> >      loadvm_free_handlers(mis);
> >  }
> > @@ -433,7 +440,6 @@ static void process_incoming_migration_co(void *opaque)
> >          exit(EXIT_FAILURE);
> >      }
> >  
> > -    qemu_fclose(f);
> >      free_xbzrle_decoded_buf();
> >  
> >      mis->bh = qemu_bh_new(process_incoming_migration_bh, mis);
> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > index a0489f6..57aa208 100644
> > --- a/migration/postcopy-ram.c
> > +++ b/migration/postcopy-ram.c
> > @@ -320,7 +320,6 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
> >      }
> >  
> >      postcopy_state_set(POSTCOPY_INCOMING_END);
> > -    migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 0);
> >  
> >      if (mis->postcopy_tmp_page) {
> >          munmap(mis->postcopy_tmp_page, mis->largest_page_size);

-- 
Peter Xu

Re: [Qemu-devel] [PATCH RFC 5/6] migration: let MigrationState be an QObject

[Peter Xu]

On Tue, May 30, 2017 at 05:57:44PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > Let the old man "MigrationState" join the object family. Direct benefit
> > is that we can start to use all the property features derived from
> > current QObject, like: HW_COMPAT_* bits, command line setup for
> > migration parameters (so will never need to set them up each time using
> > HMP/QMP, this is really, really attractive for test writters), etc.
> >
> > I see no reason to disallow this happen yet. So let's start from this
> > one, to see whether it would be anything good.
> >
> > No functional change at all.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> 
> Please, send this patch in toplevel.
> 
> Once there, as for following patch, could you incorporate as properties:
> - only_migratable: And we don't have to have the global variable in vl.c
> - global_state_set_optional()
> - savevm_skip_configuration()
> - savevm_skip_section_footers()
> 
> So we don't have to export functions from them?  they are just
> properties of this?

Okay. I'll try. :)

Thanks.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Tue, May 30, 2017 at 05:50:27PM +0200, Juan Quintela wrote:
>> Peter Xu <peterx@redhat.com> wrote:
>> > We were do the shutting off only for postcopy. Now we do this as long as
>> > the source return path is there.
>> >
>> > Moving the cleanup of from_src_file there too.
>> >
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >  migration/migration.c    | 8 +++++++-
>> >  migration/postcopy-ram.c | 1 -
>> >  2 files changed, 7 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index 92617fc..a4006b4 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -131,10 +131,17 @@ void migration_incoming_state_destroy(void)
>> >      struct MigrationIncomingState *mis = migration_incoming_get_current();
>> >  
>> >      if (mis->to_src_file) {
>> > +        /* Tell source that we are done */
>> > +        migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 0);
>> 
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> 
>> 
>> I think this one belongs to previous patch (with accompaining line from below).
>> But just if you want to change it.
>
> I separated it since these two patches were actually doing different
> things:
>
> - previous patch fixed one possible leak, while
>
> - this patch postponed MIG_RP_MSG_SHUT a bit to the end, and let it
>   not depending on postcopy, but the return path itself (so that we
>   can enable the return path even without postcopy then)
>
> Meanwhile, there might be problem if we just put this single line into
> previous patch, since this line depends on below change [1]
> (from_src_file should better be closed after this
> qemu_file_get_error() call). So... I would still prefer to separate
> them using current way. Even if we really want to merge them, I would
> prefer directly squashing current patch into previous one.

ok, it is up to you.

Re: [Qemu-devel] [PATCH RFC 6/6] migration: enable return path for precopy

[Peter Xu]

On Tue, May 30, 2017 at 05:59:10PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > Let this be a flag, default to on. Turn it off for <=2.9 versions.
> >
> > After this patch, return path will be on even for pre-copy migration as
> > long as the transport support, e.g., for socket typed transport
> > including "tcp|udp|unix" typed.
> >
> > This will naturally fix the bug mentioned below, when destination failed
> > on migration but source assumed it was successful - since now even for
> > precopy, source will wait for destination's MIG_RP_MSG_SHUT signal,
> > which will carry the final migration status of destination. Then, when
> > destination failed at any point of migration, source will know it, and
> > it'll resume the VM instead of a data lost.
> >
> > Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1439147
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  include/hw/compat.h           |  4 ++++
> >  include/migration/migration.h |  3 +++
> >  migration/migration.c         | 15 ++++++++++++++-
> >  3 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > index 55b1765..049457b 100644
> > --- a/include/hw/compat.h
> > +++ b/include/hw/compat.h
> > @@ -6,6 +6,10 @@
> >          .driver   = "pci-bridge",\
> >          .property = "shpc",\
> >          .value    = "off",\
> > +    },{\
> > +        .driver   = "migration",\
> > +        .property = "return-path",\
> > +        .value    = "off",\
> >      },
> >  
> >  #define HW_COMPAT_2_8 \
> > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > index 70710de..e44119c 100644
> > --- a/include/migration/migration.h
> > +++ b/include/migration/migration.h
> > @@ -169,6 +169,9 @@ typedef struct MigrationState {
> >      int64_t colo_checkpoint_time;
> >      QEMUTimer *colo_delay_timer;
> >  
> > +    /* Whether to try to enable return-path even for pre-copy */
> > +    bool enable_return_path;
> > +
> >      /* The last error that occurred */
> >      Error *error;
> >  } MigrationState ;
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 6df3483..16a856a 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -2046,7 +2046,7 @@ static void *migration_thread(void *opaque)
> >  static bool migrate_return_path_create(MigrationState *s)
> >  {
> >      /* Whether we should enable return path */
> > -    bool enable_return_path = false;
> > +    bool enable_return_path = s->enable_return_path;
> 
> As you can see on my suggestion for this piece of code, just add the
> ()s->enable_return_path &&) to the right place on the call?
> 
> Thanks, Juan.

Do you mean this?

    /*
     * Open the return path
     */
    if (migrate_postcopy_ram() || s->enable_return_path) {
        if (!migrate_return_path_create(s)) {
            migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
                              MIGRATION_STATUS_FAILED);
            migrate_fd_cleanup(s);
            return;
        }
    }

Here what I wanted to achieve is that:

a. for postcopy, we should try to enable return path, and it must
   succeed

b. for the case when enable_return_path is set, we try to enable return
   path, but even if it failed, we can still continue

Could we really achieve (b) if with above code? Or anything I missed?

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH RFC 6/6] migration: enable return path for precopy

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Tue, May 30, 2017 at 05:59:10PM +0200, Juan Quintela wrote:
>> Peter Xu <peterx@redhat.com> wrote:
>> > Let this be a flag, default to on. Turn it off for <=2.9 versions.
>> >
>> > After this patch, return path will be on even for pre-copy migration as
>> > long as the transport support, e.g., for socket typed transport
>> > including "tcp|udp|unix" typed.
>> >
>> > This will naturally fix the bug mentioned below, when destination failed
>> > on migration but source assumed it was successful - since now even for
>> > precopy, source will wait for destination's MIG_RP_MSG_SHUT signal,
>> > which will carry the final migration status of destination. Then, when
>> > destination failed at any point of migration, source will know it, and
>> > it'll resume the VM instead of a data lost.
>> >
>> > Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1439147
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >  include/hw/compat.h           |  4 ++++
>> >  include/migration/migration.h |  3 +++
>> >  migration/migration.c         | 15 ++++++++++++++-
>> >  3 files changed, 21 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/include/hw/compat.h b/include/hw/compat.h
>> > index 55b1765..049457b 100644
>> > --- a/include/hw/compat.h
>> > +++ b/include/hw/compat.h
>> > @@ -6,6 +6,10 @@
>> >          .driver   = "pci-bridge",\
>> >          .property = "shpc",\
>> >          .value    = "off",\
>> > +    },{\
>> > +        .driver   = "migration",\
>> > +        .property = "return-path",\
>> > +        .value    = "off",\
>> >      },
>> >  
>> >  #define HW_COMPAT_2_8 \
>> > diff --git a/include/migration/migration.h b/include/migration/migration.h
>> > index 70710de..e44119c 100644
>> > --- a/include/migration/migration.h
>> > +++ b/include/migration/migration.h
>> > @@ -169,6 +169,9 @@ typedef struct MigrationState {
>> >      int64_t colo_checkpoint_time;
>> >      QEMUTimer *colo_delay_timer;
>> >  
>> > +    /* Whether to try to enable return-path even for pre-copy */
>> > +    bool enable_return_path;
>> > +
>> >      /* The last error that occurred */
>> >      Error *error;
>> >  } MigrationState ;
>> > diff --git a/migration/migration.c b/migration/migration.c
>> > index 6df3483..16a856a 100644
>> > --- a/migration/migration.c
>> > +++ b/migration/migration.c
>> > @@ -2046,7 +2046,7 @@ static void *migration_thread(void *opaque)
>> >  static bool migrate_return_path_create(MigrationState *s)
>> >  {
>> >      /* Whether we should enable return path */
>> > -    bool enable_return_path = false;
>> > +    bool enable_return_path = s->enable_return_path;
>> 
>> As you can see on my suggestion for this piece of code, just add the
>> ()s->enable_return_path &&) to the right place on the call?
>> 
>> Thanks, Juan.
>
> Do you mean this?
>
>     /*
>      * Open the return path
>      */
>     if (migrate_postcopy_ram() || s->enable_return_path) {
>         if (!migrate_return_path_create(s)) {
>             migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
>                               MIGRATION_STATUS_FAILED);
>             migrate_fd_cleanup(s);
>             return;
>         }
>     }

Yeap.

> Here what I wanted to achieve is that:
>
> a. for postcopy, we should try to enable return path, and it must
>    succeed
>
> b. for the case when enable_return_path is set, we try to enable return
>    path, but even if it failed, we can still continue
>
> Could we really achieve (b) if with above code? Or anything I missed?

if we enable_return_path -> it should success, otherwise it makes no
sense, no?  We can try to remove the return path for some transports if
needed, but it makes no sense to enable a property that means:
"please, pretty please, enable it if you can"

if we are going to do it that way, then it is better to change the
property the other way around:

- disable_return_path: set for all old machine types

And not set for newer machine types, meaning that we just try.

What do you think?

Later, Juan.

Re: [Qemu-devel] [PATCH RFC 6/6] migration: enable return path for precopy

[Peter Xu]

On Wed, May 31, 2017 at 09:43:21AM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Tue, May 30, 2017 at 05:59:10PM +0200, Juan Quintela wrote:
> >> Peter Xu <peterx@redhat.com> wrote:
> >> > Let this be a flag, default to on. Turn it off for <=2.9 versions.
> >> >
> >> > After this patch, return path will be on even for pre-copy migration as
> >> > long as the transport support, e.g., for socket typed transport
> >> > including "tcp|udp|unix" typed.
> >> >
> >> > This will naturally fix the bug mentioned below, when destination failed
> >> > on migration but source assumed it was successful - since now even for
> >> > precopy, source will wait for destination's MIG_RP_MSG_SHUT signal,
> >> > which will carry the final migration status of destination. Then, when
> >> > destination failed at any point of migration, source will know it, and
> >> > it'll resume the VM instead of a data lost.
> >> >
> >> > Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1439147
> >> > Signed-off-by: Peter Xu <peterx@redhat.com>
> >> > ---
> >> >  include/hw/compat.h           |  4 ++++
> >> >  include/migration/migration.h |  3 +++
> >> >  migration/migration.c         | 15 ++++++++++++++-
> >> >  3 files changed, 21 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/include/hw/compat.h b/include/hw/compat.h
> >> > index 55b1765..049457b 100644
> >> > --- a/include/hw/compat.h
> >> > +++ b/include/hw/compat.h
> >> > @@ -6,6 +6,10 @@
> >> >          .driver   = "pci-bridge",\
> >> >          .property = "shpc",\
> >> >          .value    = "off",\
> >> > +    },{\
> >> > +        .driver   = "migration",\
> >> > +        .property = "return-path",\
> >> > +        .value    = "off",\
> >> >      },
> >> >  
> >> >  #define HW_COMPAT_2_8 \
> >> > diff --git a/include/migration/migration.h b/include/migration/migration.h
> >> > index 70710de..e44119c 100644
> >> > --- a/include/migration/migration.h
> >> > +++ b/include/migration/migration.h
> >> > @@ -169,6 +169,9 @@ typedef struct MigrationState {
> >> >      int64_t colo_checkpoint_time;
> >> >      QEMUTimer *colo_delay_timer;
> >> >  
> >> > +    /* Whether to try to enable return-path even for pre-copy */
> >> > +    bool enable_return_path;
> >> > +
> >> >      /* The last error that occurred */
> >> >      Error *error;
> >> >  } MigrationState ;
> >> > diff --git a/migration/migration.c b/migration/migration.c
> >> > index 6df3483..16a856a 100644
> >> > --- a/migration/migration.c
> >> > +++ b/migration/migration.c
> >> > @@ -2046,7 +2046,7 @@ static void *migration_thread(void *opaque)
> >> >  static bool migrate_return_path_create(MigrationState *s)
> >> >  {
> >> >      /* Whether we should enable return path */
> >> > -    bool enable_return_path = false;
> >> > +    bool enable_return_path = s->enable_return_path;
> >> 
> >> As you can see on my suggestion for this piece of code, just add the
> >> ()s->enable_return_path &&) to the right place on the call?
> >> 
> >> Thanks, Juan.
> >
> > Do you mean this?
> >
> >     /*
> >      * Open the return path
> >      */
> >     if (migrate_postcopy_ram() || s->enable_return_path) {
> >         if (!migrate_return_path_create(s)) {
> >             migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> >                               MIGRATION_STATUS_FAILED);
> >             migrate_fd_cleanup(s);
> >             return;
> >         }
> >     }
> 
> Yeap.
> 
> > Here what I wanted to achieve is that:
> >
> > a. for postcopy, we should try to enable return path, and it must
> >    succeed
> >
> > b. for the case when enable_return_path is set, we try to enable return
> >    path, but even if it failed, we can still continue
> >
> > Could we really achieve (b) if with above code? Or anything I missed?
> 
> if we enable_return_path -> it should success, otherwise it makes no
> sense, no?  We can try to remove the return path for some transports if
> needed, but it makes no sense to enable a property that means:
> "please, pretty please, enable it if you can"

(Indeed this is awkward... :)

> 
> if we are going to do it that way, then it is better to change the
> property the other way around:
> 
> - disable_return_path: set for all old machine types
> 
> And not set for newer machine types, meaning that we just try.
> 
> What do you think?

Both namings work for me.

The problem is that we cannot really force this as long as there is
any type of transports that does not support return path. E.g., when
migrating to an "exec:" typed transport with single-out IO stream. In
that case, if we fail the migration, it'll break the old behavior,
right? Then for exec: typed users they need to manually provide
enable_return_path=false to finally allow them to migrate.

(I think that's the most tricky point of this series...)

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH RFC 6/6] migration: enable return path for precopy

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:

...

>> > Here what I wanted to achieve is that:
>> >
>> > a. for postcopy, we should try to enable return path, and it must
>> >    succeed
>> >
>> > b. for the case when enable_return_path is set, we try to enable return
>> >    path, but even if it failed, we can still continue
>> >
>> > Could we really achieve (b) if with above code? Or anything I missed?
>> 
>> if we enable_return_path -> it should success, otherwise it makes no
>> sense, no?  We can try to remove the return path for some transports if
>> needed, but it makes no sense to enable a property that means:
>> "please, pretty please, enable it if you can"
>
> (Indeed this is awkward... :)
>
>> 
>> if we are going to do it that way, then it is better to change the
>> property the other way around:
>> 
>> - disable_return_path: set for all old machine types
>> 
>> And not set for newer machine types, meaning that we just try.
>> 
>> What do you think?
>
> Both namings work for me.
>
> The problem is that we cannot really force this as long as there is
> any type of transports that does not support return path. E.g., when
> migrating to an "exec:" typed transport with single-out IO stream. In
> that case, if we fail the migration, it'll break the old behavior,
> right? Then for exec: typed users they need to manually provide
> enable_return_path=false to finally allow them to migrate.
>
> (I think that's the most tricky point of this series...)

Then do what I said.  Disable it for old machine types.
And for new machine types, we just try our best.  What I object is
having a property with a meaning of "perhaps enable return path".

And yes, representing with a boolean a tri-state is tricky O:-)

Later, Juan.

Re: [Qemu-devel] [PATCH RFC 3/6] migration: fix leak of src file on dst

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> The return path channel is possibly leaked. Fix it.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 5 +++++
>  1 file changed, 5 insertions(+)
>

Queued this one.

Reviewed-by: Juan Quintela <quintela@redhat.com>



> diff --git a/migration/migration.c b/migration/migration.c
> index e4f4c8c..92617fc 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -130,6 +130,11 @@ void migration_incoming_state_destroy(void)
>  {
>      struct MigrationIncomingState *mis = migration_incoming_get_current();
>  
> +    if (mis->to_src_file) {
> +        qemu_fclose(mis->to_src_file);
> +        mis->to_src_file = NULL;
> +    }
> +
>      qemu_event_destroy(&mis->main_thread_load_event);
>      loadvm_free_handlers(mis);
>  }

Re: [Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]

On 05/19/2017 01:43 AM, Peter Xu wrote:
> We were do the shutting off only for postcopy. Now we do this as long as
> the source return path is there.
> 
> Moving the cleanup of from_src_file there too.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c    | 8 +++++++-
>  migration/postcopy-ram.c | 1 -
>  2 files changed, 7 insertions(+), 2 deletions(-)

This commit causes a regression in qemu-iotests 68:

$ cd tests/qemu-iotests
$ ./check -qcow2 68
...
068 1s ... - output mismatch (see 068.out.bad)
--- /home/eblake/qemu-tmp2/tests/qemu-iotests/068.out	2017-05-30
09:27:26.795821748 -0500
+++ 068.out.bad	2017-06-05 15:21:18.566816816 -0500
@@ -6,6 +6,8 @@
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm 0
 (qemu) quit
+./common.config: line 107:  1912 Segmentation fault      (core dumped)
( if [ -n "${QEMU_NEED_PID}" ]; then
+    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) quit
-*** done
+(qemu) *** done
Failures: 068
Failed 1 of 1 tests

I didn't investigate further; but am hoping you'll be able to fix the
segfault and get the test working again.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC 4/6] migration: shut src return path unconditionally

[Peter Xu]

On Mon, Jun 05, 2017 at 03:22:24PM -0500, Eric Blake wrote:
> On 05/19/2017 01:43 AM, Peter Xu wrote:
> > We were do the shutting off only for postcopy. Now we do this as long as
> > the source return path is there.
> > 
> > Moving the cleanup of from_src_file there too.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/migration.c    | 8 +++++++-
> >  migration/postcopy-ram.c | 1 -
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> This commit causes a regression in qemu-iotests 68:
> 
> $ cd tests/qemu-iotests
> $ ./check -qcow2 68
> ...
> 068 1s ... - output mismatch (see 068.out.bad)
> --- /home/eblake/qemu-tmp2/tests/qemu-iotests/068.out	2017-05-30
> 09:27:26.795821748 -0500
> +++ 068.out.bad	2017-06-05 15:21:18.566816816 -0500
> @@ -6,6 +6,8 @@
>  QEMU X.Y.Z monitor - type 'help' for more information
>  (qemu) savevm 0
>  (qemu) quit
> +./common.config: line 107:  1912 Segmentation fault      (core dumped)
> ( if [ -n "${QEMU_NEED_PID}" ]; then
> +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
>  QEMU X.Y.Z monitor - type 'help' for more information
> -(qemu) quit
> -*** done
> +(qemu) *** done
> Failures: 068
> Failed 1 of 1 tests
> 
> I didn't investigate further; but am hoping you'll be able to fix the
> segfault and get the test working again.

Sorry for that! I will have a look.

-- 
Peter Xu