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

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

[John Snow]

Note: This patch is now based on top of stefanha/block-next and is
intended for QEMU 2.3.

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

==

This is the next iteration of Fam's incremenetal backup feature.
I have since taken this over from him and have (slowly) worked through
the feedback to the last version of his patch and have made many
tiny little edits.

For convenience: https://github.com/jnsnow/qemu/commits/dbm-backup

John.

v9:
 - Edited commit message, for English embetterment (02/10)
 - Rebased on top of stefanha/block-next (06,08/10)
 - Adjusted error message and line length (07/10)

v8:
 - Changed int64_t return for bdrv_dbm_calc_def_granularity to uint64_t (2/10)
 - Updated commit message (2/10)
 - Removed redundant check for null in device parameter (2/10)
 - Removed comment cruft. (2/10)
 - Removed redundant local_err propagation (several)
 - Updated commit message (3/10)
 - Fix HBitmap copy loop index (4/10)
 - Remove redundant ternary (5/10)
 - Shift up the block_dirty_bitmap_lookup function (6/10)
 - Error messages cleanup (7/10)
 - Add an assertion to explain the re-use of .prepare() for two transactions.
   (8/10)
 - Removed BDS argument from bitmap enable/disable helper; it was unused. (8/10)

v7: (highlights)
 - First version being authored by jsnow
 - Addressed most list feedback from V6, many small changes.
   All feedback was either addressed on-list (as a wontfix) or patched.
 - Replaced all error_set with error_setg
 - Replaced all bdrv_find with bdrv_lookup_bs()
 - Adjusted the max granularity to share a common function with
   backup/mirror that attempts to "guess" a reasonable default.
   It clamps between [4K,64K] currently.
 - The BdrvDirtyBitmap object now counts granularity exclusively in
   bytes to match its interface.
   It leaves the sector granularity concerns to HBitmap.
 - Reworked the backup loop to utilize the hbitmap iterator.
   There are some extra concerns to handle arrhythmic cases where the
   granularity of the bitmap does not match the backup cluster size.
   This iteration works best when it does match, but it's not a
   deal-breaker if it doesn't -- it just gets less efficient.
 - Reworked the transactional functions so that abort() wouldn't "undo"
   a redundant command. They now have been split into a prepare and a
   commit function (with state) and do not provide an abort command.
 - Added a block_dirty_bitmap_lookup(device, name, errp) function to
   shorten a few of the commands added in this series, particularly
   qmp_enable, qmp_disable, and the transaction preparations.

v6: Re-send of v5.

v5: Rebase to master.

v4: Last version tailored by Fam Zheng.

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                       | 114 ++++++++++++++++++++--
 block/backup.c                | 130 +++++++++++++++++++++----
 block/mirror.c                |  16 ++--
 blockdev.c                    | 217 +++++++++++++++++++++++++++++++++++++++++-
 hmp.c                         |   4 +-
 include/block/block.h         |  17 +++-
 include/block/block_int.h     |   6 ++
 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, 711 insertions(+), 55 deletions(-)

-- 
1.9.3

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

[John Snow]

From: Fam Zheng <famz@redhat.com>

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>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block-migration.c     |  2 +-
 block.c               | 32 +++++++++++++++++++++++++++++++-
 block/mirror.c        |  2 +-
 include/block/block.h |  7 ++++++-
 qapi/block-core.json  |  4 +++-
 5 files changed, 42 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 591fbe4..e5c6ccf 100644
--- a/block.c
+++ b/block.c
@@ -53,6 +53,7 @@
 
 struct BdrvDirtyBitmap {
     HBitmap *bitmap;
+    char *name;
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -5326,7 +5327,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;
@@ -5334,6 +5356,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);
@@ -5344,6 +5370,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
     }
     bitmap = g_new0(BdrvDirtyBitmap, 1);
     bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
+    bitmap->name = g_strdup(name);
     QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
     return bitmap;
 }
@@ -5355,6 +5382,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;
         }
@@ -5373,6 +5401,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 = g_strdup(bm->name);
         entry->value = info;
         *plist = entry;
         plist = &entry->next;
diff --git a/block/mirror.c b/block/mirror.c
index 2c6dd2a..858e4ff 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -699,7 +699,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 610be9f..52fb6b2 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -430,8 +430,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 6e8db15..d176324 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -322,6 +322,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)
@@ -329,7 +331,7 @@
 # Since: 1.3
 ##
 { 'type': 'BlockDirtyInfo',
-  'data': {'count': 'int', 'granularity': 'int'} }
+  'data': {'*name': 'str', 'count': 'int', 'granularity': 'int'} }
 
 ##
 # @BlockInfo:
-- 
1.9.3

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

[John Snow]

From: Fam Zheng <famz@redhat.com>

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.

The granularity is an optional field. If it is not specified, we will
choose a default granularity based on the cluster size if available,
clamped to between 4K and 64K to mirror how the 'mirror' code was
already choosing granularity. If we do not have cluster size info
available, we choose 64K. This code has been factored out into a helper
shared with block/mirror.

The types added to block-core.json will be re-used in future patches
in this series, see:
'qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}'

Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 19 ++++++++++++++++++
 block/mirror.c        | 10 +---------
 blockdev.c            | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  1 +
 qapi/block-core.json  | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx       | 49 +++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 179 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index e5c6ccf..3f27519 100644
--- a/block.c
+++ b/block.c
@@ -5420,6 +5420,25 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
     }
 }
 
+#define BDB_MIN_DEF_GRANULARITY 4096
+#define BDB_MAX_DEF_GRANULARITY 65536
+#define BDB_DEFAULT_GRANULARITY BDB_MAX_DEF_GRANULARITY
+
+uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
+{
+    BlockDriverInfo bdi;
+    uint64_t granularity;
+
+    if (bdrv_get_info(bs, &bdi) >= 0 && bdi.cluster_size != 0) {
+        granularity = MAX(BDB_MIN_DEF_GRANULARITY, bdi.cluster_size);
+        granularity = MIN(BDB_MAX_DEF_GRANULARITY, granularity);
+    } else {
+        granularity = BDB_DEFAULT_GRANULARITY;
+    }
+
+    return granularity;
+}
+
 void bdrv_dirty_iter_init(BlockDriverState *bs,
                           BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
 {
diff --git a/block/mirror.c b/block/mirror.c
index 858e4ff..3633632 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -664,15 +664,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
     MirrorBlockJob *s;
 
     if (granularity == 0) {
-        /* Choose the default granularity based on the target file's cluster
-         * size, clamped between 4k and 64k.  */
-        BlockDriverInfo bdi;
-        if (bdrv_get_info(target, &bdi) >= 0 && bdi.cluster_size != 0) {
-            granularity = MAX(4096, bdi.cluster_size);
-            granularity = MIN(65536, granularity);
-        } else {
-            granularity = 65536;
-        }
+        granularity = bdrv_dbm_calc_def_granularity(target);
     }
 
     assert ((granularity & (granularity - 1)) == 0);
diff --git a/blockdev.c b/blockdev.c
index 5651a8e..4d30b09 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1894,6 +1894,60 @@ 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_lookup_bs(device, NULL, errp);
+    if (!bs) {
+        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 at least 512");
+            return;
+        }
+    } else {
+        /* Default to cluster size, if available: */
+        granularity = bdrv_dbm_calc_def_granularity(bs);
+    }
+
+    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_lookup_bs(device, NULL, errp);
+    if (!bs) {
+        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/include/block/block.h b/include/block/block.h
index 52fb6b2..066ded6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -439,6 +439,7 @@ 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);
+uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs);
 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);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d176324..da86e91 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -894,6 +894,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 explanation
+#
+# 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 d4b0010..03465a2 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

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

[John Snow]

From: Fam Zheng <famz@redhat.com>

This returns the granularity (in bytes) of dirty bitmap,
which matches the QMP interface and the existing query
interface.

Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 9 +++++++--
 include/block/block.h | 2 ++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 3f27519..a0d1150 100644
--- a/block.c
+++ b/block.c
@@ -5399,8 +5399,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
         BlockDirtyInfo *info = g_new0(BlockDirtyInfo, 1);
         BlockDirtyInfoList *entry = g_new0(BlockDirtyInfoList, 1);
         info->count = bdrv_get_dirty_count(bs, bm);
-        info->granularity =
-            ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
+        info->granularity = bdrv_dirty_bitmap_granularity(bs, bm);
         info->has_name = !!bm->name;
         info->name = g_strdup(bm->name);
         entry->value = info;
@@ -5439,6 +5438,12 @@ uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
     return granularity;
 }
 
+int64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
+                                  BdrvDirtyBitmap *bitmap)
+{
+    return BDRV_SECTOR_SIZE << 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 066ded6..f180f93 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -440,6 +440,8 @@ 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);
 uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs);
+int64_t 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

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

[John Snow]

From: Fam Zheng <famz@redhat.com>

This makes a deep copy of an HBitmap.

Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@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..8aa7406 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;
+    uint64_t size;
+    HBitmap *hb = g_memdup(bitmap, sizeof(HBitmap));
+
+    size = bitmap->size;
+    for (i = HBITMAP_LEVELS - 1; i >= 0; i--) {
+        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

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

[John Snow]

From: Fam Zheng <famz@redhat.com>

Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 35 +++++++++++++++++++++++++++++++----
 include/block/block.h |  4 ++++
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index a0d1150..2d08b9f 100644
--- a/block.c
+++ b/block.c
@@ -53,6 +53,8 @@
 
 struct BdrvDirtyBitmap {
     HBitmap *bitmap;
+    int64_t size;
+    int64_t granularity;
     char *name;
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
@@ -5346,6 +5348,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 = g_strdup(name);
+    QLIST_INSERT_HEAD(&bs->dirty_bitmaps, new_bitmap, list);
+    return new_bitmap;
+}
+
+void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+    hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
+}
+
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
                                           int granularity,
                                           const char *name,
@@ -5353,6 +5375,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
 {
     int64_t bitmap_size;
     BdrvDirtyBitmap *bitmap;
+    int sector_granularity;
 
     assert((granularity & (granularity - 1)) == 0);
 
@@ -5360,8 +5383,8 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
         error_setg(errp, "Bitmap already exists: %s", name);
         return NULL;
     }
-    granularity >>= BDRV_SECTOR_BITS;
-    assert(granularity);
+    sector_granularity = granularity >> BDRV_SECTOR_BITS;
+    assert(sector_granularity);
     bitmap_size = bdrv_nb_sectors(bs);
     if (bitmap_size < 0) {
         error_setg_errno(errp, -bitmap_size, "could not get length of device");
@@ -5369,7 +5392,9 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
         return NULL;
     }
     bitmap = g_new0(BdrvDirtyBitmap, 1);
-    bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
+    bitmap->size = bitmap_size;
+    bitmap->granularity = granularity;
+    bitmap->bitmap = hbitmap_alloc(bitmap->size, ffs(sector_granularity) - 1);
     bitmap->name = g_strdup(name);
     QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
     return bitmap;
@@ -5441,7 +5466,9 @@ uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
 int64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
                                   BdrvDirtyBitmap *bitmap)
 {
-    return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
+    g_assert(BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap) ==
+             bitmap->granularity);
+    return bitmap->granularity;
 }
 
 void bdrv_dirty_iter_init(BlockDriverState *bs,
diff --git a/include/block/block.h b/include/block/block.h
index f180f93..5522eba 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -437,6 +437,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);
 uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs);
-- 
1.9.3

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

[John Snow]

From: Fam Zheng <famz@redhat.com>

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>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 15 +++++++++++++
 blockdev.c            | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |  2 ++
 qapi/block-core.json  | 28 +++++++++++++++++++++++
 qmp-commands.hx       | 10 +++++++++
 5 files changed, 116 insertions(+)

diff --git a/block.c b/block.c
index 2d08b9f..85215b3 100644
--- a/block.c
+++ b/block.c
@@ -56,6 +56,7 @@ struct BdrvDirtyBitmap {
     int64_t size;
     int64_t granularity;
     char *name;
+    bool enabled;
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -5396,6 +5397,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
     bitmap->granularity = granularity;
     bitmap->bitmap = hbitmap_alloc(bitmap->size, ffs(sector_granularity) - 1);
     bitmap->name = g_strdup(name);
+    bitmap->enabled = true;
     QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
     return bitmap;
 }
@@ -5414,6 +5416,16 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
     }
 }
 
+void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+{
+    bitmap->enabled = false;
+}
+
+void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+{
+    bitmap->enabled = true;
+}
+
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
 {
     BdrvDirtyBitmap *bm;
@@ -5482,6 +5494,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 4d30b09..c6b0bf1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1173,6 +1173,41 @@ out_aio_context:
     return NULL;
 }
 
+/**
+ * Return a dirty bitmap (if present), after validating
+ * the device and bitmap names. Returns NULL on error,
+ * including when the device and/or bitmap is not found.
+ */
+static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *device,
+                                                  const char *name,
+                                                  Error **errp)
+{
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+
+    if (!device) {
+        error_setg(errp, "Device cannot be NULL");
+        return NULL;
+    }
+    if (!name) {
+        error_setg(errp, "Bitmap name cannot be NULL");
+        return NULL;
+    }
+
+    bs = bdrv_lookup_bs(device, NULL, errp);
+    if (!bs) {
+        return NULL;
+    }
+
+    bitmap = bdrv_find_dirty_bitmap(bs, name);
+    if (!bitmap) {
+        error_setg(errp, "Dirty bitmap not found: %s", name);
+        return NULL;
+    }
+
+    return bitmap;
+}
+
 /* New and old BlockDriverState structs for atomic group operations */
 
 typedef struct BlkTransactionState BlkTransactionState;
@@ -1948,6 +1983,32 @@ 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)
+{
+    BdrvDirtyBitmap *bitmap;
+
+    bitmap = block_dirty_bitmap_lookup(device, name, errp);
+    if (!bitmap) {
+        return;
+    }
+
+    bdrv_enable_dirty_bitmap(bitmap);
+}
+
+void qmp_block_dirty_bitmap_disable(const char *device, const char *name,
+                                    Error **errp)
+{
+    BdrvDirtyBitmap *bitmap;
+
+    bitmap = block_dirty_bitmap_lookup(device, name, errp);
+    if (!bitmap) {
+        return;
+    }
+
+    bdrv_disable_dirty_bitmap(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 5522eba..b583457 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -442,6 +442,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(BdrvDirtyBitmap *bitmap);
+void bdrv_enable_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs);
 int64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index da86e91..64b5755 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -949,6 +949,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 03465a2..9a69633 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

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

[John Snow]

From: Fam Zheng <famz@redhat.com>

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>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c                   |   5 ++
 block/backup.c            | 130 ++++++++++++++++++++++++++++++++++++++--------
 block/mirror.c            |   4 ++
 blockdev.c                |  17 +++++-
 hmp.c                     |   4 +-
 include/block/block.h     |   1 +
 include/block/block_int.h |   6 +++
 qapi/block-core.json      |  30 +++++++++--
 qmp-commands.hx           |   7 +--
 9 files changed, 174 insertions(+), 30 deletions(-)

diff --git a/block.c b/block.c
index 85215b3..42244f6 100644
--- a/block.c
+++ b/block.c
@@ -5489,6 +5489,11 @@ void bdrv_dirty_iter_init(BlockDriverState *bs,
     hbitmap_iter_init(hbi, bitmap->bitmap, 0);
 }
 
+void bdrv_dirty_iter_set(HBitmapIter *hbi, int64_t offset)
+{
+    hbitmap_iter_init(hbi, hbi->hb, offset);
+}
+
 void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
                     int nr_sectors)
 {
diff --git a/block/backup.c b/block/backup.c
index 792e655..2aab68f 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 */
+    int64_t sync_bitmap_gran;
     MirrorSyncMode sync_mode;
     RateLimit limit;
     BlockdevOnError on_source_error;
@@ -242,6 +246,31 @@ static void backup_complete(BlockJob *job, void *opaque)
     g_free(data);
 }
 
+static bool yield_and_check(BackupBlockJob *job)
+{
+    if (block_job_is_cancelled(&job->common)) {
+        return true;
+    }
+
+    /* we need to yield so that qemu_aio_flush() returns.
+     * (without, VM does not reboot)
+     */
+    if (job->common.speed) {
+        uint64_t delay_ns = ratelimit_calculate_delay(&job->limit,
+                                                      job->sectors_read);
+        job->sectors_read = 0;
+        block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, delay_ns);
+    } else {
+        block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, 0);
+    }
+
+    if (block_job_is_cancelled(&job->common)) {
+        return true;
+    }
+
+    return false;
+}
+
 static void coroutine_fn backup_run(void *opaque)
 {
     BackupBlockJob *job = opaque;
@@ -254,13 +283,13 @@ static void coroutine_fn backup_run(void *opaque)
     };
     int64_t start, end;
     int ret = 0;
+    bool error_is_read;
 
     QLIST_INIT(&job->inflight_reqs);
     qemu_co_rwlock_init(&job->flush_rwlock);
 
     start = 0;
-    end = DIV_ROUND_UP(job->common.len / BDRV_SECTOR_SIZE,
-                       BACKUP_SECTORS_PER_CLUSTER);
+    end = DIV_ROUND_UP(job->common.len, BACKUP_CLUSTER_SIZE);
 
     job->bitmap = hbitmap_alloc(end, 0);
 
@@ -278,28 +307,44 @@ static void coroutine_fn backup_run(void *opaque)
             qemu_coroutine_yield();
             job->common.busy = true;
         }
+    } else if (job->sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+        /* Dirty Bitmap sync has a slightly different iteration method */
+        HBitmapIter hbi;
+        int64_t sector;
+        int64_t cluster;
+        bool polyrhythmic;
+
+        bdrv_dirty_iter_init(bs, job->sync_bitmap, &hbi);
+        /* Does the granularity happen to match our backup cluster size? */
+        polyrhythmic = (job->sync_bitmap_gran != BACKUP_CLUSTER_SIZE);
+
+        /* Find the next dirty /sector/ and copy that /cluster/ */
+        while ((sector = hbitmap_iter_next(&hbi)) != -1) {
+            if (yield_and_check(job)) {
+                goto leave;
+            }
+            cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
+
+            do {
+                ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER,
+                                    BACKUP_SECTORS_PER_CLUSTER, &error_is_read);
+                if ((ret < 0) &&
+                    backup_error_action(job, error_is_read, -ret) ==
+                    BLOCK_ERROR_ACTION_REPORT) {
+                    goto leave;
+                }
+            } while (ret < 0);
+
+            /* Advance (or rewind) our iterator if we need to. */
+            if (polyrhythmic) {
+                bdrv_dirty_iter_set(&hbi,
+                                    (cluster + 1) * BACKUP_SECTORS_PER_CLUSTER);
+            }
+        }
     } else {
         /* Both FULL and TOP SYNC_MODE's require copying.. */
         for (; start < end; start++) {
-            bool error_is_read;
-
-            if (block_job_is_cancelled(&job->common)) {
-                break;
-            }
-
-            /* we need to yield so that qemu_aio_flush() returns.
-             * (without, VM does not reboot)
-             */
-            if (job->common.speed) {
-                uint64_t delay_ns = ratelimit_calculate_delay(
-                        &job->limit, job->sectors_read);
-                job->sectors_read = 0;
-                block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, delay_ns);
-            } else {
-                block_job_sleep_ns(&job->common, QEMU_CLOCK_REALTIME, 0);
-            }
-
-            if (block_job_is_cancelled(&job->common)) {
+            if (yield_and_check(job)) {
                 break;
             }
 
@@ -351,12 +396,16 @@ static void coroutine_fn backup_run(void *opaque)
         }
     }
 
+leave:
     notifier_with_return_remove(&before_write);
 
     /* wait until pending backup_do_cow() calls have completed */
     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);
@@ -368,12 +417,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);
@@ -386,6 +438,36 @@ 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:
+            error_setg(errp, "Invalid BitmapUseMode (%s) given to backup_start",
+                       BitmapUseMode_lookup[bitmap_mode]);
+            return;
+        }
+        bdrv_disable_dirty_bitmap(sync_bitmap);
+    } else if (sync_bitmap) {
+        error_setg(errp,
+                   "a sync_bitmap was provided to backup_run, "
+                   "but received an incompatible sync_mode (%s)",
+                   BitmapUseMode_lookup[sync_mode]);
+        return;
+    }
+
     len = bdrv_getlength(bs);
     if (len < 0) {
         error_setg_errno(errp, -len, "unable to get length for '%s'",
@@ -403,6 +485,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 3633632..af91ae0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -714,6 +714,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 c6b0bf1..3ab3404 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1562,6 +1562,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);
@@ -2319,6 +2321,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)
@@ -2326,6 +2330,7 @@ void qmp_drive_backup(const char *device, const char *target,
     BlockDriverState *bs;
     BlockDriverState *target_bs;
     BlockDriverState *source = NULL;
+    BdrvDirtyBitmap *bmap = NULL;
     AioContext *aio_context;
     BlockDriver *drv = NULL;
     Error *local_err = NULL;
@@ -2421,7 +2426,17 @@ void qmp_drive_backup(const char *device, const char *target,
 
     bdrv_set_aio_context(target_bs, aio_context);
 
-    backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
+    if (has_bitmap) {
+        bmap = bdrv_find_dirty_bitmap(bs, bitmap);
+        if (!bmap) {
+            error_setg(errp, "Bitmap '%s' could not be found", bitmap);
+            goto out;
+        }
+    }
+
+    backup_start(bs, target_bs, speed, sync, bmap,
+                 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 481be80..17a2a2b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1021,7 +1021,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.h b/include/block/block.h
index b583457..b861433 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -453,6 +453,7 @@ 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);
 void bdrv_dirty_iter_init(BlockDriverState *bs,
                           BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
+void bdrv_dirty_iter_set(struct HBitmapIter *hbi, int64_t offset);
 int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 
 void bdrv_enable_copy_on_read(BlockDriverState *bs);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 192fce8..ae4b475 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -576,6 +576,10 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
  * @target: Block device to write to.
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @sync_mode: What parts of the disk image should be copied to the destination.
+ * @sync_bitmap: The dirty bitmap if sync_mode is MIRROR_SYNC_MODE_DIRTY_BITMAP.
+ * @bitmap_mode: BITMAP_USE_MODE_{CONSUME, RESET}
+ *               Reset: Make a copy and reset the original bitmap.
+ *               Consume: Anonymize the bitmap and free it after completion.
  * @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.
@@ -586,6 +590,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 64b5755..04f9824 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -500,7 +500,7 @@
 # Since: 1.3
 ##
 { 'enum': 'MirrorSyncMode',
-  'data': ['top', 'full', 'none'] }
+  'data': ['top', 'full', 'none', 'dirty-bitmap'] }
 
 ##
 # @BlockJobType:
@@ -663,6 +663,21 @@
             '*format': 'str', '*mode': 'NewImageMode' } }
 
 ##
+# @BitmapUseMode
+#
+# An enumeration that tells QEMU what operation to take when using a bitmap
+# in drive backup sync mode dirty-bitmap.
+#
+# @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.
@@ -675,14 +690,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.3)
+#
 # @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).
@@ -700,7 +721,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 9a69633..1eba583 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

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

[John Snow]

From: Fam Zheng <famz@redhat.com>

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>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c       | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |  5 +++-
 2 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 3ab3404..da03025 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1596,6 +1596,76 @@ static void drive_backup_clean(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_lookup_bs(action->device, NULL, NULL);
+    if (bs) {
+        bm = bdrv_find_dirty_bitmap(bs, action->name);
+        if (bm) {
+            bdrv_release_dirty_bitmap(bs, bm);
+        }
+    }
+}
+
+typedef struct BlockDirtyBitmapState {
+    BlkTransactionState common;
+    BdrvDirtyBitmap *bitmap;
+} BlockDirtyBitmapState;
+
+/**
+ * Enable and Disable re-uses the same preparation.
+ */
+static void block_dirty_bitmap_en_toggle_prepare(BlkTransactionState *common,
+                                                 Error **errp)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+    BlockDirtyBitmap *action;
+
+    /* We may be used by either enable or disable;
+     * We use the "enable" member of the union here,
+     * but "disable" should be functionally equivalent: */
+    action = common->action->block_dirty_bitmap_enable;
+    assert(action == common->action->block_dirty_bitmap_disable);
+
+    state->bitmap = block_dirty_bitmap_lookup(action->device,
+                                              action->name,
+                                              errp);
+    if (!state->bitmap) {
+        return;
+    }
+}
+
+static void block_dirty_bitmap_enable_commit(BlkTransactionState *common)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+    bdrv_enable_dirty_bitmap(state->bitmap);
+}
+
+static void block_dirty_bitmap_disable_commit(BlkTransactionState *common)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+    bdrv_disable_dirty_bitmap(state->bitmap);
+}
+
 static void abort_prepare(BlkTransactionState *common, Error **errp)
 {
     error_setg(errp, "Transaction aborted using Abort action");
@@ -1630,6 +1700,21 @@ static const BdrvActionOps actions[] = {
         .abort = internal_snapshot_abort,
         .clean = internal_snapshot_clean,
     },
+    [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(BlockDirtyBitmapState),
+        .prepare = block_dirty_bitmap_en_toggle_prepare,
+        .commit = block_dirty_bitmap_enable_commit,
+    },
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
+        .instance_size = sizeof(BlockDirtyBitmapState),
+        .prepare = block_dirty_bitmap_en_toggle_prepare,
+        .commit = block_dirty_bitmap_disable_commit,
+    },
 };
 
 /*
diff --git a/qapi-schema.json b/qapi-schema.json
index 9ffdcf8..793031b 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

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

[John Snow]

From: Fam Zheng <famz@redhat.com>

Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@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 42244f6..677bc6f 100644
--- a/block.c
+++ b/block.c
@@ -5439,6 +5439,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
         info->granularity = bdrv_dirty_bitmap_granularity(bs, bm);
         info->has_name = !!bm->name;
         info->name = g_strdup(bm->name);
+        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 04f9824..4efc66e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -328,10 +328,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

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

[John Snow]

From: Fam Zheng <famz@redhat.com>

Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@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 f57f154..95bb959 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 v9 01/10] qapi: Add optional field "name" to block dirty bitmap

[Eric Blake]

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

On 12/01/2014 01:30 PM, John Snow wrote:
> From: Fam Zheng <famz@redhat.com>
> 
> 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>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block-migration.c     |  2 +-
>  block.c               | 32 +++++++++++++++++++++++++++++++-
>  block/mirror.c        |  2 +-
>  include/block/block.h |  7 ++++++-
>  qapi/block-core.json  |  4 +++-
>  5 files changed, 42 insertions(+), 5 deletions(-)
> 

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

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

[Eric Blake]

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

On 12/01/2014 01:30 PM, John Snow wrote:
> From: Fam Zheng <famz@redhat.com>
> 
> 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.
> 
> The granularity is an optional field. If it is not specified, we will
> choose a default granularity based on the cluster size if available,
> clamped to between 4K and 64K to mirror how the 'mirror' code was
> already choosing granularity. If we do not have cluster size info
> available, we choose 64K. This code has been factored out into a helper
> shared with block/mirror.
> 
> The types added to block-core.json will be re-used in future patches
> in this series, see:
> 'qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}'
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---

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

Worth mentioning that this is optional?

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

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

[Stefan Hajnoczi]

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

On Mon, Dec 01, 2014 at 03:30:08PM -0500, John Snow wrote:
> diff --git a/block.c b/block.c
> index e5c6ccf..3f27519 100644
> --- a/block.c
> +++ b/block.c
> @@ -5420,6 +5420,25 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
>      }
>  }
>  
> +#define BDB_MIN_DEF_GRANULARITY 4096
> +#define BDB_MAX_DEF_GRANULARITY 65536
> +#define BDB_DEFAULT_GRANULARITY BDB_MAX_DEF_GRANULARITY
> +
> +uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)

Long names are unwieldy but introducing multiple abbreviations is not a
good solution, it makes the code more confusing (BDB vs dbm).

I would call the function bdrv_get_default_bitmap_granularity().

The constants weren't necessary since the point of this function is to
capture the default value.  No one else should use the constants -
otherwise there is a high probability that they are reimplementing this
logic.  I would just leave them as literals in the code.

> diff --git a/blockdev.c b/blockdev.c
> index 5651a8e..4d30b09 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1894,6 +1894,60 @@ 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_lookup_bs(device, NULL, errp);

Markus: I think we need to support node-name here so dirty bitmaps can
be applied at any node in the graph?

> +    if (!bs) {
> +        return;
> +    }

These new monitor commands need to acquire/release the
BlockDriverState's AioContext like the other monitor commands.  This
ensures that BDS accesses in other threads are synchronized (stopped
while we run).

AioContext *aio_context;
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
...
out:
aio_context_release(aio_context);

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Stefan Hajnoczi]

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

On Mon, Dec 01, 2014 at 03:30:09PM -0500, John Snow wrote:
> diff --git a/include/block/block.h b/include/block/block.h
> index 066ded6..f180f93 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -440,6 +440,8 @@ 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);
>  uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs);
> +int64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
> +                                      BdrvDirtyBitmap *bitmap);

This looks weird now.  The default granularity has uint64_t type but the
actual granularity has int64_t.

Please make them consistent.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Stefan Hajnoczi]

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

On Mon, Dec 01, 2014 at 03:30:10PM -0500, John Snow wrote:
> +HBitmap *hbitmap_copy(const HBitmap *bitmap)
> +{
> +    int i;
> +    uint64_t size;
> +    HBitmap *hb = g_memdup(bitmap, sizeof(HBitmap));
> +
> +    size = bitmap->size;
> +    for (i = HBITMAP_LEVELS - 1; i >= 0; i--) {
> +        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;
> +}

It would be handy to put a comment in struct HBitmap warning people that
hbitmap_copy() must be kept in sync when new pointer fields are added.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Stefan Hajnoczi]

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

On Mon, Dec 01, 2014 at 03:30:12PM -0500, John Snow wrote:
> +/**
> + * Return a dirty bitmap (if present), after validating
> + * the device and bitmap names. Returns NULL on error,
> + * including when the device and/or bitmap is not found.
> + */
> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *device,
> +                                                  const char *name,
> +                                                  Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +
> +    if (!device) {
> +        error_setg(errp, "Device cannot be NULL");
> +        return NULL;
> +    }
> +    if (!name) {
> +        error_setg(errp, "Bitmap name cannot be NULL");
> +        return NULL;
> +    }
> +
> +    bs = bdrv_lookup_bs(device, NULL, errp);
> +    if (!bs) {
> +        return NULL;
> +    }
> +
> +    bitmap = bdrv_find_dirty_bitmap(bs, name);
> +    if (!bitmap) {
> +        error_setg(errp, "Dirty bitmap not found: %s", name);
> +        return NULL;
> +    }
> +
> +    return bitmap;
> +}

qmp_block_dirty_bitmap_remove() in an earlier patch should use this
instead of duplicating code.

> +
>  /* New and old BlockDriverState structs for atomic group operations */
>  
>  typedef struct BlkTransactionState BlkTransactionState;
> @@ -1948,6 +1983,32 @@ 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)
> +{
> +    BdrvDirtyBitmap *bitmap;
> +
> +    bitmap = block_dirty_bitmap_lookup(device, name, errp);
> +    if (!bitmap) {
> +        return;
> +    }
> +
> +    bdrv_enable_dirty_bitmap(bitmap);
> +}
> +
> +void qmp_block_dirty_bitmap_disable(const char *device, const char *name,
> +                                    Error **errp)
> +{
> +    BdrvDirtyBitmap *bitmap;
> +
> +    bitmap = block_dirty_bitmap_lookup(device, name, errp);
> +    if (!bitmap) {
> +        return;
> +    }
> +
> +    bdrv_disable_dirty_bitmap(bitmap);
> +}

The dirty bitmap is accessed during the BDS request processing code
path and is owned by the BDS, so it should be protected by AioContext
too.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Stefan Hajnoczi]

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

On Mon, Dec 01, 2014 at 03:30:13PM -0500, John Snow wrote:
> From: Fam Zheng <famz@redhat.com>
> 
> 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.

It's not obvious to me that we need both modes.  Can you explain why the
choice between reset and consume is necessary?

> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c                   |   5 ++
>  block/backup.c            | 130 ++++++++++++++++++++++++++++++++++++++--------
>  block/mirror.c            |   4 ++
>  blockdev.c                |  17 +++++-
>  hmp.c                     |   4 +-
>  include/block/block.h     |   1 +
>  include/block/block_int.h |   6 +++
>  qapi/block-core.json      |  30 +++++++++--
>  qmp-commands.hx           |   7 +--
>  9 files changed, 174 insertions(+), 30 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 85215b3..42244f6 100644
> --- a/block.c
> +++ b/block.c
> @@ -5489,6 +5489,11 @@ void bdrv_dirty_iter_init(BlockDriverState *bs,
>      hbitmap_iter_init(hbi, bitmap->bitmap, 0);
>  }
>  
> +void bdrv_dirty_iter_set(HBitmapIter *hbi, int64_t offset)
> +{
> +    hbitmap_iter_init(hbi, hbi->hb, offset);
> +}
> +
>  void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
>                      int nr_sectors)
>  {
> diff --git a/block/backup.c b/block/backup.c
> index 792e655..2aab68f 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 */
> +    int64_t sync_bitmap_gran;

What is the point of this field?

I think we've duplicated granularity twice now (originally stored in
HBitmap, once in BdrvDirtyBitmap, and now again).

Maybe the one user of this field should just call
bdrv_dirty_bitmap_granularity().

>      MirrorSyncMode sync_mode;
>      RateLimit limit;
>      BlockdevOnError on_source_error;
> @@ -242,6 +246,31 @@ static void backup_complete(BlockJob *job, void *opaque)
>      g_free(data);
>  }
>  
> +static bool yield_and_check(BackupBlockJob *job)

missing coroutine_fn annotation.  coroutine_fn tells the reader that
this function may only be called from coroutine context.

(In this case the function and contains "yield" so that's a big hint but
please still always mark coroutine functions coroutine_fn so that we can
enforce static checking in the future.)

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Stefan Hajnoczi]

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

On Mon, Dec 01, 2014 at 03:30:14PM -0500, John Snow wrote:
> From: Fam Zheng <famz@redhat.com>
> 
> 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>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev.c       | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json |  5 +++-
>  2 files changed, 89 insertions(+), 1 deletion(-)

Please the following commint on block-next for examples of how to
acquire/release AioContext in qmp_transaction:

commit 6a50d4a7f758436828091336c8905b6fa870f8cb
Author: Stefan Hajnoczi <stefanha@redhat.com>
Date:   Fri Nov 21 10:48:59 2014 +0000

    blockdev: acquire AioContext in QMP 'transaction' actions

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Fam Zheng]

On Tue, 12/09 17:44, Stefan Hajnoczi wrote:
> On Mon, Dec 01, 2014 at 03:30:13PM -0500, John Snow wrote:
> > From: Fam Zheng <famz@redhat.com>
> > 
> > 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.
> 
> It's not obvious to me that we need both modes.  Can you explain why the
> choice between reset and consume is necessary?

Reset is used in continuous incremental backup, which is obvious: user can
track the new data that comes after this drive-backup starts.

Consume is better when user has no intention to use this dirty bitmap
afterward. It comes with more convenience and less overhead: no need to
explicitly free the bitmap, and no need for drive-backup job to copy the dirty
bitmap.

Copy shouldn't be too slow if it's just in memory, but it does require
2x memory anyway.

Alternatively these can all be implemented with transactions.

Fam

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

[Vladimir Sementsov-Ogievskiy]

about naming:

We will need functions for set/unset a subregion of BdrvDirtyBitmap, to 
fix migration and mirror (accordingly to my "[PATCH v2] block: fix 
spoiling all dirty bitmaps by mirror and migration"). Having the 
function 'bdrv_reset_dirty_bitmap' from this patch, we'll have to add 
functions like

bdrv_set_dirty_bitmap_region and bdrv_reset_dirty_bitmap_region

But, may be, it is more consistent to have

bdrv_set_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, uint64_t start, uint64_t count)
bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, uint64_t start, uint64_t count)

for more transparent access to underlaying hbitmap interface? And then, 
name the considered function like 'bdrv_clear_dirty_bitmap'?

Best regards,
Vladimir

On 01.12.2014 23:30, John Snow wrote:
> bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)

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

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Mon, Dec 01, 2014 at 03:30:08PM -0500, John Snow wrote:
>> diff --git a/block.c b/block.c
>> index e5c6ccf..3f27519 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -5420,6 +5420,25 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
>>      }
>>  }
>>  
>> +#define BDB_MIN_DEF_GRANULARITY 4096
>> +#define BDB_MAX_DEF_GRANULARITY 65536
>> +#define BDB_DEFAULT_GRANULARITY BDB_MAX_DEF_GRANULARITY
>> +
>> +uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
>
> Long names are unwieldy but introducing multiple abbreviations is not a
> good solution, it makes the code more confusing (BDB vs dbm).
>
> I would call the function bdrv_get_default_bitmap_granularity().
>
> The constants weren't necessary since the point of this function is to
> capture the default value.  No one else should use the constants -
> otherwise there is a high probability that they are reimplementing this
> logic.  I would just leave them as literals in the code.
>
>> diff --git a/blockdev.c b/blockdev.c
>> index 5651a8e..4d30b09 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1894,6 +1894,60 @@ 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_lookup_bs(device, NULL, errp);
>
> Markus: I think we need to support node-name here so dirty bitmaps can
> be applied at any node in the graph?

We need to consider node names for all new QMP commands.  Whenever we
add a backend name parameter, like we do for the two new commands in
this patch, we need to decide whether nodes make sense, too.  Do they?

I'm afraid we haven't quite made up our mind what to do when nodes make
sense.

Existing patterns of backend / node name parameters:

1. Backend name only

   Parameter is commonly called @device.  Examples: eject,
   block_set_io_throttle.

   Code uses blk_by_name() or bdrv_find().  The latter needs to be
   converted to the former.

2. Backend or node name

2a. Two optional parameters, commonly called @device and @node-name,
   of which exactly one must be given.  Example: block_passwd.

   Code uses

        bs = bdrv_lookup_bs(has_device ? device : NULL,
                            has_node_name ? node_name : NULL,
                                &local_err);

   which is a roundabout way to say

        bs = bdrv_lookup_bs(device, node_name, &local_err);

2b. Single parameter.  The single example is anonymous union
   BlockdevRef.

   Code uses

        bs = bdrv_lookup_bs(reference, reference, errp);

   If we want to adopt "single parameter" going forward, we need to
   decide on a naming convention.  Existing commands are probably stuck
   with @device for compatibility.  Do we care for names enough to
   improve on that?

   A convenience wrapper around bdrv_lookup_bs() to avoid stuttering
   name argument could make sense.

3. Node name only

   No known example.

4. Backend and node name

   The backend parameter is commonly called @device.  The node parameter
   name varies.  Example: blockdev-snapshot-sync's @snapshot-node-name,
   change-backing-file's @image-node-name.

   Code uses either

        bdrv_find_node(node_name)

   or

        bdrv_lookup_bs(NULL, node_name, &local_err).
 
This patch invents yet another code pattern:

        bdrv_lookup_bs(device, NULL, errp);

I take this as a sign that something's amiss, either with the existing
patterns or with the new one :)

[...]

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

[Stefan Hajnoczi]

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

On Wed, Dec 10, 2014 at 01:43:47PM +0100, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@gmail.com> writes:
> 
> > On Mon, Dec 01, 2014 at 03:30:08PM -0500, John Snow wrote:
> >> diff --git a/block.c b/block.c
> >> index e5c6ccf..3f27519 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -5420,6 +5420,25 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
> >>      }
> >>  }
> >>  
> >> +#define BDB_MIN_DEF_GRANULARITY 4096
> >> +#define BDB_MAX_DEF_GRANULARITY 65536
> >> +#define BDB_DEFAULT_GRANULARITY BDB_MAX_DEF_GRANULARITY
> >> +
> >> +uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
> >
> > Long names are unwieldy but introducing multiple abbreviations is not a
> > good solution, it makes the code more confusing (BDB vs dbm).
> >
> > I would call the function bdrv_get_default_bitmap_granularity().
> >
> > The constants weren't necessary since the point of this function is to
> > capture the default value.  No one else should use the constants -
> > otherwise there is a high probability that they are reimplementing this
> > logic.  I would just leave them as literals in the code.
> >
> >> diff --git a/blockdev.c b/blockdev.c
> >> index 5651a8e..4d30b09 100644
> >> --- a/blockdev.c
> >> +++ b/blockdev.c
> >> @@ -1894,6 +1894,60 @@ 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_lookup_bs(device, NULL, errp);
> >
> > Markus: I think we need to support node-name here so dirty bitmaps can
> > be applied at any node in the graph?
> 
> We need to consider node names for all new QMP commands.  Whenever we
> add a backend name parameter, like we do for the two new commands in
> this patch, we need to decide whether nodes make sense, too.  Do they?
> 
> I'm afraid we haven't quite made up our mind what to do when nodes make
> sense.
> 
> Existing patterns of backend / node name parameters:
> 
> 1. Backend name only
> 
>    Parameter is commonly called @device.  Examples: eject,
>    block_set_io_throttle.
> 
>    Code uses blk_by_name() or bdrv_find().  The latter needs to be
>    converted to the former.
> 
> 2. Backend or node name
> 
> 2a. Two optional parameters, commonly called @device and @node-name,
>    of which exactly one must be given.  Example: block_passwd.
> 
>    Code uses
> 
>         bs = bdrv_lookup_bs(has_device ? device : NULL,
>                             has_node_name ? node_name : NULL,
>                                 &local_err);
> 
>    which is a roundabout way to say
> 
>         bs = bdrv_lookup_bs(device, node_name, &local_err);
> 
> 2b. Single parameter.  The single example is anonymous union
>    BlockdevRef.
> 
>    Code uses
> 
>         bs = bdrv_lookup_bs(reference, reference, errp);
> 
>    If we want to adopt "single parameter" going forward, we need to
>    decide on a naming convention.  Existing commands are probably stuck
>    with @device for compatibility.  Do we care for names enough to
>    improve on that?
> 
>    A convenience wrapper around bdrv_lookup_bs() to avoid stuttering
>    name argument could make sense.

Initially only the backend needs dirty bitmap support (this satisfies
the incremental backup use case).

It is possible that future use cases will require node-name.

I'm happy with just allowing the device parameter today and adding the
node-name parameter if needed later.

This conservative approach seems good because IMO we shouldn't add
unused features to the API since they need to be tested and maintained
forever.

So maybe use a device argument with blk_by_name() for now.

In the future switch to bdrv_lookup_bs() with has_device/has_node_name.

If anyone strongly feels we should support node-name from day 1, I'm
okay with that too but there needs to be a test case which actually
exercises that code!

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Stefan Hajnoczi]

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

On Wed, Dec 10, 2014 at 09:34:00AM +0800, Fam Zheng wrote:
> On Tue, 12/09 17:44, Stefan Hajnoczi wrote:
> > On Mon, Dec 01, 2014 at 03:30:13PM -0500, John Snow wrote:
> > > From: Fam Zheng <famz@redhat.com>
> > > 
> > > 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.
> > 
> > It's not obvious to me that we need both modes.  Can you explain why the
> > choice between reset and consume is necessary?
> 
> Reset is used in continuous incremental backup, which is obvious: user can
> track the new data that comes after this drive-backup starts.
> 
> Consume is better when user has no intention to use this dirty bitmap
> afterward. It comes with more convenience and less overhead: no need to
> explicitly free the bitmap, and no need for drive-backup job to copy the dirty
> bitmap.
> 
> Copy shouldn't be too slow if it's just in memory, but it does require
> 2x memory anyway.
> 
> Alternatively these can all be implemented with transactions.

I'm concerned about how error cases and live migration will be handled.

It is important not to lose the contents of the dirty bitmap until
backup has completed successfully.

If there is an I/O error, what happens?  While in error state can QEMU
be shut down or can live migration take place?

I prefer if we have just one mode that is fully tested and thought
through.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Markus Armbruster]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Wed, Dec 10, 2014 at 01:43:47PM +0100, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>> 
>> > On Mon, Dec 01, 2014 at 03:30:08PM -0500, John Snow wrote:
>> >> diff --git a/block.c b/block.c
>> >> index e5c6ccf..3f27519 100644
>> >> --- a/block.c
>> >> +++ b/block.c
>> >> @@ -5420,6 +5420,25 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
>> >>      }
>> >>  }
>> >>  
>> >> +#define BDB_MIN_DEF_GRANULARITY 4096
>> >> +#define BDB_MAX_DEF_GRANULARITY 65536
>> >> +#define BDB_DEFAULT_GRANULARITY BDB_MAX_DEF_GRANULARITY
>> >> +
>> >> +uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
>> >
>> > Long names are unwieldy but introducing multiple abbreviations is not a
>> > good solution, it makes the code more confusing (BDB vs dbm).
>> >
>> > I would call the function bdrv_get_default_bitmap_granularity().
>> >
>> > The constants weren't necessary since the point of this function is to
>> > capture the default value.  No one else should use the constants -
>> > otherwise there is a high probability that they are reimplementing this
>> > logic.  I would just leave them as literals in the code.
>> >
>> >> diff --git a/blockdev.c b/blockdev.c
>> >> index 5651a8e..4d30b09 100644
>> >> --- a/blockdev.c
>> >> +++ b/blockdev.c
>> >> @@ -1894,6 +1894,60 @@ 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_lookup_bs(device, NULL, errp);
>> >
>> > Markus: I think we need to support node-name here so dirty bitmaps can
>> > be applied at any node in the graph?
>> 
>> We need to consider node names for all new QMP commands.  Whenever we
>> add a backend name parameter, like we do for the two new commands in
>> this patch, we need to decide whether nodes make sense, too.  Do they?
>> 
>> I'm afraid we haven't quite made up our mind what to do when nodes make
>> sense.
>> 
>> Existing patterns of backend / node name parameters:
>> 
>> 1. Backend name only
>> 
>>    Parameter is commonly called @device.  Examples: eject,
>>    block_set_io_throttle.
>> 
>>    Code uses blk_by_name() or bdrv_find().  The latter needs to be
>>    converted to the former.
>> 
>> 2. Backend or node name
>> 
>> 2a. Two optional parameters, commonly called @device and @node-name,
>>    of which exactly one must be given.  Example: block_passwd.
>> 
>>    Code uses
>> 
>>         bs = bdrv_lookup_bs(has_device ? device : NULL,
>>                             has_node_name ? node_name : NULL,
>>                                 &local_err);
>> 
>>    which is a roundabout way to say
>> 
>>         bs = bdrv_lookup_bs(device, node_name, &local_err);
>> 
>> 2b. Single parameter.  The single example is anonymous union
>>    BlockdevRef.
>> 
>>    Code uses
>> 
>>         bs = bdrv_lookup_bs(reference, reference, errp);
>> 
>>    If we want to adopt "single parameter" going forward, we need to
>>    decide on a naming convention.  Existing commands are probably stuck
>>    with @device for compatibility.  Do we care for names enough to
>>    improve on that?
>> 
>>    A convenience wrapper around bdrv_lookup_bs() to avoid stuttering
>>    name argument could make sense.
>
> Initially only the backend needs dirty bitmap support (this satisfies
> the incremental backup use case).
>
> It is possible that future use cases will require node-name.
>
> I'm happy with just allowing the device parameter today and adding the
> node-name parameter if needed later.
>
> This conservative approach seems good because IMO we shouldn't add
> unused features to the API since they need to be tested and maintained
> forever.
>
> So maybe use a device argument with blk_by_name() for now.
>
> In the future switch to bdrv_lookup_bs() with has_device/has_node_name.
>
> If anyone strongly feels we should support node-name from day 1, I'm
> okay with that too but there needs to be a test case which actually
> exercises that code!

Agree with not adding unused features.

However, we should make up our minds how we want QMP to do backend and
node names in the future.  I see two basic options:

1. Inertia

   Keep adding separate arguments for backend name (commonly called
   @device) and node name (commonly called @node-name).  If the command
   can take only one, make it mandatory.  If it can take either, make it
   either/or.

   Cements the inconsistency between single parameter in BlockdevRef and
   the two either/or parameters elsewhere.

2. Clean up the mess

   New commands take a single parameter.  The command accepts backends
   or nodes as they make sense for the command.  Acceptable parameter
   name needed.

   Either existing commands get changed to single parameter (with the
   necessary compatibility and discoverability gunk), or they get
   replaced by new commands.

   I'll analyze how the gunk could be done in a separate message,
   hopefully soon.

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

[John Snow]

On 12/15/2014 03:50 AM, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@gmail.com> writes:
>
>> On Wed, Dec 10, 2014 at 01:43:47PM +0100, Markus Armbruster wrote:
>>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>>>
>>>> On Mon, Dec 01, 2014 at 03:30:08PM -0500, John Snow wrote:
>>>>> diff --git a/block.c b/block.c
>>>>> index e5c6ccf..3f27519 100644
>>>>> --- a/block.c
>>>>> +++ b/block.c
>>>>> @@ -5420,6 +5420,25 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
>>>>>       }
>>>>>   }
>>>>>
>>>>> +#define BDB_MIN_DEF_GRANULARITY 4096
>>>>> +#define BDB_MAX_DEF_GRANULARITY 65536
>>>>> +#define BDB_DEFAULT_GRANULARITY BDB_MAX_DEF_GRANULARITY
>>>>> +
>>>>> +uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
>>>>
>>>> Long names are unwieldy but introducing multiple abbreviations is not a
>>>> good solution, it makes the code more confusing (BDB vs dbm).
>>>>
>>>> I would call the function bdrv_get_default_bitmap_granularity().
>>>>
>>>> The constants weren't necessary since the point of this function is to
>>>> capture the default value.  No one else should use the constants -
>>>> otherwise there is a high probability that they are reimplementing this
>>>> logic.  I would just leave them as literals in the code.
>>>>
>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>> index 5651a8e..4d30b09 100644
>>>>> --- a/blockdev.c
>>>>> +++ b/blockdev.c
>>>>> @@ -1894,6 +1894,60 @@ 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_lookup_bs(device, NULL, errp);
>>>>
>>>> Markus: I think we need to support node-name here so dirty bitmaps can
>>>> be applied at any node in the graph?
>>>
>>> We need to consider node names for all new QMP commands.  Whenever we
>>> add a backend name parameter, like we do for the two new commands in
>>> this patch, we need to decide whether nodes make sense, too.  Do they?
>>>
>>> I'm afraid we haven't quite made up our mind what to do when nodes make
>>> sense.
>>>
>>> Existing patterns of backend / node name parameters:
>>>
>>> 1. Backend name only
>>>
>>>     Parameter is commonly called @device.  Examples: eject,
>>>     block_set_io_throttle.
>>>
>>>     Code uses blk_by_name() or bdrv_find().  The latter needs to be
>>>     converted to the former.
>>>
>>> 2. Backend or node name
>>>
>>> 2a. Two optional parameters, commonly called @device and @node-name,
>>>     of which exactly one must be given.  Example: block_passwd.
>>>
>>>     Code uses
>>>
>>>          bs = bdrv_lookup_bs(has_device ? device : NULL,
>>>                              has_node_name ? node_name : NULL,
>>>                                  &local_err);
>>>
>>>     which is a roundabout way to say
>>>
>>>          bs = bdrv_lookup_bs(device, node_name, &local_err);
>>>
>>> 2b. Single parameter.  The single example is anonymous union
>>>     BlockdevRef.
>>>
>>>     Code uses
>>>
>>>          bs = bdrv_lookup_bs(reference, reference, errp);
>>>
>>>     If we want to adopt "single parameter" going forward, we need to
>>>     decide on a naming convention.  Existing commands are probably stuck
>>>     with @device for compatibility.  Do we care for names enough to
>>>     improve on that?
>>>
>>>     A convenience wrapper around bdrv_lookup_bs() to avoid stuttering
>>>     name argument could make sense.
>>
>> Initially only the backend needs dirty bitmap support (this satisfies
>> the incremental backup use case).
>>
>> It is possible that future use cases will require node-name.
>>
>> I'm happy with just allowing the device parameter today and adding the
>> node-name parameter if needed later.
>>
>> This conservative approach seems good because IMO we shouldn't add
>> unused features to the API since they need to be tested and maintained
>> forever.
>>
>> So maybe use a device argument with blk_by_name() for now.
>>
>> In the future switch to bdrv_lookup_bs() with has_device/has_node_name.
>>
>> If anyone strongly feels we should support node-name from day 1, I'm
>> okay with that too but there needs to be a test case which actually
>> exercises that code!
>
> Agree with not adding unused features.
>
> However, we should make up our minds how we want QMP to do backend and
> node names in the future.  I see two basic options:
>
> 1. Inertia
>
>     Keep adding separate arguments for backend name (commonly called
>     @device) and node name (commonly called @node-name).  If the command
>     can take only one, make it mandatory.  If it can take either, make it
>     either/or.
>
>     Cements the inconsistency between single parameter in BlockdevRef and
>     the two either/or parameters elsewhere.
>
> 2. Clean up the mess
>
>     New commands take a single parameter.  The command accepts backends
>     or nodes as they make sense for the command.  Acceptable parameter
>     name needed.
>
>     Either existing commands get changed to single parameter (with the
>     necessary compatibility and discoverability gunk), or they get
>     replaced by new commands.
>
>     I'll analyze how the gunk could be done in a separate message,
>     hopefully soon.
>

OK, given the most recent email, it seems as if you would prefer to use 
"@device" for backends and "@node-name" for arbitrary node selection. 
This command only needs to support the backend for now, I think.

Assuming we want to allow arbitrary nodes in the future, should I leave 
the parameters as @device but rename the monitor commands to allow for 
an arbitrary node version sometime later? I don't want to introduce a 
new command that creates a new mess for us to clean up as we unify these 
parameter semantics.

I said I'd use your mail as a guide but I hadn't skimmed it yet to see 
how specific the prescriptions were ;)

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 12/15/2014 03:50 AM, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>>
>>> On Wed, Dec 10, 2014 at 01:43:47PM +0100, Markus Armbruster wrote:
>>>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>>>>
>>>>> On Mon, Dec 01, 2014 at 03:30:08PM -0500, John Snow wrote:
>>>>>> diff --git a/block.c b/block.c
>>>>>> index e5c6ccf..3f27519 100644
>>>>>> --- a/block.c
>>>>>> +++ b/block.c
>>>>>> @@ -5420,6 +5420,25 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
>>>>>>       }
>>>>>>   }
>>>>>>
>>>>>> +#define BDB_MIN_DEF_GRANULARITY 4096
>>>>>> +#define BDB_MAX_DEF_GRANULARITY 65536
>>>>>> +#define BDB_DEFAULT_GRANULARITY BDB_MAX_DEF_GRANULARITY
>>>>>> +
>>>>>> +uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)
>>>>>
>>>>> Long names are unwieldy but introducing multiple abbreviations is not a
>>>>> good solution, it makes the code more confusing (BDB vs dbm).
>>>>>
>>>>> I would call the function bdrv_get_default_bitmap_granularity().
>>>>>
>>>>> The constants weren't necessary since the point of this function is to
>>>>> capture the default value.  No one else should use the constants -
>>>>> otherwise there is a high probability that they are reimplementing this
>>>>> logic.  I would just leave them as literals in the code.
>>>>>
>>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>>> index 5651a8e..4d30b09 100644
>>>>>> --- a/blockdev.c
>>>>>> +++ b/blockdev.c
>>>>>> @@ -1894,6 +1894,60 @@ 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_lookup_bs(device, NULL, errp);
>>>>>
>>>>> Markus: I think we need to support node-name here so dirty bitmaps can
>>>>> be applied at any node in the graph?
>>>>
>>>> We need to consider node names for all new QMP commands.  Whenever we
>>>> add a backend name parameter, like we do for the two new commands in
>>>> this patch, we need to decide whether nodes make sense, too.  Do they?
>>>>
>>>> I'm afraid we haven't quite made up our mind what to do when nodes make
>>>> sense.
>>>>
>>>> Existing patterns of backend / node name parameters:
>>>>
>>>> 1. Backend name only
>>>>
>>>>     Parameter is commonly called @device.  Examples: eject,
>>>>     block_set_io_throttle.
>>>>
>>>>     Code uses blk_by_name() or bdrv_find().  The latter needs to be
>>>>     converted to the former.
>>>>
>>>> 2. Backend or node name
>>>>
>>>> 2a. Two optional parameters, commonly called @device and @node-name,
>>>>     of which exactly one must be given.  Example: block_passwd.
>>>>
>>>>     Code uses
>>>>
>>>>          bs = bdrv_lookup_bs(has_device ? device : NULL,
>>>>                              has_node_name ? node_name : NULL,
>>>>                                  &local_err);
>>>>
>>>>     which is a roundabout way to say
>>>>
>>>>          bs = bdrv_lookup_bs(device, node_name, &local_err);
>>>>
>>>> 2b. Single parameter.  The single example is anonymous union
>>>>     BlockdevRef.
>>>>
>>>>     Code uses
>>>>
>>>>          bs = bdrv_lookup_bs(reference, reference, errp);
>>>>
>>>>     If we want to adopt "single parameter" going forward, we need to
>>>>     decide on a naming convention.  Existing commands are probably stuck
>>>>     with @device for compatibility.  Do we care for names enough to
>>>>     improve on that?
>>>>
>>>>     A convenience wrapper around bdrv_lookup_bs() to avoid stuttering
>>>>     name argument could make sense.
>>>
>>> Initially only the backend needs dirty bitmap support (this satisfies
>>> the incremental backup use case).
>>>
>>> It is possible that future use cases will require node-name.
>>>
>>> I'm happy with just allowing the device parameter today and adding the
>>> node-name parameter if needed later.
>>>
>>> This conservative approach seems good because IMO we shouldn't add
>>> unused features to the API since they need to be tested and maintained
>>> forever.
>>>
>>> So maybe use a device argument with blk_by_name() for now.
>>>
>>> In the future switch to bdrv_lookup_bs() with has_device/has_node_name.
>>>
>>> If anyone strongly feels we should support node-name from day 1, I'm
>>> okay with that too but there needs to be a test case which actually
>>> exercises that code!
>>
>> Agree with not adding unused features.
>>
>> However, we should make up our minds how we want QMP to do backend and
>> node names in the future.  I see two basic options:
>>
>> 1. Inertia
>>
>>     Keep adding separate arguments for backend name (commonly called
>>     @device) and node name (commonly called @node-name).  If the command
>>     can take only one, make it mandatory.  If it can take either, make it
>>     either/or.
>>
>>     Cements the inconsistency between single parameter in BlockdevRef and
>>     the two either/or parameters elsewhere.
>>
>> 2. Clean up the mess
>>
>>     New commands take a single parameter.  The command accepts backends
>>     or nodes as they make sense for the command.  Acceptable parameter
>>     name needed.
>>
>>     Either existing commands get changed to single parameter (with the
>>     necessary compatibility and discoverability gunk), or they get
>>     replaced by new commands.
>>
>>     I'll analyze how the gunk could be done in a separate message,
>>     hopefully soon.
>>
>
> OK, given the most recent email, it seems as if you would prefer to
> use "@device" for backends and "@node-name" for arbitrary node
> selection. This command only needs to support the backend for now, I
> think.

Whenever you think "only backend for now", you should pause and think
hard whether the command makes sense only for backends, or whether we
merely choose to implement only backends for now.

If it's the former, go ahead and call it @device.  It's a stupid name,
but it's traditional.

If it's the latter:

> Assuming we want to allow arbitrary nodes in the future, should I
> leave the parameters as @device but rename the monitor commands to
> allow for an arbitrary node version sometime later? I don't want to
> introduce a new command that creates a new mess for us to clean up as
> we unify these parameter semantics.

As you wrote, calling it @device now will make future generalization to
nodes awkward.  I'd lean towards calling it @node-name right away,
documenting "backend root nodes only" as a restriction.  But others
could have different opinions.  Let's hash it out.

> I said I'd use your mail as a guide but I hadn't skimmed it yet to see
> how specific the prescriptions were ;)

Okay :)