Re: [RFC PATCH 14/14] migration: Fix return-path thread exit

[Fabiano Rosas <farosas@suse.de>]

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.