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