Re: [PATCH 2/2] migration/multifd: Fix multifd_send_setup cleanup when channel creation fails

[Peter Xu]

On Thu, Aug 01, 2024 at 02:41:01PM -0300, Fabiano Rosas wrote:
> When a channel fails to create, the code currently just returns. This
> is wrong for two reasons:
> 
> 1) Channel n+1 will not get to initialize it's semaphores, leading to
>    an assert when terminate_threads tries to post to it:
> 
>  qemu-system-x86_64: ../util/qemu-thread-posix.c:92:
>  qemu_mutex_lock_impl: Assertion `mutex->initialized' failed.
> 
> 2) (theoretical) If channel n-1 already started creation it will
>    defeat the purpose of the channels_created logic which is in place
>    to avoid migrate_fd_cleanup() to run while channels are still being
>    created.
> 
>    This cannot really happen today because the current failure cases
>    for multifd_new_send_channel_create() are all synchronous,
>    resulting from qio_channel_file_new_path() getting a bad
>    filename. This would hit all channels equally.
> 
>    But I don't want to set a trap for future people, so have all
>    channels try to create (even if failing), and only fail after the
>    channels_created semaphore has been posted.
> 
> While here, remove the error_report_err call. There's one already at
> migrate_fd_cleanup later on.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Jim Fehlig <jfehlig@suse.com>
> Fixes: bd8b0a8f82 ("migration/multifd: Move multifd_send_setup error handling 
> in to the function")

Should it be this one instead?

b7b03eb614 ("migration/multifd: Add outgoing QIOChannelFile support")

> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

PS: what's your plan on your other multifd SendData series?  I got a bit
overloaded on downstream stuff and I still have plenty review debts
recently (CPR one of them.. needs follow ups), so just to say I may delay a
bit on reading that one.  I assume it's next-release stuff anyway, but let
me know otherwise.

Thanks,

-- 
Peter Xu