Re: [Qemu-devel] [PATCH 22/24] block/dirty-bitmap: deep release dirty bitmaps

[Max Reitz]

On 23.01.2017 13:10, Vladimir Sementsov-Ogievskiy wrote:
> Remove persistent bitmap from its storage on bdrv_release_dirty_bitmap.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/dirty-bitmap.c      | 21 ++++++++++++++++++---
>  include/block/block_int.h |  3 +++
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 775181c9ab..0977df6309 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -297,7 +297,8 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>  
>  static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
>                                                    BdrvDirtyBitmap *bitmap,
> -                                                  bool only_named)
> +                                                  bool only_named,
> +                                                  bool deep)

I'd rather call it "remove_persistent" or something, which is bad
grammar, but it's better at getting the point across.

>  {
>      BdrvDirtyBitmap *bm, *next;
>      QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
> @@ -305,6 +306,19 @@ static void 
> bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
>              assert(!bm->active_iterators);
>              assert(!bdrv_dirty_bitmap_frozen(bm));
>              assert(!bm->meta);
> +
> +            if (deep && bm->persistent && bs->drv &&
> +                bs->drv->bdrv_remove_persistent_dirty_bitmap)
> +            {
> +                Error *local_err = NULL;
> +                bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, bm->name,
> +                                                             &local_err);
> +                if (local_err != NULL) {
> +                    error_report_err(local_err);

This looks like maybe it's the wrong thing to do... I do agree that
sometimes it may not be fatal, but sometimes it probably is.

> +                }
> +            }
> +
> +
>              QLIST_REMOVE(bm, list);
>              hbitmap_free(bm->bitmap);
>              g_free(bm->name);
> @@ -322,16 +336,17 @@ static void 
> bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
>  
>  void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>  {
> -    bdrv_do_release_matching_dirty_bitmap(bs, bitmap, false);
> +    bdrv_do_release_matching_dirty_bitmap(bs, bitmap, false, true);

The @deep parameter (or however you decide to call it) should be
available to this function's caller, too. I don't believe qcow2's
load_bitmap() wants to delete the persistent bitmap in the error path;
and block-dirty-bitmap-remove should probably allow the user to decide
what to do.

Which brings me to the error thing above: If the user (or management
tool) decides that block-dirty-bitmap-remove should remove the
persistent bitmap, I believe that the operation should be aborted if the
persistent bitmap cannot be removed. It should not just go on and
release the in-memory bitmap but abort altogether. If the user just
wants to release that in-memory bitmap then, they can still go on an
call block-dirty-bitmap-remove with deep=false.

>  }
>  
>  /**
>   * Release all named dirty bitmaps attached to a BDS (for use in 
> bdrv_close()).
>   * There must not be any frozen bitmaps attached.
> + * This function do not remove persistent bitmaps from the storage.

*does

Max

>   */
>  void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
>  {
> -    bdrv_do_release_matching_dirty_bitmap(bs, NULL, true);
> +    bdrv_do_release_matching_dirty_bitmap(bs, NULL, true, false);
>  }
>  
>  void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index b38b3aa198..3e080d6d68 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -326,6 +326,9 @@ struct BlockDriver {
>                                                  Error **errp);
>      bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs, const char 
> *name,
>                                              uint32_t granularity, Error 
> **errp);
> +    void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
> +                                                const char *name,
> +                                                Error **errp);
>  
>      QLIST_ENTRY(BlockDriver) list;
>  };
>

signature.asc
Description: OpenPGP digital signature