On 31.01.2017 17:03, Vladimir Sementsov-Ogievskiy wrote:
> 29.01.2017 19:54, Max Reitz wrote:
>> 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 <vsementsov@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.
> 
> I've started to rewrite it in that way and it seems like deep almost
> always will be false. What about just move call to
> bs->drv->bdrv_remove_persistent_dirty_bitmap() directly to
> qmp_block_dirty_bitmap_remove and add parameter
> 'remove_persistent' (or 'remove_from_storage', to make it maximum
> descriptive) only to this qmp command?

Or you could add an explicit bdrv_remove_persistent_dirty_bitmap() which
just wraps the bs->drv function.

> Or even without that parameter, as leaving inconsistent version of
> bitmap in the storage doesn't seem useful.

Hm, right, the bitmap will be automatically removed from the image once
it's closed, right?

Max