On 04/12/2017 01:49 PM, Eric Blake wrote:
> We are still using an internal hbitmap that tracks a size in sectors,
> with the granularity scaled down accordingly, because it lets us
> use a shortcut for our iterators which are currently sector-based.
> But there's no reason we can't track the dirty bitmap size in bytes,
> since it is an internal-only variable.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
> block/dirty-bitmap.c | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 32698d5..a413df1 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -41,7 +41,7 @@ struct BdrvDirtyBitmap {
> 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) */
> + int64_t size; /* Size of the bitmap, in bytes */
> bool disabled; /* Bitmap is read-only */
> int active_iterators; /* How many iterators are active */
> QLIST_ENTRY(BdrvDirtyBitmap) list;
> @@ -79,24 +79,26 @@ BdrvDirtyBitmap
> *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> {
> int64_t bitmap_size;
> BdrvDirtyBitmap *bitmap;
> - uint32_t sector_granularity;
>
> - assert((granularity & (granularity - 1)) == 0);
> + assert(is_power_of_2(granularity) && granularity >= BDRV_SECTOR_SIZE);
>
> if (name && bdrv_find_dirty_bitmap(bs, name)) {
> error_setg(errp, "Bitmap already exists: %s", name);
> return NULL;
> }
> - sector_granularity = granularity >> BDRV_SECTOR_BITS;
> - assert(sector_granularity);
> - bitmap_size = bdrv_nb_sectors(bs);
> + bitmap_size = bdrv_getlength(bs);
> if (bitmap_size < 0) {
> error_setg_errno(errp, -bitmap_size, "could not get length of
> device");
> errno = -bitmap_size;
> return NULL;
> }
> bitmap = g_new0(BdrvDirtyBitmap, 1);
> - bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(sector_granularity));
> + /*
> + * TODO - let hbitmap track full granularity. For now, it is tracking
> + * only sector granularity, as a shortcut for our iterators.
> + */
> + bitmap->bitmap = hbitmap_alloc(bitmap_size >> BDRV_SECTOR_BITS,
> + ctz32(granularity) - BDRV_SECTOR_BITS);
> bitmap->size = bitmap_size;
> bitmap->name = g_strdup(name);
> bitmap->disabled = false;
> @@ -246,12 +248,13 @@ BdrvDirtyBitmap
> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
> {
> BdrvDirtyBitmap *bitmap;
> - uint64_t size = bdrv_nb_sectors(bs);
> + int64_t size = bdrv_getlength(bs);
>
> + assert(size >= 0);
> QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> assert(!bdrv_dirty_bitmap_frozen(bitmap));
> assert(!bitmap->active_iterators);
> - hbitmap_truncate(bitmap->bitmap, size);
> + hbitmap_truncate(bitmap->bitmap, size >> BDRV_SECTOR_BITS);
> bitmap->size = size;
> }
> }
> @@ -419,7 +422,7 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> HBitmap **out)
> hbitmap_reset_all(bitmap->bitmap);
> } else {
> HBitmap *backup = bitmap->bitmap;
> - bitmap->bitmap = hbitmap_alloc(bitmap->size,
> + bitmap->bitmap = hbitmap_alloc(bitmap->size >> BDRV_SECTOR_BITS,
> hbitmap_granularity(backup));
> *out = backup;
> }
>
Reviewed-by: John Snow <js...@redhat.com>
