This is for the purpose of toggling on/off persistence on a bitmap.
This enables you to save a bitmap that was not persistent, but may
have already accumulated valuable data.

This is simply a QOL enhancement:
- Allows user to "upgrade" an existing bitmap to persistent
- Allows user to "downgrade" an existing bitmap to transient,
  removing it from storage without deleting the bitmap.

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

This is just an RFC because I'm not sure if I really want to pursue
adding this, but it was raised in a discussion I had recently that it
was a little annoying as an API design that persistence couldn't be
changed after addition, so I wanted to see how much code it would take
to address that.

(So this patch isn't really tested; just: "Hey, look!")

I don't like this patch because it exacerbates my perceived problems
with the "check if I can make it persistent, then toggle the flag"
model, where I prefer the "Just try to set it persistent and let it fail
if it cannot" model, but there were some issues with that patchset that
I want to revisit.

---

 blockdev.c           | 49 ++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json | 34 ++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 2d7e7be538..230442e921 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3095,6 +3095,55 @@ void qmp_block_dirty_bitmap_merge(const char *node, const char *target,
     do_block_dirty_bitmap_merge(node, target, bitmaps, NULL, errp);
 }
 
+void qmp_block_dirty_bitmap_persist(const char *node, const char *name,
+                                    bool persist, Error **errp)
+{
+    BdrvDirtyBitmap *bitmap;
+    BlockDriverState *bs;
+    AioContext *aio_context = NULL;
+    Error *local_err = NULL;
+    bool persistent;
+
+    bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp);
+    if (!bitmap || !bs) {
+        return;
+    }
+
+    if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
+        return;
+    }
+
+    persistent = bdrv_dirty_bitmap_get_persistence(bitmap);
+
+    if (persist != persistent) {
+        aio_context = bdrv_get_aio_context(bs);
+        aio_context_acquire(aio_context);
+    }
+
+    if (!persist && persistent) {
+        bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err);
+        if (local_err != NULL) {
+            error_propagate(errp, local_err);
+            goto out;
+        }
+    }
+
+    if (persist && !persistent) {
+        uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
+        if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) {
+            goto out;
+        }
+    }
+
+    bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
+
+ out:
+    if (aio_context) {
+        aio_context_release(aio_context);
+    }
+    return;
+}
+
 BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
                                                               const char *name,
                                                               Error **errp)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 3dbf23d874..9c0957f528 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2001,6 +2001,19 @@
   'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
             '*persistent': 'bool', '*autoload': 'bool', '*disabled': 'bool' } }
 
+##
+# @BlockDirtyBitmapPersist:
+#
+# @persist: True sets the specified bitmap as persistent.
+#           False will remove it from storage and mark it transient.
+#
+# Since: 4.2
+##
+{ 'struct': 'BlockDirtyBitmapPersist',
+  'base': 'BlockDirtyBitmap',
+  'data': { 'persist': 'bool' }
+}
+
 ##
 # @BlockDirtyBitmapMergeSource:
 #
@@ -2173,6 +2186,27 @@
       { 'command': 'block-dirty-bitmap-merge',
         'data': 'BlockDirtyBitmapMerge' }
 
+##
+# @block-dirty-bitmap-persist:
+#
+# Mark a dirty bitmap as either persistent or transient.
+#
+# Returns: nothing on success; including for no-op.
+#          GenericError with explanation if the operation did not succeed.
+#
+# Example:
+#
+# -> { "execute": "block-dirty-bitmap-persist",
+#      "arguments": { "node": "drive0",
+#                     "bitmap": "bitmap0",
+#                     "persist": true } }
+# <- { "return": {} }
+#
+# Since: 4.2
+##
+{ 'command': 'block-dirty-bitmap-persist',
+  'data': 'BlockDirtyBitmapPersist' }
+
 ##
 # @BlockDirtyBitmapSha256:
 #
-- 
2.21.0

On 8/13/19 5:44 PM, John Snow wrote:
> This is for the purpose of toggling on/off persistence on a bitmap.
> This enables you to save a bitmap that was not persistent, but may
> have already accumulated valuable data.
> 
> This is simply a QOL enhancement:
> - Allows user to "upgrade" an existing bitmap to persistent
> - Allows user to "downgrade" an existing bitmap to transient,
>   removing it from storage without deleting the bitmap.
> 

In the meantime, a workaround is:

create tmp bitmap (non-persistent is fine)
merge existing bitmap into tmp bitmap
delete existing bitmap
recreate original bitmap with desired change in persistence
merge tmp bitmap into re-created original bitmap
delete tmp bitmap

(I'm not sure how much, if any of that, has to be done with a
transaction; ideally none, since merging two bitmaps that are both
enabled is not going to lose any bits.  And since one of the two ends of
the transaction has a non-persistent bitmap, qemu failing in the narrow
window where the original bitmap does not exist at all is not that much
different from failing while the bitmap is transient. If losing data due
to qemu failure was important, the bitmap should never have been
transient in the first place)

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> 
> This is just an RFC because I'm not sure if I really want to pursue
> adding this, but it was raised in a discussion I had recently that it
> was a little annoying as an API design that persistence couldn't be
> changed after addition, so I wanted to see how much code it would take
> to address that.
> 
> (So this patch isn't really tested; just: "Hey, look!")
> 
> I don't like this patch because it exacerbates my perceived problems
> with the "check if I can make it persistent, then toggle the flag"
> model, where I prefer the "Just try to set it persistent and let it fail
> if it cannot" model, but there were some issues with that patchset that
> I want to revisit.

The idea itself makes sense. I don't know if libvirt would ever use it,
but it does seem like it could make hand-management of bitmaps easier to
reason about.

> +++ b/qapi/block-core.json
> @@ -2001,6 +2001,19 @@
>    'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
>              '*persistent': 'bool', '*autoload': 'bool', '*disabled': 'bool' } }
>  
> +##
> +# @BlockDirtyBitmapPersist:

The QAPI additions look fine to me, regardless of whether you respin the
code based on review there.

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

On 8/13/19 8:08 PM, Eric Blake wrote:
> On 8/13/19 5:44 PM, John Snow wrote:
>> This is for the purpose of toggling on/off persistence on a bitmap.
>> This enables you to save a bitmap that was not persistent, but may
>> have already accumulated valuable data.
>>
>> This is simply a QOL enhancement:
>> - Allows user to "upgrade" an existing bitmap to persistent
>> - Allows user to "downgrade" an existing bitmap to transient,
>>   removing it from storage without deleting the bitmap.
>>
> 
> In the meantime, a workaround is:
> 
> create tmp bitmap (non-persistent is fine)
> merge existing bitmap into tmp bitmap
> delete existing bitmap
> recreate original bitmap with desired change in persistence
> merge tmp bitmap into re-created original bitmap
> delete tmp bitmap
> 

Merge really lets us get away with a lot :) It's a powerful command. And
now that merge supports cross-granularities, you can even use it to
change the granularity of a bitmap.

> (I'm not sure how much, if any of that, has to be done with a
> transaction; ideally none, since merging two bitmaps that are both
> enabled is not going to lose any bits.  And since one of the two ends of
> the transaction has a non-persistent bitmap, qemu failing in the narrow
> window where the original bitmap does not exist at all is not that much
> different from failing while the bitmap is transient. If losing data due
> to qemu failure was important, the bitmap should never have been
> transient in the first place)
> 

Yup, quite a lengthy workaround, but it _IS_ possible, it's just not
very nice.

>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>
>> This is just an RFC because I'm not sure if I really want to pursue
>> adding this, but it was raised in a discussion I had recently that it
>> was a little annoying as an API design that persistence couldn't be
>> changed after addition, so I wanted to see how much code it would take
>> to address that.
>>
>> (So this patch isn't really tested; just: "Hey, look!")
>>
>> I don't like this patch because it exacerbates my perceived problems
>> with the "check if I can make it persistent, then toggle the flag"
>> model, where I prefer the "Just try to set it persistent and let it fail
>> if it cannot" model, but there were some issues with that patchset that
>> I want to revisit.
> 
> The idea itself makes sense. I don't know if libvirt would ever use it,
> but it does seem like it could make hand-management of bitmaps easier to
> reason about.
> 

Right, this isn't for libvirt as much as it is people doing manual
configuration with e.g. qmp-shell (or heaven forbid, their own QMP tooling.)

>> +++ b/qapi/block-core.json
>> @@ -2001,6 +2001,19 @@
>>    'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
>>              '*persistent': 'bool', '*autoload': 'bool', '*disabled': 'bool' } }
>>  
>> +##
>> +# @BlockDirtyBitmapPersist:
> 
> The QAPI additions look fine to me, regardless of whether you respin the
> code based on review there.
> 

Thanks, just wanted this one out there on the record.

--js