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

[Max Reitz]

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.

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.

Max

signature.asc
Description: OpenPGP digital signature