[Qemu-devel] [PATCH v3 3/4] block: Manage granularity in BdrvDirtyBitmap

[Fam Zheng]

BdrvDirtyBitmap has to know about granularity because it's in the API,
but currently the logic is in HBitmap. Because HBitmap has a delicate
implementation that has a multi-layer array of arrays of different
granularities, handling the "bit granularity" there only makes it harder
to understand.

Now pull the granularity logic up to BdrvDirtyBitmap completely so that
HBitmap is always 1:1 mapped from position range to bit range from the
interface point of view.

Signed-off-by: Fam Zheng <address@hidden>
---
 block.c | 49 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/block.c b/block.c
index 3eb6ff9..e225050 100644
--- a/block.c
+++ b/block.c
@@ -63,6 +63,8 @@
  *     or enabled. A frozen bitmap can only abdicate() or reclaim().
  */
 struct BdrvDirtyBitmap {
+    int gran_shift;             /* Bits to right shift from sector number to
+                                   bit index. */
     HBitmap *bitmap;            /* Dirty sector bitmap implementation */
     BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
     char *name;                 /* Optional non-empty unique ID */
@@ -3163,24 +3165,26 @@ BdrvDirtyBitmap 
*bdrv_create_dirty_bitmap(BlockDriverState *bs,
 {
     int64_t bitmap_size;
     BdrvDirtyBitmap *bitmap;
-    uint32_t sector_granularity;
+    int gran_shift;
 
     assert((granularity & (granularity - 1)) == 0);
+    /* Caller should check that */
+    assert(granularity >= BDRV_SECTOR_SIZE);
 
+    gran_shift = ctz32(granularity) - BDRV_SECTOR_BITS;
     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 = DIV_ROUND_UP(bdrv_getlength(bs), granularity);
     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));
+    bitmap->bitmap = hbitmap_alloc(bitmap_size, 0);
+    bitmap->gran_shift = gran_shift;
     bitmap->size = bitmap_size;
     bitmap->name = g_strdup(name);
     bitmap->disabled = false;
@@ -3299,9 +3303,10 @@ BdrvDirtyBitmap 
*bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
 static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
 {
     BdrvDirtyBitmap *bitmap;
-    uint64_t size = bdrv_nb_sectors(bs);
 
     QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+        int64_t size = bdrv_nb_sectors(bs) >> bitmap->gran_shift;
+        /* TODO: what if size < 0? */
         assert(!bdrv_dirty_bitmap_frozen(bitmap));
         hbitmap_truncate(bitmap->bitmap, size);
         bitmap->size = size;
@@ -3361,7 +3366,7 @@ BlockDirtyInfoList 
*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
 int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t 
sector)
 {
     if (bitmap) {
-        return hbitmap_get(bitmap->bitmap, sector);
+        return hbitmap_get(bitmap->bitmap, sector >> bitmap->gran_shift);
     } else {
         return 0;
     }
@@ -3389,14 +3394,15 @@ uint32_t 
bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
 
 uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
 {
-    return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
+    return BDRV_SECTOR_SIZE << bitmap->gran_shift;
 }
 
 BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
                                          uint64_t first_sector)
 {
     BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
-    hbitmap_iter_init(&iter->hbi, bitmap->bitmap, first_sector);
+    hbitmap_iter_init(&iter->hbi, bitmap->bitmap,
+                      first_sector >> bitmap->gran_shift);
     iter->bitmap = bitmap;
     bitmap->active_iterators++;
     return iter;
@@ -3414,21 +3420,30 @@ void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)
 
 int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
 {
-    return hbitmap_iter_next(&iter->hbi);
+    int64_t ret = hbitmap_iter_next(&iter->hbi);
+    return ret < 0 ? ret : ret << iter->bitmap->gran_shift;
 }
 
 void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                            int64_t cur_sector, int nr_sectors)
 {
+    int64_t start = cur_sector >> bitmap->gran_shift;
+    int64_t end = DIV_ROUND_UP(cur_sector + nr_sectors,
+                               1 << bitmap->gran_shift);
+
     assert(bdrv_dirty_bitmap_enabled(bitmap));
-    hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
+    hbitmap_set(bitmap->bitmap, start, end - start);
 }
 
 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                              int64_t cur_sector, int nr_sectors)
 {
+    int64_t start = cur_sector >> bitmap->gran_shift;
+    int64_t end = DIV_ROUND_UP(cur_sector + nr_sectors,
+                               1 << bitmap->gran_shift);
+
     assert(bdrv_dirty_bitmap_enabled(bitmap));
-    hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
+    hbitmap_reset(bitmap->bitmap, start, end - start);
 }
 
 void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
@@ -3438,8 +3453,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,
-                                       hbitmap_granularity(backup));
+        bitmap->bitmap = hbitmap_alloc(bitmap->size, 0);
         *out = backup;
     }
 }
@@ -3460,7 +3474,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t 
cur_sector,
         if (!bdrv_dirty_bitmap_enabled(bitmap)) {
             continue;
         }
-        hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
+        bdrv_set_dirty_bitmap(bitmap, cur_sector, nr_sectors);
     }
 }
 
@@ -3469,12 +3483,13 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t 
cur_sector,
  */
 void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t sector_num)
 {
-    hbitmap_iter_init(&iter->hbi, iter->bitmap->bitmap, sector_num);
+    hbitmap_iter_init(&iter->hbi, iter->bitmap->bitmap,
+                      sector_num >> iter->bitmap->gran_shift);
 }
 
 int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
 {
-    return hbitmap_count(bitmap->bitmap);
+    return hbitmap_count(bitmap->bitmap) << bitmap->gran_shift;
 }
 
 /* Get a reference to bs */
-- 
2.4.3