[Qemu-devel] [PATCH v6 00/10] block: Incremental backup series

[Qemu-devel] [PATCH v6 00/10] block: Incremental backup series

[Fam Zheng]

v6: Rebased v4 of the series on top of qemu.git. (skipping v5 since it was used
    by me as a private sending, for those who received it, the code is the same
    :)

This is the in memory part of the incremental backup feature.

With the added commands, we can create a bitmap on a block backend, from which
point of time all the writes are tracked by the bitmap, marking sectors as
dirty.  Later, we call drive-backup and pass the bitmap to it, to do an
incremental backup.

See the last patch which adds some tests for this use case.

Fam

Fam Zheng (10):
  qapi: Add optional field "name" to block dirty bitmap
  qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove
  block: Introduce bdrv_dirty_bitmap_granularity()
  hbitmap: Add hbitmap_copy
  block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap
  qmp: Add block-dirty-bitmap-enable and block-dirty-bitmap-disable
  qmp: Add support of "dirty-bitmap" sync mode for drive-backup
  qapi: Add transaction support to
    block-dirty-bitmap-{add,enable,disable}
  qmp: Add dirty bitmap 'enabled' field in query-block
  qemu-iotests: Add tests for drive-backup sync=dirty-bitmap

 block-migration.c             |   2 +-
 block.c                       |  82 ++++++++++++++++-
 block/backup.c                |  54 +++++++++++-
 block/mirror.c                |   6 +-
 blockdev.c                    | 198 +++++++++++++++++++++++++++++++++++++++++-
 hmp.c                         |   4 +-
 include/block/block.h         |  15 +++-
 include/block/block_int.h     |   4 +
 include/qemu/hbitmap.h        |   8 ++
 qapi-schema.json              |   5 +-
 qapi/block-core.json          | 120 +++++++++++++++++++++++--
 qmp-commands.hx               |  66 +++++++++++++-
 tests/qemu-iotests/056        |  33 ++++++-
 tests/qemu-iotests/056.out    |   4 +-
 tests/qemu-iotests/iotests.py |   8 ++
 util/hbitmap.c                |  16 ++++
 16 files changed, 603 insertions(+), 22 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v6 01/10] 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     |  2 +-
 block.c               | 34 +++++++++++++++++++++++++++++++++-
 block/mirror.c        |  2 +-
 include/block/block.h |  7 ++++++-
 qapi/block-core.json  |  4 +++-
 5 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index 08db01a..6f3aa18 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -319,7 +319,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);
         if (!bmds->dirty_bitmap) {
             ret = -errno;
             goto fail;
diff --git a/block.c b/block.c
index 88f6d9b..dafde4b 100644
--- a/block.c
+++ b/block.c
@@ -53,6 +53,7 @@
 
 struct BdrvDirtyBitmap {
     HBitmap *bitmap;
+    char *name;
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -5260,7 +5261,28 @@ 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;
+
+    assert(name);
+    QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
+        if (bm->name && !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;
@@ -5268,6 +5290,10 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
 
     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_nb_sectors(bs);
@@ -5278,6 +5304,9 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
     }
     bitmap = g_new0(BdrvDirtyBitmap, 1);
     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;
 }
@@ -5289,6 +5318,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;
         }
@@ -5307,6 +5337,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;
+        info->name = info->has_name ? g_strdup(bm->name) : NULL;
         entry->value = info;
         *plist = entry;
         plist = &entry->next;
diff --git a/block/mirror.c b/block/mirror.c
index e8a43eb..cecd448 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -656,7 +656,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, errp);
+    s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
     if (!s->dirty_bitmap) {
         return;
     }
diff --git a/include/block/block.h b/include/block/block.h
index 341054d..b0ef82c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -419,8 +419,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/block-core.json b/qapi/block-core.json
index 8f7089e..62b0356 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -307,6 +307,8 @@
 #
 # Block dirty bitmap information.
 #
+# @name: #optional the name of the dirty bitmap (Since 2.3)
+#
 # @count: number of dirty bytes according to the dirty bitmap
 #
 # @granularity: granularity of the dirty bitmap in bytes (since 1.4)
@@ -314,7 +316,7 @@
 # Since: 1.3
 ##
 { 'type': 'BlockDirtyInfo',
-  'data': {'count': 'int', 'granularity': 'int'} }
+  'data': {'*name': 'str', 'count': 'int', 'granularity': 'int'} }
 
 ##
 # @BlockInfo:
-- 
1.9.3

Re: [Qemu-devel] [PATCH v6 01/10] qapi: Add optional field "name" to block dirty bitmap

[Max Reitz]

On 2014-10-30 at 04:22, Fam Zheng wrote:
> 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     |  2 +-
>   block.c               | 34 +++++++++++++++++++++++++++++++++-
>   block/mirror.c        |  2 +-
>   include/block/block.h |  7 ++++++-
>   qapi/block-core.json  |  4 +++-
>   5 files changed, 44 insertions(+), 5 deletions(-)

[snip]

> diff --git a/block.c b/block.c
> index 88f6d9b..dafde4b 100644
> --- a/block.c
> +++ b/block.c
> @@ -5307,6 +5337,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;

Simply info->has_name = bm->name would have sufficed but it's probably 
clearer this way.

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

Re: [Qemu-devel] [PATCH v6 01/10] qapi: Add optional field "name" to block dirty bitmap

[Eric Blake]

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

On 10/30/2014 04:22 AM, Fam Zheng wrote:
> 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>
> ---

> @@ -5278,6 +5304,9 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
>      }
>      bitmap = g_new0(BdrvDirtyBitmap, 1);
>      bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
> +    if (name) {
> +        bitmap->name = g_strdup(name);
> +    }

No need for the 'if (name)' (g_strdup(NULL) does the right thing).

> @@ -5307,6 +5337,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;
> +        info->name = info->has_name ? g_strdup(bm->name) : NULL;

And again.

With those two changes,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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: 539 bytes --]

[Qemu-devel] [PATCH v6 02/10] qmp: Add block-dirty-bitmap-add and block-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           | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx      | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 159 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index eb743a9..025ba3a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1803,6 +1803,61 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
     aio_context_release(aio_context);
 }
 
+void qmp_block_dirty_bitmap_add(const char *device, const char *name,
+                                bool has_granularity, int64_t granularity,
+                                Error **errp)
+{
+    BlockDriverState *bs;
+
+    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 < 512 || is_power_of_2(granularity)) {
+            error_setg(errp, "Granularity must be power of 2 "
+                             "and greater than 512");
+            return;
+        }
+    } else {
+        granularity = 65536;
+    }
+
+    bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+}
+
+void qmp_block_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;
+    }
+
+    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/block-core.json b/qapi/block-core.json
index 62b0356..a162f63 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -865,6 +865,61 @@
             '*on-target-error': 'BlockdevOnError' } }
 
 ##
+# @BlockDirtyBitmap
+#
+# @device: name of device which the bitmap is tracking
+#
+# @name: name of the dirty bitmap
+#
+# Since 2.3
+##
+{ 'type': 'BlockDirtyBitmap',
+  'data': { 'device': 'str', 'name': 'str' } }
+
+##
+# @BlockDirtyBitmapAdd
+#
+# @device: name of device which the bitmap is tracking
+#
+# @name: name of the dirty bitmap
+#
+# @granularity: #optional the bitmap granularity, default is 64k for
+#               block-dirty-bitmap-add
+#
+# Since 2.3
+##
+{ 'type': 'BlockDirtyBitmapAdd',
+  'data': { 'device': 'str', 'name': 'str', '*granularity': 'int' } }
+
+##
+# @block-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.3
+##
+{'command': 'block-dirty-bitmap-add',
+  'data': 'BlockDirtyBitmapAdd' }
+
+##
+# @block-dirty-bitmap-remove
+#
+# Remove a dirty bitmap on the device
+#
+# 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.3
+##
+{'command': 'block-dirty-bitmap-remove',
+  'data': 'BlockDirtyBitmap' }
+
+##
 # @block_set_io_throttle:
 #
 # Change I/O throttle limits for a block drive.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 1abd619..6f7e04c 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1202,6 +1202,55 @@ Example:
 EQMP
 
     {
+        .name       = "block-dirty-bitmap-add",
+        .args_type  = "device:B,name:s,granularity:i?",
+        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_add,
+    },
+    {
+        .name       = "block-dirty-bitmap-remove",
+        .args_type  = "device:B,name:s",
+        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove,
+    },
+
+SQMP
+
+block-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": "block-dirty-bitmap-add", "arguments": { "device": "drive0",
+                                                   "name": "bitmap0" } }
+<- { "return": {} }
+
+block-dirty-bitmap-remove
+----------------
+
+Stop write tracking and remove the dirty bitmap that was created with
+block-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": "block-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.3

Re: [Qemu-devel] [PATCH v6 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[Max Reitz]

On 2014-10-30 at 04:22, 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>
> ---
>   blockdev.c           | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   qapi/block-core.json | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   qmp-commands.hx      | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 159 insertions(+)

Sorry if there's already been a discussion about this; I did not review 
this series before this version.

You are mixing up devices and BDSs here: The variable names and the QMP 
descriptions clearly refer to devices while you are using bdrv_find() to 
look for a BDS. Luckily (?), bdrv_find() actually uses blk_by_name() and 
blk_bs(), so it really does find the BDS for a device.

So it is not really wrong because it does the right thing(TM), but I'd 
prefer an explicit call to blk_by_name() and then blk_bs(); this would 
confuse me less, at the least.

> diff --git a/blockdev.c b/blockdev.c
> index eb743a9..025ba3a 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1803,6 +1803,61 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
>       aio_context_release(aio_context);
>   }
>   
> +void qmp_block_dirty_bitmap_add(const char *device, const char *name,
> +                                bool has_granularity, int64_t granularity,
> +                                Error **errp)
> +{
> +    BlockDriverState *bs;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);

Yesterday I saw Markus posting 
http://wiki.qemu.org/CodeTransitions#Error_reporting which states to 
prefer error_setg() over error_set(). I don't know the reason for this, 
so I won't object to error_set(), though.

> +        return;
> +    }
> +
> +    if (!name || name[0] == '\0') {
> +        error_setg(errp, "Bitmap name cannot be empty");
> +        return;
> +    }
> +    if (has_granularity) {
> +        if (granularity < 512 || is_power_of_2(granularity)) {

I think you meant !is_power_of_2().

> +            error_setg(errp, "Granularity must be power of 2 "
> +                             "and greater than 512");
> +            return;
> +        }
> +    } else {
> +        granularity = 65536;
> +    }
> +
> +    bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> +}
> +
> +void qmp_block_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;
> +    }
> +
> +    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/block-core.json b/qapi/block-core.json
> index 62b0356..a162f63 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -865,6 +865,61 @@
>               '*on-target-error': 'BlockdevOnError' } }
>   
>   ##
> +# @BlockDirtyBitmap
> +#
> +# @device: name of device which the bitmap is tracking
> +#
> +# @name: name of the dirty bitmap
> +#
> +# Since 2.3
> +##
> +{ 'type': 'BlockDirtyBitmap',
> +  'data': { 'device': 'str', 'name': 'str' } }
> +
> +##
> +# @BlockDirtyBitmapAdd
> +#
> +# @device: name of device which the bitmap is tracking
> +#
> +# @name: name of the dirty bitmap
> +#
> +# @granularity: #optional the bitmap granularity, default is 64k for
> +#               block-dirty-bitmap-add
> +#
> +# Since 2.3
> +##
> +{ 'type': 'BlockDirtyBitmapAdd',
> +  'data': { 'device': 'str', 'name': 'str', '*granularity': 'int' } }

Is there a reason you're creating an own object for this?

> +
> +##
> +# @block-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.3
> +##
> +{'command': 'block-dirty-bitmap-add',
> +  'data': 'BlockDirtyBitmapAdd' }

Why not just inline it here? I can't imagine using BlockDirtyBitmapAdd 
somewhere else, but I haven't looked into the rest of the series yet, so 
maybe it is useful to separate it.

> +
> +##
> +# @block-dirty-bitmap-remove
> +#
> +# Remove a dirty bitmap on the device
> +#
> +# 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.3
> +##
> +{'command': 'block-dirty-bitmap-remove',
> +  'data': 'BlockDirtyBitmap' }

I can understand not inlining the arguments here, though.

> +
> +##
>   # @block_set_io_throttle:
>   #
>   # Change I/O throttle limits for a block drive.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 1abd619..6f7e04c 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1202,6 +1202,55 @@ Example:
>   EQMP
>   
>       {
> +        .name       = "block-dirty-bitmap-add",
> +        .args_type  = "device:B,name:s,granularity:i?",
> +        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_add,
> +    },
> +    {
> +        .name       = "block-dirty-bitmap-remove",
> +        .args_type  = "device:B,name:s",
> +        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove,
> +    },
> +
> +SQMP
> +
> +block-dirty-bitmap-add
> +----------------

The other commands seem to use as many dashes as there are characters in 
the header. It probably doesn't matter in the end but I have heard the 
argument of OCD before.

Max

> +
> +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": "block-dirty-bitmap-add", "arguments": { "device": "drive0",
> +                                                   "name": "bitmap0" } }
> +<- { "return": {} }
> +
> +block-dirty-bitmap-remove
> +----------------
> +
> +Stop write tracking and remove the dirty bitmap that was created with
> +block-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": "block-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,

Re: [Qemu-devel] [PATCH v6 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[Eric Blake]

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

On 10/30/2014 04:22 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>
> ---
>  blockdev.c           | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx      | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 159 insertions(+)
> 

> +    if (has_granularity) {
> +        if (granularity < 512 || is_power_of_2(granularity)) {
> +            error_setg(errp, "Granularity must be power of 2 "
> +                             "and greater than 512");

Assuming you fix the logic of the condition, the error message is still
wrong (you allow 512, not just greater than 512).


> +++ b/qapi/block-core.json
> @@ -865,6 +865,61 @@
>              '*on-target-error': 'BlockdevOnError' } }
>  
>  ##
> +# @BlockDirtyBitmap
> +#
> +# @device: name of device which the bitmap is tracking
> +#
> +# @name: name of the dirty bitmap
> +#
> +# Since 2.3
> +##
> +{ 'type': 'BlockDirtyBitmap',
> +  'data': { 'device': 'str', 'name': 'str' } }
> +
> +##
> +# @BlockDirtyBitmapAdd
> +#
> +# @device: name of device which the bitmap is tracking
> +#
> +# @name: name of the dirty bitmap
> +#
> +# @granularity: #optional the bitmap granularity, default is 64k for
> +#               block-dirty-bitmap-add

Do you still need to call out the command, given that it is the only
client of this type?

> +#
> +# Since 2.3
> +##
> +{ 'type': 'BlockDirtyBitmapAdd',
> +  'data': { 'device': 'str', 'name': 'str', '*granularity': 'int' } }

Is it worth using type inheritance, as in:

{ 'type': 'BlockDirtyBitmapAdd',
  'base': 'BlockDirtyBitmap',
  'data': { '*granularity': 'int' } }

> +
> +##
> +# @block-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

s/explaining message/explanation/ (or even ditch the error mentions;
fewer commands are bothering with it these days)


> +
> +block-dirty-bitmap-add
> +----------------

Pad out the --- to the length of the command name.

> +
> +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)

Inconsistent on whether you use trailing '.'

> +
> +Example:
> +
> +-> { "execute": "block-dirty-bitmap-add", "arguments": { "device": "drive0",
> +                                                   "name": "bitmap0" } }
> +<- { "return": {} }
> +
> +block-dirty-bitmap-remove
> +----------------

Pad out the ---

-- 
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: 539 bytes --]

[Qemu-devel] qmp-commands.hx and inherited types (Was: Re: [PATCH v6 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove)

[John Snow]

On 11/07/2014 08:00 AM, Eric Blake wrote:
> On 10/30/2014 04:22 AM, Fam Zheng wrote:

[snip]

>> +++ b/qapi/block-core.json
>> @@ -865,6 +865,61 @@
>>               '*on-target-error': 'BlockdevOnError' } }
>>
>>   ##
>> +# @BlockDirtyBitmap
>> +#
>> +# @device: name of device which the bitmap is tracking
>> +#
>> +# @name: name of the dirty bitmap
>> +#
>> +# Since 2.3
>> +##
>> +{ 'type': 'BlockDirtyBitmap',
>> +  'data': { 'device': 'str', 'name': 'str' } }
>> +
>> +##
>> +# @BlockDirtyBitmapAdd
>> +#
>> +# @device: name of device which the bitmap is tracking
>> +#
>> +# @name: name of the dirty bitmap
>> +#
>> +# @granularity: #optional the bitmap granularity, default is 64k for
>> +#               block-dirty-bitmap-add
>
> Do you still need to call out the command, given that it is the only
> client of this type?
>
>> +#
>> +# Since 2.3
>> +##
>> +{ 'type': 'BlockDirtyBitmapAdd',
>> +  'data': { 'device': 'str', 'name': 'str', '*granularity': 'int' } }
>
> Is it worth using type inheritance, as in:
>
> { 'type': 'BlockDirtyBitmapAdd',
>    'base': 'BlockDirtyBitmap',
>    'data': { '*granularity': 'int' } }
>

Strictly speaking, I would argue against inheritance here because 
"BlockDirtyBitmapAdd" is not "isa" "BlockDirtyBitmap". It's more of a 
"Hasa" relationship.

At any rate, I tried to implement this for giggles to see if I could, 
and ran into the following issue with which I'd be curious to get an 
answer for.

As an example, If you have some type:

{ 'type': 'example',
   'data': { 'foo': 'int' } }

And an extension of it:

{ 'type': 'academicExample'
   'base': 'example',
   'data': { 'bar': 'str' } }

How would you write a command that expected both "foo" and "bar"?
The following doesn't seem appropriate (the generated code SKIPS the 
base fields, which leads to missing arguments in the prototype:

{ 'command': 'academic-command',
   'data': 'academicExample' }

...

{
	.name = "academic-command",
	.args_type = "foo:i,bar:s",
	.mhandler.cmd_new = qmp_marshal_input_academic_command,
},


The generated prototype appears to skip the "foo" argument, including 
only the arguments associated with the base type, in this case, 'bar'.

Do we support this kind of use? I didn't see it in-use currently, but I 
only gave it a cursory skimming.

Re: [Qemu-devel] qmp-commands.hx and inherited types (Was: Re: [PATCH v6 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove)

[Eric Blake]

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

On 11/18/2014 09:44 AM, John Snow wrote:
>> Is it worth using type inheritance, as in:
>>
>> { 'type': 'BlockDirtyBitmapAdd',
>>    'base': 'BlockDirtyBitmap',
>>    'data': { '*granularity': 'int' } }
>>
> 
> Strictly speaking, I would argue against inheritance here because
> "BlockDirtyBitmapAdd" is not "isa" "BlockDirtyBitmap". It's more of a
> "Hasa" relationship.

Fair enough.

> 
> At any rate, I tried to implement this for giggles to see if I could,
> and ran into the following issue with which I'd be curious to get an
> answer for.
> 
> As an example, If you have some type:
> 
> { 'type': 'example',
>   'data': { 'foo': 'int' } }
> 
> And an extension of it:
> 
> { 'type': 'academicExample'
>   'base': 'example',
>   'data': { 'bar': 'str' } }
> 
> How would you write a command that expected both "foo" and "bar"?
> The following doesn't seem appropriate (the generated code SKIPS the
> base fields, which leads to missing arguments in the prototype:
> 
> { 'command': 'academic-command',
>   'data': 'academicExample' }

Ouch.  Sounds like a bug in the code generator.  Obviously, someone will
have to patch that (and add a testsuite entry to make sure it doesn't
regress) before we can rely on it.

> 
> ...
> 
> {
>     .name = "academic-command",
>     .args_type = "foo:i,bar:s",
>     .mhandler.cmd_new = qmp_marshal_input_academic_command,
> },
> 
> 
> The generated prototype appears to skip the "foo" argument, including
> only the arguments associated with the base type, in this case, 'bar'.
> 
> Do we support this kind of use? I didn't see it in-use currently, but I
> only gave it a cursory skimming.

We supposedly document it as working, but as no one is using it
(including no testsuite entry), I'm not surprised that it doesn't work yet.

-- 
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: 539 bytes --]

[Qemu-devel] [PATCH v6 03/10] 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 dafde4b..1b12541 100644
--- a/block.c
+++ b/block.c
@@ -5356,6 +5356,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 b0ef82c..87fa48e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -428,6 +428,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.3

Re: [Qemu-devel] [PATCH v6 03/10] block: Introduce bdrv_dirty_bitmap_granularity()

[Max Reitz]

On 2014-10-30 at 04:22, Fam Zheng wrote:
> This returns the granularity (in sectors) of dirty bitmap.

Actually, it does not.

> 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 dafde4b..1b12541 100644
> --- a/block.c
> +++ b/block.c
> @@ -5356,6 +5356,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);
> +}
> +

hbitmap_granularity() returns bitmap->bitmap->granularity. 
Unfortunately, that value is ffs(granularity >> BDRV_SECTOR_BITS) - 1, 
(= ffs(granularity) - 1 - BDRV_SECTOR_BITS) where "granularity" is the 
original granularity from when the dirty bitmap was created. Therefore, 
this should be 1 << hbitmap_granularity(bitmap->bitmap) to be the 
granularity in sectors.

See bdrv_query_dirty_bitmaps() (written by you :-)).

And frankly, I'd prefer this not to return the granularity in sectors 
but in bytes, because that's the unit the granularity is specified in 
when the dirty bitmap is created; also, as far as my understanding goes, 
we want to get away from sectors, so I wouldn't introduce a new function 
using that unit if there's no real need for it.

Max

>   void bdrv_dirty_iter_init(BlockDriverState *bs,
>                             BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
>   {
> diff --git a/include/block/block.h b/include/block/block.h
> index b0ef82c..87fa48e 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -428,6 +428,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);

[Qemu-devel] [PATCH v6 04/10] 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 b3060e6..78d449e 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -395,3 +395,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.3

Re: [Qemu-devel] [PATCH v6 04/10] hbitmap: Add hbitmap_copy

[Max Reitz]

On 2014-10-30 at 04:22, Fam Zheng wrote:
> 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 b3060e6..78d449e 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -395,3 +395,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;

Why not uint64_t, like HBitmap::size? Not that it matters in practice, I 
hope.

> +    HBitmap *hb = g_memdup(bitmap, sizeof(struct HBitmap));

"struct" can be omitted.

> +
> +    size = bitmap->size;
> +    for (i = HBITMAP_LEVELS; i-- > 0; ) {

Urgh... I'd prefer "for (i = HBITMAP_LEVELS - 1; i >= 0; i--)" though 
yours is correct and shorter.

> +        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;
> +}

With or without any of the changes:

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

[Qemu-devel] [PATCH v6 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap

[Fam Zheng]

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

diff --git a/block.c b/block.c
index 1b12541..39381cd 100644
--- a/block.c
+++ b/block.c
@@ -53,6 +53,8 @@
 
 struct BdrvDirtyBitmap {
     HBitmap *bitmap;
+    int64_t size;
+    int granularity;
     char *name;
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
@@ -5280,6 +5282,26 @@ 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_malloc0(sizeof(BdrvDirtyBitmap));
+    new_bitmap->bitmap = hbitmap_copy(bitmap->bitmap);
+    new_bitmap->size = bitmap->size;
+    new_bitmap->granularity = bitmap->granularity;
+    new_bitmap->name = name ? g_strdup(name) : NULL;
+    QLIST_INSERT_HEAD(&bs->dirty_bitmaps, new_bitmap, list);
+    return new_bitmap;
+}
+
+void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+    hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
+}
+
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
                                           int granularity,
                                           const char *name,
@@ -5303,7 +5325,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 = 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 87fa48e..c5a3aa6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -426,6 +426,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.3

Re: [Qemu-devel] [PATCH v6 05/10] block: Add bdrv_copy_dirty_bitmap and bdrv_reset_dirty_bitmap

[Max Reitz]

On 2014-10-30 at 04:22, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block.c               | 26 +++++++++++++++++++++++++-
>   include/block/block.h |  4 ++++
>   2 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index 1b12541..39381cd 100644
> --- a/block.c
> +++ b/block.c
> @@ -53,6 +53,8 @@
>   
>   struct BdrvDirtyBitmap {
>       HBitmap *bitmap;
> +    int64_t size;
> +    int granularity;

Why do you add this field when you never use it except for setting it in 
bdrv_create_dirty_bitmap()? I at least couldn't find any place which 
uses it by quickly scanning the rest of the series...

You could use it for bdrv_dirty_bitmap_granularity(), though.

>       char *name;
>       QLIST_ENTRY(BdrvDirtyBitmap) list;
>   };
> @@ -5280,6 +5282,26 @@ 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_malloc0(sizeof(BdrvDirtyBitmap));
> +    new_bitmap->bitmap = hbitmap_copy(bitmap->bitmap);
> +    new_bitmap->size = bitmap->size;
> +    new_bitmap->granularity = bitmap->granularity;
> +    new_bitmap->name = name ? g_strdup(name) : NULL;
> +    QLIST_INSERT_HEAD(&bs->dirty_bitmaps, new_bitmap, list);
> +    return new_bitmap;
> +}
> +
> +void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +{
> +    hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
> +}
> +
>   BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>                                             int granularity,
>                                             const char *name,
> @@ -5303,7 +5325,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 = 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 87fa48e..c5a3aa6 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -426,6 +426,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,

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

[Qemu-devel] [PATCH v6 06/10] qmp: Add block-dirty-bitmap-enable and block-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            | 42 ++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  2 ++
 qapi/block-core.json  | 28 ++++++++++++++++++++++++++++
 qmp-commands.hx       | 10 ++++++++++
 5 files changed, 97 insertions(+)

diff --git a/block.c b/block.c
index 39381cd..e574faa 100644
--- a/block.c
+++ b/block.c
@@ -56,6 +56,7 @@ struct BdrvDirtyBitmap {
     int64_t size;
     int granularity;
     char *name;
+    bool enabled;
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -5331,6 +5332,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;
 }
@@ -5349,6 +5351,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;
@@ -5397,6 +5409,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 025ba3a..c7e98ad 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1858,6 +1858,48 @@ void qmp_block_dirty_bitmap_remove(const char *device, const char *name,
     bdrv_release_dirty_bitmap(bs, bitmap);
 }
 
+void qmp_block_dirty_bitmap_enable(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;
+    }
+
+    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_block_dirty_bitmap_disable(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;
+    }
+
+    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 c5a3aa6..ef06cb6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -431,6 +431,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/block-core.json b/qapi/block-core.json
index a162f63..97c31bf 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -920,6 +920,34 @@
   'data': 'BlockDirtyBitmap' }
 
 ##
+# @block-dirty-bitmap-enable
+#
+# Enable a dirty bitmap on the device
+#
+# 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.3
+##
+{'command': 'block-dirty-bitmap-enable',
+  'data': 'BlockDirtyBitmap' }
+
+##
+# @block-dirty-bitmap-disable
+#
+# Disable a dirty bitmap on the device
+#
+# 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.3
+##
+{'command': 'block-dirty-bitmap-disable',
+  'data': 'BlockDirtyBitmap' }
+
+##
 # @block_set_io_throttle:
 #
 # Change I/O throttle limits for a block drive.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6f7e04c..6d0c944 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1211,6 +1211,16 @@ EQMP
         .args_type  = "device:B,name:s",
         .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove,
     },
+    {
+        .name       = "block-dirty-bitmap-enable",
+        .args_type  = "device:B,name:s",
+        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_enable,
+    },
+    {
+        .name       = "block-dirty-bitmap-disable",
+        .args_type  = "device:B,name:s",
+        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_disable,
+    },
 
 SQMP
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH v6 06/10] qmp: Add block-dirty-bitmap-enable and block-dirty-bitmap-disable

[Max Reitz]

On 2014-10-30 at 04:22, 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.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block.c               | 15 +++++++++++++++
>   blockdev.c            | 42 ++++++++++++++++++++++++++++++++++++++++++
>   include/block/block.h |  2 ++
>   qapi/block-core.json  | 28 ++++++++++++++++++++++++++++
>   qmp-commands.hx       | 10 ++++++++++
>   5 files changed, 97 insertions(+)

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

[Qemu-devel] [PATCH v6 07/10] 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            | 54 ++++++++++++++++++++++++++++++++++++++++++++++-
 block/mirror.c            |  4 ++++
 blockdev.c                |  9 +++++++-
 hmp.c                     |  4 +++-
 include/block/block_int.h |  4 ++++
 qapi/block-core.json      | 30 ++++++++++++++++++++++----
 qmp-commands.hx           |  7 +++---
 7 files changed, 102 insertions(+), 10 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index e334740..4870694 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 modes 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,
                   BlockCompletionFunc *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) {
+        if (!sync_bitmap) {
+            error_setg(errp, "must provide a valid bitmap name for "
+                             "\"dirty-bitmap\" sync mode");
+            return;
+        }
+
+        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,12 @@ 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_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP ?
+                       sync_bitmap : NULL;
+    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 cecd448..8dfd908 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -679,6 +679,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, replaces,
diff --git a/blockdev.c b/blockdev.c
index c7e98ad..fca909b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1463,6 +1463,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);
@@ -2186,6 +2188,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)
@@ -2282,7 +2286,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 63d7686..bc5a2d2 100644
--- a/hmp.c
+++ b/hmp.c
@@ -966,7 +966,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, &err);
+                     true, mode, false, 0, false, NULL,
+                     false, 0,
+                     false, 0, false, 0, &err);
     hmp_handle_error(mon, &err);
 }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8898c6c..0671c89 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -567,6 +567,8 @@ 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 name of 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.
@@ -577,6 +579,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,
                   BlockCompletionFunc *cb, void *opaque,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 97c31bf..e225220 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -473,7 +473,7 @@
 # Since: 1.3
 ##
 { 'enum': 'MirrorSyncMode',
-  'data': ['top', 'full', 'none'] }
+  'data': ['top', 'full', 'none', 'dirty-bitmap'] }
 
 ##
 # @BlockJobType:
@@ -634,6 +634,21 @@
             '*format': 'str', '*mode': 'NewImageMode' } }
 
 ##
+# @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.3
+##
+{ 'enum': 'BitmapUseMode',
+'data': [ 'consume', 'reset' ] }
+
+##
 # @DriveBackup
 #
 # @device: the name of the device which should be copied.
@@ -646,14 +661,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.3)
+#
+# @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).
@@ -671,7 +692,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 6d0c944..75035e1 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1048,7 +1048,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,
     },
 
@@ -1075,8 +1075,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": dirty bitmap name 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.3

Re: [Qemu-devel] [PATCH v6 07/10] qmp: Add support of "dirty-bitmap" sync mode for drive-backup

[Max Reitz]

On 2014-10-30 at 04:22, Fam Zheng wrote:
> 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            | 54 ++++++++++++++++++++++++++++++++++++++++++++++-
>   block/mirror.c            |  4 ++++
>   blockdev.c                |  9 +++++++-
>   hmp.c                     |  4 +++-
>   include/block/block_int.h |  4 ++++
>   qapi/block-core.json      | 30 ++++++++++++++++++++++----
>   qmp-commands.hx           |  7 +++---
>   7 files changed, 102 insertions(+), 10 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index e334740..4870694 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 modes 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) {

->sync_bitmap_gran should be a sector count here... [1]

> +                    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,
>                     BlockCompletionFunc *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) {
> +        if (!sync_bitmap) {
> +            error_setg(errp, "must provide a valid bitmap name for "
> +                             "\"dirty-bitmap\" sync mode");
> +            return;
> +        }

How about an error when sync_mode != MIRROR_SYNC_MODE_DIRTY_BITMAP and 
sync_bitmap != NULL? It's not really fatal, but I wouldn't call it a 
valid combination either.

> +
> +        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);

Better use abort() directly (or g_assert_not_reached(), which however 
suffers from the same problem as assert(0) which is NDEBUG or the 
equivalent for glib).

> +        }
> +        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,12 @@ 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_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP ?
> +                       sync_bitmap : NULL;
> +    if (sync_bitmap) {
> +        job->sync_bitmap_gran =
> +            bdrv_dirty_bitmap_granularity(bs, job->sync_bitmap);

[1] so this should return a sector count (relevant for patch 3); 
although I'd still personally prefer it to return a byte count which is 
converted to sectors here (but it doesn't matter from a technical 
perspective).

> +    }
>       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 cecd448..8dfd908 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -679,6 +679,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, replaces,
> diff --git a/blockdev.c b/blockdev.c
> index c7e98ad..fca909b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1463,6 +1463,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);
> @@ -2186,6 +2188,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)
> @@ -2282,7 +2286,10 @@ void qmp_drive_backup(const char *device, const char *target,
>           return;
>       }
>   

This conflicts with Stefan's blockjob/dataplane series (can be easily 
fixed, though).

> -    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,

Shouldn't we be checking the return value of bdrv_find_dirty_bitmap() here?

I know backup_start() does a check itself, but I'd prefer a check here, 
too, nonetheless.

> +                 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 63d7686..bc5a2d2 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -966,7 +966,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, &err);
> +                     true, mode, false, 0, false, NULL,
> +                     false, 0,
> +                     false, 0, false, 0, &err);
>       hmp_handle_error(mon, &err);
>   }
>   
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 8898c6c..0671c89 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -567,6 +567,8 @@ 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 name of dirty bitmap if sync_mode is
> + *               MIRROR_SYNC_MODE_DIRTY_BITMAP.

It's not the name, it's the bitmap itself. Also, documentation for 
bitmap_mode is missing.

>    * @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.
> @@ -577,6 +579,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,
>                     BlockCompletionFunc *cb, void *opaque,
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 97c31bf..e225220 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -473,7 +473,7 @@
>   # Since: 1.3
>   ##
>   { 'enum': 'MirrorSyncMode',
> -  'data': ['top', 'full', 'none'] }
> +  'data': ['top', 'full', 'none', 'dirty-bitmap'] }
>   
>   ##
>   # @BlockJobType:
> @@ -634,6 +634,21 @@
>               '*format': 'str', '*mode': 'NewImageMode' } }
>   
>   ##
> +# @BitmapUseMode
> +#
> +# An enumeration that tells QEMU what operation to take when using a bitmap
> +# in drive backup sync mode.

Did you mean "when using a bitmap in drive backup sync mode 
dirty-bitmap"? (or just "when using a bitmap for drive-backup")

> +#
> +# @consume: QEMU should just consume the bitmap and release it after using
> +#
> +# @reset: QEMU should reset the dirty bitmap
> +#
> +# Since: 2.3
> +##
> +{ 'enum': 'BitmapUseMode',
> +'data': [ 'consume', 'reset' ] }
> +
> +##
>   # @DriveBackup
>   #
>   # @device: the name of the device which should be copied.
> @@ -646,14 +661,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.3)
> +#
> +# @bitmap-use-mode: #optional which operation to take when consuming @bitmap,
> +#                   default is reset. (Since 2.1)

*2.3

Max

> +#
>   # @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).
> @@ -671,7 +692,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 6d0c944..75035e1 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1048,7 +1048,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,
>       },
>   
> @@ -1075,8 +1075,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": dirty bitmap name 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)

[Qemu-devel] [PATCH v6 08/10] qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}

[Fam Zheng]

This adds three qmp commands to transactions.

Users 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       | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |  5 ++-
 2 files changed, 96 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index fca909b..34fa314 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1490,6 +1490,83 @@ static void drive_backup_abort(BlkTransactionState *common)
     }
 }
 
+static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
+                                           Error **errp)
+{
+    BlockDirtyBitmapAdd *action;
+
+    action = common->action->block_dirty_bitmap_add;
+    qmp_block_dirty_bitmap_add(action->device, action->name,
+                               action->has_granularity, action->granularity,
+                               errp);
+}
+
+static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
+{
+    BlockDirtyBitmapAdd *action;
+    BdrvDirtyBitmap *bm;
+    BlockDriverState *bs;
+
+    action = common->action->block_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 block_dirty_bitmap_enable_prepare(BlkTransactionState *common,
+                                               Error **errp)
+{
+    BlockDirtyBitmap *action;
+
+    action = common->action->block_dirty_bitmap_enable;
+    qmp_block_dirty_bitmap_enable(action->device, action->name, errp);
+}
+
+static void block_dirty_bitmap_enable_abort(BlkTransactionState *common)
+{
+    BlockDirtyBitmap *action;
+    BdrvDirtyBitmap *bitmap;
+    BlockDriverState *bs;
+
+    action = common->action->block_dirty_bitmap_enable;
+    bs = bdrv_find(action->device);
+    if (bs) {
+        bitmap = bdrv_find_dirty_bitmap(bs, action->name);
+        if (bitmap) {
+            bdrv_disable_dirty_bitmap(bs, bitmap);
+        }
+    }
+}
+
+static void block_dirty_bitmap_disable_prepare(BlkTransactionState *common,
+                                               Error **errp)
+{
+    BlockDirtyBitmap *action;
+
+    action = common->action->block_dirty_bitmap_disable;
+    qmp_block_dirty_bitmap_disable(action->device, action->name, errp);
+}
+
+static void block_dirty_bitmap_disable_abort(BlkTransactionState *common)
+{
+    BlockDirtyBitmap *action;
+    BdrvDirtyBitmap *bitmap;
+    BlockDriverState *bs;
+
+    action = common->action->block_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");
@@ -1522,6 +1599,21 @@ static const BdrvActionOps actions[] = {
         .prepare  = internal_snapshot_prepare,
         .abort = internal_snapshot_abort,
     },
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] = {
+        .instance_size = sizeof(BlkTransactionState),
+        .prepare = block_dirty_bitmap_add_prepare,
+        .abort = block_dirty_bitmap_add_abort,
+    },
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
+        .instance_size = sizeof(BlkTransactionState),
+        .prepare = block_dirty_bitmap_enable_prepare,
+        .abort = block_dirty_bitmap_enable_abort,
+    },
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
+        .instance_size = sizeof(BlkTransactionState),
+        .prepare = block_dirty_bitmap_disable_prepare,
+        .abort = block_dirty_bitmap_disable_abort,
+    },
 };
 
 /*
diff --git a/qapi-schema.json b/qapi-schema.json
index 24379ab..ae07677 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1260,7 +1260,10 @@
        'blockdev-snapshot-sync': 'BlockdevSnapshot',
        'drive-backup': 'DriveBackup',
        'abort': 'Abort',
-       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
+       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
+       'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
+       'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
+       'block-dirty-bitmap-disable': 'BlockDirtyBitmap'
    } }
 
 ##
-- 
1.9.3

Re: [Qemu-devel] [PATCH v6 08/10] qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}

[Max Reitz]

On 2014-10-30 at 04:22, Fam Zheng wrote:
> This adds three qmp commands to transactions.
>
> Users 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       | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   qapi-schema.json |  5 ++-
>   2 files changed, 96 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index fca909b..34fa314 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1490,6 +1490,83 @@ static void drive_backup_abort(BlkTransactionState *common)
>       }
>   }
>   
> +static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
> +                                           Error **errp)
> +{
> +    BlockDirtyBitmapAdd *action;
> +
> +    action = common->action->block_dirty_bitmap_add;
> +    qmp_block_dirty_bitmap_add(action->device, action->name,
> +                               action->has_granularity, action->granularity,
> +                               errp);
> +}
> +
> +static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmapAdd *action;
> +    BdrvDirtyBitmap *bm;
> +    BlockDriverState *bs;
> +
> +    action = common->action->block_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 block_dirty_bitmap_enable_prepare(BlkTransactionState *common,
> +                                               Error **errp)
> +{
> +    BlockDirtyBitmap *action;
> +
> +    action = common->action->block_dirty_bitmap_enable;
> +    qmp_block_dirty_bitmap_enable(action->device, action->name, errp);
> +}
> +
> +static void block_dirty_bitmap_enable_abort(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmap *action;
> +    BdrvDirtyBitmap *bitmap;
> +    BlockDriverState *bs;
> +
> +    action = common->action->block_dirty_bitmap_enable;
> +    bs = bdrv_find(action->device);
> +    if (bs) {
> +        bitmap = bdrv_find_dirty_bitmap(bs, action->name);
> +        if (bitmap) {
> +            bdrv_disable_dirty_bitmap(bs, bitmap);

But what if the dirty bitmap was enabled before so that this enabling 
transaction was supposed to be a no-op?

> +        }
> +    }
> +}
> +
> +static void block_dirty_bitmap_disable_prepare(BlkTransactionState *common,
> +                                               Error **errp)
> +{
> +    BlockDirtyBitmap *action;
> +
> +    action = common->action->block_dirty_bitmap_disable;
> +    qmp_block_dirty_bitmap_disable(action->device, action->name, errp);
> +}
> +
> +static void block_dirty_bitmap_disable_abort(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmap *action;
> +    BdrvDirtyBitmap *bitmap;
> +    BlockDriverState *bs;
> +
> +    action = common->action->block_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);

Same here, the other way round.

> +        }
> +    }
> +}
> +
>   static void abort_prepare(BlkTransactionState *common, Error **errp)
>   {
>       error_setg(errp, "Transaction aborted using Abort action");
> @@ -1522,6 +1599,21 @@ static const BdrvActionOps actions[] = {
>           .prepare  = internal_snapshot_prepare,
>           .abort = internal_snapshot_abort,
>       },
> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] = {
> +        .instance_size = sizeof(BlkTransactionState),
> +        .prepare = block_dirty_bitmap_add_prepare,
> +        .abort = block_dirty_bitmap_add_abort,
> +    },
> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
> +        .instance_size = sizeof(BlkTransactionState),
> +        .prepare = block_dirty_bitmap_enable_prepare,
> +        .abort = block_dirty_bitmap_enable_abort,
> +    },
> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
> +        .instance_size = sizeof(BlkTransactionState),
> +        .prepare = block_dirty_bitmap_disable_prepare,
> +        .abort = block_dirty_bitmap_disable_abort,
> +    },
>   };
>   
>   /*
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 24379ab..ae07677 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1260,7 +1260,10 @@
>          'blockdev-snapshot-sync': 'BlockdevSnapshot',
>          'drive-backup': 'DriveBackup',
>          'abort': 'Abort',
> -       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
> +       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
> +       'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',

Ah, you're reusing that type here. Very well then.

Max

> +       'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
> +       'block-dirty-bitmap-disable': 'BlockDirtyBitmap'
>      } }
>   
>   ##

Re: [Qemu-devel] [PATCH v6 08/10] qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}

[John Snow]

On 11/04/2014 06:03 AM, Max Reitz wrote:
> On 2014-10-30 at 04:22, Fam Zheng wrote:
>> This adds three qmp commands to transactions.
>>
>> Users 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       | 92
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qapi-schema.json |  5 ++-
>>   2 files changed, 96 insertions(+), 1 deletion(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index fca909b..34fa314 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1490,6 +1490,83 @@ static void
>> drive_backup_abort(BlkTransactionState *common)
>>       }
>>   }
>> +static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
>> +                                           Error **errp)
>> +{
>> +    BlockDirtyBitmapAdd *action;
>> +
>> +    action = common->action->block_dirty_bitmap_add;
>> +    qmp_block_dirty_bitmap_add(action->device, action->name,
>> +                               action->has_granularity,
>> action->granularity,
>> +                               errp);
>> +}
>> +
>> +static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
>> +{
>> +    BlockDirtyBitmapAdd *action;
>> +    BdrvDirtyBitmap *bm;
>> +    BlockDriverState *bs;
>> +
>> +    action = common->action->block_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 block_dirty_bitmap_enable_prepare(BlkTransactionState
>> *common,
>> +                                               Error **errp)
>> +{
>> +    BlockDirtyBitmap *action;
>> +
>> +    action = common->action->block_dirty_bitmap_enable;
>> +    qmp_block_dirty_bitmap_enable(action->device, action->name, errp);
>> +}
>> +
>> +static void block_dirty_bitmap_enable_abort(BlkTransactionState *common)
>> +{
>> +    BlockDirtyBitmap *action;
>> +    BdrvDirtyBitmap *bitmap;
>> +    BlockDriverState *bs;
>> +
>> +    action = common->action->block_dirty_bitmap_enable;
>> +    bs = bdrv_find(action->device);
>> +    if (bs) {
>> +        bitmap = bdrv_find_dirty_bitmap(bs, action->name);
>> +        if (bitmap) {
>> +            bdrv_disable_dirty_bitmap(bs, bitmap);
>
> But what if the dirty bitmap was enabled before so that this enabling
> transaction was supposed to be a no-op?

Maybe it's not a problem: The only case in which the enable/disable 
operation will fail is if it cannot find the device or bitmap -- in 
which case, the abort operation isn't going to be able to affect 
anything either.

Unless you know something I don't.

Still, it does look goofy. Thoughts?

>> +        }
>> +    }
>> +}
>> +
>> +static void block_dirty_bitmap_disable_prepare(BlkTransactionState
>> *common,
>> +                                               Error **errp)
>> +{
>> +    BlockDirtyBitmap *action;
>> +
>> +    action = common->action->block_dirty_bitmap_disable;
>> +    qmp_block_dirty_bitmap_disable(action->device, action->name, errp);
>> +}
>> +
>> +static void block_dirty_bitmap_disable_abort(BlkTransactionState
>> *common)
>> +{
>> +    BlockDirtyBitmap *action;
>> +    BdrvDirtyBitmap *bitmap;
>> +    BlockDriverState *bs;
>> +
>> +    action = common->action->block_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);
>
> Same here, the other way round.
>
>> +        }
>> +    }
>> +}
>> +
>>   static void abort_prepare(BlkTransactionState *common, Error **errp)
>>   {
>>       error_setg(errp, "Transaction aborted using Abort action");
>> @@ -1522,6 +1599,21 @@ static const BdrvActionOps actions[] = {
>>           .prepare  = internal_snapshot_prepare,
>>           .abort = internal_snapshot_abort,
>>       },
>> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] = {
>> +        .instance_size = sizeof(BlkTransactionState),
>> +        .prepare = block_dirty_bitmap_add_prepare,
>> +        .abort = block_dirty_bitmap_add_abort,
>> +    },
>> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
>> +        .instance_size = sizeof(BlkTransactionState),
>> +        .prepare = block_dirty_bitmap_enable_prepare,
>> +        .abort = block_dirty_bitmap_enable_abort,
>> +    },
>> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
>> +        .instance_size = sizeof(BlkTransactionState),
>> +        .prepare = block_dirty_bitmap_disable_prepare,
>> +        .abort = block_dirty_bitmap_disable_abort,
>> +    },
>>   };
>>   /*
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 24379ab..ae07677 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1260,7 +1260,10 @@
>>          'blockdev-snapshot-sync': 'BlockdevSnapshot',
>>          'drive-backup': 'DriveBackup',
>>          'abort': 'Abort',
>> -       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
>> +       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
>> +       'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
>
> Ah, you're reusing that type here. Very well then.
>
> Max
>
>> +       'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
>> +       'block-dirty-bitmap-disable': 'BlockDirtyBitmap'
>>      } }
>>   ##
>
>

-- 
—js

Re: [Qemu-devel] [PATCH v6 08/10] qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}

[Max Reitz]

On 2014-11-21 at 23:24, John Snow wrote:
>
>
> On 11/04/2014 06:03 AM, Max Reitz wrote:
>> On 2014-10-30 at 04:22, Fam Zheng wrote:
>>> This adds three qmp commands to transactions.
>>>
>>> Users 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       | 92
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   qapi-schema.json |  5 ++-
>>>   2 files changed, 96 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index fca909b..34fa314 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -1490,6 +1490,83 @@ static void
>>> drive_backup_abort(BlkTransactionState *common)
>>>       }
>>>   }
>>> +static void block_dirty_bitmap_add_prepare(BlkTransactionState 
>>> *common,
>>> +                                           Error **errp)
>>> +{
>>> +    BlockDirtyBitmapAdd *action;
>>> +
>>> +    action = common->action->block_dirty_bitmap_add;
>>> +    qmp_block_dirty_bitmap_add(action->device, action->name,
>>> +                               action->has_granularity,
>>> action->granularity,
>>> +                               errp);
>>> +}
>>> +
>>> +static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
>>> +{
>>> +    BlockDirtyBitmapAdd *action;
>>> +    BdrvDirtyBitmap *bm;
>>> +    BlockDriverState *bs;
>>> +
>>> +    action = common->action->block_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 block_dirty_bitmap_enable_prepare(BlkTransactionState
>>> *common,
>>> +                                               Error **errp)
>>> +{
>>> +    BlockDirtyBitmap *action;
>>> +
>>> +    action = common->action->block_dirty_bitmap_enable;
>>> +    qmp_block_dirty_bitmap_enable(action->device, action->name, errp);
>>> +}
>>> +
>>> +static void block_dirty_bitmap_enable_abort(BlkTransactionState 
>>> *common)
>>> +{
>>> +    BlockDirtyBitmap *action;
>>> +    BdrvDirtyBitmap *bitmap;
>>> +    BlockDriverState *bs;
>>> +
>>> +    action = common->action->block_dirty_bitmap_enable;
>>> +    bs = bdrv_find(action->device);
>>> +    if (bs) {
>>> +        bitmap = bdrv_find_dirty_bitmap(bs, action->name);
>>> +        if (bitmap) {
>>> +            bdrv_disable_dirty_bitmap(bs, bitmap);
>>
>> But what if the dirty bitmap was enabled before so that this enabling
>> transaction was supposed to be a no-op?
>
> Maybe it's not a problem: The only case in which the enable/disable 
> operation will fail is if it cannot find the device or bitmap -- in 
> which case, the abort operation isn't going to be able to affect 
> anything either.

Well, it's part of a transaction. As far as I understand it, one groups 
several operations in one transaction and if any fails, all are aborted. 
Therefore, the "enable the dirty bitmap" operation can trivially succeed 
(because it was enabled before), but some different operation may fail, 
which then results in invocation of this abort function.

Max

> Unless you know something I don't.
>
> Still, it does look goofy. Thoughts?

Re: [Qemu-devel] [PATCH v6 08/10] qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}

[Paolo Bonzini]

On 24/11/2014 09:35, Max Reitz wrote:
>>>
>>> But what if the dirty bitmap was enabled before so that this enabling
>>> transaction was supposed to be a no-op?
>>
>> Maybe it's not a problem: The only case in which the enable/disable
>> operation will fail is if it cannot find the device or bitmap -- in
>> which case, the abort operation isn't going to be able to affect
>> anything either.
> 
> Well, it's part of a transaction. As far as I understand it, one groups
> several operations in one transaction and if any fails, all are aborted.
> Therefore, the "enable the dirty bitmap" operation can trivially succeed
> (because it was enabled before), but some different operation may fail,
> which then results in invocation of this abort function.

Would it work to do the actual enabling/disabling in the commit
function, since it cannot fail?

Paolo

Re: [Qemu-devel] [PATCH v6 08/10] qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}

[Max Reitz]

On 2014-11-24 at 10:41, Paolo Bonzini wrote:
>
> On 24/11/2014 09:35, Max Reitz wrote:
>>>> But what if the dirty bitmap was enabled before so that this enabling
>>>> transaction was supposed to be a no-op?
>>> Maybe it's not a problem: The only case in which the enable/disable
>>> operation will fail is if it cannot find the device or bitmap -- in
>>> which case, the abort operation isn't going to be able to affect
>>> anything either.
>> Well, it's part of a transaction. As far as I understand it, one groups
>> several operations in one transaction and if any fails, all are aborted.
>> Therefore, the "enable the dirty bitmap" operation can trivially succeed
>> (because it was enabled before), but some different operation may fail,
>> which then results in invocation of this abort function.
> Would it work to do the actual enabling/disabling in the commit
> function, since it cannot fail?

Too easy. :-)

This patch could then no longer reuse qmp_block_dirty_bitmap_enable(), 
but reimplementing it here should be fine (it's short enough).

Max

Re: [Qemu-devel] [PATCH v6 08/10] qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}

[Paolo Bonzini]

On 24/11/2014 10:46, Max Reitz wrote:
> On 2014-11-24 at 10:41, Paolo Bonzini wrote:
>>
>> On 24/11/2014 09:35, Max Reitz wrote:
>>>>> But what if the dirty bitmap was enabled before so that this enabling
>>>>> transaction was supposed to be a no-op?
>>>> Maybe it's not a problem: The only case in which the enable/disable
>>>> operation will fail is if it cannot find the device or bitmap -- in
>>>> which case, the abort operation isn't going to be able to affect
>>>> anything either.
>>> Well, it's part of a transaction. As far as I understand it, one groups
>>> several operations in one transaction and if any fails, all are aborted.
>>> Therefore, the "enable the dirty bitmap" operation can trivially succeed
>>> (because it was enabled before), but some different operation may fail,
>>> which then results in invocation of this abort function.
>> Would it work to do the actual enabling/disabling in the commit
>> function, since it cannot fail?
> 
> Too easy. :-)
> 
> This patch could then no longer reuse qmp_block_dirty_bitmap_enable(),
> but reimplementing it here should be fine (it's short enough).

Ah, right:

prepare:

+    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;
+    }

commit:

+    bdrv_enable_dirty_bitmap(bs, bitmap);

But qmp_block_dirty_bitmap_enable can reuse qmp_transaction instead! :)

Paolo

[Qemu-devel] [PATCH v6 09/10] qmp: Add dirty bitmap 'enabled' field in query-block

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c              | 1 +
 qapi/block-core.json | 5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index e574faa..f4d145c 100644
--- a/block.c
+++ b/block.c
@@ -5375,6 +5375,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
             ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
         info->has_name = !!bm->name;
         info->name = info->has_name ? g_strdup(bm->name) : NULL;
+        info->enabled = bm->enabled;
         entry->value = info;
         *plist = entry;
         plist = &entry->next;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e225220..edd74c8 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -313,10 +313,13 @@
 #
 # @granularity: granularity of the dirty bitmap in bytes (since 1.4)
 #
+# @enabled: whether the dirty bitmap is enabled (Since 2.3)
+#
 # Since: 1.3
 ##
 { 'type': 'BlockDirtyInfo',
-  'data': {'*name': 'str', 'count': 'int', 'granularity': 'int'} }
+  'data': {'*name': 'str', 'count': 'int', 'granularity': 'int',
+           'enabled': 'bool'} }
 
 ##
 # @BlockInfo:
-- 
1.9.3

Re: [Qemu-devel] [PATCH v6 09/10] qmp: Add dirty bitmap 'enabled' field in query-block

[Max Reitz]

On 2014-10-30 at 04:22, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block.c              | 1 +
>   qapi/block-core.json | 5 ++++-
>   2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index e574faa..f4d145c 100644
> --- a/block.c
> +++ b/block.c
> @@ -5375,6 +5375,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>               ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));

Although just context, this could use bdrv_dirty_bitmap_granularity() or 
bm->granularity.

>           info->has_name = !!bm->name;
>           info->name = info->has_name ? g_strdup(bm->name) : NULL;
> +        info->enabled = bm->enabled;
>           entry->value = info;
>           *plist = entry;
>           plist = &entry->next;
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index e225220..edd74c8 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -313,10 +313,13 @@
>   #
>   # @granularity: granularity of the dirty bitmap in bytes (since 1.4)
>   #
> +# @enabled: whether the dirty bitmap is enabled (Since 2.3)
> +#
>   # Since: 1.3
>   ##
>   { 'type': 'BlockDirtyInfo',
> -  'data': {'*name': 'str', 'count': 'int', 'granularity': 'int'} }
> +  'data': {'*name': 'str', 'count': 'int', 'granularity': 'int',
> +           'enabled': 'bool'} }
>   
>   ##
>   # @BlockInfo:

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

[Qemu-devel] [PATCH v6 10/10] qemu-iotests: Add tests for drive-backup sync=dirty-bitmap

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/056        | 33 ++++++++++++++++++++++++++++++---
 tests/qemu-iotests/056.out    |  4 ++--
 tests/qemu-iotests/iotests.py |  8 ++++++++
 3 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
index 54e4bd0..fc9114e 100755
--- a/tests/qemu-iotests/056
+++ b/tests/qemu-iotests/056
@@ -23,17 +23,17 @@
 import time
 import os
 import iotests
-from iotests import qemu_img, qemu_io, create_image
+from iotests import qemu_img, qemu_img_map_assert, qemu_io, create_image
 
 backing_img = os.path.join(iotests.test_dir, 'backing.img')
 test_img = os.path.join(iotests.test_dir, 'test.img')
 target_img = os.path.join(iotests.test_dir, 'target.img')
 
-class TestSyncModesNoneAndTop(iotests.QMPTestCase):
+class TestSyncModes(iotests.QMPTestCase):
     image_len = 64 * 1024 * 1024 # MB
 
     def setUp(self):
-        create_image(backing_img, TestSyncModesNoneAndTop.image_len)
+        create_image(backing_img, TestSyncModes.image_len)
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
         qemu_io('-c', 'write -P0x41 0 512', test_img)
         qemu_io('-c', 'write -P0xd5 1M 32k', test_img)
@@ -64,6 +64,33 @@ class TestSyncModesNoneAndTop(iotests.QMPTestCase):
         self.assertTrue(iotests.compare_images(test_img, target_img),
                         'target image does not match source after backup')
 
+    def test_sync_dirty_bitmap_missing(self):
+        self.assert_no_active_block_jobs()
+        result = self.vm.qmp('drive-backup', device='drive0', sync='dirty-bitmap',
+                             format=iotests.imgfmt, target=target_img)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_sync_dirty_bitmap_not_found(self):
+        self.assert_no_active_block_jobs()
+        result = self.vm.qmp('drive-backup', device='drive0', sync='dirty-bitmap',
+                             bitmap='unknown',
+                             format=iotests.imgfmt, target=target_img)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_sync_dirty_bitmap(self):
+        self.assert_no_active_block_jobs()
+        result = self.vm.qmp('block-dirty-bitmap-add', device='drive0', name='bitmap0')
+        self.assert_qmp(result, 'return', {})
+        self.vm.hmp_qemu_io('drive0', 'write -P0x5a 0 512')
+        self.vm.hmp_qemu_io('drive0', 'write -P0x5a 48M 512')
+        result = self.vm.qmp('drive-backup', device='drive0', sync='dirty-bitmap',
+                             bitmap='bitmap0',
+                             format=iotests.imgfmt, target=target_img)
+        self.assert_qmp(result, 'return', {})
+        self.wait_until_completed(check_offset=False)
+        self.assert_no_active_block_jobs()
+        qemu_img_map_assert(target_img, [0, 0x3000000])
+
     def test_cancel_sync_none(self):
         self.assert_no_active_block_jobs()
 
diff --git a/tests/qemu-iotests/056.out b/tests/qemu-iotests/056.out
index fbc63e6..914e373 100644
--- a/tests/qemu-iotests/056.out
+++ b/tests/qemu-iotests/056.out
@@ -1,5 +1,5 @@
-..
+.....
 ----------------------------------------------------------------------
-Ran 2 tests
+Ran 5 tests
 
 OK
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 39a4cfc..ae81364 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -55,6 +55,14 @@ def qemu_img_pipe(*args):
     '''Run qemu-img and return its output'''
     return subprocess.Popen(qemu_img_args + list(args), stdout=subprocess.PIPE).communicate()[0]
 
+def qemu_img_map_assert(img, offsets):
+    '''Run qemu-img map on img and check the mapped ranges'''
+    offs = []
+    for line in qemu_img_pipe('map', img).splitlines()[1:]:
+        offset, length, mapped, fname = line.split()
+        offs.append(int(offset, 16))
+    assert set(offs) == set(offsets), "mapped offsets in image '%s' not equal to '%s'" % (str(offs), str(offsets))
+
 def qemu_io(*args):
     '''Run qemu-io and return the stdout data'''
     args = qemu_io_args + list(args)
-- 
1.9.3

Re: [Qemu-devel] [PATCH v6 10/10] qemu-iotests: Add tests for drive-backup sync=dirty-bitmap

[Max Reitz]

On 2014-10-30 at 04:22, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   tests/qemu-iotests/056        | 33 ++++++++++++++++++++++++++++++---
>   tests/qemu-iotests/056.out    |  4 ++--
>   tests/qemu-iotests/iotests.py |  8 ++++++++
>   3 files changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
> index 54e4bd0..fc9114e 100755
> --- a/tests/qemu-iotests/056
> +++ b/tests/qemu-iotests/056
> @@ -23,17 +23,17 @@
>   import time
>   import os
>   import iotests
> -from iotests import qemu_img, qemu_io, create_image
> +from iotests import qemu_img, qemu_img_map_assert, qemu_io, create_image
>   
>   backing_img = os.path.join(iotests.test_dir, 'backing.img')
>   test_img = os.path.join(iotests.test_dir, 'test.img')
>   target_img = os.path.join(iotests.test_dir, 'target.img')
>   
> -class TestSyncModesNoneAndTop(iotests.QMPTestCase):
> +class TestSyncModes(iotests.QMPTestCase):

Well, you're still not testing Full (though 
"TestSyncModesNoneTopAndDirtyBitmap" sounds really awful, I'll give you 
that)...

>       image_len = 64 * 1024 * 1024 # MB
>   
>       def setUp(self):
> -        create_image(backing_img, TestSyncModesNoneAndTop.image_len)
> +        create_image(backing_img, TestSyncModes.image_len)
>           qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
>           qemu_io('-c', 'write -P0x41 0 512', test_img)
>           qemu_io('-c', 'write -P0xd5 1M 32k', test_img)
> @@ -64,6 +64,33 @@ class TestSyncModesNoneAndTop(iotests.QMPTestCase):
>           self.assertTrue(iotests.compare_images(test_img, target_img),
>                           'target image does not match source after backup')
>   
> +    def test_sync_dirty_bitmap_missing(self):
> +        self.assert_no_active_block_jobs()
> +        result = self.vm.qmp('drive-backup', device='drive0', sync='dirty-bitmap',
> +                             format=iotests.imgfmt, target=target_img)
> +        self.assert_qmp(result, 'error/class', 'GenericError')
> +
> +    def test_sync_dirty_bitmap_not_found(self):
> +        self.assert_no_active_block_jobs()
> +        result = self.vm.qmp('drive-backup', device='drive0', sync='dirty-bitmap',
> +                             bitmap='unknown',
> +                             format=iotests.imgfmt, target=target_img)
> +        self.assert_qmp(result, 'error/class', 'GenericError')
> +
> +    def test_sync_dirty_bitmap(self):
> +        self.assert_no_active_block_jobs()
> +        result = self.vm.qmp('block-dirty-bitmap-add', device='drive0', name='bitmap0')
> +        self.assert_qmp(result, 'return', {})
> +        self.vm.hmp_qemu_io('drive0', 'write -P0x5a 0 512')
> +        self.vm.hmp_qemu_io('drive0', 'write -P0x5a 48M 512')
> +        result = self.vm.qmp('drive-backup', device='drive0', sync='dirty-bitmap',
> +                             bitmap='bitmap0',
> +                             format=iotests.imgfmt, target=target_img)
> +        self.assert_qmp(result, 'return', {})
> +        self.wait_until_completed(check_offset=False)
> +        self.assert_no_active_block_jobs()
> +        qemu_img_map_assert(target_img, [0, 0x3000000])
> +
>       def test_cancel_sync_none(self):
>           self.assert_no_active_block_jobs()
>   
> diff --git a/tests/qemu-iotests/056.out b/tests/qemu-iotests/056.out
> index fbc63e6..914e373 100644
> --- a/tests/qemu-iotests/056.out
> +++ b/tests/qemu-iotests/056.out
> @@ -1,5 +1,5 @@
> -..
> +.....
>   ----------------------------------------------------------------------
> -Ran 2 tests
> +Ran 5 tests
>   
>   OK
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 39a4cfc..ae81364 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -55,6 +55,14 @@ def qemu_img_pipe(*args):
>       '''Run qemu-img and return its output'''
>       return subprocess.Popen(qemu_img_args + list(args), stdout=subprocess.PIPE).communicate()[0]
>   
> +def qemu_img_map_assert(img, offsets):
> +    '''Run qemu-img map on img and check the mapped ranges'''
> +    offs = []
> +    for line in qemu_img_pipe('map', img).splitlines()[1:]:
> +        offset, length, mapped, fname = line.split()
> +        offs.append(int(offset, 16))
> +    assert set(offs) == set(offsets), "mapped offsets in image '%s' not equal to '%s'" % (str(offs), str(offsets))
> +
>   def qemu_io(*args):
>       '''Run qemu-io and return the stdout data'''
>       args = qemu_io_args + list(args)

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

Re: [Qemu-devel] [PATCH v6 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[Vladimir Sementsov-Ogievskiy]

> +    if (!name || name[0] == '\0') {
Isn't is better to move "name[0] == '\0'" check to 
bdrv_create_dirty_bitmap, near existed name checking?

> +        if (granularity < 512 || is_power_of_2(granularity)) {
> +            error_setg(errp, "Granularity must be power of 2 "
> +                             "and greater than 512");
> +            return;
> +        }
> +    } else {
> +        granularity = 65536;
> +    }
Why not using something like DEFAULT_CLUSTER_SIZE, as in block/qcow2.h ?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v6 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[John Snow]

On 11/07/2014 07:24 AM, Vladimir Sementsov-Ogievskiy wrote:
>> +    if (!name || name[0] == '\0') {
> Isn't is better to move "name[0] == '\0'" check to
> bdrv_create_dirty_bitmap, near existed name checking?

Hm, OK, but then we still need to check for the presence of a name in 
these functions, so we're still going to be checking the validity of the 
name in both places regardless.

Probably not worth the effort. If somebody adds an empty string bitmap 
using bdrv_dirty_bitmap_create, it will at least be the only one named 
as such, and I don't think it will create any problems.

>> +        if (granularity < 512 || is_power_of_2(granularity)) {
>> +            error_setg(errp, "Granularity must be power of 2 "
>> +                             "and greater than 512");
>> +            return;
>> +        }
>> +    } else {
>> +        granularity = 65536;
>> +    }
> Why not using something like DEFAULT_CLUSTER_SIZE, as in block/qcow2.h ?
>

I don't want to tie this to explicitly qcow2, but on a suggestion from 
Fam, I think I will look at how block/mirror.c does this and tie it to 
the current cluster size -- which, if it's qcow2 -- will accomplish what 
you want, here.

-- 
—js