From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 2719FEB64DD for ; Thu, 29 Jun 2023 19:19:51 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qExAS-00062K-Nl; Thu, 29 Jun 2023 15:19:01 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qExAP-00061z-VY for qemu-devel@nongnu.org; Thu, 29 Jun 2023 15:18:58 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qExAJ-0001rj-3r for qemu-devel@nongnu.org; Thu, 29 Jun 2023 15:18:52 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1688066329; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=G/qNspMSpZb8m14NDmdXJ7lYaPwgR3R6PCyOiGZZZ5c=; b=eEyJrvRpBQNul9d6zV7wLJKtVwc48qGFl05hJTqeGfbICksFLgijwRyj2yC3Z1Y145oxRf QXriep0CfcgObNY7WwaxGErnvxzXmmbZgJD8BUeJIo6kJv7YtCjQxpE5nyADTSzgJauIlP oxO7RJ8EMiuI4SxTWuyGzh7Fhg60TwQ= Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-600-iMmVkOj9N6GQjGGYFrxTPQ-1; Thu, 29 Jun 2023 15:18:46 -0400 X-MC-Unique: iMmVkOj9N6GQjGGYFrxTPQ-1 Received: by mail-qk1-f199.google.com with SMTP id af79cd13be357-7672918d8a4so27659585a.0 for ; Thu, 29 Jun 2023 12:18:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688066326; x=1690658326; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=G/qNspMSpZb8m14NDmdXJ7lYaPwgR3R6PCyOiGZZZ5c=; b=amcMLnfsBiO0e6CghEw6hJqkFsX0Xqa5a6leT+g6fMUIh1KASdVHBfHStiDv9OrXaF tqGDfJ+PrqqLOGAPGb1hIUwXxlrUQ4fT1xseP/X1GLV2xx94iE6fsSTos7DWspPpQNad 7QnToiVBP6Ck5tdcysVBeEqRZRWgMzA+xu6HPDsRmporj+8TUhN+XRPGBuLV63CNOgbT 4oanv/Jg/Sgb/USTH3bIrBVp34DH7DLe3gfJX5GW+d/xvF10IcAVbZqCKY9UVDQsvXrL ctPyZTwgGuG6ilaUY95Onrr77WSOzyn/4vNVg4uz+RuBk6gJ5X/YZtWocxbohxU1pmFi ZJjQ== X-Gm-Message-State: AC+VfDyqMe++ME4jXCR0uZFoyOdoIrdnbv/HnGXPgCpML7pzJsNgfc5r Wqn5vNsWKFlZfVHinKX2RyR1OTcAb0mx/5Pnp5vUUgBEvsJVuQbt5aE1NEMmCzErYKpiVbCMefw zqhcz/F72+x4tg3U= X-Received: by 2002:a05:620a:4010:b0:766:3190:8052 with SMTP id h16-20020a05620a401000b0076631908052mr397235qko.0.1688066326299; Thu, 29 Jun 2023 12:18:46 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ5XNJ7bGEjuE1yM/NQsmwPVkolIoy9Fd9SozdB06EQQJojp2hJuWazFNUcd5k6jbKXfGKaURw== X-Received: by 2002:a05:620a:4010:b0:766:3190:8052 with SMTP id h16-20020a05620a401000b0076631908052mr397220qko.0.1688066325988; Thu, 29 Jun 2023 12:18:45 -0700 (PDT) Received: from x1n (cpe5c7695f3aee0-cm5c7695f3aede.cpe.net.cable.rogers.com. [99.254.144.39]) by smtp.gmail.com with ESMTPSA id t3-20020a05620a034300b007654bb4a842sm6441179qkm.104.2023.06.29.12.18.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 29 Jun 2023 12:18:45 -0700 (PDT) Date: Thu, 29 Jun 2023 15:18:32 -0400 From: Peter Xu To: Fabiano Rosas Cc: qemu-devel@nongnu.org, Juan Quintela , Steve Sistare , Daniel P =?utf-8?B?LiBCZXJyYW5nw6k=?= , Leonardo Bras Subject: Re: [PATCH 1/6] migration: Set migration status early in incoming side Message-ID: References: <20230628165542.17214-1-farosas@suse.de> <20230628165542.17214-2-farosas@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20230628165542.17214-2-farosas@suse.de> Received-SPF: pass client-ip=170.10.133.124; envelope-from=peterx@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H5=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org On Wed, Jun 28, 2023 at 01:55:37PM -0300, Fabiano Rosas wrote: > We are sending a migration event of MIGRATION_STATUS_SETUP at > qemu_start_incoming_migration but never actually setting the state. > > This creates a window between qmp_migrate_incoming and > process_incoming_migration_co where the migration status is still > MIGRATION_STATUS_NONE. Calling query-migrate during this time will > return an empty response even though the incoming migration command > has already been issued. > > Commit 7cf1fe6d68 ("migration: Add migration events on target side") > has added support to the 'events' capability to the incoming part of > migration, but chose to send the SETUP event without setting the > state. I'm assuming this was a mistake. > > To avoid introducing a change in behavior, we need to keep sending the > SETUP event, even if the 'events' capability is not set. Add the > force-emit-setup-event migration property to enable it. This is so unfortunate... since qemu 2.4..... Does it mean that when cap-events is set we can send duplicated events? The fix makes sense to me in general, butt I'm curious whether we can fix it without having a compat bit doing the wrong thing, even if having the risk of breaking someone, with the hope that the only thing he/she needs to do is to enable the cap-events if didn't. I'd consider that if e.g. as long as libvirt is fine. Does anyone know how libvirt handles this? The worst case is if there's major breakage we can apply a patch adding the compat bit and copy stable, which should cover all the recent releases. And if no report after a few releases, probably mean we're fine anyway. It's just feel so unfortunate migration needs to carry over so much legacy issues along the way. So hope to avoid it if any possiblility. > > Signed-off-by: Fabiano Rosas > --- > migration/migration.c | 17 +++++++++++++++-- > migration/migration.h | 11 +++++++++++ > migration/options.c | 13 +++++++++++++ > migration/options.h | 1 + > 4 files changed, 40 insertions(+), 2 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 7c8292d4d4..6da1865e80 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -424,13 +424,26 @@ void migrate_add_address(SocketAddress *address) > static void qemu_start_incoming_migration(const char *uri, Error **errp) > { > const char *p = NULL; > + MigrationIncomingState *mis = migration_incoming_get_current(); > > /* URI is not suitable for migration? */ > if (!migration_channels_and_uri_compatible(uri, errp)) { > return; > } > > - qapi_event_send_migration(MIGRATION_STATUS_SETUP); > + migrate_set_state(&mis->state, MIGRATION_STATUS_NONE, > + MIGRATION_STATUS_SETUP); > + /* > + * QMP clients should have set the 'events' migration capability > + * if they want to receive this event, in which case the > + * migrate_set_state() call above will have already sent the > + * event. We still need to send the event for compatibility even > + * if migration events are disabled. > + */ > + if (migrate_emit_setup_event()) { > + qapi_event_send_migration(MIGRATION_STATUS_SETUP); > + } > + > if (strstart(uri, "tcp:", &p) || > strstart(uri, "unix:", NULL) || > strstart(uri, "vsock:", NULL)) { > @@ -524,7 +537,7 @@ process_incoming_migration_co(void *opaque) > > mis->largest_page_size = qemu_ram_pagesize_largest(); > postcopy_state_set(POSTCOPY_INCOMING_NONE); > - migrate_set_state(&mis->state, MIGRATION_STATUS_NONE, > + migrate_set_state(&mis->state, MIGRATION_STATUS_SETUP, > MIGRATION_STATUS_ACTIVE); > > mis->loadvm_co = qemu_coroutine_self(); > diff --git a/migration/migration.h b/migration/migration.h > index 30c3e97635..05e1e19e4f 100644 > --- a/migration/migration.h > +++ b/migration/migration.h > @@ -433,6 +433,17 @@ struct MigrationState { > */ > uint8_t clear_bitmap_shift; > > + /* > + * Always emit the incoming migration's SETUP event, even when the > + * 'events' capability is not enabled. > + * > + * QMP clients that wish to receive migration events should always > + * enable the 'events' capability. This property is for > + * compatibility with clients that rely on the older QEMU behavior > + * of unconditionally emitting the SETUP event. > + */ > + bool force_emit_setup_event; > + > /* > * This save hostname when out-going migration starts > */ > diff --git a/migration/options.c b/migration/options.c > index b62ab30cd5..b0eda7cb05 100644 > --- a/migration/options.c > +++ b/migration/options.c > @@ -95,6 +95,8 @@ Property migration_properties[] = { > clear_bitmap_shift, CLEAR_BITMAP_SHIFT_DEFAULT), > DEFINE_PROP_BOOL("x-preempt-pre-7-2", MigrationState, > preempt_pre_7_2, false), > + DEFINE_PROP_BOOL("force-emit-setup-event", MigrationState, > + force_emit_setup_event, true), > > /* Migration parameters */ > DEFINE_PROP_UINT8("x-compress-level", MigrationState, > @@ -338,6 +340,17 @@ bool migrate_zero_copy_send(void) > > /* pseudo capabilities */ > > +bool migrate_emit_setup_event(void) > +{ > + MigrationState *s = migrate_get_current(); > + > + /* > + * If migration events are enabled the setup event will have > + * already been sent. > + */ > + return !migrate_events() && s->force_emit_setup_event; > +} > + > bool migrate_multifd_flush_after_each_section(void) > { > MigrationState *s = migrate_get_current(); > diff --git a/migration/options.h b/migration/options.h > index 45991af3c2..5c9785e455 100644 > --- a/migration/options.h > +++ b/migration/options.h > @@ -52,6 +52,7 @@ bool migrate_zero_copy_send(void); > * check, but they are not a capability. > */ > > +bool migrate_emit_setup_event(void); > bool migrate_multifd_flush_after_each_section(void); > bool migrate_postcopy(void); > bool migrate_tls(void); > -- > 2.35.3 > -- Peter Xu