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

[Vladimir Sementsov-Ogievskiy]

20.02.2019 1:00, John Snow wrote:
> 
> 
> 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.

I agree.


-- 
Best regards,
Vladimir