Re: [Qemu-devel] [PATCH v8 01/16] block: Release dirty bitmaps in bdrv_close()

[Fam Zheng <famz@redhat.com>]

On Wed, 01/27 18:59, Max Reitz wrote:
> bdrv_delete() is not very happy about deleting BlockDriverStates with
> dirty bitmaps still attached to them. In the past, we got around that
> very easily by relying on bdrv_close_all() bypassing bdrv_delete(), and
> bdrv_close() simply ignoring that condition. We should fix that by
> releasing all dirty bitmaps in bdrv_close() and drop the assertion in
> bdrv_delete().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> ---
>  block.c | 37 +++++++++++++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 5709d3d..9a31e20 100644
> --- a/block.c
> +++ b/block.c
> @@ -88,6 +88,8 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
>                               const BdrvChildRole *child_role, Error **errp);
>  
>  static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs);
> +
>  /* If non-zero, use only whitelisted block drivers */
>  static int use_bdrv_whitelist;
>  
> @@ -2157,6 +2159,8 @@ void bdrv_close(BlockDriverState *bs)
>  
>      notifier_list_notify(&bs->close_notifiers, bs);
>  
> +    bdrv_release_all_dirty_bitmaps(bs);
> +
>      if (bs->blk) {
>          blk_dev_change_media_cb(bs->blk, false);
>      }
> @@ -2366,7 +2370,6 @@ static void bdrv_delete(BlockDriverState *bs)
>      assert(!bs->job);
>      assert(bdrv_op_blocker_is_empty(bs));
>      assert(!bs->refcnt);
> -    assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>  
>      bdrv_close(bs);
>  
> @@ -3582,21 +3585,39 @@ static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>      }
>  }
>  
> -void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
> +                                                  BdrvDirtyBitmap *bitmap)
>  {
>      BdrvDirtyBitmap *bm, *next;
>      QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
> -        if (bm == bitmap) {
> +        if (!bitmap || bm == bitmap) {
>              assert(!bdrv_dirty_bitmap_frozen(bm));
> -            QLIST_REMOVE(bitmap, list);
> -            hbitmap_free(bitmap->bitmap);
> -            g_free(bitmap->name);
> -            g_free(bitmap);
> -            return;
> +            QLIST_REMOVE(bm, list);
> +            hbitmap_free(bm->bitmap);
> +            g_free(bm->name);
> +            g_free(bm);
> +
> +            if (bitmap) {
> +                return;
> +            }
>          }
>      }
>  }
>  
> +void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +{
> +    bdrv_do_release_matching_dirty_bitmap(bs, bitmap);
> +}
> +
> +/**
> + * Release all dirty bitmaps attached to a BDS (for use in bdrv_close()). There
> + * must not be any frozen bitmaps attached.

Should we assert that? And IIUC the intention of this function is to release
all monitor owned (i.e. user created) dirty bitmaps, which must be named. If
so, can we assert that too?

Fam

> + */
> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs)
> +{
> +    bdrv_do_release_matching_dirty_bitmap(bs, NULL);
> +}
> +
>  void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>  {
>      assert(!bdrv_dirty_bitmap_frozen(bitmap));
> -- 
> 2.7.0
>