Re: [Qemu-devel] [PATCH 08/13] block: Support meta dirty bitmap

[John Snow <jsnow@redhat.com>]

On 01/20/2016 01:07 AM, Fam Zheng wrote:
> On Thu, 01/07 14:30, John Snow wrote:
>>> +void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>> +{
>>> +    assert(bitmap->meta);
>>> +    hbitmap_free(bitmap->meta);
>>
>> This leaves a dangling pointer inside the Hbitmap, no?
> 
> Yes, will fix.
> 
>>
>>> +    bitmap->meta = NULL;
>>> +}
>>> +
>>> +int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
>>> +                               BdrvDirtyBitmap *bitmap, int64_t sector,
>>> +                               int nb_sectors)
>>> +{
>>> +    uint64_t i;
>>> +    int gran = bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
>>> +
>>> +    /* To optimize: we can make hbitmap to internally check the range in a
>>> +     * coarse level, or at least do it word by word. */
>>> +    for (i = sector; i < sector + nb_sectors; i += gran) {
>>> +        if (hbitmap_get(bitmap->meta, i)) {
>>> +            return true;
>>> +        }
>>> +    }
>>> +    return false;
>>> +}
>>> +
>>
>> In essence get_meta() is a greedy algorithm that simply returns true if
>> anything is set between [sector, sector + nb_sectors], yes?
>>
>> Is this more useful than just using an iterator directly on the
>> meta-bitmap?
>>
>> I haven't finished reading but, I imagine that:
>>
>> - If we need to check to see what is dirty specifically, we can just use
>> the iterator. If the iterator doesn't return anything, we know it's
>> empty. If it does return, we know exactly what's dirty.
>> - If we need to explicitly check for emptiness in general, we can use
>> the internal popcount.
>>
>>
>> I'm not sure when a 'dirty range bool' will be explicitly useful all by
>> itself, but maybe that becomes obvious later.
> 
> It's for the meta bitmap user to decide. In the case of persistent dirty bitmap
> driver, I simply check whether the range of write request is meta-dirty, and
> write the corresponding dirty bitmap range accordingly, rather than splitting
> one write req into potentially multiple bit ranges that are meta-dirty. I think
> this is reasonable, hence the interface.
> 

OK, I think I see what the use case is, thanks.