Re: [PATCH v2 6/7] migration: Fix incoming channel race with multifd and preempt_pre_7_2

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> PROBLEM
> =======
>
> Migration reliably hangs with old machine types (7.2-) when these migration
> capabilities are enabled: multifd, postcopy-ram, postcopy-preempt.
>
> Simplest reproducer :
>
>   $ QTEST_QEMU_MACHINE_TYPE=pc-q35-7.2 \
>       QTEST_QEMU_BINARY=./qemu-system-x86_64 \
>       ./tests/qtest/migration-test --full \
>       -r /x86_64/migration/multifd+postcopy/preempt/plain
>
> Current workaround is to kill destination QEMU, cancel the migration on
> source (with/without yank), disable one or more of the features, and retry.
>
> The hang is caused by receiving preempt channel before most of the multifd
> channels, the peek buffer operation will stuck forever.
>
> Incoming side QEMU hang (with relevant tracepoints enabled):
>
>   migrate_set_state new state setup
>   migration_socket_incoming_accepted
>   migration_set_incoming_channel ioc=0x557d82c1caa0 ioctype=qio-channel-socket
>   migration_socket_incoming_accepted
>   migration_set_incoming_channel ioc=0x557d83023840 ioctype=qio-channel-socket
>   multifd_recv_new_channel channel 1
>   migration_socket_incoming_accepted
>   migration_set_incoming_channel ioc=0x557d830c08c0 ioctype=qio-channel-socket
>   multifd_recv_new_channel channel 0
>   multifd_recv_thread_start 1
>   migration_socket_incoming_accepted
>   migration_set_incoming_channel ioc=0x557d8284a010 ioctype=qio-channel-socket
>   multifd_recv_thread_start 0
>   multifd_recv_unfill channel 0 packet_num 0 flags 0x1 next packet size 0
>   multifd_recv_unfill channel 1 packet_num 1 flags 0x1 next packet size 0
>   [dest qemu hangs here]
>
> Incoming side backtrace:
>
>   __syscall_cancel_arch
>   __internal_syscall_cancel
>   __syscall_cancel
>   recvmsg
>   qio_channel_socket_readv
>   qio_channel_readv_full
>   migration_channel_read_peek
>   migration_channel_identify
>   migration_channel_process_incoming
>   socket_accept_incoming_migration
>   qio_net_listener_channel_func
>   qio_channel_fd_source_dispatch
>   g_main_context_dispatch_unlocked.lto_priv
>   g_main_context_dispatch
>   glib_pollfds_poll
>   os_host_main_loop_wait
>   main_loop_wait
>   qemu_main_loop
>   qemu_default_main
>   main
>
> This patch tries to fix it, from incoming side first.  This patch alone
> should enable single-direction migration to this new QEMU.  The next patch
> will propose a fix for the outgoing path, fixing new->old migrations.
>
> BACKGROUND
> ==========
>
> preempt_pre_7_2 used to be a flag marking QEMU releases 7.1/7.2 only
> because they have a flaw on channel establishment on preempt channel.
>
> Now, after we have multifd + postcopy support, new QEMU binaries can
> migrate old machine types with both features enabled.  Juraj reported
> constant migration hang at the very beginning for those old machine types
> in our downstream systems with such setup.  Juraj found it's because of
> this flag.
>
> QEMU source initiates channels in this order on current master when both
> features enabled with postcopy-preempt mode (machine type 7.2, implies
> preempt_pre_7_2=on):
>
>   - First, main channel connect() happens,
>   - Second, postcopy_preempt_setup() invokes connect() for preempt channel,
>   - Last, in migration_thread(), multifd_send_setup() invokes connect()s
>     for all the multifd channels
>
> However on an old QEMU, it expect the channels arrive in order of
> "main"->"multifd"->"preempt".  Even on a new QEMU, the destination expects
> the same order of arrival on the channels when the channels are not
> peekable.
>
> For peekable channels (non-tls sockets), this time we'll likely see preempt
> channel appearing very soon, peeking at it causing main thread hang, like
> mentioned in the PROBLEM section above.
>
> SOLUTION
> ========
>
> One major problem of current hang is we try to peek at preempt channel,
> which offers nothing at the beginning.
>
> Since we'll need to accept stream from old binaries (QEMU 10.1 to 11.1),
> arrival of preempt channel early is unavoidable.
>
> What we can do, though, is to allow concurrent monitoring of all channels
> rather than one.  Meanwhile, we shouldn't peek at anything that has no
> data.
>
> migration_channel_read_peek() almost does the wait, but it's too late.
> This patch proposes to watch all channels from the start without reading at
> them at all, relying on an IO watch attached.  This will make sure (1) for
> preempt channel, we never read on it (watch can be gracefully disarmed when
> we know it's preempt channel later) (2) since we have one IO watch for each
> channel, then we can allow accept() to happen in almost any order, process
> only when there's some header to read.
>
> To maintain these watches, a new GArray @channels_early is introduced, with
> each element to be a (ioc, GSource) tuple (@MigEarlyIncomingChannel), so
> that QEMU can manage all the IO watches later with it.  Here I used GSource
> rather than io_tag because the GSource may or may not be attached to the
> default main thread gcontext: when OOB is used, it may be the iothread
> context instead.  IOW, g_source_remove() on io_tags stops working, which
> only works for default main context.
>
> The trick this patch used is when we have the ability to wait on all
> channel headers at the same time, we pick one by one from main and multifd
> channels (since both of the types will contain headers dumped right at the
> very beginning), leave preempt channel alone.  It means, after dest QEMU
> finds all main and multifd channels, there are three possibilities:
>
>   - If preempt not enabled, we finished full channel establishment, done.
>
>   - If preempt enabled,
>
>     - If we have one registered IO watch with the IOC, it must be the
>       preempt channel,
>
>     - If we have no further IO watch, wait for the next accept(), which
>       must be the preempt channel.
>
> This will make channel establish work on incoming side even with the old
> QEMU binaries.
>
> While working on it, I managed to refactor the peeking logic quite a bit:
> we used to have a very convoluted migration_channel_identify() function,
> taking care of two different cases:
>
> (1) when the channel is peekable (like normal socket channels),
> (2) when the channel is not peekable (like TLS or file migrations).
>
> This patch refactored that part to make it crystal clear on how we make
> decisions on which channel is which, and split the peek/identify paths.
>
> One example incoming migration from pc-q35-7.2 after this patch applied,
> and with relevant tracepoints set (8 multifd channels, 1 preempt channel):
>
>   migrate_set_state new state setup
>   migration_channel_process_incoming ioc=0x557dc8af4e20 ioctype=qio-channel-socket
>   migration_incoming_channel_watch ioc=0x557dc8af4e20 ioctype=qio-channel-socket
>   migration_channel_process_incoming ioc=0x557dc8abc5d0 ioctype=qio-channel-socket
>   migration_incoming_channel_watch ioc=0x557dc8abc5d0 ioctype=qio-channel-socket
>   migration_channel_process_incoming ioc=0x557dc953d680 ioctype=qio-channel-socket
>   migration_incoming_channel_watch ioc=0x557dc953d680 ioctype=qio-channel-socket
>   migration_set_incoming_channel ioc=0x557dc8abc5d0 ioctype=qio-channel-socket type=MULTIFD
>   migration_channel_process_incoming ioc=0x557dc898a820 ioctype=qio-channel-socket
>   migration_incoming_channel_watch ioc=0x557dc898a820 ioctype=qio-channel-socket
>   migration_set_incoming_channel ioc=0x557dc953d680 ioctype=qio-channel-socket type=MULTIFD
>   migration_channel_process_incoming ioc=0x557dc919bd70 ioctype=qio-channel-socket
>   migration_incoming_channel_watch ioc=0x557dc919bd70 ioctype=qio-channel-socket
>   migration_set_incoming_channel ioc=0x557dc898a820 ioctype=qio-channel-socket type=MULTIFD
>   migration_channel_process_incoming ioc=0x557dc8734b20 ioctype=qio-channel-socket
>   migration_incoming_channel_watch ioc=0x557dc8734b20 ioctype=qio-channel-socket
>   migration_set_incoming_channel ioc=0x557dc919bd70 ioctype=qio-channel-socket type=MULTIFD
>   migration_channel_process_incoming ioc=0x557dc898bdf0 ioctype=qio-channel-socket
>   migration_incoming_channel_watch ioc=0x557dc898bdf0 ioctype=qio-channel-socket
>   migration_set_incoming_channel ioc=0x557dc8734b20 ioctype=qio-channel-socket type=MULTIFD
>   migration_channel_process_incoming ioc=0x557dc968cfd0 ioctype=qio-channel-socket
>   migration_incoming_channel_watch ioc=0x557dc968cfd0 ioctype=qio-channel-socket
>   migration_set_incoming_channel ioc=0x557dc898bdf0 ioctype=qio-channel-socket type=MULTIFD
>   migration_channel_process_incoming ioc=0x557dc8c31810 ioctype=qio-channel-socket
>   migration_incoming_channel_watch ioc=0x557dc8c31810 ioctype=qio-channel-socket
>   migration_set_incoming_channel ioc=0x557dc968cfd0 ioctype=qio-channel-socket type=MULTIFD
>   migration_set_incoming_channel ioc=0x557dc8c31810 ioctype=qio-channel-socket type=MULTIFD
>   migration_set_incoming_channel ioc=0x557dc8af4e20 ioctype=qio-channel-socket type=MAIN
>   migration_start_incoming
>   migrate_set_state new state active
>   migration_channel_process_incoming ioc=0x557dc8966160 ioctype=qio-channel-socket
>   migration_set_incoming_channel ioc=0x557dc8966160 ioctype=qio-channel-socket type=PREEMPT
>
> While at it, I renamed the four channel incoming/outgoing tracepoints, make
> all to follow migration_[incoming|outgoing]_channel_*().  Especially for
> incoming, then we can enable migration_incoming_channel* for incoming
> channel debuggings.
>
> FINAL NOTE ON TLS
> =================
>
> Note that this solution isn't a magic bullet fixing everything.  TLS is not
> fixable with this approach because TLS isn't peekable channel, hence the
> watch won't help.
>
> We can have other ways to hack it to make it work, e.g. we can make TLS
> channels peekable too, but I don't want to pollute the TLS iochannel only
> for this: let's hope handshake will arrive soon so we can drop these code.
>
> For now, when TLS is involved with such old machine type (7.2-), one needs
> to migrate it out with any combination of features except enabling all
> four (multifd + TLS + postcopy + preempt) to a patched binary, after that,
> all combinations will start working.
>
> BugLink: https://redhat.atlassian.net/browse/RHEL-254933
> Fixes: e27418861288 ("migration: enable multifd and postcopy together")
> Reported-by: Juraj Marcin <jmarcin@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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