In case of error, close_return_path_on_source() can perform a shutdown
to exit the return-path thread.  However, in migrate_fd_cleanup(),
'to_dst_file' is closed before calling close_return_path_on_source()
and the shutdown fails, leaving the source and destination waiting for
an event to occur.

Close the file after calling close_return_path_on_source() so that the
shutdown succeeds and the return-path thread exits.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
---

 This is an RFC because the correct fix implies reworking the QEMUFile
 construct, built on top of the QEMU I/O channel.

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

diff --git a/migration/migration.c b/migration/migration.c
index 5f55af3d7624750ca416c4177781241b3e291e5d..de329f2c553288935d824748286e79e535929b8b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1313,6 +1313,8 @@ void migrate_set_state(int *state, int old_state, int new_state)
 
 static void migrate_fd_cleanup(MigrationState *s)
 {
+    QEMUFile *tmp = NULL;
+
     g_free(s->hostname);
     s->hostname = NULL;
     json_writer_free(s->vmdesc);
@@ -1321,8 +1323,6 @@ static void migrate_fd_cleanup(MigrationState *s)
     qemu_savevm_state_cleanup();
 
     if (s->to_dst_file) {
-        QEMUFile *tmp;
-
         trace_migrate_fd_cleanup();
         bql_unlock();
         if (s->migration_thread_running) {
@@ -1341,15 +1341,14 @@ static void migrate_fd_cleanup(MigrationState *s)
          * critical section won't block for long.
          */
         migration_ioc_unregister_yank_from_file(tmp);
-        qemu_fclose(tmp);
     }
 
-    /*
-     * We already cleaned up to_dst_file, so errors from the return
-     * path might be due to that, ignore them.
-     */
     close_return_path_on_source(s);
 
+    if (tmp) {
+        qemu_fclose(tmp);
+    }
+
     assert(!migration_is_active(s));
 
     if (s->state == MIGRATION_STATUS_CANCELLING) {
-- 
2.43.0

Cédric Le Goater <clg@redhat.com> writes:

> In case of error, close_return_path_on_source() can perform a shutdown
> to exit the return-path thread.  However, in migrate_fd_cleanup(),
> 'to_dst_file' is closed before calling close_return_path_on_source()
> and the shutdown fails, leaving the source and destination waiting for
> an event to occur.

Hi, Cédric

Are you sure this is not caused by patch 13? That 'if (ms->to_dst_file'
was there to avoid this sort of thing happening.

Is there some reordering possibility that I'm not spotting in the code
below? I think the data dependency on to_dst_file shouldn't allow it.

migrate_fd_cleanup:
        qemu_mutex_lock(&s->qemu_file_lock);
        tmp = s->to_dst_file;
        s->to_dst_file = NULL;
        qemu_mutex_unlock(&s->qemu_file_lock);
        ...
        qemu_fclose(tmp);

close_return_path_on_source:
    WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
        if (ms->to_dst_file && ms->rp_state.from_dst_file &&
            qemu_file_get_error(ms->to_dst_file)) {
            qemu_file_shutdown(ms->rp_state.from_dst_file);
        }
    }

I'm thinking maybe the culprit is the close_return_path_on_source() at
migration_completion(). It might be possible for it to race with the
migrate_fd_cleanup_bh from migration_iteration_finish().

If that's the case, then I think that one possible fix would be to hold
the BQL at migration_completion() so the BH doesn't get dispatched until
we properly close the return path.

>
> Close the file after calling close_return_path_on_source() so that the
> shutdown succeeds and the return-path thread exits.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>
>  This is an RFC because the correct fix implies reworking the QEMUFile
>  construct, built on top of the QEMU I/O channel.
>
>  migration/migration.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 5f55af3d7624750ca416c4177781241b3e291e5d..de329f2c553288935d824748286e79e535929b8b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1313,6 +1313,8 @@ void migrate_set_state(int *state, int old_state, int new_state)
>  
>  static void migrate_fd_cleanup(MigrationState *s)
>  {
> +    QEMUFile *tmp = NULL;
> +
>      g_free(s->hostname);
>      s->hostname = NULL;
>      json_writer_free(s->vmdesc);
> @@ -1321,8 +1323,6 @@ static void migrate_fd_cleanup(MigrationState *s)
>      qemu_savevm_state_cleanup();
>  
>      if (s->to_dst_file) {
> -        QEMUFile *tmp;
> -
>          trace_migrate_fd_cleanup();
>          bql_unlock();
>          if (s->migration_thread_running) {
> @@ -1341,15 +1341,14 @@ static void migrate_fd_cleanup(MigrationState *s)
>           * critical section won't block for long.
>           */
>          migration_ioc_unregister_yank_from_file(tmp);
> -        qemu_fclose(tmp);
>      }
>  
> -    /*
> -     * We already cleaned up to_dst_file, so errors from the return
> -     * path might be due to that, ignore them.
> -     */
>      close_return_path_on_source(s);
>  
> +    if (tmp) {
> +        qemu_fclose(tmp);
> +    }
> +
>      assert(!migration_is_active(s));
>  
>      if (s->state == MIGRATION_STATUS_CANCELLING) {

Hello Fabiano

On 2/8/24 14:29, Fabiano Rosas wrote:
> Cédric Le Goater <clg@redhat.com> writes:
> 
>> In case of error, close_return_path_on_source() can perform a shutdown
>> to exit the return-path thread.  However, in migrate_fd_cleanup(),
>> 'to_dst_file' is closed before calling close_return_path_on_source()
>> and the shutdown fails, leaving the source and destination waiting for
>> an event to occur.
> 
> Hi, Cédric
> 
> Are you sure this is not caused by patch 13? 

It happens with upstream QEMU without any patch.

When vfio_listener_log_global_start() fails, it sets an error on the
QEMUFile. To reproduce without a VFIO device, you can inject an error
when dirty tracking is started. Something like below,

     @@ -2817,6 +2817,8 @@ static void ram_init_bitmaps(RAMState *r
           * containing all 1s to exclude any discarded pages from migration.
           */
          migration_bitmap_clear_discarded_pages(rs);
     +
     +    qemu_file_set_error(migrate_get_current()->to_dst_file, -EAGAIN);
      }
      
      static int ram_init_all(RAMState **rsp)

Activate return-path and migrate.

> That 'if (ms->to_dst_file'
> was there to avoid this sort of thing happening.
> 
> Is there some reordering possibility that I'm not spotting in the code
> below? I think the data dependency on to_dst_file shouldn't allow it.
> 
> migrate_fd_cleanup:
>          qemu_mutex_lock(&s->qemu_file_lock);
>          tmp = s->to_dst_file;
>          s->to_dst_file = NULL;
>          qemu_mutex_unlock(&s->qemu_file_lock);
>          ...
>          qemu_fclose(tmp);
> 
> close_return_path_on_source:
>      WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
>          if (ms->to_dst_file && ms->rp_state.from_dst_file &&
>              qemu_file_get_error(ms->to_dst_file)) {
>              qemu_file_shutdown(ms->rp_state.from_dst_file);
>          }
>      }

close_return_path_on_source() is called by migrate_fd_cleanup() in
the same thread. So, when we reach the locking section ms->to_dst_file
is already NULL and qemu_fclose() has been closed :/

May be I misunderstood. Please try to reproduce with the little hack
above.

Thanks,

C.

> I'm thinking maybe the culprit is the close_return_path_on_source() at
> migration_completion(). It might be possible for it to race with the
> migrate_fd_cleanup_bh from migration_iteration_finish().
> 
> If that's the case, then I think that one possible fix would be to hold
> the BQL at migration_completion() so the BH doesn't get dispatched until
> we properly close the return path.
> 
>>
>> Close the file after calling close_return_path_on_source() so that the
>> shutdown succeeds and the return-path thread exits.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>
>>   This is an RFC because the correct fix implies reworking the QEMUFile
>>   construct, built on top of the QEMU I/O channel.
>>
>>   migration/migration.c | 13 ++++++-------
>>   1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 5f55af3d7624750ca416c4177781241b3e291e5d..de329f2c553288935d824748286e79e535929b8b 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1313,6 +1313,8 @@ void migrate_set_state(int *state, int old_state, int new_state)
>>   
>>   static void migrate_fd_cleanup(MigrationState *s)
>>   {
>> +    QEMUFile *tmp = NULL;
>> +
>>       g_free(s->hostname);
>>       s->hostname = NULL;
>>       json_writer_free(s->vmdesc);
>> @@ -1321,8 +1323,6 @@ static void migrate_fd_cleanup(MigrationState *s)
>>       qemu_savevm_state_cleanup();
>>   
>>       if (s->to_dst_file) {
>> -        QEMUFile *tmp;
>> -
>>           trace_migrate_fd_cleanup();
>>           bql_unlock();
>>           if (s->migration_thread_running) {
>> @@ -1341,15 +1341,14 @@ static void migrate_fd_cleanup(MigrationState *s)
>>            * critical section won't block for long.
>>            */
>>           migration_ioc_unregister_yank_from_file(tmp);
>> -        qemu_fclose(tmp);
>>       }
>>   
>> -    /*
>> -     * We already cleaned up to_dst_file, so errors from the return
>> -     * path might be due to that, ignore them.
>> -     */
>>       close_return_path_on_source(s);
>>   
>> +    if (tmp) {
>> +        qemu_fclose(tmp);
>> +    }
>> +
>>       assert(!migration_is_active(s));
>>   
>>       if (s->state == MIGRATION_STATUS_CANCELLING) {
> 

Cédric Le Goater <clg@redhat.com> writes:

> Hello Fabiano
>
> On 2/8/24 14:29, Fabiano Rosas wrote:
>> Cédric Le Goater <clg@redhat.com> writes:
>> 
>>> In case of error, close_return_path_on_source() can perform a shutdown
>>> to exit the return-path thread.  However, in migrate_fd_cleanup(),
>>> 'to_dst_file' is closed before calling close_return_path_on_source()
>>> and the shutdown fails, leaving the source and destination waiting for
>>> an event to occur.
>> 
>> Hi, Cédric
>> 
>> Are you sure this is not caused by patch 13? 
>
> It happens with upstream QEMU without any patch.

I might have taken that "shutdown fails" in the commit message too
literaly. Anyway, I have a proposed solution:

-->8--
From 729aa7b5b7f130f756d41649fdd0862bd2e90430 Mon Sep 17 00:00:00 2001
From: Fabiano Rosas <farosas@suse.de>
Date: Wed, 14 Feb 2024 16:45:43 -0300
Subject: [PATCH] migration: Join the return path thread before releasing
 to_dst_file
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The return path thread might hang at a blocking system call. Before
joining the thread we might need to issue a shutdown() on the socket
file descriptor to release it. To determine whether the shutdown() is
necessary we look at the QEMUFile error.

Make sure we only clean up the QEMUFile after the return path has been
waited for.

This fixes a hang when qemu_savevm_state_setup() produced an error
that was detected by migration_detect_error(). That skips
migration_completion() so close_return_path_on_source() would get
stuck waiting for the RP thread to terminate.

At migrate_fd_cleanup() I'm keeping the relative order of joining the
migration thread and the return path just in case.

Reported-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index ab21de2cad..f0b70e8a9d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1326,17 +1326,19 @@ static void migrate_fd_cleanup(MigrationState *s)
 
     qemu_savevm_state_cleanup();
 
+    bql_unlock();
+    if (s->migration_thread_running) {
+        qemu_thread_join(&s->thread);
+        s->migration_thread_running = false;
+    }
+    bql_lock();
+
+    close_return_path_on_source(s);
+
     if (s->to_dst_file) {
         QEMUFile *tmp;
 
         trace_migrate_fd_cleanup();
-        bql_unlock();
-        if (s->migration_thread_running) {
-            qemu_thread_join(&s->thread);
-            s->migration_thread_running = false;
-        }
-        bql_lock();
-
         multifd_send_shutdown();
         qemu_mutex_lock(&s->qemu_file_lock);
         tmp = s->to_dst_file;
@@ -1350,12 +1352,6 @@ static void migrate_fd_cleanup(MigrationState *s)
         qemu_fclose(tmp);
     }
 
-    /*
-     * We already cleaned up to_dst_file, so errors from the return
-     * path might be due to that, ignore them.
-     */
-    close_return_path_on_source(s);
-
     assert(!migration_is_active(s));
 
     if (s->state == MIGRATION_STATUS_CANCELLING) {
@@ -2874,6 +2870,13 @@ static MigThrError postcopy_pause(MigrationState *s)
     while (true) {
         QEMUFile *file;
 
+        /*
+         * We're already pausing, so ignore any errors on the return
+         * path and just wait for the thread to finish. It will be
+         * re-created when we resume.
+         */
+        close_return_path_on_source(s);
+
         /*
          * Current channel is possibly broken. Release it.  Note that this is
          * guaranteed even without lock because to_dst_file should only be
@@ -2893,13 +2896,6 @@ static MigThrError postcopy_pause(MigrationState *s)
         qemu_file_shutdown(file);
         qemu_fclose(file);
 
-        /*
-         * We're already pausing, so ignore any errors on the return
-         * path and just wait for the thread to finish. It will be
-         * re-created when we resume.
-         */
-        close_return_path_on_source(s);
-
         migrate_set_state(&s->state, s->state,
                           MIGRATION_STATUS_POSTCOPY_PAUSED);
 
-- 
2.35.3

Hello Fabiano

On 2/14/24 21:35, Fabiano Rosas wrote:
> Cédric Le Goater <clg@redhat.com> writes:
> 
>> Hello Fabiano
>>
>> On 2/8/24 14:29, Fabiano Rosas wrote:
>>> Cédric Le Goater <clg@redhat.com> writes:
>>>
>>>> In case of error, close_return_path_on_source() can perform a shutdown
>>>> to exit the return-path thread.  However, in migrate_fd_cleanup(),
>>>> 'to_dst_file' is closed before calling close_return_path_on_source()
>>>> and the shutdown fails, leaving the source and destination waiting for
>>>> an event to occur.
>>>
>>> Hi, Cédric
>>>
>>> Are you sure this is not caused by patch 13?
>>
>> It happens with upstream QEMU without any patch.
> 
> I might have taken that "shutdown fails" in the commit message too
> literaly. Anyway, I have a proposed solution:
> 
> -->8--
>  From 729aa7b5b7f130f756d41649fdd0862bd2e90430 Mon Sep 17 00:00:00 2001
> From: Fabiano Rosas <farosas@suse.de>
> Date: Wed, 14 Feb 2024 16:45:43 -0300
> Subject: [PATCH] migration: Join the return path thread before releasing
>   to_dst_file
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> The return path thread might hang at a blocking system call. Before
> joining the thread we might need to issue a shutdown() on the socket
> file descriptor to release it. To determine whether the shutdown() is
> necessary we look at the QEMUFile error.
> 
> Make sure we only clean up the QEMUFile after the return path has been
> waited for.

Yes. That's the important part.

> This fixes a hang when qemu_savevm_state_setup() produced an error
> that was detected by migration_detect_error(). That skips
> migration_completion() so close_return_path_on_source() would get
> stuck waiting for the RP thread to terminate.
> 
> At migrate_fd_cleanup() I'm keeping the relative order of joining the
> migration thread and the return path just in case.

That doesn't look necessary. What was the reason to join the migration
thread only when s->to_dst_file is valid ?


> Reported-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

LGTM, it fixes the hang when an error is detected, the migration is
aborted and the VM resumes execution. FWIW,

Tested-by: Cédric Le Goater <clg@redhat.com>

It requires more thorough testing though.

Thanks,

C.




> ---
>   migration/migration.c | 36 ++++++++++++++++--------------------
>   1 file changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index ab21de2cad..f0b70e8a9d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1326,17 +1326,19 @@ static void migrate_fd_cleanup(MigrationState *s)
>   
>       qemu_savevm_state_cleanup();
>   
> +    bql_unlock();
> +    if (s->migration_thread_running) {
> +        qemu_thread_join(&s->thread);
> +        s->migration_thread_running = false;
> +    }
> +    bql_lock();
> +
> +    close_return_path_on_source(s);
> +
>       if (s->to_dst_file) {
>           QEMUFile *tmp;
>   
>           trace_migrate_fd_cleanup();
> -        bql_unlock();
> -        if (s->migration_thread_running) {
> -            qemu_thread_join(&s->thread);
> -            s->migration_thread_running = false;
> -        }
> -        bql_lock();
> -
>           multifd_send_shutdown();
>           qemu_mutex_lock(&s->qemu_file_lock);
>           tmp = s->to_dst_file;
> @@ -1350,12 +1352,6 @@ static void migrate_fd_cleanup(MigrationState *s)
>           qemu_fclose(tmp);
>       }
>   
> -    /*
> -     * We already cleaned up to_dst_file, so errors from the return
> -     * path might be due to that, ignore them.
> -     */
> -    close_return_path_on_source(s);
> -
>       assert(!migration_is_active(s));
>   
>       if (s->state == MIGRATION_STATUS_CANCELLING) {
> @@ -2874,6 +2870,13 @@ static MigThrError postcopy_pause(MigrationState *s)
>       while (true) {
>           QEMUFile *file;
>   
> +        /*
> +         * We're already pausing, so ignore any errors on the return
> +         * path and just wait for the thread to finish. It will be
> +         * re-created when we resume.
> +         */
> +        close_return_path_on_source(s);
> +
>           /*
>            * Current channel is possibly broken. Release it.  Note that this is
>            * guaranteed even without lock because to_dst_file should only be
> @@ -2893,13 +2896,6 @@ static MigThrError postcopy_pause(MigrationState *s)
>           qemu_file_shutdown(file);
>           qemu_fclose(file);
>   
> -        /*
> -         * We're already pausing, so ignore any errors on the return
> -         * path and just wait for the thread to finish. It will be
> -         * re-created when we resume.
> -         */
> -        close_return_path_on_source(s);
> -
>           migrate_set_state(&s->state, s->state,
>                             MIGRATION_STATUS_POSTCOPY_PAUSED);
>   

Cédric Le Goater <clg@redhat.com> writes:

> Hello Fabiano
>
> On 2/14/24 21:35, Fabiano Rosas wrote:
>> Cédric Le Goater <clg@redhat.com> writes:
>> 
>>> Hello Fabiano
>>>
>>> On 2/8/24 14:29, Fabiano Rosas wrote:
>>>> Cédric Le Goater <clg@redhat.com> writes:
>>>>
>>>>> In case of error, close_return_path_on_source() can perform a shutdown
>>>>> to exit the return-path thread.  However, in migrate_fd_cleanup(),
>>>>> 'to_dst_file' is closed before calling close_return_path_on_source()
>>>>> and the shutdown fails, leaving the source and destination waiting for
>>>>> an event to occur.
>>>>
>>>> Hi, Cédric
>>>>
>>>> Are you sure this is not caused by patch 13?
>>>
>>> It happens with upstream QEMU without any patch.
>> 
>> I might have taken that "shutdown fails" in the commit message too
>> literaly. Anyway, I have a proposed solution:
>> 
>> -->8--
>>  From 729aa7b5b7f130f756d41649fdd0862bd2e90430 Mon Sep 17 00:00:00 2001
>> From: Fabiano Rosas <farosas@suse.de>
>> Date: Wed, 14 Feb 2024 16:45:43 -0300
>> Subject: [PATCH] migration: Join the return path thread before releasing
>>   to_dst_file
>> MIME-Version: 1.0
>> Content-Type: text/plain; charset=UTF-8
>> Content-Transfer-Encoding: 8bit
>> 
>> The return path thread might hang at a blocking system call. Before
>> joining the thread we might need to issue a shutdown() on the socket
>> file descriptor to release it. To determine whether the shutdown() is
>> necessary we look at the QEMUFile error.
>> 
>> Make sure we only clean up the QEMUFile after the return path has been
>> waited for.
>
> Yes. That's the important part.
>
>> This fixes a hang when qemu_savevm_state_setup() produced an error
>> that was detected by migration_detect_error(). That skips
>> migration_completion() so close_return_path_on_source() would get
>> stuck waiting for the RP thread to terminate.
>> 
>> At migrate_fd_cleanup() I'm keeping the relative order of joining the
>> migration thread and the return path just in case.
>
> That doesn't look necessary.

Indeed. But I don't trust the migration code, it's full of undocumented
dependencies like that.

> What was the reason to join the migration thread only when
> s->to_dst_file is valid ?

I didn't find any explicit reason looking through the history. It seems
we used to rely on to_dst_file before migration_thread_running was
introduced.

I wouldn't mind keeping that 'if' there.

Let's see what Peter thinks about it.

On Fri, Feb 16, 2024 at 02:35:26PM -0300, Fabiano Rosas wrote:
> Cédric Le Goater <clg@redhat.com> writes:
> 
> > Hello Fabiano
> >
> > On 2/14/24 21:35, Fabiano Rosas wrote:
> >> Cédric Le Goater <clg@redhat.com> writes:
> >> 
> >>> Hello Fabiano
> >>>
> >>> On 2/8/24 14:29, Fabiano Rosas wrote:
> >>>> Cédric Le Goater <clg@redhat.com> writes:
> >>>>
> >>>>> In case of error, close_return_path_on_source() can perform a shutdown
> >>>>> to exit the return-path thread.  However, in migrate_fd_cleanup(),
> >>>>> 'to_dst_file' is closed before calling close_return_path_on_source()
> >>>>> and the shutdown fails, leaving the source and destination waiting for
> >>>>> an event to occur.
> >>>>
> >>>> Hi, Cédric
> >>>>
> >>>> Are you sure this is not caused by patch 13?
> >>>
> >>> It happens with upstream QEMU without any patch.
> >> 
> >> I might have taken that "shutdown fails" in the commit message too
> >> literaly. Anyway, I have a proposed solution:
> >> 
> >> -->8--
> >>  From 729aa7b5b7f130f756d41649fdd0862bd2e90430 Mon Sep 17 00:00:00 2001
> >> From: Fabiano Rosas <farosas@suse.de>
> >> Date: Wed, 14 Feb 2024 16:45:43 -0300
> >> Subject: [PATCH] migration: Join the return path thread before releasing
> >>   to_dst_file
> >> MIME-Version: 1.0
> >> Content-Type: text/plain; charset=UTF-8
> >> Content-Transfer-Encoding: 8bit
> >> 
> >> The return path thread might hang at a blocking system call. Before
> >> joining the thread we might need to issue a shutdown() on the socket
> >> file descriptor to release it. To determine whether the shutdown() is
> >> necessary we look at the QEMUFile error.
> >> 
> >> Make sure we only clean up the QEMUFile after the return path has been
> >> waited for.
> >
> > Yes. That's the important part.
> >
> >> This fixes a hang when qemu_savevm_state_setup() produced an error
> >> that was detected by migration_detect_error(). That skips
> >> migration_completion() so close_return_path_on_source() would get
> >> stuck waiting for the RP thread to terminate.
> >> 
> >> At migrate_fd_cleanup() I'm keeping the relative order of joining the
> >> migration thread and the return path just in case.
> >
> > That doesn't look necessary.
> 
> Indeed. But I don't trust the migration code, it's full of undocumented
> dependencies like that.
> 
> > What was the reason to join the migration thread only when
> > s->to_dst_file is valid ?
> 
> I didn't find any explicit reason looking through the history. It seems
> we used to rely on to_dst_file before migration_thread_running was
> introduced.
> 
> I wouldn't mind keeping that 'if' there.
> 
> Let's see what Peter thinks about it.

Frankly I don't have a strong opinion on current patch 14 or the new
proposal, but it seems we reached a consensus.

Fabiano, would you repost with a formal patch, with the proper tags?

One thing I am still not sure is whether we should still have patch 13
altogether? Please see my other reply on whether it's possible to have
migrate_get_error() == true but qemu_file_get_error() == false.  In
postcopy_pause(), currently we constantly shutdown() so the join() should
always work:

        qemu_file_shutdown(file);
        qemu_fclose(file);

        /*
         * We're already pausing, so ignore any errors on the return
         * path and just wait for the thread to finish. It will be
         * re-created when we resume.
         */
        close_return_path_on_source(s);

If move close_return_path_on_source() upper, qemu_file_shutdown() may not
be needed? And I think we need to make sure close_return_path_on_source()
will always properly kick the other thread.

Thanks,

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Fri, Feb 16, 2024 at 02:35:26PM -0300, Fabiano Rosas wrote:
>> Cédric Le Goater <clg@redhat.com> writes:
>> 
>> > Hello Fabiano
>> >
>> > On 2/14/24 21:35, Fabiano Rosas wrote:
>> >> Cédric Le Goater <clg@redhat.com> writes:
>> >> 
>> >>> Hello Fabiano
>> >>>
>> >>> On 2/8/24 14:29, Fabiano Rosas wrote:
>> >>>> Cédric Le Goater <clg@redhat.com> writes:
>> >>>>
>> >>>>> In case of error, close_return_path_on_source() can perform a shutdown
>> >>>>> to exit the return-path thread.  However, in migrate_fd_cleanup(),
>> >>>>> 'to_dst_file' is closed before calling close_return_path_on_source()
>> >>>>> and the shutdown fails, leaving the source and destination waiting for
>> >>>>> an event to occur.
>> >>>>
>> >>>> Hi, Cédric
>> >>>>
>> >>>> Are you sure this is not caused by patch 13?
>> >>>
>> >>> It happens with upstream QEMU without any patch.
>> >> 
>> >> I might have taken that "shutdown fails" in the commit message too
>> >> literaly. Anyway, I have a proposed solution:
>> >> 
>> >> -->8--
>> >>  From 729aa7b5b7f130f756d41649fdd0862bd2e90430 Mon Sep 17 00:00:00 2001
>> >> From: Fabiano Rosas <farosas@suse.de>
>> >> Date: Wed, 14 Feb 2024 16:45:43 -0300
>> >> Subject: [PATCH] migration: Join the return path thread before releasing
>> >>   to_dst_file
>> >> MIME-Version: 1.0
>> >> Content-Type: text/plain; charset=UTF-8
>> >> Content-Transfer-Encoding: 8bit
>> >> 
>> >> The return path thread might hang at a blocking system call. Before
>> >> joining the thread we might need to issue a shutdown() on the socket
>> >> file descriptor to release it. To determine whether the shutdown() is
>> >> necessary we look at the QEMUFile error.
>> >> 
>> >> Make sure we only clean up the QEMUFile after the return path has been
>> >> waited for.
>> >
>> > Yes. That's the important part.
>> >
>> >> This fixes a hang when qemu_savevm_state_setup() produced an error
>> >> that was detected by migration_detect_error(). That skips
>> >> migration_completion() so close_return_path_on_source() would get
>> >> stuck waiting for the RP thread to terminate.
>> >> 
>> >> At migrate_fd_cleanup() I'm keeping the relative order of joining the
>> >> migration thread and the return path just in case.
>> >
>> > That doesn't look necessary.
>> 
>> Indeed. But I don't trust the migration code, it's full of undocumented
>> dependencies like that.
>> 
>> > What was the reason to join the migration thread only when
>> > s->to_dst_file is valid ?
>> 
>> I didn't find any explicit reason looking through the history. It seems
>> we used to rely on to_dst_file before migration_thread_running was
>> introduced.
>> 
>> I wouldn't mind keeping that 'if' there.
>> 
>> Let's see what Peter thinks about it.
>
> Frankly I don't have a strong opinion on current patch 14 or the new
> proposal, but it seems we reached a consensus.
>
> Fabiano, would you repost with a formal patch, with the proper tags?

Yes, I'll post it soon.

>
> One thing I am still not sure is whether we should still have patch 13
> altogether? Please see my other reply on whether it's possible to have
> migrate_get_error() == true but qemu_file_get_error() == false.

I'll include it then.

> In
> postcopy_pause(), currently we constantly shutdown() so the join() should
> always work:
>
>         qemu_file_shutdown(file);
>         qemu_fclose(file);
>
>         /*
>          * We're already pausing, so ignore any errors on the return
>          * path and just wait for the thread to finish. It will be
>          * re-created when we resume.
>          */
>         close_return_path_on_source(s);
>
> If move close_return_path_on_source() upper, qemu_file_shutdown() may not
> be needed? And I think we need to make sure close_return_path_on_source()
> will always properly kick the other thread.

On 2/23/24 15:05, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
>> On Fri, Feb 16, 2024 at 02:35:26PM -0300, Fabiano Rosas wrote:
>>> Cédric Le Goater <clg@redhat.com> writes:
>>>
>>>> Hello Fabiano
>>>>
>>>> On 2/14/24 21:35, Fabiano Rosas wrote:
>>>>> Cédric Le Goater <clg@redhat.com> writes:
>>>>>
>>>>>> Hello Fabiano
>>>>>>
>>>>>> On 2/8/24 14:29, Fabiano Rosas wrote:
>>>>>>> Cédric Le Goater <clg@redhat.com> writes:
>>>>>>>
>>>>>>>> In case of error, close_return_path_on_source() can perform a shutdown
>>>>>>>> to exit the return-path thread.  However, in migrate_fd_cleanup(),
>>>>>>>> 'to_dst_file' is closed before calling close_return_path_on_source()
>>>>>>>> and the shutdown fails, leaving the source and destination waiting for
>>>>>>>> an event to occur.
>>>>>>>
>>>>>>> Hi, Cédric
>>>>>>>
>>>>>>> Are you sure this is not caused by patch 13?
>>>>>>
>>>>>> It happens with upstream QEMU without any patch.
>>>>>
>>>>> I might have taken that "shutdown fails" in the commit message too
>>>>> literaly. Anyway, I have a proposed solution:
>>>>>
>>>>> -->8--
>>>>>   From 729aa7b5b7f130f756d41649fdd0862bd2e90430 Mon Sep 17 00:00:00 2001
>>>>> From: Fabiano Rosas <farosas@suse.de>
>>>>> Date: Wed, 14 Feb 2024 16:45:43 -0300
>>>>> Subject: [PATCH] migration: Join the return path thread before releasing
>>>>>    to_dst_file
>>>>> MIME-Version: 1.0
>>>>> Content-Type: text/plain; charset=UTF-8
>>>>> Content-Transfer-Encoding: 8bit
>>>>>
>>>>> The return path thread might hang at a blocking system call. Before
>>>>> joining the thread we might need to issue a shutdown() on the socket
>>>>> file descriptor to release it. To determine whether the shutdown() is
>>>>> necessary we look at the QEMUFile error.
>>>>>
>>>>> Make sure we only clean up the QEMUFile after the return path has been
>>>>> waited for.
>>>>
>>>> Yes. That's the important part.
>>>>
>>>>> This fixes a hang when qemu_savevm_state_setup() produced an error
>>>>> that was detected by migration_detect_error(). That skips
>>>>> migration_completion() so close_return_path_on_source() would get
>>>>> stuck waiting for the RP thread to terminate.
>>>>>
>>>>> At migrate_fd_cleanup() I'm keeping the relative order of joining the
>>>>> migration thread and the return path just in case.
>>>>
>>>> That doesn't look necessary.
>>>
>>> Indeed. But I don't trust the migration code, it's full of undocumented
>>> dependencies like that.
>>>
>>>> What was the reason to join the migration thread only when
>>>> s->to_dst_file is valid ?
>>>
>>> I didn't find any explicit reason looking through the history. It seems
>>> we used to rely on to_dst_file before migration_thread_running was
>>> introduced.
>>>
>>> I wouldn't mind keeping that 'if' there.
>>>
>>> Let's see what Peter thinks about it.
>>
>> Frankly I don't have a strong opinion on current patch 14 or the new
>> proposal, but it seems we reached a consensus.
>>
>> Fabiano, would you repost with a formal patch, with the proper tags?
> 
> Yes, I'll post it soon.
> 
>>
>> One thing I am still not sure is whether we should still have patch 13
>> altogether? Please see my other reply on whether it's possible to have
>> migrate_get_error() == true but qemu_file_get_error() == false.
> 
> I'll include it then.

Thanks for taking over.

I have included :

  [PATCH] migration: Join the return path thread before releasing to_dst_file

in my series and dropped 13-14. I hope to send a follow up on :

   https://lore.kernel.org/qemu-devel/20240207133347.1115903-1-clg@redhat.com/

before we reach soft freeze. It's growing quite a lot.

C.

On Wed, Feb 07, 2024 at 02:33:47PM +0100, Cédric Le Goater wrote:
> In case of error, close_return_path_on_source() can perform a shutdown
> to exit the return-path thread.  However, in migrate_fd_cleanup(),
> 'to_dst_file' is closed before calling close_return_path_on_source()
> and the shutdown fails, leaving the source and destination waiting for
> an event to occur.
> 
> Close the file after calling close_return_path_on_source() so that the
> shutdown succeeds and the return-path thread exits.
> 
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
> 
>  This is an RFC because the correct fix implies reworking the QEMUFile
>  construct, built on top of the QEMU I/O channel.
> 
>  migration/migration.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 5f55af3d7624750ca416c4177781241b3e291e5d..de329f2c553288935d824748286e79e535929b8b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1313,6 +1313,8 @@ void migrate_set_state(int *state, int old_state, int new_state)
>  
>  static void migrate_fd_cleanup(MigrationState *s)
>  {
> +    QEMUFile *tmp = NULL;
> +
>      g_free(s->hostname);
>      s->hostname = NULL;
>      json_writer_free(s->vmdesc);
> @@ -1321,8 +1323,6 @@ static void migrate_fd_cleanup(MigrationState *s)
>      qemu_savevm_state_cleanup();
>  
>      if (s->to_dst_file) {
> -        QEMUFile *tmp;
> -
>          trace_migrate_fd_cleanup();
>          bql_unlock();
>          if (s->migration_thread_running) {
> @@ -1341,15 +1341,14 @@ static void migrate_fd_cleanup(MigrationState *s)
>           * critical section won't block for long.
>           */
>          migration_ioc_unregister_yank_from_file(tmp);
> -        qemu_fclose(tmp);
>      }
>  
> -    /*
> -     * We already cleaned up to_dst_file, so errors from the return
> -     * path might be due to that, ignore them.
> -     */
>      close_return_path_on_source(s);
>  
> +    if (tmp) {
> +        qemu_fclose(tmp);
> +    }
> +
>      assert(!migration_is_active(s));
>  
>      if (s->state == MIGRATION_STATUS_CANCELLING) {

I think this is okay to me for a short term plan.  I'll see how others
think, also add Dan into the loop.

If so, would you please add a rich comment explaining why tmp needs to be
closed later?  Especially, explicit comment on the ordering requirement
would be helpful: IMHO here it's an order that qemu_fclose() must happen
after close_return_path_on_source().  So when others work on this code we
don't easily break it without noticing.

Also please feel free to post separately on migration patches if you'd like
us to merge the patches when repost.

Thanks,

-- 
Peter Xu

Hello Peter

On 2/8/24 06:57, Peter Xu wrote:
> On Wed, Feb 07, 2024 at 02:33:47PM +0100, Cédric Le Goater wrote:
>> In case of error, close_return_path_on_source() can perform a shutdown
>> to exit the return-path thread.  However, in migrate_fd_cleanup(),
>> 'to_dst_file' is closed before calling close_return_path_on_source()
>> and the shutdown fails, leaving the source and destination waiting for
>> an event to occur.
>>
>> Close the file after calling close_return_path_on_source() so that the
>> shutdown succeeds and the return-path thread exits.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>
>>   This is an RFC because the correct fix implies reworking the QEMUFile
>>   construct, built on top of the QEMU I/O channel.
>>
>>   migration/migration.c | 13 ++++++-------
>>   1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 5f55af3d7624750ca416c4177781241b3e291e5d..de329f2c553288935d824748286e79e535929b8b 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1313,6 +1313,8 @@ void migrate_set_state(int *state, int old_state, int new_state)
>>   
>>   static void migrate_fd_cleanup(MigrationState *s)
>>   {
>> +    QEMUFile *tmp = NULL;
>> +
>>       g_free(s->hostname);
>>       s->hostname = NULL;
>>       json_writer_free(s->vmdesc);
>> @@ -1321,8 +1323,6 @@ static void migrate_fd_cleanup(MigrationState *s)
>>       qemu_savevm_state_cleanup();
>>   
>>       if (s->to_dst_file) {
>> -        QEMUFile *tmp;
>> -
>>           trace_migrate_fd_cleanup();
>>           bql_unlock();
>>           if (s->migration_thread_running) {
>> @@ -1341,15 +1341,14 @@ static void migrate_fd_cleanup(MigrationState *s)
>>            * critical section won't block for long.
>>            */
>>           migration_ioc_unregister_yank_from_file(tmp);
>> -        qemu_fclose(tmp);
>>       }
>>   
>> -    /*
>> -     * We already cleaned up to_dst_file, so errors from the return
>> -     * path might be due to that, ignore them.
>> -     */
>>       close_return_path_on_source(s);
>>   
>> +    if (tmp) {
>> +        qemu_fclose(tmp);
>> +    }
>> +
>>       assert(!migration_is_active(s));
>>   
>>       if (s->state == MIGRATION_STATUS_CANCELLING) {
> 
> I think this is okay to me for a short term plan.  I'll see how others
> think, also add Dan into the loop.
> 
> If so, would you please add a rich comment explaining why tmp needs to be
> closed later?  Especially, explicit comment on the ordering requirement
> would be helpful: IMHO here it's an order that qemu_fclose() must happen
> after close_return_path_on_source().  So when others work on this code we
> don't easily break it without noticing.

Sure. I will when we have clarified with Fabiano what is the best
approach.

> Also please feel free to post separately on migration patches if you'd like
> us to merge the patches when repost.

This series is a collection of multiple (related) changes :

* extra Error** parameter to save_setup() migration handlers.
   This change has consequences on the various callers which are not
   fully analyzed.
* similar changes for memory logging handlers. These looks more self
   contained and I will see if I can send then separately.
* return-path thread termination

and then, in background we have open questions regarding :

* the QEMUfile implementation and its QIOChannel usage for migration
   streams
* qemu_file_set_error* vs. migrate_set_error. It is confusing, at least
   for me. Do we have some documentation on best practices ?

Thanks,

C.

On Mon, Feb 12, 2024 at 05:04:28PM +0100, Cédric Le Goater wrote:
> and then, in background we have open questions regarding :
> 
> * the QEMUfile implementation and its QIOChannel usage for migration
>   streams
> * qemu_file_set_error* vs. migrate_set_error. It is confusing, at least
>   for me. Do we have some documentation on best practices ?

Right it is confusing..  It can all boil down to the acient qemufile api
that Fabiano also mentioned in the other reply.  IMHO ideally iochannel
errors should be reported through the stack (rather than kept within the
object) from the channel's API and stored with migrate_set_error() if
necessary, and the channel itself may not need to maintain its own errors.
Right now it's needed because many qemufile APIs do not return errors.

Thanks,

-- 
Peter Xu