Re: [Qemu-devel] [Qemu-block] [PATCH v6 03/15] block: Release dirty bitmaps in bdrv_close()

[John Snow <jsnow@redhat.com>]

On 11/04/2015 01:57 PM, 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>
> ---
>  block.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 3493501..23448ed 100644
> --- a/block.c
> +++ b/block.c
> @@ -87,6 +87,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;
>  
> @@ -1916,6 +1918,8 @@ void bdrv_close(BlockDriverState *bs)
>      bdrv_drain(bs); /* in case flush left pending I/O */
>      notifier_list_notify(&bs->close_notifiers, bs);
>  
> +    bdrv_release_all_dirty_bitmaps(bs);
> +
>      if (bs->blk) {
>          blk_dev_change_media_cb(bs->blk, false);
>      }
> @@ -2123,7 +2127,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);
>  
> @@ -3318,6 +3321,21 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>      }
>  }
>  
> +/**
> + * Release all dirty bitmaps attached to a BDS, independently of whether they
> + * are frozen or not (for use in bdrv_close()).
> + */

This comment caught my attention ...

> +static void bdrv_release_all_dirty_bitmaps(BlockDriverState *bs)
> +{
> +    BdrvDirtyBitmap *bm, *next;
> +    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
> +        QLIST_REMOVE(bm, list);
> +        hbitmap_free(bm->bitmap);
> +        g_free(bm->name);
> +        g_free(bm);
> +    }
> +}
> +
>  void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>  {
>      assert(!bdrv_dirty_bitmap_frozen(bitmap));
> 

Currently, the only way a bitmap could/should be frozen is if it is in
use by a job. If a job is running, you probably shouldn't delete stuff
it is using out from under it.

I am assuming by now that it's actually likely that you've canceled any
jobs attached to this node before you go through the motions of deleting
it, and it should be safe to just call bdrv_release_dirty_bitmap ...

We don't want the two foreach loops though, so maybe just factor out a
helper that bdrv_release_all_dirty_bitmaps and bdrv_release_dirty_bitmap
can share. You can leave the frozen assertion
in just bdrv_release_dirty_bitmap before it invokes the helper, so that
bdrv_delete always succeeds in case something gets out-of-sync in the
internals.

(Hmm, to prevent leaks, if you delete a bitmap that is frozen, you
should probably call the helper on its child object too... or just make
the helper recursive.)