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 <mre...@redhat.com>
> ---
> 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 <js...@redhat.com>
