[PATCH 0/2] migration: Two fixes around yank and postcopy recovery

[PATCH 0/2] migration: Two fixes around yank and postcopy recovery

[Peter Xu]

The 1st patch should fix yank with unregister instance; I think it should also
fix the issue that Leonardo used to fix in this patch:

https://lore.kernel.org/qemu-devel/20210629050522.147057-1-leobras@redhat.com/

The 2nd patch fixes postcopy recovery cannot retry if e.g. the 1st attempt
provided a wrong port address.

Note that the multifd zstd test may fail if run migration-test with sudo on
master (which seems to be a known issue now), and it'll still fail after these
two patches applied, however all running tests keep usual.

(Leo: please let me know if this series didn't fix the issue you used to fix)

Please review, thanks.

Peter Xu (2):
  migration: Move yank outside qemu_start_incoming_migration()
  migration: Allow reset of postcopy_recover_triggered when failed

 migration/migration.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

-- 
2.31.1

[PATCH 1/2] migration: Move yank outside qemu_start_incoming_migration()

[Peter Xu]

Starting from commit b5eea99ec2f5c, qmp_migrate_recover() calls unregister
before calling qemu_start_incoming_migration(). I believe it wanted to mitigate
the next call to yank_register_instance(), but I think that's wrong.

Firstly, if during recover, we should keep the yank instance there, not
"quickly removing and adding it back".

Meanwhile, calling qmp_migrate_recover() twice with b5eea99ec2f5c will directly
crash the dest qemu (right now it can't; but it'll start to work right after
the next patch) because the 1st call of qmp_migrate_recover() will unregister
permanently when the channel failed to establish, then the 2nd call of
qmp_migrate_recover() crashes at yank_unregister_instance().

This patch fixes it by moving yank ops out of qemu_start_incoming_migration()
into qmp_migrate_incoming.  For qmp_migrate_recover(), drop the unregister of
yank instance too since we keep it there during the recovery phase.

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

diff --git a/migration/migration.c b/migration/migration.c
index 4228635d18..1bb03d1eca 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -456,10 +456,6 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
     const char *p = NULL;
 
-    if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
-        return;
-    }
-
     qapi_event_send_migration(MIGRATION_STATUS_SETUP);
     if (strstart(uri, "tcp:", &p) ||
         strstart(uri, "unix:", NULL) ||
@@ -474,7 +470,6 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
     } else if (strstart(uri, "fd:", &p)) {
         fd_start_incoming_migration(p, errp);
     } else {
-        yank_unregister_instance(MIGRATION_YANK_INSTANCE);
         error_setg(errp, "unknown migration protocol: %s", uri);
     }
 }
@@ -2083,9 +2078,14 @@ void qmp_migrate_incoming(const char *uri, Error **errp)
         return;
     }
 
+    if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
+        return;
+    }
+
     qemu_start_incoming_migration(uri, &local_err);
 
     if (local_err) {
+        yank_unregister_instance(MIGRATION_YANK_INSTANCE);
         error_propagate(errp, local_err);
         return;
     }
@@ -2114,7 +2114,6 @@ void qmp_migrate_recover(const char *uri, Error **errp)
      * only re-setup the migration stream and poke existing migration
      * to continue using that newly established channel.
      */
-    yank_unregister_instance(MIGRATION_YANK_INSTANCE);
     qemu_start_incoming_migration(uri, errp);
 }
 
-- 
2.31.1

Re: [PATCH 1/2] migration: Move yank outside qemu_start_incoming_migration()

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> Starting from commit b5eea99ec2f5c, qmp_migrate_recover() calls unregister
> before calling qemu_start_incoming_migration(). I believe it wanted to mitigate
> the next call to yank_register_instance(), but I think that's wrong.
> 
> Firstly, if during recover, we should keep the yank instance there, not
> "quickly removing and adding it back".
> 
> Meanwhile, calling qmp_migrate_recover() twice with b5eea99ec2f5c will directly
> crash the dest qemu (right now it can't; but it'll start to work right after
> the next patch) because the 1st call of qmp_migrate_recover() will unregister
> permanently when the channel failed to establish, then the 2nd call of
> qmp_migrate_recover() crashes at yank_unregister_instance().
> 
> This patch fixes it by moving yank ops out of qemu_start_incoming_migration()
> into qmp_migrate_incoming.  For qmp_migrate_recover(), drop the unregister of
> yank instance too since we keep it there during the recovery phase.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

> ---
>  migration/migration.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 4228635d18..1bb03d1eca 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -456,10 +456,6 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>      const char *p = NULL;
>  
> -    if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
> -        return;
> -    }
> -
>      qapi_event_send_migration(MIGRATION_STATUS_SETUP);
>      if (strstart(uri, "tcp:", &p) ||
>          strstart(uri, "unix:", NULL) ||
> @@ -474,7 +470,6 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
>      } else if (strstart(uri, "fd:", &p)) {
>          fd_start_incoming_migration(p, errp);
>      } else {
> -        yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>          error_setg(errp, "unknown migration protocol: %s", uri);
>      }
>  }
> @@ -2083,9 +2078,14 @@ void qmp_migrate_incoming(const char *uri, Error **errp)
>          return;
>      }
>  
> +    if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
> +        return;
> +    }
> +
>      qemu_start_incoming_migration(uri, &local_err);
>  
>      if (local_err) {
> +        yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>          error_propagate(errp, local_err);
>          return;
>      }
> @@ -2114,7 +2114,6 @@ void qmp_migrate_recover(const char *uri, Error **errp)
>       * only re-setup the migration stream and poke existing migration
>       * to continue using that newly established channel.
>       */
> -    yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>      qemu_start_incoming_migration(uri, errp);
>  }
>  
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH 2/2] migration: Allow reset of postcopy_recover_triggered when failed

[Peter Xu]

It's possible qemu_start_incoming_migration() failed at any point, when it
happens we should reset postcopy_recover_triggered to false so that the user
can still retry with a saner incoming port.

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

diff --git a/migration/migration.c b/migration/migration.c
index 1bb03d1eca..fcca289ef7 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2097,6 +2097,13 @@ void qmp_migrate_recover(const char *uri, Error **errp)
 {
     MigrationIncomingState *mis = migration_incoming_get_current();
 
+    /*
+     * Don't even bother to use ERRP_GUARD() as it _must_ always be set by
+     * callers (no one should ignore a recover failure); if there is, it's a
+     * programming error.
+     */
+    assert(errp);
+
     if (mis->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
         error_setg(errp, "Migrate recover can only be run "
                    "when postcopy is paused.");
@@ -2115,6 +2122,12 @@ void qmp_migrate_recover(const char *uri, Error **errp)
      * to continue using that newly established channel.
      */
     qemu_start_incoming_migration(uri, errp);
+
+    /* Safe to dereference with the assert above */
+    if (*errp) {
+        /* Reset the flag so user could still retry */
+        qatomic_set(&mis->postcopy_recover_triggered, false);
+    }
 }
 
 void qmp_migrate_pause(Error **errp)
-- 
2.31.1

Re: [PATCH 2/2] migration: Allow reset of postcopy_recover_triggered when failed

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> It's possible qemu_start_incoming_migration() failed at any point, when it
> happens we should reset postcopy_recover_triggered to false so that the user
> can still retry with a saner incoming port.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

> ---
>  migration/migration.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 1bb03d1eca..fcca289ef7 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2097,6 +2097,13 @@ void qmp_migrate_recover(const char *uri, Error **errp)
>  {
>      MigrationIncomingState *mis = migration_incoming_get_current();
>  
> +    /*
> +     * Don't even bother to use ERRP_GUARD() as it _must_ always be set by
> +     * callers (no one should ignore a recover failure); if there is, it's a
> +     * programming error.
> +     */
> +    assert(errp);
> +
>      if (mis->state != MIGRATION_STATUS_POSTCOPY_PAUSED) {
>          error_setg(errp, "Migrate recover can only be run "
>                     "when postcopy is paused.");
> @@ -2115,6 +2122,12 @@ void qmp_migrate_recover(const char *uri, Error **errp)
>       * to continue using that newly established channel.
>       */
>      qemu_start_incoming_migration(uri, errp);
> +
> +    /* Safe to dereference with the assert above */
> +    if (*errp) {
> +        /* Reset the flag so user could still retry */
> +        qatomic_set(&mis->postcopy_recover_triggered, false);
> +    }
>  }
>  
>  void qmp_migrate_pause(Error **errp)
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 0/2] migration: Two fixes around yank and postcopy recovery

[Peter Xu]

On Tue, Jun 29, 2021 at 02:13:54PM -0400, Peter Xu wrote:
> Note that the multifd zstd test may fail if run migration-test with sudo on
> master (which seems to be a known issue now), and it'll still fail after these
> two patches applied, however all running tests keep usual.

There's an unexpected accident; please ignore this paragraph as zstd test
actually passes with/without the patchset applied.

-- 
Peter Xu

Re: [PATCH 0/2] migration: Two fixes around yank and postcopy recovery

[Leonardo Bras Soares Passos]

On Tue, Jun 29, 2021 at 3:14 PM Peter Xu <peterx@redhat.com> wrote:
>
> The 1st patch should fix yank with unregister instance; I think it should also
> fix the issue that Leonardo used to fix in this patch:
>
> https://lore.kernel.org/qemu-devel/20210629050522.147057-1-leobras@redhat.com/
>
> The 2nd patch fixes postcopy recovery cannot retry if e.g. the 1st attempt
> provided a wrong port address.
>
> Note that the multifd zstd test may fail if run migration-test with sudo on
> master (which seems to be a known issue now), and it'll still fail after these
> two patches applied, however all running tests keep usual.
>
> (Leo: please let me know if this series didn't fix the issue you used to fix)

It does fix the issue, as far as I tested.

>
> Please review, thanks.
>
> Peter Xu (2):
>   migration: Move yank outside qemu_start_incoming_migration()
>   migration: Allow reset of postcopy_recover_triggered when failed
>
>  migration/migration.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
>
> --
> 2.31.1
>
>

Re: [PATCH 0/2] migration: Two fixes around yank and postcopy recovery

[Peter Xu]

On Tue, Jun 29, 2021 at 07:38:32PM -0300, Leonardo Bras Soares Passos wrote:
> > (Leo: please let me know if this series didn't fix the issue you used to fix)
> 
> It does fix the issue, as far as I tested.

Thanks, Leo!

-- 
Peter Xu