qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH RFC v2 4/8] block: add dirty-dirty bitmaps


From: John Snow
Subject: Re: [Qemu-devel] [PATCH RFC v2 4/8] block: add dirty-dirty bitmaps
Date: Tue, 10 Feb 2015 16:30:46 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0

I had hoped it wouldn't come to this :)

On 01/27/2015 05:56 AM, Vladimir Sementsov-Ogievskiy wrote:
A dirty-dirty bitmap is a dirty bitmap for BdrvDirtyBitmap. It tracks
set/unset changes of BdrvDirtyBitmap.

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
  block.c               | 44 ++++++++++++++++++++++++++++++++++++++++++++
  include/block/block.h |  5 +++++
  2 files changed, 49 insertions(+)

diff --git a/block.c b/block.c
index 9860fc1..8ab724b 100644
--- a/block.c
+++ b/block.c
@@ -53,6 +53,7 @@

  struct BdrvDirtyBitmap {
      HBitmap *bitmap;
+    HBitmap *dirty_dirty_bitmap;

Just opinions: Maybe we can call it the "meta_bitmap" to help keep the name less confusing, and accompany it with a good structure comment here to explain what the heck it's for.

If you feel that's a good idea; s/dirty_dirty_/meta_/g below.

Regardless, let's make sure this patch adds documentation for it.

      BdrvDirtyBitmap *originator;
      int64_t size;
      int64_t granularity;
@@ -5373,6 +5374,30 @@ BdrvDirtyBitmap 
*bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
      return originator;
  }

+HBitmap *bdrv_create_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap,
+                                        uint64_t granularity)
+{
+    uint64_t sector_granularity;
+
+    assert((granularity & (granularity - 1)) == 0);
+
+    granularity *= 8 * bitmap->granularity;
+    sector_granularity = granularity >> BDRV_SECTOR_BITS;
+    assert(sector_granularity);
+
+    bitmap->dirty_dirty_bitmap =
+        hbitmap_alloc(bitmap->size, ffsll(sector_granularity) - 1);
+
+    return bitmap->dirty_dirty_bitmap;
+}
+
+void bdrv_release_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+{
+    if (bitmap->dirty_dirty_bitmap) {
+        hbitmap_free(bitmap->dirty_dirty_bitmap);
+        bitmap->dirty_dirty_bitmap = NULL;
+    }
+}

  void bdrv_print_dirty_bitmap(BdrvDirtyBitmap *bitmap)
  {
@@ -5447,6 +5472,9 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap)
          if (bm == bitmap) {
              QLIST_REMOVE(bitmap, list);
              hbitmap_free(bitmap->bitmap);
+            if (bitmap->dirty_dirty_bitmap) {
+                hbitmap_free(bitmap->dirty_dirty_bitmap);
+            }
              g_free(bitmap->name);
              g_free(bitmap);
              return;
@@ -5534,6 +5562,10 @@ void bdrv_set_dirty_bitmap(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap,
  {
      if (bitmap->enabled) {
          hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
+
+        if (bitmap->dirty_dirty_bitmap) {
+            hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, nr_sectors);
+        }
      }
  }

@@ -5541,6 +5573,9 @@ void bdrv_reset_dirty_bitmap(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap,
                               int64_t cur_sector, int nr_sectors)
  {
      hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
+    if (bitmap->dirty_dirty_bitmap) {
+        hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, nr_sectors);
+    }
  }

  /**
@@ -5550,6 +5585,9 @@ void bdrv_reset_dirty_bitmap(BlockDriverState *bs, 
BdrvDirtyBitmap *bitmap,
  void bdrv_clear_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
  {
      hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
+    if (bitmap->dirty_dirty_bitmap) {
+        hbitmap_set(bitmap->dirty_dirty_bitmap, 0, bitmap->size);
+    }
  }

  const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap)
@@ -5597,6 +5635,9 @@ static void bdrv_set_dirty(BlockDriverState *bs, int64_t 
cur_sector,
              continue;
          }
          hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
+        if (bitmap->dirty_dirty_bitmap) {
+            hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, nr_sectors);
+        }
      }
  }

@@ -5606,6 +5647,9 @@ static void bdrv_reset_dirty(BlockDriverState *bs, 
int64_t cur_sector,
      BdrvDirtyBitmap *bitmap;
      QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
          hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
+        if (bitmap->dirty_dirty_bitmap) {
+            hbitmap_set(bitmap->dirty_dirty_bitmap, cur_sector, nr_sectors);
+        }
      }
  }

diff --git a/include/block/block.h b/include/block/block.h
index 0890cd2..648b0a9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -4,6 +4,7 @@
  #include "block/aio.h"
  #include "qemu-common.h"
  #include "qemu/option.h"
+#include "qemu/hbitmap.h"
  #include "block/coroutine.h"
  #include "block/accounting.h"
  #include "qapi/qmp/qobject.h"
@@ -473,6 +474,10 @@ void bdrv_dirty_bitmap_deserialize_part0(BdrvDirtyBitmap 
*bitmap,
                                           uint64_t start, uint64_t count);
  void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);

+HBitmap *bdrv_create_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap,
+                                        uint64_t granularity);
+void bdrv_release_dirty_dirty_bitmap(BdrvDirtyBitmap *bitmap);
+
  void bdrv_enable_copy_on_read(BlockDriverState *bs);
  void bdrv_disable_copy_on_read(BlockDriverState *bs);



Looks correct, it just needs documentation in my opinion to help explain what this extra bitmap is for.

Musings / opinions:

I also think that at this point, we may want to make bdrv_reset_dirty and bdrv_set_dirty call the bdrv_reset_dirty_bitmap and bdrv_set_dirty_bitmap functions instead of re-implementing the behavior.

I used to think it was fine as-is, but the more conditions we add to how these bitmaps should be accessed, the more I think limiting the interface to as few functions as possible is a good idea.

Maybe I'll even do that myself. It might even be nice to split off all the bitmap functions off into something like block/dirty_bitmaps.c as the complexity creeps up and we're trying to improve the maintainability of block.c.

Anyway, we can worry about that in a later series.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]