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 <vsement...@virtuozzo.com> > --- > 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
