diff mbox

[1/3] block: allow creation of detached dirty bitmaps

Message ID 9ae0ce111c95c6f76e872b12b2a6af895f5cfab6.1443410673.git.jcody@redhat.com
State New
Headers show

Commit Message

Jeff Cody Sept. 28, 2015, 3:29 a.m. UTC 
This allows the creation of detached dirty bitmaps, so that the
block driver dirty bitmaps can be used without inserting the
bitmap into the dirty bitmap list for a BDS.

To free a bitmap that was created "detached = true", call
bdrv_release_dirty_bitmap() with the BlockDriverState argument
as NULL.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block.c               | 26 ++++++++++++++++++++------
 block/mirror.c        |  3 ++-
 blockdev.c            |  2 +-
 include/block/block.h |  1 +
 migration/block.c     |  2 +-
 5 files changed, 25 insertions(+), 9 deletions(-)

Comments

Kevin Wolf Sept. 28, 2015, 2:41 p.m. UTC | #1 
Am 28.09.2015 um 05:29 hat Jeff Cody geschrieben:
> This allows the creation of detached dirty bitmaps, so that the
> block driver dirty bitmaps can be used without inserting the
> bitmap into the dirty bitmap list for a BDS.
> 
> To free a bitmap that was created "detached = true", call
> bdrv_release_dirty_bitmap() with the BlockDriverState argument
> as NULL.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>

It this really still a proper dirty bitmap? After all, bdrv_set_dirty()
doesn't affect the bitmap any more, query-block doesn't mention them
etc. What is the advantage of using the dirty bitmap infrastructure
instead of just using a plain bitmap without the "dirty" part?

Also, the bitmap is still made for a specific BDS, especially with
regards to its size. For dirty bitmaps, bdrv_truncate() calls
bdrv_dirty_bitmap_truncate() in order to resize the bitmaps as well.
This mechanism doesn't work any more if the bitmap isn't in the dirty
bitmaps list.

Do we need something like an op blocker? Though I'm not sure if an
operation as basic as bdrv_truncate() can be reasonably blocked at all.
Maybe we can only block the QMP resize command.

Kevin
Stefan Hajnoczi Sept. 28, 2015, 3:13 p.m. UTC | #2 
On Sun, Sep 27, 2015 at 11:29:16PM -0400, Jeff Cody wrote:
> This allows the creation of detached dirty bitmaps, so that the
> block driver dirty bitmaps can be used without inserting the
> bitmap into the dirty bitmap list for a BDS.
> 
> To free a bitmap that was created "detached = true", call
> bdrv_release_dirty_bitmap() with the BlockDriverState argument
> as NULL.

I wonder if just disabling the bitmap with bdrv_disable_dirty_bitmap()
is enough?
Max Reitz Sept. 28, 2015, 4:38 p.m. UTC | #3 
On 28.09.2015 05:29, Jeff Cody wrote:
> This allows the creation of detached dirty bitmaps, so that the
> block driver dirty bitmaps can be used without inserting the
> bitmap into the dirty bitmap list for a BDS.
> 
> To free a bitmap that was created "detached = true", call
> bdrv_release_dirty_bitmap() with the BlockDriverState argument
> as NULL.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block.c               | 26 ++++++++++++++++++++------
>  block/mirror.c        |  3 ++-
>  blockdev.c            |  2 +-
>  include/block/block.h |  1 +
>  migration/block.c     |  2 +-
>  5 files changed, 25 insertions(+), 9 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>
diff mbox

Patch

diff --git a/block.c b/block.c
index 53f7b08..1567cc2 100644
--- a/block.c
+++ b/block.c
@@ -3334,6 +3334,7 @@  void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
                                           uint32_t granularity,
                                           const char *name,
+                                          bool detached,
                                           Error **errp)
 {
     int64_t bitmap_size;
@@ -3342,7 +3343,7 @@  BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
 
     assert((granularity & (granularity - 1)) == 0);
 
-    if (name && bdrv_find_dirty_bitmap(bs, name)) {
+    if (name && !detached && bdrv_find_dirty_bitmap(bs, name)) {
         error_setg(errp, "Bitmap already exists: %s", name);
         return NULL;
     }
@@ -3359,7 +3360,9 @@  BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
     bitmap->size = bitmap_size;
     bitmap->name = g_strdup(name);
     bitmap->disabled = false;
-    QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
+    if (!detached) {
+        QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
+    }
     return bitmap;
 }
 
@@ -3403,7 +3406,7 @@  int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
 
     /* Create an anonymous successor */
     granularity = bdrv_dirty_bitmap_granularity(bitmap);
-    child = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
+    child = bdrv_create_dirty_bitmap(bs, granularity, NULL, false, errp);
     if (!child) {
         return -1;
     }
@@ -3483,16 +3486,27 @@  static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
     }
 }
 
+static void bdrv_free_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+{
+    hbitmap_free(bitmap->bitmap);
+    g_free(bitmap->name);
+    g_free(bitmap);
+}
+
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
 {
     BdrvDirtyBitmap *bm, *next;
+
+    if (bs == NULL) {
+        bdrv_free_dirty_bitmap(bitmap);
+        return;
+    }
+
     QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
         if (bm == bitmap) {
             assert(!bdrv_dirty_bitmap_frozen(bm));
             QLIST_REMOVE(bitmap, list);
-            hbitmap_free(bitmap->bitmap);
-            g_free(bitmap->name);
-            g_free(bitmap);
+            bdrv_free_dirty_bitmap(bitmap);
             return;
         }
     }
diff --git a/block/mirror.c b/block/mirror.c
index a258926..8b3e89b 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -733,7 +733,8 @@  static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
     s->buf_size = ROUND_UP(buf_size, granularity);
     s->unmap = unmap;
 
-    s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
+    s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, false,
+                                               errp);
     if (!s->dirty_bitmap) {
         g_free(s->replaces);
         block_job_release(bs);
diff --git a/blockdev.c b/blockdev.c
index 32b04b4..cb9f78d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2069,7 +2069,7 @@  void qmp_block_dirty_bitmap_add(const char *node, const char *name,
         granularity = bdrv_get_default_bitmap_granularity(bs);
     }
 
-    bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+    bdrv_create_dirty_bitmap(bs, granularity, name, false, errp);
 
  out:
     aio_context_release(aio_context);
diff --git a/include/block/block.h b/include/block/block.h
index ef67353..2ec1e20 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -476,6 +476,7 @@  typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
                                           uint32_t granularity,
                                           const char *name,
+                                          bool detached,
                                           Error **errp);
 int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
                                        BdrvDirtyBitmap *bitmap,
diff --git a/migration/block.c b/migration/block.c
index ed865ed..c3e2b86 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -320,7 +320,7 @@  static int set_dirty_tracking(void)
 
     QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
         bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE,
-                                                      NULL, NULL);
+                                                      NULL, false, NULL);
         if (!bmds->dirty_bitmap) {
             ret = -errno;
             goto fail;