[Qemu-devel] [PATCH v22 23/30] qmp: add persistent flag to block-dirty-bitmap-add

Vladimir Sementsov-Ogievskiy posted 30 patches 8 years, 7 months ago
[Qemu-devel] [PATCH v22 23/30] qmp: add persistent flag to block-dirty-bitmap-add
Posted by Vladimir Sementsov-Ogievskiy 8 years, 7 months ago
Add optional 'persistent' flag to qmp command block-dirty-bitmap-add.
Default is false.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 blockdev.c           | 18 +++++++++++++++++-
 qapi/block-core.json |  8 +++++++-
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 64e03c0caf..125acabc07 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1973,6 +1973,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState *common,
     /* AIO context taken and released within qmp_block_dirty_bitmap_add */
     qmp_block_dirty_bitmap_add(action->node, action->name,
                                action->has_granularity, action->granularity,
+                               action->has_persistent, action->persistent,
                                &local_err);
 
     if (!local_err) {
@@ -2720,9 +2721,11 @@ out:
 
 void qmp_block_dirty_bitmap_add(const char *node, const char *name,
                                 bool has_granularity, uint32_t granularity,
+                                bool has_persistent, bool persistent,
                                 Error **errp)
 {
     BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
 
     if (!name || name[0] == '\0') {
         error_setg(errp, "Bitmap name cannot be empty");
@@ -2745,7 +2748,20 @@ void qmp_block_dirty_bitmap_add(const char *node, const char *name,
         granularity = bdrv_get_default_bitmap_granularity(bs);
     }
 
-    bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+    if (!has_persistent) {
+        persistent = false;
+    }
+
+    if (persistent &&
+        !bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp))
+    {
+        return;
+    }
+
+    bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+    if (bitmap != NULL) {
+        bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
+    }
 }
 
 void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f85c2235c7..13f98ec146 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1561,10 +1561,16 @@
 # @granularity: the bitmap granularity, default is 64k for
 #               block-dirty-bitmap-add
 #
+# @persistent: the bitmap is persistent, i.e. it will be saved to the
+#              corresponding block device image file on its close. For now only
+#              Qcow2 disks support persistent bitmaps. Default is false for
+#              block-dirty-bitmap-add. (Since: 2.10)
+#
 # Since: 2.4
 ##
 { 'struct': 'BlockDirtyBitmapAdd',
-  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
+  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
+            '*persistent': 'bool' } }
 
 ##
 # @block-dirty-bitmap-add:
-- 
2.11.1


Re: [Qemu-devel] [PATCH v22 23/30] qmp: add persistent flag to block-dirty-bitmap-add
Posted by Markus Armbruster 8 years, 7 months ago
QAPI schema review only...  I apologize for its lateness.

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> Add optional 'persistent' flag to qmp command block-dirty-bitmap-add.
> Default is false.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> ---
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index f85c2235c7..13f98ec146 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1561,10 +1561,16 @@
>  # @granularity: the bitmap granularity, default is 64k for
>  #               block-dirty-bitmap-add
>  #
> +# @persistent: the bitmap is persistent, i.e. it will be saved to the
> +#              corresponding block device image file on its close. For now only
> +#              Qcow2 disks support persistent bitmaps. Default is false for
> +#              block-dirty-bitmap-add. (Since: 2.10)

"for block-dirty-bitmap-add" suggests there could be other users, with
different (but unspecified) defaults.  What about replacing the sentence
by "(default: false)"?

Please wrap your comment lines around column 70.

> +#
>  # Since: 2.4
>  ##
>  { 'struct': 'BlockDirtyBitmapAdd',
> -  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
> +  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
> +            '*persistent': 'bool' } }
>  
>  ##
>  # @block-dirty-bitmap-add:

Re: [Qemu-devel] [PATCH v22 23/30] qmp: add persistent flag to block-dirty-bitmap-add
Posted by Vladimir Sementsov-Ogievskiy 8 years, 7 months ago
07.07.2017 10:54, Markus Armbruster wrote:
> QAPI schema review only...  I apologize for its lateness.
>
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> Add optional 'persistent' flag to qmp command block-dirty-bitmap-add.
>> Default is false.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>> ---
> [...]
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index f85c2235c7..13f98ec146 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1561,10 +1561,16 @@
>>   # @granularity: the bitmap granularity, default is 64k for
>>   #               block-dirty-bitmap-add
>>   #
>> +# @persistent: the bitmap is persistent, i.e. it will be saved to the
>> +#              corresponding block device image file on its close. For now only
>> +#              Qcow2 disks support persistent bitmaps. Default is false for
>> +#              block-dirty-bitmap-add. (Since: 2.10)
> "for block-dirty-bitmap-add" suggests there could be other users, with
> different (but unspecified) defaults.  What about replacing the sentence
> by "(default: false)"?

then, this should be done in other places, @granularity is an example.

>
> Please wrap your comment lines around column 70.
>
>> +#
>>   # Since: 2.4
>>   ##
>>   { 'struct': 'BlockDirtyBitmapAdd',
>> -  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
>> +  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
>> +            '*persistent': 'bool' } }
>>   
>>   ##
>>   # @block-dirty-bitmap-add:


-- 
Best regards,
Vladimir


Re: [Qemu-devel] [PATCH v22 23/30] qmp: add persistent flag to block-dirty-bitmap-add
Posted by Vladimir Sementsov-Ogievskiy 8 years, 7 months ago
07.07.2017 10:54, Markus Armbruster wrote:
> QAPI schema review only...  I apologize for its lateness.
>
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> Add optional 'persistent' flag to qmp command block-dirty-bitmap-add.
>> Default is false.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>> ---
> [...]
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index f85c2235c7..13f98ec146 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1561,10 +1561,16 @@
>>   # @granularity: the bitmap granularity, default is 64k for
>>   #               block-dirty-bitmap-add
>>   #
>> +# @persistent: the bitmap is persistent, i.e. it will be saved to the
>> +#              corresponding block device image file on its close. For now only
>> +#              Qcow2 disks support persistent bitmaps. Default is false for
>> +#              block-dirty-bitmap-add. (Since: 2.10)
> "for block-dirty-bitmap-add" suggests there could be other users, with
> different (but unspecified) defaults.  What about replacing the sentence
> by "(default: false)"?
>
> Please wrap your comment lines around column 70.

Why 70, is it written somewhere? There are a lot of lines over 70 
characters in this file, so, as series are already in Max's block branch 
I think it would be better to fix the whole file, if it is really needed.

>
>> +#
>>   # Since: 2.4
>>   ##
>>   { 'struct': 'BlockDirtyBitmapAdd',
>> -  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
>> +  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
>> +            '*persistent': 'bool' } }
>>   
>>   ##
>>   # @block-dirty-bitmap-add:


-- 
Best regards,
Vladimir


Re: [Qemu-devel] [PATCH v22 23/30] qmp: add persistent flag to block-dirty-bitmap-add
Posted by Markus Armbruster 8 years, 7 months ago
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 07.07.2017 10:54, Markus Armbruster wrote:
>> QAPI schema review only...  I apologize for its lateness.
>>
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> Add optional 'persistent' flag to qmp command block-dirty-bitmap-add.
>>> Default is false.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>> ---
>> [...]
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index f85c2235c7..13f98ec146 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -1561,10 +1561,16 @@
>>>   # @granularity: the bitmap granularity, default is 64k for
>>>   #               block-dirty-bitmap-add
>>>   #
>>> +# @persistent: the bitmap is persistent, i.e. it will be saved to the
>>> +#              corresponding block device image file on its close. For now only
>>> +#              Qcow2 disks support persistent bitmaps. Default is false for
>>> +#              block-dirty-bitmap-add. (Since: 2.10)
>> "for block-dirty-bitmap-add" suggests there could be other users, with
>> different (but unspecified) defaults.  What about replacing the sentence
>> by "(default: false)"?
>>
>> Please wrap your comment lines around column 70.
>
> Why 70, is it written somewhere? There are a lot of lines over 70
> characters in this file, so, as series are already in Max's block
> branch I think it would be better to fix the whole file, if it is
> really needed.

There's no hard rule on comment line length.  I routinely advise people
to wrap anyway, for legibility.  Humans tend to have trouble following
long lines with their eyes (I sure do).  Typographic manuals suggest to
limit columns to roughly 60 characters for exactly that reason[*].

I'm not the maintainer here, so this is really advice, not a demand.



[*] https://en.wikipedia.org/wiki/Column_(typography)#Typographic_style