[Qemu-devel] [PATCH v3] block: maintain persistent disabled bitmaps

[Vladimir Sementsov-Ogievskiy]

To maintain load/store disabled bitmap there is new approach:

 - deprecate @autoload flag of block-dirty-bitmap-add, make it ignored
 - store enabled bitmaps as "auto" to qcow2
 - store disabled bitmaps without "auto" flag to qcow2
 - on qcow2 open load "auto" bitmaps as enabled and others
   as disabled (except in_use bitmaps)

Also, adjust iotests 165 and 176 appropriately.

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---

v3: fix iotests
    add info into qemu-doc.texi
    rm John's r-b (sorry=(

v2: add John's r-b
    fix spelling

bitmaps-api and bitmaps-postcopy migrations depend on this patch, so
let's discuss (and merge if in case of consent) as soon as possible.

Reasoning:

Firstly, consider only enabled bitmaps (for now, actually, we can't
get disabled bitmap in Qemu, as block-dirty-bitmap-enable command
is not merged yet).

We (Virtuozzo) always use persistent=true and autoload=true. As, to
maintain dirty bitmap (enabled) valid, we must to load it every time,
when we load our disk for r/w, to track disk changes. I do not think,
that something like "not loading enabled dirty bitmaps, when open disk
for read-only" gives real benefits (and now it is not possible,
anyway, as loading depends on qcow2 bitmap "auto" flag, not on r-w
mode).

So, this flag is useless for now. Moreover, if we save bitmap with
autoload=false, it will not be loaded next time, and will become
invalid on first write to the disk. Creating bitmap with flags
"persistent=true, autoload=false", actually means "make it disabled
after storing" (weird semantic, isn't it?), so it will not be
automatically updated more. So, this flag is a bit dangerous.

Let's move to disabled bitmaps. Assume, that my patches will be
merged, and we will really have a possibility of enable/disable
bitmaps when we want. So, it's natural to expect, that if we have
persistent-enabled bitmaps and persistent-disabled bitmaps, then
enabled one will be enabled on next Qemu start and disabled will be
disabled.

How to achieve this?

Let's start from qcow2. We need a stored information on "is this
bitmap enabled or not". But we actually have it. Let's look on qcow2
"auto" flag definiton:
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'.

Isn't it a definition of "enabled" bitmap?
And yes, current mapping from qapi flag "autoload" to qcow2 flag
"auto" is not good mapping.

So, it looks ok, to map !BdrvDirtyBitmap.disabled to "auto". If we
have in future some bitmaps in qcow2, which are not enabled and
not disabled (something more complicated), we can introduce new flags
or even new bitmap type.
(from qcow2 spec):
16:    type
       This field describes the sort of the bitmap.
       Values:
         1: Dirty tracking bitmap

       Values 0, 2 - 255 are reserved.
So, it looks like we _do not need_ qcow2 format changing for now. It
already maintain enabled (auto=true) and disabled (auto=false) bitmaps
(may be, additional note in spec about mapping of this flag in Qemu
will be not bad)

And actually, we do not have anything about "load this bitmap or not"
in qcow2.  And I do not think that we need. Qemu (and user) should
decide, which bitmaps to load. (and it is obvious, that we must load
"auto" bitmaps, to not break them)

Then about qapi. What will occur, if we store
disabled-persistent-autolading bitmap? It will be enabled on next
Qemu start! And it shows that mapping "autoload"->"auto" is definitely
bad. So, as we do not want information on "load or not the bitmap" in
qcow2 (ok, I don't want, but I think Kevin and Max will agree, as it
keeps qcow2 format more generic and separated from Qemu specifics), we
see again, that "autoload" is useless, dangerous and wrong.

If we agreed, that for now auto = !BdrvDirtyBitmap.disabled and
"autoload" is deprecated, we need to decide, which disabled bitmaps we
want to load.

The simplest way to solve the problem is to load all bitmaps, mapping
BdrvDirtyBitmap.disabled = !auto. In future, if we need, we'll be able
to introduce some cmd-line flags to select disabled bitmaps for
loading or separate qmp-command to load them (and do not load them on
start).

Or we can go another way, and continue loading all disabled bitmaps,
but in "lazy mode", so bitmap is not actually loaded, only its name
and some metadata. And we can actually load it, if user enables or
exports it. It looks very interesting approach, as we do not lose RAM
on (possibly) a lot of not needed bitmaps, but we can manage them (for
example, remove).

Any way, loading all bitmaps looks like a good start.
 qemu-doc.texi                |  7 +++++++
 qapi/block-core.json         |  6 +++---
 block/qcow2.h                |  2 +-
 include/block/dirty-bitmap.h |  1 -
 block/dirty-bitmap.c         | 18 ------------------
 block/qcow2-bitmap.c         | 12 +++++++-----
 block/qcow2.c                |  2 +-
 blockdev.c                   | 10 ++--------
 tests/qemu-iotests/165       |  2 +-
 tests/qemu-iotests/176       |  2 +-
 10 files changed, 23 insertions(+), 39 deletions(-)

diff --git a/qemu-doc.texi b/qemu-doc.texi
index 3e9eb819a6..7beaf1f6e8 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2749,6 +2749,13 @@ used and it will be removed with no replacement.
 The ``convert -s snapshot_id_or_name'' argument is obsoleted
 by the ``convert -l snapshot_param'' argument instead.
 
address@hidden QEMU Machine Protocol (QMP) commands
+
address@hidden block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
+
+"autoload" parameter is now ignored. All bitmaps are automatically loaded
+from qcow2 image.
+
 @section System emulator human monitor commands
 
 @subsection host_net_add (since 2.10.0)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e94a6881b2..a3fffeac19 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1593,9 +1593,9 @@
 #              Qcow2 disks support persistent bitmaps. Default is false for
 #              block-dirty-bitmap-add. (Since: 2.10)
 #
-# @autoload: the bitmap will be automatically loaded when the image it is 
stored
-#            in is opened. This flag may only be specified for persistent
-#            bitmaps. Default is false for block-dirty-bitmap-add. (Since: 
2.10)
+# @autoload: ignored and deprecated since 2.12.
+#            Currently, all dirty tracking bitmaps are loaded from Qcow2 on
+#            open.
 #
 # Since: 2.4
 ##
diff --git a/block/qcow2.h b/block/qcow2.h
index 46c8cf44ec..016e87c81a 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -663,7 +663,7 @@ void qcow2_cache_discard(BlockDriverState *bs, Qcow2Cache 
*c, void *table);
 int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                                   void **refcount_table,
                                   int64_t *refcount_table_size);
-bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index a591c27213..89dc50946b 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -65,7 +65,6 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap 
*bitmap,
 void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
 
 void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value);
-void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload);
 void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
                                        bool persistent);
 
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 7879d13ddb..909f0517f8 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -52,8 +52,6 @@ struct BdrvDirtyBitmap {
                                    Such operations must fail and both the image
                                    and this bitmap must remain unchanged while
                                    this flag is set. */
-    bool autoload;              /* For persistent bitmaps: bitmap must be
-                                   autoloaded on image opening */
     bool persistent;            /* bitmap must be saved to owner disk image */
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
@@ -104,7 +102,6 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
     g_free(bitmap->name);
     bitmap->name = NULL;
     bitmap->persistent = false;
-    bitmap->autoload = false;
 }
 
 /* Called with BQL taken.  */
@@ -261,8 +258,6 @@ BdrvDirtyBitmap 
*bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
     bitmap->successor = NULL;
     successor->persistent = bitmap->persistent;
     bitmap->persistent = false;
-    successor->autoload = bitmap->autoload;
-    bitmap->autoload = false;
     bdrv_release_dirty_bitmap(bs, bitmap);
 
     return successor;
@@ -667,19 +662,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
 }
 
 /* Called with BQL taken. */
-void bdrv_dirty_bitmap_set_autoload(BdrvDirtyBitmap *bitmap, bool autoload)
-{
-    qemu_mutex_lock(bitmap->mutex);
-    bitmap->autoload = autoload;
-    qemu_mutex_unlock(bitmap->mutex);
-}
-
-bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap)
-{
-    return bitmap->autoload;
-}
-
-/* Called with BQL taken. */
 void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool 
persistent)
 {
     qemu_mutex_lock(bitmap->mutex);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index f45e46cfbd..ae14464de6 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -933,14 +933,14 @@ static void set_readonly_helper(gpointer bitmap, gpointer 
value)
     bdrv_dirty_bitmap_set_readonly(bitmap, (bool)value);
 }
 
-/* qcow2_load_autoloading_dirty_bitmaps()
+/* qcow2_load_dirty_bitmaps()
  * Return value is a hint for caller: true means that the Qcow2 header was
  * updated. (false doesn't mean that the header should be updated by the
  * caller, it just means that updating was not needed or the image cannot be
  * written to).
  * On failure the function returns false.
  */
-bool qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     Qcow2BitmapList *bm_list;
@@ -960,14 +960,16 @@ bool 
qcow2_load_autoloading_dirty_bitmaps(BlockDriverState *bs, Error **errp)
     }
 
     QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-        if ((bm->flags & BME_FLAG_AUTO) && !(bm->flags & BME_FLAG_IN_USE)) {
+        if (!(bm->flags & BME_FLAG_IN_USE)) {
             BdrvDirtyBitmap *bitmap = load_bitmap(bs, bm, errp);
             if (bitmap == NULL) {
                 goto fail;
             }
 
+            if (!(bm->flags & BME_FLAG_AUTO)) {
+                bdrv_disable_dirty_bitmap(bitmap);
+            }
             bdrv_dirty_bitmap_set_persistance(bitmap, true);
-            bdrv_dirty_bitmap_set_autoload(bitmap, true);
             bm->flags |= BME_FLAG_IN_USE;
             created_dirty_bitmaps =
                     g_slist_append(created_dirty_bitmaps, bitmap);
@@ -1369,7 +1371,7 @@ void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
             bm->table.size = 0;
             QSIMPLEQ_INSERT_TAIL(&drop_tables, tb, entry);
         }
-        bm->flags = bdrv_dirty_bitmap_get_autoload(bitmap) ? BME_FLAG_AUTO : 0;
+        bm->flags = bdrv_dirty_bitmap_enabled(bitmap) ? BME_FLAG_AUTO : 0;
         bm->granularity_bits = ctz32(bdrv_dirty_bitmap_granularity(bitmap));
         bm->dirty_bitmap = bitmap;
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index 4348b2c0c5..47df95b416 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1450,7 +1450,7 @@ static int qcow2_do_open(BlockDriverState *bs, QDict 
*options, int flags,
         s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
     }
 
-    if (qcow2_load_autoloading_dirty_bitmaps(bs, &local_err)) {
+    if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
         update_header = false;
     }
     if (local_err != NULL) {
diff --git a/blockdev.c b/blockdev.c
index 29d569a24e..79aab74d27 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2805,14 +2805,9 @@ void qmp_block_dirty_bitmap_add(const char *node, const 
char *name,
     if (!has_persistent) {
         persistent = false;
     }
-    if (!has_autoload) {
-        autoload = false;
-    }
 
-    if (has_autoload && !persistent) {
-        error_setg(errp, "Autoload flag must be used only for persistent "
-                         "bitmaps");
-        return;
+    if (has_autoload) {
+        warn_report("Autoload option is deprecated and its value is ignored");
     }
 
     if (persistent &&
@@ -2827,7 +2822,6 @@ void qmp_block_dirty_bitmap_add(const char *node, const 
char *name,
     }
 
     bdrv_dirty_bitmap_set_persistance(bitmap, persistent);
-    bdrv_dirty_bitmap_set_autoload(bitmap, autoload);
 }
 
 void qmp_block_dirty_bitmap_remove(const char *node, const char *name,
diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
index a3932db3de..2936929627 100755
--- a/tests/qemu-iotests/165
+++ b/tests/qemu-iotests/165
@@ -64,7 +64,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
 
     def qmpAddBitmap(self):
         self.vm.qmp('block-dirty-bitmap-add', node='drive0',
-                    name='bitmap0', persistent=True, autoload=True)
+                    name='bitmap0', persistent=True)
 
     def test_persistent(self):
         self.vm = self.mkVm()
diff --git a/tests/qemu-iotests/176 b/tests/qemu-iotests/176
index b8dc17c592..699ef3c526 100755
--- a/tests/qemu-iotests/176
+++ b/tests/qemu-iotests/176
@@ -93,7 +93,7 @@ case $reason in
      "file": { "driver": "file", "filename": "$TEST_IMG" } } }
 { "execute": "block-dirty-bitmap-add",
   "arguments": { "node": "drive0", "name": "bitmap0",
-     "persistent": true, "autoload": true } }
+     "persistent": true } }
 { "execute": "quit" }
 EOF
        ;;
-- 
2.11.1