[Qemu-devel] [PATCH v4 0/9] QMP: Introduce incremental drive-backup with in-memory dirty bitmap

[Qemu-devel] [PATCH v4 0/9] QMP: Introduce incremental drive-backup with in-memory dirty bitmap

[Fam Zheng]

v4: [08/09] qmp: Add support of "dirty-bitmap" sync mode for drive-backup
            Comment text fixes. (Eric)

v3:
    Address Benoit's comments.

    [01/09] qapi: Add optional field "name" to block dirty bitmap
            Don't split line.

    [03/09] block: Handle error of bdrv_getlength in bdrv_create_dirty_bitmap
            Add reviewed-by.

    [04/09] block: Introduce bdrv_dirty_bitmap_granularity()
            Add reviewed-by.

    [05/09] hbitmap: Add hbitmap_copy
            Fix size calculation.

    [08/09] qmp: Add support of "dirty-bitmap" sync mode for drive-backup
            Fix typo in commit message.
            Add comment for sync_bitmap_gran.
            Add (Since 2.1).

Thanks,
Fam


Fam Zheng (9):
  qapi: Add optional field "name" to block dirty bitmap
  qmp: Add dirty-bitmap-add and dirty-bitmap-remove
  block: Handle error of bdrv_getlength in bdrv_create_dirty_bitmap
  block: Introduce bdrv_dirty_bitmap_granularity()
  hbitmap: Add hbitmap_copy
  block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap
  qmp: Add dirty-bitmap-enable and dirty-bitmap-disable
  qmp: Add support of "dirty-bitmap" sync mode for drive-backup
  qapi: Add transaction support to dirty-bitmap-{add,disable}

 block-migration.c         |   3 +-
 block.c                   |  89 ++++++++++++++++++++++-
 block/backup.c            |  53 +++++++++++++-
 block/mirror.c            |   6 +-
 blockdev.c                | 181 +++++++++++++++++++++++++++++++++++++++++++++-
 hmp.c                     |   4 +-
 include/block/block.h     |  16 +++-
 include/block/block_int.h |   3 +
 include/qemu/hbitmap.h    |   8 ++
 qapi-schema.json          | 117 ++++++++++++++++++++++++++++--
 qmp-commands.hx           |  66 ++++++++++++++++-
 util/hbitmap.c            |  16 ++++
 12 files changed, 544 insertions(+), 18 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v4 1/9] qapi: Add optional field "name" to block dirty bitmap

[Fam Zheng]

This field will be set for user created dirty bitmap. Also pass in an
error pointer to bdrv_create_dirty_bitmap, so when a name is already
taken on this BDS, it can report an error message. This is not global
check, two BDSes can have dirty bitmap with a common name.

Implemented bdrv_find_dirty_bitmap to find a dirty bitmap by name, will
be used later when other QMP commands want to reference dirty bitmap by
name.

Add bdrv_dirty_bitmap_make_anon. This unsets the name of dirty bitmap.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block-migration.c     |  3 ++-
 block.c               | 33 ++++++++++++++++++++++++++++++++-
 block/mirror.c        |  2 +-
 include/block/block.h |  8 +++++++-
 qapi-schema.json      |  4 +++-
 5 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index 897fdba..e6e016a 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -315,7 +315,8 @@ static void set_dirty_tracking(void)
     BlkMigDevState *bmds;
 
     QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
-        bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE);
+        bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE,
+                                                      NULL, NULL);
     }
 }
 
diff --git a/block.c b/block.c
index acb70fd..3f880d6 100644
--- a/block.c
+++ b/block.c
@@ -52,6 +52,7 @@
 
 struct BdrvDirtyBitmap {
     HBitmap *bitmap;
+    char *name;
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -5079,18 +5080,45 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov)
     return true;
 }
 
-BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity)
+BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
+{
+    BdrvDirtyBitmap *bm;
+    QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
+        if (!strcmp(name, bm->name)) {
+            return bm;
+        }
+    }
+    return NULL;
+}
+
+void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+    g_free(bitmap->name);
+    bitmap->name = NULL;
+}
+
+BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
+                                          int granularity,
+                                          const char *name,
+                                          Error **errp)
 {
     int64_t bitmap_size;
     BdrvDirtyBitmap *bitmap;
 
     assert((granularity & (granularity - 1)) == 0);
 
+    if (name && bdrv_find_dirty_bitmap(bs, name)) {
+        error_setg(errp, "Bitmap already exists: %s", name);
+        return NULL;
+    }
     granularity >>= BDRV_SECTOR_BITS;
     assert(granularity);
     bitmap_size = (bdrv_getlength(bs) >> BDRV_SECTOR_BITS);
     bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
     bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
+    if (name) {
+        bitmap->name = g_strdup(name);
+    }
     QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
     return bitmap;
 }
@@ -5102,6 +5130,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
         if (bm == bitmap) {
             QLIST_REMOVE(bitmap, list);
             hbitmap_free(bitmap->bitmap);
+            g_free(bitmap->name);
             g_free(bitmap);
             return;
         }
@@ -5120,6 +5149,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
         info->count = bdrv_get_dirty_count(bs, bm);
         info->granularity =
             ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
+        info->has_name = bm->name[0] != '\0';
+        info->name = g_strdup(bm->name);
         entry->value = info;
         *plist = entry;
         plist = &entry->next;
diff --git a/block/mirror.c b/block/mirror.c
index 0ef41f9..9b73cd9 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -605,7 +605,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
     s->granularity = granularity;
     s->buf_size = MAX(buf_size, granularity);
 
-    s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity);
+    s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
     bdrv_set_enable_write_cache(s->target, true);
     bdrv_set_on_error(s->target, on_target_error, on_target_error);
     bdrv_iostatus_enable(s->target);
diff --git a/include/block/block.h b/include/block/block.h
index 1ed55d8..aa30b2a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -428,7 +428,13 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov);
 
 struct HBitmapIter;
 typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
-BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity);
+BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
+                                          int granularity,
+                                          const char *name,
+                                          Error **errp);
+BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
+                                        const char *name);
+void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
diff --git a/qapi-schema.json b/qapi-schema.json
index 391356f..506c242 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1029,6 +1029,8 @@
 #
 # Block dirty bitmap information.
 #
+# @name: #optional the name of dirty bitmap (Since 2.1)
+#
 # @count: number of dirty bytes according to the dirty bitmap
 #
 # @granularity: granularity of the dirty bitmap in bytes (since 1.4)
@@ -1036,7 +1038,7 @@
 # Since: 1.3
 ##
 { 'type': 'BlockDirtyInfo',
-  'data': {'count': 'int', 'granularity': 'int'} }
+  'data': {'*name': 'str', 'count': 'int', 'granularity': 'int'} }
 
 ##
 # @BlockInfo:
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 1/9] qapi: Add optional field "name" to block dirty bitmap

[Stefan Hajnoczi]

On Thu, Mar 27, 2014 at 05:09:40PM +0800, Fam Zheng wrote:
> @@ -5079,18 +5080,45 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov)
>      return true;
>  }
>  
> -BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity)
> +BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
> +{
> +    BdrvDirtyBitmap *bm;
> +    QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
> +        if (!strcmp(name, bm->name)) {
> +            return bm;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +{
> +    g_free(bitmap->name);
> +    bitmap->name = NULL;
> +}

This looks dangerous since strcmp() does not check for NULL pointers.  I
guess you need to add a check to bdrv_find_dirty_bitmap().

> @@ -5120,6 +5149,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>          info->count = bdrv_get_dirty_count(bs, bm);
>          info->granularity =
>              ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
> +        info->has_name = bm->name[0] != '\0';
> +        info->name = g_strdup(bm->name);

This looks dangerous too.  What if ->name is NULL?

Re: [Qemu-devel] [PATCH v4 1/9] qapi: Add optional field "name" to block dirty bitmap

[Fam Zheng]

On Thu, 03/27 16:30, Stefan Hajnoczi wrote:
> On Thu, Mar 27, 2014 at 05:09:40PM +0800, Fam Zheng wrote:
> > @@ -5079,18 +5080,45 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov)
> >      return true;
> >  }
> >  
> > -BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity)
> > +BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
> > +{
> > +    BdrvDirtyBitmap *bm;
> > +    QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
> > +        if (!strcmp(name, bm->name)) {
> > +            return bm;
> > +        }
> > +    }
> > +    return NULL;
> > +}
> > +
> > +void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> > +{
> > +    g_free(bitmap->name);
> > +    bitmap->name = NULL;
> > +}
> 
> This looks dangerous since strcmp() does not check for NULL pointers.  I
> guess you need to add a check to bdrv_find_dirty_bitmap().

OK.

> 
> > @@ -5120,6 +5149,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
> >          info->count = bdrv_get_dirty_count(bs, bm);
> >          info->granularity =
> >              ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
> > +        info->has_name = bm->name[0] != '\0';
> > +        info->name = g_strdup(bm->name);
> 
> This looks dangerous too.  What if ->name is NULL?

Will add a check here.

Thanks,
Fam

[Qemu-devel] [PATCH v4 2/9] qmp: Add dirty-bitmap-add and dirty-bitmap-remove

[Fam Zheng]

The new command pair is added to manage user created dirty bitmap. The
dirty bitmap's name is mandatory and must be unique for the same device,
but different devices can have bitmaps with the same names.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c       | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json | 45 ++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx  | 49 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 154 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index c3422a1..662c950 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1713,6 +1713,66 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
     }
 }
 
+void qmp_dirty_bitmap_add(const char *device, const char *name,
+                          bool has_granularity, int64_t granularity,
+                          Error **errp)
+{
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    if (!name || name[0] == '\0') {
+        error_setg(errp, "Bitmap name cannot be empty");
+        return;
+    }
+    if (has_granularity) {
+        if (granularity & (granularity - 1)) {
+            error_setg(errp, "Granularity must be power of 2");
+            return;
+        }
+    } else {
+        granularity = 65536;
+    }
+
+    bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+    if (!bitmap) {
+        return;
+    }
+}
+
+void qmp_dirty_bitmap_remove(const char *device, const char *name,
+                             Error **errp)
+{
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    if (!name || name[0] == '\0') {
+        error_setg(errp, "Bitmap name cannot be empty");
+        return;
+    }
+    bitmap = bdrv_find_dirty_bitmap(bs, name);
+    if (!bitmap) {
+        error_setg(errp, "Dirty bitmap not found: %s", name);
+        return;
+    }
+
+    /* Make it invisible to user in case the following
+     * bdrv_release_dirty_bitmap doens't free it because of refcnt */
+    bdrv_dirty_bitmap_make_anon(bs, bitmap);
+    bdrv_release_dirty_bitmap(bs, bitmap);
+}
+
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     const char *id = qdict_get_str(qdict, "id");
diff --git a/qapi-schema.json b/qapi-schema.json
index 506c242..56f16a9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2209,6 +2209,51 @@
             '*on-target-error': 'BlockdevOnError' } }
 
 ##
+# @DirtyBitmap
+#
+# @device: name of device which the bitmap is tracking
+#
+# @name: name of the dirty bitmap
+#
+# @granularity: #optional the bitmap granularity, default is 64k for
+#               dirty-bitmap-add
+#
+# Since 2.1
+##
+{ 'type': 'DirtyBitmap',
+  'data': { 'device': 'str', 'name': 'str', '*granularity': 'int' } }
+
+##
+# @dirty-bitmap-add
+#
+# Create a dirty bitmap with a name on the device
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If @name is already taken, GenericError with an explaining message
+#
+# Since 2.1
+##
+{'command': 'dirty-bitmap-add',
+  'data': 'DirtyBitmap' }
+
+##
+# @dirty-bitmap-remove
+#
+# Remove a dirty bitmap on the device
+#
+# Setting granularity has no effect here.
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If @name is not found, GenericError with an explaining message
+#
+# Since 2.1
+##
+{'command': 'dirty-bitmap-remove',
+  'data': { 'device': 'str', 'name': 'str' } }
+
+##
 # @migrate_cancel
 #
 # Cancel the current executing migration process.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index ed3ab92..b74f6ed 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1185,6 +1185,55 @@ Example:
 EQMP
 
     {
+        .name       = "dirty-bitmap-add",
+        .args_type  = "device:B,name:s,granularity:i?",
+        .mhandler.cmd_new = qmp_marshal_input_dirty_bitmap_add,
+    },
+    {
+        .name       = "dirty-bitmap-remove",
+        .args_type  = "device:B,name:s",
+        .mhandler.cmd_new = qmp_marshal_input_dirty_bitmap_remove,
+    },
+
+SQMP
+
+dirty-bitmap-add
+----------------
+
+Create a dirty bitmap with a name on the device, and start tracking the writes.
+
+Arguments:
+
+- "device": device name to create dirty bitmap (json-string)
+- "name": name of the new dirty bitmap (json-string)
+- "granularity": granularity to track writes with. (int)
+
+Example:
+
+-> { "execute": "dirty-bitmap-add", "arguments": { "device": "drive0",
+                                                   "name": "bitmap0" } }
+<- { "return": {} }
+
+dirty-bitmap-remove
+----------------
+
+Stop write tracking and remove the dirty bitmap that was created with
+dirty-bitmap-add.
+
+Arguments:
+
+- "device": device name to remove dirty bitmap (json-string)
+- "name": name of the dirty bitmap to remove (json-string)
+
+Example:
+
+-> { "execute": "dirty-bitmap-remove", "arguments": { "device": "drive0",
+                                                      "name": "bitmap0" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "blockdev-snapshot-sync",
         .args_type  = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?",
         .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 2/9] qmp: Add dirty-bitmap-add and dirty-bitmap-remove

[Stefan Hajnoczi]

On Thu, Mar 27, 2014 at 05:09:41PM +0800, Fam Zheng wrote:
> @@ -1713,6 +1713,66 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
>      }
>  }
>  
> +void qmp_dirty_bitmap_add(const char *device, const char *name,
> +                          bool has_granularity, int64_t granularity,
> +                          Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    if (!name || name[0] == '\0') {
> +        error_setg(errp, "Bitmap name cannot be empty");
> +        return;
> +    }
> +    if (has_granularity) {
> +        if (granularity & (granularity - 1)) {
> +            error_setg(errp, "Granularity must be power of 2");
> +            return;
> +        }

granularity must be non-zero, otherwise bdrv_create_dirty_bitmap() hits
an assertion failure.

It should probably also be at least 512.

> +    } else {
> +        granularity = 65536;
> +    }
> +
> +    bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> +    if (!bitmap) {
> +        return;
> +    }

Useless error return.

> +}
> +
> +void qmp_dirty_bitmap_remove(const char *device, const char *name,
> +                             Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    if (!name || name[0] == '\0') {
> +        error_setg(errp, "Bitmap name cannot be empty");
> +        return;
> +    }
> +    bitmap = bdrv_find_dirty_bitmap(bs, name);
> +    if (!bitmap) {
> +        error_setg(errp, "Dirty bitmap not found: %s", name);
> +        return;
> +    }
> +
> +    /* Make it invisible to user in case the following
> +     * bdrv_release_dirty_bitmap doens't free it because of refcnt */

"doesn't"

Re: [Qemu-devel] [PATCH v4 2/9] qmp: Add dirty-bitmap-add and dirty-bitmap-remove

[Fam Zheng]

On Thu, 03/27 16:43, Stefan Hajnoczi wrote:
> On Thu, Mar 27, 2014 at 05:09:41PM +0800, Fam Zheng wrote:
> > @@ -1713,6 +1713,66 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
> >      }
> >  }
> >  
> > +void qmp_dirty_bitmap_add(const char *device, const char *name,
> > +                          bool has_granularity, int64_t granularity,
> > +                          Error **errp)
> > +{
> > +    BlockDriverState *bs;
> > +    BdrvDirtyBitmap *bitmap;
> > +
> > +    bs = bdrv_find(device);
> > +    if (!bs) {
> > +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> > +        return;
> > +    }
> > +
> > +    if (!name || name[0] == '\0') {
> > +        error_setg(errp, "Bitmap name cannot be empty");
> > +        return;
> > +    }
> > +    if (has_granularity) {
> > +        if (granularity & (granularity - 1)) {
> > +            error_setg(errp, "Granularity must be power of 2");
> > +            return;
> > +        }
> 
> granularity must be non-zero, otherwise bdrv_create_dirty_bitmap() hits
> an assertion failure.
> 
> It should probably also be at least 512.

Sure, adding a check.

> 
> > +    } else {
> > +        granularity = 65536;
> > +    }
> > +
> > +    bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> > +    if (!bitmap) {
> > +        return;
> > +    }
> 
> Useless error return.

Removing.

> 
> > +}
> > +
> > +void qmp_dirty_bitmap_remove(const char *device, const char *name,
> > +                             Error **errp)
> > +{
> > +    BlockDriverState *bs;
> > +    BdrvDirtyBitmap *bitmap;
> > +
> > +    bs = bdrv_find(device);
> > +    if (!bs) {
> > +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> > +        return;
> > +    }
> > +
> > +    if (!name || name[0] == '\0') {
> > +        error_setg(errp, "Bitmap name cannot be empty");
> > +        return;
> > +    }
> > +    bitmap = bdrv_find_dirty_bitmap(bs, name);
> > +    if (!bitmap) {
> > +        error_setg(errp, "Dirty bitmap not found: %s", name);
> > +        return;
> > +    }
> > +
> > +    /* Make it invisible to user in case the following
> > +     * bdrv_release_dirty_bitmap doens't free it because of refcnt */
> 
> "doesn't"

Thanks,
Fam

Re: [Qemu-devel] [PATCH v4 2/9] qmp: Add dirty-bitmap-add and dirty-bitmap-remove

[Stefan Hajnoczi]

On Thu, Mar 27, 2014 at 05:09:41PM +0800, Fam Zheng wrote:
> +    if (has_granularity) {
> +        if (granularity & (granularity - 1)) {
> +            error_setg(errp, "Granularity must be power of 2");
> +            return;
> +        }
> +    } else {
> +        granularity = 65536;
> +    }

util/hbitmap.c has:
assert(granularity >= 0 && granularity < 64);

Please make sure the argument is checked before we pass it down.  We
should never hit an assertion failure due to bad inputs.

Stefan

Re: [Qemu-devel] [PATCH v4 2/9] qmp: Add dirty-bitmap-add and dirty-bitmap-remove

[Dr. David Alan Gilbert]

Hi Fam,
  Could you make this something like block-dirty-bitmap  - the RAM migration
also has a dirty bitmap, and it would just make it clearer.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 2/9] qmp: Add dirty-bitmap-add and dirty-bitmap-remove

[Fam Zheng]

On Thu, 03/27 16:39, Dr. David Alan Gilbert wrote:
> Hi Fam,
>   Could you make this something like block-dirty-bitmap  - the RAM migration
> also has a dirty bitmap, and it would just make it clearer.

Good idea. Thanks, Dave.

Fam

Re: [Qemu-devel] [PATCH v4 2/9] qmp: Add dirty-bitmap-add and dirty-bitmap-remove

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1832 bytes --]

On 03/27/2014 03:09 AM, Fam Zheng wrote:
> The new command pair is added to manage user created dirty bitmap. The
> dirty bitmap's name is mandatory and must be unique for the same device,
> but different devices can have bitmaps with the same names.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---

> +++ b/qapi-schema.json
> @@ -2209,6 +2209,51 @@
>              '*on-target-error': 'BlockdevOnError' } }
>  
>  ##
> +# @DirtyBitmap
> +#
> +# @device: name of device which the bitmap is tracking
> +#
> +# @name: name of the dirty bitmap
> +#
> +# @granularity: #optional the bitmap granularity, default is 64k for
> +#               dirty-bitmap-add

Optional, but only affects dirty-bitmap-add.  You later document...

> +# @dirty-bitmap-remove
> +#
> +# Remove a dirty bitmap on the device
> +#
> +# Setting granularity has no effect here.

...that it is silently ignored where it can't be used here, and again in
7/9 for both dirty-bitmap-disable and dirty-bitmap-enable.

I think it would be smarter to do:

{ 'type': 'DirtyBitmap',
  'data': { 'device': 'str', 'name': 'str' } }

{'command': 'dirty-bitmap-add',
  'data': { 'map': 'DirtyBitmap', '*granularity': 'int' } }

Or:

{ 'type': 'DirtyBitmap',
  'data': { 'device': 'str', 'name': 'str' } }
{ 'type': 'DirtyBitmapGranularity',
  'base': 'DirtyBitmap',
  'data': { '*granularity': 'int' } }
{'command': 'dirty-bitmap-add',
  'data': 'DirtyBitmapGranularity' }


which says that the 'DirtyBitmap' struct has no optional members, and
instead of silently ignoring an optional member in 3 commands, we
instead write the one command that takes the optional argument when we
actually care about it.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 2/9] qmp: Add dirty-bitmap-add and dirty-bitmap-remove

[Fam Zheng]

On Thu, 03/27 10:41, Eric Blake wrote:
> On 03/27/2014 03:09 AM, Fam Zheng wrote:
> > The new command pair is added to manage user created dirty bitmap. The
> > dirty bitmap's name is mandatory and must be unique for the same device,
> > but different devices can have bitmaps with the same names.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> 
> > +++ b/qapi-schema.json
> > @@ -2209,6 +2209,51 @@
> >              '*on-target-error': 'BlockdevOnError' } }
> >  
> >  ##
> > +# @DirtyBitmap
> > +#
> > +# @device: name of device which the bitmap is tracking
> > +#
> > +# @name: name of the dirty bitmap
> > +#
> > +# @granularity: #optional the bitmap granularity, default is 64k for
> > +#               dirty-bitmap-add
> 
> Optional, but only affects dirty-bitmap-add.  You later document...
> 
> > +# @dirty-bitmap-remove
> > +#
> > +# Remove a dirty bitmap on the device
> > +#
> > +# Setting granularity has no effect here.
> 
> ...that it is silently ignored where it can't be used here, and again in
> 7/9 for both dirty-bitmap-disable and dirty-bitmap-enable.
> 
> I think it would be smarter to do:
> 
> { 'type': 'DirtyBitmap',
>   'data': { 'device': 'str', 'name': 'str' } }
> 
> {'command': 'dirty-bitmap-add',
>   'data': { 'map': 'DirtyBitmap', '*granularity': 'int' } }
> 
> Or:
> 
> { 'type': 'DirtyBitmap',
>   'data': { 'device': 'str', 'name': 'str' } }
> { 'type': 'DirtyBitmapGranularity',
>   'base': 'DirtyBitmap',
>   'data': { '*granularity': 'int' } }
> {'command': 'dirty-bitmap-add',
>   'data': 'DirtyBitmapGranularity' }
> 
> 
> which says that the 'DirtyBitmap' struct has no optional members, and
> instead of silently ignoring an optional member in 3 commands, we
> instead write the one command that takes the optional argument when we
> actually care about it.
> 

Yes, taking the later one since a type is needed for transaction support.
Thanks,

Fam

[Qemu-devel] [PATCH v4 3/9] block: Handle error of bdrv_getlength in bdrv_create_dirty_bitmap

[Fam Zheng]

bdrv_getlength could fail, check the return value before using it.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 block.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 3f880d6..b5265bb 100644
--- a/block.c
+++ b/block.c
@@ -5113,7 +5113,12 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
     }
     granularity >>= BDRV_SECTOR_BITS;
     assert(granularity);
-    bitmap_size = (bdrv_getlength(bs) >> BDRV_SECTOR_BITS);
+    bitmap_size = bdrv_getlength(bs);
+    if (bitmap_size < 0) {
+        error_setg(errp, "could not get length of device");
+        return NULL;
+    }
+    bitmap_size >>= BDRV_SECTOR_BITS;
     bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
     bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
     if (name) {
-- 
1.9.1

[Qemu-devel] [PATCH v4 4/9] block: Introduce bdrv_dirty_bitmap_granularity()

[Fam Zheng]

This returns the granularity (in sectors) of dirty bitmap.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 block.c               | 6 ++++++
 include/block/block.h | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/block.c b/block.c
index b5265bb..6b82bf0 100644
--- a/block.c
+++ b/block.c
@@ -5173,6 +5173,12 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
     }
 }
 
+int bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
+                                  BdrvDirtyBitmap *bitmap)
+{
+    return hbitmap_granularity(bitmap->bitmap);
+}
+
 void bdrv_dirty_iter_init(BlockDriverState *bs,
                           BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
 {
diff --git a/include/block/block.h b/include/block/block.h
index aa30b2a..ed85f85 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -437,6 +437,8 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
 void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
+int bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
+                                  BdrvDirtyBitmap *bitmap);
 int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
 void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
 void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
-- 
1.9.1

[Qemu-devel] [PATCH v4 5/9] hbitmap: Add hbitmap_copy

[Fam Zheng]

This makes a deep copy of an HBitmap.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/qemu/hbitmap.h |  8 ++++++++
 util/hbitmap.c         | 16 ++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index 550d7ce..b645cfc 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -65,6 +65,14 @@ struct HBitmapIter {
 HBitmap *hbitmap_alloc(uint64_t size, int granularity);
 
 /**
+ * hbitmap_copy:
+ * @bitmap: The original bitmap to copy.
+ *
+ * Copy a HBitmap.
+ */
+HBitmap *hbitmap_copy(const HBitmap *bitmap);
+
+/**
  * hbitmap_empty:
  * @hb: HBitmap to operate on.
  *
diff --git a/util/hbitmap.c b/util/hbitmap.c
index d936831..d906c06 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -400,3 +400,19 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
     hb->levels[0][0] |= 1UL << (BITS_PER_LONG - 1);
     return hb;
 }
+
+HBitmap *hbitmap_copy(const HBitmap *bitmap)
+{
+    int i;
+    int64_t size;
+    HBitmap *hb = g_memdup(bitmap, sizeof(struct HBitmap));
+
+    size = bitmap->size;
+    for (i = HBITMAP_LEVELS; i-- > 0; ) {
+        size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
+        hb->levels[i] = g_memdup(bitmap->levels[i],
+                                 size * sizeof(unsigned long));
+    }
+
+    return hb;
+}
-- 
1.9.1

[Qemu-devel] [PATCH v4 6/9] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c               | 30 ++++++++++++++++++++++++++++--
 include/block/block.h |  4 ++++
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 6b82bf0..0abc593 100644
--- a/block.c
+++ b/block.c
@@ -52,6 +52,8 @@
 
 struct BdrvDirtyBitmap {
     HBitmap *bitmap;
+    int64_t size;
+    int64_t granularity;
     char *name;
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
@@ -5097,6 +5099,29 @@ void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
     bitmap->name = NULL;
 }
 
+BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
+                                        const BdrvDirtyBitmap *bitmap,
+                                        const char *name)
+{
+    BdrvDirtyBitmap *new_bitmap;
+
+    new_bitmap = g_memdup(bitmap, sizeof(BdrvDirtyBitmap));
+    new_bitmap->bitmap = hbitmap_copy(bitmap->bitmap);
+    if (name) {
+        new_bitmap->name = g_strdup(name);
+    }
+    QLIST_INSERT_HEAD(&bs->dirty_bitmaps, new_bitmap, list);
+    return new_bitmap;
+}
+
+void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+    HBitmap *original = bitmap->bitmap;
+
+    bitmap->bitmap = hbitmap_alloc(bitmap->size, bitmap->granularity);
+    hbitmap_free(original);
+}
+
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
                                           int granularity,
                                           const char *name,
@@ -5118,9 +5143,10 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
         error_setg(errp, "could not get length of device");
         return NULL;
     }
-    bitmap_size >>= BDRV_SECTOR_BITS;
     bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
-    bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
+    bitmap->size = bitmap_size >> BDRV_SECTOR_BITS;
+    bitmap->granularity = ffs(granularity) - 1;
+    bitmap->bitmap = hbitmap_alloc(bitmap->size, bitmap->granularity);
     if (name) {
         bitmap->name = g_strdup(name);
     }
diff --git a/include/block/block.h b/include/block/block.h
index ed85f85..a29169f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -435,6 +435,10 @@ 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);
+void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
+                                        const BdrvDirtyBitmap *bitmap,
+                                        const char *name);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 int bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 6/9] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap

[Stefan Hajnoczi]

On Thu, Mar 27, 2014 at 05:09:45PM +0800, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c               | 30 ++++++++++++++++++++++++++++--
>  include/block/block.h |  4 ++++
>  2 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 6b82bf0..0abc593 100644
> --- a/block.c
> +++ b/block.c
> @@ -52,6 +52,8 @@
>  
>  struct BdrvDirtyBitmap {
>      HBitmap *bitmap;
> +    int64_t size;
> +    int64_t granularity;

HBitmap *hbitmap_alloc(uint64_t size, int granularity)

Please use the same types as hbitmap_alloc() for size and granularity.
This way there's less chance of truncation, signedness, or
overflow/underflow problems later.  Code becomes messy if types are
inconsistent.

> +void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +{
> +    HBitmap *original = bitmap->bitmap;
> +
> +    bitmap->bitmap = hbitmap_alloc(bitmap->size, bitmap->granularity);
> +    hbitmap_free(original);
> +}

hbitmap_reset() exists, why allocate a new instance?  If speed is a
concern then add a special case to hbitmap_reset() for clearing the
entire bitmap quickly.  That way all callers benefit and don't have to
work around the missing functionality like you did here.

[Qemu-devel] [PATCH v4 7/9] qmp: Add dirty-bitmap-enable and dirty-bitmap-disable

[Fam Zheng]

This allows to put the dirty bitmap into a disabled state where no more
writes will be tracked.

It will be used before backup or writing to persistent file.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c               | 15 +++++++++++++++
 blockdev.c            | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  2 ++
 qapi-schema.json      | 32 ++++++++++++++++++++++++++++++++
 qmp-commands.hx       | 10 ++++++++++
 5 files changed, 103 insertions(+)

diff --git a/block.c b/block.c
index 0abc593..f396f92 100644
--- a/block.c
+++ b/block.c
@@ -55,6 +55,7 @@ struct BdrvDirtyBitmap {
     int64_t size;
     int64_t granularity;
     char *name;
+    bool enabled;
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -5150,6 +5151,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
     if (name) {
         bitmap->name = g_strdup(name);
     }
+    bitmap->enabled = true;
     QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
     return bitmap;
 }
@@ -5168,6 +5170,16 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
     }
 }
 
+void bdrv_disable_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+    bitmap->enabled = false;
+}
+
+void bdrv_enable_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+    bitmap->enabled = true;
+}
+
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
 {
     BdrvDirtyBitmap *bm;
@@ -5216,6 +5228,9 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
 {
     BdrvDirtyBitmap *bitmap;
     QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+        if (!bitmap->enabled) {
+            continue;
+        }
         hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
     }
 }
diff --git a/blockdev.c b/blockdev.c
index 662c950..aa3ee55 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1773,6 +1773,50 @@ void qmp_dirty_bitmap_remove(const char *device, const char *name,
     bdrv_release_dirty_bitmap(bs, bitmap);
 }
 
+void qmp_dirty_bitmap_enable(const char *device, const char *name,
+                             bool has_granularity, int64_t granularity,
+                             Error **errp)
+{
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    bitmap = bdrv_find_dirty_bitmap(bs, name);
+    if (!bitmap) {
+        error_setg(errp, "Dirty bitmap not found: %s", name);
+        return;
+    }
+
+    bdrv_enable_dirty_bitmap(bs, bitmap);
+}
+
+void qmp_dirty_bitmap_disable(const char *device, const char *name,
+                              bool has_granularity, int64_t granularity,
+                              Error **errp)
+{
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    bitmap = bdrv_find_dirty_bitmap(bs, name);
+    if (!bitmap) {
+        error_setg(errp, "Dirty bitmap not found: %s", name);
+        return;
+    }
+
+    bdrv_disable_dirty_bitmap(bs, bitmap);
+}
+
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     const char *id = qdict_get_str(qdict, "id");
diff --git a/include/block/block.h b/include/block/block.h
index a29169f..eef28df 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -440,6 +440,8 @@ BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
                                         const BdrvDirtyBitmap *bitmap,
                                         const char *name);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_disable_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_enable_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 int bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
                                   BdrvDirtyBitmap *bitmap);
diff --git a/qapi-schema.json b/qapi-schema.json
index 56f16a9..cf1cc8a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2254,6 +2254,38 @@
   'data': { 'device': 'str', 'name': 'str' } }
 
 ##
+# @dirty-bitmap-enable
+#
+# Enable a dirty bitmap on the device
+#
+# Setting granularity has no effect here.
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If @name is not found, GenericError with an explaining message
+#
+# Since 2.1
+##
+{'command': 'dirty-bitmap-enable',
+  'data': 'DirtyBitmap' }
+
+##
+# @dirty-bitmap-disable
+#
+# Disable a dirty bitmap on the device
+#
+# Setting granularity has no effect here.
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If @name is not found, GenericError with an explaining message
+#
+# Since 2.1
+##
+{'command': 'dirty-bitmap-disable',
+  'data': 'DirtyBitmap' }
+
+##
 # @migrate_cancel
 #
 # Cancel the current executing migration process.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b74f6ed..c7ed39f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1194,6 +1194,16 @@ EQMP
         .args_type  = "device:B,name:s",
         .mhandler.cmd_new = qmp_marshal_input_dirty_bitmap_remove,
     },
+    {
+        .name       = "dirty-bitmap-enable",
+        .args_type  = "device:B,name:s",
+        .mhandler.cmd_new = qmp_marshal_input_dirty_bitmap_enable,
+    },
+    {
+        .name       = "dirty-bitmap-disable",
+        .args_type  = "device:B,name:s",
+        .mhandler.cmd_new = qmp_marshal_input_dirty_bitmap_disable,
+    },
 
 SQMP
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 7/9] qmp: Add dirty-bitmap-enable and dirty-bitmap-disable

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1906 bytes --]

On 03/27/2014 03:09 AM, Fam Zheng wrote:
> This allows to put the dirty bitmap into a disabled state where no more
> writes will be tracked.
> 
> It will be used before backup or writing to persistent file.

Is it worth the add-bitmap command being able to add a bitmap in the
disabled state to begin with?  I'm trying to figure out if there is a
race between creating a bitmap and disabling it that might matter to any
control flow, where being able to create it disabled would break that race.

> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c               | 15 +++++++++++++++
>  blockdev.c            | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h |  2 ++
>  qapi-schema.json      | 32 ++++++++++++++++++++++++++++++++
>  qmp-commands.hx       | 10 ++++++++++
>  5 files changed, 103 insertions(+)
> 
> +++ b/qapi-schema.json
> @@ -2254,6 +2254,38 @@
>    'data': { 'device': 'str', 'name': 'str' } }
>  
>  ##
> +# @dirty-bitmap-enable
> +#
> +# Enable a dirty bitmap on the device
> +#
> +# Setting granularity has no effect here.

See my comments on 2/9 about how this looks odd.

> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If @name is not found, GenericError with an explaining message
> +#
> +# Since 2.1
> +##
> +{'command': 'dirty-bitmap-enable',
> +  'data': 'DirtyBitmap' }

Is it worth consolidating this into one command with a 'bool' parameter
that says whether to enable or disable, instead of two separate
commands?  Also, is there a counterpart query- command that I can use to
see the current state of a named dirty bitmap and whether it is
currently enabled, so that this isn't a write-only interface?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v4 7/9] qmp: Add dirty-bitmap-enable and dirty-bitmap-disable

[Fam Zheng]

On Thu, 03/27 10:46, Eric Blake wrote:
> On 03/27/2014 03:09 AM, Fam Zheng wrote:
>  Also, is there a counterpart query- command that I can use to
> see the current state of a named dirty bitmap and whether it is
> currently enabled, so that this isn't a write-only interface?
> 

Will add the enable status into query-block information in another patch.

Thanks,
Fam

[Qemu-devel] [PATCH v4 8/9] qmp: Add support of "dirty-bitmap" sync mode for drive-backup

[Fam Zheng]

For "dirty-bitmap" sync mode, the block job will iterate through the
given dirty bitmap to decide if a sector needs backup (backup all the
dirty clusters and skip clean ones), just as allocation conditions of
"top" sync mode.

There are two bitmap use modes for sync=dirty-bitmap:

 - reset: backup job makes a copy of bitmap and resets the original
   one.
 - consume: backup job makes the original anonymous (invisible to user)
   and releases it after use.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/backup.c            | 53 ++++++++++++++++++++++++++++++++++++++++++++++-
 block/mirror.c            |  4 ++++
 blockdev.c                |  9 +++++++-
 hmp.c                     |  4 +++-
 include/block/block_int.h |  3 +++
 qapi-schema.json          | 32 ++++++++++++++++++++++++----
 qmp-commands.hx           |  7 ++++---
 7 files changed, 102 insertions(+), 10 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 15a2e55..24b8d2c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -37,6 +37,10 @@ typedef struct CowRequest {
 typedef struct BackupBlockJob {
     BlockJob common;
     BlockDriverState *target;
+    /* bitmap for sync=dirty-bitmap */
+    BdrvDirtyBitmap *sync_bitmap;
+    /* dirty bitmap granularity */
+    int sync_bitmap_gran;
     MirrorSyncMode sync_mode;
     RateLimit limit;
     BlockdevOnError on_source_error;
@@ -263,7 +267,7 @@ static void coroutine_fn backup_run(void *opaque)
             job->common.busy = true;
         }
     } else {
-        /* Both FULL and TOP SYNC_MODE's require copying.. */
+        /* FULL, TOP and DIRTY_BITMAP SYNC_MODE's require copying.. */
         for (; start < end; start++) {
             bool error_is_read;
 
@@ -317,7 +321,21 @@ static void coroutine_fn backup_run(void *opaque)
                 if (alloced == 0) {
                     continue;
                 }
+            } else if (job->sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+                int i, dirty = 0;
+                for (i = 0; i < BACKUP_SECTORS_PER_CLUSTER;
+                     i += job->sync_bitmap_gran) {
+                    if (bdrv_get_dirty(bs, job->sync_bitmap,
+                            start * BACKUP_SECTORS_PER_CLUSTER + i)) {
+                        dirty = 1;
+                        break;
+                    }
+                }
+                if (!dirty) {
+                    continue;
+                }
             }
+
             /* FULL sync mode we copy the whole drive. */
             ret = backup_do_cow(bs, start * BACKUP_SECTORS_PER_CLUSTER,
                     BACKUP_SECTORS_PER_CLUSTER, &error_is_read);
@@ -341,6 +359,9 @@ static void coroutine_fn backup_run(void *opaque)
     qemu_co_rwlock_wrlock(&job->flush_rwlock);
     qemu_co_rwlock_unlock(&job->flush_rwlock);
 
+    if (job->sync_bitmap) {
+        bdrv_release_dirty_bitmap(bs, job->sync_bitmap);
+    }
     hbitmap_free(job->bitmap);
 
     bdrv_iostatus_disable(target);
@@ -351,12 +372,15 @@ static void coroutine_fn backup_run(void *opaque)
 
 void backup_start(BlockDriverState *bs, BlockDriverState *target,
                   int64_t speed, MirrorSyncMode sync_mode,
+                  BdrvDirtyBitmap *sync_bitmap,
+                  BitmapUseMode bitmap_mode,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   BlockDriverCompletionFunc *cb, void *opaque,
                   Error **errp)
 {
     int64_t len;
+    BdrvDirtyBitmap *original;
 
     assert(bs);
     assert(target);
@@ -369,6 +393,28 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 
+    if (sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP && !sync_bitmap) {
+        error_setg(errp, "must provide a valid bitmap name for \"dirty-bitmap\""
+                         "sync mode");
+        return;
+    }
+
+    if (sync_bitmap) {
+        switch (bitmap_mode) {
+        case BITMAP_USE_MODE_RESET:
+            original = sync_bitmap;
+            sync_bitmap = bdrv_copy_dirty_bitmap(bs, sync_bitmap, NULL);
+            bdrv_reset_dirty_bitmap(bs, original);
+            break;
+        case BITMAP_USE_MODE_CONSUME:
+            bdrv_dirty_bitmap_make_anon(bs, sync_bitmap);
+            break;
+        default:
+            assert(0);
+        }
+        bdrv_disable_dirty_bitmap(bs, sync_bitmap);
+    }
+
     len = bdrv_getlength(bs);
     if (len < 0) {
         error_setg_errno(errp, -len, "unable to get length for '%s'",
@@ -386,6 +432,11 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
     job->on_target_error = on_target_error;
     job->target = target;
     job->sync_mode = sync_mode;
+    job->sync_bitmap = sync_bitmap;
+    if (sync_bitmap) {
+        job->sync_bitmap_gran =
+            bdrv_dirty_bitmap_granularity(bs, job->sync_bitmap);
+    }
     job->common.len = len;
     job->common.co = qemu_coroutine_create(backup_run);
     qemu_coroutine_enter(job->common.co, job);
diff --git a/block/mirror.c b/block/mirror.c
index 9b73cd9..faf7427 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -624,6 +624,10 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
     bool is_none_mode;
     BlockDriverState *base;
 
+    if (mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+        error_setg(errp, "Sync mode 'dirty-bitmap' not supported");
+        return;
+    }
     is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
     base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL;
     mirror_start_job(bs, target, speed, granularity, buf_size,
diff --git a/blockdev.c b/blockdev.c
index aa3ee55..4120dee 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1384,6 +1384,8 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
                      backup->sync,
                      backup->has_mode, backup->mode,
                      backup->has_speed, backup->speed,
+                     backup->has_bitmap, backup->bitmap,
+                     backup->has_bitmap_use_mode, backup->bitmap_use_mode,
                      backup->has_on_source_error, backup->on_source_error,
                      backup->has_on_target_error, backup->on_target_error,
                      &local_err);
@@ -2032,6 +2034,8 @@ void qmp_drive_backup(const char *device, const char *target,
                       enum MirrorSyncMode sync,
                       bool has_mode, enum NewImageMode mode,
                       bool has_speed, int64_t speed,
+                      bool has_bitmap, const char *bitmap,
+                      bool has_bitmap_use_mode, enum BitmapUseMode bitmap_mode,
                       bool has_on_source_error, BlockdevOnError on_source_error,
                       bool has_on_target_error, BlockdevOnError on_target_error,
                       Error **errp)
@@ -2129,7 +2133,10 @@ void qmp_drive_backup(const char *device, const char *target,
         return;
     }
 
-    backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
+    backup_start(bs, target_bs, speed, sync,
+                 has_bitmap ? bdrv_find_dirty_bitmap(bs, bitmap) : NULL,
+                 has_bitmap_use_mode ? bitmap_mode : BITMAP_USE_MODE_RESET,
+                 on_source_error, on_target_error,
                  block_job_cb, bs, &local_err);
     if (local_err != NULL) {
         bdrv_unref(target_bs);
diff --git a/hmp.c b/hmp.c
index 2f279c4..2c84377 100644
--- a/hmp.c
+++ b/hmp.c
@@ -950,7 +950,9 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
 
     qmp_drive_backup(device, filename, !!format, format,
                      full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
-                     true, mode, false, 0, false, 0, false, 0, &errp);
+                     true, mode, false, 0, false, NULL,
+                     false, 0,
+                     false, 0, false, 0, &errp);
     hmp_handle_error(mon, &errp);
 }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index cd5bc73..5f9de10 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -484,6 +484,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
  * @target: Block device to write to.
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @sync_mode: What parts of the disk image should be copied to the destination.
+ * @sync_bitmap: The dirty bitmap if sync_mode is MIRROR_SYNC_MODE_DIRTY_BITMAP.
  * @on_source_error: The action to take upon error reading from the source.
  * @on_target_error: The action to take upon error writing to the target.
  * @cb: Completion function for the job.
@@ -494,6 +495,8 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
  */
 void backup_start(BlockDriverState *bs, BlockDriverState *target,
                   int64_t speed, MirrorSyncMode sync_mode,
+                  BdrvDirtyBitmap *sync_bitmap,
+                  BitmapUseMode bitmap_mode,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   BlockDriverCompletionFunc *cb, void *opaque,
diff --git a/qapi-schema.json b/qapi-schema.json
index cf1cc8a..d99041c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1529,10 +1529,12 @@
 #
 # @none: only copy data written from now on
 #
+# @dirty-bitmap: only copy dirty data according to the bitmap (Since 2.1)
+#
 # Since: 1.3
 ##
 { 'enum': 'MirrorSyncMode',
-  'data': ['top', 'full', 'none'] }
+  'data': ['top', 'full', 'none', 'dirty-bitmap'] }
 
 ##
 # @BlockJobType:
@@ -1908,6 +1910,21 @@
   'data': { 'device': 'str', 'name': 'str' } }
 
 ##
+# @BitmapUseMode
+#
+# An enumeration that tells QEMU what operation to take when using a bitmap
+# in drive backup sync mode.
+#
+# @consume: QEMU should just consume the bitmap and release it after using
+#
+# @reset: QEMU should reset the dirty bitmap
+#
+# Since: 2.1
+##
+{ 'enum': 'BitmapUseMode',
+  'data': [ 'consume', 'reset' ] }
+
+##
 # @DriveBackup
 #
 # @device: the name of the device which should be copied.
@@ -1920,14 +1937,20 @@
 #          probe if @mode is 'existing', else the format of the source
 #
 # @sync: what parts of the disk image should be copied to the destination
-#        (all the disk, only the sectors allocated in the topmost image, or
-#        only new I/O).
+#        (all the disk, only the sectors allocated in the topmost image, from a
+#        dirty bitmap, or only new I/O).
 #
 # @mode: #optional whether and how QEMU should create a new image, default is
 #        'absolute-paths'.
 #
 # @speed: #optional the maximum speed, in bytes per second
 #
+# @bitmap: #optional the name of dirty bitmap if sync is "dirty-bitmap"
+#          (Since 2.1)
+#
+# @bitmap-use-mode: #optional which operation to take when consuming @bitmap,
+#                   default is reset. (Since 2.1)
+#
 # @on-source-error: #optional the action to take on an error on the source,
 #                   default 'report'.  'stop' and 'enospc' can only be used
 #                   if the block device supports io-status (see BlockInfo).
@@ -1945,7 +1968,8 @@
 { 'type': 'DriveBackup',
   'data': { 'device': 'str', 'target': 'str', '*format': 'str',
             'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
-            '*speed': 'int',
+            '*speed': 'int', '*bitmap': 'str',
+            '*bitmap-use-mode': 'BitmapUseMode',
             '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError' } }
 
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c7ed39f..420a901 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1031,7 +1031,7 @@ EQMP
     {
         .name       = "drive-backup",
         .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
-                      "on-source-error:s?,on-target-error:s?",
+                      "bitmap:s?,on-source-error:s?,on-target-error:s?",
         .mhandler.cmd_new = qmp_marshal_input_drive_backup,
     },
 
@@ -1058,8 +1058,9 @@ Arguments:
             (json-string, optional)
 - "sync": what parts of the disk image should be copied to the destination;
   possibilities include "full" for all the disk, "top" for only the sectors
-  allocated in the topmost image, or "none" to only replicate new I/O
-  (MirrorSyncMode).
+  allocated in the topmost image, "dirty-bitmap" for only the dirty sectors in
+  the bitmap, or "none" to only replicate new I/O (MirrorSyncMode).
+- "bitmap": bitmap name to use for sync==dirty-bitmap
 - "mode": whether and how QEMU should create a new image
           (NewImageMode, optional, default 'absolute-paths')
 - "speed": the maximum speed, in bytes per second (json-int, optional)
-- 
1.9.1

[Qemu-devel] [PATCH v4 9/9] qapi: Add transaction support to dirty-bitmap-{add, disable}

[Fam Zheng]

This adds dirty-bitmap-add and dirty-bitmap-disable to transactions.
With this, user can stop a dirty bitmap, start backup of it, and start
another dirty bitmap atomically, so that the dirty bitmap is tracked
incrementally and we don't miss any write.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c       | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |  4 +++-
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 4120dee..38dabe6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1411,6 +1411,64 @@ static void drive_backup_abort(BlkTransactionState *common)
     }
 }
 
+static void dirty_bitmap_add_prepare(BlkTransactionState *common, Error **errp)
+{
+    DirtyBitmap *action;
+    Error *local_err = NULL;
+
+    action = common->action->dirty_bitmap_add;
+    qmp_dirty_bitmap_add(action->device, action->name, false, 0, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+    }
+}
+
+static void dirty_bitmap_add_abort(BlkTransactionState *common)
+{
+    DirtyBitmap *action;
+    BdrvDirtyBitmap *bm;
+    BlockDriverState *bs;
+
+    action = common->action->dirty_bitmap_add;
+    bs = bdrv_find(action->device);
+    if (bs) {
+        bm = bdrv_find_dirty_bitmap(bs, action->name);
+        if (bm) {
+            bdrv_release_dirty_bitmap(bs, bm);
+        }
+    }
+}
+
+static void dirty_bitmap_disable_prepare(BlkTransactionState *common,
+                                         Error **errp)
+{
+    DirtyBitmap *action;
+    Error *local_err = NULL;
+
+    action = common->action->dirty_bitmap_disable;
+    qmp_dirty_bitmap_disable(action->device, action->name,
+                             false, 0, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+    }
+}
+
+static void dirty_bitmap_disable_abort(BlkTransactionState *common)
+{
+    DirtyBitmap *action;
+    BdrvDirtyBitmap *bitmap;
+    BlockDriverState *bs;
+
+    action = common->action->dirty_bitmap_disable;
+    bs = bdrv_find(action->device);
+    if (bs) {
+        bitmap = bdrv_find_dirty_bitmap(bs, action->name);
+        if (bitmap) {
+            bdrv_enable_dirty_bitmap(bs, bitmap);
+        }
+    }
+}
+
 static void abort_prepare(BlkTransactionState *common, Error **errp)
 {
     error_setg(errp, "Transaction aborted using Abort action");
@@ -1443,6 +1501,16 @@ static const BdrvActionOps actions[] = {
         .prepare  = internal_snapshot_prepare,
         .abort = internal_snapshot_abort,
     },
+    [TRANSACTION_ACTION_KIND_DIRTY_BITMAP_ADD] = {
+        .instance_size = sizeof(BlkTransactionState),
+        .prepare = dirty_bitmap_add_prepare,
+        .abort = dirty_bitmap_add_abort,
+    },
+    [TRANSACTION_ACTION_KIND_DIRTY_BITMAP_DISABLE] = {
+        .instance_size = sizeof(BlkTransactionState),
+        .prepare = dirty_bitmap_disable_prepare,
+        .abort = dirty_bitmap_disable_abort,
+    },
 };
 
 /*
diff --git a/qapi-schema.json b/qapi-schema.json
index d99041c..48f7c15 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1994,7 +1994,9 @@
        'blockdev-snapshot-sync': 'BlockdevSnapshot',
        'drive-backup': 'DriveBackup',
        'abort': 'Abort',
-       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
+       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
+       'dirty-bitmap-add': 'DirtyBitmap',
+       'dirty-bitmap-disable': 'DirtyBitmap'
    } }
 
 ##
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 9/9] qapi: Add transaction support to dirty-bitmap-{add, disable}

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1533 bytes --]

On 03/27/2014 03:09 AM, Fam Zheng wrote:
> This adds dirty-bitmap-add and dirty-bitmap-disable to transactions.
> With this, user can stop a dirty bitmap, start backup of it, and start
> another dirty bitmap atomically, so that the dirty bitmap is tracked
> incrementally and we don't miss any write.

Ah, this addresses my question in 7/9 about atomicity of creating a map
in disabled state.  Nice idea to use 'transaction' to merge the two
commands, instead of having to bloat the creation with ever more options.

> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c       | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json |  4 +++-
>  2 files changed, 71 insertions(+), 1 deletion(-)
> 
> +++ b/qapi-schema.json
> @@ -1994,7 +1994,9 @@
>         'blockdev-snapshot-sync': 'BlockdevSnapshot',
>         'drive-backup': 'DriveBackup',
>         'abort': 'Abort',
> -       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
> +       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
> +       'dirty-bitmap-add': 'DirtyBitmap',
> +       'dirty-bitmap-disable': 'DirtyBitmap'
>     } }

However, shouldn't it ALSO be possible to enable a bitmap in tandem with
other transaction operations?  I can imagine a transaction that both
kicks off a snapshot and enables a bitmap that was previously added but
left disabled.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]