We should release them here to reload on invalidate cache.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c                      |  4 ++++
 block/dirty-bitmap.c         | 29 +++++++++++++++++++++++------
 include/block/dirty-bitmap.h |  1 +
 3 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 795d36bb64..14896c65fa 100644
--- a/block.c
+++ b/block.c
@@ -4001,6 +4001,10 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
     if (setting_flag) {
         bs->open_flags |= BDRV_O_INACTIVE;
     }
+
+    /* At this point persistent bitmaps should be stored by format driver */
+    bdrv_release_persistent_dirty_bitmaps(bs);
+
     return 0;
 }
 
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 659617a3f8..30fe2b4355 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -296,13 +296,18 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
     }
 }
 
-static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
-                                                  BdrvDirtyBitmap *bitmap,
-                                                  bool only_named)
+static bool bdrv_dirty_bitmap_has_name(BdrvDirtyBitmap *bitmap)
+{
+    return !!bdrv_dirty_bitmap_name(bitmap);
+}
+
+static void bdrv_do_release_matching_dirty_bitmap(
+    BlockDriverState *bs, BdrvDirtyBitmap *bitmap,
+    bool (*cond)(BdrvDirtyBitmap *bitmap))
 {
     BdrvDirtyBitmap *bm, *next;
     QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
-        if ((!bitmap || bm == bitmap) && (!only_named || bm->name)) {
+        if ((!bitmap || bm == bitmap) && (!cond || cond(bm))) {
             assert(!bm->active_iterators);
             assert(!bdrv_dirty_bitmap_frozen(bm));
             assert(!bm->meta);
@@ -323,7 +328,7 @@ 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, NULL);
 }
 
 /**
@@ -333,7 +338,19 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
  */
 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, bdrv_dirty_bitmap_has_name);
+}
+
+/**
+ * Release all persistent dirty bitmaps attached to a BDS (for use in
+ * bdrv_inactivate_recurse()).
+ * There must not be any frozen bitmaps attached.
+ * This function does not remove persistent bitmaps from the storage.
+ */
+void bdrv_release_persistent_dirty_bitmaps(BlockDriverState *bs)
+{
+    bdrv_do_release_matching_dirty_bitmap(bs, NULL,
+                                          bdrv_dirty_bitmap_get_persistance);
 }
 
 /**
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 2ee63c46e9..23f669bd2e 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -25,6 +25,7 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
 void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
+void bdrv_release_persistent_dirty_bitmaps(BlockDriverState *bs);
 void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
                                          const char *name,
                                          Error **errp);
-- 
2.11.1

On 2017-05-03 14:25, Vladimir Sementsov-Ogievskiy wrote:
> We should release them here to reload on invalidate cache.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block.c                      |  4 ++++
>  block/dirty-bitmap.c         | 29 +++++++++++++++++++++++------
>  include/block/dirty-bitmap.h |  1 +
>  3 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 795d36bb64..14896c65fa 100644
> --- a/block.c
> +++ b/block.c
> @@ -4001,6 +4001,10 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
>      if (setting_flag) {
>          bs->open_flags |= BDRV_O_INACTIVE;
>      }
> +
> +    /* At this point persistent bitmaps should be stored by format driver */

s/by format driver/by the format driver/

> +    bdrv_release_persistent_dirty_bitmaps(bs);

Also, as far as I can see, this doesn't store the bitmaps but just
releases them (without storing them). I'm not sure whether that is
right, but it definitely contradicts the comment above.

Max

> +
>      return 0;
>  }
> 

29.05.2017 20:54, Max Reitz wrote:
> On 2017-05-03 14:25, Vladimir Sementsov-Ogievskiy wrote:
>> We should release them here to reload on invalidate cache.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block.c                      |  4 ++++
>>   block/dirty-bitmap.c         | 29 +++++++++++++++++++++++------
>>   include/block/dirty-bitmap.h |  1 +
>>   3 files changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 795d36bb64..14896c65fa 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4001,6 +4001,10 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
>>       if (setting_flag) {
>>           bs->open_flags |= BDRV_O_INACTIVE;
>>       }
>> +
>> +    /* At this point persistent bitmaps should be stored by format driver */
> s/by format driver/by the format driver/
>
>> +    bdrv_release_persistent_dirty_bitmaps(bs);
> Also, as far as I can see, this doesn't store the bitmaps but just
> releases them (without storing them). I'm not sure whether that is
> right, but it definitely contradicts the comment above.

I mean they should be _already_ stored.. They actually are stored in  
qcow2_inactivate.

>
> Max
>
>> +
>>       return 0;
>>   }
>>

-- 
Best regards,
Vladimir

On 2017-05-30 08:30, Vladimir Sementsov-Ogievskiy wrote:
> 29.05.2017 20:54, Max Reitz wrote:
>> On 2017-05-03 14:25, Vladimir Sementsov-Ogievskiy wrote:
>>> We should release them here to reload on invalidate cache.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block.c                      |  4 ++++
>>>   block/dirty-bitmap.c         | 29 +++++++++++++++++++++++------
>>>   include/block/dirty-bitmap.h |  1 +
>>>   3 files changed, 28 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 795d36bb64..14896c65fa 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -4001,6 +4001,10 @@ static int
>>> bdrv_inactivate_recurse(BlockDriverState *bs,
>>>       if (setting_flag) {
>>>           bs->open_flags |= BDRV_O_INACTIVE;
>>>       }
>>> +
>>> +    /* At this point persistent bitmaps should be stored by format
>>> driver */
>> s/by format driver/by the format driver/
>>
>>> +    bdrv_release_persistent_dirty_bitmaps(bs);
>> Also, as far as I can see, this doesn't store the bitmaps but just
>> releases them (without storing them). I'm not sure whether that is
>> right, but it definitely contradicts the comment above.
> 
> I mean they should be _already_ stored.. They actually are stored in 
> qcow2_inactivate.

Ah, right, that makes more sense.

However, to be more clear the comment should then read "By this point"
instead of "At this point"; maybe even "By this point the persistent
bitmaps should have been stored by the format driver already", or a
totally different "The format driver's bdrv_inactivate() implementation
should have stored all persistent bitmaps".

Max