Re: [Qemu-devel] [PATCH RFC v2 4/8] block: add dirty-dirty bitmaps

[Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>]

On 11.02.2015 00:30, John Snow wrote:
> I had hoped it wouldn't come to this :)
>
> On 01/27/2015 05:56 AM, Vladimir Sementsov-Ogievskiy wrote:
>> A dirty-dirty bitmap is a dirty bitmap for BdrvDirtyBitmap. It tracks
>> set/unset changes of BdrvDirtyBitmap.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>> ---
>>   block.c               | 44 
>> ++++++++++++++++++++++++++++++++++++++++++++
>>   include/block/block.h |  5 +++++
>>   2 files changed, 49 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index 9860fc1..8ab724b 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -53,6 +53,7 @@
>>
>>   struct BdrvDirtyBitmap {
>>       HBitmap *bitmap;
>> +    HBitmap *dirty_dirty_bitmap;
>
> Just opinions: Maybe we can call it the "meta_bitmap" to help keep the 
> name less confusing, and accompany it with a good structure comment 
> here to explain what the heck it's for.
>
> If you feel that's a good idea; s/dirty_dirty_/meta_/g below.
>
> Regardless, let's make sure this patch adds documentation for it.
No objections. If everyone is happy with meta_bitmaps - I'll  use this 
name. Dirty-dirty bitmaps are just more fanny, I think ;)
>
>>       BdrvDirtyBitmap *originator;
>>       int64_t size;
>>       int64_t granularity;
>> @@ -5373,6 +5374,30 @@ BdrvDirtyBitmap 
>> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>>       return originator;
>>   }
>>
>> +HBitmap *bdrv_create_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>> +                                        uint64_t granularity)
>> +{
>> +    uint64_t sector_granularity;
>> +
>> +    assert((granularity & (granularity - 1)) == 0);
>> +
>> +    granularity *= 8 * bitmap->granularity;
>> +    sector_granularity = granularity >> BDRV_SECTOR_BITS;
>> +    assert(sector_granularity);
>> +
>> +    bitmap->dirty_dirty_bitmap =
>> +        hbitmap_alloc(bitmap->size, ffsll(sector_granularity) - 1);
>> +
>> +    return bitmap->dirty_dirty_bitmap;
>> +}
>> +
>> +void bdrv_release_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>> +{
>> +    if (bitmap->dirty_dirty_bitmap) {
>> +        hbitmap_free(bitmap->dirty_dirty_bitmap);
>> +        bitmap->dirty_dirty_bitmap = NULL;
>> +    }
>> +}
>>
>>   void bdrv_print_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>   {
>> @@ -5447,6 +5472,9 @@ void bdrv_release_dirty_bitmap(BlockDriverState 
>> *bs, BdrvDirtyBitmap *bitmap)
>>           if (bm == bitmap) {
>>               QLIST_REMOVE(bitmap, list);
>>               hbitmap_free(bitmap->bitmap);
>> +            if (bitmap->dirty_dirty_bitmap) {
>> +                hbitmap_free(bitmap->dirty_dirty_bitmap);
>> +            }
>>               g_free(bitmap->name);
>>               g_free(bitmap);
>>               return;
>> @@ -5534,6 +5562,10 @@ void bdrv_set_dirty_bitmap(BlockDriverState 
>> *bs, BdrvDirtyBitmap *bitmap,
>>   {
>>       if (bitmap->enabled) {
>>           hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
>> +
>> +        if (bitmap->dirty_dirty_bitmap) {
>> +            hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, 
>> nr_sectors);
>> +        }
>>       }
>>   }
>>
>> @@ -5541,6 +5573,9 @@ void bdrv_reset_dirty_bitmap(BlockDriverState 
>> *bs, BdrvDirtyBitmap *bitmap,
>>                                int64_t cur_sector, int nr_sectors)
>>   {
>>       hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
>> +    if (bitmap->dirty_dirty_bitmap) {
>> +        hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, 
>> nr_sectors);
>> +    }
>>   }
>>
>>   /**
>> @@ -5550,6 +5585,9 @@ void bdrv_reset_dirty_bitmap(BlockDriverState 
>> *bs, BdrvDirtyBitmap *bitmap,
>>   void bdrv_clear_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap 
>> *bitmap)
>>   {
>>       hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
>> +    if (bitmap->dirty_dirty_bitmap) {
>> +        hbitmap_set(bitmap->dirty_dirty_bitmap, 0, bitmap->size);
>> +    }
>>   }
>>
>>   const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap)
>> @@ -5597,6 +5635,9 @@ static void bdrv_set_dirty(BlockDriverState 
>> *bs, int64_t cur_sector,
>>               continue;
>>           }
>>           hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
>> +        if (bitmap->dirty_dirty_bitmap) {
>> +            hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, 
>> nr_sectors);
>> +        }
>>       }
>>   }
>>
>> @@ -5606,6 +5647,9 @@ static void bdrv_reset_dirty(BlockDriverState 
>> *bs, int64_t cur_sector,
>>       BdrvDirtyBitmap *bitmap;
>>       QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
>>           hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
>> +        if (bitmap->dirty_dirty_bitmap) {
>> +            hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, 
>> nr_sectors);
>> +        }
>>       }
>>   }
>>
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 0890cd2..648b0a9 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -4,6 +4,7 @@
>>   #include "block/aio.h"
>>   #include "qemu-common.h"
>>   #include "qemu/option.h"
>> +#include "qemu/hbitmap.h"
>>   #include "block/coroutine.h"
>>   #include "block/accounting.h"
>>   #include "qapi/qmp/qobject.h"
>> @@ -473,6 +474,10 @@ void 
>> bdrv_dirty_bitmap_deserialize_part0(BdrvDirtyBitmap *bitmap,
>>                                            uint64_t start, uint64_t 
>> count);
>>   void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
>>
>> +HBitmap *bdrv_create_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>> +                                        uint64_t granularity);
>> +void bdrv_release_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap);
>> +
>>   void bdrv_enable_copy_on_read(BlockDriverState *bs);
>>   void bdrv_disable_copy_on_read(BlockDriverState *bs);
>>
>>
>
> Looks correct, it just needs documentation in my opinion to help 
> explain what this extra bitmap is for.
>
> Musings / opinions:
>
> I also think that at this point, we may want to make bdrv_reset_dirty 
> and bdrv_set_dirty call the bdrv_reset_dirty_bitmap and 
> bdrv_set_dirty_bitmap functions instead of re-implementing the behavior.
>
> I used to think it was fine as-is, but the more conditions we add to 
> how these bitmaps should be accessed, the more I think limiting the 
> interface to as few functions as possible is a good idea.
>
> Maybe I'll even do that myself. It might even be nice to split off all 
> the bitmap functions off into something like block/dirty_bitmaps.c as 
> the complexity creeps up and we're trying to improve the 
> maintainability of block.c.
>
> Anyway, we can worry about that in a later series.


-- 
Best regards,
Vladimir