[Qemu-devel] [PATCH v4 00/10] qcow2-bitmaps: rewrite reopening logic

[Qemu-devel] [PATCH v4 00/10] qcow2-bitmaps: rewrite reopening logic

[Vladimir Sementsov-Ogievskiy]

Hi all!

Bitmaps reopening is buggy, reopening-rw just not working at all and
reopening-ro may lead to producing broken incremental
backup if we do temporary snapshot in a meantime.

v4: Drop complicated solution around reopening logic [Kevin], fix
    the existing bug in a simplest way

Structure:

02: fix reopen to RW
03: test reopen to RW

07: fix reopen to RO
08: test reopen to RO

Others are less significant improvements and refactoring

Changelog:

01-03: new patches, to fix reopening bitmaps to RW and personal test for
       this bug
08: merged test from v3, it covers both bugs (reopen to RW and reopen to RO)
10: instead of moving bitmap-reopening to prepare(as in 09 in v3) we now keep it
    in commit, but in right place
others: unchanged

v3:
02: John's events_wait already merged in, so my 02 from v2 is not needed.
    Instead, add two simple logging wrappers here
03: rebase on 02 - use new wrappers, move to 260
05: add John's r-b
06: improve function docs [John], add John's r-b

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 (10):
  block: switch reopen queue from QSIMPLEQ to QTAILQ
  block: reverse order for reopen commits
  iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW
  iotests.py: add event_wait_log and events_wait_log helpers
  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
  iotests: add test 260 to check bitmap life after snapshot + commit
  block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw
  qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit

 block/qcow2.h                 |   5 +-
 include/block/block.h         |   2 +-
 include/block/block_int.h     |   6 --
 include/block/dirty-bitmap.h  |   1 -
 block.c                       |  51 +++++-------
 block/dirty-bitmap.c          |  12 ---
 block/qcow2-bitmap.c          | 143 ++++++++++++++++++++--------------
 block/qcow2.c                 |  17 +++-
 tests/qemu-iotests/165        |  46 ++++++++++-
 tests/qemu-iotests/165.out    |   4 +-
 tests/qemu-iotests/260        |  87 +++++++++++++++++++++
 tests/qemu-iotests/260.out    |  52 +++++++++++++
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |  10 +++
 14 files changed, 318 insertions(+), 119 deletions(-)
 create mode 100755 tests/qemu-iotests/260
 create mode 100644 tests/qemu-iotests/260.out

-- 
2.18.0

[Qemu-devel] [PATCH v4 01/10] block: switch reopen queue from QSIMPLEQ to QTAILQ

[Vladimir Sementsov-Ogievskiy]

We'll need reverse-foreach in the following commit, QTAILQ support it,
so move to QTAILQ.

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

diff --git a/include/block/block.h b/include/block/block.h
index 50a07c1c33..3d84b2a2c9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -192,7 +192,7 @@ typedef struct HDGeometry {
 #define BDRV_BLOCK_RECURSE      0x40
 #define BDRV_BLOCK_OFFSET_MASK  BDRV_SECTOR_MASK
 
-typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
+typedef QTAILQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
 
 typedef struct BDRVReopenState {
     BlockDriverState *bs;
diff --git a/block.c b/block.c
index cbd8da5f3b..696162cd7a 100644
--- a/block.c
+++ b/block.c
@@ -1718,7 +1718,7 @@ typedef struct BlockReopenQueueEntry {
      bool prepared;
      bool perms_checked;
      BDRVReopenState state;
-     QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
+     QTAILQ_ENTRY(BlockReopenQueueEntry) entry;
 } BlockReopenQueueEntry;
 
 /*
@@ -1731,7 +1731,7 @@ static int bdrv_reopen_get_flags(BlockReopenQueue *q, BlockDriverState *bs)
     BlockReopenQueueEntry *entry;
 
     if (q != NULL) {
-        QSIMPLEQ_FOREACH(entry, q, entry) {
+        QTAILQ_FOREACH(entry, q, entry) {
             if (entry->state.bs == bs) {
                 return entry->state.flags;
             }
@@ -3280,7 +3280,7 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 
     if (bs_queue == NULL) {
         bs_queue = g_new0(BlockReopenQueue, 1);
-        QSIMPLEQ_INIT(bs_queue);
+        QTAILQ_INIT(bs_queue);
     }
 
     if (!options) {
@@ -3288,7 +3288,7 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
     }
 
     /* Check if this BlockDriverState is already in the queue */
-    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+    QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
         if (bs == bs_entry->state.bs) {
             break;
         }
@@ -3344,7 +3344,7 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 
     if (!bs_entry) {
         bs_entry = g_new0(BlockReopenQueueEntry, 1);
-        QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry);
+        QTAILQ_INSERT_TAIL(bs_queue, bs_entry, entry);
     } else {
         qobject_unref(bs_entry->state.options);
         qobject_unref(bs_entry->state.explicit_options);
@@ -3445,7 +3445,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
 
     assert(bs_queue != NULL);
 
-    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+    QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
         assert(bs_entry->state.bs->quiesce_counter > 0);
         if (bdrv_reopen_prepare(&bs_entry->state, bs_queue, errp)) {
             goto cleanup;
@@ -3453,7 +3453,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
         bs_entry->prepared = true;
     }
 
-    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+    QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
         BDRVReopenState *state = &bs_entry->state;
         ret = bdrv_check_perm(state->bs, bs_queue, state->perm,
                               state->shared_perm, NULL, NULL, errp);
@@ -3479,13 +3479,13 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
     /* If we reach this point, we have success and just need to apply the
      * changes
      */
-    QSIMPLEQ_FOREACH(bs_entry, bs_queue, entry) {
+    QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
         bdrv_reopen_commit(&bs_entry->state);
     }
 
     ret = 0;
 cleanup_perm:
-    QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
+    QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
         BDRVReopenState *state = &bs_entry->state;
 
         if (!bs_entry->perms_checked) {
@@ -3502,7 +3502,7 @@ cleanup_perm:
         }
     }
 cleanup:
-    QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
+    QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
         if (ret) {
             if (bs_entry->prepared) {
                 bdrv_reopen_abort(&bs_entry->state);
@@ -3542,7 +3542,7 @@ static BlockReopenQueueEntry *find_parent_in_reopen_queue(BlockReopenQueue *q,
 {
     BlockReopenQueueEntry *entry;
 
-    QSIMPLEQ_FOREACH(entry, q, entry) {
+    QTAILQ_FOREACH(entry, q, entry) {
         BlockDriverState *bs = entry->state.bs;
         BdrvChild *child;
 
-- 
2.18.0

[Qemu-devel] [PATCH v4 02/10] block: reverse order for reopen commits

[Vladimir Sementsov-Ogievskiy]

It's needed to fix reopening qcow2 with bitmaps to RW. Currently it
can't work, as qcow2 needs write access to file child, to mark bitmaps
in-image with IN_USE flag. But usually children goes after parents in
reopen queue and file child is still RO on qcow2 reopen commit. Reverse
reopen order to fix it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 696162cd7a..d59f9f97cb 100644
--- a/block.c
+++ b/block.c
@@ -3476,10 +3476,16 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
         bs_entry->perms_checked = true;
     }
 
-    /* If we reach this point, we have success and just need to apply the
-     * changes
+    /*
+     * If we reach this point, we have success and just need to apply the
+     * changes.
+     *
+     * Reverse order is used to comfort qcow2 driver: on commit it need to write
+     * IN_USE flag to the image, to mark bitmaps in the image as invalid. But
+     * children are usually goes after parents in reopen-queue, so go from last
+     * to first element.
      */
-    QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
+    QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) {
         bdrv_reopen_commit(&bs_entry->state);
     }
 
-- 
2.18.0

[Qemu-devel] [PATCH v4 03/10] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW

[Vladimir Sementsov-Ogievskiy]

Reopening bitmaps to RW was broken prior to previous commit. Check that
it works now.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/165     | 46 ++++++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/165.out |  4 ++--
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
index 88f62d3c6d..dd93b5a2d0 100755
--- a/tests/qemu-iotests/165
+++ b/tests/qemu-iotests/165
@@ -43,10 +43,10 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
         os.remove(disk)
 
     def mkVm(self):
-        return iotests.VM().add_drive(disk)
+        return iotests.VM().add_drive(disk, opts='node-name=node0')
 
     def mkVmRo(self):
-        return iotests.VM().add_drive(disk, opts='readonly=on')
+        return iotests.VM().add_drive(disk, opts='readonly=on,node-name=node0')
 
     def getSha256(self):
         result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
@@ -102,5 +102,47 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
 
         self.vm.shutdown()
 
+    def test_reopen_rw(self):
+        self.vm = self.mkVm()
+        self.vm.launch()
+        self.qmpAddBitmap()
+
+        # Calculate sha256 corresponding to regions1
+        self.writeRegions(regions1)
+        sha256 = self.getSha256()
+        result = self.vm.qmp('block-dirty-bitmap-clear', node='drive0',
+                             name='bitmap0')
+        self.assert_qmp(result, 'return', {})
+
+        self.vm.shutdown()
+
+        self.vm = self.mkVmRo()
+        self.vm.launch()
+
+        # We've loaded empty bitmap
+        assert sha256 != self.getSha256()
+
+        # Check that we are in RO mode
+        self.writeRegions(regions1)
+        assert sha256 != self.getSha256()
+
+        result = self.vm.qmp('x-blockdev-reopen', **{
+            'node-name': 'node0',
+            'driver': iotests.imgfmt,
+            'file': {
+                'driver': 'file',
+                'filename': disk
+            },
+            'read-only': False
+        })
+        self.assert_qmp(result, 'return', {})
+
+        # Check that bitmap is reopened to RW and we can write to it.
+        self.writeRegions(regions1)
+        assert sha256 == self.getSha256()
+
+        self.vm.shutdown()
+
+
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/165.out b/tests/qemu-iotests/165.out
index ae1213e6f8..fbc63e62f8 100644
--- a/tests/qemu-iotests/165.out
+++ b/tests/qemu-iotests/165.out
@@ -1,5 +1,5 @@
-.
+..
 ----------------------------------------------------------------------
-Ran 1 tests
+Ran 2 tests
 
 OK
-- 
2.18.0

[Qemu-devel] [PATCH v4 04/10] iotests.py: add event_wait_log and events_wait_log helpers

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/iotests.py | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index ce74177ab1..4ad265f140 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -540,6 +540,16 @@ class VM(qtest.QEMUQtestMachine):
         log(result, filters, indent=indent)
         return result
 
+    def event_wait_log(self, name, **kwargs):
+        event = self.event_wait(name, **kwargs)
+        log(event, filters=[filter_qmp_event])
+        return event
+
+    def events_wait_log(self, events, **kwargs):
+        event = self.events_wait(events, **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,
                 pre_finalize=None, use_log=True, wait=60.0):
-- 
2.18.0

[Qemu-devel] [PATCH v4 05/10] 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 62682eb865..acca86d0fc 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -104,7 +104,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 95a9c2a5d8..2e0cecf5ff 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -774,18 +774,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 v4 06/10] 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>
Reviewed-by: John Snow <jsnow@redhat.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 fc1b0d3c1e..a5b24f4eec 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -736,8 +736,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 v4 07/10] 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
260 iotests still. Reopening bitmaps rw will be fixed in the following
patches.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 block/qcow2.h        |  3 ++-
 block/qcow2-bitmap.c | 49 ++++++++++++++++++++++++++++++--------------
 block/qcow2.c        |  2 +-
 3 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index a5b24f4eec..a67e6a7d7c 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -738,7 +738,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..a636dc50ca 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1432,7 +1432,32 @@ fail:
     bitmap_list_free(bm_list);
 }
 
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+/*
+ * qcow2_store_persistent_dirty_bitmaps
+ *
+ * Stores persistent BdrvDirtyBitmap objects.
+ *
+ * @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. Moreover, 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 +1570,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 +1605,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 039bdc2f7e..5c1187e2f9 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2356,7 +2356,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 v4 08/10] iotests: add test 260 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/260     | 87 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/260.out | 52 +++++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 140 insertions(+)
 create mode 100755 tests/qemu-iotests/260
 create mode 100644 tests/qemu-iotests/260.out

diff --git a/tests/qemu-iotests/260 b/tests/qemu-iotests/260
new file mode 100755
index 0000000000..d8fcf4567a
--- /dev/null
+++ b/tests/qemu-iotests/260
@@ -0,0 +1,87 @@
+#!/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.events_wait_log((('BLOCK_JOB_READY', None),
+                             ('BLOCK_JOB_COMPLETED', None)))
+    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)
+test(persistent=True, restart=False)
+test(persistent=True, restart=True)
diff --git a/tests/qemu-iotests/260.out b/tests/qemu-iotests/260.out
new file mode 100644
index 0000000000..2f0d98d036
--- /dev/null
+++ b/tests/qemu-iotests/260.out
@@ -0,0 +1,52 @@
+
+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
+
+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
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index f13e5f2e23..06f1ea904c 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -271,3 +271,4 @@
 254 rw backing quick
 255 rw quick
 256 rw quick
+260 rw auto quick
-- 
2.18.0

[Qemu-devel] [PATCH v4 09/10] 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 following commit),
  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 a636dc50ca..e276a95154 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 v4 10/10] qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit

[Vladimir Sementsov-Ogievskiy]

The only reason I can imagine for this strange code at the very-end of
bdrv_reopen_commit is the fact that bs->read_only updated after
calling drv->bdrv_reopen_commit in bdrv_reopen_commit. And in the same
time, prior to previous commit, qcow2_reopen_bitmaps_rw did a wrong
check for being writable, when actually it only need writable file
child not self.

So, as it's fixed, let's move things to correct place.

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

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3aa1e832a8..18a1e81194 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -531,12 +531,6 @@ struct BlockDriver {
                              uint64_t parent_perm, uint64_t parent_shared,
                              uint64_t *nperm, uint64_t *nshared);
 
-    /**
-     * 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 d59f9f97cb..395bc88045 100644
--- a/block.c
+++ b/block.c
@@ -3925,16 +3925,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);
@@ -3978,21 +3974,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 5c1187e2f9..9e6210c282 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1828,6 +1828,20 @@ fail:
 static void qcow2_reopen_commit(BDRVReopenState *state)
 {
     qcow2_update_options_commit(state->bs, state->opaque);
+    if (state->flags & BDRV_O_RDWR) {
+        Error *local_err = NULL;
+
+        if (qcow2_reopen_bitmaps_rw(state->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 or retry reopen.
+             */
+            error_reportf_err(local_err,
+                              "%s: Failed to make dirty bitmaps writable: ",
+                              bdrv_get_node_name(state->bs));
+        }
+    }
     g_free(state->opaque);
 }
 
@@ -5229,7 +5243,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,
 };
-- 
2.18.0

Re: [Qemu-devel] [PATCH v4 00/10] qcow2-bitmaps: rewrite reopening logic

[Vladimir Sementsov-Ogievskiy]

ping

07.08.2019 17:12, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Bitmaps reopening is buggy, reopening-rw just not working at all and
> reopening-ro may lead to producing broken incremental
> backup if we do temporary snapshot in a meantime.
> 
> v4: Drop complicated solution around reopening logic [Kevin], fix
>      the existing bug in a simplest way
> 
> Structure:
> 
> 02: fix reopen to RW
> 03: test reopen to RW
> 
> 07: fix reopen to RO
> 08: test reopen to RO
> 
> Others are less significant improvements and refactoring
> 
> Changelog:
> 
> 01-03: new patches, to fix reopening bitmaps to RW and personal test for
>         this bug
> 08: merged test from v3, it covers both bugs (reopen to RW and reopen to RO)
> 10: instead of moving bitmap-reopening to prepare(as in 09 in v3) we now keep it
>      in commit, but in right place
> others: unchanged
> 
> v3:
> 02: John's events_wait already merged in, so my 02 from v2 is not needed.
>      Instead, add two simple logging wrappers here
> 03: rebase on 02 - use new wrappers, move to 260
> 05: add John's r-b
> 06: improve function docs [John], add John's r-b
> 
> 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 (10):
>    block: switch reopen queue from QSIMPLEQ to QTAILQ
>    block: reverse order for reopen commits
>    iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW
>    iotests.py: add event_wait_log and events_wait_log helpers
>    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
>    iotests: add test 260 to check bitmap life after snapshot + commit
>    block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw
>    qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit
> 
>   block/qcow2.h                 |   5 +-
>   include/block/block.h         |   2 +-
>   include/block/block_int.h     |   6 --
>   include/block/dirty-bitmap.h  |   1 -
>   block.c                       |  51 +++++-------
>   block/dirty-bitmap.c          |  12 ---
>   block/qcow2-bitmap.c          | 143 ++++++++++++++++++++--------------
>   block/qcow2.c                 |  17 +++-
>   tests/qemu-iotests/165        |  46 ++++++++++-
>   tests/qemu-iotests/165.out    |   4 +-
>   tests/qemu-iotests/260        |  87 +++++++++++++++++++++
>   tests/qemu-iotests/260.out    |  52 +++++++++++++
>   tests/qemu-iotests/group      |   1 +
>   tests/qemu-iotests/iotests.py |  10 +++
>   14 files changed, 318 insertions(+), 119 deletions(-)
>   create mode 100755 tests/qemu-iotests/260
>   create mode 100644 tests/qemu-iotests/260.out
> 


-- 
Best regards,
Vladimir

bugfix ping Re: [PATCH v4 00/10] qcow2-bitmaps: rewrite reopening logic

[Vladimir Sementsov-Ogievskiy]

ping
05.09.2019 12:54, Vladimir Sementsov-Ogievskiy wrote:
> ping
> 
> 07.08.2019 17:12, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Bitmaps reopening is buggy, reopening-rw just not working at all and
>> reopening-ro may lead to producing broken incremental
>> backup if we do temporary snapshot in a meantime.
>>
>> v4: Drop complicated solution around reopening logic [Kevin], fix
>>      the existing bug in a simplest way
>>
>> Structure:
>>
>> 02: fix reopen to RW
>> 03: test reopen to RW
>>
>> 07: fix reopen to RO
>> 08: test reopen to RO
>>
>> Others are less significant improvements and refactoring
>>
>> Changelog:
>>
>> 01-03: new patches, to fix reopening bitmaps to RW and personal test for
>>         this bug
>> 08: merged test from v3, it covers both bugs (reopen to RW and reopen to RO)
>> 10: instead of moving bitmap-reopening to prepare(as in 09 in v3) we now keep it
>>      in commit, but in right place
>> others: unchanged
>>
>> v3:
>> 02: John's events_wait already merged in, so my 02 from v2 is not needed.
>>      Instead, add two simple logging wrappers here
>> 03: rebase on 02 - use new wrappers, move to 260
>> 05: add John's r-b
>> 06: improve function docs [John], add John's r-b
>>
>> 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 (10):
>>    block: switch reopen queue from QSIMPLEQ to QTAILQ
>>    block: reverse order for reopen commits
>>    iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW
>>    iotests.py: add event_wait_log and events_wait_log helpers
>>    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
>>    iotests: add test 260 to check bitmap life after snapshot + commit
>>    block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw
>>    qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit
>>
>>   block/qcow2.h                 |   5 +-
>>   include/block/block.h         |   2 +-
>>   include/block/block_int.h     |   6 --
>>   include/block/dirty-bitmap.h  |   1 -
>>   block.c                       |  51 +++++-------
>>   block/dirty-bitmap.c          |  12 ---
>>   block/qcow2-bitmap.c          | 143 ++++++++++++++++++++--------------
>>   block/qcow2.c                 |  17 +++-
>>   tests/qemu-iotests/165        |  46 ++++++++++-
>>   tests/qemu-iotests/165.out    |   4 +-
>>   tests/qemu-iotests/260        |  87 +++++++++++++++++++++
>>   tests/qemu-iotests/260.out    |  52 +++++++++++++
>>   tests/qemu-iotests/group      |   1 +
>>   tests/qemu-iotests/iotests.py |  10 +++
>>   14 files changed, 318 insertions(+), 119 deletions(-)
>>   create mode 100755 tests/qemu-iotests/260
>>   create mode 100644 tests/qemu-iotests/260.out
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [PATCH v4 01/10] block: switch reopen queue from QSIMPLEQ to QTAILQ

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 611 bytes --]

On 07.08.19 16:12, Vladimir Sementsov-Ogievskiy wrote:
> We'll need reverse-foreach in the following commit, QTAILQ support it,
> so move to QTAILQ.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block.h |  2 +-
>  block.c               | 22 +++++++++++-----------
>  2 files changed, 12 insertions(+), 12 deletions(-)

There’s a comment above bdrv_reopen_queue_child() that speaks of
QSIMPLE_INIT.  I suppose that should be QTAILQ_INIT now (and was just a
bit wrong before).

With that fixed:

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 02/10] block: reverse order for reopen commits

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2273 bytes --]

On 07.08.19 16:12, Vladimir Sementsov-Ogievskiy wrote:
> It's needed to fix reopening qcow2 with bitmaps to RW. Currently it
> can't work, as qcow2 needs write access to file child, to mark bitmaps
> in-image with IN_USE flag. But usually children goes after parents in
> reopen queue and file child is still RO on qcow2 reopen commit. Reverse
> reopen order to fix it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 696162cd7a..d59f9f97cb 100644
> --- a/block.c
> +++ b/block.c
> @@ -3476,10 +3476,16 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
>          bs_entry->perms_checked = true;
>      }
>  
> -    /* If we reach this point, we have success and just need to apply the
> -     * changes
> +    /*
> +     * If we reach this point, we have success and just need to apply the
> +     * changes.
> +     *
> +     * Reverse order is used to comfort qcow2 driver: on commit it need to write
> +     * IN_USE flag to the image, to mark bitmaps in the image as invalid. But
> +     * children are usually goes after parents in reopen-queue, so go from last
> +     * to first element.
>       */
> -    QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
> +    QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) {
>          bdrv_reopen_commit(&bs_entry->state);
>      }

I suppose this works, but only because everything but the IN_USE thing
actually behaves correctly.  In theory, all the work is done by the time
.prepare is through, so we can call commit in any order anyway.

So I’m still of the opinion that writing IN_USE in commit is just plain
wrong.

It feels like you’re trying to work around wrongs in reopen by piling
more wrongs on top.  I don’t like reopen already, and I don’t think this
makes it any better.

I don’t like how the comment implies that everything is just as it
should be, but that isn’t the real problem here, so whatever.


Well, at least the change is simple, and it doesn’t make things worse
than they actually are already (that is, wrong), so

Acked-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v4 03/10] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW

[John Snow]

On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> Reopening bitmaps to RW was broken prior to previous commit. Check that
> it works now.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/165     | 46 ++++++++++++++++++++++++++++++++++++--
>  tests/qemu-iotests/165.out |  4 ++--
>  2 files changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
> index 88f62d3c6d..dd93b5a2d0 100755
> --- a/tests/qemu-iotests/165
> +++ b/tests/qemu-iotests/165
> @@ -43,10 +43,10 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
>          os.remove(disk)
>  
>      def mkVm(self):
> -        return iotests.VM().add_drive(disk)
> +        return iotests.VM().add_drive(disk, opts='node-name=node0')
>  
>      def mkVmRo(self):
> -        return iotests.VM().add_drive(disk, opts='readonly=on')
> +        return iotests.VM().add_drive(disk, opts='readonly=on,node-name=node0')
>  
>      def getSha256(self):
>          result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
> @@ -102,5 +102,47 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
>  
>          self.vm.shutdown()
>  
> +    def test_reopen_rw(self):
> +        self.vm = self.mkVm()
> +        self.vm.launch()
> +        self.qmpAddBitmap()
> +
> +        # Calculate sha256 corresponding to regions1
> +        self.writeRegions(regions1)
> +        sha256 = self.getSha256()
> +        result = self.vm.qmp('block-dirty-bitmap-clear', node='drive0',
> +                             name='bitmap0')
> +        self.assert_qmp(result, 'return', {})
> +
> +        self.vm.shutdown()
> +
> +        self.vm = self.mkVmRo()
> +        self.vm.launch()
> +
> +        # We've loaded empty bitmap
> +        assert sha256 != self.getSha256()
> +
> +        # Check that we are in RO mode
> +        self.writeRegions(regions1)
> +        assert sha256 != self.getSha256()
> +

the HMP monitor lets you attempt writes to a Read Only drive...? Or does
it error out and we just don't check the reply?

I would prefer we use an actual dirty sector count here instead; we have
the new API that should make it easy to do.

If the debug SHA changes this might be a little fragile.

ACK otherwise.

> +        result = self.vm.qmp('x-blockdev-reopen', **{
> +            'node-name': 'node0',
> +            'driver': iotests.imgfmt,
> +            'file': {
> +                'driver': 'file',
> +                'filename': disk
> +            },
> +            'read-only': False
> +        })
> +        self.assert_qmp(result, 'return', {})
> +
> +        # Check that bitmap is reopened to RW and we can write to it.
> +        self.writeRegions(regions1)
> +        assert sha256 == self.getSha256()
> +
> +        self.vm.shutdown()
> +
> +
>  if __name__ == '__main__':
>      iotests.main(supported_fmts=['qcow2'])
> diff --git a/tests/qemu-iotests/165.out b/tests/qemu-iotests/165.out
> index ae1213e6f8..fbc63e62f8 100644
> --- a/tests/qemu-iotests/165.out
> +++ b/tests/qemu-iotests/165.out
> @@ -1,5 +1,5 @@
> -.
> +..
>  ----------------------------------------------------------------------
> -Ran 1 tests
> +Ran 2 tests
>  
>  OK
> 

This is a suggestion for an even more rigorous test:

- Create bitmap
- Write a region or two
- Record the dirty count and the SHA; assert it is equal to known /
predetermined values.
- reopen RO
- verify the bitmap still exists and that the hash and count are the same.
- Stop the VM
- Start the VM in readonly mode
- verify the bitmap still exists and that the hash and count are the same.
- Reopen-RW
- verify the bitmap still exists and that the hash and count are the same.
- Write further region(s)
- Get the new dirty count and SHA, and assert it is equal to known /
predetermined values.

Re: [Qemu-devel] [PATCH v4 04/10] iotests.py: add event_wait_log and events_wait_log helpers

[John Snow]

On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/iotests.py | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index ce74177ab1..4ad265f140 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -540,6 +540,16 @@ class VM(qtest.QEMUQtestMachine):
>          log(result, filters, indent=indent)
>          return result
>  
> +    def event_wait_log(self, name, **kwargs):
> +        event = self.event_wait(name, **kwargs)
> +        log(event, filters=[filter_qmp_event])
> +        return event
> +
> +    def events_wait_log(self, events, **kwargs):
> +        event = self.events_wait(events, **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,
>                  pre_finalize=None, use_log=True, wait=60.0):
> 

I'm not sure these are really needed, since you can just log the event
you get after calling either of these methods anyway. There's nothing
stopping you from:

```
event = event_wait_log(...)
log(filter_qmp_event(event))
```

Re: [Qemu-devel] [PATCH v4 08/10] iotests: add test 260 to check bitmap life after snapshot + commit

[John Snow]

On 8/7/19 10:12 AM, 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>

OK:

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

there may or may not be conflicts on test setup, depending on when the
iotests logging changes go in, but that can be handled.

> ---
>  tests/qemu-iotests/260     | 87 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/260.out | 52 +++++++++++++++++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 140 insertions(+)
>  create mode 100755 tests/qemu-iotests/260
>  create mode 100644 tests/qemu-iotests/260.out
> 
> diff --git a/tests/qemu-iotests/260 b/tests/qemu-iotests/260
> new file mode 100755
> index 0000000000..d8fcf4567a
> --- /dev/null
> +++ b/tests/qemu-iotests/260
> @@ -0,0 +1,87 @@
> +#!/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.events_wait_log((('BLOCK_JOB_READY', None),
> +                             ('BLOCK_JOB_COMPLETED', None)))
> +    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)
> +test(persistent=True, restart=False)
> +test(persistent=True, restart=True)
> diff --git a/tests/qemu-iotests/260.out b/tests/qemu-iotests/260.out
> new file mode 100644
> index 0000000000..2f0d98d036
> --- /dev/null
> +++ b/tests/qemu-iotests/260.out
> @@ -0,0 +1,52 @@
> +
> +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
> +
> +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
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index f13e5f2e23..06f1ea904c 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -271,3 +271,4 @@
>  254 rw backing quick
>  255 rw quick
>  256 rw quick
> +260 rw auto quick
> 

Re: [PATCH v4 02/10] block: reverse order for reopen commits

[John Snow]

On 9/24/19 6:12 AM, Max Reitz wrote:
> On 07.08.19 16:12, Vladimir Sementsov-Ogievskiy wrote:
>> It's needed to fix reopening qcow2 with bitmaps to RW. Currently it
>> can't work, as qcow2 needs write access to file child, to mark bitmaps
>> in-image with IN_USE flag. But usually children goes after parents in
>> reopen queue and file child is still RO on qcow2 reopen commit. Reverse
>> reopen order to fix it.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>  block.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 696162cd7a..d59f9f97cb 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3476,10 +3476,16 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp)
>>          bs_entry->perms_checked = true;
>>      }
>>  
>> -    /* If we reach this point, we have success and just need to apply the
>> -     * changes
>> +    /*
>> +     * If we reach this point, we have success and just need to apply the
>> +     * changes.
>> +     *
>> +     * Reverse order is used to comfort qcow2 driver: on commit it need to write
>> +     * IN_USE flag to the image, to mark bitmaps in the image as invalid. But
>> +     * children are usually goes after parents in reopen-queue, so go from last
>> +     * to first element.
>>       */
>> -    QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
>> +    QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) {
>>          bdrv_reopen_commit(&bs_entry->state);
>>      }
> 
> I suppose this works, but only because everything but the IN_USE thing
> actually behaves correctly.  In theory, all the work is done by the time
> .prepare is through, so we can call commit in any order anyway.
> 
> So I’m still of the opinion that writing IN_USE in commit is just plain
> wrong.
> 
> It feels like you’re trying to work around wrongs in reopen by piling
> more wrongs on top.  I don’t like reopen already, and I don’t think this
> makes it any better.
> 
> I don’t like how the comment implies that everything is just as it
> should be, but that isn’t the real problem here, so whatever.
> 
> 
> Well, at least the change is simple, and it doesn’t make things worse
> than they actually are already (that is, wrong), so
> 
> Acked-by: Max Reitz <mreitz@redhat.com>
> 

Thanks, Max!

Unfortunate, but I agree.

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

Re: [Qemu-devel] [PATCH v4 09/10] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw

[John Snow]

On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> - 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 following commit),
>   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 a636dc50ca..e276a95154 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;
>          }
>  

I think you can actually drop the definite article, because the image
name is the specifier.

"Unexpected bitmap '%s' in image '%s'" is sufficient.

> -        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;
> +            }

We support RW --> RW now, but what happens if something is marked IN_USE
on RO --> RW? It's not obvious from this function alone why that's safe
to ignore.

> +
> +            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);
> 

Seems OK otherwise, but I just have that one doubt.

Re: [Qemu-devel] [PATCH v4 10/10] qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit

[John Snow]

On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> The only reason I can imagine for this strange code at the very-end of
> bdrv_reopen_commit is the fact that bs->read_only updated after
> calling drv->bdrv_reopen_commit in bdrv_reopen_commit. And in the same
> time, prior to previous commit, qcow2_reopen_bitmaps_rw did a wrong
> check for being writable, when actually it only need writable file
> child not self.
> 
> So, as it's fixed, let's move things to correct place.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block_int.h |  6 ------
>  block.c                   | 19 -------------------
>  block/qcow2.c             | 15 ++++++++++++++-
>  3 files changed, 14 insertions(+), 26 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 3aa1e832a8..18a1e81194 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -531,12 +531,6 @@ struct BlockDriver {
>                               uint64_t parent_perm, uint64_t parent_shared,
>                               uint64_t *nperm, uint64_t *nshared);
>  
> -    /**
> -     * 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 d59f9f97cb..395bc88045 100644
> --- a/block.c
> +++ b/block.c
> @@ -3925,16 +3925,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);
> @@ -3978,21 +3974,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 5c1187e2f9..9e6210c282 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1828,6 +1828,20 @@ fail:
>  static void qcow2_reopen_commit(BDRVReopenState *state)
>  {
>      qcow2_update_options_commit(state->bs, state->opaque);
> +    if (state->flags & BDRV_O_RDWR) {
> +        Error *local_err = NULL;
> +
> +        if (qcow2_reopen_bitmaps_rw(state->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 or retry reopen.
> +             */
> +            error_reportf_err(local_err,
> +                              "%s: Failed to make dirty bitmaps writable: ",
> +                              bdrv_get_node_name(state->bs));
> +        }
> +    }
>      g_free(state->opaque);
>  }
>  
> @@ -5229,7 +5243,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,
>  };
> 

Makes sense to me -- bitmap reopen should happen when a specific driver
needs to reopen. It was a weird top-level driver callback.

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

Max, can you please review this one as well?

Re: [Qemu-devel] [PATCH v4 00/10] qcow2-bitmaps: rewrite reopening logic

[John Snow]

On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Bitmaps reopening is buggy, reopening-rw just not working at all and
> reopening-ro may lead to producing broken incremental
> backup if we do temporary snapshot in a meantime.
> 
> v4: Drop complicated solution around reopening logic [Kevin], fix
>     the existing bug in a simplest way
> 

Overall, seems good. I want Max to take a look at 10/10, and there are
some minor rebase conflicts. I hope to get this staged next week if at
all possible.

Thanks,
--js

> Structure:
> 
> 02: fix reopen to RW
> 03: test reopen to RW
> 
> 07: fix reopen to RO
> 08: test reopen to RO
> 
> Others are less significant improvements and refactoring
> 
> Changelog:
> 
> 01-03: new patches, to fix reopening bitmaps to RW and personal test for
>        this bug
> 08: merged test from v3, it covers both bugs (reopen to RW and reopen to RO)
> 10: instead of moving bitmap-reopening to prepare(as in 09 in v3) we now keep it
>     in commit, but in right place
> others: unchanged
> 
> v3:
> 02: John's events_wait already merged in, so my 02 from v2 is not needed.
>     Instead, add two simple logging wrappers here
> 03: rebase on 02 - use new wrappers, move to 260
> 05: add John's r-b
> 06: improve function docs [John], add John's r-b
> 
> 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 (10):
>   block: switch reopen queue from QSIMPLEQ to QTAILQ
>   block: reverse order for reopen commits
>   iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW
>   iotests.py: add event_wait_log and events_wait_log helpers
>   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
>   iotests: add test 260 to check bitmap life after snapshot + commit
>   block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw
>   qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit
> 
>  block/qcow2.h                 |   5 +-
>  include/block/block.h         |   2 +-
>  include/block/block_int.h     |   6 --
>  include/block/dirty-bitmap.h  |   1 -
>  block.c                       |  51 +++++-------
>  block/dirty-bitmap.c          |  12 ---
>  block/qcow2-bitmap.c          | 143 ++++++++++++++++++++--------------
>  block/qcow2.c                 |  17 +++-
>  tests/qemu-iotests/165        |  46 ++++++++++-
>  tests/qemu-iotests/165.out    |   4 +-
>  tests/qemu-iotests/260        |  87 +++++++++++++++++++++
>  tests/qemu-iotests/260.out    |  52 +++++++++++++
>  tests/qemu-iotests/group      |   1 +
>  tests/qemu-iotests/iotests.py |  10 +++
>  14 files changed, 318 insertions(+), 119 deletions(-)
>  create mode 100755 tests/qemu-iotests/260
>  create mode 100644 tests/qemu-iotests/260.out
> 

Re: [Qemu-devel] [PATCH v4 03/10] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW

[Vladimir Sementsov-Ogievskiy]

27.09.2019 1:57, John Snow wrote:
> 
> 
> On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Reopening bitmaps to RW was broken prior to previous commit. Check that
>> it works now.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/165     | 46 ++++++++++++++++++++++++++++++++++++--
>>   tests/qemu-iotests/165.out |  4 ++--
>>   2 files changed, 46 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
>> index 88f62d3c6d..dd93b5a2d0 100755
>> --- a/tests/qemu-iotests/165
>> +++ b/tests/qemu-iotests/165
>> @@ -43,10 +43,10 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
>>           os.remove(disk)
>>   
>>       def mkVm(self):
>> -        return iotests.VM().add_drive(disk)
>> +        return iotests.VM().add_drive(disk, opts='node-name=node0')
>>   
>>       def mkVmRo(self):
>> -        return iotests.VM().add_drive(disk, opts='readonly=on')
>> +        return iotests.VM().add_drive(disk, opts='readonly=on,node-name=node0')
>>   
>>       def getSha256(self):
>>           result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
>> @@ -102,5 +102,47 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
>>   
>>           self.vm.shutdown()
>>   
>> +    def test_reopen_rw(self):
>> +        self.vm = self.mkVm()
>> +        self.vm.launch()
>> +        self.qmpAddBitmap()
>> +
>> +        # Calculate sha256 corresponding to regions1
>> +        self.writeRegions(regions1)
>> +        sha256 = self.getSha256()
>> +        result = self.vm.qmp('block-dirty-bitmap-clear', node='drive0',
>> +                             name='bitmap0')
>> +        self.assert_qmp(result, 'return', {})
>> +
>> +        self.vm.shutdown()
>> +
>> +        self.vm = self.mkVmRo()
>> +        self.vm.launch()
>> +
>> +        # We've loaded empty bitmap
>> +        assert sha256 != self.getSha256()
>> +
>> +        # Check that we are in RO mode
>> +        self.writeRegions(regions1)
>> +        assert sha256 != self.getSha256()
>> +
> 
> the HMP monitor lets you attempt writes to a Read Only drive...? Or does
> it error out and we just don't check the reply?

It must fail and we check this by comparing dirty bitmap hash.

> 
> I would prefer we use an actual dirty sector count here instead; we have
> the new API that should make it easy to do.
> 
> If the debug SHA changes this might be a little fragile.

Hmm, I agree that checking that bitmap is empty by comparing with some hash
is not very reliable. Will do.

> 
> ACK otherwise.
> 
>> +        result = self.vm.qmp('x-blockdev-reopen', **{
>> +            'node-name': 'node0',
>> +            'driver': iotests.imgfmt,
>> +            'file': {
>> +                'driver': 'file',
>> +                'filename': disk
>> +            },
>> +            'read-only': False
>> +        })
>> +        self.assert_qmp(result, 'return', {})
>> +
>> +        # Check that bitmap is reopened to RW and we can write to it.
>> +        self.writeRegions(regions1)
>> +        assert sha256 == self.getSha256()
>> +
>> +        self.vm.shutdown()
>> +
>> +
>>   if __name__ == '__main__':
>>       iotests.main(supported_fmts=['qcow2'])
>> diff --git a/tests/qemu-iotests/165.out b/tests/qemu-iotests/165.out
>> index ae1213e6f8..fbc63e62f8 100644
>> --- a/tests/qemu-iotests/165.out
>> +++ b/tests/qemu-iotests/165.out
>> @@ -1,5 +1,5 @@
>> -.
>> +..
>>   ----------------------------------------------------------------------
>> -Ran 1 tests
>> +Ran 2 tests
>>   
>>   OK
>>
> 
> This is a suggestion for an even more rigorous test:
> 
> - Create bitmap
> - Write a region or two
> - Record the dirty count and the SHA; assert it is equal to known /
> predetermined values.
> - reopen RO
> - verify the bitmap still exists and that the hash and count are the same.
> - Stop the VM
> - Start the VM in readonly mode
> - verify the bitmap still exists and that the hash and count are the same.
> - Reopen-RW
> - verify the bitmap still exists and that the hash and count are the same.
> - Write further region(s)
> - Get the new dirty count and SHA, and assert it is equal to known /
> predetermined values.
> 

OK

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 04/10] iotests.py: add event_wait_log and events_wait_log helpers

[Vladimir Sementsov-Ogievskiy]

27.09.2019 2:05, John Snow wrote:
> 
> 
> On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/iotests.py | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index ce74177ab1..4ad265f140 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -540,6 +540,16 @@ class VM(qtest.QEMUQtestMachine):
>>           log(result, filters, indent=indent)
>>           return result
>>   
>> +    def event_wait_log(self, name, **kwargs):
>> +        event = self.event_wait(name, **kwargs)
>> +        log(event, filters=[filter_qmp_event])
>> +        return event
>> +
>> +    def events_wait_log(self, events, **kwargs):
>> +        event = self.events_wait(events, **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,
>>                   pre_finalize=None, use_log=True, wait=60.0):
>>
> 
> I'm not sure these are really needed, since you can just log the event
> you get after calling either of these methods anyway. There's nothing
> stopping you from:
> 
> ```
> event = event_wait_log(...)
> log(filter_qmp_event(event))
> ```

two lines vs one

Hm, just simple wrappers like qmp_log(), to make test a bit more readable, why not..

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 09/10] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw

[Vladimir Sementsov-Ogievskiy]

27.09.2019 2:21, John Snow wrote:
> 
> 
> On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>> - 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 following commit),
>>    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 a636dc50ca..e276a95154 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;
>>           }
>>   
> 
> I think you can actually drop the definite article, because the image
> name is the specifier.
> 
> "Unexpected bitmap '%s' in image '%s'" is sufficient.
> 
>> -        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;
>> +            }
> 
> We support RW --> RW now, but what happens if something is marked IN_USE
> on RO --> RW? It's not obvious from this function alone why that's safe
> to ignore.

If bitmap is IN_USE in the image, it's always safe to ignore it, as it's marked as
invalid anyway.

Consider RO->RW with IN_USE.

if corresponding BdrvDirtyBitmap is inconsistent, it's OK.

If not, how can that be? I can't imagine. But we have correct bitmap in RAM, should
we mark it inconsistent, because of bitmap in image? I don't know.


> 
>> +
>> +            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);
>>
> 
> Seems OK otherwise, but I just have that one doubt.
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 00/10] qcow2-bitmaps: rewrite reopening logic

[Vladimir Sementsov-Ogievskiy]

27.09.2019 2:25, John Snow wrote:
> 
> On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Bitmaps reopening is buggy, reopening-rw just not working at all and
>> reopening-ro may lead to producing broken incremental
>> backup if we do temporary snapshot in a meantime.
>>
>> v4: Drop complicated solution around reopening logic [Kevin], fix
>>      the existing bug in a simplest way
>>
> Overall, seems good. I want Max to take a look at 10/10, and there are
> some minor rebase conflicts. I hope to get this staged next week if at
> all possible.
> 
> Thanks,
> --js
> 

Thank you for reviewing!

-- 
Best regards,
Vladimir

Re: [PATCH v4 10/10] qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_commit

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 4980 bytes --]

On 07.08.19 16:12, Vladimir Sementsov-Ogievskiy wrote:
> The only reason I can imagine for this strange code at the very-end of
> bdrv_reopen_commit is the fact that bs->read_only updated after
> calling drv->bdrv_reopen_commit in bdrv_reopen_commit. And in the same
> time, prior to previous commit, qcow2_reopen_bitmaps_rw did a wrong
> check for being writable, when actually it only need writable file
> child not self.
> 
> So, as it's fixed, let's move things to correct place.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block_int.h |  6 ------
>  block.c                   | 19 -------------------
>  block/qcow2.c             | 15 ++++++++++++++-
>  3 files changed, 14 insertions(+), 26 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 3aa1e832a8..18a1e81194 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -531,12 +531,6 @@ struct BlockDriver {
>                               uint64_t parent_perm, uint64_t parent_shared,
>                               uint64_t *nperm, uint64_t *nshared);
>  
> -    /**
> -     * 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 d59f9f97cb..395bc88045 100644
> --- a/block.c
> +++ b/block.c
> @@ -3925,16 +3925,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);
> @@ -3978,21 +3974,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 5c1187e2f9..9e6210c282 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1828,6 +1828,20 @@ fail:
>  static void qcow2_reopen_commit(BDRVReopenState *state)
>  {
>      qcow2_update_options_commit(state->bs, state->opaque);
> +    if (state->flags & BDRV_O_RDWR) {
> +        Error *local_err = NULL;
> +
> +        if (qcow2_reopen_bitmaps_rw(state->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 or retry reopen.
> +             */

It’s still my impression that this is absolutely fatal, because that
means the node isn’t actually writable, and that means the reopen
effectively failed.

But again, it doesn’t make things worse.

Assuming the RW -> RW transition is a no-op now (the previous patch
claims to handle that case):

Acked-by: Max Reitz <mreitz@redhat.com>

> +            error_reportf_err(local_err,
> +                              "%s: Failed to make dirty bitmaps writable: ",
> +                              bdrv_get_node_name(state->bs));
> +        }
> +    }
>      g_free(state->opaque);
>  }
>  
> @@ -5229,7 +5243,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,
>  };
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v4 03/10] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW

[Vladimir Sementsov-Ogievskiy]

27.09.2019 10:28, Vladimir Sementsov-Ogievskiy wrote:
> 27.09.2019 1:57, John Snow wrote:
>>
>>
>> On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Reopening bitmaps to RW was broken prior to previous commit. Check that
>>> it works now.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   tests/qemu-iotests/165     | 46 ++++++++++++++++++++++++++++++++++++--
>>>   tests/qemu-iotests/165.out |  4 ++--
>>>   2 files changed, 46 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
>>> index 88f62d3c6d..dd93b5a2d0 100755
>>> --- a/tests/qemu-iotests/165
>>> +++ b/tests/qemu-iotests/165
>>> @@ -43,10 +43,10 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
>>>           os.remove(disk)
>>>       def mkVm(self):
>>> -        return iotests.VM().add_drive(disk)
>>> +        return iotests.VM().add_drive(disk, opts='node-name=node0')
>>>       def mkVmRo(self):
>>> -        return iotests.VM().add_drive(disk, opts='readonly=on')
>>> +        return iotests.VM().add_drive(disk, opts='readonly=on,node-name=node0')
>>>       def getSha256(self):
>>>           result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
>>> @@ -102,5 +102,47 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
>>>           self.vm.shutdown()
>>> +    def test_reopen_rw(self):
>>> +        self.vm = self.mkVm()
>>> +        self.vm.launch()
>>> +        self.qmpAddBitmap()
>>> +
>>> +        # Calculate sha256 corresponding to regions1
>>> +        self.writeRegions(regions1)
>>> +        sha256 = self.getSha256()
>>> +        result = self.vm.qmp('block-dirty-bitmap-clear', node='drive0',
>>> +                             name='bitmap0')
>>> +        self.assert_qmp(result, 'return', {})
>>> +
>>> +        self.vm.shutdown()
>>> +
>>> +        self.vm = self.mkVmRo()
>>> +        self.vm.launch()
>>> +
>>> +        # We've loaded empty bitmap
>>> +        assert sha256 != self.getSha256()
>>> +
>>> +        # Check that we are in RO mode
>>> +        self.writeRegions(regions1)
>>> +        assert sha256 != self.getSha256()
>>> +
>>
>> the HMP monitor lets you attempt writes to a Read Only drive...? Or does
>> it error out and we just don't check the reply?
> 
> It must fail and we check this by comparing dirty bitmap hash.
> 
>>
>> I would prefer we use an actual dirty sector count here instead; we have
>> the new API that should make it easy to do.
>>
>> If the debug SHA changes this might be a little fragile.
> 
> Hmm, I agree that checking that bitmap is empty by comparing with some hash
> is not very reliable. Will do.
> 
>>
>> ACK otherwise.
>>
>>> +        result = self.vm.qmp('x-blockdev-reopen', **{
>>> +            'node-name': 'node0',
>>> +            'driver': iotests.imgfmt,
>>> +            'file': {
>>> +                'driver': 'file',
>>> +                'filename': disk
>>> +            },
>>> +            'read-only': False
>>> +        })
>>> +        self.assert_qmp(result, 'return', {})
>>> +
>>> +        # Check that bitmap is reopened to RW and we can write to it.
>>> +        self.writeRegions(regions1)
>>> +        assert sha256 == self.getSha256()
>>> +
>>> +        self.vm.shutdown()
>>> +
>>> +
>>>   if __name__ == '__main__':
>>>       iotests.main(supported_fmts=['qcow2'])
>>> diff --git a/tests/qemu-iotests/165.out b/tests/qemu-iotests/165.out
>>> index ae1213e6f8..fbc63e62f8 100644
>>> --- a/tests/qemu-iotests/165.out
>>> +++ b/tests/qemu-iotests/165.out
>>> @@ -1,5 +1,5 @@
>>> -.
>>> +..
>>>   ----------------------------------------------------------------------
>>> -Ran 1 tests
>>> +Ran 2 tests
>>>   OK
>>>
>>
>> This is a suggestion for an even more rigorous test:
>>
>> - Create bitmap
>> - Write a region or two
>> - Record the dirty count and the SHA; assert it is equal to known /

Still, I don't think we need to check count, if we check SHA.

>> predetermined values.
>> - reopen RO
>> - verify the bitmap still exists and that the hash and count are the same.
>> - Stop the VM
>> - Start the VM in readonly mode
>> - verify the bitmap still exists and that the hash and count are the same.
>> - Reopen-RW
>> - verify the bitmap still exists and that the hash and count are the same.
>> - Write further region(s)
>> - Get the new dirty count and SHA, and assert it is equal to known /
>> predetermined values.
>>
> 
> OK
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 03/10] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW

[Vladimir Sementsov-Ogievskiy]

27.09.2019 10:28, Vladimir Sementsov-Ogievskiy wrote:
> 27.09.2019 1:57, John Snow wrote:
>>
>>
>> On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Reopening bitmaps to RW was broken prior to previous commit. Check that
>>> it works now.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   tests/qemu-iotests/165     | 46 ++++++++++++++++++++++++++++++++++++--
>>>   tests/qemu-iotests/165.out |  4 ++--
>>>   2 files changed, 46 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
>>> index 88f62d3c6d..dd93b5a2d0 100755
>>> --- a/tests/qemu-iotests/165
>>> +++ b/tests/qemu-iotests/165
>>> @@ -43,10 +43,10 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
>>>           os.remove(disk)
>>>       def mkVm(self):
>>> -        return iotests.VM().add_drive(disk)
>>> +        return iotests.VM().add_drive(disk, opts='node-name=node0')
>>>       def mkVmRo(self):
>>> -        return iotests.VM().add_drive(disk, opts='readonly=on')
>>> +        return iotests.VM().add_drive(disk, opts='readonly=on,node-name=node0')
>>>       def getSha256(self):
>>>           result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
>>> @@ -102,5 +102,47 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
>>>           self.vm.shutdown()
>>> +    def test_reopen_rw(self):
>>> +        self.vm = self.mkVm()
>>> +        self.vm.launch()
>>> +        self.qmpAddBitmap()
>>> +
>>> +        # Calculate sha256 corresponding to regions1
>>> +        self.writeRegions(regions1)
>>> +        sha256 = self.getSha256()
>>> +        result = self.vm.qmp('block-dirty-bitmap-clear', node='drive0',
>>> +                             name='bitmap0')
>>> +        self.assert_qmp(result, 'return', {})
>>> +
>>> +        self.vm.shutdown()
>>> +
>>> +        self.vm = self.mkVmRo()
>>> +        self.vm.launch()
>>> +
>>> +        # We've loaded empty bitmap
>>> +        assert sha256 != self.getSha256()
>>> +
>>> +        # Check that we are in RO mode
>>> +        self.writeRegions(regions1)
>>> +        assert sha256 != self.getSha256()
>>> +
>>
>> the HMP monitor lets you attempt writes to a Read Only drive...? Or does
>> it error out and we just don't check the reply?
> 
> It must fail and we check this by comparing dirty bitmap hash.
> 
>>
>> I would prefer we use an actual dirty sector count here instead; we have
>> the new API that should make it easy to do.
>>
>> If the debug SHA changes this might be a little fragile.
> 
> Hmm, I agree that checking that bitmap is empty by comparing with some hash
> is not very reliable. Will do.
> 
>>
>> ACK otherwise.
>>
>>> +        result = self.vm.qmp('x-blockdev-reopen', **{
>>> +            'node-name': 'node0',
>>> +            'driver': iotests.imgfmt,
>>> +            'file': {
>>> +                'driver': 'file',
>>> +                'filename': disk
>>> +            },
>>> +            'read-only': False
>>> +        })
>>> +        self.assert_qmp(result, 'return', {})
>>> +
>>> +        # Check that bitmap is reopened to RW and we can write to it.
>>> +        self.writeRegions(regions1)
>>> +        assert sha256 == self.getSha256()
>>> +
>>> +        self.vm.shutdown()
>>> +
>>> +
>>>   if __name__ == '__main__':
>>>       iotests.main(supported_fmts=['qcow2'])
>>> diff --git a/tests/qemu-iotests/165.out b/tests/qemu-iotests/165.out
>>> index ae1213e6f8..fbc63e62f8 100644
>>> --- a/tests/qemu-iotests/165.out
>>> +++ b/tests/qemu-iotests/165.out
>>> @@ -1,5 +1,5 @@
>>> -.
>>> +..
>>>   ----------------------------------------------------------------------
>>> -Ran 1 tests
>>> +Ran 2 tests
>>>   OK
>>>
>>
>> This is a suggestion for an even more rigorous test:
>>
>> - Create bitmap
>> - Write a region or two
>> - Record the dirty count and the SHA; assert it is equal to known /
>> predetermined values.
>> - reopen RO
>> - verify the bitmap still exists and that the hash and count are the same.
>> - Stop the VM
>> - Start the VM in readonly mode
>> - verify the bitmap still exists and that the hash and count are the same.
>> - Reopen-RW
>> - verify the bitmap still exists and that the hash and count are the same.
>> - Write further region(s)
>> - Get the new dirty count and SHA, and assert it is equal to known /
>> predetermined values.
>>
> 
> OK
> 

Ah, stop. We can't check reopen RO at this point, as it will be fixed in later patch

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 04/10] iotests.py: add event_wait_log and events_wait_log helpers

[Vladimir Sementsov-Ogievskiy]

27.09.2019 10:31, Vladimir Sementsov-Ogievskiy wrote:
> 27.09.2019 2:05, John Snow wrote:
>>
>>
>> On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   tests/qemu-iotests/iotests.py | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index ce74177ab1..4ad265f140 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -540,6 +540,16 @@ class VM(qtest.QEMUQtestMachine):
>>>           log(result, filters, indent=indent)
>>>           return result
>>> +    def event_wait_log(self, name, **kwargs):
>>> +        event = self.event_wait(name, **kwargs)
>>> +        log(event, filters=[filter_qmp_event])
>>> +        return event
>>> +
>>> +    def events_wait_log(self, events, **kwargs):
>>> +        event = self.events_wait(events, **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,
>>>                   pre_finalize=None, use_log=True, wait=60.0):
>>>
>>
>> I'm not sure these are really needed, since you can just log the event
>> you get after calling either of these methods anyway. There's nothing
>> stopping you from:
>>
>> ```
>> event = event_wait_log(...)
>> log(filter_qmp_event(event))
>> ```
> 
> two lines vs one
> 
> Hm, just simple wrappers like qmp_log(), to make test a bit more readable, why not..
> 

Still keeping in mind idea of global logging turn on/off, it may be bad to increase number of f_log
function versions, it remember me the pain with _locked APIs in dirty-bitmaps.

OK, I'll drop it.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 03/10] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW

[John Snow]

On 9/27/19 6:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> 27.09.2019 10:28, Vladimir Sementsov-Ogievskiy wrote:
>> 27.09.2019 1:57, John Snow wrote:
>>>
>>>
>>> On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Reopening bitmaps to RW was broken prior to previous commit. Check that
>>>> it works now.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>   tests/qemu-iotests/165     | 46 ++++++++++++++++++++++++++++++++++++--
>>>>   tests/qemu-iotests/165.out |  4 ++--
>>>>   2 files changed, 46 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
>>>> index 88f62d3c6d..dd93b5a2d0 100755
>>>> --- a/tests/qemu-iotests/165
>>>> +++ b/tests/qemu-iotests/165
>>>> @@ -43,10 +43,10 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
>>>>           os.remove(disk)
>>>>       def mkVm(self):
>>>> -        return iotests.VM().add_drive(disk)
>>>> +        return iotests.VM().add_drive(disk, opts='node-name=node0')
>>>>       def mkVmRo(self):
>>>> -        return iotests.VM().add_drive(disk, opts='readonly=on')
>>>> +        return iotests.VM().add_drive(disk, opts='readonly=on,node-name=node0')
>>>>       def getSha256(self):
>>>>           result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
>>>> @@ -102,5 +102,47 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
>>>>           self.vm.shutdown()
>>>> +    def test_reopen_rw(self):
>>>> +        self.vm = self.mkVm()
>>>> +        self.vm.launch()
>>>> +        self.qmpAddBitmap()
>>>> +
>>>> +        # Calculate sha256 corresponding to regions1
>>>> +        self.writeRegions(regions1)
>>>> +        sha256 = self.getSha256()
>>>> +        result = self.vm.qmp('block-dirty-bitmap-clear', node='drive0',
>>>> +                             name='bitmap0')
>>>> +        self.assert_qmp(result, 'return', {})
>>>> +
>>>> +        self.vm.shutdown()
>>>> +
>>>> +        self.vm = self.mkVmRo()
>>>> +        self.vm.launch()
>>>> +
>>>> +        # We've loaded empty bitmap
>>>> +        assert sha256 != self.getSha256()
>>>> +
>>>> +        # Check that we are in RO mode
>>>> +        self.writeRegions(regions1)
>>>> +        assert sha256 != self.getSha256()
>>>> +
>>>
>>> the HMP monitor lets you attempt writes to a Read Only drive...? Or does
>>> it error out and we just don't check the reply?
>>
>> It must fail and we check this by comparing dirty bitmap hash.
>>
>>>
>>> I would prefer we use an actual dirty sector count here instead; we have
>>> the new API that should make it easy to do.
>>>
>>> If the debug SHA changes this might be a little fragile.
>>
>> Hmm, I agree that checking that bitmap is empty by comparing with some hash
>> is not very reliable. Will do.
>>
>>>
>>> ACK otherwise.
>>>
>>>> +        result = self.vm.qmp('x-blockdev-reopen', **{
>>>> +            'node-name': 'node0',
>>>> +            'driver': iotests.imgfmt,
>>>> +            'file': {
>>>> +                'driver': 'file',
>>>> +                'filename': disk
>>>> +            },
>>>> +            'read-only': False
>>>> +        })
>>>> +        self.assert_qmp(result, 'return', {})
>>>> +
>>>> +        # Check that bitmap is reopened to RW and we can write to it.
>>>> +        self.writeRegions(regions1)
>>>> +        assert sha256 == self.getSha256()
>>>> +
>>>> +        self.vm.shutdown()
>>>> +
>>>> +
>>>>   if __name__ == '__main__':
>>>>       iotests.main(supported_fmts=['qcow2'])
>>>> diff --git a/tests/qemu-iotests/165.out b/tests/qemu-iotests/165.out
>>>> index ae1213e6f8..fbc63e62f8 100644
>>>> --- a/tests/qemu-iotests/165.out
>>>> +++ b/tests/qemu-iotests/165.out
>>>> @@ -1,5 +1,5 @@
>>>> -.
>>>> +..
>>>>   ----------------------------------------------------------------------
>>>> -Ran 1 tests
>>>> +Ran 2 tests
>>>>   OK
>>>>
>>>
>>> This is a suggestion for an even more rigorous test:
>>>
>>> - Create bitmap
>>> - Write a region or two
>>> - Record the dirty count and the SHA; assert it is equal to known /
>>> predetermined values.
>>> - reopen RO
>>> - verify the bitmap still exists and that the hash and count are the same.
>>> - Stop the VM
>>> - Start the VM in readonly mode
>>> - verify the bitmap still exists and that the hash and count are the same.
>>> - Reopen-RW
>>> - verify the bitmap still exists and that the hash and count are the same.
>>> - Write further region(s)
>>> - Get the new dirty count and SHA, and assert it is equal to known /
>>> predetermined values.
>>>
>>
>> OK
>>
> 
> Ah, stop. We can't check reopen RO at this point, as it will be fixed in later patch
> 

Oh :(

Is it a reopen bug, a qcow2 bug, or a qcow2-bitmap bug?

Re: [Qemu-devel] [PATCH v4 09/10] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw

[John Snow]

On 9/27/19 3:52 AM, Vladimir Sementsov-Ogievskiy wrote:
> 27.09.2019 2:21, John Snow wrote:
>>
>>
>> On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> - 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 following commit),
>>>    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 a636dc50ca..e276a95154 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;
>>>           }
>>>   
>>
>> I think you can actually drop the definite article, because the image
>> name is the specifier.
>>
>> "Unexpected bitmap '%s' in image '%s'" is sufficient.
>>
>>> -        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;
>>> +            }
>>
>> We support RW --> RW now, but what happens if something is marked IN_USE
>> on RO --> RW? It's not obvious from this function alone why that's safe
>> to ignore.
> 
> If bitmap is IN_USE in the image, it's always safe to ignore it, as it's marked as
> invalid anyway.
> 

Oh, okay -- we're relying on the initial open to have handled this for
us. I only asked because you're doing a lot of additional "sanity
checking" here and wondered.

(That's one drawback to doing sanity checking -- you give the impression
that we expect these combinations to be able to occur.)

> Consider RO->RW with IN_USE.
> 
> if corresponding BdrvDirtyBitmap is inconsistent, it's OK.
> 
> If not, how can that be? I can't imagine. But we have correct bitmap in RAM, should
> we mark it inconsistent, because of bitmap in image? I don't know.
> 

Yeah, I wouldn't know what to make of this occurrence either. I don't
expect it to be able to happen.

If it's +ro -inconsistent and +in_use, we absolutely have a bug
somewhere and we don't know what the right thing to do is because we're
outside of the expected state machine.

Hmm...

If it was RO, then we know we wouldn't be able to have any changes to
disk since we loaded the bitmap, and yet we have a clear state mismatch.

1) We loaded the bitmap incorrectly. Not very likely, but very severe if
true.
2) The disk changed without QEMU's consent.

It's probably the case that we shouldn't be working on this image
anymore -- this would be a very high-level corruption event; the
qcow2_signal_corruption kind.

AKA: "I have no idea what the hell happened, and since it's not possible
to know, please cease using this image immediately."

--js

Re: [Qemu-devel] [PATCH v4 03/10] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW

[Vladimir Sementsov-Ogievskiy]

[-- Attachment #1: Type: text/plain, Size: 4996 bytes --]



27 сент. 2019 г. 21:30 пользователь John Snow <jsnow@redhat.com> написал:


On 9/27/19 6:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> 27.09.2019 10:28, Vladimir Sementsov-Ogievskiy wrote:
>> 27.09.2019 1:57, John Snow wrote:
>>>
>>>
>>> On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Reopening bitmaps to RW was broken prior to previous commit. Check that
>>>> it works now.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>   tests/qemu-iotests/165     | 46 ++++++++++++++++++++++++++++++++++++--
>>>>   tests/qemu-iotests/165.out |  4 ++--
>>>>   2 files changed, 46 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
>>>> index 88f62d3c6d..dd93b5a2d0 100755
>>>> --- a/tests/qemu-iotests/165
>>>> +++ b/tests/qemu-iotests/165
>>>> @@ -43,10 +43,10 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
>>>>           os.remove(disk)
>>>>       def mkVm(self):
>>>> -        return iotests.VM().add_drive(disk)
>>>> +        return iotests.VM().add_drive(disk, opts='node-name=node0')
>>>>       def mkVmRo(self):
>>>> -        return iotests.VM().add_drive(disk, opts='readonly=on')
>>>> +        return iotests.VM().add_drive(disk, opts='readonly=on,node-name=node0')
>>>>       def getSha256(self):
>>>>           result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
>>>> @@ -102,5 +102,47 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
>>>>           self.vm.shutdown()
>>>> +    def test_reopen_rw(self):
>>>> +        self.vm = self.mkVm()
>>>> +        self.vm.launch()
>>>> +        self.qmpAddBitmap()
>>>> +
>>>> +        # Calculate sha256 corresponding to regions1
>>>> +        self.writeRegions(regions1)
>>>> +        sha256 = self.getSha256()
>>>> +        result = self.vm.qmp('block-dirty-bitmap-clear', node='drive0',
>>>> +                             name='bitmap0')
>>>> +        self.assert_qmp(result, 'return', {})
>>>> +
>>>> +        self.vm.shutdown()
>>>> +
>>>> +        self.vm = self.mkVmRo()
>>>> +        self.vm.launch()
>>>> +
>>>> +        # We've loaded empty bitmap
>>>> +        assert sha256 != self.getSha256()
>>>> +
>>>> +        # Check that we are in RO mode
>>>> +        self.writeRegions(regions1)
>>>> +        assert sha256 != self.getSha256()
>>>> +
>>>
>>> the HMP monitor lets you attempt writes to a Read Only drive...? Or does
>>> it error out and we just don't check the reply?
>>
>> It must fail and we check this by comparing dirty bitmap hash.
>>
>>>
>>> I would prefer we use an actual dirty sector count here instead; we have
>>> the new API that should make it easy to do.
>>>
>>> If the debug SHA changes this might be a little fragile.
>>
>> Hmm, I agree that checking that bitmap is empty by comparing with some hash
>> is not very reliable. Will do.
>>
>>>
>>> ACK otherwise.
>>>
>>>> +        result = self.vm.qmp('x-blockdev-reopen', **{
>>>> +            'node-name': 'node0',
>>>> +            'driver': iotests.imgfmt,
>>>> +            'file': {
>>>> +                'driver': 'file',
>>>> +                'filename': disk
>>>> +            },
>>>> +            'read-only': False
>>>> +        })
>>>> +        self.assert_qmp(result, 'return', {})
>>>> +
>>>> +        # Check that bitmap is reopened to RW and we can write to it.
>>>> +        self.writeRegions(regions1)
>>>> +        assert sha256 == self.getSha256()
>>>> +
>>>> +        self.vm.shutdown()
>>>> +
>>>> +
>>>>   if __name__ == '__main__':
>>>>       iotests.main(supported_fmts=['qcow2'])
>>>> diff --git a/tests/qemu-iotests/165.out b/tests/qemu-iotests/165.out
>>>> index ae1213e6f8..fbc63e62f8 100644
>>>> --- a/tests/qemu-iotests/165.out
>>>> +++ b/tests/qemu-iotests/165.out
>>>> @@ -1,5 +1,5 @@
>>>> -.
>>>> +..
>>>>   ----------------------------------------------------------------------
>>>> -Ran 1 tests
>>>> +Ran 2 tests
>>>>   OK
>>>>
>>>
>>> This is a suggestion for an even more rigorous test:
>>>
>>> - Create bitmap
>>> - Write a region or two
>>> - Record the dirty count and the SHA; assert it is equal to known /
>>> predetermined values.
>>> - reopen RO
>>> - verify the bitmap still exists and that the hash and count are the same.
>>> - Stop the VM
>>> - Start the VM in readonly mode
>>> - verify the bitmap still exists and that the hash and count are the same.
>>> - Reopen-RW
>>> - verify the bitmap still exists and that the hash and count are the same.
>>> - Write further region(s)
>>> - Get the new dirty count and SHA, and assert it is equal to known /
>>> predetermined values.
>>>
>>
>> OK
>>
>
> Ah, stop. We can't check reopen RO at this point, as it will be fixed in later patch
>

Oh :(

Is it a reopen bug, a qcow2 bug, or a qcow2-bitmap bug?

It`s a bug fixed later in this series :)

--
Best regards,
Vladimir

[-- Attachment #2: Type: text/html, Size: 9823 bytes --]

Re: [Qemu-devel] [PATCH v4 09/10] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw

[Vladimir Sementsov-Ogievskiy]

[-- Attachment #1: Type: text/plain, Size: 5576 bytes --]



27 сент. 2019 г. 21:59 пользователь John Snow <jsnow@redhat.com> написал:


On 9/27/19 3:52 AM, Vladimir Sementsov-Ogievskiy wrote:
> 27.09.2019 2:21, John Snow wrote:
>>
>>
>> On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> - 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 following commit),
>>>    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 a636dc50ca..e276a95154 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;
>>>           }
>>>
>>
>> I think you can actually drop the definite article, because the image
>> name is the specifier.
>>
>> "Unexpected bitmap '%s' in image '%s'" is sufficient.
>>
>>> -        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;
>>> +            }
>>
>> We support RW --> RW now, but what happens if something is marked IN_USE
>> on RO --> RW? It's not obvious from this function alone why that's safe
>> to ignore.
>
> If bitmap is IN_USE in the image, it's always safe to ignore it, as it's marked as
> invalid anyway.
>

Oh, okay -- we're relying on the initial open to have handled this for
us. I only asked because you're doing a lot of additional "sanity
checking" here and wondered.

(That's one drawback to doing sanity checking -- you give the impression
that we expect these combinations to be able to occur.)

> Consider RO->RW with IN_USE.
>
> if corresponding BdrvDirtyBitmap is inconsistent, it's OK.
>
> If not, how can that be? I can't imagine. But we have correct bitmap in RAM, should
> we mark it inconsistent, because of bitmap in image? I don't know.
>

Yeah, I wouldn't know what to make of this occurrence either. I don't
expect it to be able to happen.

If it's +ro -inconsistent and +in_use, we absolutely have a bug
somewhere and we don't know what the right thing to do is because we're
outside of the expected state machine.

Hmm...

If it was RO, then we know we wouldn't be able to have any changes to
disk since we loaded the bitmap, and yet we have a clear state mismatch.

1) We loaded the bitmap incorrectly. Not very likely, but very severe if
true.
2) The disk changed without QEMU's consent.

It's probably the case that we shouldn't be working on this image
anymore -- this would be a very high-level corruption event; the
qcow2_signal_corruption kind.

AKA: "I have no idea what the hell happened, and since it's not possible
to know, please cease using this image immediately."

I decided to handle this is in same way as other corruptions here in v5

--
Best regards,
Vladimir


[-- Attachment #2: Type: text/html, Size: 10999 bytes --]