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

[John Snow]

On 11/09/2015 05:39 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 <address@hidden>
> ---
>  block.c | 37 +++++++++++++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/block.c b/block.c
> index cffac75..94c0ecf 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);
>  
> @@ -3303,21 +3306,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.
> + */
> +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));
> 

Thanks!

Reviewed-by: John Snow <address@hidden>