Peter Xu <pet...@redhat.com> writes:
> 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-sta...@nongnu.org
>> Reported-by: Jim Fehlig <jfeh...@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")
Yep, thanks. I'll fix it up.
>
>> Signed-off-by: Fabiano Rosas <faro...@suse.de>
>
> Reviewed-by: Peter Xu <pet...@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.
That one is pretty ready. From my side I don't intend to change anything
else, save for review comments. And it's definitely 9.2 material.
I think CPR is more important at this point because it's been lagging
behind for a while.
I have a PR to send with these fixes and catch up on that virtio-net
discussion. After that I should be able to get some reviews done.
>
> Thanks,
