qemu-img: add bitmap queries

We don't need to re-read this list every time, exactly. We can keep it cached and delete our copy when we flush to disk.

Because we don't try to flush bitmaps on close if there's nothing to flush, add a new conditional to delete the state anyway for a clean exit.

Signed-off-by John Snow <[email protected]>

---
  block/qcow2-bitmap.c | 74 ++++++++++++++++++++++++++++++++--------------------
  block/qcow2.c        |  2 ++
  block/qcow2.h        |  2 ++
  3 files changed, 50 insertions(+), 28 deletions(-)

block/qcow2-bitmap.c +1 -2

index 6e93ec43e1..fb0a4f3ec4 100644
@@ -536,8 +536,7 @@ static uint32_t bitmap_list_count(Qcow2BitmapList *bm_list)
   * Get bitmap list from qcow2 image. Actually reads bitmap directory,
   * checks it and convert to bitmap list.
   */
-static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
-                                         uint64_t size, Error **errp)
+static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error **errp)
  {
      int ret;
      BDRVQcow2State *s = bs->opaque;

block/qcow2-bitmap.c +43 -26

      if (size == 0) {
          error_setg(errp, "Requested bitmap directory size is zero");

@@ -636,6 +637,30 @@ fail:
      return NULL;
  }
 
+static Qcow2BitmapList *get_bitmap_list(BlockDriverState *bs, Error **errp)
+{
+    BDRVQcow2State *s = bs->opaque;
+    Qcow2BitmapList *bm_list;
+
+    if (s->bitmap_list) {
+        return (Qcow2BitmapList *)s->bitmap_list;
+    }
+
+    bm_list = bitmap_list_load(bs, errp);
+    s->bitmap_list = bm_list;
+    return bm_list;
+}
+
+static void del_bitmap_list(BlockDriverState *bs)
+{
+    BDRVQcow2State *s = bs->opaque;
+
+    if (s->bitmap_list) {
+        bitmap_list_free(s->bitmap_list);
+        s->bitmap_list = NULL;
+    }
+}
+
  int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                                    void **refcount_table,
                                    int64_t *refcount_table_size)

@@ -656,8 +681,7 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
          return ret;
      }
 
-    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, NULL);
+    bm_list = get_bitmap_list(bs, NULL);
      if (bm_list == NULL) {
          res->corruptions++;
          return -EINVAL;

@@ -707,8 +731,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
      }
 
  out:
-    bitmap_list_free(bm_list);
-
      return ret;
  }
 

@@ -953,8 +975,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
          return false;
      }
 
-    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, errp);
+    bm_list = get_bitmap_list(bs, errp);
      if (bm_list == NULL) {
          return false;
      }

@@ -992,14 +1013,12 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
      }
 
      g_slist_free(created_dirty_bitmaps);
-    bitmap_list_free(bm_list);
-
      return header_updated;
 
  fail:
      g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs);
      g_slist_free(created_dirty_bitmaps);
-    bitmap_list_free(bm_list);
+    del_bitmap_list(bs);
 
      return false;
  }

@@ -1027,8 +1046,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
          return -EINVAL;
      }
 
-    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, errp);
+    bm_list = get_bitmap_list(bs, errp);
      if (bm_list == NULL) {
          return -EINVAL;
      }

@@ -1068,7 +1086,6 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
 
  out:
      g_slist_free(ro_dirty_bitmaps);
-    bitmap_list_free(bm_list);
 
      return ret;
  }

@@ -1277,8 +1294,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
          return;
      }
 
-    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, errp);
+    bm_list = get_bitmap_list(bs, errp);
      if (bm_list == NULL) {
          return;
      }

@@ -1300,7 +1316,11 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
 
  fail:
      bitmap_free(bm);
-    bitmap_list_free(bm_list);
+}
+
+void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs)
+{
+    del_bitmap_list(bs);
  }
 
  void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)

@@ -1317,12 +1337,12 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 
      if (!bdrv_has_changed_persistent_bitmaps(bs)) {
          /* nothing to do */
-        return;
+        goto out;
      }
 
      if (!can_write(bs)) {
          error_setg(errp, "No write access");
-        return;
+        goto out;
      }
 
      QSIMPLEQ_INIT(&drop_tables);

@@ -1330,10 +1350,9 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
      if (s->nb_bitmaps == 0) {
          bm_list = bitmap_list_new();
      } else {
-        bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                                   s->bitmap_directory_size, errp);
+        bm_list = get_bitmap_list(bs, errp);
          if (bm_list == NULL) {
-            return;
+            goto out;
          }
      }
 

@@ -1414,8 +1433,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
          g_free(tb);
      }
 
-    bitmap_list_free(bm_list);
-    return;
+    goto out;
 
  fail:
      QSIMPLEQ_FOREACH(bm, bm_list, entry) {

@@ -1430,7 +1448,9 @@ fail:
          g_free(tb);
      }
 
-    bitmap_list_free(bm_list);
+ out:
+    del_bitmap_list(bs);
+    return;
  }
 
  int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)

@@ -1495,14 +1515,12 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
          goto fail;
      }
 
-    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, errp);
+    bm_list = get_bitmap_list(bs, errp);
      if (bm_list == NULL) {
          goto fail;
      }
 
      found = find_bitmap_by_name(bm_list, name);
-    bitmap_list_free(bm_list);
      if (found) {
          error_setg(errp, "Bitmap with the same name is already stored");
          goto fail;

block/qcow2.c +2 -0

index 2f36e632f9..7ae9000656 100644
@@ -2135,6 +2135,8 @@ static void qcow2_close(BlockDriverState *bs)
 
      if (!(s->flags & BDRV_O_INACTIVE)) {
          qcow2_inactivate(bs);
+    } else {
+        qcow2_persistent_dirty_bitmaps_cache_destroy(bs);
      }
 
      cache_clean_timer_del(bs);

block/qcow2.h +2 -0

index adf5c3950f..796a8c914b 100644
@@ -299,6 +299,7 @@ typedef struct BDRVQcow2State {
      uint64_t bitmap_directory_size;
      uint64_t bitmap_directory_offset;
      bool dirty_bitmaps_loaded;
+    void *bitmap_list;
 
      int flags;
      int qcow_version;

@@ -675,6 +676,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
  int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
                                   Error **errp);
  int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
+void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs);
  void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
  int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
  bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,

Quoted 2 lines · John Snow

We don't need to re-read this list every time, exactly. We can keep it cached and delete our copy when we flush to disk.

Why not simply delete cache only on close (unconditionally)? Why do we need to remove it after flush?

Actually, I think we need to remove it only in qcow2_inactive, after storing persistent bitmaps.

Quoted 30 lines

Because we don't try to flush bitmaps on close if there's nothing to flush, add a new conditional to delete the state anyway for a clean exit.

Signed-off-by John Snow <[email protected]>

---
  block/qcow2-bitmap.c | 74
++++++++++++++++++++++++++++++++--------------------
  block/qcow2.c        |  2 ++
  block/qcow2.h        |  2 ++

  3 files changed, 50 insertions(+), 28 deletions(-)

block/qcow2-bitmap.c +1 -2

index 6e93ec43e1..fb0a4f3ec4 100644
@@ -536,8 +536,7 @@ static uint32_t bitmap_list_count(Qcow2BitmapList 
 *bm_list)
    * Get bitmap list from qcow2 image. Actually reads bitmap directory,
    * checks it and convert to bitmap list.
    */
-static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, 
 uint64_t offset,
-                                         uint64_t size, Error **errp)
+static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error 
 **errp)

  {
      int ret;
      BDRVQcow2State *s = bs->opaque;

block/qcow2-bitmap.c static Qcow2BitmapList · line 549

Quoted 6 lines

block/qcow2-bitmap.c +2 -0

@@ -545,6 +544,8 @@ static Qcow2BitmapList 
 *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
       Qcow2BitmapDirEntry *e;
       uint32_t nb_dir_entries = 0;
       Qcow2BitmapList *bm_list = NULL;
+    uint64_t offset = s->bitmap_directory_offset;
+    uint64_t size = s->bitmap_directory_size;

Worth split this change as a refactoring patch (just remove extra parameters)?

Quoted 20 lines

block/qcow2-bitmap.c +14 -0

         if (size == 0) {
           error_setg(errp, "Requested bitmap directory size is zero");

@@ -636,6 +637,30 @@ fail:
       return NULL;
   }
   +static Qcow2BitmapList *get_bitmap_list(BlockDriverState *bs, Error **errp)
+{
+    BDRVQcow2State *s = bs->opaque;
+    Qcow2BitmapList *bm_list;
+
+    if (s->bitmap_list) {
+        return (Qcow2BitmapList *)s->bitmap_list;
+    }
+
+    bm_list = bitmap_list_load(bs, errp);
+    s->bitmap_list = bm_list;
+    return bm_list;
+}
+
+static void del_bitmap_list(BlockDriverState *bs)

block/qcow2-bitmap.c fail: · line 662

Quoted 8 lines

block/qcow2-bitmap.c +8 -0

+{
+    BDRVQcow2State *s = bs->opaque;
+
+    if (s->bitmap_list) {
+        bitmap_list_free(s->bitmap_list);
+        s->bitmap_list = NULL;
+    }
+}

Quoted 174 lines

block/qcow2-bitmap.c +2 -1

+
   int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                                     void **refcount_table,
                                     int64_t *refcount_table_size)

@@ -656,8 +681,7 @@ int 
 qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
           return ret;
       }
   -    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, NULL);
+    bm_list = get_bitmap_list(bs, NULL);
       if (bm_list == NULL) {
           res->corruptions++;

          return -EINVAL;

block/qcow2-bitmap.c +0 -0

@@ -707,8 +731,6 @@ int 
 qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
       }

    out:
-    bitmap_list_free(bm_list);
-
      return ret;
  }
  @@ -953,8 +975,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp) return false;
      }
  -    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, errp);
+    bm_list = get_bitmap_list(bs, errp);
      if (bm_list == NULL) {
          return false;
      }

block/qcow2-bitmap.c +0 -2

@@ -992,14 +1013,12 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState 
 *bs, Error **errp)
       }
         g_slist_free(created_dirty_bitmaps);
-    bitmap_list_free(bm_list);
-
       return header_updated;

    fail:
      g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs); g_slist_free(created_dirty_bitmaps);
-    bitmap_list_free(bm_list);
+    del_bitmap_list(bs);
        return false;
  }

block/qcow2-bitmap.c +1 -1

@@ -1027,8 +1046,7 @@ int 
 qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
           return -EINVAL;
       }
   -    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, errp);
+    bm_list = get_bitmap_list(bs, errp);
       if (bm_list == NULL) {
           return -EINVAL;

      }

block/qcow2-bitmap.c +0 -0

@@ -1068,7 +1086,6 @@ int 
 qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,

    out:
      g_slist_free(ro_dirty_bitmaps);
-    bitmap_list_free(bm_list);
        return ret;
  }

block/qcow2-bitmap.c +1 -1

@@ -1277,8 +1294,7 @@ void 
 qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
           return;
       }
   -    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, errp);
+    bm_list = get_bitmap_list(bs, errp);
       if (bm_list == NULL) {
           return;

      }

block/qcow2-bitmap.c +0 -0

@@ -1300,7 +1316,11 @@ void 
 qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,

    fail:
      bitmap_free(bm);
-    bitmap_list_free(bm_list);

block/qcow2-bitmap.c +5 -0

+}
+
+void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState *bs)
+{
+    del_bitmap_list(bs);

  }
    void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)

block/qcow2-bitmap.c +4 -5

@@ -1317,12 +1337,12 @@ void 
 qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
         if (!bdrv_has_changed_persistent_bitmaps(bs)) {
           /* nothing to do */
-        return;
+        goto out;
       }
         if (!can_write(bs)) {
           error_setg(errp, "No write access");
-        return;
+        goto out;
       }
         QSIMPLEQ_INIT(&drop_tables);

@@ -1330,10 +1350,9 @@ void 
 qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
       if (s->nb_bitmaps == 0) {
           bm_list = bitmap_list_new();
       } else {
-        bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                                   s->bitmap_directory_size, errp);
+        bm_list = get_bitmap_list(bs, errp);
           if (bm_list == NULL) {
-            return;
+            goto out;
           }
       }

  @@ -1414,8 +1433,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) g_free(tb);
      }
  -    bitmap_list_free(bm_list);
-    return;
+    goto out;
    fail:
      QSIMPLEQ_FOREACH(bm, bm_list, entry) {

block/qcow2-bitmap.c +4 -2

@@ -1430,7 +1448,9 @@ fail:
           g_free(tb);
       }
   -    bitmap_list_free(bm_list);
+ out:
+    del_bitmap_list(bs);
+    return;
   }
     int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)

@@ -1495,14 +1515,12 @@ bool 
 qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
           goto fail;
       }
   -    bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
-                               s->bitmap_directory_size, errp);
+    bm_list = get_bitmap_list(bs, errp);
       if (bm_list == NULL) {
           goto fail;
       }
         found = find_bitmap_by_name(bm_list, name);
-    bitmap_list_free(bm_list);
       if (found) {
           error_setg(errp, "Bitmap with the same name is already stored");
           goto fail;

block/qcow2.c +2 -0

index 2f36e632f9..7ae9000656 100644
@@ -2135,6 +2135,8 @@ static void qcow2_close(BlockDriverState *bs)
         if (!(s->flags & BDRV_O_INACTIVE)) {
           qcow2_inactivate(bs);
+    } else {
+        qcow2_persistent_dirty_bitmaps_cache_destroy(bs);
       }
         cache_clean_timer_del(bs);

block/qcow2.h +2 -0

index adf5c3950f..796a8c914b 100644
@@ -299,6 +299,7 @@ typedef struct BDRVQcow2State {
       uint64_t bitmap_directory_size;
       uint64_t bitmap_directory_offset;
       bool dirty_bitmaps_loaded;
+    void *bitmap_list;
         int flags;
       int qcow_version;

@@ -675,6 +676,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState 
 *bs, Error **errp);
   int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
                                    Error **errp);
   int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
+void qcow2_persistent_dirty_bitmaps_cache_destroy(BlockDriverState 
 *bs);
   void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);

  int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp); bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,

Quoted 80 lines · Vladimir Sementsov-Ogievskiy

Quoted 3 lines · John Snow

We don't need to re-read this list every time, exactly. We can keep it cached
and delete our copy when we flush to disk.

Why not simply delete cache only on close (unconditionally)? Why do we need to remove it after flush?

Actually, I think we need to remove it only in qcow2_inactive, after storing persistent bitmaps.

Quoted 31 lines

Because we don't try to flush bitmaps on close if there's nothing to flush,
add a new conditional to delete the state anyway for a clean exit.

Signed-off-by John Snow <[email protected]>

---
  block/qcow2-bitmap.c | 74
++++++++++++++++++++++++++++++++--------------------
  block/qcow2.c        |  2 ++
  block/qcow2.h        |  2 ++

  3 files changed, 50 insertions(+), 28 deletions(-)

block/qcow2-bitmap.c +1 -2

index 6e93ec43e1..fb0a4f3ec4 100644
@@ -536,8 +536,7 @@ static uint32_t bitmap_list_count(Qcow2BitmapList
 *bm_list)
    * Get bitmap list from qcow2 image. Actually reads bitmap directory,
    * checks it and convert to bitmap list.
    */
-static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs,
 uint64_t offset,
-                                         uint64_t size, Error **errp)
+static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error
 **errp)

  {
      int ret;
      BDRVQcow2State *s = bs->opaque;

block/qcow2-bitmap.c static Qcow2BitmapList · line 549

Quoted 6 lines

block/qcow2-bitmap.c +2 -0

@@ -545,6 +544,8 @@ static Qcow2BitmapList
 *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
       Qcow2BitmapDirEntry *e;
       uint32_t nb_dir_entries = 0;
       Qcow2BitmapList *bm_list = NULL;
+    uint64_t offset = s->bitmap_directory_offset;
+    uint64_t size = s->bitmap_directory_size;

Worth split this change as a refactoring patch (just remove extra parameters)?

Quoted 27 lines

block/qcow2-bitmap.c +20 -0

         if (size == 0) {
           error_setg(errp, "Requested bitmap directory size is zero");

@@ -636,6 +637,30 @@ fail:
       return NULL;
   }
   +static Qcow2BitmapList *get_bitmap_list(BlockDriverState *bs,
 Error **errp)
+{
+    BDRVQcow2State *s = bs->opaque;
+    Qcow2BitmapList *bm_list;
+
+    if (s->bitmap_list) {
+        return (Qcow2BitmapList *)s->bitmap_list;
+    }
+
+    bm_list = bitmap_list_load(bs, errp);
+    s->bitmap_list = bm_list;
+    return bm_list;
+}
+
+static void del_bitmap_list(BlockDriverState *bs)
+{
+    BDRVQcow2State *s = bs->opaque;
+
+    if (s->bitmap_list) {
+        bitmap_list_free(s->bitmap_list);
+        s->bitmap_list = NULL;

Quoted 2 lines

block/qcow2-bitmap.c fail: · line 662

Quoted 2 lines

block/qcow2-bitmap.c +2 -0

+    }
+}

so, with this functions, we see, that list is always safely loaded through the cache. But we need also guarantee, that list is always saved through the cache. There are a lot of functions, which stores an abstract bitmap list, given as a parameter, but we want always store our cache..

Quoted 3 lines · John Snow

We don't need to re-read this list every time, exactly. We can keep it cached
and delete our copy when we flush to disk.

Why not simply delete cache only on close (unconditionally)? Why do we need to remove it after flush?

Actually, I think we need to remove it only in qcow2_inactive, after storing persistent bitmaps.

Quoted 33 lines

Because we don't try to flush bitmaps on close if there's nothing to flush,
add a new conditional to delete the state anyway for a clean exit.

Signed-off-by John Snow <[email protected]>

---
  block/qcow2-bitmap.c | 74
++++++++++++++++++++++++++++++++--------------------
  block/qcow2.c        |  2 ++
  block/qcow2.h        |  2 ++

  3 files changed, 50 insertions(+), 28 deletions(-)

block/qcow2-bitmap.c +1 -2

index 6e93ec43e1..fb0a4f3ec4 100644
@@ -536,8 +536,7 @@ static uint32_t bitmap_list_count(Qcow2BitmapList
 *bm_list)
    * Get bitmap list from qcow2 image. Actually reads bitmap directory,
    * checks it and convert to bitmap list.
    */
-static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs,
 uint64_t offset,
-                                         uint64_t size, Error **errp)
+static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error
 **errp)

  {
      int ret;
      BDRVQcow2State *s = bs->opaque;

block/qcow2-bitmap.c +0 -0

@@ -545,6 +544,8 @@ static Qcow2BitmapList
 *bitmap_list_load(BlockDriverState *bs, uint64_t offset,

block/qcow2-bitmap.c static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset, · line 548

Quoted 5 lines

block/qcow2-bitmap.c +2 -0

       Qcow2BitmapDirEntry *e;
       uint32_t nb_dir_entries = 0;
       Qcow2BitmapList *bm_list = NULL;
+    uint64_t offset = s->bitmap_directory_offset;
+    uint64_t size = s->bitmap_directory_size;

Worth split this change as a refactoring patch (just remove extra parameters)?

For bitmaps that we succeeded in loading, we can cache a reference to that object. This will let us iterate over the more convenient form of in-memory bitmaps for qemu-img bitmap manipulation tools.

Signed-off-by John Snow <[email protected]>

---
  block/qcow2-bitmap.c | 1 +
  1 file changed, 1 insertion(+)

block/qcow2-bitmap.c +0 -0

index fb0a4f3ec4..d89758f235 100644

Instead of always setting IN_USE, handle whether or not the bitmap is read-only instead of a two-loop pass. This will allow us to show the flags exactly as they appear for bitmaps in qemu-img info.

Signed-off-by John Snow <[email protected]>

---
  block/qcow2-bitmap.c | 48 ++++++++++++++++++++----------------------------
  1 file changed, 20 insertions(+), 28 deletions(-)

block/qcow2-bitmap.c +1 -16

index d89758f235..fc8d52fc3e 100644
@@ -942,13 +942,6 @@ fail:
      return ret;
  }
 
-/* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */
-static void release_dirty_bitmap_helper(gpointer bitmap,
-                                        gpointer bs)
-{
-    bdrv_release_dirty_bitmap(bs, bitmap);
-}
-
  /* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */
  static void set_readonly_helper(gpointer bitmap, gpointer value)
  {

@@ -967,8 +960,8 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
      BDRVQcow2State *s = bs->opaque;
      Qcow2BitmapList *bm_list;
      Qcow2Bitmap *bm;
-    GSList *created_dirty_bitmaps = NULL;
      bool header_updated = false;
+    bool needs_update = false;
 
      if (s->nb_bitmaps == 0) {
          /* No bitmaps - nothing to do */

@@ -992,33 +985,32 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
                  bdrv_disable_dirty_bitmap(bitmap);
              }
              bdrv_dirty_bitmap_set_persistance(bitmap, true);
-            bm->flags |= BME_FLAG_IN_USE;
-            created_dirty_bitmaps =
-                    g_slist_append(created_dirty_bitmaps, bitmap);
-        }
-    }
-
-    if (created_dirty_bitmaps != NULL) {
-        if (can_write(bs)) {

block/qcow2-bitmap.c +0 -5

              }
-            header_updated = true;
-        } else {
-            g_slist_foreach(created_dirty_bitmaps, set_readonly_helper,
-                            (gpointer)true);
          }
      }
 
-    g_slist_free(created_dirty_bitmaps);

Quoted 56 lines · John Snow

Instead of always setting IN_USE, handle whether or not the bitmap is read-only instead of a two-loop pass. This will allow us to show the flags exactly as they appear for bitmaps in qemu-img info.

Signed-off-by John Snow <[email protected]>

---
  block/qcow2-bitmap.c | 48
++++++++++++++++++++----------------------------

  1 file changed, 20 insertions(+), 28 deletions(-)

block/qcow2-bitmap.c +0 -0

index d89758f235..fc8d52fc3e 100644
@@ -942,13 +942,6 @@ fail:
       return ret;
   }

  -/* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */

block/qcow2-bitmap.c +0 -6

-static void release_dirty_bitmap_helper(gpointer bitmap,
-                                        gpointer bs)
-{
-    bdrv_release_dirty_bitmap(bs, bitmap);
-}
-

  /* for g_slist_foreach for GSList of BdrvDirtyBitmap* elements */ static void set_readonly_helper(gpointer bitmap, gpointer value) {

block/qcow2-bitmap.c +1 -14

@@ -967,8 +960,8 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState
 *bs, Error **errp)
       BDRVQcow2State *s = bs->opaque;
       Qcow2BitmapList *bm_list;
       Qcow2Bitmap *bm;
-    GSList *created_dirty_bitmaps = NULL;
       bool header_updated = false;
+    bool needs_update = false;
         if (s->nb_bitmaps == 0) {
           /* No bitmaps - nothing to do */

@@ -992,33 +985,32 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState
 *bs, Error **errp)
                   bdrv_disable_dirty_bitmap(bitmap);
               }
               bdrv_dirty_bitmap_set_persistance(bitmap, true);
-            bm->flags |= BME_FLAG_IN_USE;
-            created_dirty_bitmaps =
-                    g_slist_append(created_dirty_bitmaps, bitmap);
-        }
-    }
-
-    if (created_dirty_bitmaps != NULL) {
-        if (can_write(bs)) {
-            /* in_use flags must be updated */
-            int ret = update_ext_header_and_dir_in_place(bs, bm_list);
-            if (ret < 0) {
-                error_setg_errno(errp, -ret, "Can't update bitmap
 directory");
-                goto fail;

block/qcow2-bitmap.c bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp) · line 992

Quoted 5 lines

block/qcow2-bitmap.c +5 -0

+            if (can_write(bs)) {
+                bm->flags |= BME_FLAG_IN_USE;
+                needs_update = true;
+            } else {
+                bdrv_dirty_bitmap_set_readonly(bitmap, true);

oops. This shows, that patch 01 was incorrect: before this (03) patch, in this case we'll have IN_USE set in cache but not in the image.

Quoted 9 lines

block/qcow2-bitmap.c +1 -4

               }
-            header_updated = true;
-        } else {
-            g_slist_foreach(created_dirty_bitmaps, set_readonly_helper,
-                            (gpointer)true);
           }
       }
   -    g_slist_free(created_dirty_bitmaps);
+    /* in_use flags must be updated */

block/qcow2-bitmap.c bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp) · line 1006

Quoted 10 lines

block/qcow2-bitmap.c +8 -0

+    if (needs_update) {
+        int ret = update_ext_header_and_dir_in_place(bs, bm_list);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Can't update bitmap
 directory");
+            goto fail;
+        }
+        header_updated = true;
+    }
       return header_updated;

may be safely changed to return needs_update, as if we failed to update, we'll go to fail.

Quoted 11 lines

    fail:
-    g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs);
-    g_slist_free(created_dirty_bitmaps);
+    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+        if (bm->dirty_bitmap) {
+            bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap); +        }
+    }
      del_bitmap_list(bs);
        return false;

overall looks good, may be worth move it before 01.

For the purposes of qemu-img manipulation and querying of bitmaps, load bitmaps that are "in use" -- if the image is read only. This will allow us to diagnose problems with this flag using the qemu-img tool.

Signed-off-by John Snow <[email protected]>

---
  block/qcow2-bitmap.c | 32 +++++++++++++++-----------------
  1 file changed, 15 insertions(+), 17 deletions(-)

block/qcow2-bitmap.c +15 -17

index fc8d52fc3e..b556dbdccd 100644
@@ -346,7 +346,7 @@ static BdrvDirtyBitmap *load_bitmap(BlockDriverState *bs,
      uint32_t granularity;
      BdrvDirtyBitmap *bitmap = NULL;
 
-    if (bm->flags & BME_FLAG_IN_USE) {
+    if (can_write(bs) && (bm->flags & BME_FLAG_IN_USE)) {
          error_setg(errp, "Bitmap '%s' is in use", bm->name);
          goto fail;
      }

@@ -974,23 +974,21 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
      }
 
      QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-        if (!(bm->flags & BME_FLAG_IN_USE)) {
-            BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
-            if (bitmap == NULL) {
-                goto fail;
-            }
-            bm->dirty_bitmap = bitmap;
+        BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
+        if (bitmap == NULL) {
+            goto fail;
+        }
+        bm->dirty_bitmap = bitmap;
 
-            if (!(bm->flags & BME_FLAG_AUTO)) {
-                bdrv_disable_dirty_bitmap(bitmap);
-            }
-            bdrv_dirty_bitmap_set_persistance(bitmap, true);
-            if (can_write(bs)) {
-                bm->flags |= BME_FLAG_IN_USE;
-                needs_update = true;
-            } else {
-                bdrv_dirty_bitmap_set_readonly(bitmap, true);
-            }
+        if (!(bm->flags & BME_FLAG_AUTO)) {
+            bdrv_disable_dirty_bitmap(bitmap);
+        }
+        bdrv_dirty_bitmap_set_persistance(bitmap, true);
+        if (can_write(bs)) {
+            bm->flags |= BME_FLAG_IN_USE;
+            needs_update = true;
+        } else {
+            bdrv_dirty_bitmap_set_readonly(bitmap, true);
          }
      }
 

Quoted 3 lines · John Snow

For the purposes of qemu-img manipulation and querying of bitmaps, load bitmaps that are "in use" -- if the image is read only. This will allow us to diagnose problems with this flag using the qemu-img tool.

It looks unsafe.. We can load them, then reopen rw and then store them on vm close, the result would be unpredictable. And they become available for qmp commands..

If we want to load them, we may need some additional state for such dirty bitmaps, something like "invalid", to be sure, that they will not be used like normal bitmap. And we may
need some additional flag to load or not load them, and this behavior will be unrelated to can_write().

Add some of the necessary scaffolding for reporting bitmap information.

Signed-off-by John Snow <[email protected]>

---
  qapi/block-core.json | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 59 insertions(+), 1 deletion(-)

qapi/block-core.json +13 -0

index c50517bff3..8f33f41ce7 100644
@@ -33,6 +33,61 @@
              'date-sec': 'int', 'date-nsec': 'int',
              'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
 
+##
+# @BitmapTypeEnum:
+#
+# An enumeration of possible serialized bitmap types.
+#
+# @dirty-tracking: This bitmap records information on dirty
+#                  segments within the file.
+#
+# @unknown: This bitmap is of an unknown or reserved type.
+#
+# Since: 2.13
+##
+{ 'enum': 'BitmapTypeEnum', 'data': [ 'dirty-tracking', 'unknown' ] }

qapi/block-core.json +15 -0

+#
+# Since: 2.13
+##
+{ 'enum': 'BitmapFlagEnum', 'data': [ 'in-use', 'auto',
+                                      'extra-data-compatible', 'unknown' ] }
+
+##
+# @BitmapInfo:
+#
+# @name: The name of the bitmap.
+#
+# @type: The type of bitmap.
+#
+# @granularity: Bitmap granularity, in bytes.
+#

qapi/block-core.json +7 -1

+  }
+}
+
  ##
  # @ImageInfoSpecificQCow2EncryptionBase:
  #

@@ -69,6 +124,8 @@
  # @encrypt: details about encryption parameters; only set if image
  #           is encrypted (since 2.10)
  #
+# @bitmaps: list of image bitmaps (since 2.13)
+#
  # Since: 1.7
  ##
  { 'struct': 'ImageInfoSpecificQCow2',

@@ -77,7 +134,8 @@
        '*lazy-refcounts': 'bool',
        '*corrupt': 'bool',
        'refcount-bits': 'int',
-      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
+      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
+      '*bitmaps': ['BitmapInfo']
    } }
 
  ##

Quoted 35 lines · John Snow

Add some of the necessary scaffolding for reporting bitmap information.

Signed-off-by John Snow <[email protected]>

---
  qapi/block-core.json | 60
+++++++++++++++++++++++++++++++++++++++++++++++++++-

  1 file changed, 59 insertions(+), 1 deletion(-)

qapi/block-core.json +0 -0

index c50517bff3..8f33f41ce7 100644
@@ -33,6 +33,61 @@
               'date-sec': 'int', 'date-nsec': 'int',
               'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }

  +##

qapi/block-core.json +19 -0

+# @BitmapTypeEnum:
+#
+# An enumeration of possible serialized bitmap types.
+#
+# @dirty-tracking: This bitmap records information on dirty
+#                  segments within the file.
+#
+# @unknown: This bitmap is of an unknown or reserved type.
+#
+# Since: 2.13
+##
+{ 'enum': 'BitmapTypeEnum', 'data': [ 'dirty-tracking', 'unknown' ] }
+
+##
+# @BitmapFlagEnum:
+#
+# An enumeration of possible flags for serialized bitmaps.
+#
+# @in-use: This bitmap is considered to be in-use, and may now be

Quoted 5 lines

inconsistent.

qapi/block-core.json +2 -0

+#
+# @auto: This bitmap must reflect any and all changes to the file it

describes.
+#        The type of this bitmap must be @DirtyTrackingBitmap.

logical, but I don't see this restriction in the spec. May be we need to update the spec

Quoted 3 lines

qapi/block-core.json +2 -0

+#
+# @extra-data-compatible: This bitmap has extra information

associated with it.

no, this flag means, that extra data is compatible. So, if you don't know what is this extra data, you can read and modify the bitmap, leaving this data as is. If this flag is unset, and there are some extra data, bitmap must not be used.

Finally, this spec should be consistent (or, may be better, duplicate) spec from docs/interop/qcow2.txt..

qapi/block-core.json

Quoted 2 lines

qapi/block-core.json +2 -0

+#
+# @unknown: This bitmap has unknown or reserved properties.

Better is only "reserved flags" (not unknown and not properties), they are reserved by spec.

Quoted 21 lines

qapi/block-core.json +5 -0

+#
+# Since: 2.13
+##
+{ 'enum': 'BitmapFlagEnum', 'data': [ 'in-use', 'auto',
+                                      'extra-data-compatible',

'unknown' ] }

qapi/block-core.json +15 -0

+
+##
+# @BitmapInfo:
+#
+# @name: The name of the bitmap.
+#
+# @type: The type of bitmap.
+#
+# @granularity: Bitmap granularity, in bytes.
+#
+# @count: Overall bitmap dirtiness, in bytes.
+#
+# @flags: Bitmap flags, if any.
+#
+# Since: 2.13

Quoted 6 lines

qapi/block-core.json +4 -0

+#
+##
+{ 'struct': 'BitmapInfo',
+  'data': { 'name': 'str', 'type': 'BitmapTypeEnum', 'granularity':

'int',
+            'count': 'int', '*flags': ['BitmapFlagEnum']

may be worth add 'has-extra-data'

Quoted 24 lines

+  }

qapi/block-core.json +2 -0

+}
+

  ##
  # @ImageInfoSpecificQCow2EncryptionBase:
  #

qapi/block-core.json +4 -1

@@ -69,6 +124,8 @@
   # @encrypt: details about encryption parameters; only set if image
   #           is encrypted (since 2.10)
   #
+# @bitmaps: list of image bitmaps (since 2.13)
+#
   # Since: 1.7
   ##
   { 'struct': 'ImageInfoSpecificQCow2',

@@ -77,7 +134,8 @@
         '*lazy-refcounts': 'bool',
         '*corrupt': 'bool',
         'refcount-bits': 'int',
-      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
+      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
+      '*bitmaps': ['BitmapInfo']
     } }
     ##

Quoted 35 lines · John Snow

Add some of the necessary scaffolding for reporting bitmap information.

Signed-off-by John Snow <[email protected]>

---
  qapi/block-core.json | 60
+++++++++++++++++++++++++++++++++++++++++++++++++++-

  1 file changed, 59 insertions(+), 1 deletion(-)

qapi/block-core.json +0 -0

index c50517bff3..8f33f41ce7 100644
@@ -33,6 +33,61 @@
               'date-sec': 'int', 'date-nsec': 'int',
               'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }

  +##

qapi/block-core.json +19 -0

+# @BitmapTypeEnum:
+#
+# An enumeration of possible serialized bitmap types.
+#
+# @dirty-tracking: This bitmap records information on dirty
+#                  segments within the file.
+#
+# @unknown: This bitmap is of an unknown or reserved type.
+#
+# Since: 2.13
+##
+{ 'enum': 'BitmapTypeEnum', 'data': [ 'dirty-tracking', 'unknown' ] }
+
+##
+# @BitmapFlagEnum:
+#
+# An enumeration of possible flags for serialized bitmaps.
+#
+# @in-use: This bitmap is considered to be in-use, and may now be

Quoted 5 lines

inconsistent.

qapi/block-core.json +2 -0

+#
+# @auto: This bitmap must reflect any and all changes to the file it

describes.
+#        The type of this bitmap must be @DirtyTrackingBitmap.

logical, but I don't see this restriction in the spec. May be we need to update the spec

Quoted 3 lines

qapi/block-core.json +2 -0

+#
+# @extra-data-compatible: This bitmap has extra information

associated with it.

no, this flag means, that extra data is compatible. So, if you don't know what is this extra data, you can read and modify the bitmap, leaving this data as is. If this flag is unset, and there are some extra data, bitmap must not be used.

Finally, this spec should be consistent (or, may be better, duplicate) spec from docs/interop/qcow2.txt..

qapi/block-core.json

Quoted 2 lines

qapi/block-core.json +2 -0

+#
+# @unknown: This bitmap has unknown or reserved properties.

Better is only "reserved flags" (not unknown and not properties), they are reserved by spec.

Quoted 25 lines

qapi/block-core.json +5 -0

+#
+# Since: 2.13
+##
+{ 'enum': 'BitmapFlagEnum', 'data': [ 'in-use', 'auto',
+                                      'extra-data-compatible',

'unknown' ] }

qapi/block-core.json +19 -0

+
+##
+# @BitmapInfo:
+#
+# @name: The name of the bitmap.
+#
+# @type: The type of bitmap.
+#
+# @granularity: Bitmap granularity, in bytes.
+#
+# @count: Overall bitmap dirtiness, in bytes.
+#
+# @flags: Bitmap flags, if any.
+#
+# Since: 2.13
+#
+##
+{ 'struct': 'BitmapInfo',
+  'data': { 'name': 'str', 'type': 'BitmapTypeEnum', 'granularity':

Quoted 2 lines

'int',
+            'count': 'int', '*flags': ['BitmapFlagEnum']

may be worth add 'has-extra-data'

Quoted 24 lines

+  }

qapi/block-core.json +2 -0

+}
+

  ##
  # @ImageInfoSpecificQCow2EncryptionBase:
  #

qapi/block-core.json +4 -1

@@ -69,6 +124,8 @@
   # @encrypt: details about encryption parameters; only set if image
   #           is encrypted (since 2.10)
   #
+# @bitmaps: list of image bitmaps (since 2.13)
+#
   # Since: 1.7
   ##
   { 'struct': 'ImageInfoSpecificQCow2',

@@ -77,7 +134,8 @@
         '*lazy-refcounts': 'bool',
         '*corrupt': 'bool',
         'refcount-bits': 'int',
-      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
+      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
+      '*bitmaps': ['BitmapInfo']
     } }
     ##

Quoted 35 lines · Vladimir Sementsov-Ogievskiy

Quoted 34 lines · John Snow

Add some of the necessary scaffolding for reporting bitmap information.

Signed-off-by John Snow <[email protected]>

---
   qapi/block-core.json | 60
+++++++++++++++++++++++++++++++++++++++++++++++++++-
   1 file changed, 59 insertions(+), 1 deletion(-)

qapi/block-core.json +18 -0

index c50517bff3..8f33f41ce7 100644
@@ -33,6 +33,61 @@
               'date-sec': 'int', 'date-nsec': 'int',
               'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
   +##
+# @BitmapTypeEnum:
+#
+# An enumeration of possible serialized bitmap types.
+#
+# @dirty-tracking: This bitmap records information on dirty
+#                  segments within the file.
+#
+# @unknown: This bitmap is of an unknown or reserved type.
+#
+# Since: 2.13
+##
+{ 'enum': 'BitmapTypeEnum', 'data': [ 'dirty-tracking', 'unknown' ] }
+
+##
+# @BitmapFlagEnum:
+#
+# An enumeration of possible flags for serialized bitmaps.
+#

Quoted 9 lines

Quoted 6 lines

qapi/block-core.json +1 -0

+# @in-use: This bitmap is considered to be in-use, and may now be

inconsistent.

qapi/block-core.json +2 -0

+#
+# @auto: This bitmap must reflect any and all changes to the file it

describes.
+#        The type of this bitmap must be @DirtyTrackingBitmap.

logical, but I don't see this restriction in the spec. May be we need to update the spec

1: auto

"The bitmap must reflect all changes of the virtual disk by any application that would write to this qcow2 file (including writes, snapshot switching, etc.). The type of this bitmap must be 'dirty tracking bitmap'."

Actually, this looks correct now that I'm looking at the spec again. I've used a terser phrasing but I think it's correct.

Quoted 11 lines

Quoted 4 lines

qapi/block-core.json +2 -0

+#
+# @extra-data-compatible: This bitmap has extra information

associated with it.

no, this flag means, that extra data is compatible. So, if you don't know what is this extra data, you can read and modify the bitmap, leaving this data as is. If this flag is unset, and there are some extra data, bitmap must not be used.

Finally, this spec should be consistent (or, may be better, duplicate) spec from docs/interop/qcow2.txt..

I might suggest a rewrite of this portion of the spec as it's a little unclear to me.

I've given this portion a rewrite.

Quoted 59 lines

qapi/block-core.json

Quoted 2 lines

qapi/block-core.json +2 -0

+#
+# @unknown: This bitmap has unknown or reserved properties.

Better is only "reserved flags" (not unknown and not properties), they are reserved by spec.

Quoted 25 lines

qapi/block-core.json +5 -0

+#
+# Since: 2.13
+##
+{ 'enum': 'BitmapFlagEnum', 'data': [ 'in-use', 'auto',
+                                      'extra-data-compatible',

'unknown' ] }

qapi/block-core.json +19 -0

+
+##
+# @BitmapInfo:
+#
+# @name: The name of the bitmap.
+#
+# @type: The type of bitmap.
+#
+# @granularity: Bitmap granularity, in bytes.
+#
+# @count: Overall bitmap dirtiness, in bytes.
+#
+# @flags: Bitmap flags, if any.
+#
+# Since: 2.13
+#
+##
+{ 'struct': 'BitmapInfo',
+  'data': { 'name': 'str', 'type': 'BitmapTypeEnum', 'granularity':

Quoted 2 lines

'int',
+            'count': 'int', '*flags': ['BitmapFlagEnum']

may be worth add 'has-extra-data'

Quoted 24 lines

+  }

qapi/block-core.json +6 -1

+}
+
   ##
   # @ImageInfoSpecificQCow2EncryptionBase:
   #

@@ -69,6 +124,8 @@
   # @encrypt: details about encryption parameters; only set if image
   #           is encrypted (since 2.10)
   #
+# @bitmaps: list of image bitmaps (since 2.13)
+#
   # Since: 1.7
   ##
   { 'struct': 'ImageInfoSpecificQCow2',

@@ -77,7 +134,8 @@
         '*lazy-refcounts': 'bool',
         '*corrupt': 'bool',
         'refcount-bits': 'int',
-      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
+      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
+      '*bitmaps': ['BitmapInfo']
     } }
     ##

Quoted 53 lines · John Snow

Quoted 35 lines · Vladimir Sementsov-Ogievskiy

Quoted 34 lines · John Snow

Add some of the necessary scaffolding for reporting bitmap information.

Signed-off-by John Snow <[email protected]>

---
   qapi/block-core.json | 60
+++++++++++++++++++++++++++++++++++++++++++++++++++-

   1 file changed, 59 insertions(+), 1 deletion(-)

qapi/block-core.json +0 -0

index c50517bff3..8f33f41ce7 100644
@@ -33,6 +33,61 @@
                'date-sec': 'int', 'date-nsec': 'int',
                'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }

   +##

qapi/block-core.json +18 -0

+# @BitmapTypeEnum:
+#
+# An enumeration of possible serialized bitmap types.
+#
+# @dirty-tracking: This bitmap records information on dirty
+#                  segments within the file.
+#
+# @unknown: This bitmap is of an unknown or reserved type.
+#
+# Since: 2.13
+##
+{ 'enum': 'BitmapTypeEnum', 'data': [ 'dirty-tracking', 'unknown' ] }
+
+##
+# @BitmapFlagEnum:
+#
+# An enumeration of possible flags for serialized bitmaps.
+#

Quoted 9 lines

Quoted 6 lines

+# @in-use: This bitmap is considered to be in-use, and may now be inconsistent.

qapi/block-core.json +2 -0

+#
+# @auto: This bitmap must reflect any and all changes to the file it

describes.
+#        The type of this bitmap must be @DirtyTrackingBitmap.

logical, but I don't see this restriction in the spec. May be we need to update the spec

1: auto

"The bitmap must reflect all changes of the virtual disk by any application that would write to this qcow2 file (including writes, snapshot switching, etc.). The type of this bitmap must be 'dirty tracking bitmap'."

Quoted 2 lines

Actually, this looks correct now that I'm looking at the spec again. I've used a terser phrasing but I think it's correct.

another thought: why must? We know nothing about other types.. May be for other type this flag will have similar or other meaning.. For me, this flag looks like a property of dirty-tracking bitmap, not the thing which dictates only that type.

Add functions for querying the basic information inside of bitmaps. Restructure the bitmaps flags masks to facilitate providing a list of flags belonging to the bitmap(s) being queried.

Signed-off-by John Snow <[email protected]>

---
  block/qcow2-bitmap.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++--
  block/qcow2.c        |  7 +++++
  block/qcow2.h        |  1 +
  3 files changed, 87 insertions(+), 2 deletions(-)

block/qcow2-bitmap.c +6 -2

index 60e01abfd7..811b82743a 100644
@@ -49,8 +49,28 @@
 
  /* Bitmap directory entry flags */
  #define BME_RESERVED_FLAGS 0xfffffffcU
-#define BME_FLAG_IN_USE (1U << 0)
-#define BME_FLAG_AUTO   (1U << 1)
+
+enum BME_FLAG_BITS {
+    BME_FLAG_BIT__BEGIN = 0,
+    BME_FLAG_BIT_IN_USE = BME_FLAG_BIT__BEGIN,
+    BME_FLAG_BIT_AUTO   = 1,
+    BME_FLAG_BIT_EXTRA  = 2,

block/qcow2-bitmap.c +9 -0

+    int qapi_value;
+} BMEFlagMap[BME_FLAG_BIT__MAX] = {
+    [BME_FLAG_BIT_IN_USE] = { BME_FLAG_IN_USE, BITMAP_FLAG_ENUM_IN_USE },
+    [BME_FLAG_BIT_AUTO]   = { BME_FLAG_AUTO,   BITMAP_FLAG_ENUM_AUTO },
+    [BME_FLAG_BIT_EXTRA]  = { BME_FLAG_EXTRA,  BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE },
+};
 
  /* bits [1, 8] U [56, 63] are reserved */
  #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001feULL

@@ -663,6 +683,63 @@ static void del_bitmap_list(BlockDriverState *bs)
      }
  }
 
+static BitmapFlagEnumList *get_bitmap_flags(uint32_t flags)
+{
+    int i;

block/qcow2-bitmap.c +44 -0

+        }
+        ftmp->next = flist;
+        flist = ftmp;
+        flags &= ~(1 << i);
+    }
+
+    return flist;
+}
+
+BitmapInfoList *qcow2_get_bitmap_info(BlockDriverState *bs)
+{
+    Qcow2BitmapList *bm_list;
+    Qcow2Bitmap *bm;
+    BitmapInfoList *info_list = NULL;
+    BitmapInfoList *tmp;
+    BitmapInfo *bitmap_info;
+
+    bm_list = get_bitmap_list(bs, NULL);
+    if (!bm_list) {
+        return info_list;
+    }
+
+    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+        bitmap_info = g_new0(BitmapInfo, 1);
+        bitmap_info->name = g_strdup(bm->name);
+        if (bm->type == BT_DIRTY_TRACKING_BITMAP) {
+            bitmap_info->type = BITMAP_TYPE_ENUM_DIRTY_TRACKING;
+        } else {
+            bitmap_info->type = BITMAP_TYPE_ENUM_UNKNOWN;
+        }
+        bitmap_info->granularity = 1 << bm->granularity_bits;
+        bitmap_info->count = bdrv_get_dirty_count(bm->dirty_bitmap);
+        bitmap_info->flags = get_bitmap_flags(bm->flags);
+        bitmap_info->has_flags = !!bitmap_info->flags;
+
+        tmp = g_new0(BitmapInfoList, 1);
+        tmp->value = bitmap_info;
+        tmp->next = info_list;
+        info_list = tmp;
+    }
+
+    return info_list;
+}
+
  int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                                    void **refcount_table,
                                    int64_t *refcount_table_size)

block/qcow2.c +7 -0

index 7ae9000656..0b24ecb6b2 100644
@@ -3960,6 +3960,7 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
      BDRVQcow2State *s = bs->opaque;
      ImageInfoSpecific *spec_info;
      QCryptoBlockInfo *encrypt_info = NULL;
+    BitmapInfoList *bitmap_list = NULL;
 
      if (s->crypto != NULL) {
          encrypt_info = qcrypto_block_get_info(s->crypto, &error_abort);

@@ -4016,6 +4017,12 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
          spec_info->u.qcow2.data->encrypt = qencrypt;
      }
 
+    bitmap_list = qcow2_get_bitmap_info(bs);
+    if (bitmap_list) {
+        spec_info->u.qcow2.data->has_bitmaps = true;
+        spec_info->u.qcow2.data->bitmaps = bitmap_list;
+    }
+
      return spec_info;
  }
 

block/qcow2.h +1 -0

index 796a8c914b..76bf5fa55d 100644
@@ -686,5 +686,6 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
  void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
                                            const char *name,
                                            Error **errp);
+BitmapInfoList *qcow2_get_bitmap_info(BlockDriverState *bs);
 
  #endif

Quoted 31 lines · John Snow

Add functions for querying the basic information inside of bitmaps. Restructure the bitmaps flags masks to facilitate providing a list of flags belonging to the bitmap(s) being queried.

Signed-off-by John Snow <[email protected]>

---
  block/qcow2-bitmap.c | 81
++++++++++++++++++++++++++++++++++++++++++++++++++--
  block/qcow2.c        |  7 +++++
  block/qcow2.h        |  1 +

  3 files changed, 87 insertions(+), 2 deletions(-)

block/qcow2-bitmap.c +10 -2

index 60e01abfd7..811b82743a 100644
@@ -49,8 +49,28 @@
     /* Bitmap directory entry flags */
   #define BME_RESERVED_FLAGS 0xfffffffcU
-#define BME_FLAG_IN_USE (1U << 0)
-#define BME_FLAG_AUTO   (1U << 1)
+
+enum BME_FLAG_BITS {
+    BME_FLAG_BIT__BEGIN = 0,
+    BME_FLAG_BIT_IN_USE = BME_FLAG_BIT__BEGIN,
+    BME_FLAG_BIT_AUTO   = 1,
+    BME_FLAG_BIT_EXTRA  = 2,
+    BME_FLAG_BIT__MAX,
+};
+
+#define BME_FLAG_IN_USE (1U << BME_FLAG_BIT_IN_USE)

block/qcow2-bitmap.c line 67

Quoted 6 lines

block/qcow2-bitmap.c +6 -0

+#define BME_FLAG_AUTO   (1U << BME_FLAG_BIT_AUTO)
+#define BME_FLAG_EXTRA  (1U << BME_FLAG_BIT_EXTRA)
+
+/* Correlate canonical spec values to autogenerated QAPI values */
+struct {
+    uint32_t mask;

mask is unused in this patch

Quoted 21 lines

block/qcow2-bitmap.c +13 -0

+    int qapi_value;
+} BMEFlagMap[BME_FLAG_BIT__MAX] = {
+    [BME_FLAG_BIT_IN_USE] = { BME_FLAG_IN_USE,
 BITMAP_FLAG_ENUM_IN_USE },
+    [BME_FLAG_BIT_AUTO]   = { BME_FLAG_AUTO,   BITMAP_FLAG_ENUM_AUTO },
+    [BME_FLAG_BIT_EXTRA]  = { BME_FLAG_EXTRA, 
 BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE },
+};
     /* bits [1, 8] U [56, 63] are reserved */
   #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001feULL

@@ -663,6 +683,63 @@ static void del_bitmap_list(BlockDriverState *bs)
       }
   }
   +static BitmapFlagEnumList *get_bitmap_flags(uint32_t flags)
+{
+    int i;
+    BitmapFlagEnumList *flist = NULL;
+    BitmapFlagEnumList *ftmp;
+
+    while (flags) {
+        i = ctz32(flags);

block/qcow2-bitmap.c static void del_bitmap_list(BlockDriverState *bs) · line 698

Quoted 5 lines

block/qcow2-bitmap.c +5 -0

+        ftmp = g_new0(BitmapFlagEnumList, 1);
+        if (i >= BME_FLAG_BIT__BEGIN && i < BME_FLAG_BIT__MAX) {
+            ftmp->value = BMEFlagMap[i].qapi_value;
+        } else {
+            ftmp->value = BITMAP_FLAG_ENUM_UNKNOWN;

so, there may be several "unknown" entries. It's inconsistent with "@unknown: This bitmap has unknown or reserved properties.".

Finally, can we export values for unknown flags? It should be more informative.

Quoted 27 lines · John Snow

Add functions for querying the basic information inside of bitmaps. Restructure the bitmaps flags masks to facilitate providing a list of flags belonging to the bitmap(s) being queried.

Signed-off-by John Snow <[email protected]>

---
  block/qcow2-bitmap.c | 81
++++++++++++++++++++++++++++++++++++++++++++++++++--
  block/qcow2.c        |  7 +++++
  block/qcow2.h        |  1 +

  3 files changed, 87 insertions(+), 2 deletions(-)

block/qcow2-bitmap.c +6 -2

index 60e01abfd7..811b82743a 100644
@@ -49,8 +49,28 @@
     /* Bitmap directory entry flags */
   #define BME_RESERVED_FLAGS 0xfffffffcU
-#define BME_FLAG_IN_USE (1U << 0)
-#define BME_FLAG_AUTO   (1U << 1)
+
+enum BME_FLAG_BITS {
+    BME_FLAG_BIT__BEGIN = 0,
+    BME_FLAG_BIT_IN_USE = BME_FLAG_BIT__BEGIN,
+    BME_FLAG_BIT_AUTO   = 1,
+    BME_FLAG_BIT_EXTRA  = 2,

block/qcow2-bitmap.c line 66

Quoted 10 lines

block/qcow2-bitmap.c +10 -0

+    BME_FLAG_BIT__MAX,
+};
+
+#define BME_FLAG_IN_USE (1U << BME_FLAG_BIT_IN_USE)
+#define BME_FLAG_AUTO   (1U << BME_FLAG_BIT_AUTO)
+#define BME_FLAG_EXTRA  (1U << BME_FLAG_BIT_EXTRA)
+
+/* Correlate canonical spec values to autogenerated QAPI values */
+struct {
+    uint32_t mask;

mask is unused in this patch

Quoted 16 lines

block/qcow2-bitmap.c +8 -0

+    int qapi_value;
+} BMEFlagMap[BME_FLAG_BIT__MAX] = {
+    [BME_FLAG_BIT_IN_USE] = { BME_FLAG_IN_USE,
 BITMAP_FLAG_ENUM_IN_USE },
+    [BME_FLAG_BIT_AUTO]   = { BME_FLAG_AUTO,   BITMAP_FLAG_ENUM_AUTO },
+    [BME_FLAG_BIT_EXTRA]  = { BME_FLAG_EXTRA, 
 BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE },
+};
     /* bits [1, 8] U [56, 63] are reserved */
   #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001feULL

@@ -663,6 +683,63 @@ static void del_bitmap_list(BlockDriverState *bs)
       }
   }
   +static BitmapFlagEnumList *get_bitmap_flags(uint32_t flags)
+{
+    int i;

block/qcow2-bitmap.c static void del_bitmap_list(BlockDriverState *bs) · line 697

Quoted 10 lines

block/qcow2-bitmap.c +10 -0

+    BitmapFlagEnumList *flist = NULL;
+    BitmapFlagEnumList *ftmp;
+
+    while (flags) {
+        i = ctz32(flags);
+        ftmp = g_new0(BitmapFlagEnumList, 1);
+        if (i >= BME_FLAG_BIT__BEGIN && i < BME_FLAG_BIT__MAX) {
+            ftmp->value = BMEFlagMap[i].qapi_value;
+        } else {
+            ftmp->value = BITMAP_FLAG_ENUM_UNKNOWN;

so, there may be several "unknown" entries. It's inconsistent with "@unknown: This bitmap has unknown or reserved properties.".

Finally, can we export values for unknown flags? It should be more informative.

Quoted 69 lines · Vladimir Sementsov-Ogievskiy

Quoted 27 lines · John Snow

Add functions for querying the basic information inside of bitmaps. Restructure the bitmaps flags masks to facilitate providing a list of flags belonging to the bitmap(s) being queried.

Signed-off-by John Snow <[email protected]>

---
   block/qcow2-bitmap.c | 81
++++++++++++++++++++++++++++++++++++++++++++++++++--
   block/qcow2.c        |  7 +++++
   block/qcow2.h        |  1 +
   3 files changed, 87 insertions(+), 2 deletions(-)

block/qcow2-bitmap.c +6 -2

index 60e01abfd7..811b82743a 100644
@@ -49,8 +49,28 @@
     /* Bitmap directory entry flags */
   #define BME_RESERVED_FLAGS 0xfffffffcU
-#define BME_FLAG_IN_USE (1U << 0)
-#define BME_FLAG_AUTO   (1U << 1)
+
+enum BME_FLAG_BITS {
+    BME_FLAG_BIT__BEGIN = 0,
+    BME_FLAG_BIT_IN_USE = BME_FLAG_BIT__BEGIN,
+    BME_FLAG_BIT_AUTO   = 1,
+    BME_FLAG_BIT_EXTRA  = 2,

block/qcow2-bitmap.c line 66

Quoted 10 lines

block/qcow2-bitmap.c +10 -0

+    BME_FLAG_BIT__MAX,
+};
+
+#define BME_FLAG_IN_USE (1U << BME_FLAG_BIT_IN_USE)
+#define BME_FLAG_AUTO   (1U << BME_FLAG_BIT_AUTO)
+#define BME_FLAG_EXTRA  (1U << BME_FLAG_BIT_EXTRA)
+
+/* Correlate canonical spec values to autogenerated QAPI values */
+struct {
+    uint32_t mask;

mask is unused in this patch

Quoted 16 lines

block/qcow2-bitmap.c +8 -0

+    int qapi_value;
+} BMEFlagMap[BME_FLAG_BIT__MAX] = {
+    [BME_FLAG_BIT_IN_USE] = { BME_FLAG_IN_USE,
 BITMAP_FLAG_ENUM_IN_USE },
+    [BME_FLAG_BIT_AUTO]   = { BME_FLAG_AUTO,   BITMAP_FLAG_ENUM_AUTO },
+    [BME_FLAG_BIT_EXTRA]  = { BME_FLAG_EXTRA,
 BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE },
+};
     /* bits [1, 8] U [56, 63] are reserved */
   #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001feULL

@@ -663,6 +683,63 @@ static void del_bitmap_list(BlockDriverState *bs)
       }
   }
   +static BitmapFlagEnumList *get_bitmap_flags(uint32_t flags)
+{
+    int i;

block/qcow2-bitmap.c static void del_bitmap_list(BlockDriverState *bs) · line 697

Quoted 10 lines

block/qcow2-bitmap.c +10 -0

+    BitmapFlagEnumList *flist = NULL;
+    BitmapFlagEnumList *ftmp;
+
+    while (flags) {
+        i = ctz32(flags);
+        ftmp = g_new0(BitmapFlagEnumList, 1);
+        if (i >= BME_FLAG_BIT__BEGIN && i < BME_FLAG_BIT__MAX) {
+            ftmp->value = BMEFlagMap[i].qapi_value;
+        } else {
+            ftmp->value = BITMAP_FLAG_ENUM_UNKNOWN;

so, there may be several "unknown" entries. It's inconsistent with "@unknown: This bitmap has unknown or reserved properties.".

Quoted 3 lines

Finally, can we export values for unknown flags? It should be more informative.

I changed my mind -- qemu-img is not a debugging tool and shouldn't get lost in the weeds trying to enumerate the different kinds of reserved values that are set.

It's an error to have them set, or not -- which ones specifically is not information that an end-user need know or care about, and I don't want to create an extra structure to contain the value.

I'm going to rewrite this loop to just only ever have one unknown value at a maximum.

--js

The function already exists, just expose it.

Signed-off-by John Snow <[email protected]>

---
  include/qapi/qmp/qjson.h |  1 +
  qobject/qjson.c          | 21 +++++++++++----------
  2 files changed, 12 insertions(+), 10 deletions(-)

include/qapi/qmp/qjson.h +0 -0

index b274ac3a86..0e8624c1fa 100644

Quoted 8 lines · John Snow

The function already exists, just expose it.

Signed-off-by John Snow <[email protected]>

---
  include/qapi/qmp/qjson.h |  1 + qobject/qjson.c          | 21 +++++++++++----------

  2 files changed, 12 insertions(+), 10 deletions(-)

Quoted 11 lines

include/qapi/qmp/qjson.h +0 -0

index b274ac3a86..0e8624c1fa 100644
@@ -19,6 +19,7 @@ QObject *qobject_from_jsonf(const char *string, ...)
 GCC_FMT_ATTR(1, 2);

  QObject *qobject_from_jsonv(const char *string, va_list *ap, Error **errp)
      GCC_FMT_ATTR(1, 0);
  +QString *qobject_to_json_opt(const QObject *obj, int pretty, int indent);

pretty seems like it is better as a bool.  Even though to_json() isn't typed correctly, that's no excuse for the public function to copy a bad interface (and maybe it's worth a separate cleanup patch to fix to_json() first).

Add two new structures for detailing the marked regions of bitmaps as saved in e.g. qcow2 files.

Signed-off-by John Snow <[email protected]>

---
  qapi/block-core.json | 32 ++++++++++++++++++++++++++++++++
  1 file changed, 32 insertions(+)

qapi/block-core.json +14 -0

index 8f33f41ce7..de8ad73a78 100644
@@ -298,6 +298,38 @@
             'zero': 'bool', 'depth': 'int', '*offset': 'int',
             '*filename': 'str' } }
 
+##
+# @BitmapEntry:
+#
+# Dirty Bitmap region information for a virtual block range
+#
+# @offset: the start byte of the dirty virtual range
+#
+# @length: the number of bytes of the dirty virtual range
+#
+# Since: 2.13
+#
+##
+{ 'struct': 'BitmapEntry',
+  'data': { 'offset': 'int', 'length': 'int' } }

qapi/block-core.json +8 -0

+#
+# Since: 2.13
+#
+##
+{ 'struct': 'BitmapMapping',
+  'data': { 'name': 'str',
+            'entries': [ 'BitmapEntry' ] } }
+
  ##
  # @BlockdevCacheInfo:
  #

Quoted 6 lines

qapi/block-core.json +6 -0

+##
+# @BitmapMapping:
+#
+# List of described regions correlated to a named bitmap.
+#
+# @name: The name of the bitmap whose range is described here

qapi/block-core.json line 324

Quoted 3 lines

qapi/block-core.json +3 -0

+#
+# @entries: A list of zero or more @BitmapEntry elements representing
+#           the range(s) described by the bitmap.

Is it also worth documenting that the list will be in ascending order, with no overlaps (no two entries covering the same offset); and in fact with a gap between all entries (as otherwise those two consecutive entries could have been consolidated to one)?

Quoted 11 lines

qapi/block-core.json +8 -0

+#
+# Since: 2.13
+#
+##
+{ 'struct': 'BitmapMapping',
+  'data': { 'name': 'str',
+            'entries': [ 'BitmapEntry' ] } }
+

  ##
  # @BlockdevCacheInfo:
  #

Allow qemu-img to show information about bitmaps stored in qcow2 images. Add a 'bitmap' meta-command with 'dump' sub-command to retrieve a list of dirty regions in bitmaps stored in a qcow2 image.

RFC:
- I am not 1000% convinced the bm_list caching is perfectly safe, especially with respect to migration and inactivation. There are more efficiencies and tweaks I can make, but I think this is the minimal set.

- I decided not to gate bitmap info in the "info" command behind extra flags.

- Bitmap data-gathering in "bitmap dump" could be made more space-efficient by just reporting one segment at a time, probably.

- bitmap rm and bitmap mk need extra work, so I am not submitting them just yet: rm needs more work around the remove persistence API, and make needs more work around the "can add" API.

- None of these commands will work with "in use" bitmaps; we need qemu-img check -r bitmaps support for this. I'm not sure what the right behavior to "fix" in-use bitmaps should be:
- Cleared: This might be dangerous.
- Fully Set: This is safer, but stupid.
- Deleted: This might be the best option.

- Un-set in-use: VERY dangerous; would rather not.

John Snow (12):
qcow2-bitmap: cache bm_list
qcow2/dirty-bitmap: cache loaded bitmaps block/qcow2-bitmap: avoid adjusting bm->flags for RO bitmaps qcow2/dirty-bitmaps: load IN_USE bitmaps if disk is RO
qcow2-bitmap: track bitmap type
qapi: add bitmap info
qcow2-bitmap: add basic bitmaps info
qjson: allow caller to ask for arbitrary indent qapi/block-core: add BitmapMapping and BitmapEntry structs
qemu-img: split off common chunk of map command
qemu-img: add bitmap dump
qemu-img: add bitmap clear

  block/qcow2-bitmap.c     | 220 +++++++++++++++++--------
  block/qcow2.c            |   9 +
  block/qcow2.h            |   3 +
  include/qapi/qmp/qjson.h |   1 +
  qapi/block-core.json     |  92 ++++++++++-
  qemu-img-cmds.hx         |   6 +
  qemu-img.c               | 419 +++++++++++++++++++++++++++++++++++++----------
  qobject/qjson.c          |  21 +--
  8 files changed, 612 insertions(+), 159 deletions(-)