Re: [Qemu-devel] [PATCH v2 5/6] qapi: add block-dirty-bitmap-merge

[Vladimir Sementsov-Ogievskiy]

20.09.2018 22:40, Eric Blake wrote:
> [reviving an old patch]
>
> On 1/16/18 6:54 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>   qapi/block-core.json         | 38 
>> ++++++++++++++++++++++++++++++++++++++
>>   include/block/dirty-bitmap.h |  2 ++
>>   block/dirty-bitmap.c         | 18 ++++++++++++++++++
>>   blockdev.c                   | 30 ++++++++++++++++++++++++++++++
>>   4 files changed, 88 insertions(+)
>>
>
> We've already hit a couple issues with this patch mentioned in other 
> threads:
>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index b3851ee760..9f9cfa0a44 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1604,6 +1604,20 @@
>>               '*persistent': 'bool', '*autoload': 'bool' } }
>>     ##
>> +# @BlockDirtyBitmapMerge:
>> +#
>> +# @node: name of device/node which the bitmap is tracking
>> +#
>> +# @dst_name: name of the destination dirty bitmap
>> +#
>> +# @src_name: name of the source dirty bitmap
>
> Naming choice (prefer - over _):
> https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg02352.html
>
>> +++ b/block/dirty-bitmap.c
>> @@ -699,3 +699,21 @@ char *bdrv_dirty_bitmap_sha256(const 
>> BdrvDirtyBitmap *bitmap, Error **errp)
>>   {
>>       return hbitmap_sha256(bitmap->bitmap, errp);
>>   }
>> +
>> +void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const 
>> BdrvDirtyBitmap *src,
>> +                             Error **errp)
>> +{
>> +    /* only bitmaps from one bds are supported */
>> +    assert(dest->mutex == src->mutex);
>> +
>> +    qemu_mutex_lock(dest->mutex);
>> +
>> +    assert(bdrv_dirty_bitmap_enabled(dest));
>
> Merging to a disabled bitmap is desirable:
> https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg02358.html
>
>> + assert(!bdrv_dirty_bitmap_readonly(dest));
>> +
>> +    if (!hbitmap_merge(dest->bitmap, src->bitmap)) {
>
> And here's another one I ran into. hbitmap_merge() does NOT properly 
> update hbitmap_count(), which means statistics about the number of 
> bits set in the bitmap, as visible through QMP "query-block", are 
> inaccurate after a merge.
>
> Check out hbitmap_set() and hbitmap_reset(), which carefully recompute 
> hb->count using hb_count_between() as part of flipping bits. But since 
> hbitmap_merge() fails to recompute bits, various other aspects of the 
> code go haywire (for example, code that uses hbitmap_empty() or 
> hbitmap_count()==0 to short-circuit operations [and hbitmap_merge() is 
> one such caller] makes the wrong choice on a bitmap that started life 
> empty, and had a non-empty bitmap merged in, because hb->count is 
> still 0).
>
> I think that x-debug-block-dirty-bitmap-sha256 should be enhanced to 
> output count as well as the checksum of the bitmap contents, and then 
> that we need iotests coverage of x-block-dirty-bitmap-merge, complete 
> with a check that the count is properly updated. iotests 165, 169, 
> 176, 199 all perform bitmap checksum validations, but so far none of 
> them perform a bitmap merge.  Be careful in writing any such test that 
> we don't re-introduce any sensitivities on 32- vs. 64-bit or 
> endianness in our checksum computation (see commit 2807746f for 
> comparison).
>

or at least we should check count in tests/test-hbitmap..

About bitmap api: can we now switch naming style from '_' to '-' and 
drop x- prefixes? Or we are waiting for your libvirt API proposal 
acceptance?

-- 
Best regards,
Vladimir