[Qemu-devel] [PATCH v2 0/9] qcow2-bitmaps: rewrite reopening logic

[Qemu-devel] [PATCH v2 0/9] qcow2-bitmaps: rewrite reopening logic

[Vladimir Sementsov-Ogievskiy]

Hi all!

Bitmaps reopening is buggy, we may easily produce broken incremental
backup if we do temporary snapshot. Let's fix it!

v2:
01: new
02-03: test: splat into two patches, some wording
       improvements and event_wait improved
04: add John's r-b
05: new
06-09: fixes: changed, splat, use patch 01

Vladimir Sementsov-Ogievskiy (9):
  block: add .bdrv_need_rw_file_child_during_reopen_rw handler
  python/qemu: improve event_wait method of vm
  iotests: add test 255 to check bitmap life after snapshot + commit
  block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps
  block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint()
  block/qcow2-bitmap: do not remove bitmaps on reopen-ro
  block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw
  block/qcow2-bitmap: fix reopening bitmaps to RW
  qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_prepare

 block/qcow2.h                 |   6 +-
 include/block/block_int.h     |   8 +-
 include/block/dirty-bitmap.h  |   1 -
 block.c                       | 163 +++++++++++++++++++++++++++-------
 block/dirty-bitmap.c          |  12 ---
 block/qcow2-bitmap.c          | 146 ++++++++++++++++++------------
 block/qcow2.c                 |   9 +-
 python/qemu/__init__.py       |   9 +-
 tests/qemu-iotests/255        |  86 ++++++++++++++++++
 tests/qemu-iotests/255.out    |  52 +++++++++++
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |   5 ++
 12 files changed, 381 insertions(+), 117 deletions(-)
 create mode 100755 tests/qemu-iotests/255
 create mode 100644 tests/qemu-iotests/255.out

-- 
2.18.0

[Qemu-devel] [PATCH v2 1/9] block: add .bdrv_need_rw_file_child_during_reopen_rw handler

[Vladimir Sementsov-Ogievskiy]

On reopen to rw parent may need rw access to child in .prepare, for
example qcow2 needs to write IN_USE flags into stored bitmaps
(currently it is done in a hacky way after commit and don't work).
So, let's introduce such logic.

The drawback is that in worst case bdrv_reopen_set_read_only may finish
with error and in some intermediate state: some nodes reopened RW and
some are not. But this is a way to fix bug around reopening qcow2
bitmaps in the following commits.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block_int.h |   2 +
 block.c                   | 144 ++++++++++++++++++++++++++++++++++----
 2 files changed, 133 insertions(+), 13 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1eebc7c8f3..7b22fb5d9c 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -531,6 +531,8 @@ struct BlockDriver {
                              uint64_t parent_perm, uint64_t parent_shared,
                              uint64_t *nperm, uint64_t *nshared);
 
+     bool (*bdrv_need_rw_file_child_during_reopen_rw)(BlockDriverState *bs);
+
     /**
      * Bitmaps should be marked as 'IN_USE' in the image on reopening image
      * as rw. This handler should realize it. It also should unset readonly
diff --git a/block.c b/block.c
index 1a73e310c1..56e1388908 100644
--- a/block.c
+++ b/block.c
@@ -1713,10 +1713,12 @@ static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
                                      uint64_t *shared_perm);
 
 typedef struct BlockReopenQueueEntry {
-     bool prepared;
-     bool perms_checked;
-     BDRVReopenState state;
-     QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
+    bool reopened_file_child_rw;
+    bool changed_file_child_perm_rw;
+    bool prepared;
+    bool perms_checked;
+    BDRVReopenState state;
+    QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
 } BlockReopenQueueEntry;
 
 /*
@@ -3278,6 +3280,105 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
                                    keep_old_opts);
 }
 
+static int bdrv_reopen_set_read_only_drained(BlockDriverState *bs,
+                                             bool read_only,
+                                             Error **errp)
+{
+    BlockReopenQueue *queue;
+    QDict *opts = qdict_new();
+
+    qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
+
+    queue = bdrv_reopen_queue(NULL, bs, opts, true);
+
+    return bdrv_reopen_multiple(queue, errp);
+}
+
+/*
+ * handle_recursive_reopens
+ *
+ * On fail it needs rollback_recursive_reopens to be called.
+ */
+static int handle_recursive_reopens(BlockReopenQueueEntry *current,
+                                    Error **errp)
+{
+    int ret;
+    BlockDriverState *bs = current->state.bs;
+
+    /*
+     * We use the fact that in reopen-queue children are always following
+     * parents.
+     * TODO: Switch BlockReopenQueue to be QTAILQ and use
+     *       QTAILQ_FOREACH_REVERSE.
+     */
+    if (QSIMPLEQ_NEXT(current, entry)) {
+        ret = handle_recursive_reopens(QSIMPLEQ_NEXT(current, entry), errp);
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
+    if ((current->state.flags & BDRV_O_RDWR) && bs->file && bs->drv &&
+        bs->drv->bdrv_need_rw_file_child_during_reopen_rw &&
+        bs->drv->bdrv_need_rw_file_child_during_reopen_rw(bs))
+    {
+        if (!bdrv_is_writable(bs->file->bs)) {
+            ret = bdrv_reopen_set_read_only_drained(bs->file->bs, false, errp);
+            if (ret < 0) {
+                return ret;
+            }
+            current->reopened_file_child_rw = true;
+        }
+
+        if (!(bs->file->perm & BLK_PERM_WRITE)) {
+            ret = bdrv_child_try_set_perm(bs->file,
+                                          bs->file->perm | BLK_PERM_WRITE,
+                                          bs->file->shared_perm, errp);
+            if (ret < 0) {
+                return ret;
+            }
+            current->changed_file_child_perm_rw = true;
+        }
+    }
+
+    return 0;
+}
+
+static int rollback_recursive_reopens(BlockReopenQueue *bs_queue,
+                                      Error **errp)
+{
+    int ret;
+    BlockReopenQueueEntry *bs_entry;
+
+    /*
+     * We use the fact that in reopen-queue children are always following
+     * parents.
+     */
+    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+        BlockDriverState *bs = bs_entry->state.bs;
+
+        if (bs_entry->changed_file_child_perm_rw) {
+            ret = bdrv_child_try_set_perm(bs->file,
+                                          bs->file->perm & ~BLK_PERM_WRITE,
+                                          bs->file->shared_perm, errp);
+
+            if (ret < 0) {
+                return ret;
+            }
+        }
+
+        if (bs_entry->reopened_file_child_rw) {
+            ret = bdrv_reopen_set_read_only_drained(bs, true, errp);
+
+            if (ret < 0) {
+                return ret;
+            }
+        }
+    }
+
+    return 0;
+}
+
 /*
  * Reopen multiple BlockDriverStates atomically & transactionally.
  *
@@ -3297,11 +3398,18 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
  */
 int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
 {
-    int ret = -1;
+    int ret;
     BlockReopenQueueEntry *bs_entry, *next;
 
     assert(bs_queue != NULL);
 
+    ret = handle_recursive_reopens(QSIMPLEQ_FIRST(bs_queue), errp);
+    if (ret < 0) {
+        goto rollback_recursion;
+    }
+
+    ret = -1;
+
     QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
         assert(bs_entry->state.bs->quiesce_counter > 0);
         if (bdrv_reopen_prepare(&bs_entry->state, bs_queue, errp)) {
@@ -3342,7 +3450,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
 
     ret = 0;
 cleanup_perm:
-    QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
+    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
         BDRVReopenState *state = &bs_entry->state;
 
         if (!bs_entry->perms_checked) {
@@ -3359,7 +3467,7 @@ cleanup_perm:
         }
     }
 cleanup:
-    QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
+    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
         if (ret) {
             if (bs_entry->prepared) {
                 bdrv_reopen_abort(&bs_entry->state);
@@ -3370,8 +3478,23 @@ cleanup:
         if (bs_entry->state.new_backing_bs) {
             bdrv_unref(bs_entry->state.new_backing_bs);
         }
+    }
+
+rollback_recursion:
+    if (ret) {
+        Error *local_err = NULL;
+        int ret2 = rollback_recursive_reopens(bs_queue, &local_err);
+
+        if (ret2 < 0) {
+            error_reportf_err(local_err, "Failed to rollback partially "
+                              "succeeded reopen to RW: ");
+        }
+    }
+
+    QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
         g_free(bs_entry);
     }
+
     g_free(bs_queue);
 
     return ret;
@@ -3381,14 +3504,9 @@ int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
                               Error **errp)
 {
     int ret;
-    BlockReopenQueue *queue;
-    QDict *opts = qdict_new();
-
-    qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
 
     bdrv_subtree_drained_begin(bs);
-    queue = bdrv_reopen_queue(NULL, bs, opts, true);
-    ret = bdrv_reopen_multiple(queue, errp);
+    ret = bdrv_reopen_set_read_only_drained(bs, read_only, errp);
     bdrv_subtree_drained_end(bs);
 
     return ret;
-- 
2.18.0

[Qemu-devel] [PATCH v2 2/9] python/qemu: improve event_wait method of vm

[Vladimir Sementsov-Ogievskiy]

Support several names to wait for, which useful, when we don't sure
which event will we get. For example when mirror fails we get
BLOCK_JOB_COMPLETE otherwise we get BLOCK_JOB_READY (and only then,
after completing block-job we get BLOCK_JOB_COMPLETE).

Also, add filtered version for convenient use.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 python/qemu/__init__.py       | 9 ++++++---
 tests/qemu-iotests/iotests.py | 5 +++++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
index 81d9657ec0..5e517025b9 100644
--- a/python/qemu/__init__.py
+++ b/python/qemu/__init__.py
@@ -402,7 +402,7 @@ class QEMUMachine(object):
         self._qmp.clear_events()
         return events
 
-    def event_wait(self, name, timeout=60.0, match=None):
+    def event_wait(self, names, timeout=60.0, match=None):
         """
         Wait for specified timeout on named event in QMP; optionally filter
         results by match.
@@ -412,6 +412,9 @@ class QEMUMachine(object):
            {"foo": {"bar": 1}} matches {"foo": None}
            {"foo": {"bar": 1}} does not matches {"foo": {"baz": None}}
         """
+        if not isinstance(names, list):
+            names = [names]
+
         def event_match(event, match=None):
             if match is None:
                 return True
@@ -430,14 +433,14 @@ class QEMUMachine(object):
 
         # Search cached events
         for event in self._events:
-            if (event['event'] == name) and event_match(event, match):
+            if (event['event'] in names) and event_match(event, match):
                 self._events.remove(event)
                 return event
 
         # Poll for new events
         while True:
             event = self._qmp.pull_event(wait=timeout)
-            if (event['event'] == name) and event_match(event, match):
+            if (event['event'] in names) and event_match(event, match):
                 return event
             self._events.append(event)
 
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7bde380d96..4218fc908b 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -532,6 +532,11 @@ class VM(qtest.QEMUQtestMachine):
         log(result, filters, indent=indent)
         return result
 
+    def event_wait_log(self, names, **kwargs):
+        event = self.event_wait(names, **kwargs)
+        log(event, filters=[filter_qmp_event])
+        return event
+
     # Returns None on success, and an error string on failure
     def run_job(self, job, auto_finalize=True, auto_dismiss=False):
         error = None
-- 
2.18.0

[Qemu-devel] [PATCH v2 3/9] iotests: add test 255 to check bitmap life after snapshot + commit

[Vladimir Sementsov-Ogievskiy]

Two testcases with persistent bitmaps are not added here, as there are
bugs to be fixed soon.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/255     | 84 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/255.out | 17 ++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 102 insertions(+)
 create mode 100755 tests/qemu-iotests/255
 create mode 100644 tests/qemu-iotests/255.out

diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255
new file mode 100755
index 0000000000..1b3c081a68
--- /dev/null
+++ b/tests/qemu-iotests/255
@@ -0,0 +1,84 @@
+#!/usr/bin/env python
+#
+# Tests for temporary external snapshot when we have bitmaps.
+#
+# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import iotests
+from iotests import qemu_img_create, file_path, log
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+
+base, top = file_path('base', 'top')
+size = 64 * 1024 * 3
+
+
+def print_bitmap(msg, vm):
+    result = vm.qmp('query-block')['return'][0]
+    if 'dirty-bitmaps' in result:
+        bitmap = result['dirty-bitmaps'][0]
+        log('{}: name={} dirty-clusters={}'.format(msg, bitmap['name'],
+            bitmap['count'] // 64 // 1024))
+    else:
+        log(msg + ': not found')
+
+
+def test(persistent, restart):
+    assert persistent or not restart
+    log("\nTestcase {}persistent {} restart\n".format(
+            '' if persistent else 'non-', 'with' if restart else 'without'))
+
+    qemu_img_create('-f', iotests.imgfmt, base, str(size))
+
+    vm = iotests.VM().add_drive(base)
+    vm.launch()
+
+    vm.qmp_log('block-dirty-bitmap-add', node='drive0', name='bitmap0',
+               persistent=persistent)
+    vm.hmp_qemu_io('drive0', 'write 0 64K')
+    print_bitmap('initial bitmap', vm)
+
+    vm.qmp_log('blockdev-snapshot-sync', device='drive0', snapshot_file=top,
+               format=iotests.imgfmt, filters=[iotests.filter_qmp_testfiles])
+    vm.hmp_qemu_io('drive0', 'write 64K 512')
+    print_bitmap('check that no bitmaps are in snapshot', vm)
+
+    if restart:
+        log("... Restart ...")
+        vm.shutdown()
+        vm = iotests.VM().add_drive(top)
+        vm.launch()
+
+    vm.qmp_log('block-commit', device='drive0', top=top,
+               filters=[iotests.filter_qmp_testfiles])
+    ev = vm.event_wait_log(['BLOCK_JOB_READY', 'BLOCK_JOB_COMPLETED'])
+    if (ev['event'] == 'BLOCK_JOB_COMPLETED'):
+        vm.shutdown()
+        log(vm.get_log())
+        exit()
+
+    vm.qmp_log('block-job-complete', device='drive0')
+    vm.event_wait_log('BLOCK_JOB_COMPLETED')
+    print_bitmap('check bitmap after commit', vm)
+
+    vm.hmp_qemu_io('drive0', 'write 128K 64K')
+    print_bitmap('check updated bitmap', vm)
+
+    vm.shutdown()
+
+
+test(persistent=False, restart=False)
diff --git a/tests/qemu-iotests/255.out b/tests/qemu-iotests/255.out
new file mode 100644
index 0000000000..5239d27c46
--- /dev/null
+++ b/tests/qemu-iotests/255.out
@@ -0,0 +1,17 @@
+
+Testcase non-persistent without restart
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": "drive0", "persistent": false}}
+{"return": {}}
+initial bitmap: name=bitmap0 dirty-clusters=1
+{"execute": "blockdev-snapshot-sync", "arguments": {"device": "drive0", "format": "qcow2", "snapshot-file": "TEST_DIR/PID-top"}}
+{"return": {}}
+check that no bitmaps are in snapshot: not found
+{"execute": "block-commit", "arguments": {"device": "drive0", "top": "TEST_DIR/PID-top"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "block-job-complete", "arguments": {"device": "drive0"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+check bitmap after commit: name=bitmap0 dirty-clusters=2
+check updated bitmap: name=bitmap0 dirty-clusters=3
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 859c4b5e9f..88049ad46c 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -265,3 +265,4 @@
 252 rw auto backing quick
 253 rw auto quick
 254 rw auto backing quick
+255 rw auto quick
-- 
2.18.0

[Qemu-devel] [PATCH v2 4/9] block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps

[Vladimir Sementsov-Ogievskiy]

Firstly, no reason to optimize failure path. Then, function name is
ambiguous: it checks for readonly and similar things, but someone may
think that it will ignore normal bitmaps which was just unchanged, and
this is in bad relation with the fact that we should drop IN_USE flag
for unchanged bitmaps in the image.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 include/block/dirty-bitmap.h |  1 -
 block/dirty-bitmap.c         | 12 ------------
 block/qcow2-bitmap.c         | 23 +++++++++++++----------
 3 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 8044ace63e..816022972b 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -105,7 +105,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
 bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap);
 bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
-bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
 BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
                                         BdrvDirtyBitmap *bitmap);
 char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 49646a30e6..d4923ee614 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -775,18 +775,6 @@ bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap)
     return bitmap->inconsistent;
 }
 
-bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
-{
-    BdrvDirtyBitmap *bm;
-    QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
-        if (bm->persistent && !bm->readonly && !bm->migration) {
-            return true;
-        }
-    }
-
-    return false;
-}
-
 BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
                                         BdrvDirtyBitmap *bitmap)
 {
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b2487101ed..60b79f1dac 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1456,16 +1456,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
     Qcow2Bitmap *bm;
     QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables;
     Qcow2BitmapTable *tb, *tb_next;
-
-    if (!bdrv_has_changed_persistent_bitmaps(bs)) {
-        /* nothing to do */
-        return;
-    }
-
-    if (!can_write(bs)) {
-        error_setg(errp, "No write access");
-        return;
-    }
+    bool need_write = false;
 
     QSIMPLEQ_INIT(&drop_tables);
 
@@ -1493,6 +1484,8 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
             continue;
         }
 
+        need_write = true;
+
         if (check_constraints_on_bitmap(bs, name, granularity, errp) < 0) {
             error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ",
                           name);
@@ -1531,6 +1524,15 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
         bm->dirty_bitmap = bitmap;
     }
 
+    if (!need_write) {
+        goto success;
+    }
+
+    if (!can_write(bs)) {
+        error_setg(errp, "No write access");
+        goto fail;
+    }
+
     /* allocate clusters and store bitmaps */
     QSIMPLEQ_FOREACH(bm, bm_list, entry) {
         if (bm->dirty_bitmap == NULL) {
@@ -1572,6 +1574,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
         bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
     }
 
+success:
     bitmap_list_free(bm_list);
     return;
 
-- 
2.18.0

[Qemu-devel] [PATCH v2 5/9] block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint()

[Vladimir Sementsov-Ogievskiy]

The function is unused, drop it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.h        |  2 --
 block/qcow2-bitmap.c | 15 +--------------
 2 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 567375e56c..88a2030f54 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -732,8 +732,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
 bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(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);
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 60b79f1dac..fbeee37243 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1102,8 +1102,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
     return list;
 }
 
-int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
-                                 Error **errp)
+int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     Qcow2BitmapList *bm_list;
@@ -1111,10 +1110,6 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
     GSList *ro_dirty_bitmaps = NULL;
     int ret = 0;
 
-    if (header_updated != NULL) {
-        *header_updated = false;
-    }
-
     if (s->nb_bitmaps == 0) {
         /* No bitmaps - nothing to do */
         return 0;
@@ -1156,9 +1151,6 @@ int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
             error_setg_errno(errp, -ret, "Can't update bitmap directory");
             goto out;
         }
-        if (header_updated != NULL) {
-            *header_updated = true;
-        }
         g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
     }
 
@@ -1169,11 +1161,6 @@ out:
     return ret;
 }
 
-int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
-{
-    return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp);
-}
-
 /* Checks to see if it's safe to resize bitmaps */
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp)
 {
-- 
2.18.0

[Qemu-devel] [PATCH v2 6/9] block/qcow2-bitmap: do not remove bitmaps on reopen-ro

[Vladimir Sementsov-Ogievskiy]

qcow2_reopen_bitmaps_ro wants to store bitmaps and then mark them all
readonly. But the latter don't work, as
qcow2_store_persistent_dirty_bitmaps removes bitmaps after storing.
It's OK for inactivation but bad idea for reopen-ro. And this leads to
the following bug:

Assume we have persistent bitmap 'bitmap0'.
Create external snapshot
  bitmap0 is stored and therefore removed
Commit snapshot
  now we have no bitmaps
Do some writes from guest (*)
  they are not marked in bitmap
Shutdown
Start
  bitmap0 is loaded as valid, but it is actually broken! It misses
  writes (*)
Incremental backup
  it will be inconsistent

So, let's stop removing bitmaps on reopen-ro. But don't rejoice:
reopening bitmaps to rw is broken too, so the whole scenario will not
work after this patch and we can't enable corresponding test cases in
255 iotests still. Reopening bitmaps rw will be fixed in the following
patches.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.h        |  3 ++-
 block/qcow2-bitmap.c | 46 +++++++++++++++++++++++++++++---------------
 block/qcow2.c        |  2 +-
 3 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 88a2030f54..4c8435141b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -734,7 +734,8 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
                                                 Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+                                          bool release_stored, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
 bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
                                       const char *name,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index fbeee37243..25b1e069a7 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1432,7 +1432,29 @@ fail:
     bitmap_list_free(bm_list);
 }
 
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+/*
+ * qcow2_store_persistent_dirty_bitmaps
+ *
+ * Stores persistent BdrvDirtyBitmap's.
+ *
+ * @release_stored: if true, release BdrvDirtyBitmap's after storing to the
+ * image. This is used in two cases, both via qcow2_inactivate:
+ * 1. bdrv_close: It's correct to remove bitmaps on close.
+ * 2. migration: If bitmaps are migrated through migration channel via
+ *    'dirty-bitmaps' migration capability they are not handled by this code.
+ *    Otherwise, it's OK to drop BdrvDirtyBitmap's and reload them on
+ *    invalidation.
+ *
+ * Anyway, it's correct to remove BdrvDirtyBitmap's on inactivation, as
+ * inactivation means that we lose control on disk, and therefore on bitmaps,
+ * we should sync them and do not touch more.
+ *
+ * Contrariwise, we don't want to release any bitmaps on just reopen-to-ro,
+ * when we need to store them, as image is still under our control, and it's
+ * good to keep all the bitmaps in read-only mode.
+ */
+void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+                                          bool release_stored, Error **errp)
 {
     BdrvDirtyBitmap *bitmap;
     BDRVQcow2State *s = bs->opaque;
@@ -1545,20 +1567,14 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
         g_free(tb);
     }
 
-    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-        /* For safety, we remove bitmap after storing.
-         * We may be here in two cases:
-         * 1. bdrv_close. It's ok to drop bitmap.
-         * 2. inactivation. It means migration without 'dirty-bitmaps'
-         *    capability, so bitmaps are not marked with
-         *    BdrvDirtyBitmap.migration flags. It's not bad to drop them too,
-         *    and reload on invalidation.
-         */
-        if (bm->dirty_bitmap == NULL) {
-            continue;
-        }
+    if (release_stored) {
+        QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+            if (bm->dirty_bitmap == NULL) {
+                continue;
+            }
 
-        bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
+            bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
+        }
     }
 
 success:
@@ -1586,7 +1602,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
     BdrvDirtyBitmap *bitmap;
     Error *local_err = NULL;
 
-    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
+    qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         return -EINVAL;
diff --git a/block/qcow2.c b/block/qcow2.c
index f2cb131048..02d8ce7534 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2344,7 +2344,7 @@ static int qcow2_inactivate(BlockDriverState *bs)
     int ret, result = 0;
     Error *local_err = NULL;
 
-    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
+    qcow2_store_persistent_dirty_bitmaps(bs, true, &local_err);
     if (local_err != NULL) {
         result = -EINVAL;
         error_reportf_err(local_err, "Lost persistent bitmaps during "
-- 
2.18.0

[Qemu-devel] [PATCH v2 7/9] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw

[Vladimir Sementsov-Ogievskiy]

- Correct check for write access to file child, and in correct place
  (only if we want to write).
- Support reopen rw -> rw (which will be used in furhter patches),
  for example, !bdrv_dirty_bitmap_readonly() is not a corruption if
  bitmap is marked IN_USE in the image.
- Consider unexpected bitmap as a corruption and check other
  combinations of in-image and in-RAM bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-bitmap.c | 56 +++++++++++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 19 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 25b1e069a7..f48b5b4eaf 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1108,18 +1108,14 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
     Qcow2BitmapList *bm_list;
     Qcow2Bitmap *bm;
     GSList *ro_dirty_bitmaps = NULL;
-    int ret = 0;
+    int ret = -EINVAL;
+    bool need_header_update = false;
 
     if (s->nb_bitmaps == 0) {
         /* No bitmaps - nothing to do */
         return 0;
     }
 
-    if (!can_write(bs)) {
-        error_setg(errp, "Can't write to the image on reopening bitmaps rw");
-        return -EINVAL;
-    }
-
     bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
                                s->bitmap_directory_size, errp);
     if (bm_list == NULL) {
@@ -1128,32 +1124,54 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
 
     QSIMPLEQ_FOREACH(bm, bm_list, entry) {
         BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(bs, bm->name);
-        if (bitmap == NULL) {
-            continue;
-        }
 
-        if (!bdrv_dirty_bitmap_readonly(bitmap)) {
-            error_setg(errp, "Bitmap %s was loaded prior to rw-reopen, but was "
-                       "not marked as readonly. This is a bug, something went "
-                       "wrong. All of the bitmaps may be corrupted", bm->name);
-            ret = -EINVAL;
+        if (!bitmap) {
+            error_setg(errp, "Unexpected bitmap '%s' in the image '%s'",
+                       bm->name, bs->filename);
             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)) {
+            if (!bdrv_dirty_bitmap_readonly(bitmap)) {
+                error_setg(errp, "Corruption: bitmap '%s' is not marked IN_USE "
+                           "in the image '%s' and not marked readonly in RAM",
+                           bm->name, bs->filename);
+                goto out;
+            }
+            if (bdrv_dirty_bitmap_inconsistent(bitmap)) {
+                error_setg(errp, "Corruption: bitmap '%s' is inconsistent but "
+                           "is not marked IN_USE in the image '%s'", bm->name,
+                           bs->filename);
+                goto out;
+            }
+
+            bm->flags |= BME_FLAG_IN_USE;
+            need_header_update = true;
+        }
+
+        if (bdrv_dirty_bitmap_readonly(bitmap)) {
+            ro_dirty_bitmaps = g_slist_append(ro_dirty_bitmaps, bitmap);
+        }
     }
 
-    if (ro_dirty_bitmaps != NULL) {
+    if (need_header_update) {
+        if (!can_write(bs->file->bs) || !(bs->file->perm & BLK_PERM_WRITE)) {
+            error_setg(errp, "Failed to reopen bitmaps rw: no write access "
+                       "the protocol file");
+            goto out;
+        }
+
         /* in_use flags must be updated */
         ret = update_ext_header_and_dir_in_place(bs, bm_list);
         if (ret < 0) {
-            error_setg_errno(errp, -ret, "Can't update bitmap directory");
+            error_setg_errno(errp, -ret, "Cannot update bitmap directory");
             goto out;
         }
-        g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
     }
 
+    g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
+    ret = 0;
+
 out:
     g_slist_free(ro_dirty_bitmaps);
     bitmap_list_free(bm_list);
-- 
2.18.0

[Qemu-devel] [PATCH v2 8/9] block/qcow2-bitmap: fix reopening bitmaps to RW

[Vladimir Sementsov-Ogievskiy]

Currently reopening bitmaps to RW can't work, as qcow2 needs write
access to file child, to mark bitmaps in-image with IN_USE flag.

The possibility to write-access file child during reopen-RW was
implemented several patches ago with help of
.bdrv_need_rw_file_child_during_reopen_rw handler. Let's use
this new API to fix bitmaps reopening.

Add corresponding test-cases to 255 iotest.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.h              |  1 +
 block/qcow2-bitmap.c       |  6 ++++++
 block/qcow2.c              |  1 +
 tests/qemu-iotests/255     |  2 ++
 tests/qemu-iotests/255.out | 35 +++++++++++++++++++++++++++++++++++
 5 files changed, 45 insertions(+)

diff --git a/block/qcow2.h b/block/qcow2.h
index 4c8435141b..d6398bd030 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -726,6 +726,7 @@ void *qcow2_cache_is_table_offset(Qcow2Cache *c, uint64_t offset);
 void qcow2_cache_discard(Qcow2Cache *c, void *table);
 
 /* qcow2-bitmap.c functions */
+bool qcow2_has_bitmaps(BlockDriverState *bs);
 int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                                   void **refcount_table,
                                   int64_t *refcount_table_size);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index f48b5b4eaf..803818b29a 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1102,6 +1102,12 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
     return list;
 }
 
+bool qcow2_has_bitmaps(BlockDriverState *bs)
+{
+    BDRVQcow2State *s = bs->opaque;
+    return s->nb_bitmaps;
+}
+
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
diff --git a/block/qcow2.c b/block/qcow2.c
index 02d8ce7534..ffc067a8ac 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5218,6 +5218,7 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
     .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
     .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap,
+    .bdrv_need_rw_file_child_during_reopen_rw = qcow2_has_bitmaps,
 };
 
 static void bdrv_qcow2_init(void)
diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255
index 1b3c081a68..5e8d01686f 100755
--- a/tests/qemu-iotests/255
+++ b/tests/qemu-iotests/255
@@ -82,3 +82,5 @@ def test(persistent, restart):
 
 
 test(persistent=False, restart=False)
+test(persistent=True, restart=False)
+test(persistent=True, restart=True)
diff --git a/tests/qemu-iotests/255.out b/tests/qemu-iotests/255.out
index 5239d27c46..2f0d98d036 100644
--- a/tests/qemu-iotests/255.out
+++ b/tests/qemu-iotests/255.out
@@ -15,3 +15,38 @@ check that no bitmaps are in snapshot: not found
 {"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 check bitmap after commit: name=bitmap0 dirty-clusters=2
 check updated bitmap: name=bitmap0 dirty-clusters=3
+
+Testcase persistent without restart
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": "drive0", "persistent": true}}
+{"return": {}}
+initial bitmap: name=bitmap0 dirty-clusters=1
+{"execute": "blockdev-snapshot-sync", "arguments": {"device": "drive0", "format": "qcow2", "snapshot-file": "TEST_DIR/PID-top"}}
+{"return": {}}
+check that no bitmaps are in snapshot: not found
+{"execute": "block-commit", "arguments": {"device": "drive0", "top": "TEST_DIR/PID-top"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "block-job-complete", "arguments": {"device": "drive0"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+check bitmap after commit: name=bitmap0 dirty-clusters=2
+check updated bitmap: name=bitmap0 dirty-clusters=3
+
+Testcase persistent with restart
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": "drive0", "persistent": true}}
+{"return": {}}
+initial bitmap: name=bitmap0 dirty-clusters=1
+{"execute": "blockdev-snapshot-sync", "arguments": {"device": "drive0", "format": "qcow2", "snapshot-file": "TEST_DIR/PID-top"}}
+{"return": {}}
+check that no bitmaps are in snapshot: not found
+... Restart ...
+{"execute": "block-commit", "arguments": {"device": "drive0", "top": "TEST_DIR/PID-top"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "block-job-complete", "arguments": {"device": "drive0"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+check bitmap after commit: name=bitmap0 dirty-clusters=2
+check updated bitmap: name=bitmap0 dirty-clusters=3
-- 
2.18.0

[Qemu-devel] [PATCH v2 9/9] qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_prepare

[Vladimir Sementsov-Ogievskiy]

Since we have used .bdrv_need_rw_file_child_during_reopen_rw handler,
and have write access to file child in reopen-prepare, and we prepared
qcow2_reopen_bitmaps_rw to be called from reopening rw -> rw, we now
can simple move qcow2_reopen_bitmaps_rw() call to
qcow2_reopen_prepare() and handle errors as befits.

Hacky handler .bdrv_reopen_bitmaps_rw is not needed more and therefore
dropped.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/block_int.h |  6 ------
 block.c                   | 19 -------------------
 block/qcow2.c             |  6 +++++-
 3 files changed, 5 insertions(+), 26 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7b22fb5d9c..a35b3e0d96 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -533,12 +533,6 @@ struct BlockDriver {
 
      bool (*bdrv_need_rw_file_child_during_reopen_rw)(BlockDriverState *bs);
 
-    /**
-     * Bitmaps should be marked as 'IN_USE' in the image on reopening image
-     * as rw. This handler should realize it. It also should unset readonly
-     * field of BlockDirtyBitmap's in case of success.
-     */
-    int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
     bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs,
                                             const char *name,
                                             uint32_t granularity,
diff --git a/block.c b/block.c
index 56e1388908..b5527ae713 100644
--- a/block.c
+++ b/block.c
@@ -3894,16 +3894,12 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
     BlockDriver *drv;
     BlockDriverState *bs;
     BdrvChild *child;
-    bool old_can_write, new_can_write;
 
     assert(reopen_state != NULL);
     bs = reopen_state->bs;
     drv = bs->drv;
     assert(drv != NULL);
 
-    old_can_write =
-        !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
-
     /* If there are any driver level actions to take */
     if (drv->bdrv_reopen_commit) {
         drv->bdrv_reopen_commit(reopen_state);
@@ -3947,21 +3943,6 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
     }
 
     bdrv_refresh_limits(bs, NULL);
-
-    new_can_write =
-        !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
-    if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) {
-        Error *local_err = NULL;
-        if (drv->bdrv_reopen_bitmaps_rw(bs, &local_err) < 0) {
-            /* This is not fatal, bitmaps just left read-only, so all following
-             * writes will fail. User can remove read-only bitmaps to unblock
-             * writes.
-             */
-            error_reportf_err(local_err,
-                              "%s: Failed to make dirty bitmaps writable: ",
-                              bdrv_get_node_name(bs));
-        }
-    }
 }
 
 /*
diff --git a/block/qcow2.c b/block/qcow2.c
index ffc067a8ac..945fae74b5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1815,6 +1815,11 @@ static int qcow2_reopen_prepare(BDRVReopenState *state,
         if (ret < 0) {
             goto fail;
         }
+    } else {
+        ret = qcow2_reopen_bitmaps_rw(state->bs, errp);
+        if (ret < 0) {
+            goto fail;
+        }
     }
 
     return 0;
@@ -5215,7 +5220,6 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_detach_aio_context  = qcow2_detach_aio_context,
     .bdrv_attach_aio_context  = qcow2_attach_aio_context,
 
-    .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
     .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
     .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap,
     .bdrv_need_rw_file_child_during_reopen_rw = qcow2_has_bitmaps,
-- 
2.18.0

Re: [Qemu-devel] [PATCH v2 2/9] python/qemu: improve event_wait method of vm

[John Snow]

On 5/31/19 12:31 PM, Vladimir Sementsov-Ogievskiy wrote:
> Support several names to wait for, which useful, when we don't sure
> which event will we get. For example when mirror fails we get
> BLOCK_JOB_COMPLETE otherwise we get BLOCK_JOB_READY (and only then,
> after completing block-job we get BLOCK_JOB_COMPLETE).
> 
> Also, add filtered version for convenient use.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  python/qemu/__init__.py       | 9 ++++++---
>  tests/qemu-iotests/iotests.py | 5 +++++
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
> index 81d9657ec0..5e517025b9 100644
> --- a/python/qemu/__init__.py
> +++ b/python/qemu/__init__.py
> @@ -402,7 +402,7 @@ class QEMUMachine(object):
>          self._qmp.clear_events()
>          return events
>  
> -    def event_wait(self, name, timeout=60.0, match=None):
> +    def event_wait(self, names, timeout=60.0, match=None):
>          """
>          Wait for specified timeout on named event in QMP; optionally filter
>          results by match.
> @@ -412,6 +412,9 @@ class QEMUMachine(object):
>             {"foo": {"bar": 1}} matches {"foo": None}
>             {"foo": {"bar": 1}} does not matches {"foo": {"baz": None}}
>          """
> +        if not isinstance(names, list):
> +            names = [names]
> +
>          def event_match(event, match=None):
>              if match is None:
>                  return True
> @@ -430,14 +433,14 @@ class QEMUMachine(object):
>  
>          # Search cached events
>          for event in self._events:
> -            if (event['event'] == name) and event_match(event, match):
> +            if (event['event'] in names) and event_match(event, match):
>                  self._events.remove(event)
>                  return event
>  
>          # Poll for new events
>          while True:
>              event = self._qmp.pull_event(wait=timeout)
> -            if (event['event'] == name) and event_match(event, match):
> +            if (event['event'] in names) and event_match(event, match):
>                  return event
>              self._events.append(event)
>  
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 7bde380d96..4218fc908b 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -532,6 +532,11 @@ class VM(qtest.QEMUQtestMachine):
>          log(result, filters, indent=indent)
>          return result
>  
> +    def event_wait_log(self, names, **kwargs):
> +        event = self.event_wait(names, **kwargs)
> +        log(event, filters=[filter_qmp_event])
> +        return event
> +
>      # Returns None on success, and an error string on failure
>      def run_job(self, job, auto_finalize=True, auto_dismiss=False):
>          error = None
> 

There's something like this in the queue; see:
[Qemu-devel] [PATCH v3 3/5] QEMUMachine: add events_wait method.

Though yours is a lot shorter, actually, because I made separate
event_wait and events_wait calls instead of just throwing non-iterables
in a list.

--js

Re: [Qemu-devel] [PATCH v2 3/9] iotests: add test 255 to check bitmap life after snapshot + commit

[John Snow]

On 5/31/19 12:31 PM, Vladimir Sementsov-Ogievskiy wrote:
> Two testcases with persistent bitmaps are not added here, as there are
> bugs to be fixed soon.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/255     | 84 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/255.out | 17 ++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 102 insertions(+)
>  create mode 100755 tests/qemu-iotests/255
>  create mode 100644 tests/qemu-iotests/255.out
> 
> diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255
> new file mode 100755
> index 0000000000..1b3c081a68
> --- /dev/null
> +++ b/tests/qemu-iotests/255
> @@ -0,0 +1,84 @@
> +#!/usr/bin/env python
> +#
> +# Tests for temporary external snapshot when we have bitmaps.
> +#
> +# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import iotests
> +from iotests import qemu_img_create, file_path, log
> +
> +iotests.verify_image_format(supported_fmts=['qcow2'])
> +
> +base, top = file_path('base', 'top')
> +size = 64 * 1024 * 3
> +
> +
> +def print_bitmap(msg, vm):
> +    result = vm.qmp('query-block')['return'][0]
> +    if 'dirty-bitmaps' in result:
> +        bitmap = result['dirty-bitmaps'][0]
> +        log('{}: name={} dirty-clusters={}'.format(msg, bitmap['name'],
> +            bitmap['count'] // 64 // 1024))
> +    else:
> +        log(msg + ': not found')
> +
> +
> +def test(persistent, restart):
> +    assert persistent or not restart
> +    log("\nTestcase {}persistent {} restart\n".format(
> +            '' if persistent else 'non-', 'with' if restart else 'without'))
> +
> +    qemu_img_create('-f', iotests.imgfmt, base, str(size))
> +
> +    vm = iotests.VM().add_drive(base)
> +    vm.launch()
> +
> +    vm.qmp_log('block-dirty-bitmap-add', node='drive0', name='bitmap0',
> +               persistent=persistent)
> +    vm.hmp_qemu_io('drive0', 'write 0 64K')
> +    print_bitmap('initial bitmap', vm)
> +
> +    vm.qmp_log('blockdev-snapshot-sync', device='drive0', snapshot_file=top,
> +               format=iotests.imgfmt, filters=[iotests.filter_qmp_testfiles])
> +    vm.hmp_qemu_io('drive0', 'write 64K 512')
> +    print_bitmap('check that no bitmaps are in snapshot', vm)
> +
> +    if restart:
> +        log("... Restart ...")
> +        vm.shutdown()
> +        vm = iotests.VM().add_drive(top)
> +        vm.launch()
> +
> +    vm.qmp_log('block-commit', device='drive0', top=top,
> +               filters=[iotests.filter_qmp_testfiles])
> +    ev = vm.event_wait_log(['BLOCK_JOB_READY', 'BLOCK_JOB_COMPLETED'])
> +    if (ev['event'] == 'BLOCK_JOB_COMPLETED'):
> +        vm.shutdown()
> +        log(vm.get_log())
> +        exit()
> +

What's the purpose of this conditional? what causes the difference in
behavior that we need to handle it?

> +    vm.qmp_log('block-job-complete', device='drive0')
> +    vm.event_wait_log('BLOCK_JOB_COMPLETED')
> +    print_bitmap('check bitmap after commit', vm)
> +
> +    vm.hmp_qemu_io('drive0', 'write 128K 64K')
> +    print_bitmap('check updated bitmap', vm)
> +
> +    vm.shutdown()
> +
> +
> +test(persistent=False, restart=False)
> diff --git a/tests/qemu-iotests/255.out b/tests/qemu-iotests/255.out
> new file mode 100644
> index 0000000000..5239d27c46
> --- /dev/null
> +++ b/tests/qemu-iotests/255.out
> @@ -0,0 +1,17 @@
> +
> +Testcase non-persistent without restart
> +
> +{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": "drive0", "persistent": false}}
> +{"return": {}}
> +initial bitmap: name=bitmap0 dirty-clusters=1
> +{"execute": "blockdev-snapshot-sync", "arguments": {"device": "drive0", "format": "qcow2", "snapshot-file": "TEST_DIR/PID-top"}}
> +{"return": {}}
> +check that no bitmaps are in snapshot: not found
> +{"execute": "block-commit", "arguments": {"device": "drive0", "top": "TEST_DIR/PID-top"}}
> +{"return": {}}
> +{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> +{"execute": "block-job-complete", "arguments": {"device": "drive0"}}
> +{"return": {}}
> +{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> +check bitmap after commit: name=bitmap0 dirty-clusters=2
> +check updated bitmap: name=bitmap0 dirty-clusters=3
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 859c4b5e9f..88049ad46c 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -265,3 +265,4 @@
>  252 rw auto backing quick
>  253 rw auto quick
>  254 rw auto backing quick
> +255 rw auto quick
> 

Re: [Qemu-devel] [PATCH v2 5/9] block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint()

[John Snow]

On 5/31/19 12:31 PM, Vladimir Sementsov-Ogievskiy wrote:
> The function is unused, drop it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2.h        |  2 --
>  block/qcow2-bitmap.c | 15 +--------------
>  2 files changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 567375e56c..88a2030f54 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -732,8 +732,6 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>  bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>  Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(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);
>  int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
>  void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 60b79f1dac..fbeee37243 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1102,8 +1102,7 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>      return list;
>  }
>  
> -int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
> -                                 Error **errp)
> +int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)

Well, not used outside of this file. :)

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH v2 6/9] block/qcow2-bitmap: do not remove bitmaps on reopen-ro

[John Snow]

On 5/31/19 12:31 PM, Vladimir Sementsov-Ogievskiy wrote:
> qcow2_reopen_bitmaps_ro wants to store bitmaps and then mark them all
> readonly. But the latter don't work, as
> qcow2_store_persistent_dirty_bitmaps removes bitmaps after storing.
> It's OK for inactivation but bad idea for reopen-ro. And this leads to
> the following bug:
> 
> Assume we have persistent bitmap 'bitmap0'.
> Create external snapshot
>   bitmap0 is stored and therefore removed
> Commit snapshot
>   now we have no bitmaps
> Do some writes from guest (*)
>   they are not marked in bitmap
> Shutdown
> Start
>   bitmap0 is loaded as valid, but it is actually broken! It misses
>   writes (*)
> Incremental backup
>   it will be inconsistent
> 
> So, let's stop removing bitmaps on reopen-ro. But don't rejoice:
> reopening bitmaps to rw is broken too, so the whole scenario will not
> work after this patch and we can't enable corresponding test cases in
> 255 iotests still. Reopening bitmaps rw will be fixed in the following
> patches.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2.h        |  3 ++-
>  block/qcow2-bitmap.c | 46 +++++++++++++++++++++++++++++---------------
>  block/qcow2.c        |  2 +-
>  3 files changed, 34 insertions(+), 17 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 88a2030f54..4c8435141b 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -734,7 +734,8 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>                                                  Error **errp);
>  int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>  int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
> +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> +                                          bool release_stored, Error **errp);
>  int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>  bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>                                        const char *name,
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index fbeee37243..25b1e069a7 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1432,7 +1432,29 @@ fail:
>      bitmap_list_free(bm_list);
>  }
>  
> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> +/*
> + * qcow2_store_persistent_dirty_bitmaps
> + *
> + * Stores persistent BdrvDirtyBitmap's.
> + *

No apostrophe for plural's

> + * @release_stored: if true, release BdrvDirtyBitmap's after storing to the
> + * image. This is used in two cases, both via qcow2_inactivate:
> + * 1. bdrv_close: It's correct to remove bitmaps on close.
> + * 2. migration: If bitmaps are migrated through migration channel via
> + *    'dirty-bitmaps' migration capability they are not handled by this code.
> + *    Otherwise, it's OK to drop BdrvDirtyBitmap's and reload them on
> + *    invalidation.
> + *
> + * Anyway, it's correct to remove BdrvDirtyBitmap's on inactivation, as
> + * inactivation means that we lose control on disk, and therefore on bitmaps,
> + * we should sync them and do not touch more.
> + *
> + * Contrariwise, we don't want to release any bitmaps on just reopen-to-ro,
> + * when we need to store them, as image is still under our control, and it's
> + * good to keep all the bitmaps in read-only mode.
> + */

I have to admit that 'Contrariwise' is not an everyday term for me. You
should keep it in here just for fun, in my opinion.

Regarding "it's good to keep all the bitmaps in read-only mode":
More directly, keeping them read-only is correct because this is what
would happen if we opened the node readonly to begin with, and whether
we opened directly or reopened to that state shouldn't matter for the
state we get afterward.

> +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> +                                          bool release_stored, Error **errp)
>  {
>      BdrvDirtyBitmap *bitmap;
>      BDRVQcow2State *s = bs->opaque;
> @@ -1545,20 +1567,14 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>          g_free(tb);
>      }
>  
> -    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> -        /* For safety, we remove bitmap after storing.
> -         * We may be here in two cases:
> -         * 1. bdrv_close. It's ok to drop bitmap.
> -         * 2. inactivation. It means migration without 'dirty-bitmaps'
> -         *    capability, so bitmaps are not marked with
> -         *    BdrvDirtyBitmap.migration flags. It's not bad to drop them too,
> -         *    and reload on invalidation.
> -         */
> -        if (bm->dirty_bitmap == NULL) {
> -            continue;
> -        }
> +    if (release_stored) {
> +        QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> +            if (bm->dirty_bitmap == NULL) {
> +                continue;
> +            }
>  
> -        bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
> +            bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
> +        }
>      }
>  
>  success:
> @@ -1586,7 +1602,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
>      BdrvDirtyBitmap *bitmap;
>      Error *local_err = NULL;
>  
> -    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
> +    qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err);
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
>          return -EINVAL;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index f2cb131048..02d8ce7534 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2344,7 +2344,7 @@ static int qcow2_inactivate(BlockDriverState *bs)
>      int ret, result = 0;
>      Error *local_err = NULL;
>  
> -    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
> +    qcow2_store_persistent_dirty_bitmaps(bs, true, &local_err);
>      if (local_err != NULL) {
>          result = -EINVAL;
>          error_reportf_err(local_err, "Lost persistent bitmaps during "
> 

code:
Reviewed-by: John Snow <jsnow@redhat.com>

(You can adjust the docs as you need to on further review, if any, and
keep that RB. --js)

Re: [Qemu-devel] [PATCH v2 3/9] iotests: add test 255 to check bitmap life after snapshot + commit

[Vladimir Sementsov-Ogievskiy]

01.06.2019 2:42, John Snow wrote:
> 
> 
> On 5/31/19 12:31 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Two testcases with persistent bitmaps are not added here, as there are
>> bugs to be fixed soon.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/255     | 84 ++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/255.out | 17 ++++++++
>>   tests/qemu-iotests/group   |  1 +
>>   3 files changed, 102 insertions(+)
>>   create mode 100755 tests/qemu-iotests/255
>>   create mode 100644 tests/qemu-iotests/255.out
>>
>> diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255
>> new file mode 100755
>> index 0000000000..1b3c081a68
>> --- /dev/null
>> +++ b/tests/qemu-iotests/255
>> @@ -0,0 +1,84 @@
>> +#!/usr/bin/env python
>> +#
>> +# Tests for temporary external snapshot when we have bitmaps.
>> +#
>> +# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 2 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +#
>> +
>> +import iotests
>> +from iotests import qemu_img_create, file_path, log
>> +
>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>> +
>> +base, top = file_path('base', 'top')
>> +size = 64 * 1024 * 3
>> +
>> +
>> +def print_bitmap(msg, vm):
>> +    result = vm.qmp('query-block')['return'][0]
>> +    if 'dirty-bitmaps' in result:
>> +        bitmap = result['dirty-bitmaps'][0]
>> +        log('{}: name={} dirty-clusters={}'.format(msg, bitmap['name'],
>> +            bitmap['count'] // 64 // 1024))
>> +    else:
>> +        log(msg + ': not found')
>> +
>> +
>> +def test(persistent, restart):
>> +    assert persistent or not restart
>> +    log("\nTestcase {}persistent {} restart\n".format(
>> +            '' if persistent else 'non-', 'with' if restart else 'without'))
>> +
>> +    qemu_img_create('-f', iotests.imgfmt, base, str(size))
>> +
>> +    vm = iotests.VM().add_drive(base)
>> +    vm.launch()
>> +
>> +    vm.qmp_log('block-dirty-bitmap-add', node='drive0', name='bitmap0',
>> +               persistent=persistent)
>> +    vm.hmp_qemu_io('drive0', 'write 0 64K')
>> +    print_bitmap('initial bitmap', vm)
>> +
>> +    vm.qmp_log('blockdev-snapshot-sync', device='drive0', snapshot_file=top,
>> +               format=iotests.imgfmt, filters=[iotests.filter_qmp_testfiles])
>> +    vm.hmp_qemu_io('drive0', 'write 64K 512')
>> +    print_bitmap('check that no bitmaps are in snapshot', vm)
>> +
>> +    if restart:
>> +        log("... Restart ...")
>> +        vm.shutdown()
>> +        vm = iotests.VM().add_drive(top)
>> +        vm.launch()
>> +
>> +    vm.qmp_log('block-commit', device='drive0', top=top,
>> +               filters=[iotests.filter_qmp_testfiles])
>> +    ev = vm.event_wait_log(['BLOCK_JOB_READY', 'BLOCK_JOB_COMPLETED'])
>> +    if (ev['event'] == 'BLOCK_JOB_COMPLETED'):
>> +        vm.shutdown()
>> +        log(vm.get_log())
>> +        exit()
>> +
> 
> What's the purpose of this conditional? what causes the difference in
> behavior that we need to handle it?

COMPLETED here means error. without this conditional, we'll wait 60s in next
event_wait, which is very uncomfortable for debugging. Contrariwise, stop in
this case is a lot better... Of course, it may be safely dropped, as test
passes now.

> 
>> +    vm.qmp_log('block-job-complete', device='drive0')
>> +    vm.event_wait_log('BLOCK_JOB_COMPLETED')
>> +    print_bitmap('check bitmap after commit', vm)
>> +
>> +    vm.hmp_qemu_io('drive0', 'write 128K 64K')
>> +    print_bitmap('check updated bitmap', vm)
>> +
>> +    vm.shutdown()
>> +
>> +
>> +test(persistent=False, restart=False)
>> diff --git a/tests/qemu-iotests/255.out b/tests/qemu-iotests/255.out
>> new file mode 100644
>> index 0000000000..5239d27c46
>> --- /dev/null
>> +++ b/tests/qemu-iotests/255.out
>> @@ -0,0 +1,17 @@
>> +
>> +Testcase non-persistent without restart
>> +
>> +{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": "drive0", "persistent": false}}
>> +{"return": {}}
>> +initial bitmap: name=bitmap0 dirty-clusters=1
>> +{"execute": "blockdev-snapshot-sync", "arguments": {"device": "drive0", "format": "qcow2", "snapshot-file": "TEST_DIR/PID-top"}}
>> +{"return": {}}
>> +check that no bitmaps are in snapshot: not found
>> +{"execute": "block-commit", "arguments": {"device": "drive0", "top": "TEST_DIR/PID-top"}}
>> +{"return": {}}
>> +{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
>> +{"execute": "block-job-complete", "arguments": {"device": "drive0"}}
>> +{"return": {}}
>> +{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
>> +check bitmap after commit: name=bitmap0 dirty-clusters=2
>> +check updated bitmap: name=bitmap0 dirty-clusters=3
>> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
>> index 859c4b5e9f..88049ad46c 100644
>> --- a/tests/qemu-iotests/group
>> +++ b/tests/qemu-iotests/group
>> @@ -265,3 +265,4 @@
>>   252 rw auto backing quick
>>   253 rw auto quick
>>   254 rw auto backing quick
>> +255 rw auto quick
>>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 2/9] python/qemu: improve event_wait method of vm

[Vladimir Sementsov-Ogievskiy]

01.06.2019 2:33, John Snow wrote:
> 
> 
> On 5/31/19 12:31 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Support several names to wait for, which useful, when we don't sure
>> which event will we get. For example when mirror fails we get
>> BLOCK_JOB_COMPLETE otherwise we get BLOCK_JOB_READY (and only then,
>> after completing block-job we get BLOCK_JOB_COMPLETE).
>>
>> Also, add filtered version for convenient use.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   python/qemu/__init__.py       | 9 ++++++---
>>   tests/qemu-iotests/iotests.py | 5 +++++
>>   2 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
>> index 81d9657ec0..5e517025b9 100644
>> --- a/python/qemu/__init__.py
>> +++ b/python/qemu/__init__.py
>> @@ -402,7 +402,7 @@ class QEMUMachine(object):
>>           self._qmp.clear_events()
>>           return events
>>   
>> -    def event_wait(self, name, timeout=60.0, match=None):
>> +    def event_wait(self, names, timeout=60.0, match=None):
>>           """
>>           Wait for specified timeout on named event in QMP; optionally filter
>>           results by match.
>> @@ -412,6 +412,9 @@ class QEMUMachine(object):
>>              {"foo": {"bar": 1}} matches {"foo": None}
>>              {"foo": {"bar": 1}} does not matches {"foo": {"baz": None}}
>>           """
>> +        if not isinstance(names, list):
>> +            names = [names]
>> +
>>           def event_match(event, match=None):
>>               if match is None:
>>                   return True
>> @@ -430,14 +433,14 @@ class QEMUMachine(object):
>>   
>>           # Search cached events
>>           for event in self._events:
>> -            if (event['event'] == name) and event_match(event, match):
>> +            if (event['event'] in names) and event_match(event, match):
>>                   self._events.remove(event)
>>                   return event
>>   
>>           # Poll for new events
>>           while True:
>>               event = self._qmp.pull_event(wait=timeout)
>> -            if (event['event'] == name) and event_match(event, match):
>> +            if (event['event'] in names) and event_match(event, match):
>>                   return event
>>               self._events.append(event)
>>   
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 7bde380d96..4218fc908b 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -532,6 +532,11 @@ class VM(qtest.QEMUQtestMachine):
>>           log(result, filters, indent=indent)
>>           return result
>>   
>> +    def event_wait_log(self, names, **kwargs):
>> +        event = self.event_wait(names, **kwargs)
>> +        log(event, filters=[filter_qmp_event])
>> +        return event
>> +
>>       # Returns None on success, and an error string on failure
>>       def run_job(self, job, auto_finalize=True, auto_dismiss=False):
>>           error = None
>>
> 
> There's something like this in the queue; see:
> [Qemu-devel] [PATCH v3 3/5] QEMUMachine: add events_wait method.
> 
> Though yours is a lot shorter, actually, because I made separate
> event_wait and events_wait calls instead of just throwing non-iterables
> in a list.

Hmm, it's already staged by Max. Ok, I'll rebase on it.


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 6/9] block/qcow2-bitmap: do not remove bitmaps on reopen-ro

[Vladimir Sementsov-Ogievskiy]

01.06.2019 3:06, John Snow wrote:
> 
> 
> On 5/31/19 12:31 PM, Vladimir Sementsov-Ogievskiy wrote:
>> qcow2_reopen_bitmaps_ro wants to store bitmaps and then mark them all
>> readonly. But the latter don't work, as
>> qcow2_store_persistent_dirty_bitmaps removes bitmaps after storing.
>> It's OK for inactivation but bad idea for reopen-ro. And this leads to
>> the following bug:
>>
>> Assume we have persistent bitmap 'bitmap0'.
>> Create external snapshot
>>    bitmap0 is stored and therefore removed
>> Commit snapshot
>>    now we have no bitmaps
>> Do some writes from guest (*)
>>    they are not marked in bitmap
>> Shutdown
>> Start
>>    bitmap0 is loaded as valid, but it is actually broken! It misses
>>    writes (*)
>> Incremental backup
>>    it will be inconsistent
>>
>> So, let's stop removing bitmaps on reopen-ro. But don't rejoice:
>> reopening bitmaps to rw is broken too, so the whole scenario will not
>> work after this patch and we can't enable corresponding test cases in
>> 255 iotests still. Reopening bitmaps rw will be fixed in the following
>> patches.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2.h        |  3 ++-
>>   block/qcow2-bitmap.c | 46 +++++++++++++++++++++++++++++---------------
>>   block/qcow2.c        |  2 +-
>>   3 files changed, 34 insertions(+), 17 deletions(-)
>>
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 88a2030f54..4c8435141b 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -734,7 +734,8 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>>                                                   Error **errp);
>>   int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>>   int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>> +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>> +                                          bool release_stored, Error **errp);
>>   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>>   bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>                                         const char *name,
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index fbeee37243..25b1e069a7 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1432,7 +1432,29 @@ fail:
>>       bitmap_list_free(bm_list);
>>   }
>>   
>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>> +/*
>> + * qcow2_store_persistent_dirty_bitmaps
>> + *
>> + * Stores persistent BdrvDirtyBitmap's.
>> + *
> 
> No apostrophe for plural's

I always do so, as it seems strange to me to append 's' to identifiers..
Should I write it BdrvDirtyBitmaps? It sounds as some other identifier...

> 
>> + * @release_stored: if true, release BdrvDirtyBitmap's after storing to the
>> + * image. This is used in two cases, both via qcow2_inactivate:
>> + * 1. bdrv_close: It's correct to remove bitmaps on close.
>> + * 2. migration: If bitmaps are migrated through migration channel via
>> + *    'dirty-bitmaps' migration capability they are not handled by this code.
>> + *    Otherwise, it's OK to drop BdrvDirtyBitmap's and reload them on
>> + *    invalidation.
>> + *
>> + * Anyway, it's correct to remove BdrvDirtyBitmap's on inactivation, as
>> + * inactivation means that we lose control on disk, and therefore on bitmaps,
>> + * we should sync them and do not touch more.
>> + *
>> + * Contrariwise, we don't want to release any bitmaps on just reopen-to-ro,
>> + * when we need to store them, as image is still under our control, and it's
>> + * good to keep all the bitmaps in read-only mode.
>> + */
> 
> I have to admit that 'Contrariwise' is not an everyday term for me. You
> should keep it in here just for fun, in my opinion.

Ahaha, I've just used it in my previous reply.

> 
> Regarding "it's good to keep all the bitmaps in read-only mode":
> More directly, keeping them read-only is correct because this is what
> would happen if we opened the node readonly to begin with, and whether
> we opened directly or reopened to that state shouldn't matter for the
> state we get afterward.

Agree, this is better reasoning.

> 
>> +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>> +                                          bool release_stored, Error **errp)
>>   {
>>       BdrvDirtyBitmap *bitmap;
>>       BDRVQcow2State *s = bs->opaque;
>> @@ -1545,20 +1567,14 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>           g_free(tb);
>>       }
>>   
>> -    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>> -        /* For safety, we remove bitmap after storing.
>> -         * We may be here in two cases:
>> -         * 1. bdrv_close. It's ok to drop bitmap.
>> -         * 2. inactivation. It means migration without 'dirty-bitmaps'
>> -         *    capability, so bitmaps are not marked with
>> -         *    BdrvDirtyBitmap.migration flags. It's not bad to drop them too,
>> -         *    and reload on invalidation.
>> -         */
>> -        if (bm->dirty_bitmap == NULL) {
>> -            continue;
>> -        }
>> +    if (release_stored) {
>> +        QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>> +            if (bm->dirty_bitmap == NULL) {
>> +                continue;
>> +            }
>>   
>> -        bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
>> +            bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
>> +        }
>>       }
>>   
>>   success:
>> @@ -1586,7 +1602,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
>>       BdrvDirtyBitmap *bitmap;
>>       Error *local_err = NULL;
>>   
>> -    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
>> +    qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err);
>>       if (local_err != NULL) {
>>           error_propagate(errp, local_err);
>>           return -EINVAL;
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index f2cb131048..02d8ce7534 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2344,7 +2344,7 @@ static int qcow2_inactivate(BlockDriverState *bs)
>>       int ret, result = 0;
>>       Error *local_err = NULL;
>>   
>> -    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
>> +    qcow2_store_persistent_dirty_bitmaps(bs, true, &local_err);
>>       if (local_err != NULL) {
>>           result = -EINVAL;
>>           error_reportf_err(local_err, "Lost persistent bitmaps during "
>>
> 
> code:
> Reviewed-by: John Snow <jsnow@redhat.com>
> 
> (You can adjust the docs as you need to on further review, if any, and
> keep that RB. --js)
> 

OK, thank you!

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 6/9] block/qcow2-bitmap: do not remove bitmaps on reopen-ro

[John Snow]

On 6/3/19 6:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> 01.06.2019 3:06, John Snow wrote:
>>
>>
>> On 5/31/19 12:31 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> qcow2_reopen_bitmaps_ro wants to store bitmaps and then mark them all
>>> readonly. But the latter don't work, as
>>> qcow2_store_persistent_dirty_bitmaps removes bitmaps after storing.
>>> It's OK for inactivation but bad idea for reopen-ro. And this leads to
>>> the following bug:
>>>
>>> Assume we have persistent bitmap 'bitmap0'.
>>> Create external snapshot
>>>    bitmap0 is stored and therefore removed
>>> Commit snapshot
>>>    now we have no bitmaps
>>> Do some writes from guest (*)
>>>    they are not marked in bitmap
>>> Shutdown
>>> Start
>>>    bitmap0 is loaded as valid, but it is actually broken! It misses
>>>    writes (*)
>>> Incremental backup
>>>    it will be inconsistent
>>>
>>> So, let's stop removing bitmaps on reopen-ro. But don't rejoice:
>>> reopening bitmaps to rw is broken too, so the whole scenario will not
>>> work after this patch and we can't enable corresponding test cases in
>>> 255 iotests still. Reopening bitmaps rw will be fixed in the following
>>> patches.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/qcow2.h        |  3 ++-
>>>   block/qcow2-bitmap.c | 46 +++++++++++++++++++++++++++++---------------
>>>   block/qcow2.c        |  2 +-
>>>   3 files changed, 34 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>> index 88a2030f54..4c8435141b 100644
>>> --- a/block/qcow2.h
>>> +++ b/block/qcow2.h
>>> @@ -734,7 +734,8 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>>>                                                   Error **errp);
>>>   int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>>>   int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
>>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>>> +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>> +                                          bool release_stored, Error **errp);
>>>   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>>>   bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>>                                         const char *name,
>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>> index fbeee37243..25b1e069a7 100644
>>> --- a/block/qcow2-bitmap.c
>>> +++ b/block/qcow2-bitmap.c
>>> @@ -1432,7 +1432,29 @@ fail:
>>>       bitmap_list_free(bm_list);
>>>   }
>>>   
>>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>> +/*
>>> + * qcow2_store_persistent_dirty_bitmaps
>>> + *
>>> + * Stores persistent BdrvDirtyBitmap's.
>>> + *
>>
>> No apostrophe for plural's
> 
> I always do so, as it seems strange to me to append 's' to identifiers..
> Should I write it BdrvDirtyBitmaps? It sounds as some other identifier...
> 

This is a recurring problem with English. The term "CD's" is in common
usage for this reason, even though it's grammatically incorrect.
Honestly, I don't have an answer for you, but you could try to avoid it:

"Stores persistent BdrvDirtyBitmap objects"

It's clunkier, but it avoids adding a plural to an identifier. In marked
up text, it's not uncommon to see `BdrvDirtyBitmap`s, but that would
look silly here.

>>
>>> + * @release_stored: if true, release BdrvDirtyBitmap's after storing to the
>>> + * image. This is used in two cases, both via qcow2_inactivate:
>>> + * 1. bdrv_close: It's correct to remove bitmaps on close.
>>> + * 2. migration: If bitmaps are migrated through migration channel via
>>> + *    'dirty-bitmaps' migration capability they are not handled by this code.
>>> + *    Otherwise, it's OK to drop BdrvDirtyBitmap's and reload them on
>>> + *    invalidation.
>>> + *
>>> + * Anyway, it's correct to remove BdrvDirtyBitmap's on inactivation, as
>>> + * inactivation means that we lose control on disk, and therefore on bitmaps,
>>> + * we should sync them and do not touch more.
>>> + *
>>> + * Contrariwise, we don't want to release any bitmaps on just reopen-to-ro,
>>> + * when we need to store them, as image is still under our control, and it's
>>> + * good to keep all the bitmaps in read-only mode.
>>> + */
>>
>> I have to admit that 'Contrariwise' is not an everyday term for me. You
>> should keep it in here just for fun, in my opinion.
> 
> Ahaha, I've just used it in my previous reply.
> 
>>
>> Regarding "it's good to keep all the bitmaps in read-only mode":
>> More directly, keeping them read-only is correct because this is what
>> would happen if we opened the node readonly to begin with, and whether
>> we opened directly or reopened to that state shouldn't matter for the
>> state we get afterward.
> 
> Agree, this is better reasoning.
> 
>>
>>> +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>> +                                          bool release_stored, Error **errp)
>>>   {
>>>       BdrvDirtyBitmap *bitmap;
>>>       BDRVQcow2State *s = bs->opaque;
>>> @@ -1545,20 +1567,14 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>>           g_free(tb);
>>>       }
>>>   
>>> -    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>> -        /* For safety, we remove bitmap after storing.
>>> -         * We may be here in two cases:
>>> -         * 1. bdrv_close. It's ok to drop bitmap.
>>> -         * 2. inactivation. It means migration without 'dirty-bitmaps'
>>> -         *    capability, so bitmaps are not marked with
>>> -         *    BdrvDirtyBitmap.migration flags. It's not bad to drop them too,
>>> -         *    and reload on invalidation.
>>> -         */
>>> -        if (bm->dirty_bitmap == NULL) {
>>> -            continue;
>>> -        }
>>> +    if (release_stored) {
>>> +        QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>> +            if (bm->dirty_bitmap == NULL) {
>>> +                continue;
>>> +            }
>>>   
>>> -        bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
>>> +            bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
>>> +        }
>>>       }
>>>   
>>>   success:
>>> @@ -1586,7 +1602,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
>>>       BdrvDirtyBitmap *bitmap;
>>>       Error *local_err = NULL;
>>>   
>>> -    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
>>> +    qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err);
>>>       if (local_err != NULL) {
>>>           error_propagate(errp, local_err);
>>>           return -EINVAL;
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index f2cb131048..02d8ce7534 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -2344,7 +2344,7 @@ static int qcow2_inactivate(BlockDriverState *bs)
>>>       int ret, result = 0;
>>>       Error *local_err = NULL;
>>>   
>>> -    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
>>> +    qcow2_store_persistent_dirty_bitmaps(bs, true, &local_err);
>>>       if (local_err != NULL) {
>>>           result = -EINVAL;
>>>           error_reportf_err(local_err, "Lost persistent bitmaps during "
>>>
>>
>> code:
>> Reviewed-by: John Snow <jsnow@redhat.com>
>>
>> (You can adjust the docs as you need to on further review, if any, and
>> keep that RB. --js)
>>
> 
> OK, thank you!
> 

I'll get back to the rest of this soon, it looks like you haven't gotten
review on the core block layer pieces, or maybe I've missed it if you have?

Re: [Qemu-devel] [PATCH v2 6/9] block/qcow2-bitmap: do not remove bitmaps on reopen-ro

[Vladimir Sementsov-Ogievskiy]

18.06.2019 17:30, John Snow wrote:
> 
> 
> On 6/3/19 6:14 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 01.06.2019 3:06, John Snow wrote:
>>>
>>>
>>> On 5/31/19 12:31 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>> qcow2_reopen_bitmaps_ro wants to store bitmaps and then mark them all
>>>> readonly. But the latter don't work, as
>>>> qcow2_store_persistent_dirty_bitmaps removes bitmaps after storing.
>>>> It's OK for inactivation but bad idea for reopen-ro. And this leads to
>>>> the following bug:
>>>>
>>>> Assume we have persistent bitmap 'bitmap0'.
>>>> Create external snapshot
>>>>     bitmap0 is stored and therefore removed
>>>> Commit snapshot
>>>>     now we have no bitmaps
>>>> Do some writes from guest (*)
>>>>     they are not marked in bitmap
>>>> Shutdown
>>>> Start
>>>>     bitmap0 is loaded as valid, but it is actually broken! It misses
>>>>     writes (*)
>>>> Incremental backup
>>>>     it will be inconsistent
>>>>
>>>> So, let's stop removing bitmaps on reopen-ro. But don't rejoice:
>>>> reopening bitmaps to rw is broken too, so the whole scenario will not
>>>> work after this patch and we can't enable corresponding test cases in
>>>> 255 iotests still. Reopening bitmaps rw will be fixed in the following
>>>> patches.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    block/qcow2.h        |  3 ++-
>>>>    block/qcow2-bitmap.c | 46 +++++++++++++++++++++++++++++---------------
>>>>    block/qcow2.c        |  2 +-
>>>>    3 files changed, 34 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/block/qcow2.h b/block/qcow2.h
>>>> index 88a2030f54..4c8435141b 100644
>>>> --- a/block/qcow2.h
>>>> +++ b/block/qcow2.h
>>>> @@ -734,7 +734,8 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>>>>                                                    Error **errp);
>>>>    int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>>>>    int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
>>>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
>>>> +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>>> +                                          bool release_stored, Error **errp);
>>>>    int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>>>>    bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>>>                                          const char *name,
>>>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>>>> index fbeee37243..25b1e069a7 100644
>>>> --- a/block/qcow2-bitmap.c
>>>> +++ b/block/qcow2-bitmap.c
>>>> @@ -1432,7 +1432,29 @@ fail:
>>>>        bitmap_list_free(bm_list);
>>>>    }
>>>>    
>>>> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>>> +/*
>>>> + * qcow2_store_persistent_dirty_bitmaps
>>>> + *
>>>> + * Stores persistent BdrvDirtyBitmap's.
>>>> + *
>>>
>>> No apostrophe for plural's
>>
>> I always do so, as it seems strange to me to append 's' to identifiers..
>> Should I write it BdrvDirtyBitmaps? It sounds as some other identifier...
>>
> 
> This is a recurring problem with English. The term "CD's" is in common
> usage for this reason, even though it's grammatically incorrect.
> Honestly, I don't have an answer for you, but you could try to avoid it:
> 
> "Stores persistent BdrvDirtyBitmap objects"
> 
> It's clunkier, but it avoids adding a plural to an identifier. In marked
> up text, it's not uncommon to see `BdrvDirtyBitmap`s, but that would
> look silly here.
> 
>>>
>>>> + * @release_stored: if true, release BdrvDirtyBitmap's after storing to the
>>>> + * image. This is used in two cases, both via qcow2_inactivate:
>>>> + * 1. bdrv_close: It's correct to remove bitmaps on close.
>>>> + * 2. migration: If bitmaps are migrated through migration channel via
>>>> + *    'dirty-bitmaps' migration capability they are not handled by this code.
>>>> + *    Otherwise, it's OK to drop BdrvDirtyBitmap's and reload them on
>>>> + *    invalidation.
>>>> + *
>>>> + * Anyway, it's correct to remove BdrvDirtyBitmap's on inactivation, as
>>>> + * inactivation means that we lose control on disk, and therefore on bitmaps,
>>>> + * we should sync them and do not touch more.
>>>> + *
>>>> + * Contrariwise, we don't want to release any bitmaps on just reopen-to-ro,
>>>> + * when we need to store them, as image is still under our control, and it's
>>>> + * good to keep all the bitmaps in read-only mode.
>>>> + */
>>>
>>> I have to admit that 'Contrariwise' is not an everyday term for me. You
>>> should keep it in here just for fun, in my opinion.
>>
>> Ahaha, I've just used it in my previous reply.
>>
>>>
>>> Regarding "it's good to keep all the bitmaps in read-only mode":
>>> More directly, keeping them read-only is correct because this is what
>>> would happen if we opened the node readonly to begin with, and whether
>>> we opened directly or reopened to that state shouldn't matter for the
>>> state we get afterward.
>>
>> Agree, this is better reasoning.
>>
>>>
>>>> +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>>> +                                          bool release_stored, Error **errp)
>>>>    {
>>>>        BdrvDirtyBitmap *bitmap;
>>>>        BDRVQcow2State *s = bs->opaque;
>>>> @@ -1545,20 +1567,14 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>>>>            g_free(tb);
>>>>        }
>>>>    
>>>> -    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>>> -        /* For safety, we remove bitmap after storing.
>>>> -         * We may be here in two cases:
>>>> -         * 1. bdrv_close. It's ok to drop bitmap.
>>>> -         * 2. inactivation. It means migration without 'dirty-bitmaps'
>>>> -         *    capability, so bitmaps are not marked with
>>>> -         *    BdrvDirtyBitmap.migration flags. It's not bad to drop them too,
>>>> -         *    and reload on invalidation.
>>>> -         */
>>>> -        if (bm->dirty_bitmap == NULL) {
>>>> -            continue;
>>>> -        }
>>>> +    if (release_stored) {
>>>> +        QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>>> +            if (bm->dirty_bitmap == NULL) {
>>>> +                continue;
>>>> +            }
>>>>    
>>>> -        bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
>>>> +            bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
>>>> +        }
>>>>        }
>>>>    
>>>>    success:
>>>> @@ -1586,7 +1602,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
>>>>        BdrvDirtyBitmap *bitmap;
>>>>        Error *local_err = NULL;
>>>>    
>>>> -    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
>>>> +    qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err);
>>>>        if (local_err != NULL) {
>>>>            error_propagate(errp, local_err);
>>>>            return -EINVAL;
>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>> index f2cb131048..02d8ce7534 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>>> @@ -2344,7 +2344,7 @@ static int qcow2_inactivate(BlockDriverState *bs)
>>>>        int ret, result = 0;
>>>>        Error *local_err = NULL;
>>>>    
>>>> -    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
>>>> +    qcow2_store_persistent_dirty_bitmaps(bs, true, &local_err);
>>>>        if (local_err != NULL) {
>>>>            result = -EINVAL;
>>>>            error_reportf_err(local_err, "Lost persistent bitmaps during "
>>>>
>>>
>>> code:
>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>>
>>> (You can adjust the docs as you need to on further review, if any, and
>>> keep that RB. --js)
>>>
>>
>> OK, thank you!
>>
> 
> I'll get back to the rest of this soon, it looks like you haven't gotten
> review on the core block layer pieces, or maybe I've missed it if you have?
> 

Hmm, no, I haven't.. Seems I forget about these series, it should have been pinged
several days ago.

-- 
Best regards,
Vladimir