Peter Xu <[email protected]> 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 <[email protected]>
> ---
> 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);
