Re: [Qemu-block] [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 <address@hidden>
>> ---
>>   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