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:
===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..
--
Peter Xu
