[Qemu-devel] [PATCH v3 0/7] migration: pause-before-switchover

[Qemu-devel] [PATCH v3 0/7] migration: pause-before-switchover

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Hi,
  This set attempts to make a race condition between migration and
drive-mirror (and other block users) soluble by allowing the migration
to be paused after the source qemu releases the block devices but
before the serialisation of the device state.

The symptom of this failure, as reported by Wangjie, is a:
   _co_do_pwritev: Assertion `!(bs->open_flags & 0x0800)' failed

and the source qemu dieing; so the problem is pretty nasty.
This has only been seen on 2.9 onwards, but the theory is that
prior to 2.9 it might have been happening anyway and we were
perhaps getting unreported corruptions (lost writes); so this
really needs fixing.

This flow came from discussions between Kevin and me, and we can't
see a way of fixing it without exposing a new state to the management
layer.

The flow is now:

(qemu) migrate_set_capability pause-before-switchover on
(qemu) migrate -d ...
(qemu) info migrate
...
Migration status: pre-switchover
...
<< issue commands to clean up any block jobs>>

(qemu) migrate_continue pre-switchover
(qemu) info migrate
...
Migration status: completed

This set has been _very_ lightly tested just at the normal migration
code, without the addition of the drive mirror; so this is a first
cut.  I'd appreciate some feedback from libvirt whether the inteface
is OK and ideally a hack to test it in a full libvirt setup to see
if we hit any other issues.

The precopy flow is:
active->pre-switchover->device->completed

The postcopy flow is:
active->pre-switchover->postcopy-active->completed

Although the behaviour with postcopy only gets interesting when
we add something like Max's active-sync.

Dave

--
v3
  A couple of FIXUPs that had escaped v2's merge

v2
  Pause *before* block inactivation (thanks Peter)
  Rename state and capability to Dan+KWolf's combined suggestion


Dr. David Alan Gilbert (7):
  migration: Add 'pause-before-switchover' capability
  migration: Add 'pre-switchover' and 'device' statuses
  migration: Wait for semaphore before completing migration
  migration: migrate-continue
  migrate: HMP migate_continue
  migration: allow cancel to unpause
  migration: pause-before-switchover for postcopy

 hmp-commands.hx       | 12 +++++++
 hmp.c                 | 13 ++++++++
 hmp.h                 |  1 +
 migration/migration.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++--
 migration/migration.h |  4 +++
 qapi/migration.json   | 30 ++++++++++++++++--
 6 files changed, 144 insertions(+), 4 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH v3 1/7] migration: Add 'pause-before-switchover' capability

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

When 'pause-before-switchover' is enabled, the outgoing migration
will pause before invalidating the block devices and serializing
the device state.
At this point the management layer gets the chance to clean up any
device jobs or other device users before the migration completes.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/migration.c | 10 ++++++++++
 migration/migration.h |  1 +
 qapi/migration.json   |  5 ++++-
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 98429dc843..fba3342368 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1521,6 +1521,16 @@ bool migrate_use_multifd(void)
     return s->enabled_capabilities[MIGRATION_CAPABILITY_X_MULTIFD];
 }
 
+bool migrate_pause_before_switchover(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[
+        MIGRATION_CAPABILITY_PAUSE_BEFORE_SWITCHOVER];
+}
+
 int migrate_multifd_channels(void)
 {
     MigrationState *s;
diff --git a/migration/migration.h b/migration/migration.h
index b83cceadc4..969866303e 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -177,6 +177,7 @@ bool migrate_zero_blocks(void);
 
 bool migrate_auto_converge(void);
 bool migrate_use_multifd(void);
+bool migrate_pause_before_switchover(void);
 int migrate_multifd_channels(void);
 int migrate_multifd_page_count(void);
 
diff --git a/qapi/migration.json b/qapi/migration.json
index f8b365e3f5..4960231ba2 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -341,6 +341,9 @@
 # @return-path: If enabled, migration will use the return path even
 #               for precopy. (since 2.10)
 #
+# @pause-before-switchover: Pause outgoing migration before serialising device
+#          state and before disabling block IO (since 2.11)
+#
 # @x-multifd: Use more than one fd for migration (since 2.11)
 #
 # Since: 1.2
@@ -348,7 +351,7 @@
 { 'enum': 'MigrationCapability',
   'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
            'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
-           'block', 'return-path', 'x-multifd' ] }
+           'block', 'return-path', 'pause-before-switchover', 'x-multifd' ] }
 
 ##
 # @MigrationCapabilityStatus:
-- 
2.13.6

[Qemu-devel] [PATCH v3 2/7] migration: Add 'pre-switchover' and 'device' statuses

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Add two statuses for use when the 'pause-before-switchover'
capability is enabled.

'pre-switchover' is the state that we wait in for management
to allow us to continue.
'device' is the state we enter while serialising the devices
after management gives us the OK.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/migration.c | 6 ++++++
 qapi/migration.json   | 8 +++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index fba3342368..86ae0292f0 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -526,6 +526,8 @@ static bool migration_is_setup_or_active(int state)
     case MIGRATION_STATUS_ACTIVE:
     case MIGRATION_STATUS_POSTCOPY_ACTIVE:
     case MIGRATION_STATUS_SETUP:
+    case MIGRATION_STATUS_PRE_SWITCHOVER:
+    case MIGRATION_STATUS_DEVICE:
         return true;
 
     default:
@@ -600,6 +602,8 @@ MigrationInfo *qmp_query_migrate(Error **errp)
     case MIGRATION_STATUS_ACTIVE:
     case MIGRATION_STATUS_CANCELLING:
     case MIGRATION_STATUS_POSTCOPY_ACTIVE:
+    case MIGRATION_STATUS_PRE_SWITCHOVER:
+    case MIGRATION_STATUS_DEVICE:
          /* TODO add some postcopy stats */
         info->has_status = true;
         info->has_total_time = true;
@@ -1183,6 +1187,8 @@ bool migration_is_idle(void)
     case MIGRATION_STATUS_ACTIVE:
     case MIGRATION_STATUS_POSTCOPY_ACTIVE:
     case MIGRATION_STATUS_COLO:
+    case MIGRATION_STATUS_PRE_SWITCHOVER:
+    case MIGRATION_STATUS_DEVICE:
         return false;
     case MIGRATION_STATUS__MAX:
         g_assert_not_reached();
diff --git a/qapi/migration.json b/qapi/migration.json
index 4960231ba2..b56f95db64 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -96,12 +96,18 @@
 # @colo: VM is in the process of fault tolerance, VM can not get into this
 #        state unless colo capability is enabled for migration. (since 2.8)
 #
+# @pre-switchover: Paused before device serialisation. (since 2.11)
+#
+# @device: During device serialisation when pause-before-switchover is enabled
+#        (since 2.11)
+#
 # Since: 2.3
 #
 ##
 { 'enum': 'MigrationStatus',
   'data': [ 'none', 'setup', 'cancelling', 'cancelled',
-            'active', 'postcopy-active', 'completed', 'failed', 'colo' ] }
+            'active', 'postcopy-active', 'completed', 'failed', 'colo',
+            'pre-switchover', 'device' ] }
 
 ##
 # @MigrationInfo:
-- 
2.13.6

[Qemu-devel] [PATCH v3 3/7] migration: Wait for semaphore before completing migration

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Wait for a semaphore before completing the migration,
if the previously added capability was enabled.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/migration.c | 38 ++++++++++++++++++++++++++++++++++++++
 migration/migration.h |  3 +++
 2 files changed, 41 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 86ae0292f0..6c6d5e2c75 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1968,6 +1968,39 @@ fail:
 }
 
 /**
+ * migration_maybe_pause: Pause if required to by
+ * migrate_pause_before_switchover called with the iothread locked
+ * Returns: 0 on success
+ */
+static int migration_maybe_pause(MigrationState *s, int *current_active_state)
+{
+    if (!migrate_pause_before_switchover()) {
+        return 0;
+    }
+
+    /* Since leaving this state is not atomic with posting the semaphore
+     * it's possible that someone could have issued multiple migrate_continue
+     * and the semaphore is incorrectly positive at this point;
+     * the docs say it's undefined to reinit a semaphore that's already
+     * init'd, so use timedwait to eat up any existing posts.
+     */
+    while (qemu_sem_timedwait(&s->pause_sem, 1) == 0) {
+        /* This block intentionally left blank */
+    }
+
+    qemu_mutex_unlock_iothread();
+    migrate_set_state(&s->state, *current_active_state,
+                      MIGRATION_STATUS_PRE_SWITCHOVER);
+    qemu_sem_wait(&s->pause_sem);
+    migrate_set_state(&s->state, MIGRATION_STATUS_PRE_SWITCHOVER,
+                      MIGRATION_STATUS_DEVICE);
+    *current_active_state = MIGRATION_STATUS_DEVICE;
+    qemu_mutex_lock_iothread();
+
+    return s->state == MIGRATION_STATUS_DEVICE ? 0 : -EINVAL;
+}
+
+/**
  * migration_completion: Used by migration_thread when there's not much left.
  *   The caller 'breaks' the loop when this returns.
  *
@@ -1993,6 +2026,9 @@ static void migration_completion(MigrationState *s, int current_active_state,
             bool inactivate = !migrate_colo_enabled();
             ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
             if (ret >= 0) {
+                ret = migration_maybe_pause(s, &current_active_state);
+            }
+            if (ret >= 0) {
                 qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
                 ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
                                                          inactivate);
@@ -2373,6 +2409,7 @@ static void migration_instance_finalize(Object *obj)
 
     g_free(params->tls_hostname);
     g_free(params->tls_creds);
+    qemu_sem_destroy(&ms->pause_sem);
 }
 
 static void migration_instance_init(Object *obj)
@@ -2383,6 +2420,7 @@ static void migration_instance_init(Object *obj)
     ms->state = MIGRATION_STATUS_NONE;
     ms->xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE;
     ms->mbps = -1;
+    qemu_sem_init(&ms->pause_sem, 0);
 
     params->tls_hostname = g_strdup("");
     params->tls_creds = g_strdup("");
diff --git a/migration/migration.h b/migration/migration.h
index 969866303e..cd988a99b9 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -121,6 +121,9 @@ struct MigrationState
     /* Flag set once the migration thread called bdrv_inactivate_all */
     bool block_inactive;
 
+    /* Migration is paused due to pause-before-switchover */
+    QemuSemaphore pause_sem;
+
     /* The semaphore is used to notify COLO thread that failover is finished */
     QemuSemaphore colo_exit_sem;
 
-- 
2.13.6

[Qemu-devel] [PATCH v3 4/7] migration: migrate-continue

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

A new qmp command allows the caller to continue from a given
paused state.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/migration.c | 11 +++++++++++
 qapi/migration.json   | 17 +++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 6c6d5e2c75..c9a3ee9efd 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1368,6 +1368,17 @@ void qmp_migrate_cancel(Error **errp)
     migrate_fd_cancel(migrate_get_current());
 }
 
+void qmp_migrate_continue(MigrationStatus state, Error **errp)
+{
+    MigrationState *s = migrate_get_current();
+    if (s->state != state) {
+        error_setg(errp,  "Migration not in expected state: %s",
+                   MigrationStatus_str(s->state));
+        return;
+    }
+    qemu_sem_post(&s->pause_sem);
+}
+
 void qmp_migrate_set_cache_size(int64_t value, Error **errp)
 {
     MigrationState *s = migrate_get_current();
diff --git a/qapi/migration.json b/qapi/migration.json
index b56f95db64..3b50afa6e8 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -877,6 +877,23 @@
 { 'command': 'migrate_cancel' }
 
 ##
+# @migrate-continue:
+#
+# Continue migration when it's in a paused state.
+#
+# @state: The state the migration is currently expected to be in
+#
+# Returns: nothing on success
+# Since: 2.11
+# Example:
+#
+# -> { "execute": "migrate-continue" , "arguments":
+#      { "state": "pause-before-device" } }
+# <- { "return": {} }
+##
+{ 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
+
+##
 # @migrate_set_downtime:
 #
 # Set maximum tolerated downtime for migration.
-- 
2.13.6

[Qemu-devel] [PATCH v3 5/7] migrate: HMP migate_continue

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

HMP equivalent to the just added migrate-continue
Unpause a migrate paused at a given state.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hmp-commands.hx | 12 ++++++++++++
 hmp.c           | 13 +++++++++++++
 hmp.h           |  1 +
 3 files changed, 26 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 1941e19932..4afd57cf5f 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -959,7 +959,19 @@ STEXI
 @item migrate_cancel
 @findex migrate_cancel
 Cancel the current VM migration.
+ETEXI
 
+    {
+        .name       = "migrate_continue",
+        .args_type  = "state:s",
+        .params     = "state",
+        .help       = "Continue migration from the given paused state",
+        .cmd        = hmp_migrate_continue,
+    },
+STEXI
+@item migrate_continue @var{state}
+@findex migrate_continue
+Continue migration from the paused state @var{state}
 ETEXI
 
     {
diff --git a/hmp.c b/hmp.c
index ec61329ebb..41fcce6f5a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1495,6 +1495,19 @@ void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
     qmp_migrate_cancel(NULL);
 }
 
+void hmp_migrate_continue(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    const char *state = qdict_get_str(qdict, "state");
+    int val = qapi_enum_parse(&MigrationStatus_lookup, state, -1, &err);
+
+    if (val >= 0) {
+        qmp_migrate_continue(val, &err);
+    }
+
+    hmp_handle_error(mon, &err);
+}
+
 void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
diff --git a/hmp.h b/hmp.h
index 3605003e4c..a6f56b1f29 100644
--- a/hmp.h
+++ b/hmp.h
@@ -68,6 +68,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict);
 void hmp_delvm(Monitor *mon, const QDict *qdict);
 void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
+void hmp_migrate_continue(Monitor *mon, const QDict *qdict);
 void hmp_migrate_incoming(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict);
-- 
2.13.6

[Qemu-devel] [PATCH v3 6/7] migration: allow cancel to unpause

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

If a migration_cancel is issued during the new paused state,
kick the pause_sem to get to unpause so it can cancel.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/migration.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index c9a3ee9efd..756deb3e2c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1108,6 +1108,10 @@ static void migrate_fd_cancel(MigrationState *s)
         if (!migration_is_setup_or_active(old_state)) {
             break;
         }
+        /* If the migration is paused, kick it out of the pause */
+        if (old_state == MIGRATION_STATUS_PRE_SWITCHOVER) {
+            qemu_sem_post(&s->pause_sem);
+        }
         migrate_set_state(&s->state, old_state, MIGRATION_STATUS_CANCELLING);
     } while (s->state != MIGRATION_STATUS_CANCELLING);
 
-- 
2.13.6

[Qemu-devel] [PATCH v3 7/7] migration: pause-before-switchover for postcopy

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Add pause-before-switchover support for postcopy.
After starting postcopy it will transition
    active->pre-switchover->postcopy_active

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/migration.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 756deb3e2c..af587377e9 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -104,6 +104,9 @@ enum mig_rp_message_type {
 static MigrationState *current_migration;
 
 static bool migration_object_check(MigrationState *ms, Error **errp);
+static int migration_maybe_pause(MigrationState *s,
+                                 int *current_active_state,
+                                 int new_state);
 
 void migration_object_init(void)
 {
@@ -1830,8 +1833,11 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running)
     QEMUFile *fb;
     int64_t time_at_stop = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
     bool restart_block = false;
-    migrate_set_state(&ms->state, MIGRATION_STATUS_ACTIVE,
-                      MIGRATION_STATUS_POSTCOPY_ACTIVE);
+    int cur_state = MIGRATION_STATUS_ACTIVE;
+    if (!migrate_pause_before_switchover()) {
+        migrate_set_state(&ms->state, MIGRATION_STATUS_ACTIVE,
+                          MIGRATION_STATUS_POSTCOPY_ACTIVE);
+    }
 
     trace_postcopy_start();
     qemu_mutex_lock_iothread();
@@ -1845,6 +1851,12 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running)
         goto fail;
     }
 
+    ret = migration_maybe_pause(ms, &cur_state,
+                                MIGRATION_STATUS_POSTCOPY_ACTIVE);
+    if (ret < 0) {
+        goto fail;
+    }
+
     ret = bdrv_inactivate_all();
     if (ret < 0) {
         goto fail;
@@ -1987,7 +1999,9 @@ fail:
  * migrate_pause_before_switchover called with the iothread locked
  * Returns: 0 on success
  */
-static int migration_maybe_pause(MigrationState *s, int *current_active_state)
+static int migration_maybe_pause(MigrationState *s,
+                                 int *current_active_state,
+                                 int new_state)
 {
     if (!migrate_pause_before_switchover()) {
         return 0;
@@ -2008,11 +2022,11 @@ static int migration_maybe_pause(MigrationState *s, int *current_active_state)
                       MIGRATION_STATUS_PRE_SWITCHOVER);
     qemu_sem_wait(&s->pause_sem);
     migrate_set_state(&s->state, MIGRATION_STATUS_PRE_SWITCHOVER,
-                      MIGRATION_STATUS_DEVICE);
-    *current_active_state = MIGRATION_STATUS_DEVICE;
+                      new_state);
+    *current_active_state = new_state;
     qemu_mutex_lock_iothread();
 
-    return s->state == MIGRATION_STATUS_DEVICE ? 0 : -EINVAL;
+    return s->state == new_state ? 0 : -EINVAL;
 }
 
 /**
@@ -2041,7 +2055,8 @@ static void migration_completion(MigrationState *s, int current_active_state,
             bool inactivate = !migrate_colo_enabled();
             ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
             if (ret >= 0) {
-                ret = migration_maybe_pause(s, &current_active_state);
+                ret = migration_maybe_pause(s, &current_active_state,
+                                            MIGRATION_STATUS_DEVICE);
             }
             if (ret >= 0) {
                 qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
-- 
2.13.6

Re: [Qemu-devel] [PATCH v3 1/7] migration: Add 'pause-before-switchover' capability

[Peter Xu]

On Wed, Oct 18, 2017 at 06:40:07PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> When 'pause-before-switchover' is enabled, the outgoing migration
> will pause before invalidating the block devices and serializing
> the device state.
> At this point the management layer gets the chance to clean up any
> device jobs or other device users before the migration completes.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  migration/migration.c | 10 ++++++++++
>  migration/migration.h |  1 +
>  qapi/migration.json   |  5 ++++-
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 98429dc843..fba3342368 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1521,6 +1521,16 @@ bool migrate_use_multifd(void)
>      return s->enabled_capabilities[MIGRATION_CAPABILITY_X_MULTIFD];
>  }
>  
> +bool migrate_pause_before_switchover(void)
> +{
> +    MigrationState *s;
> +
> +    s = migrate_get_current();
> +
> +    return s->enabled_capabilities[
> +        MIGRATION_CAPABILITY_PAUSE_BEFORE_SWITCHOVER];
> +}
> +
>  int migrate_multifd_channels(void)
>  {
>      MigrationState *s;
> diff --git a/migration/migration.h b/migration/migration.h
> index b83cceadc4..969866303e 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -177,6 +177,7 @@ bool migrate_zero_blocks(void);
>  
>  bool migrate_auto_converge(void);
>  bool migrate_use_multifd(void);
> +bool migrate_pause_before_switchover(void);
>  int migrate_multifd_channels(void);
>  int migrate_multifd_page_count(void);
>  
> diff --git a/qapi/migration.json b/qapi/migration.json
> index f8b365e3f5..4960231ba2 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -341,6 +341,9 @@
>  # @return-path: If enabled, migration will use the return path even
>  #               for precopy. (since 2.10)
>  #
> +# @pause-before-switchover: Pause outgoing migration before serialising device
> +#          state and before disabling block IO (since 2.11)

Nit: Not sure whether it's good to avoid naming "disabling block IO",
since after all this pre-switch state can be used to do other things
as well in the future.

Reviewed-by: Peter Xu <peterx@redhat.com>

> +#
>  # @x-multifd: Use more than one fd for migration (since 2.11)
>  #
>  # Since: 1.2
> @@ -348,7 +351,7 @@
>  { 'enum': 'MigrationCapability',
>    'data': ['xbzrle', 'rdma-pin-all', 'auto-converge', 'zero-blocks',
>             'compress', 'events', 'postcopy-ram', 'x-colo', 'release-ram',
> -           'block', 'return-path', 'x-multifd' ] }
> +           'block', 'return-path', 'pause-before-switchover', 'x-multifd' ] }
>  
>  ##
>  # @MigrationCapabilityStatus:
> -- 
> 2.13.6
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 0/7] migration: pause-before-switchover

[Peter Xu]

On Wed, Oct 18, 2017 at 06:40:06PM +0100, Dr. David Alan Gilbert (git) wrote:

[...]

> The precopy flow is:
> active->pre-switchover->device->completed
> 
> The postcopy flow is:
> active->pre-switchover->postcopy-active->completed

The naming is still slightly confusing to me:

(1) we have a capability called "pause-before-switchover", so it feels
    like there is something called "switchover" and if we enable this
    we'll pause before that point;

(2) we have a new status "pre-switchover", it feels like that's the
    point before we are in "switchover" state;

(3) we don't really have a "switchover" state, but instead it's called
    "device" which is exactly the "switchover" action.

Considering (1) and (2), I would prefer "device" state to be just
"switchover"...

Further, not sure we can unify the state transition as well (say, we
add this switchover state even without cap "pause-before-switchover"
set, although it does not make much sense itself). Then, we can also
unify the precopy/postcopy state machine into one:

active->
  [pre-switchover->]      (optional, decided by "pause-before-switchover")
    switchover->
      [postcopy-active->] (optional, decided by "postcopy-arm")
        completed

(Sorry I am discussing the naming again instead of reviewing real
 stuff!)

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 2/7] migration: Add 'pre-switchover' and 'device' statuses

[Peter Xu]

On Wed, Oct 18, 2017 at 06:40:08PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Add two statuses for use when the 'pause-before-switchover'
> capability is enabled.
> 
> 'pre-switchover' is the state that we wait in for management
> to allow us to continue.
> 'device' is the state we enter while serialising the devices
> after management gives us the OK.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

I proposed another naming in cover letter, but this one is ok to me as
well.  For either case:

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 3/7] migration: Wait for semaphore before completing migration

[Peter Xu]

On Wed, Oct 18, 2017 at 06:40:09PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Wait for a semaphore before completing the migration,
> if the previously added capability was enabled.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  migration/migration.c | 38 ++++++++++++++++++++++++++++++++++++++
>  migration/migration.h |  3 +++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 86ae0292f0..6c6d5e2c75 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1968,6 +1968,39 @@ fail:
>  }
>  
>  /**
> + * migration_maybe_pause: Pause if required to by
> + * migrate_pause_before_switchover called with the iothread locked
> + * Returns: 0 on success
> + */
> +static int migration_maybe_pause(MigrationState *s, int *current_active_state)
> +{
> +    if (!migrate_pause_before_switchover()) {
> +        return 0;
> +    }
> +
> +    /* Since leaving this state is not atomic with posting the semaphore
> +     * it's possible that someone could have issued multiple migrate_continue
> +     * and the semaphore is incorrectly positive at this point;
> +     * the docs say it's undefined to reinit a semaphore that's already
> +     * init'd, so use timedwait to eat up any existing posts.
> +     */
> +    while (qemu_sem_timedwait(&s->pause_sem, 1) == 0) {
> +        /* This block intentionally left blank */
> +    }

Not sure whether worth generalizing this (along with the comment) into
a function like qemu_sem_consume_all(), but I'm fine with either way,
especially if no respin is planned.

(The comment is not really migration specific as well - IMHO it's a
 general problem for many other semaphores)

Reviewed-by: Peter Xu <peterx@redhat.com>

> +
> +    qemu_mutex_unlock_iothread();
> +    migrate_set_state(&s->state, *current_active_state,
> +                      MIGRATION_STATUS_PRE_SWITCHOVER);
> +    qemu_sem_wait(&s->pause_sem);
> +    migrate_set_state(&s->state, MIGRATION_STATUS_PRE_SWITCHOVER,
> +                      MIGRATION_STATUS_DEVICE);
> +    *current_active_state = MIGRATION_STATUS_DEVICE;
> +    qemu_mutex_lock_iothread();
> +
> +    return s->state == MIGRATION_STATUS_DEVICE ? 0 : -EINVAL;
> +}
> +
> +/**
>   * migration_completion: Used by migration_thread when there's not much left.
>   *   The caller 'breaks' the loop when this returns.
>   *
> @@ -1993,6 +2026,9 @@ static void migration_completion(MigrationState *s, int current_active_state,
>              bool inactivate = !migrate_colo_enabled();
>              ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
>              if (ret >= 0) {
> +                ret = migration_maybe_pause(s, &current_active_state);
> +            }
> +            if (ret >= 0) {
>                  qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
>                  ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
>                                                           inactivate);
> @@ -2373,6 +2409,7 @@ static void migration_instance_finalize(Object *obj)
>  
>      g_free(params->tls_hostname);
>      g_free(params->tls_creds);
> +    qemu_sem_destroy(&ms->pause_sem);
>  }
>  
>  static void migration_instance_init(Object *obj)
> @@ -2383,6 +2420,7 @@ static void migration_instance_init(Object *obj)
>      ms->state = MIGRATION_STATUS_NONE;
>      ms->xbzrle_cache_size = DEFAULT_MIGRATE_CACHE_SIZE;
>      ms->mbps = -1;
> +    qemu_sem_init(&ms->pause_sem, 0);
>  
>      params->tls_hostname = g_strdup("");
>      params->tls_creds = g_strdup("");
> diff --git a/migration/migration.h b/migration/migration.h
> index 969866303e..cd988a99b9 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -121,6 +121,9 @@ struct MigrationState
>      /* Flag set once the migration thread called bdrv_inactivate_all */
>      bool block_inactive;
>  
> +    /* Migration is paused due to pause-before-switchover */
> +    QemuSemaphore pause_sem;
> +
>      /* The semaphore is used to notify COLO thread that failover is finished */
>      QemuSemaphore colo_exit_sem;
>  
> -- 
> 2.13.6
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 4/7] migration: migrate-continue

[Peter Xu]

On Wed, Oct 18, 2017 at 06:40:10PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> A new qmp command allows the caller to continue from a given
> paused state.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 5/7] migrate: HMP migate_continue

[Peter Xu]

On Wed, Oct 18, 2017 at 06:40:11PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> HMP equivalent to the just added migrate-continue
> Unpause a migrate paused at a given state.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 6/7] migration: allow cancel to unpause

[Peter Xu]

On Wed, Oct 18, 2017 at 06:40:12PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> If a migration_cancel is issued during the new paused state,
> kick the pause_sem to get to unpause so it can cancel.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

> ---
>  migration/migration.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index c9a3ee9efd..756deb3e2c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1108,6 +1108,10 @@ static void migrate_fd_cancel(MigrationState *s)
>          if (!migration_is_setup_or_active(old_state)) {
>              break;
>          }
> +        /* If the migration is paused, kick it out of the pause */
> +        if (old_state == MIGRATION_STATUS_PRE_SWITCHOVER) {
> +            qemu_sem_post(&s->pause_sem);
> +        }
>          migrate_set_state(&s->state, old_state, MIGRATION_STATUS_CANCELLING);
>      } while (s->state != MIGRATION_STATUS_CANCELLING);
>  
> -- 
> 2.13.6
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 7/7] migration: pause-before-switchover for postcopy

[Peter Xu]

On Wed, Oct 18, 2017 at 06:40:13PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Add pause-before-switchover support for postcopy.
> After starting postcopy it will transition
>     active->pre-switchover->postcopy_active
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  migration/migration.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 756deb3e2c..af587377e9 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -104,6 +104,9 @@ enum mig_rp_message_type {
>  static MigrationState *current_migration;
>  
>  static bool migration_object_check(MigrationState *ms, Error **errp);
> +static int migration_maybe_pause(MigrationState *s,
> +                                 int *current_active_state,
> +                                 int new_state);
>  
>  void migration_object_init(void)
>  {
> @@ -1830,8 +1833,11 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running)
>      QEMUFile *fb;
>      int64_t time_at_stop = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>      bool restart_block = false;
> -    migrate_set_state(&ms->state, MIGRATION_STATUS_ACTIVE,
> -                      MIGRATION_STATUS_POSTCOPY_ACTIVE);
> +    int cur_state = MIGRATION_STATUS_ACTIVE;
> +    if (!migrate_pause_before_switchover()) {
> +        migrate_set_state(&ms->state, MIGRATION_STATUS_ACTIVE,
> +                          MIGRATION_STATUS_POSTCOPY_ACTIVE);

(Note: so we are sending device data during
 MIGRATION_STATUS_POSTCOPY_ACTIVE state if "pause-before-switchover"
 is not set, and MIGRATION_STATUS_DEVICE state if that is set)

> +    }
>  
>      trace_postcopy_start();
>      qemu_mutex_lock_iothread();
> @@ -1845,6 +1851,12 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running)
>          goto fail;
>      }
>  
> +    ret = migration_maybe_pause(ms, &cur_state,
> +                                MIGRATION_STATUS_POSTCOPY_ACTIVE);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +

I think it works, so:

Reviewed-by: Peter Xu <peterx@redhat.com>

About the unification of state change I mentioned in cover letter,
what I proposed would look like this for postcopy (I would avoid
fiddling with cur_state and related, and for precopy it should be
similar):

migration_pre_switchover (ms)
{
  migrate_set_state(ms, MIGRATION_STATUS_PRE_SWITCHOVER);
  qemu_mutex_unlock_iothread();
  qemu_sem_wait();
  qemu_mutex_lock_iothread();
}

postcopy_start ()
{
  ...

  /* Whether we need extra pre-switchover state transition */
  if (migrate_pause_before_switchover ()) {
    migration_pre_switchover (ms);
  }

  /*
   * To "switchover" state to send device states, etc., no matter
   * whether "pause-before-switchover" is enabled
   */
  migration_set_state (ms, MIGRATION_STATUS_SWITCHOVER);

  /* send device states and the rest postcopy work... */

  migration_set_state (ms, MIGRATION_STATUS_POSTCOPY_ACTIVE)
}

I proposed this only for readability and cleaness of state transition.
At least then we'll know that we are always sending the device data
during SWITCHOVER state (or say, DEVICE state). Again, either way is
fine to me.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 0/7] migration: pause-before-switchover

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Wed, Oct 18, 2017 at 06:40:06PM +0100, Dr. David Alan Gilbert (git) wrote:
> 
> [...]
> 
> > The precopy flow is:
> > active->pre-switchover->device->completed
> > 
> > The postcopy flow is:
> > active->pre-switchover->postcopy-active->completed
> 
> The naming is still slightly confusing to me:
> 
> (1) we have a capability called "pause-before-switchover", so it feels
>     like there is something called "switchover" and if we enable this
>     we'll pause before that point;
> 
> (2) we have a new status "pre-switchover", it feels like that's the
>     point before we are in "switchover" state;
> 
> (3) we don't really have a "switchover" state, but instead it's called
>     "device" which is exactly the "switchover" action.
> 
> Considering (1) and (2), I would prefer "device" state to be just
> "switchover"...

Yes I stuck to pause-before-device and device originally; but
what we're doing during the 'device' stage is mostly saving device
state; the actual switchover occurs at the end.  So hmm.

> Further, not sure we can unify the state transition as well (say, we
> add this switchover state even without cap "pause-before-switchover"
> set, although it does not make much sense itself). Then, we can also
> unify the precopy/postcopy state machine into one:
> 
> active->
>   [pre-switchover->]      (optional, decided by "pause-before-switchover")
>     switchover->
>       [postcopy-active->] (optional, decided by "postcopy-arm")
>         completed

I didn't want to change the state transition behaviour without the
capability set, since that could upset an existing libvirt that would
get confused by the new state.

Dave

> (Sorry I am discussing the naming again instead of reviewing real
>  stuff!)
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 4/7] migration: migrate-continue

[Jiri Denemark]

On Wed, Oct 18, 2017 at 18:40:10 +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> A new qmp command allows the caller to continue from a given
> paused state.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
...
> diff --git a/qapi/migration.json b/qapi/migration.json
> index b56f95db64..3b50afa6e8 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -877,6 +877,23 @@
>  { 'command': 'migrate_cancel' }
>  
>  ##
> +# @migrate-continue:
> +#
> +# Continue migration when it's in a paused state.
> +#
> +# @state: The state the migration is currently expected to be in
> +#
> +# Returns: nothing on success
> +# Since: 2.11
> +# Example:
> +#
> +# -> { "execute": "migrate-continue" , "arguments":
> +#      { "state": "pause-before-device" } }

This example still uses the old name, it should be

     -> { "execute": "migrate-continue" , "arguments":
          { "state": "pre-switchover" } }

> +# <- { "return": {} }
> +##
> +{ 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
> +
> +##
>  # @migrate_set_downtime:
>  #
>  # Set maximum tolerated downtime for migration.

Jirka

Re: [Qemu-devel] [PATCH v3 4/7] migration: migrate-continue

[Dr. David Alan Gilbert]

* Jiri Denemark (jdenemar@redhat.com) wrote:
> On Wed, Oct 18, 2017 at 18:40:10 +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > A new qmp command allows the caller to continue from a given
> > paused state.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ...
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index b56f95db64..3b50afa6e8 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -877,6 +877,23 @@
> >  { 'command': 'migrate_cancel' }
> >  
> >  ##
> > +# @migrate-continue:
> > +#
> > +# Continue migration when it's in a paused state.
> > +#
> > +# @state: The state the migration is currently expected to be in
> > +#
> > +# Returns: nothing on success
> > +# Since: 2.11
> > +# Example:
> > +#
> > +# -> { "execute": "migrate-continue" , "arguments":
> > +#      { "state": "pause-before-device" } }
> 
> This example still uses the old name, it should be
> 
>      -> { "execute": "migrate-continue" , "arguments":
>           { "state": "pre-switchover" } }

oops, thanks; fixed.

Dave

> > +# <- { "return": {} }
> > +##
> > +{ 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
> > +
> > +##
> >  # @migrate_set_downtime:
> >  #
> >  # Set maximum tolerated downtime for migration.
> 
> Jirka
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 0/7] migration: pause-before-switchover

[Jiri Denemark]

The libvirt changes which will make use of this new migration capability
can be found in migration-pause branch of my gitlab repository:

    git fetch https://gitlab.com/jirkade/libvirt.git migration-pause

It's not properly split into patches, it has no commit message etc.,
but the functionality should be complete.

Feel free to test it and report any issues.

Thanks,

Jirka

Re: [Qemu-devel] [PATCH v3 0/7] migration: pause-before-switchover

[Dr. David Alan Gilbert]

* Jiri Denemark (jdenemar@redhat.com) wrote:
> The libvirt changes which will make use of this new migration capability
> can be found in migration-pause branch of my gitlab repository:
> 
>     git fetch https://gitlab.com/jirkade/libvirt.git migration-pause
> 
> It's not properly split into patches, it has no commit message etc.,
> but the functionality should be complete.
> 
> Feel free to test it and report any issues.

Looks promising:

virsh migrate --live --copy-storage-all --verbose

2017-10-19 17:52:38.665+0000: 31999: debug : qemuMonitorSetMigrationCapability:3948 : capability=pause-before-switchover, state=1
2017-10-19 17:52:38.666+0000: 31999: debug : virJSONValueToString:1914 : result={"execute":"migrate-set-capabilities","arguments":{"capabilities":[{"capability":"pause-before-switchover","state":true}]},"id":"libvirt-1861"}
2017-10-19 17:52:38.693+0000: 31999: debug : qemuMonitorJSONCommandWithFd:298 : Send command '{"execute":"migrate","arguments":{"detach":true,"blk":false,"inc":false,"uri":"fd:migrate"},"id":"libvirt-1865"}' for write with FD -1
2017-10-19 17:52:38.695+0000: 31998: debug : qemuMonitorJSONIOProcessLine:193 : Line [{"timestamp": {"seconds": 1508435558, "microseconds": 695732}, "event": "MIGRATION", "data": {"status": "setup"}}]
2017-10-19 17:52:38.743+0000: 31998: debug : qemuMonitorJSONIOProcessLine:193 : Line [{"timestamp": {"seconds": 1508435558, "microseconds": 743564}, "event": "MIGRATION_PASS", "data": {"pass": 1}}]
2017-10-19 17:52:38.744+0000: 31998: debug : qemuMonitorJSONIOProcessLine:193 : Line [{"timestamp": {"seconds": 1508435558, "microseconds": 743724}, "event": "MIGRATION", "data": {"status": "active"}}]
2017-10-19 17:52:43.193+0000: 31998: debug : qemuMonitorJSONIOProcessLine:193 : Line [{"timestamp": {"seconds": 1508435563, "microseconds": 192728}, "event": "MIGRATION_PASS", "data": {"pass": 2}}]
2017-10-19 17:52:43.389+0000: 31998: debug : qemuMonitorJSONIOProcessLine:193 : Line [{"timestamp": {"seconds": 1508435563, "microseconds": 388947}, "event": "STOP"}]
2017-10-19 17:52:43.862+0000: 31998: debug : qemuMonitorJSONIOProcessLine:193 : Line [{"timestamp": {"seconds": 1508435563, "microseconds": 862428}, "event": "MIGRATION", "data": {"status": "pre-switchover"}}]
2017-10-19 17:52:43.863+0000: 31999: debug : qemuMigrationDriveMirrorReady:634 : All disk mirrors are ready
2017-10-19 17:52:43.863+0000: 31999: debug : qemuMigrationCompleted:1534 : Migration paused before switchover
2017-10-19 17:52:43.865+0000: 31998: debug : qemuMonitorJSONIOProcessLine:193 : Line [{"return": {"expected-downtime": 300, "status": "pre-switchover", "setup-time": 47, "total-time": 5169, "ram": {"total": 4430053376, "postcopy-requests": 0, "dirty-sync-count": 2, "page-size": 4096, "remaining": 7204864, "mbps": 941.43529, "transferred": 450864646, "duplicate": 973832, "dirty-pages-rate": 243277, "skipped": 0, "normal-bytes": 441237504, "normal": 107724}}, "id": "libvirt-1876"}]
2017-10-19 17:52:43.866+0000: 31999: debug : qemuMigrationCancelDriveMirror:803 : Cancelling drive mirrors for domain debianlocalqemu
2017-10-19 17:52:43.866+0000: 31999: debug : qemuMonitorJSONCommandWithFd:298 : Send command '{"execute":"block-job-cancel","arguments":{"device":"drive-virtio-disk0"},"id":"libvirt-1877"}' for write with FD -1
2017-10-19 17:52:43.868+0000: 31999: debug : qemuMigrationDriveMirrorCancelled:715 : Waiting for 1 disk mirrors to finish
2017-10-19 17:52:43.872+0000: 31998: info : qemuMonitorIOProcess:439 : QEMU_MONITOR_IO_PROCESS: mon=0x7f4544008840 buf={"timestamp": {"seconds": 1508435563, "microseconds": 871816}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "drive-virtio-disk0", "len": 58430259200, "offset": 58430259200, "speed": 9223372036853727232, "type": "mirror"}}^M
2017-10-19 17:52:43.873+0000: 31998: debug : qemuProcessHandleBlockJob:1014 : Block job for device drive-virtio-disk0 (domain: 0x7f45440254c0,debianlocalqemu) type 2 status 0
2017-10-19 17:52:43.873+0000: 31999: debug : qemuBlockJobEventProcess:106 : disk=vda, mirrorState=yes, type=2, status=0
2017-10-19 17:52:43.916+0000: 31999: debug : qemuMonitorJSONCommandWithFd:298 : Send command '{"execute":"migrate-continue","arguments":{"state":"pre-switchover"},"id":"libvirt-1880"}' for write with FD -1
2017-10-19 17:52:43.918+0000: 31998: debug : qemuMonitorJSONIOProcessLine:193 : Line [{"timestamp": {"seconds": 1508435563, "microseconds": 917872}, "event": "MIGRATION", "data": {"status": "device"}}]
2017-10-19 17:52:43.921+0000: 31998: debug : qemuMonitorJSONIOProcessLine:193 : Line [{"timestamp": {"seconds": 1508435563, "microseconds": 921194}, "event": "MIGRATION_PASS", "data": {"pass": 3}}]
2017-10-19 17:52:43.991+0000: 31998: info : qemuMonitorIOProcess:439 : QEMU_MONITOR_IO_PROCESS: mon=0x7f4544008840 buf={"timestamp": {"seconds": 1508435563, "microseconds": 991528}, "event": "MIGRATION", "data": {"status": "completed"}}^M

So I think libvirt is doing the right thing - thanks!

I'll post the version with your minor comment change 1st thing tomorrow.

(I'm not too convinced qemu is that happy during the drive-mirror; the
guest complained about tasks blocked for 120seconds+ - I was running a
heavy cp;  and looking at an iostat in the guest I could see there 
were a few minute chunks where nothing was happening; and the write
performance after migrate seems way low;  however - neither of those
are related directly to this change - since the first problem
is happening before the actual migration code has started).

Dave

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

Re: [Qemu-devel] [PATCH v3 0/7] migration: pause-before-switchover

[Peter Xu]

On Thu, Oct 19, 2017 at 12:21:23PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > On Wed, Oct 18, 2017 at 06:40:06PM +0100, Dr. David Alan Gilbert (git) wrote:
> > 
> > [...]
> > 
> > > The precopy flow is:
> > > active->pre-switchover->device->completed
> > > 
> > > The postcopy flow is:
> > > active->pre-switchover->postcopy-active->completed
> > 
> > The naming is still slightly confusing to me:
> > 
> > (1) we have a capability called "pause-before-switchover", so it feels
> >     like there is something called "switchover" and if we enable this
> >     we'll pause before that point;
> > 
> > (2) we have a new status "pre-switchover", it feels like that's the
> >     point before we are in "switchover" state;
> > 
> > (3) we don't really have a "switchover" state, but instead it's called
> >     "device" which is exactly the "switchover" action.
> > 
> > Considering (1) and (2), I would prefer "device" state to be just
> > "switchover"...
> 
> Yes I stuck to pause-before-device and device originally; but
> what we're doing during the 'device' stage is mostly saving device
> state; the actual switchover occurs at the end.  So hmm.

That's fine to me.

> 
> > Further, not sure we can unify the state transition as well (say, we
> > add this switchover state even without cap "pause-before-switchover"
> > set, although it does not make much sense itself). Then, we can also
> > unify the precopy/postcopy state machine into one:
> > 
> > active->
> >   [pre-switchover->]      (optional, decided by "pause-before-switchover")
> >     switchover->
> >       [postcopy-active->] (optional, decided by "postcopy-arm")
> >         completed
> 
> I didn't want to change the state transition behaviour without the
> capability set, since that could upset an existing libvirt that would
> get confused by the new state.

Indeed.  However this (and also Juan's xbzrle cache size series) lets
me think about whether we should loosen the "compatibility" sometimes.

For most of the times, we are paying the compatibility bill by
complicating the code logic.  For this one, we satisfy live block
migration logic to introduce two new state transition paths (for
precopy and postcopy). I am just afraid we need to pay a larger bill
some day.

But I'd say it's only my worry; maybe it's just too superfluous.

(I provided all r-bs, so the series looks good to me after all)

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 0/7] migration: pause-before-switchover

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> On Thu, Oct 19, 2017 at 12:21:23PM +0100, Dr. David Alan Gilbert wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
> > > On Wed, Oct 18, 2017 at 06:40:06PM +0100, Dr. David Alan Gilbert (git) wrote:
> > > 
> > > [...]
> > > 
> > > > The precopy flow is:
> > > > active->pre-switchover->device->completed
> > > > 
> > > > The postcopy flow is:
> > > > active->pre-switchover->postcopy-active->completed
> > > 
> > > The naming is still slightly confusing to me:
> > > 
> > > (1) we have a capability called "pause-before-switchover", so it feels
> > >     like there is something called "switchover" and if we enable this
> > >     we'll pause before that point;
> > > 
> > > (2) we have a new status "pre-switchover", it feels like that's the
> > >     point before we are in "switchover" state;
> > > 
> > > (3) we don't really have a "switchover" state, but instead it's called
> > >     "device" which is exactly the "switchover" action.
> > > 
> > > Considering (1) and (2), I would prefer "device" state to be just
> > > "switchover"...
> > 
> > Yes I stuck to pause-before-device and device originally; but
> > what we're doing during the 'device' stage is mostly saving device
> > state; the actual switchover occurs at the end.  So hmm.
> 
> That's fine to me.
> 
> > 
> > > Further, not sure we can unify the state transition as well (say, we
> > > add this switchover state even without cap "pause-before-switchover"
> > > set, although it does not make much sense itself). Then, we can also
> > > unify the precopy/postcopy state machine into one:
> > > 
> > > active->
> > >   [pre-switchover->]      (optional, decided by "pause-before-switchover")
> > >     switchover->
> > >       [postcopy-active->] (optional, decided by "postcopy-arm")
> > >         completed
> > 
> > I didn't want to change the state transition behaviour without the
> > capability set, since that could upset an existing libvirt that would
> > get confused by the new state.
> 
> Indeed.  However this (and also Juan's xbzrle cache size series) lets
> me think about whether we should loosen the "compatibility" sometimes.
> 
> For most of the times, we are paying the compatibility bill by
> complicating the code logic.  For this one, we satisfy live block
> migration logic to introduce two new state transition paths (for
> precopy and postcopy). I am just afraid we need to pay a larger bill
> some day.
> 
> But I'd say it's only my worry; maybe it's just too superfluous.

Yes, it's true - almost all the behaviour we have forms part of our
API that we expose to libvirt; we have to be pretty careful.

> (I provided all r-bs, so the series looks good to me after all)

Thanks! One comment fix coming up soon as spotted by Jiri.

Dave

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