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

[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

[Vladimir Sementsov-Ogievskiy]

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

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

-- 
Best regards,
Vladimir

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

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

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

[John Snow]

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

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

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

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

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

-- 
—js