Re: [PATCH 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:
>
>   - 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 <jmarcin@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  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.

>
> 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.

>      } 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.

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].

> +
> +    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].

> +        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?

> +     *
> +     * 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.

> +     *
> +     * 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.

> +     */
> +    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].

> +    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].

> +        /*
> +         * 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].

> +    }
> +
> +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].

> +            /*
> +             * 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].

> +                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?

> +            }
> +        }
>      }
>  
>      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.

> +    current_incoming->channels_early.channels =
> +        g_array_new(FALSE, TRUE, sizeof(MigEarlyIncomingChannel));
> +    qemu_mutex_init(&current_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?

> +     *
> +     * Above migration_incoming_transport_cleanup() should have disarmed
> +     * anything that we could accept() new sockets.
> +     *
> +     * Here return of migration_incoming_free_early_channels() makes sure
> +     * even if something already fired concurrently, it won't really do
> +     * anything but return - see migration_incoming_early_channel_remove()
> +     * and its return code for details.
> +     */
> +    migration_incoming_free_early_channels(mis);
> +
>      multifd_recv_cleanup();
>  
>      if (ps != POSTCOPY_INCOMING_NONE) {
> @@ -491,7 +513,6 @@ void migration_incoming_state_destroy(void)
>          mis->postcopy_remote_fds = NULL;
>      }
>  
> -    migration_incoming_transport_cleanup(mis);
>      qemu_event_reset(&mis->main_thread_load_event);
>  
>      if (mis->page_requested) {
> diff --git a/migration/trace-events b/migration/trace-events
> index 9ef42f54d1..c408deeb72 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -207,9 +207,10 @@ migration_start_incoming(void) ""
>  migration_transferred_bytes(uint64_t qemu_file, uint64_t multifd, uint64_t rdma) "qemu_file %" PRIu64 " multifd %" PRIu64 " RDMA %" PRIu64
>  
>  # channel.c
> -migration_channel_process_incoming(void *ioc, const char *ioctype) "ioc=%p ioctype=%s"
> -migration_set_incoming_channel(void *ioc, const char *ioctype, const char *type) "ioc=%p ioctype=%s type=%s"
> -migration_set_outgoing_channel(void *ioc, const char *ioctype) "ioc=%p ioctype=%s"
> +migration_outgoing_channel_set(void *ioc, const char *ioctype) "ioc=%p ioctype=%s"
> +migration_incoming_channel_process(void *ioc, const char *ioctype) "ioc=%p ioctype=%s"
> +migration_incoming_channel_set(void *ioc, const char *ioctype, const char *type) "ioc=%p ioctype=%s type=%s"
> +migration_incoming_channel_watch(void *ioc, const char *ioctype) "ioc=%p ioctype=%s"
>  
>  # global_state.c
>  migrate_state_too_big(void) ""

[1] applies on top of the whole series. Tested:
make check
migration-test --full
multifd+postcopy/preempt/plain -M 7.2
multifd+postcopy/preempt/plain -M 8.2
multifd+postcopy/preempt/plain -M 11.1
multifd/tcp/uri/plain/none against 7.2 binary (src & dst)
multifd+postcopy/plain against 7.2 binary (src)
postcopy/preempt/plain against 7.2 binary (src)
compat against 11.1, 11.0 and 10.2.0

-- >8 --
From 72f4153d983334f63c7efb91b54fffef15828f7f Mon Sep 17 00:00:00 2001
From: Fabiano Rosas <farosas@suse.de>
Date: Thu, 24 Sep 2026 17:50:10 -0300
Subject: [PATCH] fixup! migration: Fix incoming channel race with multifd and
 preempt_pre_7_2

---
 migration/channel.c | 127 +++++++++++++++++++++-----------------------
 1 file changed, 61 insertions(+), 66 deletions(-)

diff --git a/migration/channel.c b/migration/channel.c
index acb7ef5206..5c5a610d3c 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -43,6 +43,10 @@ static const char *const mig_channel_str[CH_NUM] = {
     [CH_POSTCOPY] = "PREEMPT",
 };
 
+static void
+migration_incoming_detect_preempt_channel(MigrationIncomingState *mis);
+
+
 void migration_connect_outgoing(MigrationState *s, MigrationAddress *addr,
                                 Error **errp)
 {
@@ -203,27 +207,38 @@ static MigChannelType migration_channel_peek(MigrationIncomingState *mis,
 }
 
 /*
- * Returns: >0 if success, ==0 (CH_NONE) if error.  If error happened,
- * *errp must be set.
+ * Returns: >0 if channel was identified, ==0 (CH_NONE) if channel
+ * needs to be peeked asynchronously before identification.
  */
 static MigChannelType migration_channel_identify(MigrationIncomingState *mis,
-                                                 QIOChannel *ioc, Error **errp)
+                                                 QIOChannel *ioc)
 {
-    MigChannelType channel = CH_NONE;
+    MigChannelType channel;
 
     if (migration_has_main_and_multifd_channels()) {
+        /*
+         * If all main+multifd channels are present already, this must
+         * be the preempt channel. Note that this branch must come
+         * first because the preempt channel is peekable but QEMU
+         * can't register an IO watch for it because the channel only
+         * transfers data during postcopy, which is too late.
+         */
         assert(migrate_postcopy_preempt());
         channel = CH_POSTCOPY;
+
+    } else if (qio_channel_is_peekable(ioc)) {
+        /* Needs to be peeked first */
+        channel = CH_NONE;
+
     } 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");
     }
 
     return channel;
@@ -314,40 +329,36 @@ 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,
+static void 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) {
-        return false;
-    }
-
-    /* Installation succeeded, kickoff migration if needed */
-    if (migration_has_main_and_multifd_channels()) {
+    if (migration_incoming_setup_channel(ioc, ch, errp)) {
         migration_start_incoming();
-    }
 
-    return true;
+        /*
+         * Due to different implementations among QEMU versions, the
+         * postcopy preempt channel may not have yet been
+         * identified. Although peekable, it's not possible to use an
+         * IO watch to detect it because it does not contain any early
+         * data. At this point the main and multifd channels have
+         * already been identified, if there is any channel left in
+         * @channels_early, that would be the preempt channel. Look
+         * for it now.
+         *
+         * 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);
+    }
 }
 
 static void
@@ -409,6 +420,9 @@ static gboolean migration_incoming_channel_readable(QIOChannel *ioc,
 {
     MigrationIncomingState *mis = opaque;
     Error *local_err = NULL;
+    MigChannelType ch;
+
+    assert(qio_channel_is_peekable(ioc));
 
     /*
      * No need to monitor this channel anymore as long as anything arrived,
@@ -435,22 +449,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_has_main_and_multifd_channels()) {
-        /*
-         * 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);
-    }
+    migration_incoming_channel_install(mis, ioc, ch, &local_err);
 
 out:
     if (local_err) {
@@ -529,20 +533,10 @@ void migration_channel_process_incoming(QIOChannel *ioc)
     if (migrate_channel_requires_tls_upgrade(ioc)) {
         migration_tls_channel_process_incoming(ioc, &local_err);
     } else {
-        if (migration_has_main_and_multifd_channels()) {
-            /*
-             * 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);
+        MigChannelType ch = migration_channel_identify(mis, ioc);
+
+        if (ch) {
+            migration_incoming_channel_install(mis, ioc, ch, &local_err);
         } else {
             /*
              * Register an IO watch for peekable channels, so that channels
@@ -555,11 +549,12 @@ void migration_channel_process_incoming(QIOChannel *ioc)
              * still so far the only source of truth to identify a channel
              * in such case.
              */
-            if (qio_channel_is_peekable(ioc)) {
-                migration_incoming_channel_watch(mis, ioc);
-            } else {
-                migration_incoming_channel_install(mis, ioc, &local_err);
-            }
+            migration_incoming_channel_watch(mis, ioc);
+            /*
+             * async: the above will call into
+             * migration_incoming_channel_install() when it has
+             * identified the channel.
+             */
         }
     }
 
-- 
2.53.0