[16/22] qmp: add persistent flag to block-dirty-bitmap-add
diff mbox

Message ID 1475232808-4852-17-git-send-email-vsementsov@virtuozzo.com
State New
Headers show

Commit Message

Vladimir Sementsov-Ogievskiy Sept. 30, 2016, 10:53 a.m. UTC
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>
---
 blockdev.c           | 12 +++++++++++-
 qapi/block-core.json |  7 ++++++-
 qmp-commands.hx      |  5 ++++-
 3 files changed, 21 insertions(+), 3 deletions(-)

Comments

Eric Blake Oct. 7, 2016, 7:52 p.m. UTC | #1
On 09/30/2016 05:53 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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>
> ---
>  blockdev.c           | 12 +++++++++++-
>  qapi/block-core.json |  7 ++++++-
>  qmp-commands.hx      |  5 ++++-

This and later patches have conflicts with recent changes in the tree
that removed qmp-commands.hx; hopefully shouldn't impact review of this
series too badly.

> +++ b/qapi/block-core.json
> @@ -1235,10 +1235,15 @@
>  # @granularity: #optional the bitmap granularity, default is 64k for
>  #               block-dirty-bitmap-add
>  #
> +# @persistent: #optional the bitmap is persistent, i.e. it will be saved to
> +#              corresponding block device on it's close. Default is false.

s/corresponding/the corresponding/
s/it's/its/

("it's" is only appropriate if you can substitute "it is" or "it has")


> @@ -1458,6 +1458,9 @@ Arguments:
>  - "node": device/node on which to create dirty bitmap (json-string)
>  - "name": name of the new dirty bitmap (json-string)
>  - "granularity": granularity to track writes with (int, optional)
> +- "persistent": bitmap will be saved to corresponding block device
> +                on it's close. Block driver should maintain persistent bitmaps

One nice benefit of rebasing on top of qmp-commands.hx being removed is
that you don't have to repeat yourself :)
Max Reitz Oct. 10, 2016, 4:08 p.m. UTC | #2
On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
> 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>
> ---
>  blockdev.c           | 12 +++++++++++-
>  qapi/block-core.json |  7 ++++++-
>  qmp-commands.hx      |  5 ++++-
>  3 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 97062e3..ec0ec75 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1991,6 +1991,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) {
> @@ -2694,10 +2695,12 @@ 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)
>  {
>      AioContext *aio_context;
>      BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
>  
>      if (!name || name[0] == '\0') {
>          error_setg(errp, "Bitmap name cannot be empty");
> @@ -2723,7 +2726,14 @@ 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;
> +    }
> +
> +    bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
> +    if (bitmap != NULL) {
> +        bdrv_dirty_bitmap_set_persistance(bitmap, persistent);

As I said before, I think this command should be able to return errors
and make use of that to reject making bitmaps persistent when the
respective block driver cannot handle them.

> +    }
>  
>   out:
>      aio_context_release(aio_context);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 31f9990..2bf56cd 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1235,10 +1235,15 @@
>  # @granularity: #optional the bitmap granularity, default is 64k for
>  #               block-dirty-bitmap-add
>  #
> +# @persistent: #optional the bitmap is persistent, i.e. it will be saved to
> +#              corresponding block device on it's close. Default is false.
> +#              For block-dirty-bitmap-add. (Since 2.8)

I'm not sure what the "For block-dirty-bitmap-add." is supposed to mean,
because this whole struct is for block-dirty-bitmap-add (and for
block-dirty-bitmap-add transactions, to be exact, but @persistent will
surely work there, too, won't it?).

Also, I'd say "will be saved to the corresponding block device image
file" instead of just "block device", because in my understanding a
block device and its image file are two separate things.

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

I think normally we'd align the new line so that the opening ' of
'*persistent' is under the opening ' of 'node'.

>  
>  ##
>  # @block-dirty-bitmap-add
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index ba2a916..434b418 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1441,7 +1441,7 @@ EQMP
>  
>      {
>          .name       = "block-dirty-bitmap-add",
> -        .args_type  = "node:B,name:s,granularity:i?",
> +        .args_type  = "node:B,name:s,granularity:i?,persistent:b?",
>          .mhandler.cmd_new = qmp_marshal_block_dirty_bitmap_add,
>      },
>  
> @@ -1458,6 +1458,9 @@ Arguments:
>  - "node": device/node on which to create dirty bitmap (json-string)
>  - "name": name of the new dirty bitmap (json-string)
>  - "granularity": granularity to track writes with (int, optional)
> +- "persistent": bitmap will be saved to corresponding block device
> +                on it's close. Block driver should maintain persistent bitmaps
> +                (json-bool, optional, default false) (Since 2.8)

And I don't know what the user is supposed to make of the information
that block drivers will take care of maintaining persistent bitmaps. All
they care about is that it will be stored in the corresponding image
file, so in my opinion it would be better to just omit the last sentence
here.

Max

>  
>  Example:
>  
>
Vladimir Sementsov-Ogievskiy Oct. 24, 2016, 2:44 p.m. UTC | #3
07.10.2016 22:52, Eric Blake пишет:
> On 09/30/2016 05:53 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 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>
>> ---
>>   blockdev.c           | 12 +++++++++++-
>>   qapi/block-core.json |  7 ++++++-
>>   qmp-commands.hx      |  5 ++++-
> This and later patches have conflicts with recent changes in the tree
> that removed qmp-commands.hx; hopefully shouldn't impact review of this
> series too badly.
>
>> +++ b/qapi/block-core.json
>> @@ -1235,10 +1235,15 @@
>>   # @granularity: #optional the bitmap granularity, default is 64k for
>>   #               block-dirty-bitmap-add
>>   #
>> +# @persistent: #optional the bitmap is persistent, i.e. it will be saved to
>> +#              corresponding block device on it's close. Default is false.
> s/corresponding/the corresponding/
> s/it's/its/
>
> ("it's" is only appropriate if you can substitute "it is" or "it has")
>
>
>> @@ -1458,6 +1458,9 @@ Arguments:
>>   - "node": device/node on which to create dirty bitmap (json-string)
>>   - "name": name of the new dirty bitmap (json-string)
>>   - "granularity": granularity to track writes with (int, optional)
>> +- "persistent": bitmap will be saved to corresponding block device
>> +                on it's close. Block driver should maintain persistent bitmaps
> One nice benefit of rebasing on top of qmp-commands.hx being removed is
> that you don't have to repeat yourself :)

As I understand, duplicating description is still here, in 
docs/qmp-commands.txt. I just don't have to repeat parameters definition 
(which is good, anyway)
Vladimir Sementsov-Ogievskiy Oct. 24, 2016, 3:12 p.m. UTC | #4
10.10.2016 19:08, Max Reitz wrote:
> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>

[...]

>> +    }
>>   
>>    out:
>>       aio_context_release(aio_context);
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 31f9990..2bf56cd 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -1235,10 +1235,15 @@
>>   # @granularity: #optional the bitmap granularity, default is 64k for
>>   #               block-dirty-bitmap-add
>>   #
>> +# @persistent: #optional the bitmap is persistent, i.e. it will be saved to
>> +#              corresponding block device on it's close. Default is false.
>> +#              For block-dirty-bitmap-add. (Since 2.8)
> I'm not sure what the "For block-dirty-bitmap-add." is supposed to mean,
> because this whole struct is for block-dirty-bitmap-add (and for
> block-dirty-bitmap-add transactions, to be exact, but @persistent will
> surely work there, too, won't it?).
>
> Also, I'd say "will be saved to the corresponding block device image
> file" instead of just "block device", because in my understanding a
> block device and its image file are two separate things.

Hmm.. But 'its close' is block device close, not file close. And we call 
common bdrv_* function to save it, so we actually save it to device, and 
then the device puzzles out, how to actually save it.

>
>> +#
>>   # Since 2.4
>>   ##
>>   { 'struct': 'BlockDirtyBitmapAdd',
>> -  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
>> +  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
>> +  '*persistent': 'bool' } }
> I think normally we'd align the new line so that the opening ' of
> '*persistent' is under the opening ' of 'node'.
>
>>   
>>   ##
>>   # @block-dirty-bitmap-add
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index ba2a916..434b418 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -1441,7 +1441,7 @@ EQMP
>>   
>>       {
>>           .name       = "block-dirty-bitmap-add",
>> -        .args_type  = "node:B,name:s,granularity:i?",
>> +        .args_type  = "node:B,name:s,granularity:i?,persistent:b?",
>>           .mhandler.cmd_new = qmp_marshal_block_dirty_bitmap_add,
>>       },
>>   
>> @@ -1458,6 +1458,9 @@ Arguments:
>>   - "node": device/node on which to create dirty bitmap (json-string)
>>   - "name": name of the new dirty bitmap (json-string)
>>   - "granularity": granularity to track writes with (int, optional)
>> +- "persistent": bitmap will be saved to corresponding block device
>> +                on it's close. Block driver should maintain persistent bitmaps
>> +                (json-bool, optional, default false) (Since 2.8)
> And I don't know what the user is supposed to make of the information
> that block drivers will take care of maintaining persistent bitmaps. All
> they care about is that it will be stored in the corresponding image
> file, so in my opinion it would be better to just omit the last sentence
> here.

Users shoud know, that only qcow2 supports persistent bitmaps. Instead 
of last sentence I can write "For now only Qcow2 disks supports 
persistent bitmaps".
Max Reitz Oct. 24, 2016, 5:30 p.m. UTC | #5
On 24.10.2016 17:12, Vladimir Sementsov-Ogievskiy wrote:
> 10.10.2016 19:08, Max Reitz wrote:
>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>>
> 
> [...]
> 
>>> +    }
>>>      out:
>>>       aio_context_release(aio_context);
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 31f9990..2bf56cd 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -1235,10 +1235,15 @@
>>>   # @granularity: #optional the bitmap granularity, default is 64k for
>>>   #               block-dirty-bitmap-add
>>>   #
>>> +# @persistent: #optional the bitmap is persistent, i.e. it will be
>>> saved to
>>> +#              corresponding block device on it's close. Default is
>>> false.
>>> +#              For block-dirty-bitmap-add. (Since 2.8)
>> I'm not sure what the "For block-dirty-bitmap-add." is supposed to mean,
>> because this whole struct is for block-dirty-bitmap-add (and for
>> block-dirty-bitmap-add transactions, to be exact, but @persistent will
>> surely work there, too, won't it?).
>>
>> Also, I'd say "will be saved to the corresponding block device image
>> file" instead of just "block device", because in my understanding a
>> block device and its image file are two separate things.
> 
> Hmm.. But 'its close' is block device close, not file close.

In my understanding, it is the file close.

>                                                              And we call
> common bdrv_* function to save it, so we actually save it to device, and
> then the device puzzles out, how to actually save it.

Well, OK, it depends on what you mean by "block device". There are many
things we call a "block device", but nowadays I think it mostly refers
to either a guest block device or a BlockBackend (and as of lately,
we're more and more trying to hide the BlockBackend from the user, so
you could argue that it's only the guest device now).

The bdrv_* functions operate on block layer BDS nodes, and I don't think
we call them "block devices" (at least not any more).

In any case, I think for users the term "block device" refers to either
the device presented to the guest or to all of the block layer stuff
that's underneath, and it's not quite clear how you could save a bitmap
to that, or what it's supposed to mean to "close" a block device (you
can remove it, you can destroy it, you can delete it, but "close" it?).

But saying that it will be saved to the image file that is attached to
the block device will make it absolutely clear what we mean.

So what you have called a "device" here is neither what I'd call a
device (I'd call it a "node" or "BDS"), nor what I think users would
call a device. Also, it's not the BDS that puzzles out how to save it
but some block driver.

>>> +#
>>>   # Since 2.4
>>>   ##
>>>   { 'struct': 'BlockDirtyBitmapAdd',
>>> -  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
>>> +  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
>>> +  '*persistent': 'bool' } }
>> I think normally we'd align the new line so that the opening ' of
>> '*persistent' is under the opening ' of 'node'.
>>
>>>     ##
>>>   # @block-dirty-bitmap-add
>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>> index ba2a916..434b418 100644
>>> --- a/qmp-commands.hx
>>> +++ b/qmp-commands.hx
>>> @@ -1441,7 +1441,7 @@ EQMP
>>>         {
>>>           .name       = "block-dirty-bitmap-add",
>>> -        .args_type  = "node:B,name:s,granularity:i?",
>>> +        .args_type  = "node:B,name:s,granularity:i?,persistent:b?",
>>>           .mhandler.cmd_new = qmp_marshal_block_dirty_bitmap_add,
>>>       },
>>>   @@ -1458,6 +1458,9 @@ Arguments:
>>>   - "node": device/node on which to create dirty bitmap (json-string)
>>>   - "name": name of the new dirty bitmap (json-string)
>>>   - "granularity": granularity to track writes with (int, optional)
>>> +- "persistent": bitmap will be saved to corresponding block device
>>> +                on it's close. Block driver should maintain
>>> persistent bitmaps
>>> +                (json-bool, optional, default false) (Since 2.8)
>> And I don't know what the user is supposed to make of the information
>> that block drivers will take care of maintaining persistent bitmaps. All
>> they care about is that it will be stored in the corresponding image
>> file, so in my opinion it would be better to just omit the last sentence
>> here.
> 
> Users shoud know, that only qcow2 supports persistent bitmaps. Instead
> of last sentence I can write "For now only Qcow2 disks supports
> persistent bitmaps".

s/supports/support/, but yes, that sounds preferable to me.

Max
Vladimir Sementsov-Ogievskiy Oct. 25, 2016, 11:05 a.m. UTC | #6
24.10.2016 20:30, Max Reitz wrote:
> On 24.10.2016 17:12, Vladimir Sementsov-Ogievskiy wrote:
>> 10.10.2016 19:08, Max Reitz wrote:
>>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>>>
>> [...]
>>
>>>> +    }
>>>>       out:
>>>>        aio_context_release(aio_context);
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index 31f9990..2bf56cd 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -1235,10 +1235,15 @@
>>>>    # @granularity: #optional the bitmap granularity, default is 64k for
>>>>    #               block-dirty-bitmap-add
>>>>    #
>>>> +# @persistent: #optional the bitmap is persistent, i.e. it will be
>>>> saved to
>>>> +#              corresponding block device on it's close. Default is
>>>> false.
>>>> +#              For block-dirty-bitmap-add. (Since 2.8)
>>> I'm not sure what the "For block-dirty-bitmap-add." is supposed to mean,
>>> because this whole struct is for block-dirty-bitmap-add (and for
>>> block-dirty-bitmap-add transactions, to be exact, but @persistent will
>>> surely work there, too, won't it?).
>>>
>>> Also, I'd say "will be saved to the corresponding block device image
>>> file" instead of just "block device", because in my understanding a
>>> block device and its image file are two separate things.
>> Hmm.. But 'its close' is block device close, not file close.
> In my understanding, it is the file close.
>
>>                                                               And we call
>> common bdrv_* function to save it, so we actually save it to device, and
>> then the device puzzles out, how to actually save it.
> Well, OK, it depends on what you mean by "block device". There are many
> things we call a "block device", but nowadays I think it mostly refers
> to either a guest block device or a BlockBackend (and as of lately,
> we're more and more trying to hide the BlockBackend from the user, so
> you could argue that it's only the guest device now).
>
> The bdrv_* functions operate on block layer BDS nodes, and I don't think
> we call them "block devices" (at least not any more).
>
> In any case, I think for users the term "block device" refers to either
> the device presented to the guest or to all of the block layer stuff
> that's underneath, and it's not quite clear how you could save a bitmap
> to that, or what it's supposed to mean to "close" a block device (you
> can remove it, you can destroy it, you can delete it, but "close" it?).
>
> But saying that it will be saved to the image file that is attached to
> the block device will make it absolutely clear what we mean.
>
> So what you have called a "device" here is neither what I'd call a
> device (I'd call it a "node" or "BDS"), nor what I think users would
> call a device. Also, it's not the BDS that puzzles out how to save it
> but some block driver.
>

Ok, thank you.

>>>> +#
>>>>    # Since 2.4
>>>>    ##
>>>>    { 'struct': 'BlockDirtyBitmapAdd',
>>>> -  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
>>>> +  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
>>>> +  '*persistent': 'bool' } }
>>> I think normally we'd align the new line so that the opening ' of
>>> '*persistent' is under the opening ' of 'node'.
>>>
>>>>      ##
>>>>    # @block-dirty-bitmap-add
>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>> index ba2a916..434b418 100644
>>>> --- a/qmp-commands.hx
>>>> +++ b/qmp-commands.hx
>>>> @@ -1441,7 +1441,7 @@ EQMP
>>>>          {
>>>>            .name       = "block-dirty-bitmap-add",
>>>> -        .args_type  = "node:B,name:s,granularity:i?",
>>>> +        .args_type  = "node:B,name:s,granularity:i?,persistent:b?",
>>>>            .mhandler.cmd_new = qmp_marshal_block_dirty_bitmap_add,
>>>>        },
>>>>    @@ -1458,6 +1458,9 @@ Arguments:
>>>>    - "node": device/node on which to create dirty bitmap (json-string)
>>>>    - "name": name of the new dirty bitmap (json-string)
>>>>    - "granularity": granularity to track writes with (int, optional)
>>>> +- "persistent": bitmap will be saved to corresponding block device
>>>> +                on it's close. Block driver should maintain
>>>> persistent bitmaps
>>>> +                (json-bool, optional, default false) (Since 2.8)
>>> And I don't know what the user is supposed to make of the information
>>> that block drivers will take care of maintaining persistent bitmaps. All
>>> they care about is that it will be stored in the corresponding image
>>> file, so in my opinion it would be better to just omit the last sentence
>>> here.
>> Users shoud know, that only qcow2 supports persistent bitmaps. Instead
>> of last sentence I can write "For now only Qcow2 disks supports
>> persistent bitmaps".
> s/supports/support/, but yes, that sounds preferable to me.
>
> Max
>

Patch
diff mbox

diff --git a/blockdev.c b/blockdev.c
index 97062e3..ec0ec75 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1991,6 +1991,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) {
@@ -2694,10 +2695,12 @@  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)
 {
     AioContext *aio_context;
     BlockDriverState *bs;
+    BdrvDirtyBitmap *bitmap;
 
     if (!name || name[0] == '\0') {
         error_setg(errp, "Bitmap name cannot be empty");
@@ -2723,7 +2726,14 @@  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;
+    }
+
+    bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
+    if (bitmap != NULL) {
+        bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
+    }
 
  out:
     aio_context_release(aio_context);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 31f9990..2bf56cd 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1235,10 +1235,15 @@ 
 # @granularity: #optional the bitmap granularity, default is 64k for
 #               block-dirty-bitmap-add
 #
+# @persistent: #optional the bitmap is persistent, i.e. it will be saved to
+#              corresponding block device on it's close. Default is false.
+#              For block-dirty-bitmap-add. (Since 2.8)
+#
 # 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
diff --git a/qmp-commands.hx b/qmp-commands.hx
index ba2a916..434b418 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1441,7 +1441,7 @@  EQMP
 
     {
         .name       = "block-dirty-bitmap-add",
-        .args_type  = "node:B,name:s,granularity:i?",
+        .args_type  = "node:B,name:s,granularity:i?,persistent:b?",
         .mhandler.cmd_new = qmp_marshal_block_dirty_bitmap_add,
     },
 
@@ -1458,6 +1458,9 @@  Arguments:
 - "node": device/node on which to create dirty bitmap (json-string)
 - "name": name of the new dirty bitmap (json-string)
 - "granularity": granularity to track writes with (int, optional)
+- "persistent": bitmap will be saved to corresponding block device
+                on it's close. Block driver should maintain persistent bitmaps
+                (json-bool, optional, default false) (Since 2.8)
 
 Example: