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 <farosas@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.3On 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 <farosas@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:
===8<===
From a952479805d8bdfe532ad4e0c0092f758991af08 Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@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 <peterx@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..
--
Peter Xu
Peter Xu <peterx@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 <farosas@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 <peterx@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 <peterx@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.
On Fri, Jun 06, 2025 at 12:43:04PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@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 <farosas@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 <peterx@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 <peterx@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? We can cc the block people on that patch, assuming
we'd always better copy them when touching this part, including the current
patch.
AFAIU, as long as it takes NULL for the real parameter it'll just work.
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.
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.
--
Peter Xu
Peter Xu <peterx@redhat.com> writes:
> On Fri, Jun 06, 2025 at 12:43:04PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@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 <farosas@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 <peterx@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 <peterx@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.
© 2016 - 2025 Red Hat, Inc.