Re: [PATCH 05/21] migration: Add a flag to track block-bitmap-mapping input

[Peter Xu <peterx@redhat.com>]

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