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

[Vladimir Sementsov-Ogievskiy]

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

On 23.12.2014 04:12, John Snow wrote:
Signed-off-by: Fam Zheng <f...@redhat.com>
Signed-off-by: John Snow <js...@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);