Re: [PATCH v2 2/2] migration: dirty-bitmap: Allow control of bitmap persistance

[Eric Blake]

On 2/10/21 10:53 AM, Peter Krempa wrote:
> Bitmap's source persistance is transported over the migration stream and

persistence

> the destination mirrors it. In some cases the destination might want to
> persist bitmaps which are not persistent on the source (e.g. the result
> of merge of bitmaps from a number of layers on the source when migrating
> into a squashed image) but currently it would need to create another set
> of persistent bitmaps and merge them.
> 
> This patch adds a 'transform' property to the alias map which allows to
> override the persistance of migrated bitmaps both on the source and

persistence

> destination sides.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
> 
> v2:
>  - grammar fixes (Eric)
>  - added 'transform' object to group other possible transformations (Vladimir)
>  - transformation can also be used on source (Vladimir)
>  - put bmap_inner directly into DBMLoadState for deduplication  (Vladimir)

In addition to Vladimir's suggestion to use QAPI_CLONE() and an iotest
addition,

> 
>  migration/block-dirty-bitmap.c | 38 +++++++++++++++++++++++++++-------
>  qapi/migration.json            | 20 +++++++++++++++++-
>  2 files changed, 50 insertions(+), 8 deletions(-)
> 
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 0169f672df..a05bf74073 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -138,6 +138,13 @@ typedef struct LoadBitmapState {
>      bool enabled;
>  } LoadBitmapState;
> 
> +typedef struct AliasMapInnerBitmap {
> +    char *string;
> +
> +    /* 'transform' properties borrowed from QAPI */
> +    BitmapMigrationBitmapAliasTransform *transform;
> +} AliasMapInnerBitmap;
> +

You moved this typedef up...

>  /* State of the dirty bitmap migration (DBM) during load process */
>  typedef struct DBMLoadState {
>      uint32_t flags;
> @@ -148,6 +155,7 @@ typedef struct DBMLoadState {
>      BdrvDirtyBitmap *bitmap;
> 
>      bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start 
> */
> +    AliasMapInnerBitmap *bmap_inner;
> 
>      /*
>       * cancelled
> @@ -169,10 +177,6 @@ typedef struct DBMState {
> 
>  static DBMState dbm_state;
> 
> -typedef struct AliasMapInnerBitmap {
> -    char *string;
> -} AliasMapInnerBitmap;
> -

...from here, although it was just added here in patch 1.  Why not just
declare it up there in the first place in patch 1 to minimize the churn?


> +++ b/qapi/migration.json
> @@ -536,6 +536,20 @@
>    'data': [ 'none', 'zlib',
>              { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
> 
> +##
> +# @BitmapMigrationBitmapAliasTransform:
> +#
> +# @persistent: If present, the bitmap will be turned persistent

s/turned/made/ sounds a little nicer, although what you have wasn't wrong.

> +#              or transient depending on this parameter.
> +#              (since 6.0)

You don't need a '(since 6.0)' tag on the member, since...

> +#
> +# Since: 6.0

...the entire struct was introduced at the same time.

> +##
> +{ 'struct': 'BitmapMigrationBitmapAliasTransform',
> +  'data': {
> +      '*persistent': 'bool'
> +  } }
> +
>  ##
>  # @BitmapMigrationBitmapAlias:
>  #
> @@ -544,12 +558,16 @@
>  # @alias: An alias name for migration (for example the bitmap name on
>  #         the opposite site).
>  #
> +# @transform: Allows to modify properties of the migrated bitmap.
> +#             (since 6.0)
> +#

whereas this member tag is correct.

>  # Since: 5.2
>  ##
>  { 'struct': 'BitmapMigrationBitmapAlias',
>    'data': {
>        'name': 'str',
> -      'alias': 'str'
> +      'alias': 'str',
> +      '*transform': 'BitmapMigrationBitmapAliasTransform'
>    } }
> 
>  ##
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org