Re: [PATCH 7/7] migration: Postpone the outgoing preempt channel for preempt_pre_7_2

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> This is the corresponding outgoing fix for the same problem mentioned in
> the previous patch.
>
> The idea is simple: either migration to an old QEMU or when TLS is enabled,
> we rely on correct ordering of channels to make it work: first main, then
> multifd, finally preempt.
>
> This patch re-orders multifd and postcopy preempt channel on outgoing side,
> but only for the old machine types where preempt_pre_7_2 set, to make it
> follow the right orders to create the channels.
>
> Keep the resume path alone because the resume path never involve multifd
> channels, even if multifd is enabled.  For that, fun fact: when multifd is
> enabled with postcopy preempt, network interruption will break all multifd
> channels, but they're never re-connected, until they got recycled at the
> end of postcopy migration.

Is this a separate bug? Do we need to add some cleanup?

> OTOH, multifd_recv_all_channels_created()
> reports true even after postcopy network down, as multifd_recv_state->count
> is never decremented.
>

Seems normal to me. We're not even trying to reconnect multifd.

> Fixes: e27418861288 ("migration: enable multifd and postcopy together")
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 4cc4077a76..f8d664808f 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3733,6 +3733,20 @@ static void *migration_thread(void *opaque)
>          goto out;
>      }
>  
> +    /*
> +     * Only for old 7.1/7.2 machine types running on new binaries (that
> +     * supports multifd+postcopy+preempt).  Makes sure to initiate the
> +     * connect() after multifd channels because the old (broken) binaries
> +     * assumes the order of channels, which we cannot fix anymore.
> +     *
> +     * multifd_send_setup() will make sure all multifd channels will be
> +     * connected first, then initiate preempt channel here guarantees the
> +     * ordering to those old binaries.
> +     */

It's a bit confusing what's old binary, what's new binary here. Old/new
don't really apply I think as there are more than two cases when we look
at the global picture. There's before 7.1, before 7.2, before the
preempt_pre_7_2 flag, before the multifd+postcopy support, before this
series, etc. Specially since this code is exactly the same as the one
under if (resume), but there we're talking about a different new/old.

Also, maybe we should avoid framing the assumption on the order of
channels as "broken", that's how it always worked and is just a case of
unimaginative assumptions, i.e. not knowing multifd would ever run along
preempt. The real bug is that v10.1 added a "new" channel to the mix
without working on compatibility.

In this series you're doing both: fixing the bug that v10.1 introduced
and improving the code to not assume the order of channels.

> +    if (migrate_postcopy_preempt() && s->preempt_pre_7_2) {
> +        postcopy_preempt_setup(s);
> +    }

Don't you want to check for multifd as well and apply this fix only to
the case that can actually trigger it? To avoid inadvertently changing
the normal postcopy case.

> +
>      bql_lock();
>      qemu_savevm_state_header(s->to_dst_file);
>      bql_unlock();
> @@ -4013,16 +4027,16 @@ void migration_start_outgoing(MigrationState *s)
>          open_return_path_on_source(s);
>      }
>  
> -    /*
> -     * This needs to be done before resuming a postcopy.  Note: for newer
> -     * QEMUs we will delay the channel creation until postcopy_start(), to
> -     * avoid disorder of channel creations.
> -     */
> -    if (migrate_postcopy_preempt() && s->preempt_pre_7_2) {
> -        postcopy_preempt_setup(s);
> -    }
> -

Hm, this used to happen before qmp_migrate() returns. Now it's in the
migration thread and after multifd channels connect, there's a big
window there. We added a lot of code to ensure cleanup of multifd
happens properly and without races with TLS thread creation, etc. Have
you considered such issues?

>      if (resume) {
> +        /*
> +         * This needs to be done before resuming a postcopy.  Note: for newer
> +         * QEMUs we will delay the channel creation until postcopy_start(), to
> +         * avoid disorder of channel creations.
> +         */
> +        if (migrate_postcopy_preempt() && s->preempt_pre_7_2) {
> +            postcopy_preempt_setup(s);
> +        }
> +
>          /* Wakeup the main migration thread to do the recovery */
>          migrate_set_state(&s->state, MIGRATION_STATUS_POSTCOPY_RECOVER_SETUP,
>                            MIGRATION_STATUS_POSTCOPY_RECOVER);