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

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

[Fam Zheng]

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

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

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

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

Fam

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

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

-- 
1.9.3

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

[Fam Zheng]

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

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

Add bdrv_dirty_bitmap_make_anon. This unsets the name of dirty bitmap.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block-migration.c     |  2 +-
 block.c               | 34 +++++++++++++++++++++++++++++++++-
 block/mirror.c        |  2 +-
 include/block/block.h |  7 ++++++-
 qapi/block-core.json  |  4 +++-
 5 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index 08db01a..6f3aa18 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -319,7 +319,7 @@ static int set_dirty_tracking(void)
 
     QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
         bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE,
-                                                      NULL);
+                                                      NULL, NULL);
         if (!bmds->dirty_bitmap) {
             ret = -errno;
             goto fail;
diff --git a/block.c b/block.c
index 88f6d9b..dafde4b 100644
--- a/block.c
+++ b/block.c
@@ -53,6 +53,7 @@
 
 struct BdrvDirtyBitmap {
     HBitmap *bitmap;
+    char *name;
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -5260,7 +5261,28 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov)
     return true;
 }
 
-BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
+BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
+{
+    BdrvDirtyBitmap *bm;
+
+    assert(name);
+    QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
+        if (bm->name && !strcmp(name, bm->name)) {
+            return bm;
+        }
+    }
+    return NULL;
+}
+
+void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+    g_free(bitmap->name);
+    bitmap->name = NULL;
+}
+
+BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
+                                          int granularity,
+                                          const char *name,
                                           Error **errp)
 {
     int64_t bitmap_size;
@@ -5268,6 +5290,10 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
 
     assert((granularity & (granularity - 1)) == 0);
 
+    if (name && bdrv_find_dirty_bitmap(bs, name)) {
+        error_setg(errp, "Bitmap already exists: %s", name);
+        return NULL;
+    }
     granularity >>= BDRV_SECTOR_BITS;
     assert(granularity);
     bitmap_size = bdrv_nb_sectors(bs);
@@ -5278,6 +5304,9 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
     }
     bitmap = g_new0(BdrvDirtyBitmap, 1);
     bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
+    if (name) {
+        bitmap->name = g_strdup(name);
+    }
     QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
     return bitmap;
 }
@@ -5289,6 +5318,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
         if (bm == bitmap) {
             QLIST_REMOVE(bitmap, list);
             hbitmap_free(bitmap->bitmap);
+            g_free(bitmap->name);
             g_free(bitmap);
             return;
         }
@@ -5307,6 +5337,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
         info->count = bdrv_get_dirty_count(bs, bm);
         info->granularity =
             ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
+        info->has_name = !!bm->name;
+        info->name = info->has_name ? g_strdup(bm->name) : NULL;
         entry->value = info;
         *plist = entry;
         plist = &entry->next;
diff --git a/block/mirror.c b/block/mirror.c
index e8a43eb..cecd448 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -656,7 +656,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
     s->granularity = granularity;
     s->buf_size = MAX(buf_size, granularity);
 
-    s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, errp);
+    s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
     if (!s->dirty_bitmap) {
         return;
     }
diff --git a/include/block/block.h b/include/block/block.h
index 341054d..b0ef82c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -419,8 +419,13 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov);
 
 struct HBitmapIter;
 typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
-BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
+BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
+                                          int granularity,
+                                          const char *name,
                                           Error **errp);
+BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
+                                        const char *name);
+void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8f7089e..62b0356 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -307,6 +307,8 @@
 #
 # Block dirty bitmap information.
 #
+# @name: #optional the name of the dirty bitmap (Since 2.3)
+#
 # @count: number of dirty bytes according to the dirty bitmap
 #
 # @granularity: granularity of the dirty bitmap in bytes (since 1.4)
@@ -314,7 +316,7 @@
 # Since: 1.3
 ##
 { 'type': 'BlockDirtyInfo',
-  'data': {'count': 'int', 'granularity': 'int'} }
+  'data': {'*name': 'str', 'count': 'int', 'granularity': 'int'} }
 
 ##
 # @BlockInfo:
-- 
1.9.3

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

[Max Reitz]

On 2014-10-30 at 04:22, Fam Zheng wrote:
> This field will be set for user created dirty bitmap. Also pass in an
> error pointer to bdrv_create_dirty_bitmap, so when a name is already
> taken on this BDS, it can report an error message. This is not global
> check, two BDSes can have dirty bitmap with a common name.
>
> Implemented bdrv_find_dirty_bitmap to find a dirty bitmap by name, will
> be used later when other QMP commands want to reference dirty bitmap by
> name.
>
> Add bdrv_dirty_bitmap_make_anon. This unsets the name of dirty bitmap.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block-migration.c     |  2 +-
>   block.c               | 34 +++++++++++++++++++++++++++++++++-
>   block/mirror.c        |  2 +-
>   include/block/block.h |  7 ++++++-
>   qapi/block-core.json  |  4 +++-
>   5 files changed, 44 insertions(+), 5 deletions(-)

[snip]

> diff --git a/block.c b/block.c
> index 88f6d9b..dafde4b 100644
> --- a/block.c
> +++ b/block.c
> @@ -5307,6 +5337,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>           info->count = bdrv_get_dirty_count(bs, bm);
>           info->granularity =
>               ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
> +        info->has_name = !!bm->name;

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

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

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

[Eric Blake]

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

On 10/30/2014 04:22 AM, Fam Zheng wrote:
> This field will be set for user created dirty bitmap. Also pass in an
> error pointer to bdrv_create_dirty_bitmap, so when a name is already
> taken on this BDS, it can report an error message. This is not global
> check, two BDSes can have dirty bitmap with a common name.
> 
> Implemented bdrv_find_dirty_bitmap to find a dirty bitmap by name, will
> be used later when other QMP commands want to reference dirty bitmap by
> name.
> 
> Add bdrv_dirty_bitmap_make_anon. This unsets the name of dirty bitmap.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---

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

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

> @@ -5307,6 +5337,8 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
>          info->count = bdrv_get_dirty_count(bs, bm);
>          info->granularity =
>              ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bm->bitmap));
> +        info->has_name = !!bm->name;
> +        info->name = info->has_name ? g_strdup(bm->name) : NULL;

And again.

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

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


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

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

[Fam Zheng]

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

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c           | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx      | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 159 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index eb743a9..025ba3a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1803,6 +1803,61 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
     aio_context_release(aio_context);
 }
 
+void qmp_block_dirty_bitmap_add(const char *device, const char *name,
+                                bool has_granularity, int64_t granularity,
+                                Error **errp)
+{
+    BlockDriverState *bs;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    if (!name || name[0] == '\0') {
+        error_setg(errp, "Bitmap name cannot be empty");
+        return;
+    }
+    if (has_granularity) {
+        if (granularity < 512 || is_power_of_2(granularity)) {
+            error_setg(errp, "Granularity must be power of 2 "
+                             "and greater than 512");
+            return;
+        }
+    } else {
+        granularity = 65536;
+    }
+
+    bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+}
+
+void qmp_block_dirty_bitmap_remove(const char *device, const char *name,
+                                   Error **errp)
+{
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    if (!name || name[0] == '\0') {
+        error_setg(errp, "Bitmap name cannot be empty");
+        return;
+    }
+    bitmap = bdrv_find_dirty_bitmap(bs, name);
+    if (!bitmap) {
+        error_setg(errp, "Dirty bitmap not found: %s", name);
+        return;
+    }
+
+    bdrv_dirty_bitmap_make_anon(bs, bitmap);
+    bdrv_release_dirty_bitmap(bs, bitmap);
+}
+
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     const char *id = qdict_get_str(qdict, "id");
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 62b0356..a162f63 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -865,6 +865,61 @@
             '*on-target-error': 'BlockdevOnError' } }
 
 ##
+# @BlockDirtyBitmap
+#
+# @device: name of device which the bitmap is tracking
+#
+# @name: name of the dirty bitmap
+#
+# Since 2.3
+##
+{ 'type': 'BlockDirtyBitmap',
+  'data': { 'device': 'str', 'name': 'str' } }
+
+##
+# @BlockDirtyBitmapAdd
+#
+# @device: name of device which the bitmap is tracking
+#
+# @name: name of the dirty bitmap
+#
+# @granularity: #optional the bitmap granularity, default is 64k for
+#               block-dirty-bitmap-add
+#
+# Since 2.3
+##
+{ 'type': 'BlockDirtyBitmapAdd',
+  'data': { 'device': 'str', 'name': 'str', '*granularity': 'int' } }
+
+##
+# @block-dirty-bitmap-add
+#
+# Create a dirty bitmap with a name on the device
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If @name is already taken, GenericError with an explaining message
+#
+# Since 2.3
+##
+{'command': 'block-dirty-bitmap-add',
+  'data': 'BlockDirtyBitmapAdd' }
+
+##
+# @block-dirty-bitmap-remove
+#
+# Remove a dirty bitmap on the device
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If @name is not found, GenericError with an explaining message
+#
+# Since 2.3
+##
+{'command': 'block-dirty-bitmap-remove',
+  'data': 'BlockDirtyBitmap' }
+
+##
 # @block_set_io_throttle:
 #
 # Change I/O throttle limits for a block drive.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 1abd619..6f7e04c 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1202,6 +1202,55 @@ Example:
 EQMP
 
     {
+        .name       = "block-dirty-bitmap-add",
+        .args_type  = "device:B,name:s,granularity:i?",
+        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_add,
+    },
+    {
+        .name       = "block-dirty-bitmap-remove",
+        .args_type  = "device:B,name:s",
+        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove,
+    },
+
+SQMP
+
+block-dirty-bitmap-add
+----------------
+
+Create a dirty bitmap with a name on the device, and start tracking the writes.
+
+Arguments:
+
+- "device": device name to create dirty bitmap (json-string)
+- "name": name of the new dirty bitmap (json-string)
+- "granularity": granularity to track writes with. (int)
+
+Example:
+
+-> { "execute": "block-dirty-bitmap-add", "arguments": { "device": "drive0",
+                                                   "name": "bitmap0" } }
+<- { "return": {} }
+
+block-dirty-bitmap-remove
+----------------
+
+Stop write tracking and remove the dirty bitmap that was created with
+block-dirty-bitmap-add.
+
+Arguments:
+
+- "device": device name to remove dirty bitmap (json-string)
+- "name": name of the dirty bitmap to remove (json-string)
+
+Example:
+
+-> { "execute": "block-dirty-bitmap-remove", "arguments": { "device": "drive0",
+                                                      "name": "bitmap0" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "blockdev-snapshot-sync",
         .args_type  = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?",
         .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,
-- 
1.9.3

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

[Max Reitz]

On 2014-10-30 at 04:22, Fam Zheng wrote:
> The new command pair is added to manage user created dirty bitmap. The
> dirty bitmap's name is mandatory and must be unique for the same device,
> but different devices can have bitmaps with the same names.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   blockdev.c           | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   qapi/block-core.json | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   qmp-commands.hx      | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 159 insertions(+)

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

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

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

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

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

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

I think you meant !is_power_of_2().

> +            error_setg(errp, "Granularity must be power of 2 "
> +                             "and greater than 512");
> +            return;
> +        }
> +    } else {
> +        granularity = 65536;
> +    }
> +
> +    bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> +}
> +
> +void qmp_block_dirty_bitmap_remove(const char *device, const char *name,
> +                                   Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> +        return;
> +    }
> +
> +    if (!name || name[0] == '\0') {
> +        error_setg(errp, "Bitmap name cannot be empty");
> +        return;
> +    }
> +    bitmap = bdrv_find_dirty_bitmap(bs, name);
> +    if (!bitmap) {
> +        error_setg(errp, "Dirty bitmap not found: %s", name);
> +        return;
> +    }
> +
> +    bdrv_dirty_bitmap_make_anon(bs, bitmap);
> +    bdrv_release_dirty_bitmap(bs, bitmap);
> +}
> +
>   int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
>   {
>       const char *id = qdict_get_str(qdict, "id");
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 62b0356..a162f63 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -865,6 +865,61 @@
>               '*on-target-error': 'BlockdevOnError' } }
>   
>   ##
> +# @BlockDirtyBitmap
> +#
> +# @device: name of device which the bitmap is tracking
> +#
> +# @name: name of the dirty bitmap
> +#
> +# Since 2.3
> +##
> +{ 'type': 'BlockDirtyBitmap',
> +  'data': { 'device': 'str', 'name': 'str' } }
> +
> +##
> +# @BlockDirtyBitmapAdd
> +#
> +# @device: name of device which the bitmap is tracking
> +#
> +# @name: name of the dirty bitmap
> +#
> +# @granularity: #optional the bitmap granularity, default is 64k for
> +#               block-dirty-bitmap-add
> +#
> +# Since 2.3
> +##
> +{ 'type': 'BlockDirtyBitmapAdd',
> +  'data': { 'device': 'str', 'name': 'str', '*granularity': 'int' } }

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

> +
> +##
> +# @block-dirty-bitmap-add
> +#
> +# Create a dirty bitmap with a name on the device
> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If @name is already taken, GenericError with an explaining message
> +#
> +# Since 2.3
> +##
> +{'command': 'block-dirty-bitmap-add',
> +  'data': 'BlockDirtyBitmapAdd' }

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

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

I can understand not inlining the arguments here, though.

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

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

Max

> +
> +Create a dirty bitmap with a name on the device, and start tracking the writes.
> +
> +Arguments:
> +
> +- "device": device name to create dirty bitmap (json-string)
> +- "name": name of the new dirty bitmap (json-string)
> +- "granularity": granularity to track writes with. (int)
> +
> +Example:
> +
> +-> { "execute": "block-dirty-bitmap-add", "arguments": { "device": "drive0",
> +                                                   "name": "bitmap0" } }
> +<- { "return": {} }
> +
> +block-dirty-bitmap-remove
> +----------------
> +
> +Stop write tracking and remove the dirty bitmap that was created with
> +block-dirty-bitmap-add.
> +
> +Arguments:
> +
> +- "device": device name to remove dirty bitmap (json-string)
> +- "name": name of the dirty bitmap to remove (json-string)
> +
> +Example:
> +
> +-> { "execute": "block-dirty-bitmap-remove", "arguments": { "device": "drive0",
> +                                                      "name": "bitmap0" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>           .name       = "blockdev-snapshot-sync",
>           .args_type  = "device:s?,node-name:s?,snapshot-file:s,snapshot-node-name:s?,format:s?,mode:s?",
>           .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_sync,

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

[Eric Blake]

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

On 10/30/2014 04:22 AM, Fam Zheng wrote:
> The new command pair is added to manage user created dirty bitmap. The
> dirty bitmap's name is mandatory and must be unique for the same device,
> but different devices can have bitmaps with the same names.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c           | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qmp-commands.hx      | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 159 insertions(+)
> 

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

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


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

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

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

Is it worth using type inheritance, as in:

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

> +
> +##
> +# @block-dirty-bitmap-add
> +#
> +# Create a dirty bitmap with a name on the device
> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If @name is already taken, GenericError with an explaining message

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


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

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

> +
> +Create a dirty bitmap with a name on the device, and start tracking the writes.
> +
> +Arguments:
> +
> +- "device": device name to create dirty bitmap (json-string)
> +- "name": name of the new dirty bitmap (json-string)
> +- "granularity": granularity to track writes with. (int)

Inconsistent on whether you use trailing '.'

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

Pad out the ---

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


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

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

[John Snow]

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

[snip]

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

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

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

As an example, If you have some type:

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

And an extension of it:

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

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

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

...

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


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

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

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

[Eric Blake]

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

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

Fair enough.

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

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

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

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

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


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

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

[Fam Zheng]

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

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

diff --git a/block.c b/block.c
index dafde4b..1b12541 100644
--- a/block.c
+++ b/block.c
@@ -5356,6 +5356,12 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
     }
 }
 
+int bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
+                                  BdrvDirtyBitmap *bitmap)
+{
+    return hbitmap_granularity(bitmap->bitmap);
+}
+
 void bdrv_dirty_iter_init(BlockDriverState *bs,
                           BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
 {
diff --git a/include/block/block.h b/include/block/block.h
index b0ef82c..87fa48e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -428,6 +428,8 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
 void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
+int bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
+                                  BdrvDirtyBitmap *bitmap);
 int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
 void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
 void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
-- 
1.9.3

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

[Max Reitz]

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

Actually, it does not.

> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
> ---
>   block.c               | 6 ++++++
>   include/block/block.h | 2 ++
>   2 files changed, 8 insertions(+)
>
> diff --git a/block.c b/block.c
> index dafde4b..1b12541 100644
> --- a/block.c
> +++ b/block.c
> @@ -5356,6 +5356,12 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
>       }
>   }
>   
> +int bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
> +                                  BdrvDirtyBitmap *bitmap)
> +{
> +    return hbitmap_granularity(bitmap->bitmap);
> +}
> +

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

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

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

Max

>   void bdrv_dirty_iter_init(BlockDriverState *bs,
>                             BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
>   {
> diff --git a/include/block/block.h b/include/block/block.h
> index b0ef82c..87fa48e 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -428,6 +428,8 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
>   void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
>   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
>   BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
> +int bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
> +                                  BdrvDirtyBitmap *bitmap);
>   int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector);
>   void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
>   void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);

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

[Fam Zheng]

This makes a deep copy of an HBitmap.

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

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

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

[Max Reitz]

On 2014-10-30 at 04:22, Fam Zheng wrote:
> This makes a deep copy of an HBitmap.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   include/qemu/hbitmap.h |  8 ++++++++
>   util/hbitmap.c         | 16 ++++++++++++++++
>   2 files changed, 24 insertions(+)
>
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index 550d7ce..b645cfc 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -65,6 +65,14 @@ struct HBitmapIter {
>   HBitmap *hbitmap_alloc(uint64_t size, int granularity);
>   
>   /**
> + * hbitmap_copy:
> + * @bitmap: The original bitmap to copy.
> + *
> + * Copy a HBitmap.
> + */
> +HBitmap *hbitmap_copy(const HBitmap *bitmap);
> +
> +/**
>    * hbitmap_empty:
>    * @hb: HBitmap to operate on.
>    *
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index b3060e6..78d449e 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -395,3 +395,19 @@ HBitmap *hbitmap_alloc(uint64_t size, int granularity)
>       hb->levels[0][0] |= 1UL << (BITS_PER_LONG - 1);
>       return hb;
>   }
> +
> +HBitmap *hbitmap_copy(const HBitmap *bitmap)
> +{
> +    int i;
> +    int64_t size;

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

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

"struct" can be omitted.

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

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

> +        size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1);
> +        hb->levels[i] = g_memdup(bitmap->levels[i],
> +                                 size * sizeof(unsigned long));
> +    }
> +
> +    return hb;
> +}

With or without any of the changes:

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

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

[Fam Zheng]

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

diff --git a/block.c b/block.c
index 1b12541..39381cd 100644
--- a/block.c
+++ b/block.c
@@ -53,6 +53,8 @@
 
 struct BdrvDirtyBitmap {
     HBitmap *bitmap;
+    int64_t size;
+    int granularity;
     char *name;
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
@@ -5280,6 +5282,26 @@ void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
     bitmap->name = NULL;
 }
 
+BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
+                                        const BdrvDirtyBitmap *bitmap,
+                                        const char *name)
+{
+    BdrvDirtyBitmap *new_bitmap;
+
+    new_bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
+    new_bitmap->bitmap = hbitmap_copy(bitmap->bitmap);
+    new_bitmap->size = bitmap->size;
+    new_bitmap->granularity = bitmap->granularity;
+    new_bitmap->name = name ? g_strdup(name) : NULL;
+    QLIST_INSERT_HEAD(&bs->dirty_bitmaps, new_bitmap, list);
+    return new_bitmap;
+}
+
+void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+    hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
+}
+
 BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
                                           int granularity,
                                           const char *name,
@@ -5303,7 +5325,9 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
         return NULL;
     }
     bitmap = g_new0(BdrvDirtyBitmap, 1);
-    bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
+    bitmap->size = bitmap_size;
+    bitmap->granularity = ffs(granularity) - 1;
+    bitmap->bitmap = hbitmap_alloc(bitmap->size, bitmap->granularity);
     if (name) {
         bitmap->name = g_strdup(name);
     }
diff --git a/include/block/block.h b/include/block/block.h
index 87fa48e..c5a3aa6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -426,6 +426,10 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
 BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
                                         const char *name);
 void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
+                                        const BdrvDirtyBitmap *bitmap,
+                                        const char *name);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 int bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
-- 
1.9.3

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

[Max Reitz]

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

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

You could use it for bdrv_dirty_bitmap_granularity(), though.

>       char *name;
>       QLIST_ENTRY(BdrvDirtyBitmap) list;
>   };
> @@ -5280,6 +5282,26 @@ void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>       bitmap->name = NULL;
>   }
>   
> +BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
> +                                        const BdrvDirtyBitmap *bitmap,
> +                                        const char *name)
> +{
> +    BdrvDirtyBitmap *new_bitmap;
> +
> +    new_bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
> +    new_bitmap->bitmap = hbitmap_copy(bitmap->bitmap);
> +    new_bitmap->size = bitmap->size;
> +    new_bitmap->granularity = bitmap->granularity;
> +    new_bitmap->name = name ? g_strdup(name) : NULL;
> +    QLIST_INSERT_HEAD(&bs->dirty_bitmaps, new_bitmap, list);
> +    return new_bitmap;
> +}
> +
> +void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> +{
> +    hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
> +}
> +
>   BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>                                             int granularity,
>                                             const char *name,
> @@ -5303,7 +5325,9 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>           return NULL;
>       }
>       bitmap = g_new0(BdrvDirtyBitmap, 1);
> -    bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
> +    bitmap->size = bitmap_size;
> +    bitmap->granularity = ffs(granularity) - 1;
> +    bitmap->bitmap = hbitmap_alloc(bitmap->size, bitmap->granularity);
>       if (name) {
>           bitmap->name = g_strdup(name);
>       }
> diff --git a/include/block/block.h b/include/block/block.h
> index 87fa48e..c5a3aa6 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -426,6 +426,10 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>   BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
>                                           const char *name);
>   void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
> +void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
> +BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
> +                                        const BdrvDirtyBitmap *bitmap,
> +                                        const char *name);
>   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
>   BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
>   int bdrv_dirty_bitmap_granularity(BlockDriverState *bs,

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

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

[Fam Zheng]

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

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

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

diff --git a/block.c b/block.c
index 39381cd..e574faa 100644
--- a/block.c
+++ b/block.c
@@ -56,6 +56,7 @@ struct BdrvDirtyBitmap {
     int64_t size;
     int granularity;
     char *name;
+    bool enabled;
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 
@@ -5331,6 +5332,7 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
     if (name) {
         bitmap->name = g_strdup(name);
     }
+    bitmap->enabled = true;
     QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
     return bitmap;
 }
@@ -5349,6 +5351,16 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
     }
 }
 
+void bdrv_disable_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+    bitmap->enabled = false;
+}
+
+void bdrv_enable_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
+{
+    bitmap->enabled = true;
+}
+
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
 {
     BdrvDirtyBitmap *bm;
@@ -5397,6 +5409,9 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
 {
     BdrvDirtyBitmap *bitmap;
     QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
+        if (!bitmap->enabled) {
+            continue;
+        }
         hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
     }
 }
diff --git a/blockdev.c b/blockdev.c
index 025ba3a..c7e98ad 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1858,6 +1858,48 @@ void qmp_block_dirty_bitmap_remove(const char *device, const char *name,
     bdrv_release_dirty_bitmap(bs, bitmap);
 }
 
+void qmp_block_dirty_bitmap_enable(const char *device, const char *name,
+                                   Error **errp)
+{
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    bitmap = bdrv_find_dirty_bitmap(bs, name);
+    if (!bitmap) {
+        error_setg(errp, "Dirty bitmap not found: %s", name);
+        return;
+    }
+
+    bdrv_enable_dirty_bitmap(bs, bitmap);
+}
+
+void qmp_block_dirty_bitmap_disable(const char *device, const char *name,
+                                    Error **errp)
+{
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    bitmap = bdrv_find_dirty_bitmap(bs, name);
+    if (!bitmap) {
+        error_setg(errp, "Dirty bitmap not found: %s", name);
+        return;
+    }
+
+    bdrv_disable_dirty_bitmap(bs, bitmap);
+}
+
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     const char *id = qdict_get_str(qdict, "id");
diff --git a/include/block/block.h b/include/block/block.h
index c5a3aa6..ef06cb6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -431,6 +431,8 @@ BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
                                         const BdrvDirtyBitmap *bitmap,
                                         const char *name);
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_disable_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
+void bdrv_enable_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
 BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
 int bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
                                   BdrvDirtyBitmap *bitmap);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a162f63..97c31bf 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -920,6 +920,34 @@
   'data': 'BlockDirtyBitmap' }
 
 ##
+# @block-dirty-bitmap-enable
+#
+# Enable a dirty bitmap on the device
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If @name is not found, GenericError with an explaining message
+#
+# Since 2.3
+##
+{'command': 'block-dirty-bitmap-enable',
+  'data': 'BlockDirtyBitmap' }
+
+##
+# @block-dirty-bitmap-disable
+#
+# Disable a dirty bitmap on the device
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If @name is not found, GenericError with an explaining message
+#
+# Since 2.3
+##
+{'command': 'block-dirty-bitmap-disable',
+  'data': 'BlockDirtyBitmap' }
+
+##
 # @block_set_io_throttle:
 #
 # Change I/O throttle limits for a block drive.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6f7e04c..6d0c944 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1211,6 +1211,16 @@ EQMP
         .args_type  = "device:B,name:s",
         .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_remove,
     },
+    {
+        .name       = "block-dirty-bitmap-enable",
+        .args_type  = "device:B,name:s",
+        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_enable,
+    },
+    {
+        .name       = "block-dirty-bitmap-disable",
+        .args_type  = "device:B,name:s",
+        .mhandler.cmd_new = qmp_marshal_input_block_dirty_bitmap_disable,
+    },
 
 SQMP
 
-- 
1.9.3

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

[Max Reitz]

On 2014-10-30 at 04:22, Fam Zheng wrote:
> This allows to put the dirty bitmap into a disabled state where no more
> writes will be tracked.
>
> It will be used before backup or writing to persistent file.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block.c               | 15 +++++++++++++++
>   blockdev.c            | 42 ++++++++++++++++++++++++++++++++++++++++++
>   include/block/block.h |  2 ++
>   qapi/block-core.json  | 28 ++++++++++++++++++++++++++++
>   qmp-commands.hx       | 10 ++++++++++
>   5 files changed, 97 insertions(+)

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

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

[Fam Zheng]

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

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

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

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

diff --git a/block/backup.c b/block/backup.c
index e334740..4870694 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -37,6 +37,10 @@ typedef struct CowRequest {
 typedef struct BackupBlockJob {
     BlockJob common;
     BlockDriverState *target;
+    /* bitmap for sync=dirty-bitmap */
+    BdrvDirtyBitmap *sync_bitmap;
+    /* dirty bitmap granularity */
+    int sync_bitmap_gran;
     MirrorSyncMode sync_mode;
     RateLimit limit;
     BlockdevOnError on_source_error;
@@ -263,7 +267,7 @@ static void coroutine_fn backup_run(void *opaque)
             job->common.busy = true;
         }
     } else {
-        /* Both FULL and TOP SYNC_MODE's require copying.. */
+        /* full, top and dirty_bitmap modes require copying.. */
         for (; start < end; start++) {
             bool error_is_read;
 
@@ -317,7 +321,21 @@ static void coroutine_fn backup_run(void *opaque)
                 if (alloced == 0) {
                     continue;
                 }
+            } else if (job->sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+                int i, dirty = 0;
+                for (i = 0; i < BACKUP_SECTORS_PER_CLUSTER;
+                     i += job->sync_bitmap_gran) {
+                    if (bdrv_get_dirty(bs, job->sync_bitmap,
+                            start * BACKUP_SECTORS_PER_CLUSTER + i)) {
+                        dirty = 1;
+                        break;
+                    }
+                }
+                if (!dirty) {
+                    continue;
+                }
             }
+
             /* FULL sync mode we copy the whole drive. */
             ret = backup_do_cow(bs, start * BACKUP_SECTORS_PER_CLUSTER,
                     BACKUP_SECTORS_PER_CLUSTER, &error_is_read);
@@ -341,6 +359,9 @@ static void coroutine_fn backup_run(void *opaque)
     qemu_co_rwlock_wrlock(&job->flush_rwlock);
     qemu_co_rwlock_unlock(&job->flush_rwlock);
 
+    if (job->sync_bitmap) {
+        bdrv_release_dirty_bitmap(bs, job->sync_bitmap);
+    }
     hbitmap_free(job->bitmap);
 
     bdrv_iostatus_disable(target);
@@ -351,12 +372,15 @@ static void coroutine_fn backup_run(void *opaque)
 
 void backup_start(BlockDriverState *bs, BlockDriverState *target,
                   int64_t speed, MirrorSyncMode sync_mode,
+                  BdrvDirtyBitmap *sync_bitmap,
+                  BitmapUseMode bitmap_mode,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   BlockCompletionFunc *cb, void *opaque,
                   Error **errp)
 {
     int64_t len;
+    BdrvDirtyBitmap *original;
 
     assert(bs);
     assert(target);
@@ -369,6 +393,28 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 
+    if (sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+        if (!sync_bitmap) {
+            error_setg(errp, "must provide a valid bitmap name for "
+                             "\"dirty-bitmap\" sync mode");
+            return;
+        }
+
+        switch (bitmap_mode) {
+        case BITMAP_USE_MODE_RESET:
+            original = sync_bitmap;
+            sync_bitmap = bdrv_copy_dirty_bitmap(bs, sync_bitmap, NULL);
+            bdrv_reset_dirty_bitmap(bs, original);
+            break;
+        case BITMAP_USE_MODE_CONSUME:
+            bdrv_dirty_bitmap_make_anon(bs, sync_bitmap);
+            break;
+        default:
+            assert(0);
+        }
+        bdrv_disable_dirty_bitmap(bs, sync_bitmap);
+    }
+
     len = bdrv_getlength(bs);
     if (len < 0) {
         error_setg_errno(errp, -len, "unable to get length for '%s'",
@@ -386,6 +432,12 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
     job->on_target_error = on_target_error;
     job->target = target;
     job->sync_mode = sync_mode;
+    job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP ?
+                       sync_bitmap : NULL;
+    if (sync_bitmap) {
+        job->sync_bitmap_gran =
+            bdrv_dirty_bitmap_granularity(bs, job->sync_bitmap);
+    }
     job->common.len = len;
     job->common.co = qemu_coroutine_create(backup_run);
     qemu_coroutine_enter(job->common.co, job);
diff --git a/block/mirror.c b/block/mirror.c
index cecd448..8dfd908 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -679,6 +679,10 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
     bool is_none_mode;
     BlockDriverState *base;
 
+    if (mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
+        error_setg(errp, "Sync mode 'dirty-bitmap' not supported");
+        return;
+    }
     is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
     base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL;
     mirror_start_job(bs, target, replaces,
diff --git a/blockdev.c b/blockdev.c
index c7e98ad..fca909b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1463,6 +1463,8 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
                      backup->sync,
                      backup->has_mode, backup->mode,
                      backup->has_speed, backup->speed,
+                     backup->has_bitmap, backup->bitmap,
+                     backup->has_bitmap_use_mode, backup->bitmap_use_mode,
                      backup->has_on_source_error, backup->on_source_error,
                      backup->has_on_target_error, backup->on_target_error,
                      &local_err);
@@ -2186,6 +2188,8 @@ void qmp_drive_backup(const char *device, const char *target,
                       enum MirrorSyncMode sync,
                       bool has_mode, enum NewImageMode mode,
                       bool has_speed, int64_t speed,
+                      bool has_bitmap, const char *bitmap,
+                      bool has_bitmap_use_mode, enum BitmapUseMode bitmap_mode,
                       bool has_on_source_error, BlockdevOnError on_source_error,
                       bool has_on_target_error, BlockdevOnError on_target_error,
                       Error **errp)
@@ -2282,7 +2286,10 @@ void qmp_drive_backup(const char *device, const char *target,
         return;
     }
 
-    backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
+    backup_start(bs, target_bs, speed, sync,
+                 has_bitmap ? bdrv_find_dirty_bitmap(bs, bitmap) : NULL,
+                 has_bitmap_use_mode ? bitmap_mode : BITMAP_USE_MODE_RESET,
+                 on_source_error, on_target_error,
                  block_job_cb, bs, &local_err);
     if (local_err != NULL) {
         bdrv_unref(target_bs);
diff --git a/hmp.c b/hmp.c
index 63d7686..bc5a2d2 100644
--- a/hmp.c
+++ b/hmp.c
@@ -966,7 +966,9 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
 
     qmp_drive_backup(device, filename, !!format, format,
                      full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
-                     true, mode, false, 0, false, 0, false, 0, &err);
+                     true, mode, false, 0, false, NULL,
+                     false, 0,
+                     false, 0, false, 0, &err);
     hmp_handle_error(mon, &err);
 }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8898c6c..0671c89 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -567,6 +567,8 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
  * @target: Block device to write to.
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @sync_mode: What parts of the disk image should be copied to the destination.
+ * @sync_bitmap: The name of dirty bitmap if sync_mode is
+ *               MIRROR_SYNC_MODE_DIRTY_BITMAP.
  * @on_source_error: The action to take upon error reading from the source.
  * @on_target_error: The action to take upon error writing to the target.
  * @cb: Completion function for the job.
@@ -577,6 +579,8 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
  */
 void backup_start(BlockDriverState *bs, BlockDriverState *target,
                   int64_t speed, MirrorSyncMode sync_mode,
+                  BdrvDirtyBitmap *sync_bitmap,
+                  BitmapUseMode bitmap_mode,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   BlockCompletionFunc *cb, void *opaque,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 97c31bf..e225220 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -473,7 +473,7 @@
 # Since: 1.3
 ##
 { 'enum': 'MirrorSyncMode',
-  'data': ['top', 'full', 'none'] }
+  'data': ['top', 'full', 'none', 'dirty-bitmap'] }
 
 ##
 # @BlockJobType:
@@ -634,6 +634,21 @@
             '*format': 'str', '*mode': 'NewImageMode' } }
 
 ##
+# @BitmapUseMode
+#
+# An enumeration that tells QEMU what operation to take when using a bitmap
+# in drive backup sync mode.
+#
+# @consume: QEMU should just consume the bitmap and release it after using
+#
+# @reset: QEMU should reset the dirty bitmap
+#
+# Since: 2.3
+##
+{ 'enum': 'BitmapUseMode',
+'data': [ 'consume', 'reset' ] }
+
+##
 # @DriveBackup
 #
 # @device: the name of the device which should be copied.
@@ -646,14 +661,20 @@
 #          probe if @mode is 'existing', else the format of the source
 #
 # @sync: what parts of the disk image should be copied to the destination
-#        (all the disk, only the sectors allocated in the topmost image, or
-#        only new I/O).
+#        (all the disk, only the sectors allocated in the topmost image, from a
+#        dirty bitmap, or only new I/O).
 #
 # @mode: #optional whether and how QEMU should create a new image, default is
 #        'absolute-paths'.
 #
 # @speed: #optional the maximum speed, in bytes per second
 #
+# @bitmap: #optional the name of dirty bitmap if sync is "dirty-bitmap"
+#          (Since 2.3)
+#
+# @bitmap-use-mode: #optional which operation to take when consuming @bitmap,
+#                   default is reset. (Since 2.1)
+#
 # @on-source-error: #optional the action to take on an error on the source,
 #                   default 'report'.  'stop' and 'enospc' can only be used
 #                   if the block device supports io-status (see BlockInfo).
@@ -671,7 +692,8 @@
 { 'type': 'DriveBackup',
   'data': { 'device': 'str', 'target': 'str', '*format': 'str',
             'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
-            '*speed': 'int',
+            '*speed': 'int', '*bitmap': 'str',
+            '*bitmap-use-mode': 'BitmapUseMode',
             '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError' } }
 
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6d0c944..75035e1 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1048,7 +1048,7 @@ EQMP
     {
         .name       = "drive-backup",
         .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
-                      "on-source-error:s?,on-target-error:s?",
+                      "bitmap:s?,on-source-error:s?,on-target-error:s?",
         .mhandler.cmd_new = qmp_marshal_input_drive_backup,
     },
 
@@ -1075,8 +1075,9 @@ Arguments:
             (json-string, optional)
 - "sync": what parts of the disk image should be copied to the destination;
   possibilities include "full" for all the disk, "top" for only the sectors
-  allocated in the topmost image, or "none" to only replicate new I/O
-  (MirrorSyncMode).
+  allocated in the topmost image, "dirty-bitmap" for only the dirty sectors in
+  the bitmap, or "none" to only replicate new I/O (MirrorSyncMode).
+- "bitmap": dirty bitmap name for sync==dirty-bitmap
 - "mode": whether and how QEMU should create a new image
           (NewImageMode, optional, default 'absolute-paths')
 - "speed": the maximum speed, in bytes per second (json-int, optional)
-- 
1.9.3

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

[Max Reitz]

On 2014-10-30 at 04:22, Fam Zheng wrote:
> For "dirty-bitmap" sync mode, the block job will iterate through the
> given dirty bitmap to decide if a sector needs backup (backup all the
> dirty clusters and skip clean ones), just as allocation conditions of
> "top" sync mode.
>
> There are two bitmap use modes for sync=dirty-bitmap:
>
>   - reset: backup job makes a copy of bitmap and resets the original
>     one.
>   - consume: backup job makes the original anonymous (invisible to user)
>     and releases it after use.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block/backup.c            | 54 ++++++++++++++++++++++++++++++++++++++++++++++-
>   block/mirror.c            |  4 ++++
>   blockdev.c                |  9 +++++++-
>   hmp.c                     |  4 +++-
>   include/block/block_int.h |  4 ++++
>   qapi/block-core.json      | 30 ++++++++++++++++++++++----
>   qmp-commands.hx           |  7 +++---
>   7 files changed, 102 insertions(+), 10 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index e334740..4870694 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -37,6 +37,10 @@ typedef struct CowRequest {
>   typedef struct BackupBlockJob {
>       BlockJob common;
>       BlockDriverState *target;
> +    /* bitmap for sync=dirty-bitmap */
> +    BdrvDirtyBitmap *sync_bitmap;
> +    /* dirty bitmap granularity */
> +    int sync_bitmap_gran;
>       MirrorSyncMode sync_mode;
>       RateLimit limit;
>       BlockdevOnError on_source_error;
> @@ -263,7 +267,7 @@ static void coroutine_fn backup_run(void *opaque)
>               job->common.busy = true;
>           }
>       } else {
> -        /* Both FULL and TOP SYNC_MODE's require copying.. */
> +        /* full, top and dirty_bitmap modes require copying.. */
>           for (; start < end; start++) {
>               bool error_is_read;
>   
> @@ -317,7 +321,21 @@ static void coroutine_fn backup_run(void *opaque)
>                   if (alloced == 0) {
>                       continue;
>                   }
> +            } else if (job->sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
> +                int i, dirty = 0;
> +                for (i = 0; i < BACKUP_SECTORS_PER_CLUSTER;
> +                     i += job->sync_bitmap_gran) {

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

> +                    if (bdrv_get_dirty(bs, job->sync_bitmap,
> +                            start * BACKUP_SECTORS_PER_CLUSTER + i)) {
> +                        dirty = 1;
> +                        break;
> +                    }
> +                }
> +                if (!dirty) {
> +                    continue;
> +                }
>               }
> +
>               /* FULL sync mode we copy the whole drive. */
>               ret = backup_do_cow(bs, start * BACKUP_SECTORS_PER_CLUSTER,
>                       BACKUP_SECTORS_PER_CLUSTER, &error_is_read);
> @@ -341,6 +359,9 @@ static void coroutine_fn backup_run(void *opaque)
>       qemu_co_rwlock_wrlock(&job->flush_rwlock);
>       qemu_co_rwlock_unlock(&job->flush_rwlock);
>   
> +    if (job->sync_bitmap) {
> +        bdrv_release_dirty_bitmap(bs, job->sync_bitmap);
> +    }
>       hbitmap_free(job->bitmap);
>   
>       bdrv_iostatus_disable(target);
> @@ -351,12 +372,15 @@ static void coroutine_fn backup_run(void *opaque)
>   
>   void backup_start(BlockDriverState *bs, BlockDriverState *target,
>                     int64_t speed, MirrorSyncMode sync_mode,
> +                  BdrvDirtyBitmap *sync_bitmap,
> +                  BitmapUseMode bitmap_mode,
>                     BlockdevOnError on_source_error,
>                     BlockdevOnError on_target_error,
>                     BlockCompletionFunc *cb, void *opaque,
>                     Error **errp)
>   {
>       int64_t len;
> +    BdrvDirtyBitmap *original;
>   
>       assert(bs);
>       assert(target);
> @@ -369,6 +393,28 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
>           return;
>       }
>   
> +    if (sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
> +        if (!sync_bitmap) {
> +            error_setg(errp, "must provide a valid bitmap name for "
> +                             "\"dirty-bitmap\" sync mode");
> +            return;
> +        }

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

> +
> +        switch (bitmap_mode) {
> +        case BITMAP_USE_MODE_RESET:
> +            original = sync_bitmap;
> +            sync_bitmap = bdrv_copy_dirty_bitmap(bs, sync_bitmap, NULL);
> +            bdrv_reset_dirty_bitmap(bs, original);
> +            break;
> +        case BITMAP_USE_MODE_CONSUME:
> +            bdrv_dirty_bitmap_make_anon(bs, sync_bitmap);
> +            break;
> +        default:
> +            assert(0);

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

> +        }
> +        bdrv_disable_dirty_bitmap(bs, sync_bitmap);
> +    }
> +
>       len = bdrv_getlength(bs);
>       if (len < 0) {
>           error_setg_errno(errp, -len, "unable to get length for '%s'",
> @@ -386,6 +432,12 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
>       job->on_target_error = on_target_error;
>       job->target = target;
>       job->sync_mode = sync_mode;
> +    job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP ?
> +                       sync_bitmap : NULL;
> +    if (sync_bitmap) {
> +        job->sync_bitmap_gran =
> +            bdrv_dirty_bitmap_granularity(bs, job->sync_bitmap);

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

> +    }
>       job->common.len = len;
>       job->common.co = qemu_coroutine_create(backup_run);
>       qemu_coroutine_enter(job->common.co, job);
> diff --git a/block/mirror.c b/block/mirror.c
> index cecd448..8dfd908 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -679,6 +679,10 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
>       bool is_none_mode;
>       BlockDriverState *base;
>   
> +    if (mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
> +        error_setg(errp, "Sync mode 'dirty-bitmap' not supported");
> +        return;
> +    }
>       is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
>       base = mode == MIRROR_SYNC_MODE_TOP ? bs->backing_hd : NULL;
>       mirror_start_job(bs, target, replaces,
> diff --git a/blockdev.c b/blockdev.c
> index c7e98ad..fca909b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1463,6 +1463,8 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
>                        backup->sync,
>                        backup->has_mode, backup->mode,
>                        backup->has_speed, backup->speed,
> +                     backup->has_bitmap, backup->bitmap,
> +                     backup->has_bitmap_use_mode, backup->bitmap_use_mode,
>                        backup->has_on_source_error, backup->on_source_error,
>                        backup->has_on_target_error, backup->on_target_error,
>                        &local_err);
> @@ -2186,6 +2188,8 @@ void qmp_drive_backup(const char *device, const char *target,
>                         enum MirrorSyncMode sync,
>                         bool has_mode, enum NewImageMode mode,
>                         bool has_speed, int64_t speed,
> +                      bool has_bitmap, const char *bitmap,
> +                      bool has_bitmap_use_mode, enum BitmapUseMode bitmap_mode,
>                         bool has_on_source_error, BlockdevOnError on_source_error,
>                         bool has_on_target_error, BlockdevOnError on_target_error,
>                         Error **errp)
> @@ -2282,7 +2286,10 @@ void qmp_drive_backup(const char *device, const char *target,
>           return;
>       }
>   

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

> -    backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error,
> +    backup_start(bs, target_bs, speed, sync,
> +                 has_bitmap ? bdrv_find_dirty_bitmap(bs, bitmap) : NULL,

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

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

> +                 has_bitmap_use_mode ? bitmap_mode : BITMAP_USE_MODE_RESET,
> +                 on_source_error, on_target_error,
>                    block_job_cb, bs, &local_err);
>       if (local_err != NULL) {
>           bdrv_unref(target_bs);
> diff --git a/hmp.c b/hmp.c
> index 63d7686..bc5a2d2 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -966,7 +966,9 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
>   
>       qmp_drive_backup(device, filename, !!format, format,
>                        full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
> -                     true, mode, false, 0, false, 0, false, 0, &err);
> +                     true, mode, false, 0, false, NULL,
> +                     false, 0,
> +                     false, 0, false, 0, &err);
>       hmp_handle_error(mon, &err);
>   }
>   
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 8898c6c..0671c89 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -567,6 +567,8 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
>    * @target: Block device to write to.
>    * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
>    * @sync_mode: What parts of the disk image should be copied to the destination.
> + * @sync_bitmap: The name of dirty bitmap if sync_mode is
> + *               MIRROR_SYNC_MODE_DIRTY_BITMAP.

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

>    * @on_source_error: The action to take upon error reading from the source.
>    * @on_target_error: The action to take upon error writing to the target.
>    * @cb: Completion function for the job.
> @@ -577,6 +579,8 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
>    */
>   void backup_start(BlockDriverState *bs, BlockDriverState *target,
>                     int64_t speed, MirrorSyncMode sync_mode,
> +                  BdrvDirtyBitmap *sync_bitmap,
> +                  BitmapUseMode bitmap_mode,
>                     BlockdevOnError on_source_error,
>                     BlockdevOnError on_target_error,
>                     BlockCompletionFunc *cb, void *opaque,
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 97c31bf..e225220 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -473,7 +473,7 @@
>   # Since: 1.3
>   ##
>   { 'enum': 'MirrorSyncMode',
> -  'data': ['top', 'full', 'none'] }
> +  'data': ['top', 'full', 'none', 'dirty-bitmap'] }
>   
>   ##
>   # @BlockJobType:
> @@ -634,6 +634,21 @@
>               '*format': 'str', '*mode': 'NewImageMode' } }
>   
>   ##
> +# @BitmapUseMode
> +#
> +# An enumeration that tells QEMU what operation to take when using a bitmap
> +# in drive backup sync mode.

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

> +#
> +# @consume: QEMU should just consume the bitmap and release it after using
> +#
> +# @reset: QEMU should reset the dirty bitmap
> +#
> +# Since: 2.3
> +##
> +{ 'enum': 'BitmapUseMode',
> +'data': [ 'consume', 'reset' ] }
> +
> +##
>   # @DriveBackup
>   #
>   # @device: the name of the device which should be copied.
> @@ -646,14 +661,20 @@
>   #          probe if @mode is 'existing', else the format of the source
>   #
>   # @sync: what parts of the disk image should be copied to the destination
> -#        (all the disk, only the sectors allocated in the topmost image, or
> -#        only new I/O).
> +#        (all the disk, only the sectors allocated in the topmost image, from a
> +#        dirty bitmap, or only new I/O).
>   #
>   # @mode: #optional whether and how QEMU should create a new image, default is
>   #        'absolute-paths'.
>   #
>   # @speed: #optional the maximum speed, in bytes per second
>   #
> +# @bitmap: #optional the name of dirty bitmap if sync is "dirty-bitmap"
> +#          (Since 2.3)
> +#
> +# @bitmap-use-mode: #optional which operation to take when consuming @bitmap,
> +#                   default is reset. (Since 2.1)

*2.3

Max

> +#
>   # @on-source-error: #optional the action to take on an error on the source,
>   #                   default 'report'.  'stop' and 'enospc' can only be used
>   #                   if the block device supports io-status (see BlockInfo).
> @@ -671,7 +692,8 @@
>   { 'type': 'DriveBackup',
>     'data': { 'device': 'str', 'target': 'str', '*format': 'str',
>               'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
> -            '*speed': 'int',
> +            '*speed': 'int', '*bitmap': 'str',
> +            '*bitmap-use-mode': 'BitmapUseMode',
>               '*on-source-error': 'BlockdevOnError',
>               '*on-target-error': 'BlockdevOnError' } }
>   
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6d0c944..75035e1 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1048,7 +1048,7 @@ EQMP
>       {
>           .name       = "drive-backup",
>           .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
> -                      "on-source-error:s?,on-target-error:s?",
> +                      "bitmap:s?,on-source-error:s?,on-target-error:s?",
>           .mhandler.cmd_new = qmp_marshal_input_drive_backup,
>       },
>   
> @@ -1075,8 +1075,9 @@ Arguments:
>               (json-string, optional)
>   - "sync": what parts of the disk image should be copied to the destination;
>     possibilities include "full" for all the disk, "top" for only the sectors
> -  allocated in the topmost image, or "none" to only replicate new I/O
> -  (MirrorSyncMode).
> +  allocated in the topmost image, "dirty-bitmap" for only the dirty sectors in
> +  the bitmap, or "none" to only replicate new I/O (MirrorSyncMode).
> +- "bitmap": dirty bitmap name for sync==dirty-bitmap
>   - "mode": whether and how QEMU should create a new image
>             (NewImageMode, optional, default 'absolute-paths')
>   - "speed": the maximum speed, in bytes per second (json-int, optional)

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

[Fam Zheng]

This adds three qmp commands to transactions.

Users can stop a dirty bitmap, start backup of it, and start another
dirty bitmap atomically, so that the dirty bitmap is tracked
incrementally and we don't miss any write.

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

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

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

[Max Reitz]

On 2014-10-30 at 04:22, Fam Zheng wrote:
> This adds three qmp commands to transactions.
>
> Users can stop a dirty bitmap, start backup of it, and start another
> dirty bitmap atomically, so that the dirty bitmap is tracked
> incrementally and we don't miss any write.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   blockdev.c       | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   qapi-schema.json |  5 ++-
>   2 files changed, 96 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index fca909b..34fa314 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1490,6 +1490,83 @@ static void drive_backup_abort(BlkTransactionState *common)
>       }
>   }
>   
> +static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
> +                                           Error **errp)
> +{
> +    BlockDirtyBitmapAdd *action;
> +
> +    action = common->action->block_dirty_bitmap_add;
> +    qmp_block_dirty_bitmap_add(action->device, action->name,
> +                               action->has_granularity, action->granularity,
> +                               errp);
> +}
> +
> +static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmapAdd *action;
> +    BdrvDirtyBitmap *bm;
> +    BlockDriverState *bs;
> +
> +    action = common->action->block_dirty_bitmap_add;
> +    bs = bdrv_find(action->device);
> +    if (bs) {
> +        bm = bdrv_find_dirty_bitmap(bs, action->name);
> +        if (bm) {
> +            bdrv_release_dirty_bitmap(bs, bm);
> +        }
> +    }
> +}
> +
> +static void block_dirty_bitmap_enable_prepare(BlkTransactionState *common,
> +                                               Error **errp)
> +{
> +    BlockDirtyBitmap *action;
> +
> +    action = common->action->block_dirty_bitmap_enable;
> +    qmp_block_dirty_bitmap_enable(action->device, action->name, errp);
> +}
> +
> +static void block_dirty_bitmap_enable_abort(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmap *action;
> +    BdrvDirtyBitmap *bitmap;
> +    BlockDriverState *bs;
> +
> +    action = common->action->block_dirty_bitmap_enable;
> +    bs = bdrv_find(action->device);
> +    if (bs) {
> +        bitmap = bdrv_find_dirty_bitmap(bs, action->name);
> +        if (bitmap) {
> +            bdrv_disable_dirty_bitmap(bs, bitmap);

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

> +        }
> +    }
> +}
> +
> +static void block_dirty_bitmap_disable_prepare(BlkTransactionState *common,
> +                                               Error **errp)
> +{
> +    BlockDirtyBitmap *action;
> +
> +    action = common->action->block_dirty_bitmap_disable;
> +    qmp_block_dirty_bitmap_disable(action->device, action->name, errp);
> +}
> +
> +static void block_dirty_bitmap_disable_abort(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmap *action;
> +    BdrvDirtyBitmap *bitmap;
> +    BlockDriverState *bs;
> +
> +    action = common->action->block_dirty_bitmap_disable;
> +    bs = bdrv_find(action->device);
> +    if (bs) {
> +        bitmap = bdrv_find_dirty_bitmap(bs, action->name);
> +        if (bitmap) {
> +            bdrv_enable_dirty_bitmap(bs, bitmap);

Same here, the other way round.

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

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

Max

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

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

[John Snow]

On 11/04/2014 06:03 AM, Max Reitz wrote:
> On 2014-10-30 at 04:22, Fam Zheng wrote:
>> This adds three qmp commands to transactions.
>>
>> Users can stop a dirty bitmap, start backup of it, and start another
>> dirty bitmap atomically, so that the dirty bitmap is tracked
>> incrementally and we don't miss any write.
>>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>   blockdev.c       | 92
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qapi-schema.json |  5 ++-
>>   2 files changed, 96 insertions(+), 1 deletion(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index fca909b..34fa314 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1490,6 +1490,83 @@ static void
>> drive_backup_abort(BlkTransactionState *common)
>>       }
>>   }
>> +static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
>> +                                           Error **errp)
>> +{
>> +    BlockDirtyBitmapAdd *action;
>> +
>> +    action = common->action->block_dirty_bitmap_add;
>> +    qmp_block_dirty_bitmap_add(action->device, action->name,
>> +                               action->has_granularity,
>> action->granularity,
>> +                               errp);
>> +}
>> +
>> +static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
>> +{
>> +    BlockDirtyBitmapAdd *action;
>> +    BdrvDirtyBitmap *bm;
>> +    BlockDriverState *bs;
>> +
>> +    action = common->action->block_dirty_bitmap_add;
>> +    bs = bdrv_find(action->device);
>> +    if (bs) {
>> +        bm = bdrv_find_dirty_bitmap(bs, action->name);
>> +        if (bm) {
>> +            bdrv_release_dirty_bitmap(bs, bm);
>> +        }
>> +    }
>> +}
>> +
>> +static void block_dirty_bitmap_enable_prepare(BlkTransactionState
>> *common,
>> +                                               Error **errp)
>> +{
>> +    BlockDirtyBitmap *action;
>> +
>> +    action = common->action->block_dirty_bitmap_enable;
>> +    qmp_block_dirty_bitmap_enable(action->device, action->name, errp);
>> +}
>> +
>> +static void block_dirty_bitmap_enable_abort(BlkTransactionState *common)
>> +{
>> +    BlockDirtyBitmap *action;
>> +    BdrvDirtyBitmap *bitmap;
>> +    BlockDriverState *bs;
>> +
>> +    action = common->action->block_dirty_bitmap_enable;
>> +    bs = bdrv_find(action->device);
>> +    if (bs) {
>> +        bitmap = bdrv_find_dirty_bitmap(bs, action->name);
>> +        if (bitmap) {
>> +            bdrv_disable_dirty_bitmap(bs, bitmap);
>
> But what if the dirty bitmap was enabled before so that this enabling
> transaction was supposed to be a no-op?

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

Unless you know something I don't.

Still, it does look goofy. Thoughts?

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

-- 
—js

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

[Max Reitz]

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

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

Max

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

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

[Paolo Bonzini]

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

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

Paolo

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

[Max Reitz]

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

Too easy. :-)

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

Max

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

[Paolo Bonzini]

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

Ah, right:

prepare:

+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    bitmap = bdrv_find_dirty_bitmap(bs, name);
+    if (!bitmap) {
+        error_setg(errp, "Dirty bitmap not found: %s", name);
+        return;
+    }

commit:

+    bdrv_enable_dirty_bitmap(bs, bitmap);

But qmp_block_dirty_bitmap_enable can reuse qmp_transaction instead! :)

Paolo

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

[Fam Zheng]

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

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

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

[Max Reitz]

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

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

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

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

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

[Fam Zheng]

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

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

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

[Max Reitz]

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

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

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

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

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

[Vladimir Sementsov-Ogievskiy]

> (2) File Format
>
> Some standard file magic, which includes:
>
> - Some magic byte(s)
> - Dirty flag. Needed to tell if we can trust this data or not.
> - The size of the bitmap
> - The granularity of the bitmap
> - The offset to the first sector of bitmap data (Maybe? It can't hurt 
> if we give ourselves a sector's worth to write metadata within.)
> - Data starting at... PAGESIZE? 
- The name of the bitmap and also the size of this name

>
> (5) Partial Persistence
>
> We did not discuss only saving higher levels of the bitmap. What's the 
> primary benefit you're seeking? 
Hmm. It may be used for faster sync. Maybe, save some of bitmap levels 
on timer while vm is running and save the last level on shutdown?

CC qemu-devel - ok.

Best regards,
Vladimir

On 18.11.2014 02:46, John Snow wrote:
>
>
> On 11/13/2014 08:54 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi
>>
>> I'd just like to start working on persistent dirty bitmap. My thoughts
>> about it are the following:
>> - qemu -drive file=file,dirty_bitmap=file
>>      so,  bitmap will be loaded with drive open and saved with drive 
>> close.
>> - save only meaningful (the last) level of the bitmap, restore all
>> levels on bitmap loading
>> - bool parameter "persistent" for bdrv_create_dirty_bitmap and
>> BdrvDirtyBitmap
>> - internal dirty_bitmaps, saved in qcow2 file
>>
>> Best regards,
>> Vladimir
>
> I am thinking:
>
> (1) Command Lines
>
> If you enable dirty bitmaps and give it a file that doesn't exist, it 
> should error out on you.
>
> If you enable dirty bitmaps and give it a file that's blank, it 
> understands that it is to create a persistent bitmap file in this 
> location and it should enable persistence.
>
> If a bitmap file is given and it has valid magic, this should imply 
> persistence.
>
> I am hesitant to have it auto-create files that don't already exist in 
> case the files become large in size and a misconfiguration leads to 
> repeated creation of these files that get orphaned in random folders. 
> Perhaps we can add a create=auto flag or similar to allow this 
> behavior if wanted.
>
> (2) File Format
>
> Some standard file magic, which includes:
>
> - Some magic byte(s)
> - Dirty flag. Needed to tell if we can trust this data or not.
> - The size of the bitmap
> - The granularity of the bitmap
> - The offset to the first sector of bitmap data (Maybe? It can't hurt 
> if we give ourselves a sector's worth to write metadata within.)
> - Data starting at... PAGESIZE?
>
> (3) Data Integrity
>
> The dirty flag could work something like:
>
> - If, on first open, the file has the dirty flag set, we need to 
> discard the bitmap data because we can no longer trust it.
> - If the bitmap file is clean, proceed as normal, but take a lock 
> against any of the bitmap functions to prevent them from marking any 
> bits dirty.
> - On first write to a clean persistent bitmap, delay the write until 
> we can mark the bitmap as dirty first. This incurs a write penalty 
> when we try to use the bitmap at first...
> - Unlock the bitmap functions and allow them to mark blocks as needed.
> - At some point, based on a sync policy, re-commit the dirty 
> information to the file and mark the file as clean once more and 
> re-take the persistence lock.
>
> (4) Synchronization Policy
>
> - Sync after so many bits become dirty in the bitmap, either as an 
> absolute threshold or a density percentage?
> - Sync periodically on a fixed timer?
> - Sync periodically opportunistically when I/O utilization becomes 
> relatively low? (With some sort of starvation prevention timer?)
> - Sync only at shutdown?
>
> In discussing with Stefan, I think we rather liked the idea of a timer 
> that tries to re-commit the block data during lulls in the I/O.
>
> (5) Partial Persistence
>
> We did not discuss only saving higher levels of the bitmap. What's the 
> primary benefit you're seeking?
>
> (6) Inclusion as qcow2 Metadata
>
> And lastly, we did discuss the inclusion of the bitmap as qcow2 
> metadata, but decided it wasn't our principle target for the format to 
> allow generality to other file formats. We didn't really discuss the 
> idea of having it as an option or an extension, but I don't (off the 
> top of my head) have any reasonings against it, but I will likely not 
> work on it myself.
>
>
> You didn't CC qemu-devel on this (so I won't!), but perhaps we should 
> re-send out our ideas to the wider list for feedback before we proceed 
> any further. Maybe we can split the work if we agree upon a design.
>
> Thanks!
> --js
>
> P.S.: I'm still cleaning up Fam's first patchset based on Max's and 
> your feedback. Hope to have it out by the end of this week.
>
>> On 11.11.2014 18:59, John Snow wrote:
>>>
>>>
>>> On 11/10/2014 03:15 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Hi Fam, hi Jorn.
>>>>
>>>> Jagane's project - http://wiki.qemu.org/Features/Livebackup
>>>>
>>>> In two words:
>>>> Normal delta - like in qemu, while backuping, we save all new 
>>>> writes to
>>>> separate virtual disk - delta. When backup is done, we can merge delta
>>>> back to original image.
>>>> Reverse delta - while backuping, we don't stop writing to original 
>>>> image
>>>> (and qemu works with it, not with delta), but before every write we 
>>>> copy
>>>> the corresponding block (if backup needs it) to separate virtual disk
>>>> (reverse delta). So, for backuping, if the current block is not
>>>> rewritten, we take it from original file, otherwise we take it from
>>>> reverse delta. The benefit is that, theoretically we'll have two times
>>>> less overhead I/Os then with "normal delta backup + merge", because of
>>>> part of (about 0.5) blocks which are rewritten while backuping will be
>>>> backuped before rewrite, and they will not go to delta then. Also, 
>>>> there
>>>> are should be methods to improve this coefficient, by choosing rules
>>>> about what blocks to backup first.
>>>>
>>>> Are someone work now on persistent (while vm is turned off) 
>>>> bitmaps? If
>>>> not, are there any recommendations?
>>>>
>>>> Best regards,
>>>> Vladimir
>>>>
>>>
>>> Hi Vladimir:
>>>
>>> I recently inherited Fam's backup series and I intended to start
>>> reviewing the comments this week. After I sift through that situation,
>>> the next step for me is persistent dirty bitmap support. I had been
>>> working on AHCI and IDE an awful lot lately, so it will take me a
>>> little bit to switch over.
>>>
>>> I don't have any existing notions of how I intend to implement it, so
>>> if you'd like to discuss any concerns you have about that subsystem,
>>> we can talk :)
>>>
>>> Thanks,
>>> --John
>>>
>>>> On 08.11.2014 10:19, Fam Zheng wrote:
>>>>> On Fri, 11/07 15:23, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Hi!
>>>>>>
>>>>>> Glad to see you working on backup series again. Some time ago I've
>>>>>> already
>>>>>> fixed v4 to be mergeable, but unfortunately didn't publish.
>>>>>> Actually I need (and I'm going to develop) incremental backup with
>>>>>> reverse
>>>>>> delta (like abandoned project
>>>>>> http://wiki.qemu.org/Features/Livebackup by
>>>>>> Jagane). And I've already fixed Jagane's project to be working with
>>>>>> current
>>>>>> Qemu. But the code is bad. Can you give me a peace of advice about
>>>>>> what
>>>>>> should I notice? I have a very superficial understanding of qemu 
>>>>>> block
>>>>>> devices, block jobs, etc.
>>>>> Hi Vladimir,
>>>>>
>>>>> Thank you for reviewing my series.
>>>>>
>>>>> I haven't seen Jagane's series, so could you pleased explain what
>>>>> "reverse
>>>>> delta" is?
>>>>>
>>>>> John Snow (CC'ed) will take my work on incremental backup so I think
>>>>> you can
>>>>> coordinate. I'll probably count on John to work on your comments too,
>>>>> in the
>>>>> future I'll follow the progress and review.
>>>>>
>>>>> Thanks,
>>>>> Fam
>>>>
>>>
>>
>

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

[Vladimir Sementsov-Ogievskiy]

> (3) Data Integrity
>
> The dirty flag could work something like:
>
> - If, on first open, the file has the dirty flag set, we need to 
> discard the bitmap data because we can no longer trust it.
> - If the bitmap file is clean, proceed as normal, but take a lock 
> against any of the bitmap functions to prevent them from marking any 
> bits dirty.
> - On first write to a clean persistent bitmap, delay the write until 
> we can mark the bitmap as dirty first. This incurs a write penalty 
> when we try to use the bitmap at first...
> - Unlock the bitmap functions and allow them to mark blocks as needed.
> - At some point, based on a sync policy, re-commit the dirty 
> information to the file and mark the file as clean once more and 
> re-take the persistence lock. 
Correct me if I'm wrong.

#Read bitmap:
read in blockdev_init, before any write to device, so no lock is needed.

#Set bits in bitmap:
if bitmap.dirty_flag:
    set bits
else:
    LOCK
    set bits
    set bitmap.dirty_flag
    set dirty_flag in bitmap file
    UNLOCK

#Sync:
if not bitmap.dirty_flag:
    skip sync
else:
    LOCK
    save one of bitmap levels (saving the last one is too long and not 
very good idea, because it is fast-updateing)
    unset dirty_flag in bitmap file
    unset bitmap.dirty_flag
    UNLOCK

#Last sync in bdrv_close:
Just save the last bitmap level and unset dirty_flag in bitmap file

Also.. I'm not quite sure about locking.. As I understand, co-routines 
in qemu are not running in parallel, is locking required? Or sync timer 
will not be co-routine based?

Best regards,
Vladimir

On 18.11.2014 13:54, Vladimir Sementsov-Ogievskiy wrote:
>
>> (2) File Format
>>
>> Some standard file magic, which includes:
>>
>> - Some magic byte(s)
>> - Dirty flag. Needed to tell if we can trust this data or not.
>> - The size of the bitmap
>> - The granularity of the bitmap
>> - The offset to the first sector of bitmap data (Maybe? It can't hurt 
>> if we give ourselves a sector's worth to write metadata within.)
>> - Data starting at... PAGESIZE? 
> - The name of the bitmap and also the size of this name
>
>>
>> (5) Partial Persistence
>>
>> We did not discuss only saving higher levels of the bitmap. What's 
>> the primary benefit you're seeking? 
> Hmm. It may be used for faster sync. Maybe, save some of bitmap levels 
> on timer while vm is running and save the last level on shutdown?
>
> CC qemu-devel - ok.
>
> Best regards,
> Vladimir
>
> On 18.11.2014 02:46, John Snow wrote:
>>
>>
>> On 11/13/2014 08:54 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi
>>>
>>> I'd just like to start working on persistent dirty bitmap. My thoughts
>>> about it are the following:
>>> - qemu -drive file=file,dirty_bitmap=file
>>>      so,  bitmap will be loaded with drive open and saved with drive 
>>> close.
>>> - save only meaningful (the last) level of the bitmap, restore all
>>> levels on bitmap loading
>>> - bool parameter "persistent" for bdrv_create_dirty_bitmap and
>>> BdrvDirtyBitmap
>>> - internal dirty_bitmaps, saved in qcow2 file
>>>
>>> Best regards,
>>> Vladimir
>>
>> I am thinking:
>>
>> (1) Command Lines
>>
>> If you enable dirty bitmaps and give it a file that doesn't exist, it 
>> should error out on you.
>>
>> If you enable dirty bitmaps and give it a file that's blank, it 
>> understands that it is to create a persistent bitmap file in this 
>> location and it should enable persistence.
>>
>> If a bitmap file is given and it has valid magic, this should imply 
>> persistence.
>>
>> I am hesitant to have it auto-create files that don't already exist 
>> in case the files become large in size and a misconfiguration leads 
>> to repeated creation of these files that get orphaned in random 
>> folders. Perhaps we can add a create=auto flag or similar to allow 
>> this behavior if wanted.
>>
>> (2) File Format
>>
>> Some standard file magic, which includes:
>>
>> - Some magic byte(s)
>> - Dirty flag. Needed to tell if we can trust this data or not.
>> - The size of the bitmap
>> - The granularity of the bitmap
>> - The offset to the first sector of bitmap data (Maybe? It can't hurt 
>> if we give ourselves a sector's worth to write metadata within.)
>> - Data starting at... PAGESIZE?
>>
>> (3) Data Integrity
>>
>> The dirty flag could work something like:
>>
>> - If, on first open, the file has the dirty flag set, we need to 
>> discard the bitmap data because we can no longer trust it.
>> - If the bitmap file is clean, proceed as normal, but take a lock 
>> against any of the bitmap functions to prevent them from marking any 
>> bits dirty.
>> - On first write to a clean persistent bitmap, delay the write until 
>> we can mark the bitmap as dirty first. This incurs a write penalty 
>> when we try to use the bitmap at first...
>> - Unlock the bitmap functions and allow them to mark blocks as needed.
>> - At some point, based on a sync policy, re-commit the dirty 
>> information to the file and mark the file as clean once more and 
>> re-take the persistence lock.
>>
>> (4) Synchronization Policy
>>
>> - Sync after so many bits become dirty in the bitmap, either as an 
>> absolute threshold or a density percentage?
>> - Sync periodically on a fixed timer?
>> - Sync periodically opportunistically when I/O utilization becomes 
>> relatively low? (With some sort of starvation prevention timer?)
>> - Sync only at shutdown?
>>
>> In discussing with Stefan, I think we rather liked the idea of a 
>> timer that tries to re-commit the block data during lulls in the I/O.
>>
>> (5) Partial Persistence
>>
>> We did not discuss only saving higher levels of the bitmap. What's 
>> the primary benefit you're seeking?
>>
>> (6) Inclusion as qcow2 Metadata
>>
>> And lastly, we did discuss the inclusion of the bitmap as qcow2 
>> metadata, but decided it wasn't our principle target for the format 
>> to allow generality to other file formats. We didn't really discuss 
>> the idea of having it as an option or an extension, but I don't (off 
>> the top of my head) have any reasonings against it, but I will likely 
>> not work on it myself.
>>
>>
>> You didn't CC qemu-devel on this (so I won't!), but perhaps we should 
>> re-send out our ideas to the wider list for feedback before we 
>> proceed any further. Maybe we can split the work if we agree upon a 
>> design.
>>
>> Thanks!
>> --js
>>
>> P.S.: I'm still cleaning up Fam's first patchset based on Max's and 
>> your feedback. Hope to have it out by the end of this week.
>>
>>> On 11.11.2014 18:59, John Snow wrote:
>>>>
>>>>
>>>> On 11/10/2014 03:15 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Hi Fam, hi Jorn.
>>>>>
>>>>> Jagane's project - http://wiki.qemu.org/Features/Livebackup
>>>>>
>>>>> In two words:
>>>>> Normal delta - like in qemu, while backuping, we save all new 
>>>>> writes to
>>>>> separate virtual disk - delta. When backup is done, we can merge 
>>>>> delta
>>>>> back to original image.
>>>>> Reverse delta - while backuping, we don't stop writing to original 
>>>>> image
>>>>> (and qemu works with it, not with delta), but before every write 
>>>>> we copy
>>>>> the corresponding block (if backup needs it) to separate virtual disk
>>>>> (reverse delta). So, for backuping, if the current block is not
>>>>> rewritten, we take it from original file, otherwise we take it from
>>>>> reverse delta. The benefit is that, theoretically we'll have two 
>>>>> times
>>>>> less overhead I/Os then with "normal delta backup + merge", 
>>>>> because of
>>>>> part of (about 0.5) blocks which are rewritten while backuping 
>>>>> will be
>>>>> backuped before rewrite, and they will not go to delta then. Also, 
>>>>> there
>>>>> are should be methods to improve this coefficient, by choosing rules
>>>>> about what blocks to backup first.
>>>>>
>>>>> Are someone work now on persistent (while vm is turned off) 
>>>>> bitmaps? If
>>>>> not, are there any recommendations?
>>>>>
>>>>> Best regards,
>>>>> Vladimir
>>>>>
>>>>
>>>> Hi Vladimir:
>>>>
>>>> I recently inherited Fam's backup series and I intended to start
>>>> reviewing the comments this week. After I sift through that situation,
>>>> the next step for me is persistent dirty bitmap support. I had been
>>>> working on AHCI and IDE an awful lot lately, so it will take me a
>>>> little bit to switch over.
>>>>
>>>> I don't have any existing notions of how I intend to implement it, so
>>>> if you'd like to discuss any concerns you have about that subsystem,
>>>> we can talk :)
>>>>
>>>> Thanks,
>>>> --John
>>>>
>>>>> On 08.11.2014 10:19, Fam Zheng wrote:
>>>>>> On Fri, 11/07 15:23, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>> Hi!
>>>>>>>
>>>>>>> Glad to see you working on backup series again. Some time ago I've
>>>>>>> already
>>>>>>> fixed v4 to be mergeable, but unfortunately didn't publish.
>>>>>>> Actually I need (and I'm going to develop) incremental backup with
>>>>>>> reverse
>>>>>>> delta (like abandoned project
>>>>>>> http://wiki.qemu.org/Features/Livebackup by
>>>>>>> Jagane). And I've already fixed Jagane's project to be working with
>>>>>>> current
>>>>>>> Qemu. But the code is bad. Can you give me a peace of advice about
>>>>>>> what
>>>>>>> should I notice? I have a very superficial understanding of qemu 
>>>>>>> block
>>>>>>> devices, block jobs, etc.
>>>>>> Hi Vladimir,
>>>>>>
>>>>>> Thank you for reviewing my series.
>>>>>>
>>>>>> I haven't seen Jagane's series, so could you pleased explain what
>>>>>> "reverse
>>>>>> delta" is?
>>>>>>
>>>>>> John Snow (CC'ed) will take my work on incremental backup so I think
>>>>>> you can
>>>>>> coordinate. I'll probably count on John to work on your comments 
>>>>>> too,
>>>>>> in the
>>>>>> future I'll follow the progress and review.
>>>>>>
>>>>>> Thanks,
>>>>>> Fam
>>>>>
>>>>
>>>
>>
>
>

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

[Vladimir Sementsov-Ogievskiy]

Also, if we sync not the last level, bitmap.dirty_flag should be related 
to syncing level, not to the whole bitmap, to reduce sync overhead.
Or, if we implement difficult sync policy, there should be dirty flags 
for each bitmap level. Despite this, only one level is needed to be 
saved in the bitmap file.

PS: more ideas about file format - thanks to Denis V. Lunev 
<den@parallels.com>
1) Shouldn't we consider a possibility of storing several bitmaps in one 
file? Or one bitmap = one file?
2) Implement header extensions like in qcow2.

Best regards,
Vladimir

On 18.11.2014 16:09, Vladimir Sementsov-Ogievskiy wrote:
>> (3) Data Integrity
>>
>> The dirty flag could work something like:
>>
>> - If, on first open, the file has the dirty flag set, we need to 
>> discard the bitmap data because we can no longer trust it.
>> - If the bitmap file is clean, proceed as normal, but take a lock 
>> against any of the bitmap functions to prevent them from marking any 
>> bits dirty.
>> - On first write to a clean persistent bitmap, delay the write until 
>> we can mark the bitmap as dirty first. This incurs a write penalty 
>> when we try to use the bitmap at first...
>> - Unlock the bitmap functions and allow them to mark blocks as needed.
>> - At some point, based on a sync policy, re-commit the dirty 
>> information to the file and mark the file as clean once more and 
>> re-take the persistence lock. 
> Correct me if I'm wrong.
>
> #Read bitmap:
> read in blockdev_init, before any write to device, so no lock is needed.
>
> #Set bits in bitmap:
> if bitmap.dirty_flag:
>    set bits
> else:
>    LOCK
>    set bits
>    set bitmap.dirty_flag
>    set dirty_flag in bitmap file
>    UNLOCK
>
> #Sync:
> if not bitmap.dirty_flag:
>    skip sync
> else:
>    LOCK
>    save one of bitmap levels (saving the last one is too long and not 
> very good idea, because it is fast-updateing)
>    unset dirty_flag in bitmap file
>    unset bitmap.dirty_flag
>    UNLOCK
>
> #Last sync in bdrv_close:
> Just save the last bitmap level and unset dirty_flag in bitmap file
>
> Also.. I'm not quite sure about locking.. As I understand, co-routines 
> in qemu are not running in parallel, is locking required? Or sync 
> timer will not be co-routine based?
>
> Best regards,
> Vladimir
>
> On 18.11.2014 13:54, Vladimir Sementsov-Ogievskiy wrote:
>>
>>> (2) File Format
>>>
>>> Some standard file magic, which includes:
>>>
>>> - Some magic byte(s)
>>> - Dirty flag. Needed to tell if we can trust this data or not.
>>> - The size of the bitmap
>>> - The granularity of the bitmap
>>> - The offset to the first sector of bitmap data (Maybe? It can't 
>>> hurt if we give ourselves a sector's worth to write metadata within.)
>>> - Data starting at... PAGESIZE? 
>> - The name of the bitmap and also the size of this name
>>
>>>
>>> (5) Partial Persistence
>>>
>>> We did not discuss only saving higher levels of the bitmap. What's 
>>> the primary benefit you're seeking? 
>> Hmm. It may be used for faster sync. Maybe, save some of bitmap 
>> levels on timer while vm is running and save the last level on shutdown?
>>
>> CC qemu-devel - ok.
>>
>> Best regards,
>> Vladimir
>>
>> On 18.11.2014 02:46, John Snow wrote:
>>>
>>>
>>> On 11/13/2014 08:54 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Hi
>>>>
>>>> I'd just like to start working on persistent dirty bitmap. My thoughts
>>>> about it are the following:
>>>> - qemu -drive file=file,dirty_bitmap=file
>>>>      so,  bitmap will be loaded with drive open and saved with 
>>>> drive close.
>>>> - save only meaningful (the last) level of the bitmap, restore all
>>>> levels on bitmap loading
>>>> - bool parameter "persistent" for bdrv_create_dirty_bitmap and
>>>> BdrvDirtyBitmap
>>>> - internal dirty_bitmaps, saved in qcow2 file
>>>>
>>>> Best regards,
>>>> Vladimir
>>>
>>> I am thinking:
>>>
>>> (1) Command Lines
>>>
>>> If you enable dirty bitmaps and give it a file that doesn't exist, 
>>> it should error out on you.
>>>
>>> If you enable dirty bitmaps and give it a file that's blank, it 
>>> understands that it is to create a persistent bitmap file in this 
>>> location and it should enable persistence.
>>>
>>> If a bitmap file is given and it has valid magic, this should imply 
>>> persistence.
>>>
>>> I am hesitant to have it auto-create files that don't already exist 
>>> in case the files become large in size and a misconfiguration leads 
>>> to repeated creation of these files that get orphaned in random 
>>> folders. Perhaps we can add a create=auto flag or similar to allow 
>>> this behavior if wanted.
>>>
>>> (2) File Format
>>>
>>> Some standard file magic, which includes:
>>>
>>> - Some magic byte(s)
>>> - Dirty flag. Needed to tell if we can trust this data or not.
>>> - The size of the bitmap
>>> - The granularity of the bitmap
>>> - The offset to the first sector of bitmap data (Maybe? It can't 
>>> hurt if we give ourselves a sector's worth to write metadata within.)
>>> - Data starting at... PAGESIZE?
>>>
>>> (3) Data Integrity
>>>
>>> The dirty flag could work something like:
>>>
>>> - If, on first open, the file has the dirty flag set, we need to 
>>> discard the bitmap data because we can no longer trust it.
>>> - If the bitmap file is clean, proceed as normal, but take a lock 
>>> against any of the bitmap functions to prevent them from marking any 
>>> bits dirty.
>>> - On first write to a clean persistent bitmap, delay the write until 
>>> we can mark the bitmap as dirty first. This incurs a write penalty 
>>> when we try to use the bitmap at first...
>>> - Unlock the bitmap functions and allow them to mark blocks as needed.
>>> - At some point, based on a sync policy, re-commit the dirty 
>>> information to the file and mark the file as clean once more and 
>>> re-take the persistence lock.
>>>
>>> (4) Synchronization Policy
>>>
>>> - Sync after so many bits become dirty in the bitmap, either as an 
>>> absolute threshold or a density percentage?
>>> - Sync periodically on a fixed timer?
>>> - Sync periodically opportunistically when I/O utilization becomes 
>>> relatively low? (With some sort of starvation prevention timer?)
>>> - Sync only at shutdown?
>>>
>>> In discussing with Stefan, I think we rather liked the idea of a 
>>> timer that tries to re-commit the block data during lulls in the I/O.
>>>
>>> (5) Partial Persistence
>>>
>>> We did not discuss only saving higher levels of the bitmap. What's 
>>> the primary benefit you're seeking?
>>>
>>> (6) Inclusion as qcow2 Metadata
>>>
>>> And lastly, we did discuss the inclusion of the bitmap as qcow2 
>>> metadata, but decided it wasn't our principle target for the format 
>>> to allow generality to other file formats. We didn't really discuss 
>>> the idea of having it as an option or an extension, but I don't (off 
>>> the top of my head) have any reasonings against it, but I will 
>>> likely not work on it myself.
>>>
>>>
>>> You didn't CC qemu-devel on this (so I won't!), but perhaps we 
>>> should re-send out our ideas to the wider list for feedback before 
>>> we proceed any further. Maybe we can split the work if we agree upon 
>>> a design.
>>>
>>> Thanks!
>>> --js
>>>
>>> P.S.: I'm still cleaning up Fam's first patchset based on Max's and 
>>> your feedback. Hope to have it out by the end of this week.
>>>
>>>> On 11.11.2014 18:59, John Snow wrote:
>>>>>
>>>>>
>>>>> On 11/10/2014 03:15 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Hi Fam, hi Jorn.
>>>>>>
>>>>>> Jagane's project - http://wiki.qemu.org/Features/Livebackup
>>>>>>
>>>>>> In two words:
>>>>>> Normal delta - like in qemu, while backuping, we save all new 
>>>>>> writes to
>>>>>> separate virtual disk - delta. When backup is done, we can merge 
>>>>>> delta
>>>>>> back to original image.
>>>>>> Reverse delta - while backuping, we don't stop writing to 
>>>>>> original image
>>>>>> (and qemu works with it, not with delta), but before every write 
>>>>>> we copy
>>>>>> the corresponding block (if backup needs it) to separate virtual 
>>>>>> disk
>>>>>> (reverse delta). So, for backuping, if the current block is not
>>>>>> rewritten, we take it from original file, otherwise we take it from
>>>>>> reverse delta. The benefit is that, theoretically we'll have two 
>>>>>> times
>>>>>> less overhead I/Os then with "normal delta backup + merge", 
>>>>>> because of
>>>>>> part of (about 0.5) blocks which are rewritten while backuping 
>>>>>> will be
>>>>>> backuped before rewrite, and they will not go to delta then. 
>>>>>> Also, there
>>>>>> are should be methods to improve this coefficient, by choosing rules
>>>>>> about what blocks to backup first.
>>>>>>
>>>>>> Are someone work now on persistent (while vm is turned off) 
>>>>>> bitmaps? If
>>>>>> not, are there any recommendations?
>>>>>>
>>>>>> Best regards,
>>>>>> Vladimir
>>>>>>
>>>>>
>>>>> Hi Vladimir:
>>>>>
>>>>> I recently inherited Fam's backup series and I intended to start
>>>>> reviewing the comments this week. After I sift through that 
>>>>> situation,
>>>>> the next step for me is persistent dirty bitmap support. I had been
>>>>> working on AHCI and IDE an awful lot lately, so it will take me a
>>>>> little bit to switch over.
>>>>>
>>>>> I don't have any existing notions of how I intend to implement it, so
>>>>> if you'd like to discuss any concerns you have about that subsystem,
>>>>> we can talk :)
>>>>>
>>>>> Thanks,
>>>>> --John
>>>>>
>>>>>> On 08.11.2014 10:19, Fam Zheng wrote:
>>>>>>> On Fri, 11/07 15:23, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>> Hi!
>>>>>>>>
>>>>>>>> Glad to see you working on backup series again. Some time ago I've
>>>>>>>> already
>>>>>>>> fixed v4 to be mergeable, but unfortunately didn't publish.
>>>>>>>> Actually I need (and I'm going to develop) incremental backup with
>>>>>>>> reverse
>>>>>>>> delta (like abandoned project
>>>>>>>> http://wiki.qemu.org/Features/Livebackup by
>>>>>>>> Jagane). And I've already fixed Jagane's project to be working 
>>>>>>>> with
>>>>>>>> current
>>>>>>>> Qemu. But the code is bad. Can you give me a peace of advice about
>>>>>>>> what
>>>>>>>> should I notice? I have a very superficial understanding of 
>>>>>>>> qemu block
>>>>>>>> devices, block jobs, etc.
>>>>>>> Hi Vladimir,
>>>>>>>
>>>>>>> Thank you for reviewing my series.
>>>>>>>
>>>>>>> I haven't seen Jagane's series, so could you pleased explain what
>>>>>>> "reverse
>>>>>>> delta" is?
>>>>>>>
>>>>>>> John Snow (CC'ed) will take my work on incremental backup so I 
>>>>>>> think
>>>>>>> you can
>>>>>>> coordinate. I'll probably count on John to work on your comments 
>>>>>>> too,
>>>>>>> in the
>>>>>>> future I'll follow the progress and review.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Fam
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>
>

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

[John Snow]

On 11/18/2014 08:09 AM, Vladimir Sementsov-Ogievskiy wrote:
>> (3) Data Integrity
>>
>> The dirty flag could work something like:
>>
>> - If, on first open, the file has the dirty flag set, we need to
>> discard the bitmap data because we can no longer trust it.
>> - If the bitmap file is clean, proceed as normal, but take a lock
>> against any of the bitmap functions to prevent them from marking any
>> bits dirty.
>> - On first write to a clean persistent bitmap, delay the write until
>> we can mark the bitmap as dirty first. This incurs a write penalty
>> when we try to use the bitmap at first...
>> - Unlock the bitmap functions and allow them to mark blocks as needed.
>> - At some point, based on a sync policy, re-commit the dirty
>> information to the file and mark the file as clean once more and
>> re-take the persistence lock.
> Correct me if I'm wrong.
>
> #Read bitmap:
> read in blockdev_init, before any write to device, so no lock is needed.
>
> #Set bits in bitmap:
> if bitmap.dirty_flag:
>     set bits
> else:
>     LOCK
>     set bits
>     set bitmap.dirty_flag
>     set dirty_flag in bitmap file
>     UNLOCK
>
> #Sync:
> if not bitmap.dirty_flag:
>     skip sync
> else:
>     LOCK
>     save one of bitmap levels (saving the last one is too long and not
> very good idea, because it is fast-updateing)
>     unset dirty_flag in bitmap file
>     unset bitmap.dirty_flag
>     UNLOCK
>
> #Last sync in bdrv_close:
> Just save the last bitmap level and unset dirty_flag in bitmap file
>
> Also.. I'm not quite sure about locking.. As I understand, co-routines
> in qemu are not running in parallel, is locking required? Or sync timer
> will not be co-routine based?
>
> Best regards,
> Vladimir

Might be being too informal. I just meant a lock or barrier to prevent 
actual IO throughput until we can confirm the dirty flag has been 
adjusted to indicate that the persistent bitmap is now officially out of 
date. Nothing fancy.

Wasn't trying to imply that we needed threading protection, just 
"locking" the IO until we can configure the bitmap as we need it to be.

> On 18.11.2014 13:54, Vladimir Sementsov-Ogievskiy wrote:
>>
>>> (2) File Format
>>>
>>> Some standard file magic, which includes:
>>>
>>> - Some magic byte(s)
>>> - Dirty flag. Needed to tell if we can trust this data or not.
>>> - The size of the bitmap
>>> - The granularity of the bitmap
>>> - The offset to the first sector of bitmap data (Maybe? It can't hurt
>>> if we give ourselves a sector's worth to write metadata within.)
>>> - Data starting at... PAGESIZE?
>> - The name of the bitmap and also the size of this name
>>
>>>
>>> (5) Partial Persistence
>>>
>>> We did not discuss only saving higher levels of the bitmap. What's
>>> the primary benefit you're seeking?
>> Hmm. It may be used for faster sync. Maybe, save some of bitmap levels
>> on timer while vm is running and save the last level on shutdown?
>>
>> CC qemu-devel - ok.
>>
>> Best regards,
>> Vladimir
>>
>> On 18.11.2014 02:46, John Snow wrote:
>>>
>>>
>>> On 11/13/2014 08:54 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Hi
>>>>
>>>> I'd just like to start working on persistent dirty bitmap. My thoughts
>>>> about it are the following:
>>>> - qemu -drive file=file,dirty_bitmap=file
>>>>      so,  bitmap will be loaded with drive open and saved with drive
>>>> close.
>>>> - save only meaningful (the last) level of the bitmap, restore all
>>>> levels on bitmap loading
>>>> - bool parameter "persistent" for bdrv_create_dirty_bitmap and
>>>> BdrvDirtyBitmap
>>>> - internal dirty_bitmaps, saved in qcow2 file
>>>>
>>>> Best regards,
>>>> Vladimir
>>>
>>> I am thinking:
>>>
>>> (1) Command Lines
>>>
>>> If you enable dirty bitmaps and give it a file that doesn't exist, it
>>> should error out on you.
>>>
>>> If you enable dirty bitmaps and give it a file that's blank, it
>>> understands that it is to create a persistent bitmap file in this
>>> location and it should enable persistence.
>>>
>>> If a bitmap file is given and it has valid magic, this should imply
>>> persistence.
>>>
>>> I am hesitant to have it auto-create files that don't already exist
>>> in case the files become large in size and a misconfiguration leads
>>> to repeated creation of these files that get orphaned in random
>>> folders. Perhaps we can add a create=auto flag or similar to allow
>>> this behavior if wanted.
>>>
>>> (2) File Format
>>>
>>> Some standard file magic, which includes:
>>>
>>> - Some magic byte(s)
>>> - Dirty flag. Needed to tell if we can trust this data or not.
>>> - The size of the bitmap
>>> - The granularity of the bitmap
>>> - The offset to the first sector of bitmap data (Maybe? It can't hurt
>>> if we give ourselves a sector's worth to write metadata within.)
>>> - Data starting at... PAGESIZE?
>>>
>>> (3) Data Integrity
>>>
>>> The dirty flag could work something like:
>>>
>>> - If, on first open, the file has the dirty flag set, we need to
>>> discard the bitmap data because we can no longer trust it.
>>> - If the bitmap file is clean, proceed as normal, but take a lock
>>> against any of the bitmap functions to prevent them from marking any
>>> bits dirty.
>>> - On first write to a clean persistent bitmap, delay the write until
>>> we can mark the bitmap as dirty first. This incurs a write penalty
>>> when we try to use the bitmap at first...
>>> - Unlock the bitmap functions and allow them to mark blocks as needed.
>>> - At some point, based on a sync policy, re-commit the dirty
>>> information to the file and mark the file as clean once more and
>>> re-take the persistence lock.
>>>
>>> (4) Synchronization Policy
>>>
>>> - Sync after so many bits become dirty in the bitmap, either as an
>>> absolute threshold or a density percentage?
>>> - Sync periodically on a fixed timer?
>>> - Sync periodically opportunistically when I/O utilization becomes
>>> relatively low? (With some sort of starvation prevention timer?)
>>> - Sync only at shutdown?
>>>
>>> In discussing with Stefan, I think we rather liked the idea of a
>>> timer that tries to re-commit the block data during lulls in the I/O.
>>>
>>> (5) Partial Persistence
>>>
>>> We did not discuss only saving higher levels of the bitmap. What's
>>> the primary benefit you're seeking?
>>>
>>> (6) Inclusion as qcow2 Metadata
>>>
>>> And lastly, we did discuss the inclusion of the bitmap as qcow2
>>> metadata, but decided it wasn't our principle target for the format
>>> to allow generality to other file formats. We didn't really discuss
>>> the idea of having it as an option or an extension, but I don't (off
>>> the top of my head) have any reasonings against it, but I will likely
>>> not work on it myself.
>>>
>>>
>>> You didn't CC qemu-devel on this (so I won't!), but perhaps we should
>>> re-send out our ideas to the wider list for feedback before we
>>> proceed any further. Maybe we can split the work if we agree upon a
>>> design.
>>>
>>> Thanks!
>>> --js
>>>
>>> P.S.: I'm still cleaning up Fam's first patchset based on Max's and
>>> your feedback. Hope to have it out by the end of this week.
>>>
>>>> On 11.11.2014 18:59, John Snow wrote:
>>>>>
>>>>>
>>>>> On 11/10/2014 03:15 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Hi Fam, hi Jorn.
>>>>>>
>>>>>> Jagane's project - http://wiki.qemu.org/Features/Livebackup
>>>>>>
>>>>>> In two words:
>>>>>> Normal delta - like in qemu, while backuping, we save all new
>>>>>> writes to
>>>>>> separate virtual disk - delta. When backup is done, we can merge
>>>>>> delta
>>>>>> back to original image.
>>>>>> Reverse delta - while backuping, we don't stop writing to original
>>>>>> image
>>>>>> (and qemu works with it, not with delta), but before every write
>>>>>> we copy
>>>>>> the corresponding block (if backup needs it) to separate virtual disk
>>>>>> (reverse delta). So, for backuping, if the current block is not
>>>>>> rewritten, we take it from original file, otherwise we take it from
>>>>>> reverse delta. The benefit is that, theoretically we'll have two
>>>>>> times
>>>>>> less overhead I/Os then with "normal delta backup + merge",
>>>>>> because of
>>>>>> part of (about 0.5) blocks which are rewritten while backuping
>>>>>> will be
>>>>>> backuped before rewrite, and they will not go to delta then. Also,
>>>>>> there
>>>>>> are should be methods to improve this coefficient, by choosing rules
>>>>>> about what blocks to backup first.
>>>>>>
>>>>>> Are someone work now on persistent (while vm is turned off)
>>>>>> bitmaps? If
>>>>>> not, are there any recommendations?
>>>>>>
>>>>>> Best regards,
>>>>>> Vladimir
>>>>>>
>>>>>
>>>>> Hi Vladimir:
>>>>>
>>>>> I recently inherited Fam's backup series and I intended to start
>>>>> reviewing the comments this week. After I sift through that situation,
>>>>> the next step for me is persistent dirty bitmap support. I had been
>>>>> working on AHCI and IDE an awful lot lately, so it will take me a
>>>>> little bit to switch over.
>>>>>
>>>>> I don't have any existing notions of how I intend to implement it, so
>>>>> if you'd like to discuss any concerns you have about that subsystem,
>>>>> we can talk :)
>>>>>
>>>>> Thanks,
>>>>> --John
>>>>>
>>>>>> On 08.11.2014 10:19, Fam Zheng wrote:
>>>>>>> On Fri, 11/07 15:23, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>> Hi!
>>>>>>>>
>>>>>>>> Glad to see you working on backup series again. Some time ago I've
>>>>>>>> already
>>>>>>>> fixed v4 to be mergeable, but unfortunately didn't publish.
>>>>>>>> Actually I need (and I'm going to develop) incremental backup with
>>>>>>>> reverse
>>>>>>>> delta (like abandoned project
>>>>>>>> http://wiki.qemu.org/Features/Livebackup by
>>>>>>>> Jagane). And I've already fixed Jagane's project to be working with
>>>>>>>> current
>>>>>>>> Qemu. But the code is bad. Can you give me a peace of advice about
>>>>>>>> what
>>>>>>>> should I notice? I have a very superficial understanding of qemu
>>>>>>>> block
>>>>>>>> devices, block jobs, etc.
>>>>>>> Hi Vladimir,
>>>>>>>
>>>>>>> Thank you for reviewing my series.
>>>>>>>
>>>>>>> I haven't seen Jagane's series, so could you pleased explain what
>>>>>>> "reverse
>>>>>>> delta" is?
>>>>>>>
>>>>>>> John Snow (CC'ed) will take my work on incremental backup so I think
>>>>>>> you can
>>>>>>> coordinate. I'll probably count on John to work on your comments
>>>>>>> too,
>>>>>>> in the
>>>>>>> future I'll follow the progress and review.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Fam
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>

-- 
—js

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

[Denis V. Lunev]

On 18/11/14 19:08, John Snow wrote:
>
>
> On 11/18/2014 08:09 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> (3) Data Integrity
>>>
>>> The dirty flag could work something like:
>>>
>>> - If, on first open, the file has the dirty flag set, we need to
>>> discard the bitmap data because we can no longer trust it.
>>> - If the bitmap file is clean, proceed as normal, but take a lock
>>> against any of the bitmap functions to prevent them from marking any
>>> bits dirty.
>>> - On first write to a clean persistent bitmap, delay the write until
>>> we can mark the bitmap as dirty first. This incurs a write penalty
>>> when we try to use the bitmap at first...
>>> - Unlock the bitmap functions and allow them to mark blocks as needed.
>>> - At some point, based on a sync policy, re-commit the dirty
>>> information to the file and mark the file as clean once more and
>>> re-take the persistence lock.
>> Correct me if I'm wrong.
>>

- first of all what we are protecting against? Any QEMU or kernel
crash leads to the disaster. You can not guarantee at all the
consistency between data written to the main file (disk) and data
written to bitmap except if you wait that dirty bitmap is really
updated. In this case you will have performance halved in comparison
with the host (each guest write means 2 IOPS instead of 1)

- this effectively means that we can not be protected against crash

- if we don't we could not care at all about bitmap updates when
QEMU is running. We should write it only on stop/suspend/etc.

This simplifies things a lot.

So, the procedure is simple
- stop main VM by using vm_pause
- open/create bitmap file
- set dirty
- unpause

That's all. If QEMU is running, bitmap is always dirty. The only 
exception is backup creation. New backup means that old bitmap is
synced with dirty clear, new one is created with dirty set and
backup starts working with old bitmap.

Once again. In all other cases we can not guarantee that we will
report _all_ changed blocks to backup software if crash happens
in the middle. One missed block in the bitmap and the entire backup
is blown up.

This also means that (4) aka sync policy is simple. Do this on
close.

>> #Read bitmap:
>> read in blockdev_init, before any write to device, so no lock is needed.
>>
>> #Set bits in bitmap:
>> if bitmap.dirty_flag:
>>     set bits
>> else:
>>     LOCK
>>     set bits
>>     set bitmap.dirty_flag
>>     set dirty_flag in bitmap file
>>     UNLOCK
>>
>> #Sync:
>> if not bitmap.dirty_flag:
>>     skip sync
>> else:
>>     LOCK
>>     save one of bitmap levels (saving the last one is too long and not
>> very good idea, because it is fast-updateing)
>>     unset dirty_flag in bitmap file
>>     unset bitmap.dirty_flag
>>     UNLOCK
>>
>> #Last sync in bdrv_close:
>> Just save the last bitmap level and unset dirty_flag in bitmap file
>>
>> Also.. I'm not quite sure about locking.. As I understand, co-routines
>> in qemu are not running in parallel, is locking required? Or sync timer
>> will not be co-routine based?
>>
>> Best regards,
>> Vladimir
>
> Might be being too informal. I just meant a lock or barrier to prevent
> actual IO throughput until we can confirm the dirty flag has been
> adjusted to indicate that the persistent bitmap is now officially out of
> date. Nothing fancy.
>
> Wasn't trying to imply that we needed threading protection, just
> "locking" the IO until we can configure the bitmap as we need it to be.
>