Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] qapi: support external bitmaps in block-dirty-bitmap-merge

[John Snow]

On 5/16/19 8:27 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add new optional parameter making possible to merge bitmaps from
> different nodes. It is needed to maintain external snapshots during
> incremental backup chain history.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  qapi/block-core.json | 13 ++++++++++---
>  block/dirty-bitmap.c |  9 ++++++---
>  blockdev.c           | 46 ++++++++++++++++++++++++++++++--------------
>  3 files changed, 48 insertions(+), 20 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7ccbfff9d0..933b50771a 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2006,16 +2006,23 @@
>  ##
>  # @BlockDirtyBitmapMerge:
>  #
> -# @node: name of device/node which the bitmap is tracking
> +# @node: name of device/node which the @target and @bitmaps bitmaps are
> +#        tracking
>  #
>  # @target: name of the destination dirty bitmap
>  #
> -# @bitmaps: name(s) of the source dirty bitmap(s)
> +# @bitmaps: name(s) of the source dirty bitmap(s). The field is optional
> +#           since 4.1.
> +#
> +# @external-bitmaps: additional list of source dirty bitmaps with specified
> +#                    nodes, which allows merging bitmaps between different
> +#                    nodes. (Since: 4.1)
>  #
>  # Since: 4.0
>  ##
>  { 'struct': 'BlockDirtyBitmapMerge',
> -  'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } }
> +  'data': { 'node': 'str', 'target': 'str', '*bitmaps': ['str'],
> +            '*external-bitmaps': ['BlockDirtyBitmap'] } }
>  

I guess you can specify one, or both, or maybe neither! Seems fine.

>  ##
>  # @block-dirty-bitmap-add:
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 59e6ebb861..49646a30e6 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -816,10 +816,10 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, 
> const BdrvDirtyBitmap *src,
>  {
>      bool ret;
>  
> -    /* only bitmaps from one bds are supported */
> -    assert(dest->mutex == src->mutex);
> -
>      qemu_mutex_lock(dest->mutex);
> +    if (src->mutex != dest->mutex) {
> +        qemu_mutex_lock(src->mutex);
> +    }
>  
>      if (bdrv_dirty_bitmap_check(dest, BDRV_BITMAP_DEFAULT, errp)) {
>          goto out;
> @@ -845,4 +845,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const 
> BdrvDirtyBitmap *src,
>  
>  out:
>      qemu_mutex_unlock(dest->mutex);
> +    if (src->mutex != dest->mutex) {
> +        qemu_mutex_unlock(src->mutex);
> +    }
>  }
> diff --git a/blockdev.c b/blockdev.c
> index 79fbac8450..8d37ce5943 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2112,11 +2112,9 @@ static void 
> block_dirty_bitmap_disable_abort(BlkActionState *common)
>      }
>  }
>  
> -static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
> -                                                    const char *target,
> -                                                    strList *bitmaps,
> -                                                    HBitmap **backup,
> -                                                    Error **errp);
> +static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(
> +        const char *node, const char *target, strList *bitmaps,
> +        BlockDirtyBitmapList *external_bitmaps, HBitmap **backup, Error 
> **errp);
>  

You know, that's actually a much smarter way to reflow these ...

>  static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
>                                               Error **errp)
> @@ -2132,8 +2130,9 @@ static void 
> block_dirty_bitmap_merge_prepare(BlkActionState *common,
>      action = common->action->u.block_dirty_bitmap_merge.data;
>  
>      state->bitmap = do_block_dirty_bitmap_merge(action->node, action->target,
> -                                                action->bitmaps, 
> &state->backup,
> -                                                errp);
> +                                                action->bitmaps,
> +                                                action->external_bitmaps,
> +                                                &state->backup, errp);
>  }
>  
>  static void abort_prepare(BlkActionState *common, Error **errp)
> @@ -2965,15 +2964,14 @@ void qmp_block_dirty_bitmap_disable(const char *node, 
> const char *name,
>      bdrv_disable_dirty_bitmap(bitmap);
>  }
>  
> -static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
> -                                                    const char *target,
> -                                                    strList *bitmaps,
> -                                                    HBitmap **backup,
> -                                                    Error **errp)
> +static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(
> +        const char *node, const char *target, strList *bitmaps,
> +        BlockDirtyBitmapList *external_bitmaps, HBitmap **backup, Error 
> **errp)
>  {
>      BlockDriverState *bs;
>      BdrvDirtyBitmap *dst, *src, *anon;
>      strList *lst;
> +    BlockDirtyBitmapList *ext_lst;
>      Error *local_err = NULL;
>  
>      dst = block_dirty_bitmap_lookup(node, target, &bs, errp);
> @@ -3003,6 +3001,22 @@ static BdrvDirtyBitmap 
> *do_block_dirty_bitmap_merge(const char *node,
>          }
>      }
>  
> +    for (ext_lst = external_bitmaps; ext_lst; ext_lst = ext_lst->next) {
> +        src = block_dirty_bitmap_lookup(ext_lst->value->node,
> +                                        ext_lst->value->name, NULL, errp);
> +        if (!src) {
> +            dst = NULL;
> +            goto out;
> +        }
> +
> +        bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            dst = NULL;
> +            goto out;
> +        }
> +    }
> +
>      /* Merge into dst; dst is unchanged on failure. */
>      bdrv_merge_dirty_bitmap(dst, anon, backup, errp);
>  
> @@ -3012,9 +3026,13 @@ static BdrvDirtyBitmap 
> *do_block_dirty_bitmap_merge(const char *node,
>  }
>  
>  void qmp_block_dirty_bitmap_merge(const char *node, const char *target,
> -                                  strList *bitmaps, Error **errp)
> +                                  bool has_bitmaps, strList *bitmaps,
> +                                  bool has_external_bitmaps,
> +                                  BlockDirtyBitmapList *external_bitmaps,
> +                                  Error **errp)
>  {
> -    do_block_dirty_bitmap_merge(node, target, bitmaps, NULL, errp);
> +    do_block_dirty_bitmap_merge(node, target, bitmaps, external_bitmaps, 
> NULL,
> +                                errp);
>  }
>  
>  BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char 
> *node,
> 

I don't think I like the name "external-bitmaps" but I guess I don't
really have a better suggestion. Technically you can use this for
node-local source bitmaps too, it's just that it's a more verbose way to
specify them. (They're like "fully-qualified" bitmap identifiers instead
of the local/relative references we've been using.)

Well, I can't hold you up if I have no better ideas.

Reviewed-by: John Snow <address@hidden>