Re: [Qemu-devel] [PATCH v6 03/10] block: Introduce bdrv_dirty_bitmap_granularity()

[Max Reitz]

On 2014-10-30 at 04:22, Fam Zheng wrote:
> This returns the granularity (in sectors) of dirty bitmap.

Actually, it does not.

> Signed-off-by: Fam Zheng <address@hidden>
> Reviewed-by: Benoit Canet <address@hidden>
> ---
>   block.c               | 6 ++++++
>   include/block/block.h | 2 ++
>   2 files changed, 8 insertions(+)
>
> diff --git a/block.c b/block.c
> index dafde4b..1b12541 100644
> --- a/block.c
> +++ b/block.c
> @@ -5356,6 +5356,12 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap 
> *bitmap, int64_t sector
>       }
>   }
>   
> +int bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
> +                                  BdrvDirtyBitmap *bitmap)
> +{
> +    return hbitmap_granularity(bitmap->bitmap);
> +}
> +

hbitmap_granularity() returns bitmap->bitmap->granularity. 
Unfortunately, that value is ffs(granularity >> BDRV_SECTOR_BITS) - 1, 
(= ffs(granularity) - 1 - BDRV_SECTOR_BITS) where "granularity" is the 
original granularity from when the dirty bitmap was created. Therefore, 
this should be 1 << hbitmap_granularity(bitmap->bitmap) to be the 
granularity in sectors.

See bdrv_query_dirty_bitmaps() (written by you :-)).

And frankly, I'd prefer this not to return the granularity in sectors 
but in bytes, because that's the unit the granularity is specified in 
when the dirty bitmap is created; also, as far as my understanding goes, 
we want to get away from sectors, so I wouldn't introduce a new function 
using that unit if there's no real need for it.

Max

>   void bdrv_dirty_iter_init(BlockDriverState *bs,
>                             BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
>   {
> diff --git a/include/block/block.h b/include/block/block.h
> index b0ef82c..87fa48e 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -428,6 +428,8 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState 
> *bs,
>   void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap 
> *bitmap);
>   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
>   BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
> +int bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
> +                                  BdrvDirtyBitmap *bitmap);
>   int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t 
> sector);
>   void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
>   void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int 
> nr_sectors);