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 <js...@redhat.com>
>>> ---
>>> 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
