[PATCH 0/3] Fix segfault on migration return path

[PATCH 0/3] Fix segfault on migration return path

[Fabiano Rosas]

The /x86_64/migration/postcopy/preempt/recovery/plain test is
sometimes failing due a segmentation fault on the migration return
path. There is a race involving the retry logic of the return path and
the migration resume command.

The issue happens when the retry logic tries to cleanup the current
return path file, but ends up cleaning the new one and trying to use
it right after. Tracing shows it clearly:

open_return_path_on_source  <-- at migration start
open_return_path_on_source_continue <-- rp thread created
postcopy_pause_incoming
postcopy_pause_fast_load
qemu-system-x86_64: Detected IO failure for postcopy. Migration paused. (incoming)
postcopy_pause_fault_thread
qemu-system-x86_64: Detected IO failure for postcopy. Migration paused. (source)
postcopy_pause_incoming_continued
open_return_path_on_source   <-- NOK, too soon
postcopy_pause_continued
postcopy_pause_return_path   <-- too late, already operating on the new from_dst_file
postcopy_pause_return_path_continued <-- will continue and crash
postcopy_pause_incoming
qemu-system-x86_64: Detected IO failure for postcopy. Migration paused.
postcopy_pause_incoming_continued

We could solve this by adding some form of synchronization to ensure
that we always do the cleanup before setting up the new file, but I
find it more straight-forward to move the retry logic outside of the
thread by letting it finish and starting a new thread when resuming
the migration.

More details on the commit message.

CI run: https://gitlab.com/farosas/qemu/-/pipelines/947875609

Fabiano Rosas (3):
  migration: Stop marking RP bad after shutdown
  migration: Simplify calling of await_return_path_close_on_source
  migration: Replace the return path retry logic

 migration/migration.c  | 94 ++++++++++++++----------------------------
 migration/migration.h  |  1 -
 migration/trace-events |  3 +-
 3 files changed, 33 insertions(+), 65 deletions(-)

-- 
2.35.3

[PATCH 1/3] migration: Stop marking RP bad after shutdown

[Fabiano Rosas]

When waiting for the return path (RP) thread to finish, there is
really nothing wrong in the RP if the destination end of the migration
stops responding, leaving it stuck.

Stop returning an error at that point and leave it to other parts of
the code to catch. One such part is the very next routine run by
migration_completion() which checks 'to_dst_file' for an error and fails
the migration. Another is the RP thread itself when the recvmsg()
returns an error.

With this we stop marking RP bad from outside of the thread and can
reuse await_return_path_close_on_source() in the next patches to wait
on the thread during a paused migration.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 91bba630a8..051067f8c5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2049,7 +2049,6 @@ static int await_return_path_close_on_source(MigrationState *ms)
          * waiting for the destination.
          */
         qemu_file_shutdown(ms->rp_state.from_dst_file);
-        mark_source_rp_bad(ms);
     }
     trace_await_return_path_close_on_source_joining();
     qemu_thread_join(&ms->rp_state.rp_thread);
-- 
2.35.3

Re: [PATCH 1/3] migration: Stop marking RP bad after shutdown

[Peter Xu]

On Fri, Jul 28, 2023 at 09:15:14AM -0300, Fabiano Rosas wrote:
> When waiting for the return path (RP) thread to finish, there is
> really nothing wrong in the RP if the destination end of the migration
> stops responding, leaving it stuck.
> 
> Stop returning an error at that point and leave it to other parts of
> the code to catch. One such part is the very next routine run by
> migration_completion() which checks 'to_dst_file' for an error and fails
> the migration. Another is the RP thread itself when the recvmsg()
> returns an error.
> 
> With this we stop marking RP bad from outside of the thread and can
> reuse await_return_path_close_on_source() in the next patches to wait
> on the thread during a paused migration.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/migration.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 91bba630a8..051067f8c5 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2049,7 +2049,6 @@ static int await_return_path_close_on_source(MigrationState *ms)
>           * waiting for the destination.
>           */
>          qemu_file_shutdown(ms->rp_state.from_dst_file);
> -        mark_source_rp_bad(ms);
>      }
>      trace_await_return_path_close_on_source_joining();
>      qemu_thread_join(&ms->rp_state.rp_thread);

The retval of await_return_path_close_on_source() relies on
ms->rp_state.error.  If mark_source_rp_bad() is dropped, is it possible
that it'll start to return succeed where it used to return failure?

Maybe not a big deal: I see migration_completion() also has another
qemu_file_get_error() later to catch errors, but I don't know how solid
that is.

I think as long as after this patch we can fail properly on e.g. network
failures for precopy when cap return-path=on, then we should be good.

-- 
Peter Xu

Re: [PATCH 1/3] migration: Stop marking RP bad after shutdown

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Jul 28, 2023 at 09:15:14AM -0300, Fabiano Rosas wrote:
>> When waiting for the return path (RP) thread to finish, there is
>> really nothing wrong in the RP if the destination end of the migration
>> stops responding, leaving it stuck.
>> 
>> Stop returning an error at that point and leave it to other parts of
>> the code to catch. One such part is the very next routine run by
>> migration_completion() which checks 'to_dst_file' for an error and fails
>> the migration. Another is the RP thread itself when the recvmsg()
>> returns an error.
>> 
>> With this we stop marking RP bad from outside of the thread and can
>> reuse await_return_path_close_on_source() in the next patches to wait
>> on the thread during a paused migration.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/migration.c | 1 -
>>  1 file changed, 1 deletion(-)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 91bba630a8..051067f8c5 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -2049,7 +2049,6 @@ static int await_return_path_close_on_source(MigrationState *ms)
>>           * waiting for the destination.
>>           */
>>          qemu_file_shutdown(ms->rp_state.from_dst_file);
>> -        mark_source_rp_bad(ms);
>>      }
>>      trace_await_return_path_close_on_source_joining();
>>      qemu_thread_join(&ms->rp_state.rp_thread);
>
> The retval of await_return_path_close_on_source() relies on
> ms->rp_state.error.  If mark_source_rp_bad() is dropped, is it possible
> that it'll start to return succeed where it used to return failure?

Yep, as described in the commit message, I think it's ok to do that. The
critical part is doing the shutdown. Other instances of
mark_source_rp_bad() continue existing and we continue returning
rp_state.error.

>
> Maybe not a big deal: I see migration_completion() also has another
> qemu_file_get_error() later to catch errors, but I don't know how solid
> that is.

That is the instance I refer to in the commit message. At
await_return_path_close_on_source() we only call mark_source_rp_bad() if
to_dst_file has an error. That will be caught by this
qemu_file_get_error() anyway.

Re: [PATCH 1/3] migration: Stop marking RP bad after shutdown

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Peter Xu <peterx@redhat.com> writes:
>
>> On Fri, Jul 28, 2023 at 09:15:14AM -0300, Fabiano Rosas wrote:
>>> When waiting for the return path (RP) thread to finish, there is
>>> really nothing wrong in the RP if the destination end of the migration
>>> stops responding, leaving it stuck.
>>> 
>>> Stop returning an error at that point and leave it to other parts of
>>> the code to catch. One such part is the very next routine run by
>>> migration_completion() which checks 'to_dst_file' for an error and fails
>>> the migration. Another is the RP thread itself when the recvmsg()
>>> returns an error.
>>> 
>>> With this we stop marking RP bad from outside of the thread and can
>>> reuse await_return_path_close_on_source() in the next patches to wait
>>> on the thread during a paused migration.
>>> 
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>>  migration/migration.c | 1 -
>>>  1 file changed, 1 deletion(-)
>>> 
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index 91bba630a8..051067f8c5 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -2049,7 +2049,6 @@ static int await_return_path_close_on_source(MigrationState *ms)
>>>           * waiting for the destination.
>>>           */
>>>          qemu_file_shutdown(ms->rp_state.from_dst_file);
>>> -        mark_source_rp_bad(ms);
>>>      }
>>>      trace_await_return_path_close_on_source_joining();
>>>      qemu_thread_join(&ms->rp_state.rp_thread);
>>
>> The retval of await_return_path_close_on_source() relies on
>> ms->rp_state.error.  If mark_source_rp_bad() is dropped, is it possible
>> that it'll start to return succeed where it used to return failure?
>
> Yep, as described in the commit message, I think it's ok to do that. The
> critical part is doing the shutdown. Other instances of
> mark_source_rp_bad() continue existing and we continue returning
> rp_state.error.
>
>>
>> Maybe not a big deal: I see migration_completion() also has another
>> qemu_file_get_error() later to catch errors, but I don't know how solid
>> that is.
>
> That is the instance I refer to in the commit message. At
> await_return_path_close_on_source() we only call mark_source_rp_bad() if
> to_dst_file has an error. That will be caught by this
> qemu_file_get_error() anyway.

Actually, I can do better, I can merge this shutdown() into
migration_completion(). Then this dependency becomes explicit. Since you
suggested moving await_return_path_close_on_source() into
postcopy_pause(), it doesn't make sense to check to_dst_file anymore,
because when pausing we clear that file.

[PATCH 2/3] migration: Simplify calling of await_return_path_close_on_source

[Fabiano Rosas]

We're about to reuse this function so move the 'rp_thread_created'
check into it and remove the redundant tracing and comment.

Add a new tracepoint akin to what is already done at
migration_completion().

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c  | 21 +++++++--------------
 migration/trace-events |  3 +--
 2 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 051067f8c5..d6f4470265 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2038,6 +2038,10 @@ static int open_return_path_on_source(MigrationState *ms,
 /* Returns 0 if the RP was ok, otherwise there was an error on the RP */
 static int await_return_path_close_on_source(MigrationState *ms)
 {
+    if (!ms->rp_state.rp_thread_created) {
+        return 0;
+    }
+
     /*
      * If this is a normal exit then the destination will send a SHUT and the
      * rp_thread will exit, however if there's an error we need to cause
@@ -2350,20 +2354,9 @@ static void migration_completion(MigrationState *s)
         goto fail;
     }
 
-    /*
-     * If rp was opened we must clean up the thread before
-     * cleaning everything else up (since if there are no failures
-     * it will wait for the destination to send it's status in
-     * a SHUT command).
-     */
-    if (s->rp_state.rp_thread_created) {
-        int rp_error;
-        trace_migration_return_path_end_before();
-        rp_error = await_return_path_close_on_source(s);
-        trace_migration_return_path_end_after(rp_error);
-        if (rp_error) {
-            goto fail;
-        }
+    if (await_return_path_close_on_source(s)) {
+        trace_migration_completion_rp_err();
+        goto fail;
     }
 
     if (qemu_file_get_error(s->to_dst_file)) {
diff --git a/migration/trace-events b/migration/trace-events
index 5259c1044b..33a69064ca 100644
--- a/migration/trace-events
+++ b/migration/trace-events
@@ -157,13 +157,12 @@ migrate_pending_estimate(uint64_t size, uint64_t pre, uint64_t post) "estimate p
 migrate_send_rp_message(int msg_type, uint16_t len) "%d: len %d"
 migrate_send_rp_recv_bitmap(char *name, int64_t size) "block '%s' size 0x%"PRIi64
 migration_completion_file_err(void) ""
+migration_completion_rp_err(void) ""
 migration_completion_vm_stop(int ret) "ret %d"
 migration_completion_postcopy_end(void) ""
 migration_completion_postcopy_end_after_complete(void) ""
 migration_rate_limit_pre(int ms) "%d ms"
 migration_rate_limit_post(int urgent) "urgent: %d"
-migration_return_path_end_before(void) ""
-migration_return_path_end_after(int rp_error) "%d"
 migration_thread_after_loop(void) ""
 migration_thread_file_err(void) ""
 migration_thread_setup_complete(void) ""
-- 
2.35.3

Re: [PATCH 2/3] migration: Simplify calling of await_return_path_close_on_source

[Peter Xu]

On Fri, Jul 28, 2023 at 09:15:15AM -0300, Fabiano Rosas wrote:
> We're about to reuse this function so move the 'rp_thread_created'
> check into it and remove the redundant tracing and comment.
> 
> Add a new tracepoint akin to what is already done at
> migration_completion().
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/migration.c  | 21 +++++++--------------
>  migration/trace-events |  3 +--
>  2 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 051067f8c5..d6f4470265 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2038,6 +2038,10 @@ static int open_return_path_on_source(MigrationState *ms,
>  /* Returns 0 if the RP was ok, otherwise there was an error on the RP */
>  static int await_return_path_close_on_source(MigrationState *ms)
>  {
> +    if (!ms->rp_state.rp_thread_created) {
> +        return 0;
> +    }
> +
>      /*
>       * If this is a normal exit then the destination will send a SHUT and the
>       * rp_thread will exit, however if there's an error we need to cause
> @@ -2350,20 +2354,9 @@ static void migration_completion(MigrationState *s)
>          goto fail;
>      }
>  
> -    /*
> -     * If rp was opened we must clean up the thread before
> -     * cleaning everything else up (since if there are no failures
> -     * it will wait for the destination to send it's status in
> -     * a SHUT command).
> -     */
> -    if (s->rp_state.rp_thread_created) {
> -        int rp_error;
> -        trace_migration_return_path_end_before();
> -        rp_error = await_return_path_close_on_source(s);
> -        trace_migration_return_path_end_after(rp_error);
> -        if (rp_error) {
> -            goto fail;
> -        }
> +    if (await_return_path_close_on_source(s)) {
> +        trace_migration_completion_rp_err();
> +        goto fail;
>      }
>  
>      if (qemu_file_get_error(s->to_dst_file)) {
> diff --git a/migration/trace-events b/migration/trace-events
> index 5259c1044b..33a69064ca 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -157,13 +157,12 @@ migrate_pending_estimate(uint64_t size, uint64_t pre, uint64_t post) "estimate p
>  migrate_send_rp_message(int msg_type, uint16_t len) "%d: len %d"
>  migrate_send_rp_recv_bitmap(char *name, int64_t size) "block '%s' size 0x%"PRIi64
>  migration_completion_file_err(void) ""
> +migration_completion_rp_err(void) ""
>  migration_completion_vm_stop(int ret) "ret %d"
>  migration_completion_postcopy_end(void) ""
>  migration_completion_postcopy_end_after_complete(void) ""
>  migration_rate_limit_pre(int ms) "%d ms"
>  migration_rate_limit_post(int urgent) "urgent: %d"
> -migration_return_path_end_before(void) ""
> -migration_return_path_end_after(int rp_error) "%d"
>  migration_thread_after_loop(void) ""
>  migration_thread_file_err(void) ""
>  migration_thread_setup_complete(void) ""
> -- 
> 2.35.3
> 

Just in case someone may still want to see the old
trace_migration_return_path_end_before() tracepoint, maybe just move them
all over?

-- 
Peter Xu

Re: [PATCH 2/3] migration: Simplify calling of await_return_path_close_on_source

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Jul 28, 2023 at 09:15:15AM -0300, Fabiano Rosas wrote:
>> We're about to reuse this function so move the 'rp_thread_created'
>> check into it and remove the redundant tracing and comment.
>> 
>> Add a new tracepoint akin to what is already done at
>> migration_completion().
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/migration.c  | 21 +++++++--------------
>>  migration/trace-events |  3 +--
>>  2 files changed, 8 insertions(+), 16 deletions(-)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 051067f8c5..d6f4470265 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -2038,6 +2038,10 @@ static int open_return_path_on_source(MigrationState *ms,
>>  /* Returns 0 if the RP was ok, otherwise there was an error on the RP */
>>  static int await_return_path_close_on_source(MigrationState *ms)
>>  {
>> +    if (!ms->rp_state.rp_thread_created) {
>> +        return 0;
>> +    }
>> +
>>      /*
>>       * If this is a normal exit then the destination will send a SHUT and the
>>       * rp_thread will exit, however if there's an error we need to cause
>> @@ -2350,20 +2354,9 @@ static void migration_completion(MigrationState *s)
>>          goto fail;
>>      }
>>  
>> -    /*
>> -     * If rp was opened we must clean up the thread before
>> -     * cleaning everything else up (since if there are no failures
>> -     * it will wait for the destination to send it's status in
>> -     * a SHUT command).
>> -     */
>> -    if (s->rp_state.rp_thread_created) {
>> -        int rp_error;
>> -        trace_migration_return_path_end_before();
>> -        rp_error = await_return_path_close_on_source(s);
>> -        trace_migration_return_path_end_after(rp_error);
>> -        if (rp_error) {
>> -            goto fail;
>> -        }
>> +    if (await_return_path_close_on_source(s)) {
>> +        trace_migration_completion_rp_err();
>> +        goto fail;
>>      }
>>  
>>      if (qemu_file_get_error(s->to_dst_file)) {
>> diff --git a/migration/trace-events b/migration/trace-events
>> index 5259c1044b..33a69064ca 100644
>> --- a/migration/trace-events
>> +++ b/migration/trace-events
>> @@ -157,13 +157,12 @@ migrate_pending_estimate(uint64_t size, uint64_t pre, uint64_t post) "estimate p
>>  migrate_send_rp_message(int msg_type, uint16_t len) "%d: len %d"
>>  migrate_send_rp_recv_bitmap(char *name, int64_t size) "block '%s' size 0x%"PRIi64
>>  migration_completion_file_err(void) ""
>> +migration_completion_rp_err(void) ""
>>  migration_completion_vm_stop(int ret) "ret %d"
>>  migration_completion_postcopy_end(void) ""
>>  migration_completion_postcopy_end_after_complete(void) ""
>>  migration_rate_limit_pre(int ms) "%d ms"
>>  migration_rate_limit_post(int urgent) "urgent: %d"
>> -migration_return_path_end_before(void) ""
>> -migration_return_path_end_after(int rp_error) "%d"
>>  migration_thread_after_loop(void) ""
>>  migration_thread_file_err(void) ""
>>  migration_thread_setup_complete(void) ""
>> -- 
>> 2.35.3
>> 
>
> Just in case someone may still want to see the old
> trace_migration_return_path_end_before() tracepoint, maybe just move them
> all over?

They are made redundant by
trace_await_return_path_close_on_source_joining() and
trace_await_return_path_close_on_source_close() which already exist in
the function.

Re: [PATCH 2/3] migration: Simplify calling of await_return_path_close_on_source

[Peter Xu]

On Mon, Jul 31, 2023 at 09:42:27AM -0300, Fabiano Rosas wrote:
> They are made redundant by
> trace_await_return_path_close_on_source_joining() and
> trace_await_return_path_close_on_source_close() which already exist in
> the function.

Oh.. that's okay then.  Thanks,

-- 
Peter Xu

[PATCH 3/3] migration: Replace the return path retry logic

[Fabiano Rosas]

Replace the return path retry logic with finishing and restarting the
thread. This fixes a race when resuming the migration that leads to a
segfault.

Currently when doing postcopy we consider that an IO error on the
return path file could be due to a network intermittency. We then keep
the thread alive but have it do cleanup of the 'from_dst_file' and
wait on the 'postcopy_pause_rp' semaphore. When the user issues a
migrate resume, a new return path is opened and the thread is allowed
to continue.

There's a race condition in the above mechanism. It is possible for
the new return path file to be setup *before* the cleanup code in the
return path thread has had a chance to run, leading to the *new* file
being closed and the pointer set to NULL. When the thread is released
after the resume, it tries to dereference 'from_dst_file' and crashes:

Thread 7 "return path" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffd1dbf700 (LWP 9611)]
0x00005555560e4893 in qemu_file_get_error_obj (f=0x0, errp=0x0) at ../migration/qemu-file.c:154
154         return f->last_error;

(gdb) bt
 #0  0x00005555560e4893 in qemu_file_get_error_obj (f=0x0, errp=0x0) at ../migration/qemu-file.c:154
 #1  0x00005555560e4983 in qemu_file_get_error (f=0x0) at ../migration/qemu-file.c:206
 #2  0x0000555555b9a1df in source_return_path_thread (opaque=0x555556e06000) at ../migration/migration.c:1876
 #3  0x000055555602e14f in qemu_thread_start (args=0x55555782e780) at ../util/qemu-thread-posix.c:541
 #4  0x00007ffff38d76ea in start_thread (arg=0x7fffd1dbf700) at pthread_create.c:477
 #5  0x00007ffff35efa6f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Here's the race (important bit is open_return_path happening before
migration_release_dst_files):

migration                 | qmp                         | return path
--------------------------+-----------------------------+---------------------------------
			    qmp_migrate_pause()
			     shutdown(ms->to_dst_file)
			      f->last_error = -EIO
migrate_detect_error()
 postcopy_pause()
  set_state(PAUSED)
  wait(postcopy_pause_sem)
			    qmp_migrate(resume)
			    migrate_fd_connect()
			     resume = state == PAUSED
			     open_return_path <-- TOO SOON!
			     set_state(RECOVER)
			     post(postcopy_pause_sem)
							(incoming closes to_src_file)
							res = qemu_file_get_error(rp)
							migration_release_dst_files()
							ms->rp_state.from_dst_file = NULL
  post(postcopy_pause_rp_sem)
							postcopy_pause_return_path_thread()
							  wait(postcopy_pause_rp_sem)
							rp = ms->rp_state.from_dst_file
							goto retry
							qemu_file_get_error(rp)
							SIGSEGV
-------------------------------------------------------------------------------------------

We can keep the retry logic without having the thread alive and
waiting. The only piece of data used by it is the 'from_dst_file' and
it is only allowed to proceed after a migrate resume is issued and the
semaphore released at migrate_fd_connect().

Move the retry logic to outside the thread by having
open_return_path_on_source() wait for the thread to finish before
creating a new one with the updated 'from_dst_file'.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 72 +++++++++++++++----------------------------
 migration/migration.h |  1 -
 2 files changed, 25 insertions(+), 48 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index d6f4470265..36cdd7bda8 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -97,6 +97,7 @@ static int migration_maybe_pause(MigrationState *s,
                                  int *current_active_state,
                                  int new_state);
 static void migrate_fd_cancel(MigrationState *s);
+static int await_return_path_close_on_source(MigrationState *ms);
 
 static bool migration_needs_multiple_sockets(void)
 {
@@ -1764,18 +1765,6 @@ static void migrate_handle_rp_req_pages(MigrationState *ms, const char* rbname,
     }
 }
 
-/* Return true to retry, false to quit */
-static bool postcopy_pause_return_path_thread(MigrationState *s)
-{
-    trace_postcopy_pause_return_path();
-
-    qemu_sem_wait(&s->postcopy_pause_rp_sem);
-
-    trace_postcopy_pause_return_path_continued();
-
-    return true;
-}
-
 static int migrate_handle_rp_recv_bitmap(MigrationState *s, char *block_name)
 {
     RAMBlock *block = qemu_ram_block_by_name(block_name);
@@ -1859,7 +1848,6 @@ static void *source_return_path_thread(void *opaque)
     trace_source_return_path_thread_entry();
     rcu_register_thread();
 
-retry:
     while (!ms->rp_state.error && !qemu_file_get_error(rp) &&
            migration_is_setup_or_active(ms->state)) {
         trace_source_return_path_thread_loop_top();
@@ -1981,28 +1969,18 @@ retry:
     }
 
 out:
-    res = qemu_file_get_error(rp);
-    if (res) {
-        if (res && migration_in_postcopy()) {
+    if (qemu_file_get_error(rp)) {
+        if (migration_in_postcopy()) {
             /*
-             * Maybe there is something we can do: it looks like a
-             * network down issue, and we pause for a recovery.
+             * This could be a network issue that would have been
+             * detected by the main migration thread and caused the
+             * migration to pause. Do cleanup and finish.
              */
-            migration_release_dst_files(ms);
-            rp = NULL;
-            if (postcopy_pause_return_path_thread(ms)) {
-                /*
-                 * Reload rp, reset the rest.  Referencing it is safe since
-                 * it's reset only by us above, or when migration completes
-                 */
-                rp = ms->rp_state.from_dst_file;
-                ms->rp_state.error = false;
-                goto retry;
-            }
+            ms->rp_state.error = false;
+        } else {
+            trace_source_return_path_thread_bad_end();
+            mark_source_rp_bad(ms);
         }
-
-        trace_source_return_path_thread_bad_end();
-        mark_source_rp_bad(ms);
     }
 
     trace_source_return_path_thread_end();
@@ -2012,8 +1990,21 @@ out:
 }
 
 static int open_return_path_on_source(MigrationState *ms,
-                                      bool create_thread)
+                                      bool resume)
 {
+    if (resume) {
+        assert(ms->state == MIGRATION_STATUS_POSTCOPY_PAUSED);
+
+        /*
+         * We're resuming from a paused postcopy migration. Wait for
+         * the thread to do its cleanup before re-opening the return
+         * path.
+         */
+        if (await_return_path_close_on_source(ms)) {
+            return -1;
+        }
+    }
+
     ms->rp_state.from_dst_file = qemu_file_get_return_path(ms->to_dst_file);
     if (!ms->rp_state.from_dst_file) {
         return -1;
@@ -2021,11 +2012,6 @@ static int open_return_path_on_source(MigrationState *ms,
 
     trace_open_return_path_on_source();
 
-    if (!create_thread) {
-        /* We're done */
-        return 0;
-    }
-
     qemu_thread_create(&ms->rp_state.rp_thread, "return path",
                        source_return_path_thread, ms, QEMU_THREAD_JOINABLE);
     ms->rp_state.rp_thread_created = true;
@@ -2550,12 +2536,6 @@ static MigThrError postcopy_pause(MigrationState *s)
         if (s->state == MIGRATION_STATUS_POSTCOPY_RECOVER) {
             /* Woken up by a recover procedure. Give it a shot */
 
-            /*
-             * Firstly, let's wake up the return path now, with a new
-             * return path channel.
-             */
-            qemu_sem_post(&s->postcopy_pause_rp_sem);
-
             /* Do the resume logic */
             if (postcopy_do_resume(s) == 0) {
                 /* Let's continue! */
@@ -3243,7 +3223,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
      * QEMU uses the return path.
      */
     if (migrate_postcopy_ram() || migrate_return_path()) {
-        if (open_return_path_on_source(s, !resume)) {
+        if (open_return_path_on_source(s, resume)) {
             error_report("Unable to open return-path for postcopy");
             migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
             migrate_fd_cleanup(s);
@@ -3304,7 +3284,6 @@ static void migration_instance_finalize(Object *obj)
     qemu_sem_destroy(&ms->rate_limit_sem);
     qemu_sem_destroy(&ms->pause_sem);
     qemu_sem_destroy(&ms->postcopy_pause_sem);
-    qemu_sem_destroy(&ms->postcopy_pause_rp_sem);
     qemu_sem_destroy(&ms->rp_state.rp_sem);
     qemu_sem_destroy(&ms->rp_state.rp_pong_acks);
     qemu_sem_destroy(&ms->postcopy_qemufile_src_sem);
@@ -3324,7 +3303,6 @@ static void migration_instance_init(Object *obj)
     migrate_params_init(&ms->parameters);
 
     qemu_sem_init(&ms->postcopy_pause_sem, 0);
-    qemu_sem_init(&ms->postcopy_pause_rp_sem, 0);
     qemu_sem_init(&ms->rp_state.rp_sem, 0);
     qemu_sem_init(&ms->rp_state.rp_pong_acks, 0);
     qemu_sem_init(&ms->rate_limit_sem, 0);
diff --git a/migration/migration.h b/migration/migration.h
index b7c8b67542..e78db5361c 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -382,7 +382,6 @@ struct MigrationState {
 
     /* Needed by postcopy-pause state */
     QemuSemaphore postcopy_pause_sem;
-    QemuSemaphore postcopy_pause_rp_sem;
     /*
      * Whether we abort the migration if decompression errors are
      * detected at the destination. It is left at false for qemu
-- 
2.35.3

Re: [PATCH 3/3] migration: Replace the return path retry logic

[Peter Xu]

On Fri, Jul 28, 2023 at 09:15:16AM -0300, Fabiano Rosas wrote:
> Replace the return path retry logic with finishing and restarting the
> thread. This fixes a race when resuming the migration that leads to a
> segfault.
> 
> Currently when doing postcopy we consider that an IO error on the
> return path file could be due to a network intermittency. We then keep
> the thread alive but have it do cleanup of the 'from_dst_file' and
> wait on the 'postcopy_pause_rp' semaphore. When the user issues a
> migrate resume, a new return path is opened and the thread is allowed
> to continue.
> 
> There's a race condition in the above mechanism. It is possible for
> the new return path file to be setup *before* the cleanup code in the
> return path thread has had a chance to run, leading to the *new* file
> being closed and the pointer set to NULL. When the thread is released
> after the resume, it tries to dereference 'from_dst_file' and crashes:
> 
> Thread 7 "return path" received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7fffd1dbf700 (LWP 9611)]
> 0x00005555560e4893 in qemu_file_get_error_obj (f=0x0, errp=0x0) at ../migration/qemu-file.c:154
> 154         return f->last_error;
> 
> (gdb) bt
>  #0  0x00005555560e4893 in qemu_file_get_error_obj (f=0x0, errp=0x0) at ../migration/qemu-file.c:154
>  #1  0x00005555560e4983 in qemu_file_get_error (f=0x0) at ../migration/qemu-file.c:206
>  #2  0x0000555555b9a1df in source_return_path_thread (opaque=0x555556e06000) at ../migration/migration.c:1876
>  #3  0x000055555602e14f in qemu_thread_start (args=0x55555782e780) at ../util/qemu-thread-posix.c:541
>  #4  0x00007ffff38d76ea in start_thread (arg=0x7fffd1dbf700) at pthread_create.c:477
>  #5  0x00007ffff35efa6f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
> 
> Here's the race (important bit is open_return_path happening before
> migration_release_dst_files):
> 
> migration                 | qmp                         | return path
> --------------------------+-----------------------------+---------------------------------
> 			    qmp_migrate_pause()
> 			     shutdown(ms->to_dst_file)
> 			      f->last_error = -EIO
> migrate_detect_error()
>  postcopy_pause()
>   set_state(PAUSED)
>   wait(postcopy_pause_sem)
> 			    qmp_migrate(resume)
> 			    migrate_fd_connect()
> 			     resume = state == PAUSED
> 			     open_return_path <-- TOO SOON!
> 			     set_state(RECOVER)
> 			     post(postcopy_pause_sem)
> 							(incoming closes to_src_file)
> 							res = qemu_file_get_error(rp)
> 							migration_release_dst_files()
> 							ms->rp_state.from_dst_file = NULL
>   post(postcopy_pause_rp_sem)
> 							postcopy_pause_return_path_thread()
> 							  wait(postcopy_pause_rp_sem)
> 							rp = ms->rp_state.from_dst_file
> 							goto retry
> 							qemu_file_get_error(rp)
> 							SIGSEGV
> -------------------------------------------------------------------------------------------
> 
> We can keep the retry logic without having the thread alive and
> waiting. The only piece of data used by it is the 'from_dst_file' and
> it is only allowed to proceed after a migrate resume is issued and the
> semaphore released at migrate_fd_connect().
> 
> Move the retry logic to outside the thread by having
> open_return_path_on_source() wait for the thread to finish before
> creating a new one with the updated 'from_dst_file'.

If we can remove that (along with the sync sem) it'll be pretty nice.  If
you want, and if this works well, not sure whether you're interested in
doing similarly to the other threads.  Currently we halt all the threads,
I'm not sure whether we can do similiar things on dest and whether that can
also benefit on sync efforts.

Still one comment below.

> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/migration.c | 72 +++++++++++++++----------------------------
>  migration/migration.h |  1 -
>  2 files changed, 25 insertions(+), 48 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index d6f4470265..36cdd7bda8 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -97,6 +97,7 @@ static int migration_maybe_pause(MigrationState *s,
>                                   int *current_active_state,
>                                   int new_state);
>  static void migrate_fd_cancel(MigrationState *s);
> +static int await_return_path_close_on_source(MigrationState *ms);
>  
>  static bool migration_needs_multiple_sockets(void)
>  {
> @@ -1764,18 +1765,6 @@ static void migrate_handle_rp_req_pages(MigrationState *ms, const char* rbname,
>      }
>  }
>  
> -/* Return true to retry, false to quit */
> -static bool postcopy_pause_return_path_thread(MigrationState *s)
> -{
> -    trace_postcopy_pause_return_path();
> -
> -    qemu_sem_wait(&s->postcopy_pause_rp_sem);
> -
> -    trace_postcopy_pause_return_path_continued();
> -
> -    return true;
> -}
> -
>  static int migrate_handle_rp_recv_bitmap(MigrationState *s, char *block_name)
>  {
>      RAMBlock *block = qemu_ram_block_by_name(block_name);
> @@ -1859,7 +1848,6 @@ static void *source_return_path_thread(void *opaque)
>      trace_source_return_path_thread_entry();
>      rcu_register_thread();
>  
> -retry:
>      while (!ms->rp_state.error && !qemu_file_get_error(rp) &&
>             migration_is_setup_or_active(ms->state)) {
>          trace_source_return_path_thread_loop_top();
> @@ -1981,28 +1969,18 @@ retry:
>      }
>  
>  out:
> -    res = qemu_file_get_error(rp);
> -    if (res) {
> -        if (res && migration_in_postcopy()) {
> +    if (qemu_file_get_error(rp)) {
> +        if (migration_in_postcopy()) {
>              /*
> -             * Maybe there is something we can do: it looks like a
> -             * network down issue, and we pause for a recovery.
> +             * This could be a network issue that would have been
> +             * detected by the main migration thread and caused the
> +             * migration to pause. Do cleanup and finish.
>               */
> -            migration_release_dst_files(ms);
> -            rp = NULL;
> -            if (postcopy_pause_return_path_thread(ms)) {
> -                /*
> -                 * Reload rp, reset the rest.  Referencing it is safe since
> -                 * it's reset only by us above, or when migration completes
> -                 */
> -                rp = ms->rp_state.from_dst_file;
> -                ms->rp_state.error = false;
> -                goto retry;
> -            }
> +            ms->rp_state.error = false;
> +        } else {
> +            trace_source_return_path_thread_bad_end();
> +            mark_source_rp_bad(ms);
>          }
> -
> -        trace_source_return_path_thread_bad_end();
> -        mark_source_rp_bad(ms);
>      }
>  
>      trace_source_return_path_thread_end();
> @@ -2012,8 +1990,21 @@ out:
>  }
>  
>  static int open_return_path_on_source(MigrationState *ms,
> -                                      bool create_thread)
> +                                      bool resume)
>  {
> +    if (resume) {
> +        assert(ms->state == MIGRATION_STATUS_POSTCOPY_PAUSED);
> +
> +        /*
> +         * We're resuming from a paused postcopy migration. Wait for
> +         * the thread to do its cleanup before re-opening the return
> +         * path.
> +         */
> +        if (await_return_path_close_on_source(ms)) {
> +            return -1;
> +        }

This smells a bit hacky.  Can we do this in postcopy_pause(), perhaps
before we switch to PAUSED state?  Then we know after being PAUSED we're
ready to recover.

Thanks,

> +    }
> +
>      ms->rp_state.from_dst_file = qemu_file_get_return_path(ms->to_dst_file);
>      if (!ms->rp_state.from_dst_file) {
>          return -1;
> @@ -2021,11 +2012,6 @@ static int open_return_path_on_source(MigrationState *ms,
>  
>      trace_open_return_path_on_source();
>  
> -    if (!create_thread) {
> -        /* We're done */
> -        return 0;
> -    }
> -
>      qemu_thread_create(&ms->rp_state.rp_thread, "return path",
>                         source_return_path_thread, ms, QEMU_THREAD_JOINABLE);
>      ms->rp_state.rp_thread_created = true;
> @@ -2550,12 +2536,6 @@ static MigThrError postcopy_pause(MigrationState *s)
>          if (s->state == MIGRATION_STATUS_POSTCOPY_RECOVER) {
>              /* Woken up by a recover procedure. Give it a shot */
>  
> -            /*
> -             * Firstly, let's wake up the return path now, with a new
> -             * return path channel.
> -             */
> -            qemu_sem_post(&s->postcopy_pause_rp_sem);
> -
>              /* Do the resume logic */
>              if (postcopy_do_resume(s) == 0) {
>                  /* Let's continue! */
> @@ -3243,7 +3223,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>       * QEMU uses the return path.
>       */
>      if (migrate_postcopy_ram() || migrate_return_path()) {
> -        if (open_return_path_on_source(s, !resume)) {
> +        if (open_return_path_on_source(s, resume)) {
>              error_report("Unable to open return-path for postcopy");
>              migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
>              migrate_fd_cleanup(s);
> @@ -3304,7 +3284,6 @@ static void migration_instance_finalize(Object *obj)
>      qemu_sem_destroy(&ms->rate_limit_sem);
>      qemu_sem_destroy(&ms->pause_sem);
>      qemu_sem_destroy(&ms->postcopy_pause_sem);
> -    qemu_sem_destroy(&ms->postcopy_pause_rp_sem);
>      qemu_sem_destroy(&ms->rp_state.rp_sem);
>      qemu_sem_destroy(&ms->rp_state.rp_pong_acks);
>      qemu_sem_destroy(&ms->postcopy_qemufile_src_sem);
> @@ -3324,7 +3303,6 @@ static void migration_instance_init(Object *obj)
>      migrate_params_init(&ms->parameters);
>  
>      qemu_sem_init(&ms->postcopy_pause_sem, 0);
> -    qemu_sem_init(&ms->postcopy_pause_rp_sem, 0);
>      qemu_sem_init(&ms->rp_state.rp_sem, 0);
>      qemu_sem_init(&ms->rp_state.rp_pong_acks, 0);
>      qemu_sem_init(&ms->rate_limit_sem, 0);
> diff --git a/migration/migration.h b/migration/migration.h
> index b7c8b67542..e78db5361c 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -382,7 +382,6 @@ struct MigrationState {
>  
>      /* Needed by postcopy-pause state */
>      QemuSemaphore postcopy_pause_sem;
> -    QemuSemaphore postcopy_pause_rp_sem;
>      /*
>       * Whether we abort the migration if decompression errors are
>       * detected at the destination. It is left at false for qemu
> -- 
> 2.35.3
> 

-- 
Peter Xu

Re: [PATCH 3/3] migration: Replace the return path retry logic

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Jul 28, 2023 at 09:15:16AM -0300, Fabiano Rosas wrote:
>> Replace the return path retry logic with finishing and restarting the
>> thread. This fixes a race when resuming the migration that leads to a
>> segfault.
>> 
>> Currently when doing postcopy we consider that an IO error on the
>> return path file could be due to a network intermittency. We then keep
>> the thread alive but have it do cleanup of the 'from_dst_file' and
>> wait on the 'postcopy_pause_rp' semaphore. When the user issues a
>> migrate resume, a new return path is opened and the thread is allowed
>> to continue.
>> 
>> There's a race condition in the above mechanism. It is possible for
>> the new return path file to be setup *before* the cleanup code in the
>> return path thread has had a chance to run, leading to the *new* file
>> being closed and the pointer set to NULL. When the thread is released
>> after the resume, it tries to dereference 'from_dst_file' and crashes:
>> 
>> Thread 7 "return path" received signal SIGSEGV, Segmentation fault.
>> [Switching to Thread 0x7fffd1dbf700 (LWP 9611)]
>> 0x00005555560e4893 in qemu_file_get_error_obj (f=0x0, errp=0x0) at ../migration/qemu-file.c:154
>> 154         return f->last_error;
>> 
>> (gdb) bt
>>  #0  0x00005555560e4893 in qemu_file_get_error_obj (f=0x0, errp=0x0) at ../migration/qemu-file.c:154
>>  #1  0x00005555560e4983 in qemu_file_get_error (f=0x0) at ../migration/qemu-file.c:206
>>  #2  0x0000555555b9a1df in source_return_path_thread (opaque=0x555556e06000) at ../migration/migration.c:1876
>>  #3  0x000055555602e14f in qemu_thread_start (args=0x55555782e780) at ../util/qemu-thread-posix.c:541
>>  #4  0x00007ffff38d76ea in start_thread (arg=0x7fffd1dbf700) at pthread_create.c:477
>>  #5  0x00007ffff35efa6f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
>> 
>> Here's the race (important bit is open_return_path happening before
>> migration_release_dst_files):
>> 
>> migration                 | qmp                         | return path
>> --------------------------+-----------------------------+---------------------------------
>> 			    qmp_migrate_pause()
>> 			     shutdown(ms->to_dst_file)
>> 			      f->last_error = -EIO
>> migrate_detect_error()
>>  postcopy_pause()
>>   set_state(PAUSED)
>>   wait(postcopy_pause_sem)
>> 			    qmp_migrate(resume)
>> 			    migrate_fd_connect()
>> 			     resume = state == PAUSED
>> 			     open_return_path <-- TOO SOON!
>> 			     set_state(RECOVER)
>> 			     post(postcopy_pause_sem)
>> 							(incoming closes to_src_file)
>> 							res = qemu_file_get_error(rp)
>> 							migration_release_dst_files()
>> 							ms->rp_state.from_dst_file = NULL
>>   post(postcopy_pause_rp_sem)
>> 							postcopy_pause_return_path_thread()
>> 							  wait(postcopy_pause_rp_sem)
>> 							rp = ms->rp_state.from_dst_file
>> 							goto retry
>> 							qemu_file_get_error(rp)
>> 							SIGSEGV
>> -------------------------------------------------------------------------------------------
>> 
>> We can keep the retry logic without having the thread alive and
>> waiting. The only piece of data used by it is the 'from_dst_file' and
>> it is only allowed to proceed after a migrate resume is issued and the
>> semaphore released at migrate_fd_connect().
>> 
>> Move the retry logic to outside the thread by having
>> open_return_path_on_source() wait for the thread to finish before
>> creating a new one with the updated 'from_dst_file'.
>
> If we can remove that (along with the sync sem) it'll be pretty nice.  If
> you want, and if this works well, not sure whether you're interested in
> doing similarly to the other threads.  Currently we halt all the threads,
> I'm not sure whether we can do similiar things on dest and whether that can
> also benefit on sync efforts.

I'm interested but I don't know the postcopy code too well, I'll have to
spend some time with it.

Next on my list was the multifd p->running flag which seems entirely
superfluous to me.

>
> Still one comment below.
>
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/migration.c | 72 +++++++++++++++----------------------------
>>  migration/migration.h |  1 -
>>  2 files changed, 25 insertions(+), 48 deletions(-)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index d6f4470265..36cdd7bda8 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -97,6 +97,7 @@ static int migration_maybe_pause(MigrationState *s,
>>                                   int *current_active_state,
>>                                   int new_state);
>>  static void migrate_fd_cancel(MigrationState *s);
>> +static int await_return_path_close_on_source(MigrationState *ms);
>>  
>>  static bool migration_needs_multiple_sockets(void)
>>  {
>> @@ -1764,18 +1765,6 @@ static void migrate_handle_rp_req_pages(MigrationState *ms, const char* rbname,
>>      }
>>  }
>>  
>> -/* Return true to retry, false to quit */
>> -static bool postcopy_pause_return_path_thread(MigrationState *s)
>> -{
>> -    trace_postcopy_pause_return_path();
>> -
>> -    qemu_sem_wait(&s->postcopy_pause_rp_sem);
>> -
>> -    trace_postcopy_pause_return_path_continued();
>> -
>> -    return true;
>> -}
>> -
>>  static int migrate_handle_rp_recv_bitmap(MigrationState *s, char *block_name)
>>  {
>>      RAMBlock *block = qemu_ram_block_by_name(block_name);
>> @@ -1859,7 +1848,6 @@ static void *source_return_path_thread(void *opaque)
>>      trace_source_return_path_thread_entry();
>>      rcu_register_thread();
>>  
>> -retry:
>>      while (!ms->rp_state.error && !qemu_file_get_error(rp) &&
>>             migration_is_setup_or_active(ms->state)) {
>>          trace_source_return_path_thread_loop_top();
>> @@ -1981,28 +1969,18 @@ retry:
>>      }
>>  
>>  out:
>> -    res = qemu_file_get_error(rp);
>> -    if (res) {
>> -        if (res && migration_in_postcopy()) {
>> +    if (qemu_file_get_error(rp)) {
>> +        if (migration_in_postcopy()) {
>>              /*
>> -             * Maybe there is something we can do: it looks like a
>> -             * network down issue, and we pause for a recovery.
>> +             * This could be a network issue that would have been
>> +             * detected by the main migration thread and caused the
>> +             * migration to pause. Do cleanup and finish.
>>               */
>> -            migration_release_dst_files(ms);
>> -            rp = NULL;
>> -            if (postcopy_pause_return_path_thread(ms)) {
>> -                /*
>> -                 * Reload rp, reset the rest.  Referencing it is safe since
>> -                 * it's reset only by us above, or when migration completes
>> -                 */
>> -                rp = ms->rp_state.from_dst_file;
>> -                ms->rp_state.error = false;
>> -                goto retry;
>> -            }
>> +            ms->rp_state.error = false;
>> +        } else {
>> +            trace_source_return_path_thread_bad_end();
>> +            mark_source_rp_bad(ms);
>>          }
>> -
>> -        trace_source_return_path_thread_bad_end();
>> -        mark_source_rp_bad(ms);
>>      }
>>  
>>      trace_source_return_path_thread_end();
>> @@ -2012,8 +1990,21 @@ out:
>>  }
>>  
>>  static int open_return_path_on_source(MigrationState *ms,
>> -                                      bool create_thread)
>> +                                      bool resume)
>>  {
>> +    if (resume) {
>> +        assert(ms->state == MIGRATION_STATUS_POSTCOPY_PAUSED);
>> +
>> +        /*
>> +         * We're resuming from a paused postcopy migration. Wait for
>> +         * the thread to do its cleanup before re-opening the return
>> +         * path.
>> +         */
>> +        if (await_return_path_close_on_source(ms)) {
>> +            return -1;
>> +        }
>
> This smells a bit hacky.  Can we do this in postcopy_pause(), perhaps
> before we switch to PAUSED state?  Then we know after being PAUSED we're
> ready to recover.

It looks like we could. I'll move it.