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.
--js