Re: [PATCH 2/4] migration: Add block-bitmap-mapping parameter

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Max Reitz (mreitz@redhat.com) wrote:
> On 30.06.20 12:51, Dr. David Alan Gilbert wrote:
> > * Max Reitz (mreitz@redhat.com) wrote:
> >> This migration parameter allows mapping block node names and bitmap
> >> names to aliases for the purpose of block dirty bitmap migration.
> >>
> >> This way, management tools can use different node and bitmap names on
> >> the source and destination and pass the mapping of how bitmaps are to be
> >> transferred to qemu (on the source, the destination, or even both with
> >> arbitrary aliases in the migration stream).
> >>
> >> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >> Signed-off-by: Max Reitz <mreitz@redhat.com>
> >> ---
> >>  qapi/migration.json            |  83 +++++++-
> >>  migration/migration.h          |   3 +
> >>  migration/block-dirty-bitmap.c | 372 ++++++++++++++++++++++++++++-----
> >>  migration/migration.c          |  29 +++
> >>  4 files changed, 432 insertions(+), 55 deletions(-)
> >>
> >> diff --git a/qapi/migration.json b/qapi/migration.json
> >> index d5000558c6..5aeae9bea8 100644
> >> --- a/qapi/migration.json
> >> +++ b/qapi/migration.json
> >> @@ -507,6 +507,44 @@
> >>    'data': [ 'none', 'zlib',
> >>              { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
> >>  
> >> +##
> >> +# @BitmapMigrationBitmapAlias:
> >> +#
> >> +# @name: The name of the bitmap.
> >> +#
> >> +# @alias: An alias name for migration (for example the bitmap name on
> >> +#         the opposite site).
> >> +#
> >> +# Since: 5.1
> >> +##
> >> +{ 'struct': 'BitmapMigrationBitmapAlias',
> >> +  'data': {
> >> +      'name': 'str',
> >> +      'alias': 'str'
> >> +  } }
> >> +
> >> +##
> >> +# @BitmapMigrationNodeAlias:
> >> +#
> >> +# Maps a block node name and the bitmaps it has to aliases for dirty
> >> +# bitmap migration.
> >> +#
> >> +# @node-name: A block node name.
> >> +#
> >> +# @alias: An alias block node name for migration (for example the
> >> +#         node name on the opposite site).
> >> +#
> >> +# @bitmaps: Mappings for the bitmaps on this node.
> >> +#
> >> +# Since: 5.1
> >> +##
> >> +{ 'struct': 'BitmapMigrationNodeAlias',
> >> +  'data': {
> >> +      'node-name': 'str',
> >> +      'alias': 'str',
> >> +      'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
> >> +  } }
> >> +
> >>  ##
> >>  # @MigrationParameter:
> >>  #
> >> @@ -641,6 +679,18 @@
> >>  #          will consume more CPU.
> >>  #          Defaults to 1. (Since 5.0)
> >>  #
> >> +# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
> >> +#          aliases for the purpose of dirty bitmap migration.  Such
> >> +#          aliases may for example be the corresponding names on the
> >> +#          opposite site.
> >> +#          The mapping must be one-to-one and complete: On the source,
> >> +#          migrating a bitmap from a node when either is not mapped
> >> +#          will result in an error.  On the destination, similarly,
> >> +#          receiving a bitmap (by alias) from a node (by alias) when
> >> +#          either alias is not mapped will result in an error.
> >> +#          By default, all node names and bitmap names are mapped to
> >> +#          themselves. (Since 5.1)
> >> +#
> >>  # Since: 2.4
> >>  ##
> >>  { 'enum': 'MigrationParameter',
> >> @@ -655,7 +705,8 @@
> >>             'multifd-channels',
> >>             'xbzrle-cache-size', 'max-postcopy-bandwidth',
> >>             'max-cpu-throttle', 'multifd-compression',
> >> -           'multifd-zlib-level' ,'multifd-zstd-level' ] }
> >> +           'multifd-zlib-level' ,'multifd-zstd-level',
> >> +           'block-bitmap-mapping' ] }
> >>  
> >>  ##
> >>  # @MigrateSetParameters:
> >> @@ -781,6 +832,18 @@
> >>  #          will consume more CPU.
> >>  #          Defaults to 1. (Since 5.0)
> >>  #
> >> +# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
> >> +#          aliases for the purpose of dirty bitmap migration.  Such
> >> +#          aliases may for example be the corresponding names on the
> >> +#          opposite site.
> >> +#          The mapping must be one-to-one and complete: On the source,
> >> +#          migrating a bitmap from a node when either is not mapped
> >> +#          will result in an error.  On the destination, similarly,
> >> +#          receiving a bitmap (by alias) from a node (by alias) when
> >> +#          either alias is not mapped will result in an error.
> >> +#          By default, all node names and bitmap names are mapped to
> >> +#          themselves. (Since 5.1)
> >> +#
> >>  # Since: 2.4
> >>  ##
> >>  # TODO either fuse back into MigrationParameters, or make
> >> @@ -811,7 +874,8 @@
> >>              '*max-cpu-throttle': 'int',
> >>              '*multifd-compression': 'MultiFDCompression',
> >>              '*multifd-zlib-level': 'int',
> >> -            '*multifd-zstd-level': 'int' } }
> >> +            '*multifd-zstd-level': 'int',
> >> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
> > 
> > That's a hairy type for a migration parameter!
> > I'm curious what 'info migrate_parameters' does in hmp or what happens
> > if you try and set it?
> 
> Oh.  I didn’t know these were accessible via HMP.
> 
> As for setting it, that fails an assertion because I thus forgot to
> handle it in hmp_migrate_set_parameter().  I think the best thing to do
> would be to just error out, stating that you cannot set that parameter
> via HMP.

OK, we can clean that up sometime later; it seems an easy enough mapping
to add (I'd appreciate if you did, but won't force it).

> Similarly, info migrate_parameters doesn’t suport it, because I didn’t
> implement it in hmp_info_migrate_parameters().  Since
> hmp_info_migrate_parameters() seems to allow free-form reporting, I
> suppose we could report it as:
> 
> block-bitmap-mapping:
>   node1 -> alias1
>     bitmap1 -> bmap-alias1
>     bitmap2 -> bmap-alias2
>   node2 -> alias2
>     bitmap1 -> bmap-alias1
> 
> etc.
> 
> Or we just don’t report it there (apart from maybe “(present)”), because
> you can’t set it via migrate_set_parameter.
> 
> 
> If there’s any problem with exposing this via the migration parameters,
> I have no problem with adding a new QMP command as I did in the RFC.  I
> was just pointed towards the migration parameters by reviewers, which
> made sense to me, because, well, this kind of is a migration parameter.

Yep, seems like a migration parameter to me; it's just the first time
we've had such a hairy type in there;  make sure the libvirt people are
happy with using it that way please.

Dave

> Max
> 



--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK