[Qemu-devel] [PATCH v2 0/4] bitmaps: add inconsistent bit

[Qemu-devel] [PATCH v2 0/4] bitmaps: add inconsistent bit

[John Snow]

Allow QEMU to read in bitmaps that have the in-use bit set, for the
purposes of allowing users to delete those bitmaps.

This is chosen in preference to a hard error on load to minimize
impact for a non-critical error, but to force the user or management
utility to acknowledge that the bitmap is no longer viable.

Requires: [PATCH v3 00/10] dirty-bitmaps: deprecate @status field

John Snow (4):
  block/dirty-bitmaps: add inconsistent bit
  block/dirty-bitmap: add inconsistent status
  block/dirty-bitmaps: add block_dirty_bitmap_check function
  block/dirty-bitmaps: implement inconsistent bit

 block/dirty-bitmap.c           | 65 +++++++++++++++++++++++-----
 block/qcow2-bitmap.c           | 77 ++++++++++++++++++----------------
 blockdev.c                     | 49 ++++------------------
 include/block/dirty-bitmap.h   | 15 ++++++-
 migration/block-dirty-bitmap.c | 12 ++----
 nbd/server.c                   |  3 +-
 qapi/block-core.json           | 15 +++++--
 7 files changed, 133 insertions(+), 103 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH v2 1/4] block/dirty-bitmaps: add inconsistent bit

[John Snow]

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

Re: [Qemu-devel] [PATCH v2 1/4] block/dirty-bitmaps: add inconsistent bit

[Eric Blake]

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

Re: [Qemu-devel] [PATCH v2 1/4] block/dirty-bitmaps: add inconsistent bit

[Vladimir Sementsov-Ogievskiy]

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

Re: [Qemu-devel] [PATCH v2 1/4] block/dirty-bitmaps: add inconsistent bit

[Vladimir Sementsov-Ogievskiy]

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

Re: [Qemu-devel] [PATCH v2 1/4] block/dirty-bitmaps: add inconsistent bit

[John Snow]

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:
>>>
>>
>>
> 
> 

Re: [Qemu-devel] [PATCH v2 1/4] block/dirty-bitmaps: add inconsistent bit

[Vladimir Sementsov-Ogievskiy]

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

Re: [Qemu-devel] [PATCH v2 1/4] block/dirty-bitmaps: add inconsistent bit

[Eric Blake]

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

Re: [Qemu-devel] [PATCH v2 1/4] block/dirty-bitmaps: add inconsistent bit

[Vladimir Sementsov-Ogievskiy]

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

Re: [Qemu-devel] [PATCH v2 1/4] block/dirty-bitmaps: add inconsistent bit

[John Snow]

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

[Qemu-devel] [PATCH] dirty-bitmap: introduce inconsistent bitmaps

[Vladimir Sementsov-Ogievskiy]

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

[Qemu-devel] [PATCH v2 2/4] block/dirty-bitmap: add inconsistent status

[John Snow]

Even though the status field is deprecated, we still have to support
it for a few more releases. Since this is a very new kind of bitmap
state, it makes sense for it to have its own status field.

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

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 9042c04788..4e8f931659 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -209,10 +209,15 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
  *               or it can be Disabled and not recording writes.
  * (4) Locked:   Whether Active or Disabled, the user cannot modify this bitmap
  *               in any way from the monitor.
+ * (5) Inconsistent: This is a persistent bitmap whose "in use" bit is set, and
+ *                   is unusable by QEMU. It can be deleted to remove it from
+ *                   the qcow2.
  */
 DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
 {
-    if (bdrv_dirty_bitmap_has_successor(bitmap)) {
+    if (bdrv_dirty_bitmap_inconsistent(bitmap)) {
+        return DIRTY_BITMAP_STATUS_INCONSISTENT;
+    } else if (bdrv_dirty_bitmap_has_successor(bitmap)) {
         return DIRTY_BITMAP_STATUS_FROZEN;
     } else if (bdrv_dirty_bitmap_busy(bitmap)) {
         return DIRTY_BITMAP_STATUS_LOCKED;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a7209fce22..b25ca6a6c1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -442,10 +442,15 @@
 #          recording new writes. If the bitmap was @disabled, it is not
 #          recording new writes. (Since 2.12)
 #
+# @inconsistent: This is a persistent dirty bitmap that was marked in-use on
+#                disk, and is unusable by QEMU. It can only be deleted.
+#                Please rely on the inconsistent field in @BlockDirtyInfo
+#                instead, as the status field is deprecated. (Since 4.0)
+#
 # Since: 2.4
 ##
 { 'enum': 'DirtyBitmapStatus',
-  'data': ['active', 'disabled', 'frozen', 'locked'] }
+  'data': ['active', 'disabled', 'frozen', 'locked', 'inconsistent'] }
 
 ##
 # @BlockDirtyInfo:
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 2/4] block/dirty-bitmap: add inconsistent status

[Eric Blake]

On 2/22/19 6:22 PM, John Snow wrote:
> Even though the status field is deprecated, we still have to support
> it for a few more releases. Since this is a very new kind of bitmap
> state, it makes sense for it to have its own status field.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/dirty-bitmap.c | 7 ++++++-
>  qapi/block-core.json | 7 ++++++-
>  2 files changed, 12 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

[Qemu-devel] [PATCH v2 3/4] block/dirty-bitmaps: add block_dirty_bitmap_check function

[John Snow]

Instead of checking against busy, inconsistent, or read only directly,
use a check function with permissions bits that let us streamline the
checks without reproducing them in many places.

As a side effect, this adds consistency checks to all the QMP
interfaces for bitmap manipulation.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/dirty-bitmap.c           | 39 ++++++++++++++++++++-------
 blockdev.c                     | 49 +++++++---------------------------
 include/block/dirty-bitmap.h   | 13 ++++++++-
 migration/block-dirty-bitmap.c | 12 +++------
 nbd/server.c                   |  3 +--
 5 files changed, 54 insertions(+), 62 deletions(-)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 4e8f931659..2e7fd81866 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -174,7 +174,7 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap)
     return bitmap->successor;
 }
 
-bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {
+static bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {
     return bitmap->busy;
 }
 
@@ -235,6 +235,33 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap)
                                  !bitmap->successor->disabled);
 }
 
+int bdrv_dirty_bitmap_check(BdrvDirtyBitmap *bitmap, uint32_t flags,
+                            Error **errp)
+{
+    if ((flags & BDRV_BITMAP_BUSY) && bdrv_dirty_bitmap_busy(bitmap)) {
+        error_setg(errp, "Bitmap '%s' is currently in use by another"
+                   " operation and cannot be used", bitmap->name);
+        return -1;
+    }
+
+    if ((flags & BDRV_BITMAP_RO) && bdrv_dirty_bitmap_readonly(bitmap)) {
+        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
+                   bitmap->name);
+        return -1;
+    }
+
+    if ((flags & BDRV_BITMAP_INCONSISTENT) &&
+        bdrv_dirty_bitmap_inconsistent(bitmap)) {
+        error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
+                   bitmap->name);
+        error_append_hint(errp, "Try block-dirty-bitmap-remove to delete "
+                          "this bitmap from disk");
+        return -1;
+    }
+
+    return 0;
+}
+
 /**
  * Create a successor bitmap destined to replace this bitmap after an operation.
  * Requires that the bitmap is not user_locked and has no successor.
@@ -792,15 +819,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
 
     qemu_mutex_lock(dest->mutex);
 
-    if (bdrv_dirty_bitmap_busy(dest)) {
-        error_setg(errp, "Bitmap '%s' is currently in use by another"
-        " operation and cannot be modified", dest->name);
-        goto out;
-    }
-
-    if (bdrv_dirty_bitmap_readonly(dest)) {
-        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
-                   dest->name);
+    if (bdrv_dirty_bitmap_check(dest, BDRV_BITMAP_DEFAULT, errp)) {
         goto out;
     }
 
diff --git a/blockdev.c b/blockdev.c
index cbce44877d..5d74479ba7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2007,11 +2007,7 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
         return;
     }
 
-    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
-        error_setg(errp, "Cannot modify a bitmap in use by another operation");
-        return;
-    } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) {
-        error_setg(errp, "Cannot clear a readonly bitmap");
+    if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_DEFAULT, errp)) {
         return;
     }
 
@@ -2056,10 +2052,7 @@ static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
         return;
     }
 
-    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
-        error_setg(errp,
-                   "Bitmap '%s' is currently in use by another operation"
-                   " and cannot be enabled", action->name);
+    if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
         return;
     }
 
@@ -2097,10 +2090,7 @@ static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
         return;
     }
 
-    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
-        error_setg(errp,
-                   "Bitmap '%s' is currently in use by another operation"
-                   " and cannot be disabled", action->name);
+    if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
         return;
     }
 
@@ -2891,10 +2881,7 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
         return;
     }
 
-    if (bdrv_dirty_bitmap_busy(bitmap)) {
-        error_setg(errp,
-                   "Bitmap '%s' is currently in use by another operation and"
-                   " cannot be removed", name);
+    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY, errp)) {
         return;
     }
 
@@ -2930,13 +2917,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
         return;
     }
 
-    if (bdrv_dirty_bitmap_busy(bitmap)) {
-        error_setg(errp,
-                   "Bitmap '%s' is currently in use by another operation"
-                   " and cannot be cleared", name);
-        return;
-    } else if (bdrv_dirty_bitmap_readonly(bitmap)) {
-        error_setg(errp, "Bitmap '%s' is readonly and cannot be cleared", name);
+    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
         return;
     }
 
@@ -2954,10 +2935,7 @@ void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
         return;
     }
 
-    if (bdrv_dirty_bitmap_busy(bitmap)) {
-        error_setg(errp,
-                   "Bitmap '%s' is currently in use by another operation"
-                   " and cannot be enabled", name);
+    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
         return;
     }
 
@@ -2975,10 +2953,7 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
         return;
     }
 
-    if (bdrv_dirty_bitmap_busy(bitmap)) {
-        error_setg(errp,
-                   "Bitmap '%s' is currently in use by another operation"
-                   " and cannot be disabled", name);
+    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
         return;
     }
 
@@ -3551,10 +3526,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
             bdrv_unref(target_bs);
             goto out;
         }
-        if (bdrv_dirty_bitmap_busy(bmap)) {
-            error_setg(errp,
-                       "Bitmap '%s' is currently in use by another operation"
-                       " and cannot be used for backup", backup->bitmap);
+        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
             goto out;
         }
     }
@@ -3664,10 +3636,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
             error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);
             goto out;
         }
-        if (bdrv_dirty_bitmap_busy(bmap)) {
-            error_setg(errp,
-                       "Bitmap '%s' is currently in use by another operation"
-                       " and cannot be used for backup", backup->bitmap);
+        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
             goto out;
         }
     }
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index b270773f7e..ee98b5014b 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -5,6 +5,16 @@
 #include "qapi/qapi-types-block-core.h"
 #include "qemu/hbitmap.h"
 
+typedef enum BitmapCheckFlags {
+    BDRV_BITMAP_BUSY = 1,
+    BDRV_BITMAP_RO = 2,
+    BDRV_BITMAP_INCONSISTENT = 4,
+} BitmapCheckFlags;
+
+#define BDRV_BITMAP_DEFAULT (BDRV_BITMAP_BUSY | BDRV_BITMAP_RO |        \
+                             BDRV_BITMAP_INCONSISTENT)
+#define BDRV_BITMAP_ALLOW_RO (BDRV_BITMAP_BUSY | BDRV_BITMAP_INCONSISTENT)
+
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
                                           uint32_t granularity,
                                           const char *name,
@@ -24,6 +34,8 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
 void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap);
 BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
                                         const char *name);
+int bdrv_dirty_bitmap_check(BdrvDirtyBitmap *bitmap, uint32_t flags,
+                            Error **errp);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
 void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
@@ -93,7 +105,6 @@ 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,
                                         BdrvDirtyBitmap *bitmap);
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 7057fff242..0fcf897f32 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -274,6 +274,7 @@ static int init_dirty_bitmap_migration(void)
     BdrvDirtyBitmap *bitmap;
     DirtyBitmapMigBitmapState *dbms;
     BdrvNextIterator it;
+    Error *local_err = NULL;
 
     dirty_bitmap_mig_state.bulk_completed = false;
     dirty_bitmap_mig_state.prev_bs = NULL;
@@ -301,15 +302,8 @@ static int init_dirty_bitmap_migration(void)
                 goto fail;
             }
 
-            if (bdrv_dirty_bitmap_busy(bitmap)) {
-                error_report("Can't migrate a bitmap that is in use by another operation: '%s'",
-                             bdrv_dirty_bitmap_name(bitmap));
-                goto fail;
-            }
-
-            if (bdrv_dirty_bitmap_readonly(bitmap)) {
-                error_report("Can't migrate read-only dirty bitmap: '%s",
-                             bdrv_dirty_bitmap_name(bitmap));
+            if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, &local_err)) {
+                error_report_err(local_err);
                 goto fail;
             }
 
diff --git a/nbd/server.c b/nbd/server.c
index 02773e2836..9b87c7f243 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1510,8 +1510,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
             goto fail;
         }
 
-        if (bdrv_dirty_bitmap_busy(bm)) {
-            error_setg(errp, "Bitmap '%s' is in use", bitmap);
+        if (bdrv_dirty_bitmap_check(bm, BDRV_BITMAP_ALLOW_RO, errp)) {
             goto fail;
         }
 
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 3/4] block/dirty-bitmaps: add block_dirty_bitmap_check function

[Vladimir Sementsov-Ogievskiy]

23.02.2019 3:22, John Snow wrote:
> Instead of checking against busy, inconsistent, or read only directly,
> use a check function with permissions bits that let us streamline the
> checks without reproducing them in many places.
> 
> As a side effect, this adds consistency checks to all the QMP
> interfaces for bitmap manipulation.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block/dirty-bitmap.c           | 39 ++++++++++++++++++++-------
>   blockdev.c                     | 49 +++++++---------------------------
>   include/block/dirty-bitmap.h   | 13 ++++++++-
>   migration/block-dirty-bitmap.c | 12 +++------
>   nbd/server.c                   |  3 +--
>   5 files changed, 54 insertions(+), 62 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 4e8f931659..2e7fd81866 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -174,7 +174,7 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap)
>       return bitmap->successor;
>   }
>   
> -bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {
> +static bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {
>       return bitmap->busy;
>   }
>   
> @@ -235,6 +235,33 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap)
>                                    !bitmap->successor->disabled);
>   }
>   
> +int bdrv_dirty_bitmap_check(BdrvDirtyBitmap *bitmap, uint32_t flags,
> +                            Error **errp)
> +{
> +    if ((flags & BDRV_BITMAP_BUSY) && bdrv_dirty_bitmap_busy(bitmap)) {
> +        error_setg(errp, "Bitmap '%s' is currently in use by another"
> +                   " operation and cannot be used", bitmap->name);
> +        return -1;
> +    }
> +
> +    if ((flags & BDRV_BITMAP_RO) && bdrv_dirty_bitmap_readonly(bitmap)) {
> +        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
> +                   bitmap->name);
> +        return -1;
> +    }
> +
> +    if ((flags & BDRV_BITMAP_INCONSISTENT) &&
> +        bdrv_dirty_bitmap_inconsistent(bitmap)) {
> +        error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
> +                   bitmap->name);
> +        error_append_hint(errp, "Try block-dirty-bitmap-remove to delete "
> +                          "this bitmap from disk");
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>   /**
>    * Create a successor bitmap destined to replace this bitmap after an operation.
>    * Requires that the bitmap is not user_locked and has no successor.
> @@ -792,15 +819,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>   
>       qemu_mutex_lock(dest->mutex);
>   
> -    if (bdrv_dirty_bitmap_busy(dest)) {
> -        error_setg(errp, "Bitmap '%s' is currently in use by another"
> -        " operation and cannot be modified", dest->name);
> -        goto out;
> -    }
> -
> -    if (bdrv_dirty_bitmap_readonly(dest)) {
> -        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
> -                   dest->name);
> +    if (bdrv_dirty_bitmap_check(dest, BDRV_BITMAP_DEFAULT, errp)) {
>           goto out;
>       }
>   
> diff --git a/blockdev.c b/blockdev.c
> index cbce44877d..5d74479ba7 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2007,11 +2007,7 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
> -        error_setg(errp, "Cannot modify a bitmap in use by another operation");
> -        return;
> -    } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) {
> -        error_setg(errp, "Cannot clear a readonly bitmap");
> +    if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_DEFAULT, errp)) {
>           return;
>       }
>   
> @@ -2056,10 +2052,7 @@ static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
> -        error_setg(errp,
> -                   "Bitmap '%s' is currently in use by another operation"
> -                   " and cannot be enabled", action->name);
> +    if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>           return;
>       }

hmm, preexisting, but seems a very bad idea to enable readonly bitmap.

>   
> @@ -2097,10 +2090,7 @@ static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
> -        error_setg(errp,
> -                   "Bitmap '%s' is currently in use by another operation"
> -                   " and cannot be disabled", action->name);
> +    if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>           return;
>       }

as well as disable too

>   
> @@ -2891,10 +2881,7 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_busy(bitmap)) {
> -        error_setg(errp,
> -                   "Bitmap '%s' is currently in use by another operation and"
> -                   " cannot be removed", name);
> +    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY, errp)) {
>           return;
>       }

yes it's OK to remove inconsistent bitmap..

what about readonly? I don't see a problem with it, readonly just means that we didn't set INUSE
flag for bitmap, but if we are going to remove it, why not? But it most probably will fail,
as qcow2 image is most probably readonly too.

any way, it seems correct to chack only for _BUSY here.

>   
> @@ -2930,13 +2917,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_busy(bitmap)) {
> -        error_setg(errp,
> -                   "Bitmap '%s' is currently in use by another operation"
> -                   " and cannot be cleared", name);
> -        return;
> -    } else if (bdrv_dirty_bitmap_readonly(bitmap)) {
> -        error_setg(errp, "Bitmap '%s' is readonly and cannot be cleared", name);
> +    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
>           return;
>       }
>   
> @@ -2954,10 +2935,7 @@ void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_busy(bitmap)) {
> -        error_setg(errp,
> -                   "Bitmap '%s' is currently in use by another operation"
> -                   " and cannot be enabled", name);
> +    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>           return;
>       }
>   
> @@ -2975,10 +2953,7 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
>           return;
>       }
>   
> -    if (bdrv_dirty_bitmap_busy(bitmap)) {
> -        error_setg(errp,
> -                   "Bitmap '%s' is currently in use by another operation"
> -                   " and cannot be disabled", name);
> +    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>           return;
>       }
>   
> @@ -3551,10 +3526,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
>               bdrv_unref(target_bs);
>               goto out;
>           }
> -        if (bdrv_dirty_bitmap_busy(bmap)) {
> -            error_setg(errp,
> -                       "Bitmap '%s' is currently in use by another operation"
> -                       " and cannot be used for backup", backup->bitmap);
> +        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>               goto out;
>           }
>       }
> @@ -3664,10 +3636,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
>               error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);
>               goto out;
>           }
> -        if (bdrv_dirty_bitmap_busy(bmap)) {
> -            error_setg(errp,
> -                       "Bitmap '%s' is currently in use by another operation"
> -                       " and cannot be used for backup", backup->bitmap);
> +        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>               goto out;
>           }
>       }

hmm, and doing backup on RO bitmap is questionable thing. On one hand - why not, we can.

But we will not be able to abdicate, as we can't modify bitmap.

So, it seems better to restrict it too. If use needs to do an incremental backup from
read-only image, not modifying a bitmap, he should either copy this bitmap (through
merge for example) to a not-persistent one, or he need a way to skip bitmap_abdicate.

> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index b270773f7e..ee98b5014b 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -5,6 +5,16 @@
>   #include "qapi/qapi-types-block-core.h"
>   #include "qemu/hbitmap.h"
>   
> +typedef enum BitmapCheckFlags {
> +    BDRV_BITMAP_BUSY = 1,
> +    BDRV_BITMAP_RO = 2,
> +    BDRV_BITMAP_INCONSISTENT = 4,
> +} BitmapCheckFlags;
> +
> +#define BDRV_BITMAP_DEFAULT (BDRV_BITMAP_BUSY | BDRV_BITMAP_RO |        \
> +                             BDRV_BITMAP_INCONSISTENT)
> +#define BDRV_BITMAP_ALLOW_RO (BDRV_BITMAP_BUSY | BDRV_BITMAP_INCONSISTENT)
> +
>   BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>                                             uint32_t granularity,
>                                             const char *name,
> @@ -24,6 +34,8 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>   void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap);
>   BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
>                                           const char *name);
> +int bdrv_dirty_bitmap_check(BdrvDirtyBitmap *bitmap, uint32_t flags,
> +                            Error **errp);
>   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
>   void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
>   void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
> @@ -93,7 +105,6 @@ 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,
>                                           BdrvDirtyBitmap *bitmap);
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 7057fff242..0fcf897f32 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -274,6 +274,7 @@ static int init_dirty_bitmap_migration(void)
>       BdrvDirtyBitmap *bitmap;
>       DirtyBitmapMigBitmapState *dbms;
>       BdrvNextIterator it;
> +    Error *local_err = NULL;
>   
>       dirty_bitmap_mig_state.bulk_completed = false;
>       dirty_bitmap_mig_state.prev_bs = NULL;
> @@ -301,15 +302,8 @@ static int init_dirty_bitmap_migration(void)
>                   goto fail;
>               }
>   
> -            if (bdrv_dirty_bitmap_busy(bitmap)) {
> -                error_report("Can't migrate a bitmap that is in use by another operation: '%s'",
> -                             bdrv_dirty_bitmap_name(bitmap));
> -                goto fail;
> -            }
> -
> -            if (bdrv_dirty_bitmap_readonly(bitmap)) {
> -                error_report("Can't migrate read-only dirty bitmap: '%s",
> -                             bdrv_dirty_bitmap_name(bitmap));
> +            if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, &local_err)) {
> +                error_report_err(local_err);
>                   goto fail;
>               }
>   
> diff --git a/nbd/server.c b/nbd/server.c
> index 02773e2836..9b87c7f243 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1510,8 +1510,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>               goto fail;
>           }
>   
> -        if (bdrv_dirty_bitmap_busy(bm)) {
> -            error_setg(errp, "Bitmap '%s' is in use", bitmap);
> +        if (bdrv_dirty_bitmap_check(bm, BDRV_BITMAP_ALLOW_RO, errp)) {
>               goto fail;
>           }
>   
> 

and it is the only correct usage of ALLOW_RO, I think.

hmm, doesn't this mean that we should move to "int flags" in BdrvDirtyBitmap?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 3/4] block/dirty-bitmaps: add block_dirty_bitmap_check function

[Vladimir Sementsov-Ogievskiy]

25.02.2019 16:37, Vladimir Sementsov-Ogievskiy wrote:
> 23.02.2019 3:22, John Snow wrote:
>> Instead of checking against busy, inconsistent, or read only directly,
>> use a check function with permissions bits that let us streamline the
>> checks without reproducing them in many places.
>>
>> As a side effect, this adds consistency checks to all the QMP
>> interfaces for bitmap manipulation.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   block/dirty-bitmap.c           | 39 ++++++++++++++++++++-------
>>   blockdev.c                     | 49 +++++++---------------------------
>>   include/block/dirty-bitmap.h   | 13 ++++++++-
>>   migration/block-dirty-bitmap.c | 12 +++------
>>   nbd/server.c                   |  3 +--
>>   5 files changed, 54 insertions(+), 62 deletions(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 4e8f931659..2e7fd81866 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -174,7 +174,7 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap)
>>       return bitmap->successor;
>>   }
>> -bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {
>> +static bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {
>>       return bitmap->busy;
>>   }
>> @@ -235,6 +235,33 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap)
>>                                    !bitmap->successor->disabled);
>>   }
>> +int bdrv_dirty_bitmap_check(BdrvDirtyBitmap *bitmap, uint32_t flags,
>> +                            Error **errp)
>> +{
>> +    if ((flags & BDRV_BITMAP_BUSY) && bdrv_dirty_bitmap_busy(bitmap)) {
>> +        error_setg(errp, "Bitmap '%s' is currently in use by another"
>> +                   " operation and cannot be used", bitmap->name);
>> +        return -1;
>> +    }
>> +
>> +    if ((flags & BDRV_BITMAP_RO) && bdrv_dirty_bitmap_readonly(bitmap)) {
>> +        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
>> +                   bitmap->name);
>> +        return -1;
>> +    }
>> +
>> +    if ((flags & BDRV_BITMAP_INCONSISTENT) &&
>> +        bdrv_dirty_bitmap_inconsistent(bitmap)) {
>> +        error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
>> +                   bitmap->name);
>> +        error_append_hint(errp, "Try block-dirty-bitmap-remove to delete "
>> +                          "this bitmap from disk");
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   /**
>>    * Create a successor bitmap destined to replace this bitmap after an operation.
>>    * Requires that the bitmap is not user_locked and has no successor.
>> @@ -792,15 +819,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>>       qemu_mutex_lock(dest->mutex);
>> -    if (bdrv_dirty_bitmap_busy(dest)) {
>> -        error_setg(errp, "Bitmap '%s' is currently in use by another"
>> -        " operation and cannot be modified", dest->name);
>> -        goto out;
>> -    }
>> -
>> -    if (bdrv_dirty_bitmap_readonly(dest)) {
>> -        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
>> -                   dest->name);
>> +    if (bdrv_dirty_bitmap_check(dest, BDRV_BITMAP_DEFAULT, errp)) {
>>           goto out;
>>       }
>> diff --git a/blockdev.c b/blockdev.c
>> index cbce44877d..5d74479ba7 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2007,11 +2007,7 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
>>           return;
>>       }
>> -    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
>> -        error_setg(errp, "Cannot modify a bitmap in use by another operation");
>> -        return;
>> -    } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) {
>> -        error_setg(errp, "Cannot clear a readonly bitmap");
>> +    if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_DEFAULT, errp)) {
>>           return;
>>       }
>> @@ -2056,10 +2052,7 @@ static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
>>           return;
>>       }
>> -    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
>> -        error_setg(errp,
>> -                   "Bitmap '%s' is currently in use by another operation"
>> -                   " and cannot be enabled", action->name);
>> +    if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>>           return;
>>       }
> 
> hmm, preexisting, but seems a very bad idea to enable readonly bitmap.
> 
>> @@ -2097,10 +2090,7 @@ static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
>>           return;
>>       }
>> -    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
>> -        error_setg(errp,
>> -                   "Bitmap '%s' is currently in use by another operation"
>> -                   " and cannot be disabled", action->name);
>> +    if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>>           return;
>>       }
> 
> as well as disable too
> 
>> @@ -2891,10 +2881,7 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>>           return;
>>       }
>> -    if (bdrv_dirty_bitmap_busy(bitmap)) {
>> -        error_setg(errp,
>> -                   "Bitmap '%s' is currently in use by another operation and"
>> -                   " cannot be removed", name);
>> +    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY, errp)) {
>>           return;
>>       }
> 
> yes it's OK to remove inconsistent bitmap..
> 
> what about readonly? I don't see a problem with it, readonly just means that we didn't set INUSE
> flag for bitmap, but if we are going to remove it, why not? But it most probably will fail,
> as qcow2 image is most probably readonly too.
> 
> any way, it seems correct to chack only for _BUSY here.
> 
>> @@ -2930,13 +2917,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
>>           return;
>>       }
>> -    if (bdrv_dirty_bitmap_busy(bitmap)) {
>> -        error_setg(errp,
>> -                   "Bitmap '%s' is currently in use by another operation"
>> -                   " and cannot be cleared", name);
>> -        return;
>> -    } else if (bdrv_dirty_bitmap_readonly(bitmap)) {
>> -        error_setg(errp, "Bitmap '%s' is readonly and cannot be cleared", name);
>> +    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
>>           return;
>>       }
>> @@ -2954,10 +2935,7 @@ void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
>>           return;
>>       }
>> -    if (bdrv_dirty_bitmap_busy(bitmap)) {
>> -        error_setg(errp,
>> -                   "Bitmap '%s' is currently in use by another operation"
>> -                   " and cannot be enabled", name);
>> +    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>>           return;
>>       }
>> @@ -2975,10 +2953,7 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
>>           return;
>>       }
>> -    if (bdrv_dirty_bitmap_busy(bitmap)) {
>> -        error_setg(errp,
>> -                   "Bitmap '%s' is currently in use by another operation"
>> -                   " and cannot be disabled", name);
>> +    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>>           return;
>>       }
>> @@ -3551,10 +3526,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
>>               bdrv_unref(target_bs);
>>               goto out;
>>           }
>> -        if (bdrv_dirty_bitmap_busy(bmap)) {
>> -            error_setg(errp,
>> -                       "Bitmap '%s' is currently in use by another operation"
>> -                       " and cannot be used for backup", backup->bitmap);
>> +        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>>               goto out;
>>           }
>>       }
>> @@ -3664,10 +3636,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
>>               error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);
>>               goto out;
>>           }
>> -        if (bdrv_dirty_bitmap_busy(bmap)) {
>> -            error_setg(errp,
>> -                       "Bitmap '%s' is currently in use by another operation"
>> -                       " and cannot be used for backup", backup->bitmap);
>> +        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>>               goto out;
>>           }
>>       }
> 
> hmm, and doing backup on RO bitmap is questionable thing. On one hand - why not, we can.
> 
> But we will not be able to abdicate, as we can't modify bitmap.
> 
> So, it seems better to restrict it too. If use needs to do an incremental backup from
> read-only image, not modifying a bitmap, he should either copy this bitmap (through
> merge for example) to a not-persistent one, or he need a way to skip bitmap_abdicate.
> 
>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>> index b270773f7e..ee98b5014b 100644
>> --- a/include/block/dirty-bitmap.h
>> +++ b/include/block/dirty-bitmap.h
>> @@ -5,6 +5,16 @@
>>   #include "qapi/qapi-types-block-core.h"
>>   #include "qemu/hbitmap.h"
>> +typedef enum BitmapCheckFlags {
>> +    BDRV_BITMAP_BUSY = 1,
>> +    BDRV_BITMAP_RO = 2,
>> +    BDRV_BITMAP_INCONSISTENT = 4,
>> +} BitmapCheckFlags;
>> +
>> +#define BDRV_BITMAP_DEFAULT (BDRV_BITMAP_BUSY | BDRV_BITMAP_RO |        \
>> +                             BDRV_BITMAP_INCONSISTENT)
>> +#define BDRV_BITMAP_ALLOW_RO (BDRV_BITMAP_BUSY | BDRV_BITMAP_INCONSISTENT)
>> +
>>   BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>>                                             uint32_t granularity,
>>                                             const char *name,
>> @@ -24,6 +34,8 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>>   void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap);
>>   BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
>>                                           const char *name);
>> +int bdrv_dirty_bitmap_check(BdrvDirtyBitmap *bitmap, uint32_t flags,
>> +                            Error **errp);
>>   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
>>   void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
>>   void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>> @@ -93,7 +105,6 @@ 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,
>>                                           BdrvDirtyBitmap *bitmap);
>> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
>> index 7057fff242..0fcf897f32 100644
>> --- a/migration/block-dirty-bitmap.c
>> +++ b/migration/block-dirty-bitmap.c
>> @@ -274,6 +274,7 @@ static int init_dirty_bitmap_migration(void)
>>       BdrvDirtyBitmap *bitmap;
>>       DirtyBitmapMigBitmapState *dbms;
>>       BdrvNextIterator it;
>> +    Error *local_err = NULL;
>>       dirty_bitmap_mig_state.bulk_completed = false;
>>       dirty_bitmap_mig_state.prev_bs = NULL;
>> @@ -301,15 +302,8 @@ static int init_dirty_bitmap_migration(void)
>>                   goto fail;
>>               }
>> -            if (bdrv_dirty_bitmap_busy(bitmap)) {
>> -                error_report("Can't migrate a bitmap that is in use by another operation: '%s'",
>> -                             bdrv_dirty_bitmap_name(bitmap));
>> -                goto fail;
>> -            }
>> -
>> -            if (bdrv_dirty_bitmap_readonly(bitmap)) {
>> -                error_report("Can't migrate read-only dirty bitmap: '%s",
>> -                             bdrv_dirty_bitmap_name(bitmap));
>> +            if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, &local_err)) {
>> +                error_report_err(local_err);
>>                   goto fail;
>>               }
>> diff --git a/nbd/server.c b/nbd/server.c
>> index 02773e2836..9b87c7f243 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -1510,8 +1510,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>>               goto fail;
>>           }
>> -        if (bdrv_dirty_bitmap_busy(bm)) {
>> -            error_setg(errp, "Bitmap '%s' is in use", bitmap);
>> +        if (bdrv_dirty_bitmap_check(bm, BDRV_BITMAP_ALLOW_RO, errp)) {
>>               goto fail;
>>           }
>>
> 
> and it is the only correct usage of ALLOW_RO, I think.
> 
> hmm, doesn't this mean that we should move to "int flags" in BdrvDirtyBitmap?

and this will gave us cool bdrv_create_dirty_bitmap with flags, to easily create different kind of bitmaps.


And one more important question: we now creating new qapi bitmap status, consisting of several bool fields.
Shouldn't we instead implement it as an array of flags?


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 3/4] block/dirty-bitmaps: add block_dirty_bitmap_check function

[Eric Blake]

On 2/25/19 8:59 AM, Vladimir Sementsov-Ogievskiy wrote:

> 
> And one more important question: we now creating new qapi bitmap status, consisting of several bool fields.
> Shouldn't we instead implement it as an array of flags?

Parsing:

{ "busy": false, "persistent": false, "recording": true }

is easier than parsing:

{ "flags": [ "recording" ] }

(and knowing that unlisted flags "busy", "persistent", and
"inconsistent" are false), or:

{ "flags": [ { "busy": false }, { "persistent": false }, { "recording":
true } ] }

So without more reason why an array of flags would be needed, I don't
see the point for it.

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

Re: [Qemu-devel] [PATCH v2 3/4] block/dirty-bitmaps: add block_dirty_bitmap_check function

[Vladimir Sementsov-Ogievskiy]

25.02.2019 18:07, Eric Blake wrote:
> On 2/25/19 8:59 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>
>> And one more important question: we now creating new qapi bitmap status, consisting of several bool fields.
>> Shouldn't we instead implement it as an array of flags?
> 
> Parsing:
> 
> { "busy": false, "persistent": false, "recording": true }
> 
> is easier than parsing:
> 
> { "flags": [ "recording" ] }
> 
> (and knowing that unlisted flags "busy", "persistent", and
> "inconsistent" are false), or:
> 
> { "flags": [ { "busy": false }, { "persistent": false }, { "recording":
> true } ] }
> 
> So without more reason why an array of flags would be needed, I don't
> see the point for it.
> 

Ok. The only reason I see is upcoming appearance of flags in implementation. But it seems not enough reason.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 3/4] block/dirty-bitmaps: add block_dirty_bitmap_check function

[John Snow]

On 2/25/19 9:59 AM, Vladimir Sementsov-Ogievskiy wrote:
> 25.02.2019 16:37, Vladimir Sementsov-Ogievskiy wrote:
>> 23.02.2019 3:22, John Snow wrote:
>>> Instead of checking against busy, inconsistent, or read only directly,
>>> use a check function with permissions bits that let us streamline the
>>> checks without reproducing them in many places.
>>>
>>> As a side effect, this adds consistency checks to all the QMP
>>> interfaces for bitmap manipulation.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   block/dirty-bitmap.c           | 39 ++++++++++++++++++++-------
>>>   blockdev.c                     | 49 +++++++---------------------------
>>>   include/block/dirty-bitmap.h   | 13 ++++++++-
>>>   migration/block-dirty-bitmap.c | 12 +++------
>>>   nbd/server.c                   |  3 +--
>>>   5 files changed, 54 insertions(+), 62 deletions(-)
>>>
>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>>> index 4e8f931659..2e7fd81866 100644
>>> --- a/block/dirty-bitmap.c
>>> +++ b/block/dirty-bitmap.c
>>> @@ -174,7 +174,7 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap)
>>>       return bitmap->successor;
>>>   }
>>> -bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {
>>> +static bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) {
>>>       return bitmap->busy;
>>>   }
>>> @@ -235,6 +235,33 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap)
>>>                                    !bitmap->successor->disabled);
>>>   }
>>> +int bdrv_dirty_bitmap_check(BdrvDirtyBitmap *bitmap, uint32_t flags,
>>> +                            Error **errp)
>>> +{
>>> +    if ((flags & BDRV_BITMAP_BUSY) && bdrv_dirty_bitmap_busy(bitmap)) {
>>> +        error_setg(errp, "Bitmap '%s' is currently in use by another"
>>> +                   " operation and cannot be used", bitmap->name);
>>> +        return -1;
>>> +    }
>>> +
>>> +    if ((flags & BDRV_BITMAP_RO) && bdrv_dirty_bitmap_readonly(bitmap)) {
>>> +        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
>>> +                   bitmap->name);
>>> +        return -1;
>>> +    }
>>> +
>>> +    if ((flags & BDRV_BITMAP_INCONSISTENT) &&
>>> +        bdrv_dirty_bitmap_inconsistent(bitmap)) {
>>> +        error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
>>> +                   bitmap->name);
>>> +        error_append_hint(errp, "Try block-dirty-bitmap-remove to delete "
>>> +                          "this bitmap from disk");
>>> +        return -1;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   /**
>>>    * Create a successor bitmap destined to replace this bitmap after an operation.
>>>    * Requires that the bitmap is not user_locked and has no successor.
>>> @@ -792,15 +819,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>>>       qemu_mutex_lock(dest->mutex);
>>> -    if (bdrv_dirty_bitmap_busy(dest)) {
>>> -        error_setg(errp, "Bitmap '%s' is currently in use by another"
>>> -        " operation and cannot be modified", dest->name);
>>> -        goto out;
>>> -    }
>>> -
>>> -    if (bdrv_dirty_bitmap_readonly(dest)) {
>>> -        error_setg(errp, "Bitmap '%s' is readonly and cannot be modified",
>>> -                   dest->name);
>>> +    if (bdrv_dirty_bitmap_check(dest, BDRV_BITMAP_DEFAULT, errp)) {
>>>           goto out;
>>>       }
>>> diff --git a/blockdev.c b/blockdev.c
>>> index cbce44877d..5d74479ba7 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -2007,11 +2007,7 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
>>>           return;
>>>       }
>>> -    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
>>> -        error_setg(errp, "Cannot modify a bitmap in use by another operation");
>>> -        return;
>>> -    } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) {
>>> -        error_setg(errp, "Cannot clear a readonly bitmap");
>>> +    if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_DEFAULT, errp)) {
>>>           return;
>>>       }
>>> @@ -2056,10 +2052,7 @@ static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
>>>           return;
>>>       }
>>> -    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
>>> -        error_setg(errp,
>>> -                   "Bitmap '%s' is currently in use by another operation"
>>> -                   " and cannot be enabled", action->name);
>>> +    if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>>>           return;
>>>       }
>>
>> hmm, preexisting, but seems a very bad idea to enable readonly bitmap.
>>

Hmm...

I think I was thinking that you might want to enable or disable the
bitmap prior to remounting RW. I think that's a valid sequence of events.

>>> @@ -2097,10 +2090,7 @@ static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
>>>           return;
>>>       }
>>> -    if (bdrv_dirty_bitmap_busy(state->bitmap)) {
>>> -        error_setg(errp,
>>> -                   "Bitmap '%s' is currently in use by another operation"
>>> -                   " and cannot be disabled", action->name);
>>> +    if (bdrv_dirty_bitmap_check(state->bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>>>           return;
>>>       }
>>
>> as well as disable too
>>
>>> @@ -2891,10 +2881,7 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
>>>           return;
>>>       }
>>> -    if (bdrv_dirty_bitmap_busy(bitmap)) {
>>> -        error_setg(errp,
>>> -                   "Bitmap '%s' is currently in use by another operation and"
>>> -                   " cannot be removed", name);
>>> +    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY, errp)) {
>>>           return;
>>>       }
>>
>> yes it's OK to remove inconsistent bitmap..
>>
>> what about readonly? I don't see a problem with it, readonly just means that we didn't set INUSE
>> flag for bitmap, but if we are going to remove it, why not? But it most probably will fail,
>> as qcow2 image is most probably readonly too.
>>
>> any way, it seems correct to chack only for _BUSY here.
>>

Given my reasoning below, I think maybe I should just make this fail as
early as possible. We KNOW that the image is read only. We KNOW that
this will fail. Best to just make it fail very quickly.

>>> @@ -2930,13 +2917,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
>>>           return;
>>>       }
>>> -    if (bdrv_dirty_bitmap_busy(bitmap)) {
>>> -        error_setg(errp,
>>> -                   "Bitmap '%s' is currently in use by another operation"
>>> -                   " and cannot be cleared", name);
>>> -        return;
>>> -    } else if (bdrv_dirty_bitmap_readonly(bitmap)) {
>>> -        error_setg(errp, "Bitmap '%s' is readonly and cannot be cleared", name);
>>> +    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
>>>           return;
>>>       }
>>> @@ -2954,10 +2935,7 @@ void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
>>>           return;
>>>       }
>>> -    if (bdrv_dirty_bitmap_busy(bitmap)) {
>>> -        error_setg(errp,
>>> -                   "Bitmap '%s' is currently in use by another operation"
>>> -                   " and cannot be enabled", name);
>>> +    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>>>           return;
>>>       }
>>> @@ -2975,10 +2953,7 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
>>>           return;
>>>       }
>>> -    if (bdrv_dirty_bitmap_busy(bitmap)) {
>>> -        error_setg(errp,
>>> -                   "Bitmap '%s' is currently in use by another operation"
>>> -                   " and cannot be disabled", name);
>>> +    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>>>           return;
>>>       }
>>> @@ -3551,10 +3526,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
>>>               bdrv_unref(target_bs);
>>>               goto out;
>>>           }
>>> -        if (bdrv_dirty_bitmap_busy(bmap)) {
>>> -            error_setg(errp,
>>> -                       "Bitmap '%s' is currently in use by another operation"
>>> -                       " and cannot be used for backup", backup->bitmap);
>>> +        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>>>               goto out;
>>>           }
>>>       }
>>> @@ -3664,10 +3636,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
>>>               error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);
>>>               goto out;
>>>           }
>>> -        if (bdrv_dirty_bitmap_busy(bmap)) {
>>> -            error_setg(errp,
>>> -                       "Bitmap '%s' is currently in use by another operation"
>>> -                       " and cannot be used for backup", backup->bitmap);
>>> +        if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>>>               goto out;
>>>           }
>>>       }
>>
>> hmm, and doing backup on RO bitmap is questionable thing. On one hand - why not, we can.
>>
>> But we will not be able to abdicate, as we can't modify bitmap.
>>
>> So, it seems better to restrict it too. If use needs to do an incremental backup from
>> read-only image, not modifying a bitmap, he should either copy this bitmap (through
>> merge for example) to a not-persistent one, or he need a way to skip bitmap_abdicate.
>>

Hmm, you are right... we can't clear the bitmap on success, and we KNOW
we'll be unable to do that from the outset. It's better to fail early.

I'll amend this.

>>> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
>>> index b270773f7e..ee98b5014b 100644
>>> --- a/include/block/dirty-bitmap.h
>>> +++ b/include/block/dirty-bitmap.h
>>> @@ -5,6 +5,16 @@
>>>   #include "qapi/qapi-types-block-core.h"
>>>   #include "qemu/hbitmap.h"
>>> +typedef enum BitmapCheckFlags {
>>> +    BDRV_BITMAP_BUSY = 1,
>>> +    BDRV_BITMAP_RO = 2,
>>> +    BDRV_BITMAP_INCONSISTENT = 4,
>>> +} BitmapCheckFlags;
>>> +
>>> +#define BDRV_BITMAP_DEFAULT (BDRV_BITMAP_BUSY | BDRV_BITMAP_RO |        \
>>> +                             BDRV_BITMAP_INCONSISTENT)
>>> +#define BDRV_BITMAP_ALLOW_RO (BDRV_BITMAP_BUSY | BDRV_BITMAP_INCONSISTENT)
>>> +
>>>   BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>>>                                             uint32_t granularity,
>>>                                             const char *name,
>>> @@ -24,6 +34,8 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>>>   void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap);
>>>   BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
>>>                                           const char *name);
>>> +int bdrv_dirty_bitmap_check(BdrvDirtyBitmap *bitmap, uint32_t flags,
>>> +                            Error **errp);
>>>   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
>>>   void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
>>>   void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>> @@ -93,7 +105,6 @@ 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,
>>>                                           BdrvDirtyBitmap *bitmap);
>>> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
>>> index 7057fff242..0fcf897f32 100644
>>> --- a/migration/block-dirty-bitmap.c
>>> +++ b/migration/block-dirty-bitmap.c
>>> @@ -274,6 +274,7 @@ static int init_dirty_bitmap_migration(void)
>>>       BdrvDirtyBitmap *bitmap;
>>>       DirtyBitmapMigBitmapState *dbms;
>>>       BdrvNextIterator it;
>>> +    Error *local_err = NULL;
>>>       dirty_bitmap_mig_state.bulk_completed = false;
>>>       dirty_bitmap_mig_state.prev_bs = NULL;
>>> @@ -301,15 +302,8 @@ static int init_dirty_bitmap_migration(void)
>>>                   goto fail;
>>>               }
>>> -            if (bdrv_dirty_bitmap_busy(bitmap)) {
>>> -                error_report("Can't migrate a bitmap that is in use by another operation: '%s'",
>>> -                             bdrv_dirty_bitmap_name(bitmap));
>>> -                goto fail;
>>> -            }
>>> -
>>> -            if (bdrv_dirty_bitmap_readonly(bitmap)) {
>>> -                error_report("Can't migrate read-only dirty bitmap: '%s",
>>> -                             bdrv_dirty_bitmap_name(bitmap));
>>> +            if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, &local_err)) {
>>> +                error_report_err(local_err);
>>>                   goto fail;
>>>               }
>>> diff --git a/nbd/server.c b/nbd/server.c
>>> index 02773e2836..9b87c7f243 100644
>>> --- a/nbd/server.c
>>> +++ b/nbd/server.c
>>> @@ -1510,8 +1510,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
>>>               goto fail;
>>>           }
>>> -        if (bdrv_dirty_bitmap_busy(bm)) {
>>> -            error_setg(errp, "Bitmap '%s' is in use", bitmap);
>>> +        if (bdrv_dirty_bitmap_check(bm, BDRV_BITMAP_ALLOW_RO, errp)) {
>>>               goto fail;
>>>           }
>>>
>>
>> and it is the only correct usage of ALLOW_RO, I think.
>>
>> hmm, doesn't this mean that we should move to "int flags" in BdrvDirtyBitmap?
> 
> and this will gave us cool bdrv_create_dirty_bitmap with flags, to easily create different kind of bitmaps.
> 
> 
> And one more important question: we now creating new qapi bitmap status, consisting of several bool fields.
> Shouldn't we instead implement it as an array of flags?
> 
> 

To the above questions I am deferring to Eric's judgement[1], I happen
to like flags as well but I trust Eric's assessment that it's more
complicated than we need in this case.

I also don't want to encourage anybody adding any more :)


[1] en-US spelling suggests we ought to use "judgment", but that's
stupid and I hate it. I will not cater to prescriptivists.

[Qemu-devel] [PATCH v2 4/4] block/dirty-bitmaps: implement inconsistent bit

[John Snow]

Set the inconsistent bit on load instead of rejecting such bitmaps.
There is no way to un-set it; the only option is to delete it.

Obvervations:
- bitmap loading does not need to update the header for in_use bitmaps.
- inconsistent bitmaps don't need to have their data loaded; they're
  glorified corruption sentinels.
- bitmap saving does not need to save inconsistent bitmaps back to disk.
- bitmap reopening DOES need to drop the readonly flag from inconsistent
  bitmaps to allow reopening of qcow2 files with non-qemu-owned bitmaps
  being eventually flushed back to disk.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/qcow2-bitmap.c | 77 +++++++++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 37 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 3ee524da4b..d1cc11da88 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -343,9 +343,15 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
     uint32_t granularity;
     BdrvDirtyBitmap *bitmap = NULL;
 
+    granularity = 1U << bm->granularity_bits;
+    bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
+    if (bitmap == NULL) {
+        goto fail;
+    }
+
     if (bm->flags & BME_FLAG_IN_USE) {
-        error_setg(errp, "Bitmap '%s' is in use", bm->name);
-        goto fail;
+        /* Data is unusable, skip loading it */
+        return bitmap;
     }
 
     ret = bitmap_table_load(bs, &bm->table, &bitmap_table);
@@ -356,12 +362,6 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
         goto fail;
     }
 
-    granularity = 1U << bm->granularity_bits;
-    bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
-    if (bitmap == NULL) {
-        goto fail;
-    }
-
     ret = load_bitmap_data(bs, bitmap_table, bm->table.size, bitmap);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not read bitmap '%s' from image",
@@ -949,6 +949,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
     Qcow2Bitmap *bm;
     GSList *created_dirty_bitmaps = NULL;
     bool header_updated = false;
+    bool needs_update = false;
 
     if (s->nb_bitmaps == 0) {
         /* No bitmaps - nothing to do */
@@ -962,23 +963,27 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
     }
 
     QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-        if (!(bm->flags & BME_FLAG_IN_USE)) {
-            BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
-            if (bitmap == NULL) {
-                goto fail;
-            }
+        BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
+        if (bitmap == NULL) {
+            goto fail;
+        }
 
-            if (!(bm->flags & BME_FLAG_AUTO)) {
-                bdrv_disable_dirty_bitmap(bitmap);
-            }
-            bdrv_dirty_bitmap_set_persistance(bitmap, true);
+        if (bm->flags & BME_FLAG_IN_USE) {
+            bdrv_dirty_bitmap_set_inconsistent(bitmap);
+        } else {
+            /* NB: updated flags only get written if can_write(bs) is true. */
             bm->flags |= BME_FLAG_IN_USE;
-            created_dirty_bitmaps =
-                    g_slist_append(created_dirty_bitmaps, bitmap);
+            needs_update = true;
         }
+        if (!(bm->flags & BME_FLAG_AUTO)) {
+            bdrv_disable_dirty_bitmap(bitmap);
+        }
+        bdrv_dirty_bitmap_set_persistance(bitmap, true);
+        created_dirty_bitmaps =
+            g_slist_append(created_dirty_bitmaps, bitmap);
     }
 
-    if (created_dirty_bitmaps != NULL) {
+    if (needs_update) {
         if (can_write(bs)) {
             /* in_use flags must be updated */
             int ret = update_ext_header_and_dir_in_place(bs, bm_list);
@@ -1112,23 +1117,21 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
     }
 
     QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-        if (!(bm->flags & BME_FLAG_IN_USE)) {
-            BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
-            if (bitmap == NULL) {
-                continue;
-            }
-
-            if (!bdrv_dirty_bitmap_readonly(bitmap)) {
-                error_setg(errp, "Bitmap %s is not readonly but not marked"
-                                 "'IN_USE' in the image. Something went wrong,"
-                                 "all the bitmaps may be corrupted", bm->name);
-                ret = -EINVAL;
-                goto out;
-            }
+        BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
+        if (bitmap == NULL) {
+            continue;
+        }
 
-            bm->flags |= BME_FLAG_IN_USE;
-            ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
+        if (!bdrv_dirty_bitmap_readonly(bitmap)) {
+            error_setg(errp, "Bitmap %s was loaded prior to rw-reopen, but was "
+                       "not marked as readonly. This is a bug, something went "
+                       "wrong. All of the bitmaps may be corrupted", bm->name);
+            ret = -EINVAL;
+            goto out;
         }
+
+        bm->flags |= BME_FLAG_IN_USE;
+        ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
     }
 
     if (ro_dirty_bitmaps != NULL) {
@@ -1424,8 +1427,8 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
         Qcow2Bitmap *bm;
 
         if (!bdrv_dirty_bitmap_get_persistance(bitmap) ||
-            bdrv_dirty_bitmap_readonly(bitmap))
-        {
+            bdrv_dirty_bitmap_readonly(bitmap) ||
+            bdrv_dirty_bitmap_inconsistent(bitmap)) {
             continue;
         }
 
-- 
2.17.2

Re: [Qemu-devel] [PATCH v2 4/4] block/dirty-bitmaps: implement inconsistent bit

[Vladimir Sementsov-Ogievskiy]

23.02.2019 3:22, John Snow wrote:
> Set the inconsistent bit on load instead of rejecting such bitmaps.
> There is no way to un-set it; the only option is to delete it.
> 
> Obvervations:
> - bitmap loading does not need to update the header for in_use bitmaps.
> - inconsistent bitmaps don't need to have their data loaded; they're
>    glorified corruption sentinels.
> - bitmap saving does not need to save inconsistent bitmaps back to disk.
> - bitmap reopening DOES need to drop the readonly flag from inconsistent
>    bitmaps to allow reopening of qcow2 files with non-qemu-owned bitmaps
>    being eventually flushed back to disk.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block/qcow2-bitmap.c | 77 +++++++++++++++++++++++---------------------
>   1 file changed, 40 insertions(+), 37 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 3ee524da4b..d1cc11da88 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -343,9 +343,15 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
>       uint32_t granularity;
>       BdrvDirtyBitmap *bitmap = NULL;
>   
> +    granularity = 1U << bm->granularity_bits;
> +    bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
> +    if (bitmap == NULL) {
> +        goto fail;
> +    }
> +
>       if (bm->flags & BME_FLAG_IN_USE) {
> -        error_setg(errp, "Bitmap '%s' is in use", bm->name);
> -        goto fail;
> +        /* Data is unusable, skip loading it */
> +        return bitmap;
>       }
>   
>       ret = bitmap_table_load(bs, &bm->table, &bitmap_table);
> @@ -356,12 +362,6 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
>           goto fail;
>       }
>   
> -    granularity = 1U << bm->granularity_bits;
> -    bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
> -    if (bitmap == NULL) {
> -        goto fail;
> -    }
> -
>       ret = load_bitmap_data(bs, bitmap_table, bm->table.size, bitmap);
>       if (ret < 0) {
>           error_setg_errno(errp, -ret, "Could not read bitmap '%s' from image",
> @@ -949,6 +949,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>       Qcow2Bitmap *bm;
>       GSList *created_dirty_bitmaps = NULL;
>       bool header_updated = false;
> +    bool needs_update = false;
>   
>       if (s->nb_bitmaps == 0) {
>           /* No bitmaps - nothing to do */
> @@ -962,23 +963,27 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>       }
>   
>       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> -        if (!(bm->flags & BME_FLAG_IN_USE)) {
> -            BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
> -            if (bitmap == NULL) {
> -                goto fail;
> -            }
> +        BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
> +        if (bitmap == NULL) {
> +            goto fail;
> +        }
>   
> -            if (!(bm->flags & BME_FLAG_AUTO)) {
> -                bdrv_disable_dirty_bitmap(bitmap);
> -            }
> -            bdrv_dirty_bitmap_set_persistance(bitmap, true);
> +        if (bm->flags & BME_FLAG_IN_USE) {
> +            bdrv_dirty_bitmap_set_inconsistent(bitmap);
> +        } else {
> +            /* NB: updated flags only get written if can_write(bs) is true. */
>               bm->flags |= BME_FLAG_IN_USE;
> -            created_dirty_bitmaps =
> -                    g_slist_append(created_dirty_bitmaps, bitmap);
> +            needs_update = true;
>           }
> +        if (!(bm->flags & BME_FLAG_AUTO)) {
> +            bdrv_disable_dirty_bitmap(bitmap);
> +        }
> +        bdrv_dirty_bitmap_set_persistance(bitmap, true);

We define persistent bitmaps as which we are going to store.. But we are
not going to store inconsistent bitmaps. So inconsistent bitmaps are not persistent.

> +        created_dirty_bitmaps =
> +            g_slist_append(created_dirty_bitmaps, bitmap);
>       }
>   
> -    if (created_dirty_bitmaps != NULL) {
> +    if (needs_update) {

created_dirty_bitmaps list needed only for setting them readonly, if failed write that
they are IN_USE. So, instead of creating additional variable, better is just not include
IN_USE bitmaps to this list. And it may be renamed like set_in_use_list.

>           if (can_write(bs)) {
>               /* in_use flags must be updated */
>               int ret = update_ext_header_and_dir_in_place(bs, bm_list);
> @@ -1112,23 +1117,21 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>       }
>   
>       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> -        if (!(bm->flags & BME_FLAG_IN_USE)) {
> -            BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
> -            if (bitmap == NULL) {
> -                continue;
> -            }
> -
> -            if (!bdrv_dirty_bitmap_readonly(bitmap)) {
> -                error_setg(errp, "Bitmap %s is not readonly but not marked"
> -                                 "'IN_USE' in the image. Something went wrong,"
> -                                 "all the bitmaps may be corrupted", bm->name);
> -                ret = -EINVAL;
> -                goto out;
> -            }
> +        BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
> +        if (bitmap == NULL) {
> +            continue;
> +        }

>   
> -            bm->flags |= BME_FLAG_IN_USE;
> -            ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
> +        if (!bdrv_dirty_bitmap_readonly(bitmap)) {

so, we'll fail, as we didn't marked inconsistetn bitmaps as readonly above. Should we?
Or we may check here inconsistance directly.

> +            error_setg(errp, "Bitmap %s was loaded prior to rw-reopen, but was "
> +                       "not marked as readonly. This is a bug, something went "
> +                       "wrong. All of the bitmaps may be corrupted", bm->name);

Hmm, your wording covers invalid case when inconsistent bitmap is not IN_USE, and sounds
better for user, though may be less precise (as it may be not one reopen in a sequence..).
Ok for me.

> +            ret = -EINVAL;
> +            goto out;
>           }
> +
> +        bm->flags |= BME_FLAG_IN_USE;
> +        ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
>       }
>   
>       if (ro_dirty_bitmaps != NULL) {
> @@ -1424,8 +1427,8 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>           Qcow2Bitmap *bm;
>   
>           if (!bdrv_dirty_bitmap_get_persistance(bitmap) ||
> -            bdrv_dirty_bitmap_readonly(bitmap))
> -        {
> +            bdrv_dirty_bitmap_readonly(bitmap) ||
> +            bdrv_dirty_bitmap_inconsistent(bitmap)) {
>               continue;
>           }
>   
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 4/4] block/dirty-bitmaps: implement inconsistent bit

[John Snow]

On 2/25/19 11:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> 23.02.2019 3:22, John Snow wrote:
>> Set the inconsistent bit on load instead of rejecting such bitmaps.
>> There is no way to un-set it; the only option is to delete it.
>>
>> Obvervations:
>> - bitmap loading does not need to update the header for in_use bitmaps.
>> - inconsistent bitmaps don't need to have their data loaded; they're
>>    glorified corruption sentinels.
>> - bitmap saving does not need to save inconsistent bitmaps back to disk.
>> - bitmap reopening DOES need to drop the readonly flag from inconsistent
>>    bitmaps to allow reopening of qcow2 files with non-qemu-owned bitmaps
>>    being eventually flushed back to disk.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   block/qcow2-bitmap.c | 77 +++++++++++++++++++++++---------------------
>>   1 file changed, 40 insertions(+), 37 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 3ee524da4b..d1cc11da88 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -343,9 +343,15 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
>>       uint32_t granularity;
>>       BdrvDirtyBitmap *bitmap = NULL;
>>   
>> +    granularity = 1U << bm->granularity_bits;
>> +    bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
>> +    if (bitmap == NULL) {
>> +        goto fail;
>> +    }
>> +
>>       if (bm->flags & BME_FLAG_IN_USE) {
>> -        error_setg(errp, "Bitmap '%s' is in use", bm->name);
>> -        goto fail;
>> +        /* Data is unusable, skip loading it */
>> +        return bitmap;
>>       }
>>   
>>       ret = bitmap_table_load(bs, &bm->table, &bitmap_table);
>> @@ -356,12 +362,6 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
>>           goto fail;
>>       }
>>   
>> -    granularity = 1U << bm->granularity_bits;
>> -    bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
>> -    if (bitmap == NULL) {
>> -        goto fail;
>> -    }
>> -
>>       ret = load_bitmap_data(bs, bitmap_table, bm->table.size, bitmap);
>>       if (ret < 0) {
>>           error_setg_errno(errp, -ret, "Could not read bitmap '%s' from image",
>> @@ -949,6 +949,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>       Qcow2Bitmap *bm;
>>       GSList *created_dirty_bitmaps = NULL;
>>       bool header_updated = false;
>> +    bool needs_update = false;
>>   
>>       if (s->nb_bitmaps == 0) {
>>           /* No bitmaps - nothing to do */
>> @@ -962,23 +963,27 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>       }
>>   
>>       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>> -        if (!(bm->flags & BME_FLAG_IN_USE)) {
>> -            BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
>> -            if (bitmap == NULL) {
>> -                goto fail;
>> -            }
>> +        BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
>> +        if (bitmap == NULL) {
>> +            goto fail;
>> +        }
>>   
>> -            if (!(bm->flags & BME_FLAG_AUTO)) {
>> -                bdrv_disable_dirty_bitmap(bitmap);
>> -            }
>> -            bdrv_dirty_bitmap_set_persistance(bitmap, true);
>> +        if (bm->flags & BME_FLAG_IN_USE) {
>> +            bdrv_dirty_bitmap_set_inconsistent(bitmap);
>> +        } else {
>> +            /* NB: updated flags only get written if can_write(bs) is true. */
>>               bm->flags |= BME_FLAG_IN_USE;
>> -            created_dirty_bitmaps =
>> -                    g_slist_append(created_dirty_bitmaps, bitmap);
>> +            needs_update = true;
>>           }
>> +        if (!(bm->flags & BME_FLAG_AUTO)) {
>> +            bdrv_disable_dirty_bitmap(bitmap);
>> +        }
>> +        bdrv_dirty_bitmap_set_persistance(bitmap, true);
> 
> We define persistent bitmaps as which we are going to store.. But we are
> not going to store inconsistent bitmaps. So inconsistent bitmaps are not persistent.
> 

I changed the wording for v3.

>> +        created_dirty_bitmaps =
>> +            g_slist_append(created_dirty_bitmaps, bitmap);
>>       }
>>   
>> -    if (created_dirty_bitmaps != NULL) {
>> +    if (needs_update) {
> 
> created_dirty_bitmaps list needed only for setting them readonly, if failed write that
> they are IN_USE. So, instead of creating additional variable, better is just not include
> IN_USE bitmaps to this list. And it may be renamed like set_in_use_list.
> 

If I don't add them to the list, then one of the bitmaps in the list
fails, I don't know which bitmap to remove when we go to the error exit.

>>           if (can_write(bs)) {
>>               /* in_use flags must be updated */
>>               int ret = update_ext_header_and_dir_in_place(bs, bm_list);
>> @@ -1112,23 +1117,21 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>>       }
>>   
>>       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>> -        if (!(bm->flags & BME_FLAG_IN_USE)) {
>> -            BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
>> -            if (bitmap == NULL) {
>> -                continue;
>> -            }
>> -
>> -            if (!bdrv_dirty_bitmap_readonly(bitmap)) {
>> -                error_setg(errp, "Bitmap %s is not readonly but not marked"
>> -                                 "'IN_USE' in the image. Something went wrong,"
>> -                                 "all the bitmaps may be corrupted", bm->name);
>> -                ret = -EINVAL;
>> -                goto out;
>> -            }
>> +        BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
>> +        if (bitmap == NULL) {
>> +            continue;
>> +        }
> 
>>   
>> -            bm->flags |= BME_FLAG_IN_USE;
>> -            ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
>> +        if (!bdrv_dirty_bitmap_readonly(bitmap)) {
> 
> so, we'll fail, as we didn't marked inconsistetn bitmaps as readonly above. Should we?
> Or we may check here inconsistance directly.
> 

OH! I didn't mean to do this -- I want to mark all inconsistent bitmaps
as readonly, yes. I will fix this.

>> +            error_setg(errp, "Bitmap %s was loaded prior to rw-reopen, but was "
>> +                       "not marked as readonly. This is a bug, something went "
>> +                       "wrong. All of the bitmaps may be corrupted", bm->name);
> 
> Hmm, your wording covers invalid case when inconsistent bitmap is not IN_USE, and sounds
> better for user, though may be less precise (as it may be not one reopen in a sequence..).
> Ok for me.
> 

With all inconsistent bitmaps marked as readonly, this shouldn't ever
actually trigger. All persistent bitmaps on a drive that hits
reopen_rw_hint ought to be readonly, right?

That's my assumption; that we should never see this message.

>> +            ret = -EINVAL;
>> +            goto out;
>>           }
>> +
>> +        bm->flags |= BME_FLAG_IN_USE;
>> +        ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
>>       }
>>   
>>       if (ro_dirty_bitmaps != NULL) {
>> @@ -1424,8 +1427,8 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>           Qcow2Bitmap *bm;
>>   
>>           if (!bdrv_dirty_bitmap_get_persistance(bitmap) ||
>> -            bdrv_dirty_bitmap_readonly(bitmap))
>> -        {
>> +            bdrv_dirty_bitmap_readonly(bitmap) ||
>> +            bdrv_dirty_bitmap_inconsistent(bitmap)) {
>>               continue;
>>           }
>>   
>>
> 
> 

Sending a V3 for further critique, but freeze is soon.

Re: [Qemu-devel] [PATCH v2 4/4] block/dirty-bitmaps: implement inconsistent bit

[Vladimir Sementsov-Ogievskiy]

28.02.2019 2:48, John Snow wrote:
> 
> 
> On 2/25/19 11:21 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 23.02.2019 3:22, John Snow wrote:
>>> Set the inconsistent bit on load instead of rejecting such bitmaps.
>>> There is no way to un-set it; the only option is to delete it.
>>>
>>> Obvervations:
>>> - bitmap loading does not need to update the header for in_use bitmaps.
>>> - inconsistent bitmaps don't need to have their data loaded; they're
>>>     glorified corruption sentinels.
>>> - bitmap saving does not need to save inconsistent bitmaps back to disk.
>>> - bitmap reopening DOES need to drop the readonly flag from inconsistent
>>>     bitmaps to allow reopening of qcow2 files with non-qemu-owned bitmaps
>>>     being eventually flushed back to disk.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>    block/qcow2-bitmap.c | 77 +++++++++++++++++++++++---------------------
>>>    1 file changed, 40 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>> index 3ee524da4b..d1cc11da88 100644
>>> --- a/block/qcow2-bitmap.c
>>> +++ b/block/qcow2-bitmap.c
>>> @@ -343,9 +343,15 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
>>>        uint32_t granularity;
>>>        BdrvDirtyBitmap *bitmap = NULL;
>>>    
>>> +    granularity = 1U << bm->granularity_bits;
>>> +    bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
>>> +    if (bitmap == NULL) {
>>> +        goto fail;
>>> +    }
>>> +
>>>        if (bm->flags & BME_FLAG_IN_USE) {
>>> -        error_setg(errp, "Bitmap '%s' is in use", bm->name);
>>> -        goto fail;
>>> +        /* Data is unusable, skip loading it */
>>> +        return bitmap;
>>>        }
>>>    
>>>        ret = bitmap_table_load(bs, &bm->table, &bitmap_table);
>>> @@ -356,12 +362,6 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
>>>            goto fail;
>>>        }
>>>    
>>> -    granularity = 1U << bm->granularity_bits;
>>> -    bitmap = bdrv_create_dirty_bitmap(bs, granularity, bm->name, errp);
>>> -    if (bitmap == NULL) {
>>> -        goto fail;
>>> -    }
>>> -
>>>        ret = load_bitmap_data(bs, bitmap_table, bm->table.size, bitmap);
>>>        if (ret < 0) {
>>>            error_setg_errno(errp, -ret, "Could not read bitmap '%s' from image",
>>> @@ -949,6 +949,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>>        Qcow2Bitmap *bm;
>>>        GSList *created_dirty_bitmaps = NULL;
>>>        bool header_updated = false;
>>> +    bool needs_update = false;
>>>    
>>>        if (s->nb_bitmaps == 0) {
>>>            /* No bitmaps - nothing to do */
>>> @@ -962,23 +963,27 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>>        }
>>>    
>>>        QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>> -        if (!(bm->flags & BME_FLAG_IN_USE)) {
>>> -            BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
>>> -            if (bitmap == NULL) {
>>> -                goto fail;
>>> -            }
>>> +        BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
>>> +        if (bitmap == NULL) {
>>> +            goto fail;
>>> +        }
>>>    
>>> -            if (!(bm->flags & BME_FLAG_AUTO)) {
>>> -                bdrv_disable_dirty_bitmap(bitmap);
>>> -            }
>>> -            bdrv_dirty_bitmap_set_persistance(bitmap, true);
>>> +        if (bm->flags & BME_FLAG_IN_USE) {
>>> +            bdrv_dirty_bitmap_set_inconsistent(bitmap);
>>> +        } else {
>>> +            /* NB: updated flags only get written if can_write(bs) is true. */
>>>                bm->flags |= BME_FLAG_IN_USE;
>>> -            created_dirty_bitmaps =
>>> -                    g_slist_append(created_dirty_bitmaps, bitmap);
>>> +            needs_update = true;
>>>            }
>>> +        if (!(bm->flags & BME_FLAG_AUTO)) {
>>> +            bdrv_disable_dirty_bitmap(bitmap);
>>> +        }
>>> +        bdrv_dirty_bitmap_set_persistance(bitmap, true);
>>
>> We define persistent bitmaps as which we are going to store.. But we are
>> not going to store inconsistent bitmaps. So inconsistent bitmaps are not persistent.
>>
> 
> I changed the wording for v3.
> 
>>> +        created_dirty_bitmaps =
>>> +            g_slist_append(created_dirty_bitmaps, bitmap);
>>>        }
>>>    
>>> -    if (created_dirty_bitmaps != NULL) {
>>> +    if (needs_update) {
>>
>> created_dirty_bitmaps list needed only for setting them readonly, if failed write that
>> they are IN_USE. So, instead of creating additional variable, better is just not include
>> IN_USE bitmaps to this list. And it may be renamed like set_in_use_list.
>>
> 
> If I don't add them to the list, then one of the bitmaps in the list
> fails, I don't know which bitmap to remove when we go to the error exit.


Oops, you are right.

> 
>>>            if (can_write(bs)) {
>>>                /* in_use flags must be updated */
>>>                int ret = update_ext_header_and_dir_in_place(bs, bm_list);
>>> @@ -1112,23 +1117,21 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>>>        }
>>>    
>>>        QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>> -        if (!(bm->flags & BME_FLAG_IN_USE)) {
>>> -            BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
>>> -            if (bitmap == NULL) {
>>> -                continue;
>>> -            }
>>> -
>>> -            if (!bdrv_dirty_bitmap_readonly(bitmap)) {
>>> -                error_setg(errp, "Bitmap %s is not readonly but not marked"
>>> -                                 "'IN_USE' in the image. Something went wrong,"
>>> -                                 "all the bitmaps may be corrupted", bm->name);
>>> -                ret = -EINVAL;
>>> -                goto out;
>>> -            }
>>> +        BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
>>> +        if (bitmap == NULL) {
>>> +            continue;
>>> +        }
>>
>>>    
>>> -            bm->flags |= BME_FLAG_IN_USE;
>>> -            ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
>>> +        if (!bdrv_dirty_bitmap_readonly(bitmap)) {
>>
>> so, we'll fail, as we didn't marked inconsistetn bitmaps as readonly above. Should we?
>> Or we may check here inconsistance directly.
>>
> 
> OH! I didn't mean to do this -- I want to mark all inconsistent bitmaps
> as readonly, yes. I will fix this.
> 
>>> +            error_setg(errp, "Bitmap %s was loaded prior to rw-reopen, but was "
>>> +                       "not marked as readonly. This is a bug, something went "
>>> +                       "wrong. All of the bitmaps may be corrupted", bm->name);
>>
>> Hmm, your wording covers invalid case when inconsistent bitmap is not IN_USE, and sounds
>> better for user, though may be less precise (as it may be not one reopen in a sequence..).
>> Ok for me.
>>
> 
> With all inconsistent bitmaps marked as readonly, this shouldn't ever
> actually trigger. All persistent bitmaps on a drive that hits
> reopen_rw_hint ought to be readonly, right?

not necessary I think. Some bitmaps may be created through qmp command, they will not be readonly,
as they are not flushed to storage.

But it of course doesn't lead to that message. Yes it should never happen I think, so it is worded in
manner "This is a bug". On the other hand, assume that someone illegally modifies image, while
Qemu is running and clear IN_USE bit. In this case we'll see that message.

> 
> That's my assumption; that we should never see this message.
> 
>>> +            ret = -EINVAL;
>>> +            goto out;
>>>            }
>>> +
>>> +        bm->flags |= BME_FLAG_IN_USE;
>>> +        ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
>>>        }
>>>    
>>>        if (ro_dirty_bitmaps != NULL) {
>>> @@ -1424,8 +1427,8 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>>            Qcow2Bitmap *bm;
>>>    
>>>            if (!bdrv_dirty_bitmap_get_persistance(bitmap) ||
>>> -            bdrv_dirty_bitmap_readonly(bitmap))
>>> -        {
>>> +            bdrv_dirty_bitmap_readonly(bitmap) ||
>>> +            bdrv_dirty_bitmap_inconsistent(bitmap)) {
>>>                continue;
>>>            }
>>>    
>>>
>>
>>
> 
> Sending a V3 for further critique, but freeze is soon.
> 


-- 
Best regards,
Vladimir