[PATCH 0/3] migration: Three more fixes for postcopy recovery

[PATCH 0/3] migration: Three more fixes for postcopy recovery

[Peter Xu]

A few more issues spotted by either Xiaohui or me.  Please see and review
individual patches for what they do.  Thanks,

Peter Xu (3):
  migration: Release return path early for paused postcopy
  migration: Don't do migrate cleanup if during postcopy resume
  migration: Clear error at entry of migrate_fd_connect()

 migration/migration.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

-- 
2.31.1

[PATCH 1/3] migration: Release return path early for paused postcopy

[Peter Xu]

When postcopy pause triggered, we rely on the migration thread to cleanup the
to_dst_file handle, and the return path thread to cleanup the from_dst_file
handle (which is stored in the local variable "rp").

Within the process, from_dst_file cleanup (qemu_fclose) is postponed until it's
setup again due to a postcopy recovery.

It used to work before yank was born; after yank is introduced we rely on the
refcount of IOC to correctly unregister yank function in channel_close().  If
without the early and on-time release of from_dst_file handle the yank function
will be leftover during paused postcopy.

Without this patch, below steps (quoted from Xiaohui) could trigger qemu src
crash:

  1.Boot vm on src host
  2.Boot vm on dst host
  3.Enable postcopy on src&dst host
  4.Load stressapptest in vm and set postcopy speed to 50M
  5.Start migration from src to dst host, change into postcopy mode when migration is active.
  6.When postcopy is active, down the network card(do migration via this network) on dst host.
  7.Wait untill postcopy is paused on src&dst host.
  8.Before up network card, recover migration on dst host, will get error like following.
  9.Ignore the error of step 8, go on recovering migration on src host:

  After step 9, qemu on src host will core dump after some seconds:
  qemu-kvm: ../util/yank.c:107: yank_unregister_instance: Assertion `QLIST_EMPTY(&entry->yankfns)' failed.
  1.sh: line 38: 44662 Aborted                 (core dumped)

Reported-by: Li Xiaohui <xiaohuixiaohli@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 5ff7ba9d5c..8786104c9a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2818,12 +2818,12 @@ out:
              * Maybe there is something we can do: it looks like a
              * network down issue, and we pause for a recovery.
              */
+            qemu_fclose(rp);
+            ms->rp_state.from_dst_file = NULL;
+            rp = NULL;
             if (postcopy_pause_return_path_thread(ms)) {
                 /* Reload rp, reset the rest */
-                if (rp != ms->rp_state.from_dst_file) {
-                    qemu_fclose(rp);
-                    rp = ms->rp_state.from_dst_file;
-                }
+                rp = ms->rp_state.from_dst_file;
                 ms->rp_state.error = false;
                 goto retry;
             }
-- 
2.31.1

Re: [PATCH 1/3] migration: Release return path early for paused postcopy

[Peter Xu]

On Thu, Jul 08, 2021 at 03:06:51PM -0400, Peter Xu wrote:
> Reported-by: Li Xiaohui <xiaohuixiaohli@redhat.com>
                           ^^^^^^^^^^^^^^^^^^^^^^^^^
I don't know how did I messed this up.. but it should be <xiaohli@redhat.com>

-- 
Peter Xu

Re: [PATCH 1/3] migration: Release return path early for paused postcopy

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> When postcopy pause triggered, we rely on the migration thread to cleanup the
> to_dst_file handle, and the return path thread to cleanup the from_dst_file
> handle (which is stored in the local variable "rp").
> 
> Within the process, from_dst_file cleanup (qemu_fclose) is postponed until it's
> setup again due to a postcopy recovery.
> 
> It used to work before yank was born; after yank is introduced we rely on the
> refcount of IOC to correctly unregister yank function in channel_close().  If
> without the early and on-time release of from_dst_file handle the yank function
> will be leftover during paused postcopy.
> 
> Without this patch, below steps (quoted from Xiaohui) could trigger qemu src
> crash:
> 
>   1.Boot vm on src host
>   2.Boot vm on dst host
>   3.Enable postcopy on src&dst host
>   4.Load stressapptest in vm and set postcopy speed to 50M
>   5.Start migration from src to dst host, change into postcopy mode when migration is active.
>   6.When postcopy is active, down the network card(do migration via this network) on dst host.
>   7.Wait untill postcopy is paused on src&dst host.
>   8.Before up network card, recover migration on dst host, will get error like following.
>   9.Ignore the error of step 8, go on recovering migration on src host:
> 
>   After step 9, qemu on src host will core dump after some seconds:
>   qemu-kvm: ../util/yank.c:107: yank_unregister_instance: Assertion `QLIST_EMPTY(&entry->yankfns)' failed.
>   1.sh: line 38: 44662 Aborted                 (core dumped)
> 
> Reported-by: Li Xiaohui <xiaohuixiaohli@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

(and I can cleanup the email address problem)

> ---
>  migration/migration.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 5ff7ba9d5c..8786104c9a 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2818,12 +2818,12 @@ out:
>               * Maybe there is something we can do: it looks like a
>               * network down issue, and we pause for a recovery.
>               */
> +            qemu_fclose(rp);
> +            ms->rp_state.from_dst_file = NULL;
> +            rp = NULL;
>              if (postcopy_pause_return_path_thread(ms)) {
>                  /* Reload rp, reset the rest */
> -                if (rp != ms->rp_state.from_dst_file) {
> -                    qemu_fclose(rp);
> -                    rp = ms->rp_state.from_dst_file;
> -                }
> +                rp = ms->rp_state.from_dst_file;
>                  ms->rp_state.error = false;
>                  goto retry;
>              }
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 2/3] migration: Don't do migrate cleanup if during postcopy resume

[Peter Xu]

Below process could crash qemu with postcopy recovery:

  1. (hmp) migrate -d ..
  2. (hmp) migrate_start_postcopy
  3. [network down, postcopy paused]
  4. (hmp) migrate -r $WRONG_PORT
     when try the recover on an invalid $WRONG_PORT, cleanup_bh will be cleared
  5. (hmp) migrate -r $RIGHT_PORT
     [qemu crash on assert(cleanup_bh)]

The thing is we shouldn't cleanup if it's postcopy resume; the error is set
mostly because the channel is wrong, so we return directly waiting for the user
to retry.

migrate_fd_cleanup() should only be called when migration is cancelled or
completed.

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

diff --git a/migration/migration.c b/migration/migration.c
index 8786104c9a..bb1edf862a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3975,7 +3975,18 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
     }
     if (error_in) {
         migrate_fd_error(s, error_in);
-        migrate_fd_cleanup(s);
+        if (resume) {
+            /*
+             * Don't do cleanup for resume if channel is invalid, but only dump
+             * the error.  We wait for another channel connect from the user.
+             * The error_report still gives HMP user a hint on what failed.
+             * It's normally done in migrate_fd_cleanup(), but call it here
+             * explicitly.
+             */
+            error_report_err(error_copy(s->error));
+        } else {
+            migrate_fd_cleanup(s);
+        }
         return;
     }
 
-- 
2.31.1

Re: [PATCH 2/3] migration: Don't do migrate cleanup if during postcopy resume

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> Below process could crash qemu with postcopy recovery:
> 
>   1. (hmp) migrate -d ..
>   2. (hmp) migrate_start_postcopy
>   3. [network down, postcopy paused]
>   4. (hmp) migrate -r $WRONG_PORT
>      when try the recover on an invalid $WRONG_PORT, cleanup_bh will be cleared
>   5. (hmp) migrate -r $RIGHT_PORT
>      [qemu crash on assert(cleanup_bh)]
> 
> The thing is we shouldn't cleanup if it's postcopy resume; the error is set
> mostly because the channel is wrong, so we return directly waiting for the user
> to retry.
> 
> migrate_fd_cleanup() should only be called when migration is cancelled or
> completed.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

> ---
>  migration/migration.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 8786104c9a..bb1edf862a 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3975,7 +3975,18 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>      }
>      if (error_in) {
>          migrate_fd_error(s, error_in);
> -        migrate_fd_cleanup(s);
> +        if (resume) {
> +            /*
> +             * Don't do cleanup for resume if channel is invalid, but only dump
> +             * the error.  We wait for another channel connect from the user.
> +             * The error_report still gives HMP user a hint on what failed.
> +             * It's normally done in migrate_fd_cleanup(), but call it here
> +             * explicitly.
> +             */
> +            error_report_err(error_copy(s->error));
> +        } else {
> +            migrate_fd_cleanup(s);
> +        }
>          return;
>      }
>  
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 3/3] migration: Clear error at entry of migrate_fd_connect()

[Peter Xu]

For each "migrate" command, remember to clear the s->error before going on.
For one reason, when there's a new error it'll be still remembered; see
migrate_set_error() who only sets the error if error==NULL.  Meanwhile if a
failed migration completes (e.g., postcopy recovered and finished), we
shouldn't dump an error when calling migrate_fd_cleanup() at last.

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

diff --git a/migration/migration.c b/migration/migration.c
index bb1edf862a..338df01e97 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1855,6 +1855,15 @@ void migrate_set_error(MigrationState *s, const Error *error)
     }
 }
 
+static void migrate_error_free(MigrationState *s)
+{
+    QEMU_LOCK_GUARD(&s->error_mutex);
+    if (s->error) {
+        error_free(s->error);
+        s->error = NULL;
+    }
+}
+
 void migrate_fd_error(MigrationState *s, const Error *error)
 {
     trace_migrate_fd_error(error_get_pretty(error));
@@ -3966,6 +3975,13 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
     int64_t rate_limit;
     bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
 
+    /*
+     * If there's a previous error, free it and prepare for another one.
+     * Meanwhile if migration completes successfully, there won't have an error
+     * dumped when calling migrate_fd_cleanup().
+     */
+    migrate_error_free(s);
+
     s->expected_downtime = s->parameters.downtime_limit;
     if (resume) {
         assert(s->cleanup_bh);
-- 
2.31.1

Re: [PATCH 3/3] migration: Clear error at entry of migrate_fd_connect()

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> For each "migrate" command, remember to clear the s->error before going on.
> For one reason, when there's a new error it'll be still remembered; see
> migrate_set_error() who only sets the error if error==NULL.  Meanwhile if a
> failed migration completes (e.g., postcopy recovered and finished), we
> shouldn't dump an error when calling migrate_fd_cleanup() at last.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

> ---
>  migration/migration.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index bb1edf862a..338df01e97 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1855,6 +1855,15 @@ void migrate_set_error(MigrationState *s, const Error *error)
>      }
>  }
>  
> +static void migrate_error_free(MigrationState *s)
> +{
> +    QEMU_LOCK_GUARD(&s->error_mutex);
> +    if (s->error) {
> +        error_free(s->error);
> +        s->error = NULL;
> +    }
> +}
> +
>  void migrate_fd_error(MigrationState *s, const Error *error)
>  {
>      trace_migrate_fd_error(error_get_pretty(error));
> @@ -3966,6 +3975,13 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>      int64_t rate_limit;
>      bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
>  
> +    /*
> +     * If there's a previous error, free it and prepare for another one.
> +     * Meanwhile if migration completes successfully, there won't have an error
> +     * dumped when calling migrate_fd_cleanup().
> +     */
> +    migrate_error_free(s);
> +
>      s->expected_downtime = s->parameters.downtime_limit;
>      if (resume) {
>          assert(s->cleanup_bh);
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 0/3] migration: Three more fixes for postcopy recovery

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> A few more issues spotted by either Xiaohui or me.  Please see and review
> individual patches for what they do.  Thanks,
> 
> Peter Xu (3):
>   migration: Release return path early for paused postcopy
>   migration: Don't do migrate cleanup if during postcopy resume
>   migration: Clear error at entry of migrate_fd_connect()
> 
>  migration/migration.c | 37 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 32 insertions(+), 5 deletions(-)

Queued

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