Re: [Qemu-block] [RFC PATCH 2/2] block/dirty-bitmap: implement inconsistent bit

[John Snow]

On 2/18/19 1:13 PM, Vladimir Sementsov-Ogievskiy wrote:
> 14.02.2019 2:36, John Snow wrote:
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>   block/dirty-bitmap.c         | 15 +++++++++++++
>>   block/qcow2-bitmap.c         | 42 ++++++++++++++++++-----------------
>>   blockdev.c                   | 43 ++++++++++++++++++++++++++++++++++++
>>   include/block/dirty-bitmap.h |  1 +
>>   4 files changed, 81 insertions(+), 20 deletions(-)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index b1879d7fbd..06d8ee0d79 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -589,6 +589,7 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, 
>> HBitmap **out)
>>           *out = backup;
>>       }
>>       bdrv_dirty_bitmap_unlock(bitmap);
>> +    bdrv_dirty_bitmap_set_inconsistent(bitmap, false);
>>   }
>>   
>>   void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup)
>> @@ -776,6 +777,13 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap 
>> *bitmap,
>>       return hbitmap_next_dirty_area(bitmap->bitmap, offset, bytes);
>>   }
>>   
>> +void bdrv_dirty_bitmap_add_inconsistent_hint(Error **errp)
>> +{
>> +    error_append_hint(errp, "Try block-dirty-bitmap-clear to mark this "
>> +                      "bitmap consistent again, or 
>> block-dirty-bitmap-remove "
>> +                      "to delete it.");
> 
> bitmaps created by libvirt (or somebody) are related to some checkpoint. And 
> their name is
> probably (Eric?) related to this checkpoint too. So, clear will never make 
> them consistent..
> Only clear :)
> 
> So, I don't like idea of clearing in-use bitmaps.
> 
>> +}
>> +
>>   void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap 
>> *src,
>>                                HBitmap **backup, Error **errp)
>>   {
>> @@ -798,6 +806,13 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, 
>> const BdrvDirtyBitmap *src,
>>           goto out;
>>       }
>>   
>> +    if (bdrv_dirty_bitmap_inconsistent(dest)) {
>> +        error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used as"
>> +                   " a merge target", dest->name);
>> +        bdrv_dirty_bitmap_add_inconsistent_hint(errp);
>> +        goto out;
>> +    }
> 
> I think, we need common predicate which will combine busy and inconsistent, 
> as almost in all cases
> we need both to be false to do any operation.
> 
> bdrv_dirty_bitmap_usable() ? :)
> 
> and pass errp to this helper.
> 
> Actually, we already need it, to fill errp, which we almost always fill in 
> the same manner.
> 
>> +
>>       if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) {
>>           error_setg(errp, "Bitmaps are incompatible and can't be merged");
>>           goto out;
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 3ee524da4b..9bd8bc417f 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
> 
> hmm, I also think we should report our deprecated status as locked for 
> inconsistent bitmaps..
> 
> 

Though we're trying to deprecate the field altogether, I *could* add a
special status flag that makes it unambiguous. This will catch the
attention of anyone using the old API.

--js