On Thu, Sep 24, 2026 at 06:54:25PM -0300, Fabiano Rosas wrote:
> Peter Xu <[email protected]> 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:
> >
> > - 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, 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 <[email protected]>
> > Signed-off-by: Peter Xu <[email protected]>
> > ---
> > migration/channel.h | 3 +
> > migration/migration.h | 35 ++++
> > migration/channel.c | 414 +++++++++++++++++++++++++++++++++++------
> > migration/migration.c | 23 ++-
> > migration/trace-events | 7 +-
> > 5 files changed, 421 insertions(+), 61 deletions(-)
>
> Hi Peter, I think the code looks good, but there are some twists and
> turns that I think we can get rid of. I put a fixup patch [1] at the end
> with my suggestions and mentioned inline what I changed and why.
Thanks, especially for covering the tests while suggesting.
I believe I agree with many of them, I'll comment on the rest, see inline.
>
> >
> > diff --git a/migration/channel.h b/migration/channel.h
> > index 678712f291..40bd339a3e 100644
> > --- a/migration/channel.h
> > +++ b/migration/channel.h
> > @@ -49,4 +49,7 @@ bool migration_channel_parse_input(const char *uri,
> > MigrationChannel **main_channelp,
> > MigrationChannel **cpr_channelp,
> > Error **errp);
> > +
> > +void migration_incoming_free_early_channels(MigrationIncomingState *mis);
> > +
> > #endif
> > diff --git a/migration/migration.h b/migration/migration.h
> > index 683bc7bdd5..6f23e1805b 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -90,6 +90,28 @@ typedef enum {
> > PREEMPT_THREAD_QUIT,
> > } PreemptThreadStatus;
> >
> > +typedef struct {
> > + /*
> > + * The GSource we have allocated to track G_IO_IN events for the
> > + * channel. Note: it may not be attached to the default main gcontext,
> > + * for example, when in case of io watch created during an OOB
> > + * migrate-recover command.
> > + */
> > + GSource *source;
> > + /* The channel we will identify later after getting some buffers to
> > read */
> > + QIOChannel *ioc;
> > +} MigEarlyIncomingChannel;
> > +
> > +typedef struct {
> > + GArray *channels;
> > + /*
> > + * In most cases, channels are only operated with BQl, but since we
> > + * have OOB command support, don't assume it, just use a standalone
> > + * mutex to protect any access to the channels array.
> > + */
> > + QemuMutex mutex;
> > +} MigEarlyIncomingChannels;
> > +
> > /* State for the incoming migration */
> > struct MigrationIncomingState {
> > QEMUFile *from_src_file;
> > @@ -255,6 +277,19 @@ struct MigrationIncomingState {
> >
> > /* Do exit on incoming migration failure */
> > bool exit_on_error;
> > +
> > + /*
> > + * Maintains all channels only at early stage. After incoming
> > + * migration started, this should be an empty array and no use anymore.
> > + *
> > + * TODO: we logically only need this to make new QEMU binaries work for
> > + * old machines who have @preempt_pre_7_2 set on the source, but it's
> > + * also good to have this early stage anyway, making sure when peek the
> > + * channel there will be something in the channel. Ideally, we should
> > + * manage all migration channels in the future with proper headers,
> > + * then we can drop this.
> > + */
> > + MigEarlyIncomingChannels channels_early;
> > };
> >
> > MigrationIncomingState *migration_incoming_get_current(void);
> > diff --git a/migration/channel.c b/migration/channel.c
> > index b9065efd03..acb7ef5206 100644
> > --- a/migration/channel.c
> > +++ b/migration/channel.c
> > @@ -12,6 +12,8 @@
> >
> > #include "qemu/osdep.h"
> > #include "qemu/cutils.h"
> > +#include "qemu/main-loop.h"
> > +#include "qemu/error-report.h"
> > #include "channel.h"
> > #include "exec.h"
> > #include "fd.h"
> > @@ -146,61 +148,84 @@ bool migration_has_all_channels(void)
> > return true;
> > }
> >
> > -static MigChannelType migration_channel_identify(MigrationIncomingState
> > *mis,
> > - QIOChannel *ioc, Error
> > **errp)
> > +static bool qio_channel_is_peekable(QIOChannel *ioc)
> > +{
> > + return qio_channel_has_feature(ioc, QIO_CHANNEL_FEATURE_READ_MSG_PEEK);
> > +}
> > +
> > +/*
> > + * With multiple channels, it is possible that we receive channels out of
> > + * order on destination side, causing incorrect mapping of source channels
> > + * on destination side.
> > + *
> > + * When the channel is peekable (e.g. non-TLS socket channels), check
> > + * channel MAGIC to decide type of channel.
> > + *
> > + * Please note this is best effort, postcopy preempt channel does not send
> > + * any magic number so avoid it for postcopy live migration.
> > + *
> > + * Returns: >0 if success, ==0 (CH_NONE) if error. If error happened,
> > + * *errp must be set.
> > + */
> > +static MigChannelType migration_channel_peek(MigrationIncomingState *mis,
> > + QIOChannel *ioc,
> > + Error **errp)
> > {
> > MigChannelType channel = CH_NONE;
> > uint32_t channel_magic = 0;
> > int ret = 0;
> >
> > - if (!migration_has_main_and_multifd_channels()) {
> > - if (qio_channel_has_feature(ioc,
> > QIO_CHANNEL_FEATURE_READ_MSG_PEEK)) {
> > - /*
> > - * With multiple channels, it is possible that we receive
> > channels
> > - * out of order on destination side, causing incorrect mapping
> > of
> > - * source channels on destination side. Check channel MAGIC to
> > - * decide type of channel. Please note this is best effort,
> > - * postcopy preempt channel does not send any magic number so
> > - * avoid it for postcopy live migration. Also tls live
> > migration
> > - * already does tls handshake while initializing main channel
> > so
> > - * with tls this issue is not possible.
> > - */
> > - ret = migration_channel_read_peek(ioc, (void *)&channel_magic,
> > - sizeof(channel_magic), errp);
> > - if (ret != 0) {
> > - goto out;
> > - }
> > + assert(qio_channel_is_peekable(ioc));
> >
> > - channel_magic = be32_to_cpu(channel_magic);
> > - if (channel_magic == QEMU_VM_FILE_MAGIC) {
> > - channel = CH_MAIN;
> > - } else if (channel_magic == MULTIFD_MAGIC) {
> > - assert(migrate_multifd());
> > - channel = CH_MULTIFD;
> > - } else if (!mis->from_src_file &&
> > - mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> > - /* reconnect main channel for postcopy recovery */
> > - channel = CH_MAIN;
> > - } else {
> > - error_setg(errp, "unknown channel magic: %u",
> > channel_magic);
> > - }
> > - } else if (mis->from_src_file && migrate_multifd()) {
> > - /*
> > - * Non-peekable channels like tls/file are processed as
> > - * multifd channels when multifd is enabled.
> > - */
> > - channel = CH_MULTIFD;
> > - } else if (!mis->from_src_file) {
> > - channel = CH_MAIN;
> > - } else {
> > - error_setg(errp, "non-peekable channel used without multifd");
> > - }
> > + ret = migration_channel_read_peek(ioc, (void *)&channel_magic,
> > + sizeof(channel_magic), errp);
> > + if (ret != 0) {
> > + /* Failed */
> > + return channel;
> > + }
> > +
> > + channel_magic = be32_to_cpu(channel_magic);
> > +
> > + if (channel_magic == QEMU_VM_FILE_MAGIC) {
> > + channel = CH_MAIN;
> > + } else if (channel_magic == MULTIFD_MAGIC) {
> > + assert(migrate_multifd());
> > + channel = CH_MULTIFD;
> > + } else if (!mis->from_src_file &&
> > + mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> > + /* reconnect main channel for postcopy recovery */
> > + channel = CH_MAIN;
>
> This is not peeking, maybe it should be somewhere else. Not this patch's
> fault, we can think about it some other day.
If we separate "peek" from "identify", this one still falls into "peek".
Per my definition, the latter "identify" only does "pick channels by order
they arrive". Then this doesn't belong to that. As it needs to "peek for
at least something readable", rather than blindly rely on the order of
arrival.
But yeah, it'll be good we leave this for later regardless.
>
> > } else {
> > + error_setg(errp, "Unknown channel magic: %u", channel_magic);
> > + }
> > +
> > + return channel;
> > +}
> > +
> > +/*
> > + * Returns: >0 if success, ==0 (CH_NONE) if error. If error happened,
> > + * *errp must be set.
> > + */
> > +static MigChannelType migration_channel_identify(MigrationIncomingState
> > *mis,
> > + QIOChannel *ioc, Error
> > **errp)
> > +{
> > + MigChannelType channel = CH_NONE;
> > +
> > + if (migration_has_main_and_multifd_channels()) {
> > assert(migrate_postcopy_preempt());
> > channel = CH_POSTCOPY;
>
> This check is the same as the one at
> migration_channel_process_incoming(). I think we can simplify this.
True, this path should be dead code when original patch applied. I have a
slight preference on keeping the other one instead (which I know you tend
to think "awkward"). I'll explain below, and we'll see.
>
> See [1].
>
> > + } else if (!mis->from_src_file) {
> > + channel = CH_MAIN;
> > + } else if (migrate_multifd()) {
> > + /*
> > + * Non-peekable channels like tls/file are processed as
> > + * multifd channels when multifd is enabled.
> > + */
> > + channel = CH_MULTIFD;
> > + } else {
> > + error_setg(errp, "Unexpected non-peekable channel observed");
> > }
> >
> > -out:
> > return channel;
> > }
> >
> > @@ -214,31 +239,275 @@ static void
> > migration_incoming_error_propagate(MigrationIncomingState *mis,
> > }
> > }
> >
> > +/* Must be with mis->channels_early.mutex held */
> > +static void
> > +migration_incoming_early_channel_insert(MigrationIncomingState *mis,
> > + QIOChannel *ioc,
> > + GSource *source)
> > +{
> > + MigEarlyIncomingChannel chan = {
> > + .source = source,
> > + .ioc = ioc,
> > + };
> > +
> > + object_ref(OBJECT(ioc));
> > + g_source_ref(source);
> > +
> > + g_array_append_val(mis->channels_early.channels, chan);
> > +}
> > +
> > +static void migration_incoming_early_channel_free(GArray *channels, int i)
> > +{
> > + MigEarlyIncomingChannel *chan;
> > +
> > + chan = &g_array_index(channels, MigEarlyIncomingChannel, i);
> > +
> > + g_source_destroy(chan->source);
> > + g_source_unref(chan->source);
> > + object_unref(OBJECT(chan->ioc));
> > +
> > + g_array_remove_index_fast(channels, i);
> > +}
> > +
> > +/*
> > + * Remove this channel from the monitoring of @channels_early array.
> > + * Return true if found and successful, false otherwise.
> > + */
> > +static bool
> > +migration_incoming_early_channel_remove(MigrationIncomingState *mis,
> > + QIOChannel *ioc)
> > +{
> > + GArray *channels = mis->channels_early.channels;
> > + MigEarlyIncomingChannel *chan;
> > + int i;
> > +
> > + QEMU_LOCK_GUARD(&mis->channels_early.mutex);
> > +
> > + for (i = 0; i < channels->len; i++) {
> > + chan = &g_array_index(channels, MigEarlyIncomingChannel, i);
> > + if (chan->ioc != ioc) {
> > + continue;
> > + }
> > + migration_incoming_early_channel_free(channels, i);
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > +void migration_incoming_free_early_channels(MigrationIncomingState *mis)
> > +{
> > + GArray *channels = mis->channels_early.channels;
> > +
> > + QEMU_LOCK_GUARD(&mis->channels_early.mutex);
> > +
> > + while (channels->len) {
> > + migration_incoming_early_channel_free(channels, 0);
> > + }
> > +}
> > +
> > +static bool migration_incoming_setup_channel(QIOChannel *ioc,
> > + MigChannelType ch,
> > + Error **errp)
> > +{
> > + trace_migration_incoming_channel_set(ioc,
> > + object_get_typename(OBJECT(ioc)),
> > + mig_channel_str[ch]);
> > + migration_ioc_register_yank(ioc);
> > + /* TODO: make this return the success status instead */
> > + migration_incoming_setup(ioc, ch, errp);
>
> I implemented this TODO. It allows us to skip the main+mfd check after
> starting migration because migration_incoming_setup() already returns
> that value. See [1].
Actually now looking at this patch, this TODO is so trivial.. yes I'll do
it.
But when I do it, I think it too error prone to return a value returned
from migration_has_main_and_multifd_channels() for this function. I
changed it to simple true/false for success or not instead. I'll attach a
2nd version of the fixup patch at the end, applying most changes you
suggested but stick with returning true/false for this one.
>
> > +
> > + return *errp == NULL;
> > +}
> > +
> > static bool migration_incoming_channel_install(MigrationIncomingState *mis,
> > QIOChannel *ioc,
> > Error **errp)
> > {
> > - MigChannelType ch = migration_channel_identify(mis, ioc, errp);
> > + MigChannelType ch;
> > + bool ret;
> > +
> > + if (qio_channel_is_peekable(ioc)) {
>
> This check is redundant, we can only arrive here via the watch and we
> checked peekable already at migration_channel_process_incoming. See [1].
Shouldn't? Because the file path uses migration_incoming_channel_install()
too, afaict.
But I see how your question came, which I'm guessing: I used this function
in a peekable only path, so here what I can do minimum is I can move the
migration_channel_peek() to the peekable path, then move the other path of
migration_channel_identify() to the e.g. file path. I'll do that.
>
> > + ch = migration_channel_peek(mis, ioc, errp);
> > + } else {
> > + ch = migration_channel_identify(mis, ioc, errp);
> > + }
> >
> > if (!ch) {
> > - assert(*errp);
> > return false;
> > }
> >
> > - trace_migration_set_incoming_channel(ioc,
> > - object_get_typename(OBJECT(ioc)),
> > - mig_channel_str[ch]);
> > - migration_ioc_register_yank(ioc);
> > + ret = migration_incoming_setup_channel(ioc, ch, errp);
> > + if (!ret) {
> > + return false;
> > + }
> >
> > - if (migration_incoming_setup(ioc, ch, errp)) {
> > + /* Installation succeeded, kickoff migration if needed */
> > + if (migration_has_main_and_multifd_channels()) {
> > migration_start_incoming();
> > }
> >
> > - if (*errp) {
> > - return false;
> > + return true;
> > +}
> > +
> > +static void
> > +migration_incoming_channel_preempt_setup(QIOChannel *ioc)
> > +{
> > + assert(migrate_postcopy_preempt());
> > + /* Installation of preempt channel should never fail */
> > + migration_incoming_setup_channel(ioc, CH_POSTCOPY, &error_abort);
> > +}
> > +
> > +static void
> > +migration_incoming_detect_preempt_channel(MigrationIncomingState *mis)
> > +{
> > + GArray *channels = mis->channels_early.channels;
> > + MigEarlyIncomingChannel *chan;
> > +
> > + QEMU_LOCK_GUARD(&mis->channels_early.mutex);
> > +
> > + /* When preempt mode not enabled, nothing to detect.. */
> > + if (!migrate_postcopy_preempt()) {
> > + /*
> > + * .. but if we found something pending, throw an error only, which
> > + * should not happen. Even if it happens, resources will still be
> > + * released after incoming migration is completedly.
> > + */
> > + if (channels->len) {
> > + error_report("%s: Found %u unused channels",
> > + __func__, channels->len);
> > + }
> > + return;
> > }
> >
> > - return true;
> > + /* Preempt channel hasn't yet arrived? Process it later */
> > + if (!channels->len) {
> > + return;
> > + }
> > +
> > + /*
> > + * More than one channel should never happen.. capture it in case if
> > + * it happens, then there's not much we can do.
> > + */
> > + if (channels->len > 1) {
> > + error_report("%s: Found %u unused channels, "
> > + "can't identify preempt channel",
> > + __func__, channels->len);
> > + return;
> > + }
> > +
> > + assert(channels->len == 1);
> > + /* This is the preempt channel, install it directly */
> > + chan = &g_array_index(channels, MigEarlyIncomingChannel, 0);
> > + migration_incoming_channel_preempt_setup(chan->ioc);
> > + migration_incoming_early_channel_free(channels, 0);
> > +}
> > +
> > +static gboolean migration_incoming_channel_readable(QIOChannel *ioc,
> > + GIOCondition condition,
> > + gpointer opaque)
> > +{
> > + MigrationIncomingState *mis = opaque;
> > + Error *local_err = NULL;
> > +
> > + /*
> > + * No need to monitor this channel anymore as long as anything arrived,
> > + * remove it from tracking.
> > + *
> > + * NOTE: this means if partial data arrived we may still block here,
> > + * but it shouldn't happen in production, only malicious stream. Since
> > + * migration stream is trusted (either due to trusted network, or TLS),
> > + * that's non-issue.
>
> Could we check the amount of data and keep waiting?
We could, but it is about the security side of things, I'd not bother in
this patch, and maybe we don't bother forever, hopping that at the end of
this year we enable handshake in next release, then this is a ioc read()
block reading, as simple as that.
>
> > + *
> > + * NOTE2: this will also release the ioc ref that we used to hold, but
> > + * it's fine since we have at least one more refcount in the current
> > + * event handler.
>
> I don't understand "one more ref in the event handler". Looks like
> you're trying to say the refcount is consistent, but why even mention
> it? I may be missing something.
Not about consistency, it's about holding at least one refcount to make sure
migration_incoming_early_channel_remove() won't already free the IOC.
But I think maybe you're right: likely nobody will notice the context of
this path processing the gsource, and maybe this sentence brings more
confusion than clarity? Let me know, I can drop this line.
>
> > + *
> > + * NOTE3: it's theoretically possible that this entry is gone reaching
> > + * here. Example: the main thread is doing incoming cleanup having this
> > + * one removed, while this watch can be registered on the monitor
> > + * iothread's context and fired at the exact same time but in the
> > + * iothread instead. If it happens, skip the rest. I'm not sure if
> > + * this could happen at all, may depend on iothread lifespan management
> > + * in the main thread, but be prepared.
>
> I wonder if we should extend the locking a bit further. Not because of
> what you mentioned here^, but because everything the code running on the
> monitor thread touches can race with changes from the main loop. All of
> these migrate_foo_bar() helpers, the QEMUFiles, caps, params, etc.
Is that about the new lock (mis->channels_early.mutex) or existing code?
Possible to do on top, or a new concern introduced from this patch?
>
> > + */
> > + if (!migration_incoming_early_channel_remove(mis, ioc)) {
> > + goto out;
> > + }
> > +
>
> I moved the peek into here and passed the already identified
> channel into migration_incoming_channel_install(). See [1].
Agree on this change, better.
>
> > + if (!migration_incoming_channel_install(mis, ioc, &local_err)) {
> > + goto out;
> > + }
> > +
> > + if (migration_has_main_and_multifd_channels()) {
>
> This check became redundant because the _setup routine already returns
> it. See [1].
I didn't adopt this in my new fixup, because I want to avoid returning that
info deep from stack. Let's continue the discussion in my fixup there as a
reply.
>
> > + /*
> > + * Possibilities when reaching here:
> > + *
> > + * (1) if preempt not enabled, this should be no-op, all done,
> > + * (2) if preempt enabled,
> > + * (2.a) preempt channel arrived @channels_early, handle it now
> > + * (2.b) preempt channel not arrived, to be handled in
> > + * migration_channel_process_incoming() later
> > + */
> > + migration_incoming_detect_preempt_channel(mis);
>
> Moved this into migration_incoming_channel_install(). See [1].
Since my version used migration_incoming_channel_install() for both
peekable and non-peekable, this path is not needed for non-peekable.
I still left it here, assuming it's clearer it's only needed for peekable.
But you can reply with your thought. I hope at least we can be more
focused with most of the rest addressed.
>
> > + }
> > +
> > +out:
> > + if (local_err) {
> > + migration_incoming_error_propagate(mis, local_err);
> > + }
> > +
> > + /*
> > + * NOTE: we should have already detached the GSource, returning
> > + * G_SOURCE_REMOVE to be logically consistent only.
> > + */
> > + return G_SOURCE_REMOVE;
> > +}
> > +
> > +static void migration_incoming_channel_watch(MigrationIncomingState *mis,
> > + QIOChannel *ioc)
> > +{
> > + GMainContext *context = g_main_context_get_thread_default();
> > + GSource *source;
> > + guint io_tag;
> > +
> > + trace_migration_incoming_channel_watch(ioc,
> > +
> > object_get_typename(OBJECT(ioc)));
> > +
> > + /*
> > + * Careful: this can be run from either the main thread or the monitor
> > + * iothread when migrate_recover is used with OOB=on, so we need to
> > + * take the lock and use the full version to specify the correct
> > + * context.
> > + */
> > + QEMU_LOCK_GUARD(&mis->channels_early.mutex);
> > +
> > + /*
> > + * We should never watch an @ioc that is not peekable, because there's
> > + * no point. What is worse is we lose the real order of accept()s via
> > + * the asynchronous IO watch operation.
> > + *
> > + * Another note is TLS channel (non-peekable) may or may not work
> > + * properly with IO watch due to its current .io_create_watch() impl,
> > + * which is another story. Just guard both points.
> > + */
> > + assert(qio_channel_is_peekable(ioc));
> > + io_tag = qio_channel_add_watch_full(ioc, G_IO_IN,
> > +
> > migration_incoming_channel_readable,
> > + mis, NULL, context);
> > +
> > + /*
> > + * Replace this if one day qio_channel_add_watch*() API can directly
> > + * return the GSource*.. for now, stick with it.
> > + */
> > + source = g_main_context_find_source_by_id(context, io_tag);
> > + /*
> > + * Nothing can race with adding the IO watch, aka, concurrent removal
> > + * is not possible when we have the lock. So it must be present.
> > + */
> > + assert(source);
> > +
> > + migration_incoming_early_channel_insert(mis, ioc, source);
> > }
> >
> > /**
> > @@ -254,13 +523,44 @@ void migration_channel_process_incoming(QIOChannel
> > *ioc)
> > MigrationIncomingState *mis = migration_incoming_get_current();
> > Error *local_err = NULL;
> >
> > - trace_migration_channel_process_incoming(
> > + trace_migration_incoming_channel_process(
> > ioc, object_get_typename(OBJECT(ioc)));
> >
> > if (migrate_channel_requires_tls_upgrade(ioc)) {
> > migration_tls_channel_process_incoming(ioc, &local_err);
> > } else {
> > - migration_incoming_channel_install(mis, ioc, &local_err);
> > + if (migration_has_main_and_multifd_channels()) {
>
> The fact that this check must come before everything is a major
> weirdness. In fact, this is conceptually the same case as the first
> branch in migration_channel_identify(). I therefore moved that function
> to this point and added a comment explaining why this must come
> first. See [1].
This is one part that I left alone so far. My reason is that preempt
channel needs special care at least for peekable, it's because it's not
peekable there when hitting this case. So we need to have this separate
path. This is also relevant to below, where..
>
> > + /*
> > + * If all main+multifd channels are present already, this must
> > + * be the preempt channel.
> > + *
> > + * QEMU can't register an IO watch for it if there is only the
> > + * last preempt channel left, because it means the IO watch
> > + * will never fire and nobody will pick it up: we rely on the
> > + * one before the last one to pick both.
> > + *
> > + * See comment in migration_incoming_channel_readable() on
> > + * the migration_incoming_detect_preempt_channel() call.
> > + */
> > + migration_incoming_channel_preempt_setup(ioc);
> > + } else {
> > + /*
> > + * Register an IO watch for peekable channels, so that channels
> > + * can be accept()ed with any order.
> > + *
> > + * Non-peekable channels (file, TLS, etc.) cannot register IO
> > + * watch, not only because there's no data to look at to help
> > + * making the decision, but also because after registering we
> > + * will lose the real ordering we get from accept(), which is
> > + * still so far the only source of truth to identify a channel
> > + * in such case.
> > + */
> > + if (qio_channel_is_peekable(ioc)) {
>
> I moved this line into migration_channel_identify(). That way we ensure
> the ordering regarding the main+mfd check. The watch is still installed
> at this point. See [1].
... you suggested moving this into migration_channel_identify().
Personally I think it's against my purpose of split: I wanted to make
migration_channel_identify() to only rely on order of arrival to identify
channels, nothing else. Then for all peekable handling, it should be
inside the _peek() function.
>
> > + migration_incoming_channel_watch(mis, ioc);
> > + } else {
> > + migration_incoming_channel_install(mis, ioc, &local_err);
>
> This invocation of install does not check for the preempt channel after
> it returns. Is this simply assuming that if a non-peekable channel is
> present then there will be no preempt channel at all?
>
> Or if main+TLS is the first channel, then the next channel will be
> preempt and already enter the first branch?
Yes. It's because for non-peekable, preempt channel is only processed
above (in my patch logic, with/without my new fixup to be attached).
For peekable, preempt channel may be picked up by the 2nd last channel,
which is not possible for non-peekable, so that is special way for peekable
channels only.
>
> > + }
> > + }
> > }
> >
> > if (local_err) {
> > @@ -270,7 +570,7 @@ void migration_channel_process_incoming(QIOChannel *ioc)
> >
> > void migration_channel_connect_outgoing(MigrationState *s, QIOChannel *ioc)
> > {
> > - trace_migration_set_outgoing_channel(ioc,
> > object_get_typename(OBJECT(ioc)));
> > + trace_migration_outgoing_channel_set(ioc,
> > object_get_typename(OBJECT(ioc)));
> >
> > if (migrate_channel_requires_tls_upgrade(ioc)) {
> > Error *local_err = NULL;
> > diff --git a/migration/migration.c b/migration/migration.c
> > index cfadcc79c2..4cc4077a76 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -334,6 +334,10 @@ void migration_object_init(void)
> > current_incoming->page_requested = g_tree_new(page_request_addr_cmp);
> >
> > current_incoming->exit_on_error = INMIGRATE_DEFAULT_EXIT_ON_ERROR;
> > + /* zero_terminated=false, clear_=true */
>
> There's an extra _ after 'clear' I think.
See? I know someone else will get confused too. :)
https://docs.gtk.org/glib/type_func.Array.new.html
GArray*
g_array_new (
gboolean zero_terminated,
gboolean clear_,
guint element_size
)
...
clear_
Type: gboolean
If true, GArray elements should be automatically cleared to 0 when they are
allocated.
>
> > + current_incoming->channels_early.channels =
> > + g_array_new(FALSE, TRUE, sizeof(MigEarlyIncomingChannel));
> > + qemu_mutex_init(¤t_incoming->channels_early.mutex);
> >
> > migration_object_check(current_migration, &error_fatal);
> >
> > @@ -447,6 +451,24 @@ void migration_incoming_state_destroy(void)
> > MigrationIncomingState *mis = migration_incoming_get_current();
> > PostcopyState ps = postcopy_state_get();
> >
> > + /* Cleanup listener to make sure no further accept() for sockets */
> > + migration_incoming_transport_cleanup(mis);
> > +
> > + /*
> > + * It's safer to free channel watches earlier than most of the rest, in
> > + * case the IO watches could fire in the monitor iothread concurrently
> > + * against this function.
>
> Now I'm getting worried. Cleanup races are super annoying. This is
> "just" about the recovery command, right?
I think so.
Note: I didn't have any way to trigger any crash at all, this is something
when I had AI review my code and it pointed out this. I thought I should
be safe than sorry.
Below is my fixup. The hope is I addressed most of them but not all, with
reasonings inline above. Let me know.
===8<===
>From eb03b1b648b91a317a8a7559240186c87d835b8c Mon Sep 17 00:00:00 2001
From: Peter Xu <[email protected]>
Date: Mon, 28 Sep 2026 11:35:20 -0400
Subject: [PATCH] fixup! migration: Fix incoming channel race with multifd and
preempt_pre_7_2
Signed-off-by: Peter Xu <[email protected]>
---
migration/channel.c | 42 +++++++++++++++++-------------------------
migration/migration.c | 9 +++------
2 files changed, 20 insertions(+), 31 deletions(-)
diff --git a/migration/channel.c b/migration/channel.c
index acb7ef5206..72de9c9778 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -204,17 +204,14 @@ static MigChannelType
migration_channel_peek(MigrationIncomingState *mis,
/*
* Returns: >0 if success, ==0 (CH_NONE) if error. If error happened,
- * *errp must be set.
+ * *errp must be set. NOTE: preempt channel is done separately.
*/
static MigChannelType migration_channel_identify(MigrationIncomingState *mis,
QIOChannel *ioc, Error **errp)
{
MigChannelType channel = CH_NONE;
- if (migration_has_main_and_multifd_channels()) {
- assert(migrate_postcopy_preempt());
- channel = CH_POSTCOPY;
- } else if (!mis->from_src_file) {
+ if (!mis->from_src_file) {
channel = CH_MAIN;
} else if (migrate_multifd()) {
/*
@@ -314,31 +311,16 @@ static bool migration_incoming_setup_channel(QIOChannel
*ioc,
object_get_typename(OBJECT(ioc)),
mig_channel_str[ch]);
migration_ioc_register_yank(ioc);
- /* TODO: make this return the success status instead */
- migration_incoming_setup(ioc, ch, errp);
- return *errp == NULL;
+ return migration_incoming_setup(ioc, ch, errp);
}
static bool migration_incoming_channel_install(MigrationIncomingState *mis,
QIOChannel *ioc,
+ MigChannelType ch,
Error **errp)
{
- MigChannelType ch;
- bool ret;
-
- if (qio_channel_is_peekable(ioc)) {
- ch = migration_channel_peek(mis, ioc, errp);
- } else {
- ch = migration_channel_identify(mis, ioc, errp);
- }
-
- if (!ch) {
- return false;
- }
-
- ret = migration_incoming_setup_channel(ioc, ch, errp);
- if (!ret) {
+ if (!migration_incoming_setup_channel(ioc, ch, errp)) {
return false;
}
@@ -409,6 +391,7 @@ static gboolean
migration_incoming_channel_readable(QIOChannel *ioc,
{
MigrationIncomingState *mis = opaque;
Error *local_err = NULL;
+ MigChannelType ch;
/*
* No need to monitor this channel anymore as long as anything arrived,
@@ -435,7 +418,12 @@ static gboolean
migration_incoming_channel_readable(QIOChannel *ioc,
goto out;
}
- if (!migration_incoming_channel_install(mis, ioc, &local_err)) {
+ ch = migration_channel_peek(mis, ioc, &local_err);
+ if (!ch) {
+ goto out;
+ }
+
+ if (!migration_incoming_channel_install(mis, ioc, ch, &local_err)) {
goto out;
}
@@ -522,6 +510,7 @@ void migration_channel_process_incoming(QIOChannel *ioc)
{
MigrationIncomingState *mis = migration_incoming_get_current();
Error *local_err = NULL;
+ MigChannelType ch;
trace_migration_incoming_channel_process(
ioc, object_get_typename(OBJECT(ioc)));
@@ -558,7 +547,10 @@ void migration_channel_process_incoming(QIOChannel *ioc)
if (qio_channel_is_peekable(ioc)) {
migration_incoming_channel_watch(mis, ioc);
} else {
- migration_incoming_channel_install(mis, ioc, &local_err);
+ ch = migration_channel_identify(mis, ioc, &local_err);
+ if (ch) {
+ migration_incoming_channel_install(mis, ioc, ch,
&local_err);
+ }
}
}
}
diff --git a/migration/migration.c b/migration/migration.c
index adba154e46..242b8ed812 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -854,10 +854,7 @@ out:
migrate_incoming_unref_outgoing_state();
}
-/*
- * Returns whether all the necessary channels to proceed with the
- * incoming migration have been established without error.
- */
+/* Return true if success, false otherwise (with *errp set) */
bool migration_incoming_setup(QIOChannel *ioc, uint8_t channel, Error **errp)
{
MigrationIncomingState *mis = migration_incoming_get_current();
@@ -886,13 +883,13 @@ bool migration_incoming_setup(QIOChannel *ioc, uint8_t
channel, Error **errp)
assert(!mis->postcopy_qemufile_dst);
f = qemu_file_new_input(ioc);
postcopy_preempt_new_channel(mis, f);
- return false;
+ break;
default:
g_assert_not_reached();
}
- return migration_has_main_and_multifd_channels();
+ return true;
}
void migration_outgoing_setup(QIOChannel *ioc)
--
2.55.0
--
Peter Xu
