Re: [Qemu-devel] [PATCH v10 06/13] block: Add bdrv_copy_dirty_bitmap and bdrv_clear_dirty_bitmap

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

Hmm, can't find it in v11

Best regards,
Vladimir

On 10.01.2015 06:25, John Snow wrote:
>
>
> On 12/30/2014 08:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>> I'm sorry if it was already discussed, but I think it is inconsistent to
>> have "size" in sectors and "granularity" in bytes in one structure. I've
>> misused these fields because of this in my current work.
>>
>> At least, I think there should be comments about this.
>>
>> Best regards,
>> Vladimir
>
> Looking at it now, I think it'd be worse to change it, because it 
> represents the "size" of the bitmap, as in the number of logical bits 
> for the interface of the bitmap. Since it is a sector bitmap, I think 
> this should remain in sectors, for now.
>
> I really want to keep the granularity in bytes in this case, because I 
> want to match the existing interface, and size makes sense to me in 
> sectors.
>
> What I will do instead is just to document this quirk. Look out for 
> V11 :)
>
> --John
>
>>
>> On 23.12.2014 04:12, John Snow wrote:
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   block.c               | 39 +++++++++++++++++++++++++++++++++++----
>>>   include/block/block.h |  4 ++++
>>>   2 files changed, 39 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index a1d9e88..f9e0767 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -53,6 +53,8 @@
>>>   struct BdrvDirtyBitmap {
>>>       HBitmap *bitmap;
>>> +    int64_t size;
>>> +    int64_t granularity;
>>>       char *name;
>>>       QLIST_ENTRY(BdrvDirtyBitmap) list;
>>>   };
>>> @@ -5343,6 +5345,21 @@ void
>>> bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap
>>> *bitmap)
>>>       bitmap->name = NULL;
>>>   }
>>> +BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
>>> +                                        BdrvDirtyBitmap *bitmap,
>>> +                                        const char *name)
>>> +{
>>> +    BdrvDirtyBitmap *new_bitmap;
>>> +
>>> +    new_bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
>>> +    new_bitmap->bitmap = hbitmap_copy(bitmap->bitmap);
>>> +    new_bitmap->size = bitmap->size;
>>> +    new_bitmap->granularity = bitmap->granularity;
>>> +    new_bitmap->name = g_strdup(name);
>>> +    QLIST_INSERT_HEAD(&bs->dirty_bitmaps, new_bitmap, list);
>>> +    return new_bitmap;
>>> +}
>>> +
>>>   BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>>>                                             int granularity,
>>>                                             const char *name,
>>> @@ -5350,6 +5367,7 @@ BdrvDirtyBitmap
>>> *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>>>   {
>>>       int64_t bitmap_size;
>>>       BdrvDirtyBitmap *bitmap;
>>> +    int sector_granularity;
>>>       assert((granularity & (granularity - 1)) == 0);
>>> @@ -5357,8 +5375,8 @@ BdrvDirtyBitmap
>>> *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>>>           error_setg(errp, "Bitmap already exists: %s", name);
>>>           return NULL;
>>>       }
>>> -    granularity >>= BDRV_SECTOR_BITS;
>>> -    assert(granularity);
>>> +    sector_granularity = granularity >> BDRV_SECTOR_BITS;
>>> +    assert(sector_granularity);
>>>       bitmap_size = bdrv_nb_sectors(bs);
>>>       if (bitmap_size < 0) {
>>>           error_setg_errno(errp, -bitmap_size, "could not get length
>>> of device");
>>> @@ -5366,7 +5384,9 @@ BdrvDirtyBitmap
>>> *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>>>           return NULL;
>>>       }
>>>       bitmap = g_new0(BdrvDirtyBitmap, 1);
>>> -    bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
>>> +    bitmap->size = bitmap_size;
>>> +    bitmap->granularity = granularity;
>>> +    bitmap->bitmap = hbitmap_alloc(bitmap->size,
>>> ffs(sector_granularity) - 1);
>>>       bitmap->name = g_strdup(name);
>>>       QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
>>>       return bitmap;
>>> @@ -5439,7 +5459,9 @@ uint64_t
>>> bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
>>>   uint64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
>>>                                          BdrvDirtyBitmap *bitmap)
>>>   {
>>> -    return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
>>> +    g_assert(BDRV_SECTOR_SIZE << 
>>> hbitmap_granularity(bitmap->bitmap) ==
>>> +             bitmap->granularity);
>>> +    return bitmap->granularity;
>>>   }
>>>   void bdrv_dirty_iter_init(BlockDriverState *bs,
>>> @@ -5460,6 +5482,15 @@ void bdrv_reset_dirty_bitmap(BlockDriverState
>>> *bs, BdrvDirtyBitmap *bitmap,
>>>       hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
>>>   }
>>> +/**
>>> + * Effectively, reset the hbitmap from bits [0, size)
>>> + * Synonymous with bdrv_reset_dirty_bitmap(bs, bitmap, 0, 
>>> bitmap->size)
>>> + */
>>> +void bdrv_clear_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap
>>> *bitmap)
>>> +{
>>> +    hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
>>> +}
>>> +
>>>   static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
>>>                              int nr_sectors)
>>>   {
>>> diff --git a/include/block/block.h b/include/block/block.h
>>> index c7402e7..e964abd 100644
>>> --- a/include/block/block.h
>>> +++ b/include/block/block.h
>>> @@ -436,6 +436,9 @@ BdrvDirtyBitmap
>>> *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>>>   BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
>>>                                           const char *name);
>>>   void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs,
>>> BdrvDirtyBitmap *bitmap);
>>> +BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
>>> +                                        BdrvDirtyBitmap *bitmap,
>>> +                                        const char *name);
>>>   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap
>>> *bitmap);
>>>   BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
>>>   uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
>>> @@ -446,6 +449,7 @@ void bdrv_set_dirty_bitmap(BlockDriverState *bs,
>>> BdrvDirtyBitmap *bitmap,
>>>                              int64_t cur_sector, int nr_sectors);
>>>   void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap
>>> *bitmap,
>>>                                int64_t cur_sector, int nr_sectors);
>>> +void bdrv_clear_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap
>>> *bitmap);
>>>   void bdrv_dirty_iter_init(BlockDriverState *bs,
>>>                             BdrvDirtyBitmap *bitmap, struct
>>> HBitmapIter *hbi);
>>>   int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap
>>> *bitmap);
>>
>>
>