Peter Xu <[email protected]> writes:
> 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.
>
Ok, no worries.
>>
>> > + *
>> > + * 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.
>
You mean the inc at qio_channel_create_fd_watch()? Then we don't need
this comment.
>>
>> > + *
>> > + * 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 we can reach migration_channel_process_incoming() via OOB command,
then everything below it needs to be audited to ensure we're not
changing global state concurrently. Mostly pre-existing, but you are
adding more code inside the danger window.
>>
>> > + */
>> > + 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.
>
I wrote a migration_can_start() routine and tried to incorporate some
preempt channel knowledge into it, but it didn't work so well. I find it
a bit opaque to look only at main+mfd channels to start the
migration. Since nothing better came up so far, I agree you keep this
here.
>>
>> > + /*
>> > + * 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.
>
I tend to prefer having a single path and make it a noop via code than
to have two paths and use our knowledge of the code to decide what goes
where. Here we can't escape having two paths, so it's not a huge deal.
>>
>> > + }
>> > +
>> > +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..
>
You removed the check from _identify, so it's less confusing now.
>>
>> > + /*
>> > + * 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.
>
ok
>>
>> > + 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. :)
>
hahaha crap. Put a +1 next to that comment and let's increment it every
time someone mentions this.
> 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.
>
It will happen in CI one day when we least expect it, don't worry. =)
It's fine, I don't think it's good use of our time to overthink this at
the moment.
> 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;
Fair.
> - } 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);
Please carry my "async" comment if possible. It helps when navigating
the code because there's a migration_incoming_channel_install() just
below so we can jump to it without having to read
migration_incoming_channel_watch() definition.
> } 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) */
Probably not needed, this is the expected way.
> 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;
I see. Maybe a separate preliminary patch for this change?
>
> default:
> g_assert_not_reached();
> }
>
> - return migration_has_main_and_multifd_channels();
> + return true;
> }
>
> void migration_outgoing_setup(QIOChannel *ioc)
> --
> 2.55.0
