[Qemu-devel] [PATCH 4/5] block/qcow2-bitmap: Count queued bitmaps towards nb_bitmaps

[John Snow]

When we check to see if we can store a bitmap, we don't check how many
we've queued up. This can cause a problem saving bitmaps on close
instead of when we request them to be added. With the stricter add
interface, prohibit these bitmaps specifically.

To match, make the remove interface more strict as well; now rejecting
any requests to remove bitmaps that were never queued for storage.

We don't need to "find" the bitmap when the interface has been given the
bitmap explicitly, but this is done to make sure that the bitmap given
actually does belong to the bs we were passed as a paranoia check to
enforce consistency.

---

"What about directory size?" Please see the following patch.

Signed-off-by: John Snow <address@hidden>
---
 block/qcow2.h        |  1 +
 block/dirty-bitmap.c |  8 +++-----
 block/qcow2-bitmap.c | 31 ++++++++++++++++++++++++++-----
 3 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index ce07f003f7..ebf60ac236 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -317,6 +317,7 @@ typedef struct BDRVQcow2State {
     QCowSnapshot *snapshots;
 
     uint32_t nb_bitmaps;
+    uint32_t nb_queued_bitmaps;
     uint64_t bitmap_directory_size;
     uint64_t bitmap_directory_offset;
 
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 4667f9e65a..084c42af57 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -450,11 +450,9 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
 }
 
 /**
- * Remove persistent dirty bitmap from the storage if it exists.
- * Absence of bitmap is not an error, because we have the following scenario:
- * BdrvDirtyBitmap can have .persistent = true but not yet saved and have no
- * stored version. For such bitmap bdrv_remove_persistent_dirty_bitmap() should
- * not fail.
+ * Remove a persistent dirty bitmap from storage,
+ * or dequeue it from being stored if it is enqueued.
+ *
  * This function doesn't release the corresponding BdrvDirtyBitmap.
  */
 int bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 930a6c91ff..7193c66787 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1402,6 +1402,23 @@ static Qcow2Bitmap *find_bitmap_by_name(Qcow2BitmapList 
*bm_list,
     return NULL;
 }
 
+static int qcow2_remove_queued_dirty_bitmap(
+    BlockDriverState *bs, const char *name, Error **errp)
+{
+    BDRVQcow2State *s = bs->opaque;
+    BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, name);
+    if (!bitmap) {
+        error_setg(errp, "Node '%s' has no stored or enqueued bitmap '%s'",
+                   bdrv_get_node_name(bs), name);
+        return -ENOENT;
+    }
+    assert(s->nb_queued_bitmaps > 0);
+    assert(bdrv_dirty_bitmap_get_persistence(bitmap));
+    s->nb_queued_bitmaps -= 1;
+
+    return 0;
+}
+
 int qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
                                          BdrvDirtyBitmap *bitmap,
                                          Error **errp)
@@ -1413,9 +1430,7 @@ int qcow2_remove_persistent_dirty_bitmap(BlockDriverState 
*bs,
     const char *name = bdrv_dirty_bitmap_name(bitmap);
 
     if (s->nb_bitmaps == 0) {
-        /* Absence of the bitmap is not an error: see explanation above
-         * bdrv_remove_persistent_dirty_bitmap() definition. */
-        return 0;
+        return qcow2_remove_queued_dirty_bitmap(bs, name, errp);
     }
 
     if ((ret = bitmap_list_load(bs, &bm_list, errp))) {
@@ -1424,6 +1439,7 @@ int qcow2_remove_persistent_dirty_bitmap(BlockDriverState 
*bs,
 
     bm = find_bitmap_by_name(bm_list, name);
     if (bm == NULL) {
+        ret = qcow2_remove_queued_dirty_bitmap(bs, name, errp);
         goto fail;
     }
 
@@ -1544,6 +1560,7 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
         error_setg_errno(errp, -ret, "Failed to update bitmap extension");
         goto fail;
     }
+    s->nb_queued_bitmaps = 0;
 
     /* Bitmap directory was successfully updated, so, old data can be dropped.
      * TODO it is better to reuse these clusters */
@@ -1618,6 +1635,7 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState 
*bs,
     Qcow2BitmapList *bm_list;
     const char *name = bdrv_dirty_bitmap_name(bitmap);
     uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
+    uint32_t nb_bitmaps;
     int ret = 0;
 
     if (s->qcow_version < 3) {
@@ -1636,11 +1654,12 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState 
*bs,
         goto fail;
     }
 
-    if (s->nb_bitmaps == 0) {
+    nb_bitmaps = s->nb_bitmaps + s->nb_queued_bitmaps;
+    if (nb_bitmaps == 0) {
         return 0;
     }
 
-    if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) {
+    if (nb_bitmaps >= QCOW2_MAX_BITMAPS) {
         error_setg(errp,
                    "Maximum number of persistent bitmaps is already reached");
         ret = -EOVERFLOW;
@@ -1667,6 +1686,8 @@ int qcow2_add_persistent_dirty_bitmap(BlockDriverState 
*bs,
         goto fail;
     }
 
+    s->nb_queued_bitmaps += 1;
+
     return 0;
 fail:
     error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ",
-- 
2.20.1