[Qemu-devel] [PATCH v2 00/10] qemu-img: add bitmap info output

[Qemu-devel] [PATCH v2 00/10] qemu-img: add bitmap info output

[John Snow]

Allow qemu-img to show information about bitmaps stored in qcow2 images.

v2:
- Remove bitmap manipulation command in favor of a part 2 series later
- Responded to much of Vladimir's feedback (Thank you!);
  - In particular, make sure IN_USE bitmaps cannot be remounted RW.
  - Fixed semantics of the extra_data flag

John Snow (10):
  qcow2/bitmap: remove redundant arguments from bitmap_list_load
  qcow2/bitmap: avoid adjusting bm->flags for RO bitmaps
  qcow2/bitmap: cache bm_list
  qcow2/bitmap: cache loaded bitmaps
  qcow2/bitmap: reject IN_USE bitmaps on rw reload
  qcow2/bitmap: load IN_USE bitmaps if disk is RO
  qcow2/bitmap: track bitmap type
  qcow2/bitmap: track extra_data_size
  qapi: add bitmap info
  qcow2/bitmap: add basic bitmaps info

 block/qcow2-bitmap.c | 276 ++++++++++++++++++++++++++++++++++-----------------
 block/qcow2.c        |   9 ++
 block/qcow2.h        |   3 +
 qapi/block-core.json |  64 +++++++++++-
 4 files changed, 260 insertions(+), 92 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v2 01/10] qcow2/bitmap: remove redundant arguments from bitmap_list_load

[John Snow]

We always call it with the same fields of the struct we always pass.
We can split this out later if we really wind up needing to.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/qcow2-bitmap.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 69485aa1de..0670e5eb41 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -535,8 +535,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;
@@ -544,6 +543,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;
 
     if (size == 0) {
         error_setg(errp, "Requested bitmap directory size is zero");
@@ -655,8 +656,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 = bitmap_list_load(bs, NULL);
     if (bm_list == NULL) {
         res->corruptions++;
         return -EINVAL;
@@ -952,8 +952,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 = bitmap_list_load(bs, errp);
     if (bm_list == NULL) {
         return false;
     }
@@ -1026,8 +1025,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 = bitmap_list_load(bs, errp);
     if (bm_list == NULL) {
         return -EINVAL;
     }
@@ -1276,8 +1274,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 = bitmap_list_load(bs, errp);
     if (bm_list == NULL) {
         return;
     }
@@ -1329,8 +1326,7 @@ 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 = bitmap_list_load(bs, errp);
         if (bm_list == NULL) {
             return;
         }
@@ -1494,8 +1490,7 @@ 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 = bitmap_list_load(bs, errp);
     if (bm_list == NULL) {
         goto fail;
     }
-- 
2.14.3

[Qemu-devel] [PATCH v2 02/10] qcow2/bitmap: avoid adjusting bm->flags for RO bitmaps

[John Snow]

Instead of always setting IN_USE, do this conditionally based on
whether or not the bitmap is read-only. Eliminate the two-pass
processing and move the second loop to the failure case.

This will allow us to show the flags exactly as they appear
on-disk for bitmaps in `qemu-img info`.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/qcow2-bitmap.c | 51 +++++++++++++++++++++------------------------------
 1 file changed, 21 insertions(+), 30 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 0670e5eb41..85c1b5afe3 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -919,13 +919,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)
 {
@@ -944,8 +937,7 @@ 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 */
@@ -968,35 +960,34 @@ 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;
+            if (can_write(bs)) {
+                bm->flags |= BME_FLAG_IN_USE;
+                needs_update = true;
+            } else {
+                bdrv_dirty_bitmap_set_readonly(bitmap, true);
             }
-            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 */
+    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;
+        }
+    }
+
     bitmap_list_free(bm_list);
 
-    return header_updated;
+    return needs_update;
 
 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);
+        }
+    }
     bitmap_list_free(bm_list);
 
     return false;
-- 
2.14.3

[Qemu-devel] [PATCH v2 03/10] qcow2/bitmap: cache bm_list

[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.

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 <jsnow@redhat.com>
---
 block/qcow2-bitmap.c | 74 ++++++++++++++++++++++++++++++++++------------------
 block/qcow2.c        |  2 ++
 block/qcow2.h        |  2 ++
 3 files changed, 52 insertions(+), 26 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 85c1b5afe3..5ae9b17928 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -636,6 +636,34 @@ 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;
+    }
+
+    if (s->nb_bitmaps) {
+        bm_list = bitmap_list_load(bs, errp);
+    } else {
+        bm_list = bitmap_list_new();
+    }
+    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,7 +684,7 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
         return ret;
     }
 
-    bm_list = bitmap_list_load(bs, NULL);
+    bm_list = get_bitmap_list(bs, NULL);
     if (bm_list == NULL) {
         res->corruptions++;
         return -EINVAL;
@@ -706,8 +734,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
     }
 
 out:
-    bitmap_list_free(bm_list);
-
     return ret;
 }
 
@@ -944,7 +970,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
         return false;
     }
 
-    bm_list = bitmap_list_load(bs, errp);
+    bm_list = get_bitmap_list(bs, errp);
     if (bm_list == NULL) {
         return false;
     }
@@ -978,8 +1004,6 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
         }
     }
 
-    bitmap_list_free(bm_list);
-
     return needs_update;
 
 fail:
@@ -988,8 +1012,7 @@ fail:
             bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
         }
     }
-    bitmap_list_free(bm_list);
-
+    del_bitmap_list(bs);
     return false;
 }
 
@@ -1016,7 +1039,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
         return -EINVAL;
     }
 
-    bm_list = bitmap_list_load(bs, errp);
+    bm_list = get_bitmap_list(bs, errp);
     if (bm_list == NULL) {
         return -EINVAL;
     }
@@ -1056,7 +1079,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;
 }
@@ -1265,7 +1287,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
         return;
     }
 
-    bm_list = bitmap_list_load(bs, errp);
+    bm_list = get_bitmap_list(bs, errp);
     if (bm_list == NULL) {
         return;
     }
@@ -1287,7 +1309,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)
@@ -1304,23 +1330,19 @@ 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);
 
-    if (s->nb_bitmaps == 0) {
-        bm_list = bitmap_list_new();
-    } else {
-        bm_list = bitmap_list_load(bs, errp);
-        if (bm_list == NULL) {
-            return;
-        }
+    bm_list = get_bitmap_list(bs, errp);
+    if (bm_list == NULL) {
+        goto out;
     }
 
     /* check constraints and names */
@@ -1400,8 +1422,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) {
@@ -1416,7 +1437,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)
@@ -1481,13 +1504,12 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
         goto fail;
     }
 
-    bm_list = bitmap_list_load(bs, 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;
diff --git a/block/qcow2.c b/block/qcow2.c
index 6fa5e1d71a..dbd334b150 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2148,6 +2148,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);
diff --git a/block/qcow2.h b/block/qcow2.h
index 01b5250415..29b637a0ee 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -295,6 +295,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;
@@ -671,6 +672,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,
-- 
2.14.3

[Qemu-devel] [PATCH v2 04/10] qcow2/bitmap: cache loaded bitmaps

[John Snow]

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 <jsnow@redhat.com>
---
 block/qcow2-bitmap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 5ae9b17928..d94b6bd5cf 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -981,6 +981,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
             if (bitmap == NULL) {
                 goto fail;
             }
+            bm->dirty_bitmap = bitmap;
 
             if (!(bm->flags & BME_FLAG_AUTO)) {
                 bdrv_disable_dirty_bitmap(bitmap);
@@ -1382,6 +1383,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
             bm->name = g_strdup(name);
             QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry);
         } else {
+            assert(bitmap == bm->dirty_bitmap);
             if (!(bm->flags & BME_FLAG_IN_USE)) {
                 error_setg(errp, "Bitmap '%s' already exists in the image",
                            name);
-- 
2.14.3

[Qemu-devel] [PATCH v2 05/10] qcow2/bitmap: reject IN_USE bitmaps on rw reload

[John Snow]

Presently, an image with IN_USE bitmaps cannot be loaded as RO.
In preparation for changing that, IN_USE bitmaps ought to create an
error on reload instead of being skipped.

While we're here, update the function to work with two new facts:
 - All bm_list entries will have a BdrvDirtyBitmap (because we load and
   disable bitmaps when the autoload flag isn't set), and
 - The dirty_bitmap will be cached, so we don't have to look it up.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/qcow2-bitmap.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index d94b6bd5cf..859819639b 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1046,23 +1046,22 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
     }
 
     QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-        if (!(bm->flags & BME_FLAG_IN_USE)) {
-            BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
-            if (bitmap == NULL) {
-                continue;
-            }
-
-            if (!bdrv_dirty_bitmap_readonly(bitmap)) {
-                error_setg(errp, "Bitmap %s is not readonly but not marked"
-                                 "'IN_USE' in the image. Something went wrong,"
-                                 "all the bitmaps may be corrupted", bm->name);
-                ret = -EINVAL;
-                goto out;
-            }
-
-            bm->flags |= BME_FLAG_IN_USE;
-            ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
+        if (bm->flags & BME_FLAG_IN_USE) {
+            error_setg(errp, "Bitmap '%s' is in use; can't reopen RW",
+                       bm->name);
+            ret = -EINVAL;
+            goto out;
+        } else if (!bdrv_dirty_bitmap_readonly(bm->dirty_bitmap)) {
+            error_setg(errp, "Bitmap %s is neither readonly nor 'IN_USE' in "
+                       "the image. Something went wrong, all the bitmaps may "
+                       "be corrupted. Please report this to the developers at "
+                       " qemu-devel@nongnu.org", bm->name);
+            ret = -EINVAL;
+            goto out;
         }
+
+        bm->flags |= BME_FLAG_IN_USE;
+        ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bm->dirty_bitmap);
     }
 
     if (ro_dirty_bitmaps != NULL) {
-- 
2.14.3

[Qemu-devel] [PATCH v2 06/10] qcow2/bitmap: load IN_USE bitmaps if disk is RO

[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.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/qcow2-bitmap.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 859819639b..6c4fe9b281 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -345,7 +345,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;
     }
@@ -976,23 +976,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);
         }
     }
 
-- 
2.14.3

[Qemu-devel] [PATCH v2 07/10] qcow2/bitmap: track bitmap type

[John Snow]

We only have one type of persistent bitmap right now, but I'd like the
qemu-img tool to be able to give good diagnostic information if it sees
an unknown/unsupported type.

We do enforce it to be the dirty tracking type in check_dir_entry, but
I wanted positive affirmation of the type in the forthcoming info script.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/qcow2-bitmap.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 6c4fe9b281..36573f7b52 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -83,6 +83,7 @@ typedef QSIMPLEQ_HEAD(Qcow2BitmapTableList, Qcow2BitmapTable)
 typedef struct Qcow2Bitmap {
     Qcow2BitmapTable table;
     uint32_t flags;
+    uint8_t type;
     uint8_t granularity_bits;
     char *name;
 
@@ -607,6 +608,7 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error **errp)
         bm->table.offset = e->bitmap_table_offset;
         bm->table.size = e->bitmap_table_size;
         bm->flags = e->flags;
+        bm->type = e->type;
         bm->granularity_bits = e->granularity_bits;
         bm->name = dir_entry_copy_name(e);
         QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry);
@@ -777,7 +779,7 @@ static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
         e->bitmap_table_offset = bm->table.offset;
         e->bitmap_table_size = bm->table.size;
         e->flags = bm->flags;
-        e->type = BT_DIRTY_TRACKING_BITMAP;
+        e->type = bm->type;
         e->granularity_bits = bm->granularity_bits;
         e->name_size = strlen(bm->name);
         e->extra_data_size = 0;
@@ -1393,6 +1395,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
         }
         bm->flags = bdrv_dirty_bitmap_enabled(bitmap) ? BME_FLAG_AUTO : 0;
         bm->granularity_bits = ctz32(bdrv_dirty_bitmap_granularity(bitmap));
+        bm->type = BT_DIRTY_TRACKING_BITMAP;
         bm->dirty_bitmap = bitmap;
     }
 
-- 
2.14.3

[Qemu-devel] [PATCH v2 08/10] qcow2/bitmap: track extra_data_size

[John Snow]

Similarly to the last patch, track the extra_data_size field that we read.
We do reject anything other than zero, but if this restriction is lifted
in the future, we'll need to update the _info field, so loosen this now.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/qcow2-bitmap.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 36573f7b52..3c2e974458 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -86,6 +86,7 @@ typedef struct Qcow2Bitmap {
     uint8_t type;
     uint8_t granularity_bits;
     char *name;
+    uint32_t extra_data_size;
 
     BdrvDirtyBitmap *dirty_bitmap;
 
@@ -609,6 +610,7 @@ static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, Error **errp)
         bm->table.size = e->bitmap_table_size;
         bm->flags = e->flags;
         bm->type = e->type;
+        bm->extra_data_size = e->extra_data_size;
         bm->granularity_bits = e->granularity_bits;
         bm->name = dir_entry_copy_name(e);
         QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry);
@@ -782,7 +784,7 @@ static int bitmap_list_store(BlockDriverState *bs, Qcow2BitmapList *bm_list,
         e->type = bm->type;
         e->granularity_bits = bm->granularity_bits;
         e->name_size = strlen(bm->name);
-        e->extra_data_size = 0;
+        e->extra_data_size = bm->extra_data_size;
         memcpy(e + 1, bm->name, e->name_size);
 
         if (check_dir_entry(bs, e) < 0) {
-- 
2.14.3

[Qemu-devel] [PATCH v2 09/10] qapi: add bitmap info

[John Snow]

Add some of the necessary scaffolding for reporting bitmap information.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qapi/block-core.json | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index fff23fc82b..da82a82779 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -34,6 +34,65 @@
             '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 an unknown/reserved type.
+#
+# Since: 3.0
+##
+{ '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 inconsistent.
+#
+# @auto: This bitmap must reflect any and all changes to the file it describes.
+#
+# @extra-data-compatible: The extra data associated with this bitmap can be
+#                         safely ignored if it is opaque to the reader. If
+#                         this flag is absent while extra data is present and
+#                         opaque to the reader, the bitmap must not be used.
+#
+# @reserved: This bitmap has reserved flags set.
+#
+# Since: 3.0
+##
+{ 'enum': 'BitmapFlagEnum', 'data': [ 'in-use', 'auto',
+                                      'extra-data-compatible', 'reserved' ] }
+
+##
+# @BitmapInfo:
+#
+# @name: The name of the bitmap.
+#
+# @type: The type of bitmap.
+#
+# @granularity: Bitmap granularity, in bytes.
+#
+# @count: Overall bitmap dirtiness, in bytes.
+#
+# @extra-data: True if this bitmap has extra data attached.
+#
+# @flags: Bitmap flags, if any.
+#
+# Since: 3.0
+#
+##
+{ 'struct': 'BitmapInfo',
+  'data': { 'name': 'str', 'type': 'BitmapTypeEnum', 'granularity': 'int',
+            'count': 'int', 'extra-data': 'bool', '*flags': ['BitmapFlagEnum']
+  }
+}
+
 ##
 # @ImageInfoSpecificQCow2EncryptionBase:
 #
@@ -70,6 +129,8 @@
 # @encrypt: details about encryption parameters; only set if image
 #           is encrypted (since 2.10)
 #
+# @bitmaps: list of image bitmaps (since 3.0)
+#
 # Since: 1.7
 ##
 { 'struct': 'ImageInfoSpecificQCow2',
@@ -78,7 +139,8 @@
       '*lazy-refcounts': 'bool',
       '*corrupt': 'bool',
       'refcount-bits': 'int',
-      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
+      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
+      '*bitmaps': ['BitmapInfo']
   } }
 
 ##
-- 
2.14.3

[Qemu-devel] [PATCH v2 10/10] qcow2/bitmap: add basic bitmaps info

[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 <jsnow@redhat.com>
---
 block/qcow2-bitmap.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 block/qcow2.c        |  7 +++++
 block/qcow2.h        |  1 +
 3 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 3c2e974458..80bd31bfc1 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -49,8 +49,25 @@
 
 /* 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)
+#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 */
+int BMEFlagMap[BME_FLAG_BIT__MAX] = {
+    [BME_FLAG_BIT_IN_USE] = BITMAP_FLAG_ENUM_IN_USE,
+    [BME_FLAG_BIT_AUTO]   = BITMAP_FLAG_ENUM_AUTO,
+    [BME_FLAG_BIT_EXTRA]  = BITMAP_FLAG_ENUM_EXTRA_DATA_COMPATIBLE,
+};
 
 /* bits [1, 8] U [56, 63] are reserved */
 #define BME_TABLE_ENTRY_RESERVED_MASK 0xff000000000001feULL
@@ -668,6 +685,71 @@ static void del_bitmap_list(BlockDriverState *bs)
     }
 }
 
+static BitmapFlagEnumList *prepend_bitmap_flag(BitmapFlagEnumList *elist,
+                                               BitmapFlagEnum value)
+{
+    BitmapFlagEnumList *ftmp = g_new0(BitmapFlagEnumList, 1);
+    ftmp->value = value;
+    ftmp->next = elist;
+    return ftmp;
+}
+
+static BitmapFlagEnumList *get_bitmap_flags(uint32_t flags)
+{
+    int i;
+    BitmapFlagEnumList *flist = NULL;
+
+    if (flags & BME_RESERVED_FLAGS) {
+        flist = prepend_bitmap_flag(flist, BITMAP_FLAG_ENUM_RESERVED);
+        flags &= ~BME_RESERVED_FLAGS;
+    }
+
+    while (flags) {
+        i = ctz32(flags);
+        assert(i >= BME_FLAG_BIT__BEGIN && i < BME_FLAG_BIT__MAX);
+        flist = prepend_bitmap_flag(flist, BMEFlagMap[i]);
+        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->extra_data = bm->extra_data_size > 0;
+        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)
diff --git a/block/qcow2.c b/block/qcow2.c
index dbd334b150..2a52771ac8 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4133,6 +4133,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);
@@ -4189,6 +4190,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;
 }
 
diff --git a/block/qcow2.h b/block/qcow2.h
index 29b637a0ee..d3fe766a1f 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -682,5 +682,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
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 01/10] qcow2/bitmap: remove redundant arguments from bitmap_list_load

[Vladimir Sementsov-Ogievskiy]

13.06.2018 05:06, John Snow wrote:
> We always call it with the same fields of the struct we always pass.
> We can split this out later if we really wind up needing to.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 02/10] qcow2/bitmap: avoid adjusting bm->flags for RO bitmaps

[Vladimir Sementsov-Ogievskiy]

13.06.2018 05:06, John Snow wrote:
> Instead of always setting IN_USE, do this conditionally based on
> whether or not the bitmap is read-only. Eliminate the two-pass
> processing and move the second loop to the failure case.
>
> This will allow us to show the flags exactly as they appear
> on-disk for bitmaps in `qemu-img info`.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block/qcow2-bitmap.c | 51 +++++++++++++++++++++------------------------------
>   1 file changed, 21 insertions(+), 30 deletions(-)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 0670e5eb41..85c1b5afe3 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -919,13 +919,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)
>   {
> @@ -944,8 +937,7 @@ 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 */
> @@ -968,35 +960,34 @@ 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;
> +            if (can_write(bs)) {
> +                bm->flags |= BME_FLAG_IN_USE;
> +                needs_update = true;
> +            } else {
> +                bdrv_dirty_bitmap_set_readonly(bitmap, true);
>               }

+                bm->dirty_bitmap = bitmap; [1]

> -            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 */
> +    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;
> +        }
> +    }
> +
>       bitmap_list_free(bm_list);
>   
> -    return header_updated;
> +    return needs_update;
>   
>   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) {

hm stop, at this point, you didn't yet set it. may be in later patches. 
I remember that you did it in previous version of patch series, so [1].

> +            bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
> +        }
> +    }
>       bitmap_list_free(bm_list);
>   
>       return false;

with [1]:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 03/10] qcow2/bitmap: cache bm_list

[Vladimir Sementsov-Ogievskiy]

13.06.2018 05:06, John Snow wrote:
> 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 <jsnow@redhat.com>
> ---
>   block/qcow2-bitmap.c | 74 ++++++++++++++++++++++++++++++++++------------------
>   block/qcow2.c        |  2 ++
>   block/qcow2.h        |  2 ++
>   3 files changed, 52 insertions(+), 26 deletions(-)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 85c1b5afe3..5ae9b17928 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -636,6 +636,34 @@ 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;
> +    }
> +
> +    if (s->nb_bitmaps) {
> +        bm_list = bitmap_list_load(bs, errp);
> +    } else {
> +        bm_list = bitmap_list_new();
> +    }
> +    s->bitmap_list = bm_list;

may be, we shouldn't cache it in inactive mode. However, I think we'll 
finally will not load bitmaps in inactive mode and drop the on 
inactivate, so it would not matter..

Hm, I've understood the following problem: cache becomes incorrect after 
failed update_ext_header_and_dir or update_ext_header_and_dir_in_place 
operations. (after failed qcow2_remove_persistent_dirty_bitmap or 
qcow2_reopen_bitmaps_rw_hint)

And this comes from incomplete architecture after the patch:
On the one hand, we work with one singleton bitmap_list, and loading 
part is refactored to do it safely. On the other hand, storing functions 
still have old behavior, they work with bitmap list like with their own 
local variable.

So, we have safe mechanism to read list through the cache. We need also 
safe mechanism to update list both in cache and in file.

There are two possible variants:

1. drop cache after failed store
2. rollback cache after failed store

1 looks simpler..

Also, we should drop cache on inactivate (we do this) and should not 
create cache in inactive mode, because the other process may change the 
image.

Hm. may be, it is better to work with s->bitmap_list directly? In this 
case it will be more obvious that it is the cache, not local variable. 
And we will work with it like with other "parts of extension cache" 
s->nb_bitmaps, s->bitmap_directory_offset ...

After the patch, functions update_ext_header_and_dir* becomes strange:

1. before the patch, they take external parameter - bm_list, and by this 
parameter they updated the file and cached s->nb_bitmaps, s->bitmap_*, ..
2. after the patch, they take parameter (actually s->bitmap_list) of 
same nature like s->nb_bitmap, and update s->nb_bitmap from it.

Sorry for being late and for disordered stream of thoughts. Is this 
patch really needed for the whole series?

> +    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,7 +684,7 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>           return ret;
>       }
>   
> -    bm_list = bitmap_list_load(bs, NULL);
> +    bm_list = get_bitmap_list(bs, NULL);
>       if (bm_list == NULL) {
>           res->corruptions++;
>           return -EINVAL;
> @@ -706,8 +734,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>       }
>   
>   out:
> -    bitmap_list_free(bm_list);
> -
>       return ret;
>   }
>   
> @@ -944,7 +970,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>           return false;
>       }
>   
> -    bm_list = bitmap_list_load(bs, errp);
> +    bm_list = get_bitmap_list(bs, errp);
>       if (bm_list == NULL) {
>           return false;
>       }
> @@ -978,8 +1004,6 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>           }
>       }
>   
> -    bitmap_list_free(bm_list);
> -
>       return needs_update;
>   
>   fail:
> @@ -988,8 +1012,7 @@ fail:
>               bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
>           }
>       }
> -    bitmap_list_free(bm_list);
> -
> +    del_bitmap_list(bs);
>       return false;
>   }
>   
> @@ -1016,7 +1039,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>           return -EINVAL;
>       }
>   
> -    bm_list = bitmap_list_load(bs, errp);
> +    bm_list = get_bitmap_list(bs, errp);
>       if (bm_list == NULL) {
>           return -EINVAL;
>       }
> @@ -1056,7 +1079,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;
>   }
> @@ -1265,7 +1287,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>           return;
>       }
>   
> -    bm_list = bitmap_list_load(bs, errp);
> +    bm_list = get_bitmap_list(bs, errp);
>       if (bm_list == NULL) {
>           return;
>       }
> @@ -1287,7 +1309,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)
> @@ -1304,23 +1330,19 @@ 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);
>   
> -    if (s->nb_bitmaps == 0) {
> -        bm_list = bitmap_list_new();
> -    } else {
> -        bm_list = bitmap_list_load(bs, errp);
> -        if (bm_list == NULL) {
> -            return;
> -        }
> +    bm_list = get_bitmap_list(bs, errp);
> +    if (bm_list == NULL) {
> +        goto out;
>       }
>   
>       /* check constraints and names */
> @@ -1400,8 +1422,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) {
> @@ -1416,7 +1437,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)
> @@ -1481,13 +1504,12 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>           goto fail;
>       }
>   
> -    bm_list = bitmap_list_load(bs, 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;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 6fa5e1d71a..dbd334b150 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2148,6 +2148,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);
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 01b5250415..29b637a0ee 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -295,6 +295,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;
> @@ -671,6 +672,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,


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 04/10] qcow2/bitmap: cache loaded bitmaps

[Vladimir Sementsov-Ogievskiy]

13.06.2018 05:06, John Snow wrote:
> 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 <jsnow@redhat.com>
> ---
>   block/qcow2-bitmap.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 5ae9b17928..d94b6bd5cf 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -981,6 +981,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>               if (bitmap == NULL) {
>                   goto fail;
>               }
> +            bm->dirty_bitmap = bitmap;
>   
>               if (!(bm->flags & BME_FLAG_AUTO)) {
>                   bdrv_disable_dirty_bitmap(bitmap);
> @@ -1382,6 +1383,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>               bm->name = g_strdup(name);
>               QSIMPLEQ_INSERT_TAIL(bm_list, bm, entry);
>           } else {
> +            assert(bitmap == bm->dirty_bitmap);
>               if (!(bm->flags & BME_FLAG_IN_USE)) {
>                   error_setg(errp, "Bitmap '%s' already exists in the image",
>                              name);

and now it is already used in 02, so it should be reordered or merged to it

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 03/10] qcow2/bitmap: cache bm_list

[Vladimir Sementsov-Ogievskiy]

20.06.2018 16:04, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2018 05:06, John Snow wrote:
>> 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<jsnow@redhat.com>
>> ---
>>   block/qcow2-bitmap.c | 74 ++++++++++++++++++++++++++++++++++------------------
>>   block/qcow2.c        |  2 ++
>>   block/qcow2.h        |  2 ++
>>   3 files changed, 52 insertions(+), 26 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 85c1b5afe3..5ae9b17928 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -636,6 +636,34 @@ 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;
>> +    }
>> +
>> +    if (s->nb_bitmaps) {
>> +        bm_list = bitmap_list_load(bs, errp);
>> +    } else {
>> +        bm_list = bitmap_list_new();
>> +    }
>> +    s->bitmap_list = bm_list;
>
> may be, we shouldn't cache it in inactive mode. However, I think we'll 
> finally will not load bitmaps in inactive mode and drop the on 
> inactivate, so it would not matter..

really, now it would be a problem: we can start in inactive mode, load 
nothing, and then we can't reload bitmaps; my fix in
https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg03254.html
will not work after this patch.

>
> Hm, I've understood the following problem: cache becomes incorrect 
> after failed update_ext_header_and_dir or 
> update_ext_header_and_dir_in_place operations. (after failed 
> qcow2_remove_persistent_dirty_bitmap or qcow2_reopen_bitmaps_rw_hint)
>
> And this comes from incomplete architecture after the patch:
> On the one hand, we work with one singleton bitmap_list, and loading 
> part is refactored to do it safely. On the other hand, storing 
> functions still have old behavior, they work with bitmap list like 
> with their own local variable.
>
> So, we have safe mechanism to read list through the cache. We need 
> also safe mechanism to update list both in cache and in file.
>
> There are two possible variants:
>
> 1. drop cache after failed store
> 2. rollback cache after failed store
>
> 1 looks simpler..
>
> Also, we should drop cache on inactivate (we do this) and should not 
> create cache in inactive mode, because the other process may change 
> the image.
>
> Hm. may be, it is better to work with s->bitmap_list directly? In this 
> case it will be more obvious that it is the cache, not local variable. 
> And we will work with it like with other "parts of extension cache" 
> s->nb_bitmaps, s->bitmap_directory_offset ...
>
> After the patch, functions update_ext_header_and_dir* becomes strange:
>
> 1. before the patch, they take external parameter - bm_list, and by 
> this parameter they updated the file and cached s->nb_bitmaps, 
> s->bitmap_*, ..
> 2. after the patch, they take parameter (actually s->bitmap_list) of 
> same nature like s->nb_bitmap, and update s->nb_bitmap from it.
>
> Sorry for being late and for disordered stream of thoughts. Is this 
> patch really needed for the whole series?
>
>> +    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,7 +684,7 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>>           return ret;
>>       }
>>   
>> -    bm_list = bitmap_list_load(bs, NULL);
>> +    bm_list = get_bitmap_list(bs, NULL);
>>       if (bm_list == NULL) {
>>           res->corruptions++;
>>           return -EINVAL;
>> @@ -706,8 +734,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>>       }
>>   
>>   out:
>> -    bitmap_list_free(bm_list);
>> -
>>       return ret;
>>   }
>>   
>> @@ -944,7 +970,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>           return false;
>>       }
>>   
>> -    bm_list = bitmap_list_load(bs, errp);
>> +    bm_list = get_bitmap_list(bs, errp);
>>       if (bm_list == NULL) {
>>           return false;
>>       }
>> @@ -978,8 +1004,6 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>           }
>>       }
>>   
>> -    bitmap_list_free(bm_list);
>> -
>>       return needs_update;
>>   
>>   fail:
>> @@ -988,8 +1012,7 @@ fail:
>>               bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
>>           }
>>       }
>> -    bitmap_list_free(bm_list);
>> -
>> +    del_bitmap_list(bs);
>>       return false;
>>   }
>>   
>> @@ -1016,7 +1039,7 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>>           return -EINVAL;
>>       }
>>   
>> -    bm_list = bitmap_list_load(bs, errp);
>> +    bm_list = get_bitmap_list(bs, errp);
>>       if (bm_list == NULL) {
>>           return -EINVAL;
>>       }
>> @@ -1056,7 +1079,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;
>>   }
>> @@ -1265,7 +1287,7 @@ void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>>           return;
>>       }
>>   
>> -    bm_list = bitmap_list_load(bs, errp);
>> +    bm_list = get_bitmap_list(bs, errp);
>>       if (bm_list == NULL) {
>>           return;
>>       }
>> @@ -1287,7 +1309,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)
>> @@ -1304,23 +1330,19 @@ 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);
>>   
>> -    if (s->nb_bitmaps == 0) {
>> -        bm_list = bitmap_list_new();
>> -    } else {
>> -        bm_list = bitmap_list_load(bs, errp);
>> -        if (bm_list == NULL) {
>> -            return;
>> -        }
>> +    bm_list = get_bitmap_list(bs, errp);
>> +    if (bm_list == NULL) {
>> +        goto out;
>>       }
>>   
>>       /* check constraints and names */
>> @@ -1400,8 +1422,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) {
>> @@ -1416,7 +1437,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)
>> @@ -1481,13 +1504,12 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>           goto fail;
>>       }
>>   
>> -    bm_list = bitmap_list_load(bs, 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;
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 6fa5e1d71a..dbd334b150 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2148,6 +2148,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);
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 01b5250415..29b637a0ee 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -295,6 +295,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;
>> @@ -671,6 +672,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,
>
>
> -- 
> Best regards,
> Vladimir


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 03/10] qcow2/bitmap: cache bm_list

[John Snow]

On 06/20/2018 09:04 AM, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2018 05:06, John Snow wrote:
>> 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 <jsnow@redhat.com>
>> ---
>>   block/qcow2-bitmap.c | 74
>> ++++++++++++++++++++++++++++++++++------------------
>>   block/qcow2.c        |  2 ++
>>   block/qcow2.h        |  2 ++
>>   3 files changed, 52 insertions(+), 26 deletions(-)
>>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 85c1b5afe3..5ae9b17928 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -636,6 +636,34 @@ 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;
>> +    }
>> +
>> +    if (s->nb_bitmaps) {
>> +        bm_list = bitmap_list_load(bs, errp);
>> +    } else {
>> +        bm_list = bitmap_list_new();
>> +    }
>> +    s->bitmap_list = bm_list;
> 
> may be, we shouldn't cache it in inactive mode. However, I think we'll
> finally will not load bitmaps in inactive mode and drop the on
> inactivate, so it would not matter..
> 

Do we not load bitmaps when BDRV_O_INACTIVE is set? it looks like we do?

(From your subsequent email):
>
> really, now it would be a problem: we can start in inactive mode, load
> nothing, and then we can't reload bitmaps; my fix in
> https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg03254.html
> will not work after this patch.
>

So we load nothing because when we opened up the image RO, saw IN_USE
bitmaps (or saw none at all) and decided not to load them. qcow2_do_open
however marks that it has "loaded the bitmaps."

Later, when we reopen RW, we have a cached bm_list that doesn't include
this bitmap, so we don't mark it as IN_USE again or update the header.

(Wait, if you are worried about the bitmap's data having been changed,
why do we not reload the bitmap data here too?)


Now, patch 06 changes the cache so that we load all bitmaps and not just
ones not IN_USE. On RW reload, I reject any such IN_USE bitmaps as a
reason to prohibit the RW reload. However, this is broken too, because I
will miss any new flags that exist on-disk, so this function should
never use the cached data.

I'm confused, though, the comment that calls reopen_bitmaps_rw_hint says:

"It's some kind of reopen. There are no known cases where we need to
reload bitmaps in such a situation, so it's safer to skip them.
Moreover, if we have some readonly bitmaps and we are reopening for rw
we should reopen bitmaps correspondingly."

Why do we assume that bitmap data cannot change while BDRV_O_INACTIVATE
is set? Is that not wrong?

> Hm, I've understood the following problem: cache becomes incorrect after
> failed update_ext_header_and_dir or update_ext_header_and_dir_in_place
> operations. (after failed qcow2_remove_persistent_dirty_bitmap or
> qcow2_reopen_bitmaps_rw_hint)
> 
> And this comes from incomplete architecture after the patch:
> On the one hand, we work with one singleton bitmap_list, and loading
> part is refactored to do it safely. On the other hand, storing functions
> still have old behavior, they work with bitmap list like with their own
> local variable.

Yeah, I see it. Dropping the cache on such errors is fine.

> 
> So, we have safe mechanism to read list through the cache. We need also
> safe mechanism to update list both in cache and in file.
> 
> There are two possible variants:
> 
> 1. drop cache after failed store
> 2. rollback cache after failed store
> 
> 1 looks simpler..
> 
> Also, we should drop cache on inactivate (we do this) and should not
> create cache in inactive mode, because the other process may change the
> image.
> 
> Hm. may be, it is better to work with s->bitmap_list directly? In this
> case it will be more obvious that it is the cache, not local variable.
> And we will work with it like with other "parts of extension cache"
> s->nb_bitmaps, s->bitmap_directory_offset ...
> 
> After the patch, functions update_ext_header_and_dir* becomes strange:
> 
> 1. before the patch, they take external parameter - bm_list, and by this
> parameter they updated the file and cached s->nb_bitmaps, s->bitmap_*, ..
> 2. after the patch, they take parameter (actually s->bitmap_list) of
> same nature like s->nb_bitmap, and update s->nb_bitmap from it.
> 

Yeah, if we do decide that keeping a cache is the right thing, some of
the helper functions could be refactored or simplified a little to take
advantage of the new paradigm.

> Sorry for being late and for disordered stream of thoughts. Is this
> patch really needed for the whole series?
> 

The nice part is to have bm_list with pointers that correlate directly
to the in-memory bitmaps.

I can load bm_list on demand later for the purposes of `qemu-img info`,
but I have to look up the in-memory bitmaps again too. It seemed simpler
to just preserve the data the first time we build it instead of
rebuilding it all the time.

I think that we'll eventually want a cache either way, but maybe I am
still underestimating how difficult it is to do correctly... I think I
need to understand inactivate/invalidate a little more carefully before
I trudge ahead here ...

Re: [Qemu-devel] [PATCH v2 03/10] qcow2/bitmap: cache bm_list

[Vladimir Sementsov-Ogievskiy]

21.06.2018 02:29, John Snow wrote:
>
> On 06/20/2018 09:04 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 13.06.2018 05:06, John Snow wrote:
>>> 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 <jsnow@redhat.com>
>>> ---
>>>    block/qcow2-bitmap.c | 74
>>> ++++++++++++++++++++++++++++++++++------------------
>>>    block/qcow2.c        |  2 ++
>>>    block/qcow2.h        |  2 ++
>>>    3 files changed, 52 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>> index 85c1b5afe3..5ae9b17928 100644
>>> --- a/block/qcow2-bitmap.c
>>> +++ b/block/qcow2-bitmap.c
>>> @@ -636,6 +636,34 @@ 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;
>>> +    }
>>> +
>>> +    if (s->nb_bitmaps) {
>>> +        bm_list = bitmap_list_load(bs, errp);
>>> +    } else {
>>> +        bm_list = bitmap_list_new();
>>> +    }
>>> +    s->bitmap_list = bm_list;
>> may be, we shouldn't cache it in inactive mode. However, I think we'll
>> finally will not load bitmaps in inactive mode and drop the on
>> inactivate, so it would not matter..
>>
> Do we not load bitmaps when BDRV_O_INACTIVE is set? it looks like we do?
>
> (From your subsequent email):
>> really, now it would be a problem: we can start in inactive mode, load
>> nothing, and then we can't reload bitmaps; my fix in
>> https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg03254.html
>> will not work after this patch.
>>
> So we load nothing because when we opened up the image RO, saw IN_USE
> bitmaps (or saw none at all) and decided not to load them. qcow2_do_open
> however marks that it has "loaded the bitmaps."
>
> Later, when we reopen RW, we have a cached bm_list that doesn't include
> this bitmap, so we don't mark it as IN_USE again or update the header.
>
> (Wait, if you are worried about the bitmap's data having been changed,
> why do we not reload the bitmap data here too?)
>
>
> Now, patch 06 changes the cache so that we load all bitmaps and not just
> ones not IN_USE. On RW reload, I reject any such IN_USE bitmaps as a
> reason to prohibit the RW reload. However, this is broken too, because I
> will miss any new flags that exist on-disk, so this function should
> never use the cached data.
>
> I'm confused, though, the comment that calls reopen_bitmaps_rw_hint says:
>
> "It's some kind of reopen. There are no known cases where we need to
> reload bitmaps in such a situation, so it's safer to skip them.
> Moreover, if we have some readonly bitmaps and we are reopening for rw
> we should reopen bitmaps correspondingly."
>
> Why do we assume that bitmap data cannot change while BDRV_O_INACTIVATE
> is set? Is that not wrong?

agree. and this is one more reason to not load bitmaps in inactive mode 
at all. and drop them (after storing) on inactivating.
I'll make a patch.

>
>> Hm, I've understood the following problem: cache becomes incorrect after
>> failed update_ext_header_and_dir or update_ext_header_and_dir_in_place
>> operations. (after failed qcow2_remove_persistent_dirty_bitmap or
>> qcow2_reopen_bitmaps_rw_hint)
>>
>> And this comes from incomplete architecture after the patch:
>> On the one hand, we work with one singleton bitmap_list, and loading
>> part is refactored to do it safely. On the other hand, storing functions
>> still have old behavior, they work with bitmap list like with their own
>> local variable.
> Yeah, I see it. Dropping the cache on such errors is fine.
>
>> So, we have safe mechanism to read list through the cache. We need also
>> safe mechanism to update list both in cache and in file.
>>
>> There are two possible variants:
>>
>> 1. drop cache after failed store
>> 2. rollback cache after failed store
>>
>> 1 looks simpler..
>>
>> Also, we should drop cache on inactivate (we do this) and should not
>> create cache in inactive mode, because the other process may change the
>> image.
>>
>> Hm. may be, it is better to work with s->bitmap_list directly? In this
>> case it will be more obvious that it is the cache, not local variable.
>> And we will work with it like with other "parts of extension cache"
>> s->nb_bitmaps, s->bitmap_directory_offset ...
>>
>> After the patch, functions update_ext_header_and_dir* becomes strange:
>>
>> 1. before the patch, they take external parameter - bm_list, and by this
>> parameter they updated the file and cached s->nb_bitmaps, s->bitmap_*, ..
>> 2. after the patch, they take parameter (actually s->bitmap_list) of
>> same nature like s->nb_bitmap, and update s->nb_bitmap from it.
>>
> Yeah, if we do decide that keeping a cache is the right thing, some of
> the helper functions could be refactored or simplified a little to take
> advantage of the new paradigm.
>
>> Sorry for being late and for disordered stream of thoughts. Is this
>> patch really needed for the whole series?
>>
> The nice part is to have bm_list with pointers that correlate directly
> to the in-memory bitmaps.
>
> I can load bm_list on demand later for the purposes of `qemu-img info`,
> but I have to look up the in-memory bitmaps again too. It seemed simpler
> to just preserve the data the first time we build it instead of
> rebuilding it all the time.
>
> I think that we'll eventually want a cache either way, but maybe I am
> still underestimating how difficult it is to do correctly... I think I
> need to understand inactivate/invalidate a little more carefully before
> I trudge ahead here ...



-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 03/10] qcow2/bitmap: cache bm_list

[John Snow]

On 06/21/2018 06:25 AM, Vladimir Sementsov-Ogievskiy wrote:
>>
> 
> agree. and this is one more reason to not load bitmaps in inactive mode
> at all. and drop them (after storing) on inactivating.
> I'll make a patch.

Sure. I guess persistent bitmaps that exist when BDRV_O_INACTIVE is set
need to stay around -- it would be strange if they disappeared just
because the bitmap is inactive -- but we need to effectively overwrite
them on reload from disk.

And while the disk is inactive, these bitmaps definitely need to remain
in an enforced readonly state (can't be deleted, renamed, cleared, set,
reset, etc.)

(I guess it would also be an error to try to remove persistence from a
bitmap on an inactive disk too.)