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

[John Snow]

On 5/17/19 11:21 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 | 22 ++++++++++++++++---
>  block/dirty-bitmap.c |  9 +++++---
>  blockdev.c           | 50 +++++++++++++++++++++++++++++---------------
>  3 files changed, 58 insertions(+), 23 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7ccbfff9d0..dcc935d655 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2003,19 +2003,35 @@
>    'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
>              '*persistent': 'bool', '*autoload': 'bool', '*disabled': 'bool' 
> } }
>  
> +##
> +# @BlockDirtyBitmapMergeSource:
> +#
> +# @local: name of the bitmap, attached to the same node as target bitmap.
> +#
> +# @external: bitmap with specified node
> +#
> +# Since: 4.1
> +##
> +{ 'alternate': 'BlockDirtyBitmapMergeSource',
> +  'data': { 'local': 'str',
> +            'external': 'BlockDirtyBitmap' } }
> +

We might be able to use something more generic to name this type of
thing, but I think such changes are wire compatible, so we can rename it
to be more generic if we decide to use this for something else in the
future, so this is good.

>  ##
>  # @BlockDirtyBitmapMerge:
>  #
> -# @node: name of device/node which the bitmap is tracking
> +# @node: name of device/node which the @target bitmap is 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) at @node and/or fully
> +#           specifed BlockDirtyBitmap elements. The latter are supported
> +#           since 4.1.
>  #
>  # Since: 4.0
>  ##
>  { 'struct': 'BlockDirtyBitmapMerge',
> -  'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } }
> +  'data': { 'node': 'str', 'target': 'str',
> +            'bitmaps': ['BlockDirtyBitmapMergeSource'] } }
>  
>  ##
>  # @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..64ccef735b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2112,11 +2112,10 @@ 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,
> +        BlockDirtyBitmapMergeSourceList *bitmaps,
> +        HBitmap **backup, Error **errp);
>  
>  static void block_dirty_bitmap_merge_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,
> +        BlockDirtyBitmapMergeSourceList *bitmaps,
> +        HBitmap **backup, Error **errp)
>  {
>      BlockDriverState *bs;
>      BdrvDirtyBitmap *dst, *src, *anon;
> -    strList *lst;
> +    BlockDirtyBitmapMergeSourceList *lst;
>      Error *local_err = NULL;
>  
>      dst = block_dirty_bitmap_lookup(node, target, &bs, errp);
> @@ -2988,11 +2986,28 @@ static BdrvDirtyBitmap 
> *do_block_dirty_bitmap_merge(const char *node,
>      }
>  
>      for (lst = bitmaps; lst; lst = lst->next) {
> -        src = bdrv_find_dirty_bitmap(bs, lst->value);
> -        if (!src) {
> -            error_setg(errp, "Dirty bitmap '%s' not found", lst->value);
> -            dst = NULL;
> -            goto out;
> +        switch (lst->value->type) {
> +            const char *name, *node;
> +        case QTYPE_QSTRING:
> +            name = lst->value->u.local;
> +            src = bdrv_find_dirty_bitmap(bs, name);
> +            if (!src) {
> +                error_setg(errp, "Dirty bitmap '%s' not found", name);
> +                dst = NULL;
> +                goto out;
> +            }
> +            break;
> +        case QTYPE_QDICT:
> +            node = lst->value->u.external.node;
> +            name = lst->value->u.external.name;
> +            src = block_dirty_bitmap_lookup(node, name, NULL, errp);
> +            if (!src) {
> +                dst = NULL;
> +                goto out;
> +            }
> +            break;
> +        default:
> +            abort();
>          }
>  
>          bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
> @@ -3012,7 +3027,8 @@ 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)
> +                                  BlockDirtyBitmapMergeSourceList *bitmaps,
> +                                  Error **errp)
>  {
>      do_block_dirty_bitmap_merge(node, target, bitmaps, NULL, errp);
>  }
> 

Reviewed-by: John Snow <address@hidden>