[PATCH v6 18/25] parallels: implement removing a stored dirty bitmap

From: Denis V. Lunev <[email protected]>

block-dirty-bitmap-remove leaves a parallels image alone. The driver has
no bdrv_co_remove_persistent_dirty_bitmap(), and a missing handler means
nothing to do, so the command reports success while the bitmap stays in
the image until the node is inactivated. qcow2 updates its bitmap
directory on the spot.

Implement the handler. The Format Extension is written as a whole rather
than edited in place, so the bitmap is dropped by clearing its
persistence and storing what is left, and the header is updated to point
at the new extension. Releasing the bitmap itself is up to the caller.
The store runs under s->lock, as it allocates a cluster and a request
can be in flight, unlike the inactivation path where the node is
quiesced already.

Removing a bitmap which was never stored is not an error, as
bdrv_co_remove_persistent_dirty_bitmap() spells out, so a name which is
not there, or is not persistent, is answered with success and no write.

Write access is checked first. The extension is only rewritten while the
node is writable, so on a read-only or an inactive node the request could
not reach the image at all and reporting success would be a lie. The
generic BDRV_BITMAP_RO check in block_dirty_bitmap_remove() hides this
for a read-only node, but it does not cover an inactive one, and
dropping a bitmap which was never stored needs no write access at all.

Persistence is only dropped for the duration of the rewrite, so it goes
back on when the rewrite fails. The bitmap is still in the image, and a
bitmap which is no longer marked persistent would be dropped for good by
the next inactivation, turning a removal the caller was told had failed
into one which silently succeeded.

Cc: Stefan Hajnoczi <[email protected]>
Signed-off-by: Denis V. Lunev <[email protected]>
---
 block/parallels-ext.c | 47 +++++++++++++++++++++++++++++++++++++++++++
 block/parallels.c     |  4 +++-
 block/parallels.h     |  4 ++++
 3 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index b7fac2514a..f687f2da7c 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -689,3 +689,50 @@ bool coroutine_fn parallels_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
 
     return true;
 }
+
+int coroutine_fn
+parallels_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+                                            const char *name, Error **errp)
+{
+    BDRVParallelsState *s = bs->opaque;
+    BdrvDirtyBitmap *bitmap;
+    Error *err = NULL;
+    int ret;
+
+    if (bdrv_is_read_only(bs) || (bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
+        error_setg(errp, "Cannot remove persistent bitmap '%s': no write "
+                   "access to node '%s'", name, bdrv_get_node_name(bs));
+        return -EACCES;
+    }
+
+    bitmap = bdrv_find_dirty_bitmap(bs, name);
+    if (bitmap == NULL || !bdrv_dirty_bitmap_get_persistence(bitmap)) {
+        return 0;
+    }
+
+    /* The extension is written as a whole, so drop it from what goes in */
+    bdrv_dirty_bitmap_set_persistence(bitmap, false);
+
+    ret = 0;
+    WITH_QEMU_LOCK_GUARD(&s->lock) {
+        parallels_store_persistent_dirty_bitmaps(bs, &err);
+        if (err != NULL) {
+            error_propagate(errp, err);
+            ret = -EIO;
+            break;
+        }
+
+        ret = parallels_update_header(bs);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Failed to update the image header");
+            break;
+        }
+    }
+
+    if (ret < 0) {
+        /* Nothing was removed, so the bitmap is as persistent as it was */
+        bdrv_dirty_bitmap_set_persistence(bitmap, true);
+    }
+
+    return ret;
+}
diff --git a/block/parallels.c b/block/parallels.c
index 90b7f7c8de..2a5ceb8978 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1288,7 +1288,7 @@ static int parallels_probe(const uint8_t *buf, int buf_size,
     return 0;
 }
 
-static int GRAPH_RDLOCK parallels_update_header(BlockDriverState *bs)
+int GRAPH_RDLOCK parallels_update_header(BlockDriverState *bs)
 {
     BDRVParallelsState *s = bs->opaque;
     unsigned size = MAX(bdrv_opt_mem_align(bs->file->bs),
@@ -1651,6 +1651,8 @@ static BlockDriver bdrv_parallels = {
     .bdrv_inactivate            = parallels_inactivate,
     .bdrv_co_can_store_new_dirty_bitmap =
                                   parallels_co_can_store_new_dirty_bitmap,
+    .bdrv_co_remove_persistent_dirty_bitmap =
+                                  parallels_co_remove_persistent_dirty_bitmap,
 };
 
 static void bdrv_parallels_init(void)
diff --git a/block/parallels.h b/block/parallels.h
index 4684ba2890..27d8c3ac83 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -98,6 +98,7 @@ int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
 
 int64_t GRAPH_RDLOCK parallels_allocate_host_clusters(BlockDriverState *bs,
                                                       int64_t *clusters);
+int GRAPH_RDLOCK parallels_update_header(BlockDriverState *bs);
 
 int GRAPH_RDLOCK
 parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
@@ -107,5 +108,8 @@ parallels_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 bool coroutine_fn GRAPH_RDLOCK
 parallels_co_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
                                         uint32_t granularity, Error **errp);
+int coroutine_fn GRAPH_RDLOCK
+parallels_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
+                                            const char *name, Error **errp);
 
 #endif
-- 
2.53.0