[Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable

Vladimir Sementsov-Ogievskiy posted 6 patches 8 years ago
[Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable
Posted by Vladimir Sementsov-Ogievskiy 8 years ago
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++++
 blockdev.c           | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 827254db22..b3851ee760 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1672,6 +1672,48 @@
   'data': 'BlockDirtyBitmap' }
 
 ##
+# @block-dirty-bitmap-enable:
+#
+# Enable dirty bitmap, so that it will continue tracking disk changes.
+#
+# Returns: nothing on success
+#          If @node is not a valid block device, DeviceNotFound
+#          If @name is not found, GenericError with an explanation
+#
+# Since: 2.12
+#
+# Example:
+#
+# -> { "execute": "block-dirty-bitmap-enable",
+#      "arguments": { "node": "drive0", "name": "bitmap0" } }
+# <- { "return": {} }
+#
+##
+  { 'command': 'block-dirty-bitmap-enable',
+    'data': 'BlockDirtyBitmap' }
+
+##
+# @block-dirty-bitmap-disable:
+#
+# Disable dirty bitmap, so that it will stop tracking disk changes.
+#
+# Returns: nothing on success
+#          If @node is not a valid block device, DeviceNotFound
+#          If @name is not found, GenericError with an explanation
+#
+# Since: 2.12
+#
+# Example:
+#
+# -> { "execute": "block-dirty-bitmap-disable",
+#      "arguments": { "node": "drive0", "name": "bitmap0" } }
+# <- { "return": {} }
+#
+##
+    { 'command': 'block-dirty-bitmap-disable',
+      'data': 'BlockDirtyBitmap' }
+
+##
 # @BlockDirtyBitmapSha256:
 #
 # SHA256 hash of dirty bitmap data
diff --git a/blockdev.c b/blockdev.c
index 8068cbd606..997a6f514c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2821,6 +2821,48 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
     bdrv_clear_dirty_bitmap(bitmap, NULL);
 }
 
+void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
+                                   Error **errp)
+{
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+
+    bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
+    if (!bitmap || !bs) {
+        return;
+    }
+
+    if (bdrv_dirty_bitmap_frozen(bitmap)) {
+        error_setg(errp,
+                   "Bitmap '%s' is currently frozen and cannot be enabled",
+                   name);
+        return;
+    }
+
+    bdrv_enable_dirty_bitmap(bitmap);
+}
+
+void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
+                                    Error **errp)
+{
+    BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
+
+    bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
+    if (!bitmap || !bs) {
+        return;
+    }
+
+    if (bdrv_dirty_bitmap_frozen(bitmap)) {
+        error_setg(errp,
+                   "Bitmap '%s' is currently frozen and cannot be disabled",
+                   name);
+        return;
+    }
+
+    bdrv_disable_dirty_bitmap(bitmap);
+}
+
 BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
                                                               const char *name,
                                                               Error **errp)
-- 
2.11.1


Re: [Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable
Posted by John Snow 8 years ago

On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++++
>  blockdev.c           | 42 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 827254db22..b3851ee760 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1672,6 +1672,48 @@
>    'data': 'BlockDirtyBitmap' }
>  
>  ##
> +# @block-dirty-bitmap-enable:
> +#
> +# Enable dirty bitmap, so that it will continue tracking disk changes.
> +#

suggest:
"Enables a dirty bitmap so that it will begin tracking disk changes."

Key item here is "begin" instead of "continue".

> +# Returns: nothing on success
> +#          If @node is not a valid block device, DeviceNotFound
> +#          If @name is not found, GenericError with an explanation
> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# -> { "execute": "block-dirty-bitmap-enable",
> +#      "arguments": { "node": "drive0", "name": "bitmap0" } }
> +# <- { "return": {} }
> +#
> +##
> +  { 'command': 'block-dirty-bitmap-enable',
> +    'data': 'BlockDirtyBitmap' }
> +
> +##
> +# @block-dirty-bitmap-disable:
> +#
> +# Disable dirty bitmap, so that it will stop tracking disk changes.
> +#

suggest:
"Disables a dirty bitmap so that it will stop tracking disk changes."

> +# Returns: nothing on success
> +#          If @node is not a valid block device, DeviceNotFound
> +#          If @name is not found, GenericError with an explanation
> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# -> { "execute": "block-dirty-bitmap-disable",
> +#      "arguments": { "node": "drive0", "name": "bitmap0" } }
> +# <- { "return": {} }
> +#
> +##
> +    { 'command': 'block-dirty-bitmap-disable',
> +      'data': 'BlockDirtyBitmap' }
> +
> +##
>  # @BlockDirtyBitmapSha256:
>  #
>  # SHA256 hash of dirty bitmap data
> diff --git a/blockdev.c b/blockdev.c
> index 8068cbd606..997a6f514c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2821,6 +2821,48 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
>      bdrv_clear_dirty_bitmap(bitmap, NULL);
>  }
>  
> +void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
> +                                   Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +
> +    bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
> +    if (!bitmap || !bs) {
> +        return;
> +    }
> +
> +    if (bdrv_dirty_bitmap_frozen(bitmap)) {
> +        error_setg(errp,
> +                   "Bitmap '%s' is currently frozen and cannot be enabled",
> +                   name);
> +        return;
> +    }
> +
> +    bdrv_enable_dirty_bitmap(bitmap);
> +}
> +
> +void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
> +                                    Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +
> +    bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
> +    if (!bitmap || !bs) {
> +        return;
> +    }
> +
> +    if (bdrv_dirty_bitmap_frozen(bitmap)) {
> +        error_setg(errp,
> +                   "Bitmap '%s' is currently frozen and cannot be disabled",
> +                   name);
> +        return;
> +    }
> +
> +    bdrv_disable_dirty_bitmap(bitmap);
> +}
> +
>  BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
>                                                                const char *name,
>                                                                Error **errp)
> 

I have to admit exposing this interface still makes me nervous, but :)

Mechanically correct, and with suggesting phrasing changes:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable
Posted by Vladimir Sementsov-Ogievskiy 8 years ago
20.01.2018 02:50, John Snow wrote:
>
> On 01/16/2018 07:54 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   blockdev.c           | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 84 insertions(+)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 827254db22..b3851ee760 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1672,6 +1672,48 @@
>>     'data': 'BlockDirtyBitmap' }
>>   
>>   ##
>> +# @block-dirty-bitmap-enable:
>> +#
>> +# Enable dirty bitmap, so that it will continue tracking disk changes.
>> +#
> suggest:
> "Enables a dirty bitmap so that it will begin tracking disk changes."
>
> Key item here is "begin" instead of "continue".

agree.

>
>> +# Returns: nothing on success
>> +#          If @node is not a valid block device, DeviceNotFound
>> +#          If @name is not found, GenericError with an explanation
>> +#
>> +# Since: 2.12
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "block-dirty-bitmap-enable",
>> +#      "arguments": { "node": "drive0", "name": "bitmap0" } }
>> +# <- { "return": {} }
>> +#
>> +##
>> +  { 'command': 'block-dirty-bitmap-enable',
>> +    'data': 'BlockDirtyBitmap' }
>> +
>> +##
>> +# @block-dirty-bitmap-disable:
>> +#
>> +# Disable dirty bitmap, so that it will stop tracking disk changes.
>> +#
> suggest:
> "Disables a dirty bitmap so that it will stop tracking disk changes."
>
>> +# Returns: nothing on success
>> +#          If @node is not a valid block device, DeviceNotFound
>> +#          If @name is not found, GenericError with an explanation
>> +#
>> +# Since: 2.12
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "block-dirty-bitmap-disable",
>> +#      "arguments": { "node": "drive0", "name": "bitmap0" } }
>> +# <- { "return": {} }
>> +#
>> +##
>> +    { 'command': 'block-dirty-bitmap-disable',
>> +      'data': 'BlockDirtyBitmap' }
>> +
>> +##
>>   # @BlockDirtyBitmapSha256:
>>   #
>>   # SHA256 hash of dirty bitmap data
>> diff --git a/blockdev.c b/blockdev.c
>> index 8068cbd606..997a6f514c 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2821,6 +2821,48 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
>>       bdrv_clear_dirty_bitmap(bitmap, NULL);
>>   }
>>   
>> +void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
>> +                                   Error **errp)
>> +{
>> +    BlockDriverState *bs;
>> +    BdrvDirtyBitmap *bitmap;
>> +
>> +    bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
>> +    if (!bitmap || !bs) {
>> +        return;
>> +    }
>> +
>> +    if (bdrv_dirty_bitmap_frozen(bitmap)) {
>> +        error_setg(errp,
>> +                   "Bitmap '%s' is currently frozen and cannot be enabled",
>> +                   name);
>> +        return;
>> +    }
>> +
>> +    bdrv_enable_dirty_bitmap(bitmap);
>> +}
>> +
>> +void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
>> +                                    Error **errp)
>> +{
>> +    BlockDriverState *bs;
>> +    BdrvDirtyBitmap *bitmap;
>> +
>> +    bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
>> +    if (!bitmap || !bs) {
>> +        return;
>> +    }
>> +
>> +    if (bdrv_dirty_bitmap_frozen(bitmap)) {
>> +        error_setg(errp,
>> +                   "Bitmap '%s' is currently frozen and cannot be disabled",
>> +                   name);
>> +        return;
>> +    }
>> +
>> +    bdrv_disable_dirty_bitmap(bitmap);
>> +}
>> +
>>   BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
>>                                                                 const char *name,
>>                                                                 Error **errp)
>>
> I have to admit exposing this interface still makes me nervous, but :)
>
> Mechanically correct, and with suggesting phrasing changes:
>
> Reviewed-by: John Snow <jsnow@redhat.com>



-- 
Best regards,
Vladimir


Re: [Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable
Posted by Eric Blake 8 years ago
On 01/22/2018 03:09 AM, Vladimir Sementsov-Ogievskiy wrote:

>>>
>> I have to admit exposing this interface still makes me nervous, but :)
>>
>> Mechanically correct, and with suggesting phrasing changes:
>>
>> Reviewed-by: John Snow <jsnow@redhat.com>

Should we go with an x- prefix for now, and let things settle for a
release before promoting it to fully supported, just in case we find
something that justifies your nervousness in accepting the interface?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable
Posted by John Snow 8 years ago

On 01/22/2018 02:51 PM, Eric Blake wrote:
> On 01/22/2018 03:09 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>>>
>>> I have to admit exposing this interface still makes me nervous, but :)
>>>
>>> Mechanically correct, and with suggesting phrasing changes:
>>>
>>> Reviewed-by: John Snow <jsnow@redhat.com>
> 
> Should we go with an x- prefix for now, and let things settle for a
> release before promoting it to fully supported, just in case we find
> something that justifies your nervousness in accepting the interface?
> 

I proposed as much in a reply to the cover letter; I'm willing to take
patch one now for the sake of migration; the rest of this series I want
a test suite that helps document the anticipated use case, or otherwise
an x- prefix for the command names.

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable
Posted by Vladimir Sementsov-Ogievskiy 8 years ago
22.01.2018 22:56, John Snow wrote:
>
> On 01/22/2018 02:51 PM, Eric Blake wrote:
>> On 01/22/2018 03:09 AM, Vladimir Sementsov-Ogievskiy wrote:
>>
>>>> I have to admit exposing this interface still makes me nervous, but :)
>>>>
>>>> Mechanically correct, and with suggesting phrasing changes:
>>>>
>>>> Reviewed-by: John Snow <jsnow@redhat.com>
>> Should we go with an x- prefix for now, and let things settle for a
>> release before promoting it to fully supported, just in case we find
>> something that justifies your nervousness in accepting the interface?
>>
> I proposed as much in a reply to the cover letter; I'm willing to take
> patch one now for the sake of migration; the rest of this series I want
> a test suite that helps document the anticipated use case, or otherwise
> an x- prefix for the command names.

Give me a try with test, and if it will still be unclear, then let's add 
x- prefix for first steps.
(of course, my aim is to push the api without x- prefixes =)

-- 
Best regards,
Vladimir


Re: [Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable
Posted by Markus Armbruster 8 years ago
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++++
>  blockdev.c           | 42 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 827254db22..b3851ee760 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1672,6 +1672,48 @@
>    'data': 'BlockDirtyBitmap' }
>  
>  ##
> +# @block-dirty-bitmap-enable:
> +#
> +# Enable dirty bitmap, so that it will continue tracking disk changes.
> +#
> +# Returns: nothing on success
> +#          If @node is not a valid block device, DeviceNotFound
> +#          If @name is not found, GenericError with an explanation

What do you mean by "with an explanation"?  If it's the error objects
@desc member: that's trivial, any error carries it.

The less error classes other than GenericError are used, the happier I
am...  Do users really need to distinguish between these two errors?

> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# -> { "execute": "block-dirty-bitmap-enable",
> +#      "arguments": { "node": "drive0", "name": "bitmap0" } }
> +# <- { "return": {} }
> +#
> +##
> +  { 'command': 'block-dirty-bitmap-enable',
> +    'data': 'BlockDirtyBitmap' }
> +
> +##
> +# @block-dirty-bitmap-disable:
> +#
> +# Disable dirty bitmap, so that it will stop tracking disk changes.
> +#
> +# Returns: nothing on success
> +#          If @node is not a valid block device, DeviceNotFound
> +#          If @name is not found, GenericError with an explanation

Likewise.

> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# -> { "execute": "block-dirty-bitmap-disable",
> +#      "arguments": { "node": "drive0", "name": "bitmap0" } }
> +# <- { "return": {} }
> +#
> +##
> +    { 'command': 'block-dirty-bitmap-disable',
> +      'data': 'BlockDirtyBitmap' }
> +
> +##
>  # @BlockDirtyBitmapSha256:
>  #
>  # SHA256 hash of dirty bitmap data
[...]

Re: [Qemu-devel] [PATCH v2 3/6] qapi: add block-dirty-bitmap-enable/disable
Posted by Vladimir Sementsov-Ogievskiy 8 years ago
03.02.2018 19:09, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qapi/block-core.json | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   blockdev.c           | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 84 insertions(+)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 827254db22..b3851ee760 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1672,6 +1672,48 @@
>>     'data': 'BlockDirtyBitmap' }
>>   
>>   ##
>> +# @block-dirty-bitmap-enable:
>> +#
>> +# Enable dirty bitmap, so that it will continue tracking disk changes.
>> +#
>> +# Returns: nothing on success
>> +#          If @node is not a valid block device, DeviceNotFound
>> +#          If @name is not found, GenericError with an explanation
> What do you mean by "with an explanation"?  If it's the error objects
> @desc member: that's trivial, any error carries it.
>
> The less error classes other than GenericError are used, the happier I
> am...  Do users really need to distinguish between these two errors?

I've just used same semantics as it is used for 
block-dirty-bitmap-clear.. block-dirty-bitmap-remove and
block-dirty-bitmap-add have the same semantics too.

>
>> +#
>> +# Since: 2.12
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "block-dirty-bitmap-enable",
>> +#      "arguments": { "node": "drive0", "name": "bitmap0" } }
>> +# <- { "return": {} }
>> +#
>> +##
>> +  { 'command': 'block-dirty-bitmap-enable',
>> +    'data': 'BlockDirtyBitmap' }
>> +
>> +##
>> +# @block-dirty-bitmap-disable:
>> +#
>> +# Disable dirty bitmap, so that it will stop tracking disk changes.
>> +#
>> +# Returns: nothing on success
>> +#          If @node is not a valid block device, DeviceNotFound
>> +#          If @name is not found, GenericError with an explanation
> Likewise.
>
>> +#
>> +# Since: 2.12
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "block-dirty-bitmap-disable",
>> +#      "arguments": { "node": "drive0", "name": "bitmap0" } }
>> +# <- { "return": {} }
>> +#
>> +##
>> +    { 'command': 'block-dirty-bitmap-disable',
>> +      'data': 'BlockDirtyBitmap' }
>> +
>> +##
>>   # @BlockDirtyBitmapSha256:
>>   #
>>   # SHA256 hash of dirty bitmap data
> [...]


-- 
Best regards,
Vladimir