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

[John Snow <jsnow@redhat.com>]

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