Add an inconsistent bit to dirty-bitmaps that allows us to report a bitmap as
persistent but potentially inconsistent, i.e. if we find bitmaps on a qcow2
that have been marked as "in use".

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/dirty-bitmap.c         | 19 +++++++++++++++++++
 include/block/dirty-bitmap.h |  2 ++
 qapi/block-core.json         |  8 ++++++--
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 86c3b87ab9..9042c04788 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -46,6 +46,9 @@ struct BdrvDirtyBitmap {
                                    and this bitmap must remain unchanged while
                                    this flag is set. */
     bool persistent;            /* bitmap must be saved to owner disk image */
+    bool inconsistent;          /* bitmap is persistent, but not owned by QEMU.
+                                 * It cannot be used at all in any way, except
+                                 * a QMP user can remove it. */
     bool migration;             /* Bitmap is selected for migration, it should
                                    not be stored on the next inactivation
                                    (persistent flag doesn't matter until next
@@ -462,6 +465,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
         info->recording = bdrv_dirty_bitmap_recording(bm);
         info->busy = bdrv_dirty_bitmap_busy(bm);
         info->persistent = bm->persistent;
+        info->has_inconsistent = bm->inconsistent;
+        info->inconsistent = bm->inconsistent;
         entry->value = info;
         *plist = entry;
         plist = &entry->next;
@@ -709,6 +714,15 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent)
     qemu_mutex_unlock(bitmap->mutex);
 }
 
+/* Called with BQL taken. */
+void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
+{
+    qemu_mutex_lock(bitmap->mutex);
+    bitmap->inconsistent = true;
+    bitmap->disabled = true;
+    qemu_mutex_unlock(bitmap->mutex);
+}
+
 /* Called with BQL taken. */
 void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration)
 {
@@ -722,6 +736,11 @@ bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap)
     return bitmap->persistent && !bitmap->migration;
 }
 
+bool bdrv_dirty_bitmap_inconsistent(BdrvDirtyBitmap *bitmap)
+{
+    return bitmap->inconsistent;
+}
+
 bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
 {
     BdrvDirtyBitmap *bm;
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index ba8477b73f..b270773f7e 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -68,6 +68,7 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
 void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
                                        bool persistent);
+void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy);
 void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
                              HBitmap **backup, Error **errp);
@@ -91,6 +92,7 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
 bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
 bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
+bool bdrv_dirty_bitmap_inconsistent(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap);
 bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
 BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6e543594b3..a7209fce22 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -470,12 +470,16 @@
 # @persistent: true if the bitmap will eventually be flushed to persistent
 #              storage (since 4.0)
 #
+# @inconsistent: true if this is a persistent bitmap that QEMU does not own.
+#                Implies @recording and @busy to be false. To reclaim
+#                ownership, use @block-dirty-bitmap-remove. (since 4.0)
+#
 # Since: 1.3
 ##
 { 'struct': 'BlockDirtyInfo',
   'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
-           'recording': 'bool', 'busy': 'bool',
-           'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
+           'recording': 'bool', 'busy': 'bool', 'status': 'DirtyBitmapStatus',
+           'persistent': 'bool', '*inconsistent': 'bool' } }
 
 ##
 # @Qcow2BitmapInfoFlags:
-- 
2.17.2

On 2/22/19 6:22 PM, John Snow wrote:
> Add an inconsistent bit to dirty-bitmaps that allows us to report a bitmap as
> persistent but potentially inconsistent, i.e. if we find bitmaps on a qcow2
> that have been marked as "in use".
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/dirty-bitmap.c         | 19 +++++++++++++++++++
>  include/block/dirty-bitmap.h |  2 ++
>  qapi/block-core.json         |  8 ++++++--
>  3 files changed, 27 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

23.02.2019 3:22, John Snow wrote:
> Add an inconsistent bit to dirty-bitmaps that allows us to report a bitmap as
> persistent but potentially inconsistent, i.e. if we find bitmaps on a qcow2
> that have been marked as "in use".
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block/dirty-bitmap.c         | 19 +++++++++++++++++++
>   include/block/dirty-bitmap.h |  2 ++
>   qapi/block-core.json         |  8 ++++++--
>   3 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 86c3b87ab9..9042c04788 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -46,6 +46,9 @@ struct BdrvDirtyBitmap {
>                                      and this bitmap must remain unchanged while
>                                      this flag is set. */
>       bool persistent;            /* bitmap must be saved to owner disk image */
> +    bool inconsistent;          /* bitmap is persistent, but not owned by QEMU.
> +                                 * It cannot be used at all in any way, except
> +                                 * a QMP user can remove it. */
>       bool migration;             /* Bitmap is selected for migration, it should
>                                      not be stored on the next inactivation
>                                      (persistent flag doesn't matter until next
> @@ -462,6 +465,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>           info->recording = bdrv_dirty_bitmap_recording(bm);
>           info->busy = bdrv_dirty_bitmap_busy(bm);
>           info->persistent = bm->persistent;
> +        info->has_inconsistent = bm->inconsistent;
> +        info->inconsistent = bm->inconsistent;
>           entry->value = info;
>           *plist = entry;
>           plist = &entry->next;
> @@ -709,6 +714,15 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent)
>       qemu_mutex_unlock(bitmap->mutex);
>   }
>   
> +/* Called with BQL taken. */
> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
> +{
> +    qemu_mutex_lock(bitmap->mutex);
> +    bitmap->inconsistent = true;
> +    bitmap->disabled = true;
> +    qemu_mutex_unlock(bitmap->mutex);
> +}

Didn't you consider separate bdrv_create_inconsistent_bitmap, which will not allocate HBitmap?

It may be done separately ofcourse..

> +
>   /* Called with BQL taken. */
>   void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration)
>   {
> @@ -722,6 +736,11 @@ bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap)
>       return bitmap->persistent && !bitmap->migration;
>   }
>   
> +bool bdrv_dirty_bitmap_inconsistent(BdrvDirtyBitmap *bitmap)
> +{
> +    return bitmap->inconsistent;
> +}
> +
>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
>   {
>       BdrvDirtyBitmap *bm;
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index ba8477b73f..b270773f7e 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -68,6 +68,7 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>   void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
>   void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
>                                          bool persistent);
> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap);
>   void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy);
>   void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>                                HBitmap **backup, Error **errp);
> @@ -91,6 +92,7 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>   bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>   bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>   bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
> +bool bdrv_dirty_bitmap_inconsistent(BdrvDirtyBitmap *bitmap);
>   bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap);
>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>   BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 6e543594b3..a7209fce22 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -470,12 +470,16 @@
>   # @persistent: true if the bitmap will eventually be flushed to persistent
>   #              storage (since 4.0)
>   #
> +# @inconsistent: true if this is a persistent bitmap that QEMU does not own.
> +#                Implies @recording and @busy to be false. To reclaim
> +#                ownership, use @block-dirty-bitmap-remove. (since 4.0)

If we opened an image for rw with in-use bitmaps, the main thing about them not
that QEMU doesn't own them, but that they are truly inconsistent. Nobody owns them.

Also, "QEMU does not own" sound like somebody other may own. Then removing it don't
seem a correct thing to do.

And removing don't reclaim ownership, but just remove (looks like it was more about
previous version with -clear).

So for me something like: "Bitmap was not correctly saved after last usage, so it
may be inconsistent. It's useless and only take place in a list. The only possible
operation on it is remove." seems better.

> +#
>   # Since: 1.3
>   ##
>   { 'struct': 'BlockDirtyInfo',
>     'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
> -           'recording': 'bool', 'busy': 'bool',
> -           'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
> +           'recording': 'bool', 'busy': 'bool', 'status': 'DirtyBitmapStatus',
> +           'persistent': 'bool', '*inconsistent': 'bool' } }
>   
>   ##
>   # @Qcow2BitmapInfoFlags:
> 


-- 
Best regards,
Vladimir

25.02.2019 17:18, Vladimir Sementsov-Ogievskiy wrote:
> 23.02.2019 3:22, John Snow wrote:
>> Add an inconsistent bit to dirty-bitmaps that allows us to report a bitmap as
>> persistent but potentially inconsistent, i.e. if we find bitmaps on a qcow2
>> that have been marked as "in use".
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   block/dirty-bitmap.c         | 19 +++++++++++++++++++
>>   include/block/dirty-bitmap.h |  2 ++
>>   qapi/block-core.json         |  8 ++++++--
>>   3 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 86c3b87ab9..9042c04788 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -46,6 +46,9 @@ struct BdrvDirtyBitmap {
>>                                      and this bitmap must remain unchanged while
>>                                      this flag is set. */
>>       bool persistent;            /* bitmap must be saved to owner disk image */
>> +    bool inconsistent;          /* bitmap is persistent, but not owned by QEMU.
>> +                                 * It cannot be used at all in any way, except
>> +                                 * a QMP user can remove it. */
>>       bool migration;             /* Bitmap is selected for migration, it should
>>                                      not be stored on the next inactivation
>>                                      (persistent flag doesn't matter until next
>> @@ -462,6 +465,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>>           info->recording = bdrv_dirty_bitmap_recording(bm);
>>           info->busy = bdrv_dirty_bitmap_busy(bm);
>>           info->persistent = bm->persistent;
>> +        info->has_inconsistent = bm->inconsistent;
>> +        info->inconsistent = bm->inconsistent;
>>           entry->value = info;
>>           *plist = entry;
>>           plist = &entry->next;
>> @@ -709,6 +714,15 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent)
>>       qemu_mutex_unlock(bitmap->mutex);
>>   }
>> +/* Called with BQL taken. */
>> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
>> +{
>> +    qemu_mutex_lock(bitmap->mutex);
>> +    bitmap->inconsistent = true;
>> +    bitmap->disabled = true;
>> +    qemu_mutex_unlock(bitmap->mutex);
>> +}
> 
> Didn't you consider separate bdrv_create_inconsistent_bitmap, which will not allocate HBitmap?
> 
> It may be done separately ofcourse..
> 
>> +
>>   /* Called with BQL taken. */
>>   void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration)
>>   {
>> @@ -722,6 +736,11 @@ bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap)
>>       return bitmap->persistent && !bitmap->migration;
>>   }
>> +bool bdrv_dirty_bitmap_inconsistent(BdrvDirtyBitmap *bitmap)
>> +{
>> +    return bitmap->inconsistent;
>> +}
>> +
>>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
>>   {
>>       BdrvDirtyBitmap *bm;
>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>> index ba8477b73f..b270773f7e 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>> @@ -68,6 +68,7 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>>   void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
>>   void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
>>                                          bool persistent);
>> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap);
>>   void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy);
>>   void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>>                                HBitmap **backup, Error **errp);
>> @@ -91,6 +92,7 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>>   bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>>   bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>>   bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
>> +bool bdrv_dirty_bitmap_inconsistent(BdrvDirtyBitmap *bitmap);
>>   bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap);
>>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>>   BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 6e543594b3..a7209fce22 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -470,12 +470,16 @@
>>   # @persistent: true if the bitmap will eventually be flushed to persistent
>>   #              storage (since 4.0)

so, bitmap can't be inconsistent and persistent, as we don't want to flush
inconsistent bitmaps...

>>   #
>> +# @inconsistent: true if this is a persistent bitmap that QEMU does not own.
>> +#                Implies @recording and @busy to be false. To reclaim
>> +#                ownership, use @block-dirty-bitmap-remove. (since 4.0)
> 
> If we opened an image for rw with in-use bitmaps, the main thing about them not
> that QEMU doesn't own them, but that they are truly inconsistent. Nobody owns them.
> 
> Also, "QEMU does not own" sound like somebody other may own. Then removing it don't
> seem a correct thing to do.
> 
> And removing don't reclaim ownership, but just remove (looks like it was more about
> previous version with -clear).
> 
> So for me something like: "Bitmap was not correctly saved after last usage, so it
> may be inconsistent. It's useless and only take place in a list. The only possible
> operation on it is remove." seems better.
> 
>> +#
>>   # Since: 1.3
>>   ##
>>   { 'struct': 'BlockDirtyInfo',
>>     'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
>> -           'recording': 'bool', 'busy': 'bool',
>> -           'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
>> +           'recording': 'bool', 'busy': 'bool', 'status': 'DirtyBitmapStatus',
>> +           'persistent': 'bool', '*inconsistent': 'bool' } }
>>   ##
>>   # @Qcow2BitmapInfoFlags:
>>
> 
> 


-- 
Best regards,
Vladimir

On 2/25/19 10:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> 25.02.2019 17:18, Vladimir Sementsov-Ogievskiy wrote:
>> 23.02.2019 3:22, John Snow wrote:
>>> Add an inconsistent bit to dirty-bitmaps that allows us to report a bitmap as
>>> persistent but potentially inconsistent, i.e. if we find bitmaps on a qcow2
>>> that have been marked as "in use".
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   block/dirty-bitmap.c         | 19 +++++++++++++++++++
>>>   include/block/dirty-bitmap.h |  2 ++
>>>   qapi/block-core.json         |  8 ++++++--
>>>   3 files changed, 27 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>> index 86c3b87ab9..9042c04788 100644
>>> --- a/block/dirty-bitmap.c
>>> +++ b/block/dirty-bitmap.c
>>> @@ -46,6 +46,9 @@ struct BdrvDirtyBitmap {
>>>                                      and this bitmap must remain unchanged while
>>>                                      this flag is set. */
>>>       bool persistent;            /* bitmap must be saved to owner disk image */
>>> +    bool inconsistent;          /* bitmap is persistent, but not owned by QEMU.
>>> +                                 * It cannot be used at all in any way, except
>>> +                                 * a QMP user can remove it. */
>>>       bool migration;             /* Bitmap is selected for migration, it should
>>>                                      not be stored on the next inactivation
>>>                                      (persistent flag doesn't matter until next
>>> @@ -462,6 +465,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>>>           info->recording = bdrv_dirty_bitmap_recording(bm);
>>>           info->busy = bdrv_dirty_bitmap_busy(bm);
>>>           info->persistent = bm->persistent;
>>> +        info->has_inconsistent = bm->inconsistent;
>>> +        info->inconsistent = bm->inconsistent;
>>>           entry->value = info;
>>>           *plist = entry;
>>>           plist = &entry->next;
>>> @@ -709,6 +714,15 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent)
>>>       qemu_mutex_unlock(bitmap->mutex);
>>>   }
>>> +/* Called with BQL taken. */
>>> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
>>> +{
>>> +    qemu_mutex_lock(bitmap->mutex);
>>> +    bitmap->inconsistent = true;
>>> +    bitmap->disabled = true;
>>> +    qemu_mutex_unlock(bitmap->mutex);
>>> +}
>>
>> Didn't you consider separate bdrv_create_inconsistent_bitmap, which will not allocate HBitmap?
>>
>> It may be done separately ofcourse..
>>
>>> +
>>>   /* Called with BQL taken. */
>>>   void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration)
>>>   {
>>> @@ -722,6 +736,11 @@ bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap)
>>>       return bitmap->persistent && !bitmap->migration;
>>>   }
>>> +bool bdrv_dirty_bitmap_inconsistent(BdrvDirtyBitmap *bitmap)
>>> +{
>>> +    return bitmap->inconsistent;
>>> +}
>>> +
>>>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
>>>   {
>>>       BdrvDirtyBitmap *bm;
>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>>> index ba8477b73f..b270773f7e 100644
>>> --- a/include/block/dirty-bitmap.h
>>> +++ b/include/block/dirty-bitmap.h
>>> @@ -68,6 +68,7 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>>>   void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
>>>   void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
>>>                                          bool persistent);
>>> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap);
>>>   void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy);
>>>   void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>>>                                HBitmap **backup, Error **errp);
>>> @@ -91,6 +92,7 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>>>   bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>>>   bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>>>   bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
>>> +bool bdrv_dirty_bitmap_inconsistent(BdrvDirtyBitmap *bitmap);
>>>   bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap);
>>>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>>>   BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 6e543594b3..a7209fce22 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -470,12 +470,16 @@
>>>   # @persistent: true if the bitmap will eventually be flushed to persistent
>>>   #              storage (since 4.0)
> 
> so, bitmap can't be inconsistent and persistent, as we don't want to flush
> inconsistent bitmaps...
> 

I think ideally I'd just change this phrasing to say something like
"true if the bitmap is stored on-disk, or is scheduled to be flushed to
disk."

>>>   #
>>> +# @inconsistent: true if this is a persistent bitmap that QEMU does not own.
>>> +#                Implies @recording and @busy to be false. To reclaim
>>> +#                ownership, use @block-dirty-bitmap-remove. (since 4.0)
>>
>> If we opened an image for rw with in-use bitmaps, the main thing about them not
>> that QEMU doesn't own them, but that they are truly inconsistent. Nobody owns them.
>>
>> Also, "QEMU does not own" sound like somebody other may own. Then removing it don't
>> seem a correct thing to do.
>>
>> And removing don't reclaim ownership, but just remove (looks like it was more about
>> previous version with -clear).
>>
>> So for me something like: "Bitmap was not correctly saved after last usage, so it
>> may be inconsistent. It's useless and only take place in a list. The only possible
>> operation on it is remove." seems better.
>>

You're right. I was going by memory, but the spec says rather plainly:
"The bitmap was not saved correctly." Pretty unambiguous.

I *was* leaving open the window that this bitmap was just simply...
something QEMU didn't understand or know about, but the spec doesn't
allow for that, so I will tighten the phrasing.

I've looked at the counter-proposal now, but I'm not convinced that I
want to have to worry about the 97 usages of the bitmap field now that
it can be NULL... Really, we shouldn't be using the allocated bitmap at
all in these cases -- I agree -- but I worry that this is a premature
optimization that makes determining the correctness of the code by a
human reader a little more difficult.

Ideally we have no bugs, but if we did accidentally use a blank bitmap,
that's much less of a problem than an assertion or SIGSEGV.

Am I being too conservative?

>>> +#
>>>   # Since: 1.3
>>>   ##
>>>   { 'struct': 'BlockDirtyInfo',
>>>     'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
>>> -           'recording': 'bool', 'busy': 'bool',
>>> -           'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
>>> +           'recording': 'bool', 'busy': 'bool', 'status': 'DirtyBitmapStatus',
>>> +           'persistent': 'bool', '*inconsistent': 'bool' } }
>>>   ##
>>>   # @Qcow2BitmapInfoFlags:
>>>
>>
>>
> 
> 

27.02.2019 21:45, John Snow wrote:
> 
> 
> On 2/25/19 10:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 25.02.2019 17:18, Vladimir Sementsov-Ogievskiy wrote:
>>> 23.02.2019 3:22, John Snow wrote:
>>>> Add an inconsistent bit to dirty-bitmaps that allows us to report a bitmap as
>>>> persistent but potentially inconsistent, i.e. if we find bitmaps on a qcow2
>>>> that have been marked as "in use".
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>    block/dirty-bitmap.c         | 19 +++++++++++++++++++
>>>>    include/block/dirty-bitmap.h |  2 ++
>>>>    qapi/block-core.json         |  8 ++++++--
>>>>    3 files changed, 27 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>>> index 86c3b87ab9..9042c04788 100644
>>>> --- a/block/dirty-bitmap.c
>>>> +++ b/block/dirty-bitmap.c
>>>> @@ -46,6 +46,9 @@ struct BdrvDirtyBitmap {
>>>>                                       and this bitmap must remain unchanged while
>>>>                                       this flag is set. */
>>>>        bool persistent;            /* bitmap must be saved to owner disk image */
>>>> +    bool inconsistent;          /* bitmap is persistent, but not owned by QEMU.
>>>> +                                 * It cannot be used at all in any way, except
>>>> +                                 * a QMP user can remove it. */
>>>>        bool migration;             /* Bitmap is selected for migration, it should
>>>>                                       not be stored on the next inactivation
>>>>                                       (persistent flag doesn't matter until next
>>>> @@ -462,6 +465,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>>>>            info->recording = bdrv_dirty_bitmap_recording(bm);
>>>>            info->busy = bdrv_dirty_bitmap_busy(bm);
>>>>            info->persistent = bm->persistent;
>>>> +        info->has_inconsistent = bm->inconsistent;
>>>> +        info->inconsistent = bm->inconsistent;
>>>>            entry->value = info;
>>>>            *plist = entry;
>>>>            plist = &entry->next;
>>>> @@ -709,6 +714,15 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent)
>>>>        qemu_mutex_unlock(bitmap->mutex);
>>>>    }
>>>> +/* Called with BQL taken. */
>>>> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
>>>> +{
>>>> +    qemu_mutex_lock(bitmap->mutex);
>>>> +    bitmap->inconsistent = true;
>>>> +    bitmap->disabled = true;
>>>> +    qemu_mutex_unlock(bitmap->mutex);
>>>> +}
>>>
>>> Didn't you consider separate bdrv_create_inconsistent_bitmap, which will not allocate HBitmap?
>>>
>>> It may be done separately ofcourse..
>>>
>>>> +
>>>>    /* Called with BQL taken. */
>>>>    void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration)
>>>>    {
>>>> @@ -722,6 +736,11 @@ bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap)
>>>>        return bitmap->persistent && !bitmap->migration;
>>>>    }
>>>> +bool bdrv_dirty_bitmap_inconsistent(BdrvDirtyBitmap *bitmap)
>>>> +{
>>>> +    return bitmap->inconsistent;
>>>> +}
>>>> +
>>>>    bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
>>>>    {
>>>>        BdrvDirtyBitmap *bm;
>>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>>>> index ba8477b73f..b270773f7e 100644
>>>> --- a/include/block/dirty-bitmap.h
>>>> +++ b/include/block/dirty-bitmap.h
>>>> @@ -68,6 +68,7 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>>>>    void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
>>>>    void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
>>>>                                           bool persistent);
>>>> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap);
>>>>    void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy);
>>>>    void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>>>>                                 HBitmap **backup, Error **errp);
>>>> @@ -91,6 +92,7 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>>>>    bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>>>>    bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>>>>    bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
>>>> +bool bdrv_dirty_bitmap_inconsistent(BdrvDirtyBitmap *bitmap);
>>>>    bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap);
>>>>    bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>>>>    BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index 6e543594b3..a7209fce22 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -470,12 +470,16 @@
>>>>    # @persistent: true if the bitmap will eventually be flushed to persistent
>>>>    #              storage (since 4.0)
>>
>> so, bitmap can't be inconsistent and persistent, as we don't want to flush
>> inconsistent bitmaps...
>>
> 
> I think ideally I'd just change this phrasing to say something like
> "true if the bitmap is stored on-disk, or is scheduled to be flushed to
> disk."

And such wording leads to immediate question: why it could be stored on disk but
_not_ scheduled to be flushed..

So if you want, more honest is something like "true if bitmap will be flushed to
storage or if it is @inconsistent (read ahead)." but it's not looking nice...

May be something like this?

true if bitmap is marked to be flushed to persistent storage. Bitmap may or may not
already persist in the storage. Also true if bitmap persist in the storage but
considered inconsistent, in which case it will not be flushed and only may be removed,
look at @inconsistent field description.

> 
>>>>    #
>>>> +# @inconsistent: true if this is a persistent bitmap that QEMU does not own.
>>>> +#                Implies @recording and @busy to be false. To reclaim
>>>> +#                ownership, use @block-dirty-bitmap-remove. (since 4.0)
>>>
>>> If we opened an image for rw with in-use bitmaps, the main thing about them not
>>> that QEMU doesn't own them, but that they are truly inconsistent. Nobody owns them.
>>>
>>> Also, "QEMU does not own" sound like somebody other may own. Then removing it don't
>>> seem a correct thing to do.
>>>
>>> And removing don't reclaim ownership, but just remove (looks like it was more about
>>> previous version with -clear).
>>>
>>> So for me something like: "Bitmap was not correctly saved after last usage, so it
>>> may be inconsistent. It's useless and only take place in a list. The only possible
>>> operation on it is remove." seems better.
>>>
> 
> You're right. I was going by memory, but the spec says rather plainly:
> "The bitmap was not saved correctly." Pretty unambiguous.
> 
> I *was* leaving open the window that this bitmap was just simply...
> something QEMU didn't understand or know about, but the spec doesn't
> allow for that, so I will tighten the phrasing.
> 
> I've looked at the counter-proposal now, but I'm not convinced that I
> want to have to worry about the 97 usages of the bitmap field now that
> it can be NULL... Really, we shouldn't be using the allocated bitmap at
> all in these cases -- I agree -- but I worry that this is a premature
> optimization that makes determining the correctness of the code by a
> human reader a little more difficult.
> 
> Ideally we have no bugs, but if we did accidentally use a blank bitmap,
> that's much less of a problem than an assertion or SIGSEGV.

using wrong bitmap for some operation is a potential data corruption, not much
worse than SIGSEGV..

So, we must be sure that we are not using inconsistent bitmap for something except
remove. Then, with any way we implement them, we should add assertions into all
low-level bitmap APIs, and accurate error-checking in top-level, to never crash on
added asserts.. And SIGSEGV is not worse than assert-failure I think.

Hmm, may be the best way is to keep inconsistent bitmaps in separate list? I'll look,
how much is it.

> 
> Am I being too conservative?
> 
>>>> +#
>>>>    # Since: 1.3
>>>>    ##
>>>>    { 'struct': 'BlockDirtyInfo',
>>>>      'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
>>>> -           'recording': 'bool', 'busy': 'bool',
>>>> -           'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
>>>> +           'recording': 'bool', 'busy': 'bool', 'status': 'DirtyBitmapStatus',
>>>> +           'persistent': 'bool', '*inconsistent': 'bool' } }
>>>>    ##
>>>>    # @Qcow2BitmapInfoFlags:
>>>>
>>>
>>>
>>
>>
> 

Another thing: what about migration? I don't think we are going to teach migration protocol
to migrate them. So, migration is a way to get rid of inconsistent bitmaps? Or what? Or
we should restrict migration, if there are any inconsistent bitmap, to force user to remove
them first?


-- 
Best regards,
Vladimir

On 2/28/19 4:13 AM, Vladimir Sementsov-Ogievskiy wrote:

>>>>> +++ b/qapi/block-core.json
>>>>> @@ -470,12 +470,16 @@
>>>>>    # @persistent: true if the bitmap will eventually be flushed to persistent
>>>>>    #              storage (since 4.0)
>>>
>>> so, bitmap can't be inconsistent and persistent, as we don't want to flush
>>> inconsistent bitmaps...
>>>
>>
>> I think ideally I'd just change this phrasing to say something like
>> "true if the bitmap is stored on-disk, or is scheduled to be flushed to
>> disk."
> 
> And such wording leads to immediate question: why it could be stored on disk but
> _not_ scheduled to be flushed..
> 
> So if you want, more honest is something like "true if bitmap will be flushed to
> storage or if it is @inconsistent (read ahead)." but it's not looking nice...
> 
> May be something like this?
> 
> true if bitmap is marked to be flushed to persistent storage. Bitmap may or may not
> already persist in the storage. Also true if bitmap persist in the storage but
> considered inconsistent, in which case it will not be flushed and only may be removed,
> look at @inconsistent field description.

Too long. As @inconsistent is rare, I'd be happy with just:

@persistent: true if the bitmap is marked for association with
persistent storage

which covers both future flushes (for a bitmap that is not yet on disk,
but will get there later) and prior inconsistent images (where we
learned that it was inconsistent because of its existing associate with
persistent storage).

> 
> Another thing: what about migration? I don't think we are going to teach migration protocol
> to migrate them. So, migration is a way to get rid of inconsistent bitmaps? Or what? Or
> we should restrict migration, if there are any inconsistent bitmap, to force user to remove
> them first?

A conservative approach is to start by treating an inconsistent bitmap
as a migration blocker, and could be relaxed later if someone has an
argument for why making migration a backdoor for deletion of
inconsistent bitmaps is a good thing.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

28.02.2019 16:44, Eric Blake wrote:
> On 2/28/19 4:13 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>>>>> +++ b/qapi/block-core.json
>>>>>> @@ -470,12 +470,16 @@
>>>>>>     # @persistent: true if the bitmap will eventually be flushed to persistent
>>>>>>     #              storage (since 4.0)
>>>>
>>>> so, bitmap can't be inconsistent and persistent, as we don't want to flush
>>>> inconsistent bitmaps...
>>>>
>>>
>>> I think ideally I'd just change this phrasing to say something like
>>> "true if the bitmap is stored on-disk, or is scheduled to be flushed to
>>> disk."
>>
>> And such wording leads to immediate question: why it could be stored on disk but
>> _not_ scheduled to be flushed..
>>
>> So if you want, more honest is something like "true if bitmap will be flushed to
>> storage or if it is @inconsistent (read ahead)." but it's not looking nice...
>>
>> May be something like this?
>>
>> true if bitmap is marked to be flushed to persistent storage. Bitmap may or may not
>> already persist in the storage. Also true if bitmap persist in the storage but
>> considered inconsistent, in which case it will not be flushed and only may be removed,
>> look at @inconsistent field description.
> 
> Too long. As @inconsistent is rare, I'd be happy with just:
> 
> @persistent: true if the bitmap is marked for association with
> persistent storage
> 
> which covers both future flushes (for a bitmap that is not yet on disk,
> but will get there later) and prior inconsistent images (where we
> learned that it was inconsistent because of its existing associate with
> persistent storage).

Okay

> 
>>
>> Another thing: what about migration? I don't think we are going to teach migration protocol
>> to migrate them. So, migration is a way to get rid of inconsistent bitmaps? Or what? Or
>> we should restrict migration, if there are any inconsistent bitmap, to force user to remove
>> them first?
> 
> A conservative approach is to start by treating an inconsistent bitmap
> as a migration blocker, and could be relaxed later if someone has an
> argument for why making migration a backdoor for deletion of
> inconsistent bitmaps is a good thing.
> 

Agree

-- 
Best regards,
Vladimir

On 2/25/19 9:18 AM, Vladimir Sementsov-Ogievskiy wrote:
> 23.02.2019 3:22, John Snow wrote:
>> Add an inconsistent bit to dirty-bitmaps that allows us to report a bitmap as
>> persistent but potentially inconsistent, i.e. if we find bitmaps on a qcow2
>> that have been marked as "in use".
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   block/dirty-bitmap.c         | 19 +++++++++++++++++++
>>   include/block/dirty-bitmap.h |  2 ++
>>   qapi/block-core.json         |  8 ++++++--
>>   3 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 86c3b87ab9..9042c04788 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -46,6 +46,9 @@ struct BdrvDirtyBitmap {
>>                                      and this bitmap must remain unchanged while
>>                                      this flag is set. */
>>       bool persistent;            /* bitmap must be saved to owner disk image */
>> +    bool inconsistent;          /* bitmap is persistent, but not owned by QEMU.
>> +                                 * It cannot be used at all in any way, except
>> +                                 * a QMP user can remove it. */
>>       bool migration;             /* Bitmap is selected for migration, it should
>>                                      not be stored on the next inactivation
>>                                      (persistent flag doesn't matter until next
>> @@ -462,6 +465,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>>           info->recording = bdrv_dirty_bitmap_recording(bm);
>>           info->busy = bdrv_dirty_bitmap_busy(bm);
>>           info->persistent = bm->persistent;
>> +        info->has_inconsistent = bm->inconsistent;
>> +        info->inconsistent = bm->inconsistent;
>>           entry->value = info;
>>           *plist = entry;
>>           plist = &entry->next;
>> @@ -709,6 +714,15 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent)
>>       qemu_mutex_unlock(bitmap->mutex);
>>   }
>>   
>> +/* Called with BQL taken. */
>> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap)
>> +{
>> +    qemu_mutex_lock(bitmap->mutex);
>> +    bitmap->inconsistent = true;
>> +    bitmap->disabled = true;
>> +    qemu_mutex_unlock(bitmap->mutex);
>> +}
> 
> Didn't you consider separate bdrv_create_inconsistent_bitmap, which will not allocate HBitmap?
> 
> It may be done separately ofcourse..
> 

Oh, when I was allowing clear I didn't do this step. Now that we are
allowing clear it could be skipped, but if it creates a lot of null
checks I'd rather just keep the empty alloc...

>> +
>>   /* Called with BQL taken. */
>>   void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration)
>>   {
>> @@ -722,6 +736,11 @@ bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap)
>>       return bitmap->persistent && !bitmap->migration;
>>   }
>>   
>> +bool bdrv_dirty_bitmap_inconsistent(BdrvDirtyBitmap *bitmap)
>> +{
>> +    return bitmap->inconsistent;
>> +}
>> +
>>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
>>   {
>>       BdrvDirtyBitmap *bm;
>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>> index ba8477b73f..b270773f7e 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>> @@ -68,6 +68,7 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>>   void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
>>   void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
>>                                          bool persistent);
>> +void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap *bitmap);
>>   void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy);
>>   void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>>                                HBitmap **backup, Error **errp);
>> @@ -91,6 +92,7 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
>>   bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>>   bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>>   bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap);
>> +bool bdrv_dirty_bitmap_inconsistent(BdrvDirtyBitmap *bitmap);
>>   bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap);
>>   bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>>   BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 6e543594b3..a7209fce22 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -470,12 +470,16 @@
>>   # @persistent: true if the bitmap will eventually be flushed to persistent
>>   #              storage (since 4.0)
>>   #
>> +# @inconsistent: true if this is a persistent bitmap that QEMU does not own.
>> +#                Implies @recording and @busy to be false. To reclaim
>> +#                ownership, use @block-dirty-bitmap-remove. (since 4.0)
> 
> If we opened an image for rw with in-use bitmaps, the main thing about them not
> that QEMU doesn't own them, but that they are truly inconsistent. Nobody owns them.
> 
> Also, "QEMU does not own" sound like somebody other may own. Then removing it don't
> seem a correct thing to do.
> 
> And removing don't reclaim ownership, but just remove (looks like it was more about
> previous version with -clear).
> 
> So for me something like: "Bitmap was not correctly saved after last usage, so it
> may be inconsistent. It's useless and only take place in a list. The only possible
> operation on it is remove." seems better.
> 
>> +#
>>   # Since: 1.3
>>   ##
>>   { 'struct': 'BlockDirtyInfo',
>>     'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
>> -           'recording': 'bool', 'busy': 'bool',
>> -           'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
>> +           'recording': 'bool', 'busy': 'bool', 'status': 'DirtyBitmapStatus',
>> +           'persistent': 'bool', '*inconsistent': 'bool' } }
>>   
>>   ##
>>   # @Qcow2BitmapInfoFlags:
>>
> 
> 

-- 
—js

This will be used to show in dirty-bitmaps list inconsistent bitmaps
found in qcow2 image on open. On open, bitmap in qcow2 image considered
inconsistent if it has IN_USE flag already set.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

Hi!

It's my counter-proposal for
 "[PATCH v2 1/4] block/dirty-bitmaps: add inconsistent bit" by John.

Difference:

Require that we don't have data for such bitmaps, which is more strict
than just not load them in qcow2 code. Just don't allocate HBitmap.
So, we save RAM. And we don't need a lot of assertions on
!bitmap->inconsistent for operations, as we'll crash with segfault.
(of course, we still need checking this, to produce correct errors in
 QAPI, so it's only a counter-proposal to the first patch of John's
 series)

 qapi/block-core.json         |  8 +++++-
 include/block/dirty-bitmap.h |  5 ++++
 block/dirty-bitmap.c         | 56 +++++++++++++++++++++++++++++-------
 3 files changed, 57 insertions(+), 12 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6e543594b3..7620653c54 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -470,12 +470,18 @@
 # @persistent: true if the bitmap will eventually be flushed to persistent
 #              storage (since 4.0)
 #
+# @inconsistent: true if the bitmap is inconsistent. Inconsistent bitmaps may
+#                be only removed. Source of inconsistent bitmaps are persistent
+#                bitmaps which were not correctly saved on some point (for
+#                example, due to unexpected Qemu crash)
+#
 # Since: 1.3
 ##
 { 'struct': 'BlockDirtyInfo',
   'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
            'recording': 'bool', 'busy': 'bool',
-           'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
+           'status': 'DirtyBitmapStatus', 'persistent': 'bool',
+           '*inconsistent': 'bool' } }
 
 ##
 # @Qcow2BitmapInfoFlags:
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index ba8477b73f..f2cce7694d 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -9,6 +9,11 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
                                           uint32_t granularity,
                                           const char *name,
                                           Error **errp);
+BdrvDirtyBitmap *bdrv_create_inconsistent_dirty_bitmap(BlockDriverState *bs,
+                                                       uint32_t granularity,
+                                                       const char *name,
+                                                       Error **errp);
+
 void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                                    int chunk_size);
 void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 86c3b87ab9..f3b74d0900 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -30,12 +30,20 @@
 
 struct BdrvDirtyBitmap {
     QemuMutex *mutex;
-    HBitmap *bitmap;            /* Dirty bitmap implementation */
+    HBitmap *bitmap;            /*
+                                 * Dirty bitmap implementation.
+                                 * May bu NULL, in which case the bitmap is
+                                 * "inconsistent". Bitmap don't have
+                                 * bitmap data and don't support any operations
+                                 * related to it. The only allowed modifications
+                                 * are remove and truncate.
+                                 */
     HBitmap *meta;              /* Meta dirty bitmap */
     bool busy;                  /* Bitmap is busy, it can't be used via QMP */
     BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
     char *name;                 /* Optional non-empty unique ID */
     int64_t size;               /* Size of the bitmap, in bytes */
+    uint32_t granularity;       /* Granularity of the bitmap, in bytes */
     bool disabled;              /* Bitmap is disabled. It ignores all writes to
                                    the device */
     int active_iterators;       /* How many iterators are active */
@@ -93,10 +101,11 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
 }
 
 /* Called with BQL taken.  */
-BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
-                                          uint32_t granularity,
-                                          const char *name,
-                                          Error **errp)
+static BdrvDirtyBitmap *do_bdrv_create_dirty_bitmap(BlockDriverState *bs,
+                                                    uint32_t granularity,
+                                                    const char *name,
+                                                    bool inconsistent,
+                                                    Error **errp)
 {
     int64_t bitmap_size;
     BdrvDirtyBitmap *bitmap;
@@ -115,16 +124,34 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
     }
     bitmap = g_new0(BdrvDirtyBitmap, 1);
     bitmap->mutex = &bs->dirty_bitmap_mutex;
-    bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(granularity));
+    bitmap->bitmap =
+        inconsistent ? NULL : hbitmap_alloc(bitmap_size, ctz32(granularity));
     bitmap->size = bitmap_size;
+    bitmap->granularity = granularity;
     bitmap->name = g_strdup(name);
-    bitmap->disabled = false;
+    bitmap->disabled = inconsistent;
     bdrv_dirty_bitmaps_lock(bs);
     QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
     bdrv_dirty_bitmaps_unlock(bs);
     return bitmap;
 }
 
+BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
+                                          uint32_t granularity,
+                                          const char *name,
+                                          Error **errp)
+{
+    return do_bdrv_create_dirty_bitmap(bs, granularity, name, false, errp);
+}
+
+BdrvDirtyBitmap *bdrv_create_inconsistent_dirty_bitmap(BlockDriverState *bs,
+                                                       uint32_t granularity,
+                                                       const char *name,
+                                                       Error **errp)
+{
+    return do_bdrv_create_dirty_bitmap(bs, granularity, name, true, errp);
+}
+
 /* bdrv_create_meta_dirty_bitmap
  *
  * Create a meta dirty bitmap that tracks the changes of bits in @bitmap. I.e.
@@ -269,6 +296,8 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
 
 void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
 {
+    assert(bitmap->bitmap);
+
     bitmap->disabled = false;
 }
 
@@ -288,7 +317,9 @@ static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
     assert(!bdrv_dirty_bitmap_busy(bitmap));
     assert(!bitmap->meta);
     QLIST_REMOVE(bitmap, list);
-    hbitmap_free(bitmap->bitmap);
+    if (bitmap->bitmap) {
+        hbitmap_free(bitmap->bitmap);
+    }
     g_free(bitmap->name);
     g_free(bitmap);
 }
@@ -380,7 +411,9 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes)
     QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
         assert(!bdrv_dirty_bitmap_busy(bitmap));
         assert(!bitmap->active_iterators);
-        hbitmap_truncate(bitmap->bitmap, bytes);
+        if (bitmap->bitmap) {
+            hbitmap_truncate(bitmap->bitmap, bytes);
+        }
         bitmap->size = bytes;
     }
     bdrv_dirty_bitmaps_unlock(bs);
@@ -455,6 +488,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
         BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
         BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
         info->count = bdrv_get_dirty_count(bm);
+        info->has_inconsistent = info->inconsistent = !bm->bitmap;
         info->granularity = bdrv_dirty_bitmap_granularity(bm);
         info->has_name = !!bm->name;
         info->name = g_strdup(bm->name);
@@ -504,7 +538,7 @@ uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
 
 uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap)
 {
-    return 1U << hbitmap_granularity(bitmap->bitmap);
+    return bitmap->granularity;
 }
 
 BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap)
@@ -668,7 +702,7 @@ void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t offset)
 
 int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
 {
-    return hbitmap_count(bitmap->bitmap);
+    return bitmap->bitmap ? hbitmap_count(bitmap->bitmap) : -1;
 }
 
 int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
-- 
2.18.0