Peter Xu <pet...@redhat.com> writes:
> On Fri, Jun 06, 2025 at 12:43:04PM -0300, Fabiano Rosas wrote:
>> Peter Xu <pet...@redhat.com> writes:
>>
>> > On Mon, Jun 02, 2025 at 10:37:54PM -0300, Fabiano Rosas wrote:
>> >> The QAPI converts an empty list on the block-bitmap-mapping input into
>> >> a NULL BitmapMigrationNodeAliasList. The empty list is a valid input
>> >> for the block-bitmap-mapping option, so commit 3cba22c9ad ("migration:
>> >> Fix block_bitmap_mapping migration") started using the
>> >> s->parameters.has_block_bitmap_mapping field to tell when the user has
>> >> passed in an empty list vs. when no list has been passed at all.
>> >>
>> >> However, using the has_block_bitmap_mapping field of s->parameters is
>> >> only possible because MigrationParameters has had its members made
>> >> optional due to historical reasons.
>> >>
>> >> In order to make improvements to the way configuration options are set
>> >> for a migration, we'd like to reduce the usage of the has_* fields of
>> >> the global configuration object (s->parameters).
>> >>
>> >> Add a separate boolean to track the status of the block_bitmap_mapping
>> >> option.
>> >>
>> >> (this was verified to not regress iotest 300, which is the test that
>> >> 3cba22c9ad refers to)
>> >>
>> >> Signed-off-by: Fabiano Rosas <faro...@suse.de>
>> >> ---
>> >> migration/migration.h | 7 +++++++
>> >> migration/options.c | 6 +++---
>> >> 2 files changed, 10 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/migration/migration.h b/migration/migration.h
>> >> index d53f7cad84..ab797540b0 100644
>> >> --- a/migration/migration.h
>> >> +++ b/migration/migration.h
>> >> @@ -510,6 +510,13 @@ struct MigrationState {
>> >> bool rdma_migration;
>> >>
>> >> GSource *hup_source;
>> >> +
>> >> + /*
>> >> + * The block-bitmap-mapping option is allowed to be an emtpy list,
>> >> + * therefore we need a way to know wheter the user has given
>> >> + * anything as input.
>> >> + */
>> >> + bool has_block_bitmap_mapping;
>> >> };
>> >>
>> >> void migrate_set_state(MigrationStatus *state, MigrationStatus old_state,
>> >> diff --git a/migration/options.c b/migration/options.c
>> >> index f64e141394..cf77826204 100644
>> >> --- a/migration/options.c
>> >> +++ b/migration/options.c
>> >> @@ -685,7 +685,7 @@ bool migrate_has_block_bitmap_mapping(void)
>> >> {
>> >> MigrationState *s = migrate_get_current();
>> >>
>> >> - return s->parameters.has_block_bitmap_mapping;
>> >> + return s->has_block_bitmap_mapping;
>> >> }
>> >>
>> >> uint32_t migrate_checkpoint_delay(void)
>> >> @@ -989,7 +989,7 @@ MigrationParameters
>> >> *qmp_query_migrate_parameters(Error **errp)
>> >> params->has_announce_step = true;
>> >> params->announce_step = s->parameters.announce_step;
>> >>
>> >> - if (s->parameters.has_block_bitmap_mapping) {
>> >> + if (s->has_block_bitmap_mapping) {
>> >> params->has_block_bitmap_mapping = true;
>> >> params->block_bitmap_mapping =
>> >> QAPI_CLONE(BitmapMigrationNodeAliasList,
>> >> @@ -1469,7 +1469,7 @@ static void
>> >> migrate_params_apply(MigrationParameters *params)
>> >> qapi_free_BitmapMigrationNodeAliasList(
>> >> s->parameters.block_bitmap_mapping);
>> >>
>> >> - s->parameters.has_block_bitmap_mapping = true;
>> >> + s->has_block_bitmap_mapping = true;
>> >> s->parameters.block_bitmap_mapping =
>> >> QAPI_CLONE(BitmapMigrationNodeAliasList,
>> >> params->block_bitmap_mapping);
>> >> --
>> >> 2.35.3
>> >>
>> >
>> > This is definitely unfortunate, and I'm still scratching my head on
>> > understanding why it's necessary.
>> >
>> > E.g. I tried to revert this patch manually and iotest 300 passed, with:
>>
>> This (mine) patch is not needed per-se. I want it so we stop using
>> s->parameters.has_* altogether. If we think we need a flag to track
>> whether the user has passed some value or not, then we add one to some
>> migration specific state, say MigrationState.
>>
>> This decouples the migration internal usage from the QAPI. Today we use
>> MigrationParameters as defined by the QAPI, we might in the future want
>> something else. And that something else might not come with has_*
>> fields. So it's simple enough now to add this one flag to the
>> MigrationState and be able to me completely independent from the
>> QAPI-generated has_ fields.
>>
>> >
>> > ===8<===
>> > From a952479805d8bdfe532ad4e0c0092f758991af08 Mon Sep 17 00:00:00 2001
>> > From: Peter Xu <pet...@redhat.com>
>> > Date: Fri, 6 Jun 2025 10:44:37 -0400
>> > Subject: [PATCH] Revert "migration: Add a flag to track
>> > block-bitmap-mapping
>> > input"
>> >
>> > This reverts commit fd755a53c0e4ce9739d20d7cdd69400b2a37102c.
>> >
>> > Signed-off-by: Peter Xu <pet...@redhat.com>
>> > ---
>> > migration/migration.h | 7 -------
>> > migration/options.c | 4 ++--
>> > 2 files changed, 2 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/migration/migration.h b/migration/migration.h
>> > index 49761f4699..e710c421f8 100644
>> > --- a/migration/migration.h
>> > +++ b/migration/migration.h
>> > @@ -510,13 +510,6 @@ struct MigrationState {
>> > bool rdma_migration;
>> >
>> > GSource *hup_source;
>> > -
>> > - /*
>> > - * The block-bitmap-mapping option is allowed to be an emtpy list,
>> > - * therefore we need a way to know wheter the user has given
>> > - * anything as input.
>> > - */
>> > - bool has_block_bitmap_mapping;
>> > };
>> >
>> > void migrate_set_state(MigrationStatus *state, MigrationStatus old_state,
>> > diff --git a/migration/options.c b/migration/options.c
>> > index dd2288187d..e71a57764d 100644
>> > --- a/migration/options.c
>> > +++ b/migration/options.c
>> > @@ -765,7 +765,7 @@ bool migrate_has_block_bitmap_mapping(void)
>> > {
>> > MigrationState *s = migrate_get_current();
>> >
>> > - return s->has_block_bitmap_mapping;
>> > + return s->parameters.has_block_bitmap_mapping;
>> > }
>> >
>> > uint32_t migrate_checkpoint_delay(void)
>> > @@ -1376,7 +1376,7 @@ void qmp_migrate_set_parameters(MigrationParameters
>> > *params, Error **errp)
>> > * params structure with the user input around.
>> > */
>> > if (params->has_block_bitmap_mapping) {
>> > - migrate_get_current()->has_block_bitmap_mapping = true;
>> > + migrate_get_current()->parameters.has_block_bitmap_mapping = true;
>> > }
>> >
>> > if (migrate_params_check(tmp, errp)) {
>> > --
>> > 2.49.0
>> > ===8<===
>> >
>> > I'm staring at commit 3cba22c9ad now, looks like what it wants to do is
>> > making sure construct_alias_map() will be invoked even if the block bitmap
>> > mapping is NULL itself. But then right below the code, it has:
>> >
>> > static int init_dirty_bitmap_migration(DBMSaveState *s, Error **errp)
>> > {
>> > ...
>> > if (migrate_has_block_bitmap_mapping()) {
>> > alias_map = construct_alias_map(migrate_block_bitmap_mapping(),
>> > true,
>> > &error_abort);
>> > }
>> > ...
>> > if (!alias_map) {
>> > ...
>> > }
>> > }
>> >
>> > Looks like it's also ready for !alias_map anyway. I'm definitely puzzled
>> > by this code.
>> >
>> > Even if so, IIUC the question can still be asked on whether we can always
>> > assume has_block_bitmap_mapping to be always true, then here instead of:
>> >
>> > if (migrate_has_block_bitmap_mapping()) {
>> > alias_map = construct_alias_map(migrate_block_bitmap_mapping(),
>> > true,
>> > &error_abort);
>> > }
>> >
>> > We do:
>> >
>> > alias_map = construct_alias_map(migrate_block_bitmap_mapping(), true,
>> > &error_abort);
>> >
>> > After all it looks like construct_alias_map() takes NULL too..
>>
>> The point is that construct_alias_map always returns a hashtable. It
>> might be empty if the user passes [], and that's ok according to
>> 3cba22c9ad. So they needed some flag to say: "the user has tried to use
>> block-bitmap-mapping".
>>
>> I don't know why it needs to be like that and I honestly don't want to
>> go into details of block migration just to be able to do a
>> refactoring. All I want is that this code stop using s->parameters.has_*
>> so we can do nice tricks with QAPI_CLONE later on and not bother about
>> this.
>>
>> I fully support we chase this, but keep in mind this patch (mine) is
>> just gingerly moving the problem to the side so we can make progress
>> with this series.
>
> Yep that makes sense.
>
> I'm thinking whether we have other better ways to move on without digging
> another hole for ourselves, e.g. make migrate_has_block_bitmap_mapping() to
> constantly return true?
Your concept of what it takes to dig a hole is quite different from
mine.
> We can cc the block people on that patch, assuming
> we'd always better copy them when touching this part, including the current
> patch.
I think I messed up the get_maintainers usage.
>
> AFAIU, as long as it takes NULL for the real parameter it'll just work.
>
But that's what 3cba22c9ad was fixing. I belive the !alias_map is the
key, it'll be NULL if has_block_bitmap is false, no matter the actual
value of the parameters.
> Then if all tests can pass and no one is unhappy, we go with that. We can
> always add this var back when someone reports a break, then we at least
> know this is needed and why.
>
Ok, this part is a sticking point of the series indeed. I'll try to
clear this up. Let's not make this another "TLS options" situation.
> That's what I'll do, but feel free to choose yours. In all cases, I'd
> still suggest we copy block developers on similar changes.
