Re: [Qemu-block] [PATCH v5 04/10] block: Support meta dirty bitmap

[John Snow]

On 06/22/2016 11:53 AM, Max Reitz wrote:
> On 03.06.2016 06:32, Fam Zheng wrote:
>> The added group of operations enables tracking of the changed bits in
>> the dirty bitmap.
>>
>> Signed-off-by: Fam Zheng <address@hidden>
>> ---
>>  block/dirty-bitmap.c         | 52 
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  include/block/dirty-bitmap.h |  9 ++++++++
>>  2 files changed, 61 insertions(+)
>>
>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
>> index 628b77c..9c53c56 100644
>> --- a/block/dirty-bitmap.c
>> +++ b/block/dirty-bitmap.c
>> @@ -38,6 +38,7 @@
>>   */
>>  struct BdrvDirtyBitmap {
>>      HBitmap *bitmap;            /* Dirty sector bitmap implementation */
>> +    HBitmap *meta;              /* Meta dirty bitmap */
>>      BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
>>      char *name;                 /* Optional non-empty unique ID */
>>      int64_t size;               /* Size of the bitmap (Number of sectors) */
>> @@ -103,6 +104,56 @@ BdrvDirtyBitmap 
>> *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>>      return bitmap;
>>  }
>>  
>> +/* bdrv_create_meta_dirty_bitmap
>> + *
>> + * Create a meta dirty bitmap that tracks the changes of bits in @bitmap. 
>> I.e.
>> + * when a dirty status bit in @bitmap is changed (either from reset to set 
>> or
>> + * the other way around), its respective meta dirty bitmap bit will be 
>> marked
>> + * dirty as well.
> 
> Not wrong, but I'd like a note here that this is not an
> when-and-only-when relationship, i.e. that bits in the meta bitmap may
> be set even without the tracked bits in the dirty bitmap having changed.
> 

How?

You mean, if the caller manually starts setting things in the meta
bitmap object?

That's their fault then, isn't it?

> Maybe this should be mentioned somewhere in patch 2, too. Or maybe only
> in patch 2.
> 
>> + *
>> + * @bitmap: the block dirty bitmap for which to create a meta dirty bitmap.
>> + * @chunk_size: how many bytes of bitmap data does each bit in the meta 
>> bitmap
>> + * track.
>> + */
>> +void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>> +                                   int chunk_size)
>> +{
>> +    assert(!bitmap->meta);
>> +    bitmap->meta = hbitmap_create_meta(bitmap->bitmap,
>> +                                       chunk_size * BITS_PER_BYTE);
>> +}
>> +
>> +void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>> +{
>> +    assert(bitmap->meta);
>> +    hbitmap_free_meta(bitmap->bitmap);
>> +    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. */
> 
> We could also multiply gran by the granularity of the meta bitmap. Each
> bit of the meta bitmap tracks at least eight bits of the dirty bitmap,
> so we're calling hbitmap_get() at least eight times as often as
> necessary here.
> 
> Or we just use int gran = hbitmap_granularity(bitmap->meta);.
> 
> Not exactly wrong, though, so:
> 
> Reviewed-by: Max Reitz <address@hidden>
> 
>> +    for (i = sector; i < sector + nb_sectors; i += gran) {
>> +        if (hbitmap_get(bitmap->meta, i)) {
>> +            return true;
>> +        }
>> +    }
>> +    return false;
>> +}
>