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

["Cédric Le Goater" <clg@redhat.com>]

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.