[Qemu-devel] [PATCH V2 0/7] Migration/colo: Fix upstream bugs when occur failover

[Qemu-devel] [PATCH V2 0/7] Migration/colo: Fix upstream bugs when occur failover

[Zhang Chen]

From: Zhang Chen <chen.zhang@intel.com>

This series focus on COLO failover bug fix and optimization.

V2:
 - Add patch 4/7 to handle failover state. 
 - Add new patches to optimize failover status.

V1:
 - Init patch.

Zhang Chen (7):
  Migration/colo.c: Fix double close bug when occur COLO failover
  Migration/colo.c: Fix COLO failover status error
  Migration/colo.c: Make COLO node running after failover
  Migration/colo.c: Add new COLOExitReason to handle all failover state
  qapi/migration.json: Remove a variable that doesn't exist in example
  Migration/colo.c: Add the necessary checks for colo_do_failover
  Migration/colo.c: Make user obtain the COLO mode info after failover

 migration/colo.c      | 69 ++++++++++++++++++++++++++++++-------------
 migration/migration.c |  3 ++
 qapi/migration.json   | 17 ++++++-----
 3 files changed, 61 insertions(+), 28 deletions(-)

-- 
2.17.GIT

[Qemu-devel] [PATCH V2 1/7] Migration/colo.c: Fix double close bug when occur COLO failover

[Zhang Chen]

From: Zhang Chen <chen.zhang@intel.com>

In migration_incoming_state_destroy(void) will check the mis->to_src_file
to double close the mis->to_src_file when occur COLO failover.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/colo.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/colo.c b/migration/colo.c
index 398b239d1c..a916dc178c 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -872,6 +872,7 @@ out:
     /* Must be called after failover BH is completed */
     if (mis->to_src_file) {
         qemu_fclose(mis->to_src_file);
+        mis->to_src_file = NULL;
     }
     migration_incoming_disable_colo();
 
-- 
2.17.GIT

[Qemu-devel] [PATCH V2 2/7] Migration/colo.c: Fix COLO failover status error

[Zhang Chen]

From: Zhang Chen <chen.zhang@intel.com>

When finished COLO failover, the status is FAILOVER_STATUS_COMPLETED.
The origin codes misunderstand the FAILOVER_STATUS_REQUIRE.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 migration/colo.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index a916dc178c..a13acac192 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -121,6 +121,7 @@ static void secondary_vm_do_failover(void)
     }
     /* Notify COLO incoming thread that failover work is finished */
     qemu_sem_post(&mis->colo_incoming_sem);
+
     /* For Secondary VM, jump to incoming co */
     if (mis->migration_incoming_co) {
         qemu_coroutine_enter(mis->migration_incoming_co);
@@ -262,7 +263,7 @@ COLOStatus *qmp_query_colo_status(Error **errp)
     case FAILOVER_STATUS_NONE:
         s->reason = COLO_EXIT_REASON_NONE;
         break;
-    case FAILOVER_STATUS_REQUIRE:
+    case FAILOVER_STATUS_COMPLETED:
         s->reason = COLO_EXIT_REASON_REQUEST;
         break;
     default:
@@ -582,7 +583,7 @@ out:
         qapi_event_send_colo_exit(COLO_MODE_PRIMARY,
                                   COLO_EXIT_REASON_ERROR);
         break;
-    case FAILOVER_STATUS_REQUIRE:
+    case FAILOVER_STATUS_COMPLETED:
         qapi_event_send_colo_exit(COLO_MODE_PRIMARY,
                                   COLO_EXIT_REASON_REQUEST);
         break;
@@ -854,7 +855,7 @@ out:
         qapi_event_send_colo_exit(COLO_MODE_SECONDARY,
                                   COLO_EXIT_REASON_ERROR);
         break;
-    case FAILOVER_STATUS_REQUIRE:
+    case FAILOVER_STATUS_COMPLETED:
         qapi_event_send_colo_exit(COLO_MODE_SECONDARY,
                                   COLO_EXIT_REASON_REQUEST);
         break;
-- 
2.17.GIT

[Qemu-devel] [PATCH V2 3/7] Migration/colo.c: Make COLO node running after failover

[Zhang Chen]

From: Zhang Chen <chen.zhang@intel.com>

Delay to close COLO for auto start VM after failover.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/colo.c      | 1 -
 migration/migration.c | 3 +++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/migration/colo.c b/migration/colo.c
index a13acac192..89325952c7 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -875,7 +875,6 @@ out:
         qemu_fclose(mis->to_src_file);
         mis->to_src_file = NULL;
     }
-    migration_incoming_disable_colo();
 
     rcu_unregister_thread();
     return NULL;
diff --git a/migration/migration.c b/migration/migration.c
index 37e06b76dc..cec5f529c3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -383,6 +383,9 @@ static void process_incoming_migration_bh(void *opaque)
         } else {
             runstate_set(RUN_STATE_PAUSED);
         }
+    } else if (migration_incoming_colo_enabled()) {
+        migration_incoming_disable_colo();
+        vm_start();
     } else {
         runstate_set(global_state_get_runstate());
     }
-- 
2.17.GIT

[Qemu-devel] [PATCH V2 4/7] Migration/colo.c: Add new COLOExitReason to handle all failover state

[Zhang Chen]

From: Zhang Chen <chen.zhang@intel.com>

In this patch we add the processing state for COLOExitReason,
because we have to identify COLO in the failover processing state or
failover error state. In the way, we can handle all the failover state.
We have improved the description of the COLOExitReason by the way.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 migration/colo.c    | 24 +++++++++++++-----------
 qapi/migration.json | 15 +++++++++------
 2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index 89325952c7..dbe2b88807 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -267,7 +267,11 @@ COLOStatus *qmp_query_colo_status(Error **errp)
         s->reason = COLO_EXIT_REASON_REQUEST;
         break;
     default:
-        s->reason = COLO_EXIT_REASON_ERROR;
+        if (migration_in_colo_state()) {
+            s->reason = COLO_EXIT_REASON_PROCESSING;
+        } else {
+            s->reason = COLO_EXIT_REASON_ERROR;
+        }
     }
 
     return s;
@@ -579,16 +583,13 @@ out:
      * or the user triggered failover.
      */
     switch (failover_get_state()) {
-    case FAILOVER_STATUS_NONE:
-        qapi_event_send_colo_exit(COLO_MODE_PRIMARY,
-                                  COLO_EXIT_REASON_ERROR);
-        break;
     case FAILOVER_STATUS_COMPLETED:
         qapi_event_send_colo_exit(COLO_MODE_PRIMARY,
                                   COLO_EXIT_REASON_REQUEST);
         break;
     default:
-        abort();
+        qapi_event_send_colo_exit(COLO_MODE_PRIMARY,
+                                  COLO_EXIT_REASON_ERROR);
     }
 
     /* Hope this not to be too long to wait here */
@@ -850,17 +851,18 @@ out:
         error_report_err(local_err);
     }
 
+    /*
+     * There are only two reasons we can get here, some error happened
+     * or the user triggered failover.
+     */
     switch (failover_get_state()) {
-    case FAILOVER_STATUS_NONE:
-        qapi_event_send_colo_exit(COLO_MODE_SECONDARY,
-                                  COLO_EXIT_REASON_ERROR);
-        break;
     case FAILOVER_STATUS_COMPLETED:
         qapi_event_send_colo_exit(COLO_MODE_SECONDARY,
                                   COLO_EXIT_REASON_REQUEST);
         break;
     default:
-        abort();
+        qapi_event_send_colo_exit(COLO_MODE_SECONDARY,
+                                  COLO_EXIT_REASON_ERROR);
     }
 
     if (fb) {
diff --git a/qapi/migration.json b/qapi/migration.json
index 7a795ecc16..48e21880a3 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -983,19 +983,22 @@
 ##
 # @COLOExitReason:
 #
-# The reason for a COLO exit
+# Describe the reason for COLO exit.
 #
-# @none: no failover has ever happened. This can't occur in the
-# COLO_EXIT event, only in the result of query-colo-status.
+# @none: failover has never happened. This state does not occurred
+# in the COLO_EXIT event, only happened in the result of
+# query-colo-status.
 #
-# @request: COLO exit is due to an external request
+# @request: COLO exit caused by an external request.
 #
-# @error: COLO exit is due to an internal error
+# @error: COLO exit caused by an internal error.
+#
+# @processing: COLO in failover handling state.
 #
 # Since: 3.1
 ##
 { 'enum': 'COLOExitReason',
-  'data': [ 'none', 'request', 'error' ] }
+  'data': [ 'none', 'request', 'error' , 'processing' ] }
 
 ##
 # @x-colo-lost-heartbeat:
-- 
2.17.GIT

Re: [Qemu-devel] [PATCH V2 4/7] Migration/colo.c: Add new COLOExitReason to handle all failover state

[Eric Blake]

On 2/28/19 10:55 AM, Zhang Chen wrote:
> From: Zhang Chen <chen.zhang@intel.com>
> 
> In this patch we add the processing state for COLOExitReason,
> because we have to identify COLO in the failover processing state or
> failover error state. In the way, we can handle all the failover state.
> We have improved the description of the COLOExitReason by the way.
> 
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  migration/colo.c    | 24 +++++++++++++-----------
>  qapi/migration.json | 15 +++++++++------
>  2 files changed, 22 insertions(+), 17 deletions(-)
> 

> +++ b/qapi/migration.json
> @@ -983,19 +983,22 @@
>  ##
>  # @COLOExitReason:
>  #
> -# The reason for a COLO exit
> +# Describe the reason for COLO exit.
>  #
> -# @none: no failover has ever happened. This can't occur in the
> -# COLO_EXIT event, only in the result of query-colo-status.
> +# @none: failover has never happened. This state does not occurred
> +# in the COLO_EXIT event, only happened in the result of
> +# query-colo-status.

Grammar suggestion:

This state does not occur in the COLO_EXIT event, and is only visible in
the result of query-colo-status.

>  #
> -# @request: COLO exit is due to an external request
> +# @request: COLO exit caused by an external request.
>  #
> -# @error: COLO exit is due to an internal error
> +# @error: COLO exit caused by an internal error.
> +#
> +# @processing: COLO in failover handling state.

Missing a (since 4.0) tag.  Maybe:

@processing: COLO is currently handling a failover (since 4.0).

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

Re: [Qemu-devel] [PATCH V2 4/7] Migration/colo.c: Add new COLOExitReason to handle all failover state

[Zhang, Chen]

-----Original Message-----
From: Eric Blake [mailto:eblake@redhat.com] 
Sent: Friday, March 1, 2019 1:05 AM
To: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian <lizhijian@cn.fujitsu.com>; Zhang Chen <zhangckid@gmail.com>; Dr. David Alan Gilbert <dgilbert@redhat.com>; Juan Quintela <quintela@redhat.com>; zhanghailiang <zhang.zhanghailiang@huawei.com>; Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-devel@nongnu.org>
Subject: Re: [PATCH V2 4/7] Migration/colo.c: Add new COLOExitReason to handle all failover state

On 2/28/19 10:55 AM, Zhang Chen wrote:
> From: Zhang Chen <chen.zhang@intel.com>
> 
> In this patch we add the processing state for COLOExitReason, because 
> we have to identify COLO in the failover processing state or failover 
> error state. In the way, we can handle all the failover state.
> We have improved the description of the COLOExitReason by the way.
> 
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  migration/colo.c    | 24 +++++++++++++-----------
>  qapi/migration.json | 15 +++++++++------
>  2 files changed, 22 insertions(+), 17 deletions(-)
> 

> +++ b/qapi/migration.json
> @@ -983,19 +983,22 @@
>  ##
>  # @COLOExitReason:
>  #
> -# The reason for a COLO exit
> +# Describe the reason for COLO exit.
>  #
> -# @none: no failover has ever happened. This can't occur in the -# 
> COLO_EXIT event, only in the result of query-colo-status.
> +# @none: failover has never happened. This state does not occurred # 
> +in the COLO_EXIT event, only happened in the result of # 
> +query-colo-status.

Grammar suggestion:

This state does not occur in the COLO_EXIT event, and is only visible in the result of query-colo-status.

OK~ I will fix it in next version.

>  #
> -# @request: COLO exit is due to an external request
> +# @request: COLO exit caused by an external request.
>  #
> -# @error: COLO exit is due to an internal error
> +# @error: COLO exit caused by an internal error.
> +#
> +# @processing: COLO in failover handling state.

Missing a (since 4.0) tag.  Maybe:

@processing: COLO is currently handling a failover (since 4.0).

Sure, thank you for your comments~


Thanks
Zhang Chen

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

[Qemu-devel] [PATCH V2 5/7] qapi/migration.json: Remove a variable that doesn't exist in example

[Zhang Chen]

From: Zhang Chen <chen.zhang@intel.com>

Remove the "active" variable in example for query-colo-status.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 qapi/migration.json | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index 48e21880a3..f4c1762dfc 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1342,7 +1342,7 @@
 # Example:
 #
 # -> { "execute": "query-colo-status" }
-# <- { "return": { "mode": "primary", "active": true, "reason": "request" } }
+# <- { "return": { "mode": "primary", "reason": "request" } }
 #
 # Since: 3.1
 ##
-- 
2.17.GIT

Re: [Qemu-devel] [PATCH V2 5/7] qapi/migration.json: Remove a variable that doesn't exist in example

[Eric Blake]

On 2/28/19 10:55 AM, Zhang Chen wrote:
> From: Zhang Chen <chen.zhang@intel.com>
> 
> Remove the "active" variable in example for query-colo-status.
> 
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  qapi/migration.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

Might be nice to mention that the doc bug has been present since the
command's introduction in commit f56c0065.

> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 48e21880a3..f4c1762dfc 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1342,7 +1342,7 @@
>  # Example:
>  #
>  # -> { "execute": "query-colo-status" }
> -# <- { "return": { "mode": "primary", "active": true, "reason": "request" } }
> +# <- { "return": { "mode": "primary", "reason": "request" } }
>  #
>  # Since: 3.1
>  ##
> 

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

Re: [Qemu-devel] [PATCH V2 5/7] qapi/migration.json: Remove a variable that doesn't exist in example

[Zhang, Chen]

-----Original Message-----
From: Eric Blake [mailto:eblake@redhat.com] 
Sent: Friday, March 1, 2019 1:07 AM
To: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian <lizhijian@cn.fujitsu.com>; Zhang Chen <zhangckid@gmail.com>; Dr. David Alan Gilbert <dgilbert@redhat.com>; Juan Quintela <quintela@redhat.com>; zhanghailiang <zhang.zhanghailiang@huawei.com>; Markus Armbruster <armbru@redhat.com>; qemu-dev <qemu-devel@nongnu.org>
Subject: Re: [PATCH V2 5/7] qapi/migration.json: Remove a variable that doesn't exist in example

On 2/28/19 10:55 AM, Zhang Chen wrote:
> From: Zhang Chen <chen.zhang@intel.com>
> 
> Remove the "active" variable in example for query-colo-status.
> 
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  qapi/migration.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

Might be nice to mention that the doc bug has been present since the command's introduction in commit f56c0065.

Sure, will write in next version.

Thanks
Zhang Chen

> 
> diff --git a/qapi/migration.json b/qapi/migration.json index 
> 48e21880a3..f4c1762dfc 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1342,7 +1342,7 @@
>  # Example:
>  #
>  # -> { "execute": "query-colo-status" } -# <- { "return": { "mode": 
> "primary", "active": true, "reason": "request" } }
> +# <- { "return": { "mode": "primary", "reason": "request" } }
>  #
>  # Since: 3.1
>  ##
> 

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

[Qemu-devel] [PATCH V2 6/7] Migration/colo.c: Add the necessary checks for colo_do_failover

[Zhang Chen]

From: Zhang Chen <chen.zhang@intel.com>

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 migration/colo.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index dbe2b88807..d1ae2e6d11 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -197,10 +197,16 @@ void colo_do_failover(MigrationState *s)
         vm_stop_force_state(RUN_STATE_COLO);
     }
 
-    if (get_colo_mode() == COLO_MODE_PRIMARY) {
+    switch (get_colo_mode()) {
+    case COLO_MODE_PRIMARY:
         primary_vm_do_failover();
-    } else {
+        break;
+    case COLO_MODE_SECONDARY:
         secondary_vm_do_failover();
+        break;
+    default:
+        error_report("colo_do_failover failed because the colo mode"
+                     " could not be obtained");
     }
 }
 
-- 
2.17.GIT

[Qemu-devel] [PATCH V2 7/7] Migration/colo.c: Make user obtain the COLO mode info after failover

[Zhang Chen]

From: Zhang Chen <chen.zhang@intel.com>

Add the last_colo_mode to save the status after failover.
This patch can solve the issue that user got nothing to call
query_colo_status after failover.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 migration/colo.c | 28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

diff --git a/migration/colo.c b/migration/colo.c
index d1ae2e6d11..6eba8e06f2 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -38,6 +38,9 @@
 static bool vmstate_loading;
 static Notifier packets_compare_notifier;
 
+/* User need to know colo mode after COLO failover */
+static COLOMode last_colo_mode;
+
 #define COLO_BUFFER_BASE_SIZE (4 * 1024 * 1024)
 
 bool migration_in_colo_state(void)
@@ -197,7 +200,10 @@ void colo_do_failover(MigrationState *s)
         vm_stop_force_state(RUN_STATE_COLO);
     }
 
-    switch (get_colo_mode()) {
+    /* Update last_COLO_mode to avoid unexpectedly exit COLO status */
+    last_colo_mode = get_colo_mode();
+
+    switch (last_colo_mode) {
     case COLO_MODE_PRIMARY:
         primary_vm_do_failover();
         break;
@@ -263,7 +269,7 @@ COLOStatus *qmp_query_colo_status(Error **errp)
 {
     COLOStatus *s = g_new0(COLOStatus, 1);
 
-    s->mode = get_colo_mode();
+    s->mode = last_colo_mode;
 
     switch (failover_get_state()) {
     case FAILOVER_STATUS_NONE:
@@ -515,6 +521,12 @@ static void colo_process_checkpoint(MigrationState *s)
     Error *local_err = NULL;
     int ret;
 
+    last_colo_mode = get_colo_mode();
+    if (last_colo_mode != COLO_MODE_PRIMARY) {
+        error_report("COLO mode must be COLO_MODE_PRIMARY");
+        return;
+    }
+
     failover_init_state();
 
     s->rp_state.from_dst_file = qemu_file_get_return_path(s->to_dst_file);
@@ -682,12 +694,18 @@ void *colo_process_incoming_thread(void *opaque)
     Error *local_err = NULL;
     int ret;
 
-    rcu_register_thread();
-    qemu_sem_init(&mis->colo_incoming_sem, 0);
-
     migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
                       MIGRATION_STATUS_COLO);
 
+    last_colo_mode = get_colo_mode();
+    if (last_colo_mode != COLO_MODE_SECONDARY) {
+        error_report("COLO mode must be COLO_MODE_SECONDARY");
+        return NULL;
+    }
+
+    rcu_register_thread();
+    qemu_sem_init(&mis->colo_incoming_sem, 0);
+
     failover_init_state();
 
     mis->to_src_file = qemu_file_get_return_path(mis->from_src_file);
-- 
2.17.GIT