[Qemu-devel] [PATCH v3 for-4.1? 0/9] qcow2-bitmaps: rewrite reopening logic

[Qemu-devel] [PATCH v3 for-4.1? 0/9] qcow2-bitmaps: rewrite reopening logic

[Vladimir Sementsov-Ogievskiy]

Hi all!

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

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 (9):
  block: add .bdrv_need_rw_file_child_during_reopen_rw handler
  iotests.py: add event_wait_log and events_wait_log helpers
  iotests: add test 260 to check bitmap life after snapshot + commit
  block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps
  block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint()
  block/qcow2-bitmap: do not remove bitmaps on reopen-ro
  block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw
  block/qcow2-bitmap: fix reopening bitmaps to RW
  qcow2-bitmap: move bitmap reopen-rw code to qcow2_reopen_prepare

 block/qcow2.h                 |   6 +-
 include/block/block_int.h     |   8 +-
 include/block/dirty-bitmap.h  |   1 -
 block.c                       | 163 +++++++++++++++++++++++++++-------
 block/dirty-bitmap.c          |  12 ---
 block/qcow2-bitmap.c          | 149 +++++++++++++++++++------------
 block/qcow2.c                 |   9 +-
 tests/qemu-iotests/260        |  87 ++++++++++++++++++
 tests/qemu-iotests/260.out    |  52 +++++++++++
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |  10 +++
 11 files changed, 384 insertions(+), 114 deletions(-)
 create mode 100755 tests/qemu-iotests/260
 create mode 100644 tests/qemu-iotests/260.out

-- 
2.18.0

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

[Vladimir Sementsov-Ogievskiy]

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

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

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

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

[Qemu-devel] [PATCH v3 2/9] 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 v3 3/9] 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     | 85 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/260.out | 17 ++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 103 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..51288b8ee7
--- /dev/null
+++ b/tests/qemu-iotests/260
@@ -0,0 +1,85 @@
+#!/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)
diff --git a/tests/qemu-iotests/260.out b/tests/qemu-iotests/260.out
new file mode 100644
index 0000000000..5239d27c46
--- /dev/null
+++ b/tests/qemu-iotests/260.out
@@ -0,0 +1,17 @@
+
+Testcase non-persistent without restart
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": "drive0", "persistent": false}}
+{"return": {}}
+initial bitmap: name=bitmap0 dirty-clusters=1
+{"execute": "blockdev-snapshot-sync", "arguments": {"device": "drive0", "format": "qcow2", "snapshot-file": "TEST_DIR/PID-top"}}
+{"return": {}}
+check that no bitmaps are in snapshot: not found
+{"execute": "block-commit", "arguments": {"device": "drive0", "top": "TEST_DIR/PID-top"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "block-job-complete", "arguments": {"device": "drive0"}}
+{"return": {}}
+{"data": {"device": "drive0", "len": 65536, "offset": 65536, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+check bitmap after commit: name=bitmap0 dirty-clusters=2
+check updated bitmap: name=bitmap0 dirty-clusters=3
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 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 v3 4/9] block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps

[Vladimir Sementsov-Ogievskiy]

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

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

diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 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 v3 5/9] block/qcow2-bitmap: drop qcow2_reopen_bitmaps_rw_hint()

[Vladimir Sementsov-Ogievskiy]

The function is unused, drop it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
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 v3 6/9] block/qcow2-bitmap: do not remove bitmaps on reopen-ro

[Vladimir Sementsov-Ogievskiy]

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

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

So, let's stop removing bitmaps on reopen-ro. But don't rejoice:
reopening bitmaps to rw is broken too, so the whole scenario will not
work after this patch and we can't enable corresponding test cases in
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 v3 7/9] block/qcow2-bitmap: fix and improve qcow2_reopen_bitmaps_rw

[Vladimir Sementsov-Ogievskiy]

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

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

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 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 v3 8/9] block/qcow2-bitmap: fix reopening bitmaps to RW

[Vladimir Sementsov-Ogievskiy]

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

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

Add corresponding test-cases to 260 iotest.

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

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

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

[Vladimir Sementsov-Ogievskiy]

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

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

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

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7bd6fd68dd..5077b26561 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -533,12 +533,6 @@ struct BlockDriver {
 
      bool (*bdrv_need_rw_file_child_during_reopen_rw)(BlockDriverState *bs);
 
-    /**
-     * Bitmaps should be marked as 'IN_USE' in the image on reopening image
-     * as rw. This handler should realize it. It also should unset readonly
-     * field of BlockDirtyBitmap's in case of success.
-     */
-    int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
     bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs,
                                             const char *name,
                                             uint32_t granularity,
diff --git a/block.c b/block.c
index 3c8e1c59b4..71b4f9961c 100644
--- a/block.c
+++ b/block.c
@@ -4037,16 +4037,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);
@@ -4090,21 +4086,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 1d9fb3ae98..1ef71c1f1f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1815,6 +1815,11 @@ static int qcow2_reopen_prepare(BDRVReopenState *state,
         if (ret < 0) {
             goto fail;
         }
+    } else {
+        ret = qcow2_reopen_bitmaps_rw(state->bs, errp);
+        if (ret < 0) {
+            goto fail;
+        }
     }
 
     return 0;
@@ -5229,7 +5234,6 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_detach_aio_context  = qcow2_detach_aio_context,
     .bdrv_attach_aio_context  = qcow2_attach_aio_context,
 
-    .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
     .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
     .bdrv_remove_persistent_dirty_bitmap = qcow2_remove_persistent_dirty_bitmap,
     .bdrv_need_rw_file_child_during_reopen_rw = qcow2_has_bitmaps,
-- 
2.18.0

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

[Max Reitz]

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

On 25.07.19 11:18, Vladimir Sementsov-Ogievskiy wrote:
> On reopen to rw parent may need rw access to child in .prepare, for
> example qcow2 needs to write IN_USE flags into stored bitmaps
> (currently it is done in a hacky way after commit and don't work).
> So, let's introduce such logic.
> 
> The drawback is that in worst case bdrv_reopen_set_read_only may finish
> with error and in some intermediate state: some nodes reopened RW and
> some are not. But this is a way to fix bug around reopening qcow2
> bitmaps in the following commits.

This commit message doesn’t really explain what this patch does.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/block_int.h |   2 +
>  block.c                   | 144 ++++++++++++++++++++++++++++++++++----
>  2 files changed, 133 insertions(+), 13 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 3aa1e832a8..7bd6fd68dd 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -531,6 +531,8 @@ struct BlockDriver {
>                               uint64_t parent_perm, uint64_t parent_shared,
>                               uint64_t *nperm, uint64_t *nshared);
>  
> +     bool (*bdrv_need_rw_file_child_during_reopen_rw)(BlockDriverState *bs);
> +
>      /**
>       * Bitmaps should be marked as 'IN_USE' in the image on reopening image
>       * as rw. This handler should realize it. It also should unset readonly
> diff --git a/block.c b/block.c
> index cbd8da5f3b..3c8e1c59b4 100644
> --- a/block.c
> +++ b/block.c
> @@ -1715,10 +1715,12 @@ static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
>                                       uint64_t *shared_perm);
>  
>  typedef struct BlockReopenQueueEntry {
> -     bool prepared;
> -     bool perms_checked;
> -     BDRVReopenState state;
> -     QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
> +    bool reopened_file_child_rw;
> +    bool changed_file_child_perm_rw;
> +    bool prepared;
> +    bool perms_checked;
> +    BDRVReopenState state;
> +    QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
>  } BlockReopenQueueEntry;
>  
>  /*
> @@ -3421,6 +3423,105 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>                                     keep_old_opts);
>  }
>  
> +static int bdrv_reopen_set_read_only_drained(BlockDriverState *bs,
> +                                             bool read_only,
> +                                             Error **errp)
> +{
> +    BlockReopenQueue *queue;
> +    QDict *opts = qdict_new();
> +
> +    qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
> +
> +    queue = bdrv_reopen_queue(NULL, bs, opts, true);
> +
> +    return bdrv_reopen_multiple(queue, errp);
> +}
> +
> +/*
> + * handle_recursive_reopens
> + *
> + * On fail it needs rollback_recursive_reopens to be called.

It would be nice if this description actually said anything about what
the function is supposed to do.

> + */
> +static int handle_recursive_reopens(BlockReopenQueueEntry *current,
> +                                    Error **errp)
> +{
> +    int ret;
> +    BlockDriverState *bs = current->state.bs;
> +
> +    /*
> +     * We use the fact that in reopen-queue children are always following
> +     * parents.
> +     * TODO: Switch BlockReopenQueue to be QTAILQ and use
> +     *       QTAILQ_FOREACH_REVERSE.

Why don’t you do that first?  It would make the code more obvious at
least to me.

> +     */
> +    if (QSIMPLEQ_NEXT(current, entry)) {
> +        ret = handle_recursive_reopens(QSIMPLEQ_NEXT(current, entry), errp);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +    }
> +
> +    if ((current->state.flags & BDRV_O_RDWR) && bs->file && bs->drv &&
> +        bs->drv->bdrv_need_rw_file_child_during_reopen_rw &&
> +        bs->drv->bdrv_need_rw_file_child_during_reopen_rw(bs))
> +    {
> +        if (!bdrv_is_writable(bs->file->bs)) {
> +            ret = bdrv_reopen_set_read_only_drained(bs->file->bs, false, errp);

Hm.  Sorry, I find this all a bit hard to understand.  (No comments and
all.)

I understand that this is for an RO -> RW transition?  Everything is
still RO, but the parent will need an RW child before it transitions to
RW itself.


I’m going to be honest up front, I don’t like this very much.  But I
think it may be a reasonable solution for now.

As I remember, the problem was that when reopening a qcow2 node from RO
to RW, we need to write something in .prepare() (because it can fail),
but naturally no .prepare() is called after any .commit(), so no matter
the order of nodes in the ReopenQueue, the child node will never be RW
by this point.

Hm.  To me that mostly means that making the whole reopen process a
transaction was just a dream that turns out not to work.

OK, so what would be the real, proper, all-encompassing fix?  I suppose
we’d need a way to express reopen dependency relationships.  So if a
node depends on one or more of its children to be reopened before/after
it can be reopened itself, we’d need to pull them out of the ReopenQueue
(along with their dependencies) and do a separate bdrv_reopen_multiple()
on them.  So we’d want to split the ReopenQueue into as few subqueues as
possible, so that all dependencies are satisfied.

One such dependency is if you change a node from RO to RW, and that
change requires an RW child.

The reverse dependency occurs is if you change a node from RW to RO, and
the nodes wants to write something to its child, so it needs to remain
RW until then.

Currently, the former is just broken (hence this patch).  The latter is
kind of addressed by virtue of “Writing happens in .prepare(), and
parents come before children in the ReopenQueue”.


OK, so you address one specific case of a dependency, namely of a node
on its bs->file when it comes to writableness.  Not too bad, supporting
bs->file as the only dependency makes sense.  Everything else would
become complicated.


Besides being more specific than the general solution I tried to sketch
above, there is one more difference, though: In that description, I said
we should remove the node and its dependencies from the ReopenQueue, and
commit it earlier.  That has three implicatons:

First, this patch reopens the file if it is not writable, but the parent
needs it to be writable.  I think that’s wrong.  We should take the
file’s entry of the ReopenQueue and reopen it accordingly, not blindly
reopen it RW.  (If the user didn’t specify the file to be reopened RW,
that should be an error.)

Second, you need to take the dependencies into account.  (I don’t know
whether this one is a problem in practice.)  If the file node itself has
a child that needs to be RW, then you need to take that from the
ReopenQueue, too, and repoen it RW, too.

Third, the reopen may require some other options on the file.
Temporarily reopening it RW without changing those options may not be
what the user wants.  (So another reason to take the existing
ReopenQueue entry.)


So -- without having tried, of course -- I think a better design would
be to look for bs->file->bs in the ReopenQueue, recursively all of its
children, and move all of those entries into a new queue, and then
invoke bdrv_reopen_multiple() on that one first.

The question then becomes how to roll back those changes...  I don’t
know whether just having bdrv_reopen() partially succeed is so bad.
Otherwise, we’d need a function to translate an existing node's state
into a BdrvReopenQueueEntry so we can thus return to the old state.

> +            if (ret < 0) {
> +                return ret;
> +            }
> +            current->reopened_file_child_rw = true;
> +        }
> +
> +        if (!(bs->file->perm & BLK_PERM_WRITE)) {
> +            ret = bdrv_child_try_set_perm(bs->file,
> +                                          bs->file->perm | BLK_PERM_WRITE,
> +                                          bs->file->shared_perm, errp);

bdrv_child_try_set_perm() is dangerous, because its effect will be
undone whenever something happens that causes the permissions to be
refreshed.  (Hence the comment in block_int.h which says to avoid it.)
Generally, bdrv_child_refresh_perms() should be enough.  If it isn’t,
I’d say something’s off.

Max

> +            if (ret < 0) {
> +                return ret;
> +            }
> +            current->changed_file_child_perm_rw = true;
> +        }
> +    }
> +
> +    return 0;
> +}


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

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

[Vladimir Sementsov-Ogievskiy]

31.07.2019 15:09, Max Reitz wrote:
> On 25.07.19 11:18, Vladimir Sementsov-Ogievskiy wrote:
>> On reopen to rw parent may need rw access to child in .prepare, for
>> example qcow2 needs to write IN_USE flags into stored bitmaps
>> (currently it is done in a hacky way after commit and don't work).
>> So, let's introduce such logic.
>>
>> The drawback is that in worst case bdrv_reopen_set_read_only may finish
>> with error and in some intermediate state: some nodes reopened RW and
>> some are not. But this is a way to fix bug around reopening qcow2
>> bitmaps in the following commits.
> 
> This commit message doesn’t really explain what this patch does.
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/block_int.h |   2 +
>>   block.c                   | 144 ++++++++++++++++++++++++++++++++++----
>>   2 files changed, 133 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 3aa1e832a8..7bd6fd68dd 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -531,6 +531,8 @@ struct BlockDriver {
>>                                uint64_t parent_perm, uint64_t parent_shared,
>>                                uint64_t *nperm, uint64_t *nshared);
>>   
>> +     bool (*bdrv_need_rw_file_child_during_reopen_rw)(BlockDriverState *bs);
>> +
>>       /**
>>        * Bitmaps should be marked as 'IN_USE' in the image on reopening image
>>        * as rw. This handler should realize it. It also should unset readonly
>> diff --git a/block.c b/block.c
>> index cbd8da5f3b..3c8e1c59b4 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1715,10 +1715,12 @@ static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
>>                                        uint64_t *shared_perm);
>>   
>>   typedef struct BlockReopenQueueEntry {
>> -     bool prepared;
>> -     bool perms_checked;
>> -     BDRVReopenState state;
>> -     QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
>> +    bool reopened_file_child_rw;
>> +    bool changed_file_child_perm_rw;
>> +    bool prepared;
>> +    bool perms_checked;
>> +    BDRVReopenState state;
>> +    QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
>>   } BlockReopenQueueEntry;
>>   
>>   /*
>> @@ -3421,6 +3423,105 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>>                                      keep_old_opts);
>>   }
>>   
>> +static int bdrv_reopen_set_read_only_drained(BlockDriverState *bs,
>> +                                             bool read_only,
>> +                                             Error **errp)
>> +{
>> +    BlockReopenQueue *queue;
>> +    QDict *opts = qdict_new();
>> +
>> +    qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
>> +
>> +    queue = bdrv_reopen_queue(NULL, bs, opts, true);
>> +
>> +    return bdrv_reopen_multiple(queue, errp);
>> +}
>> +
>> +/*
>> + * handle_recursive_reopens
>> + *
>> + * On fail it needs rollback_recursive_reopens to be called.
> 
> It would be nice if this description actually said anything about what
> the function is supposed to do.
> 
>> + */
>> +static int handle_recursive_reopens(BlockReopenQueueEntry *current,
>> +                                    Error **errp)
>> +{
>> +    int ret;
>> +    BlockDriverState *bs = current->state.bs;
>> +
>> +    /*
>> +     * We use the fact that in reopen-queue children are always following
>> +     * parents.
>> +     * TODO: Switch BlockReopenQueue to be QTAILQ and use
>> +     *       QTAILQ_FOREACH_REVERSE.
> 
> Why don’t you do that first?  It would make the code more obvious at
> least to me.
> 
>> +     */
>> +    if (QSIMPLEQ_NEXT(current, entry)) {
>> +        ret = handle_recursive_reopens(QSIMPLEQ_NEXT(current, entry), errp);
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>> +    }
>> +
>> +    if ((current->state.flags & BDRV_O_RDWR) && bs->file && bs->drv &&
>> +        bs->drv->bdrv_need_rw_file_child_during_reopen_rw &&
>> +        bs->drv->bdrv_need_rw_file_child_during_reopen_rw(bs))
>> +    {
>> +        if (!bdrv_is_writable(bs->file->bs)) {
>> +            ret = bdrv_reopen_set_read_only_drained(bs->file->bs, false, errp);
> 
> Hm.  Sorry, I find this all a bit hard to understand.  (No comments and
> all.)
> 
> I understand that this is for an RO -> RW transition?  Everything is
> still RO, but the parent will need an RW child before it transitions to
> RW itself.
> 
> 
> I’m going to be honest up front, I don’t like this very much.  But I
> think it may be a reasonable solution for now.
> 
> As I remember, the problem was that when reopening a qcow2 node from RO
> to RW, we need to write something in .prepare() (because it can fail),
> but naturally no .prepare() is called after any .commit(), so no matter
> the order of nodes in the ReopenQueue, the child node will never be RW
> by this point.


Exactly.

> 
> Hm.  To me that mostly means that making the whole reopen process a
> transaction was just a dream that turns out not to work.
> 
> OK, so what would be the real, proper, all-encompassing fix?  I suppose
> we’d need a way to express reopen dependency relationships.  So if a
> node depends on one or more of its children to be reopened before/after
> it can be reopened itself, we’d need to pull them out of the ReopenQueue
> (along with their dependencies) and do a separate bdrv_reopen_multiple()
> on them.  So we’d want to split the ReopenQueue into as few subqueues as
> possible, so that all dependencies are satisfied.


Hmm. But actully, considering our case as example, reopening qcow2 RO->RW,
in general not dependent on reopening file child fist, but on just RW file
child.. As well as reopening qcow2 RW->RO depends on file child to be RW.

But I agree, that we'd better not doing reopens different from what we have
in the queue.

> 
> One such dependency is if you change a node from RO to RW, and that
> change requires an RW child.

Yes.

> 
> The reverse dependency occurs is if you change a node from RW to RO, and
> the nodes wants to write something to its child, so it needs to remain
> RW until then.

And yes, but actually this is the same dependency, but it should be resolved
in other way.

> 
> Currently, the former is just broken (hence this patch).  The latter is
> kind of addressed by virtue of “Writing happens in .prepare(), and
> parents come before children in the ReopenQueue”.
> 
> 
> OK, so you address one specific case of a dependency, namely of a node
> on its bs->file when it comes to writableness.  Not too bad, supporting
> bs->file as the only dependency makes sense.  Everything else would
> become complicated.
> 
> 
> Besides being more specific than the general solution I tried to sketch
> above, there is one more difference, though: In that description, I said
> we should remove the node and its dependencies from the ReopenQueue, and
> commit it earlier.  That has three implicatons:
> 
> First, this patch reopens the file if it is not writable, but the parent
> needs it to be writable.  I think that’s wrong.  We should take the
> file’s entry of the ReopenQueue and reopen it accordingly, not blindly
> reopen it RW.  (If the user didn’t specify the file to be reopened RW,
> that should be an error.)
> 
> Second, you need to take the dependencies into account.  (I don’t know
> whether this one is a problem in practice.)  If the file node itself has
> a child that needs to be RW, then you need to take that from the
> ReopenQueue, too, and repoen it RW, too.
> 
> Third, the reopen may require some other options on the file.
> Temporarily reopening it RW without changing those options may not be
> what the user wants.  (So another reason to take the existing
> ReopenQueue entry.)
> 
> 
> So -- without having tried, of course -- I think a better design would
> be to look for bs->file->bs in the ReopenQueue, recursively all of its
> children, and move all of those entries into a new queue, and then
> invoke bdrv_reopen_multiple() on that one first.

Why all children recursively? Shouldn't we instead only follow defined
dependencies?
Or it just seems bad to put not full subtree into bdrv_reopen multiple() ?


> 
> The question then becomes how to roll back those changes...  I don’t
> know whether just having bdrv_reopen() partially succeed is so bad.
> Otherwise, we’d need a function to translate an existing node's state
> into a BdrvReopenQueueEntry so we can thus return to the old state.

And this rollback may fail too and we are still in partial success state.

But if we roll-back simple ro->rw reopen it's safe enough: in worst case
file will be rw, but marked ro, so Qemu may have more access rights than
needed but will not use them, this is why I was concentrating around
only ro->rw case..

So, what about go similar way to this patch, i.e. only reopen ro->rw children
which we need to be rw, not touching other flags, but check, that in reopen
queue we have this child, and it is going to be reopened RW, and if not,
return error immediately?

> 
>> +            if (ret < 0) {
>> +                return ret;
>> +            }
>> +            current->reopened_file_child_rw = true;
>> +        }
>> +
>> +        if (!(bs->file->perm & BLK_PERM_WRITE)) {
>> +            ret = bdrv_child_try_set_perm(bs->file,
>> +                                          bs->file->perm | BLK_PERM_WRITE,
>> +                                          bs->file->shared_perm, errp);
> 
> bdrv_child_try_set_perm() is dangerous, because its effect will be
> undone whenever something happens that causes the permissions to be
> refreshed.  (Hence the comment in block_int.h which says to avoid it.)
> Generally, bdrv_child_refresh_perms() should be enough.  If it isn’t,
> I’d say something’s off.
> 
> Max
> 
>> +            if (ret < 0) {
>> +                return ret;
>> +            }
>> +            current->changed_file_child_perm_rw = true;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
> 


-- 
Best regards,
Vladimir

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

[Max Reitz]

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

On 01.08.19 16:02, Vladimir Sementsov-Ogievskiy wrote:
> 31.07.2019 15:09, Max Reitz wrote:

[...]

>> So -- without having tried, of course -- I think a better design would
>> be to look for bs->file->bs in the ReopenQueue, recursively all of its
>> children, and move all of those entries into a new queue, and then
>> invoke bdrv_reopen_multiple() on that one first.
> 
> Why all children recursively? Shouldn't we instead only follow defined
> dependencies?
> Or it just seems bad to put not full subtree into bdrv_reopen multiple() ?

For example, making a node RW without opening its children RW seems
wrong.  Whenever we reopen a node, we should reopen all of its children,
if they are in the queue.

>> The question then becomes how to roll back those changes...  I don’t
>> know whether just having bdrv_reopen() partially succeed is so bad.
>> Otherwise, we’d need a function to translate an existing node's state
>> into a BdrvReopenQueueEntry so we can thus return to the old state.
> 
> And this rollback may fail too and we are still in partial success state.
> 
> But if we roll-back simple ro->rw reopen it's safe enough: in worst case
> file will be rw, but marked ro, so Qemu may have more access rights than
> needed but will not use them, this is why I was concentrating around
> only ro->rw case..

In practice, this is always so.  The “children need to be reopened
before parent” case is always a sign of more permissions being taken;
whereas “children need to be reopened after parent” is a sign of
permissions being released.

What we want to fix now is the former “reopen children before parent”
case.  Because this is always a sign of taking more permissions, a
partial success/failure state means we always have taken more
permissions than we need to.

> So, what about go similar way to this patch, i.e. only reopen ro->rw children
> which we need to be rw, not touching other flags, but check, that in reopen
> queue we have this child, and it is going to be reopened RW, and if not,
> return error immediately?

If the RO -> RW change for the child is accompanied by other options
being changed, the user may find it vital to change these flags along
with the RO/RW access.  We shouldn’t ignore them.

Max


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

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

[Vladimir Sementsov-Ogievskiy]

01.08.2019 22:06, Max Reitz wrote:
> On 01.08.19 16:02, Vladimir Sementsov-Ogievskiy wrote:
>> 31.07.2019 15:09, Max Reitz wrote:
> 
> [...]
> 
>>> So -- without having tried, of course -- I think a better design would
>>> be to look for bs->file->bs in the ReopenQueue, recursively all of its
>>> children, and move all of those entries into a new queue, and then
>>> invoke bdrv_reopen_multiple() on that one first.
>>
>> Why all children recursively? Shouldn't we instead only follow defined
>> dependencies?
>> Or it just seems bad to put not full subtree into bdrv_reopen multiple() ?
> 
> For example, making a node RW without opening its children RW seems
> wrong.  Whenever we reopen a node, we should reopen all of its children,
> if they are in the queue.
> 
>>> The question then becomes how to roll back those changes...  I don’t
>>> know whether just having bdrv_reopen() partially succeed is so bad.
>>> Otherwise, we’d need a function to translate an existing node's state
>>> into a BdrvReopenQueueEntry so we can thus return to the old state.
>>
>> And this rollback may fail too and we are still in partial success state.
>>
>> But if we roll-back simple ro->rw reopen it's safe enough: in worst case
>> file will be rw, but marked ro, so Qemu may have more access rights than
>> needed but will not use them, this is why I was concentrating around
>> only ro->rw case..
> 
> In practice, this is always so.  The “children need to be reopened
> before parent” case is always a sign of more permissions being taken;
> whereas “children need to be reopened after parent” is a sign of
> permissions being released.
> 
> What we want to fix now is the former “reopen children before parent”
> case.  Because this is always a sign of taking more permissions, a
> partial success/failure state means we always have taken more
> permissions than we need to.

OK, I'll try.

> 
>> So, what about go similar way to this patch, i.e. only reopen ro->rw children
>> which we need to be rw, not touching other flags, but check, that in reopen
>> queue we have this child, and it is going to be reopened RW, and if not,
>> return error immediately?
> 
> If the RO -> RW change for the child is accompanied by other options
> being changed, the user may find it vital to change these flags along
> with the RO/RW access.  We shouldn’t ignore them.
> 

Hmm, understand, OK.


-- 
Best regards,
Vladimir

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

[Kevin Wolf]

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

Am 31.07.2019 um 14:09 hat Max Reitz geschrieben:
> On 25.07.19 11:18, Vladimir Sementsov-Ogievskiy wrote:
> > On reopen to rw parent may need rw access to child in .prepare, for
> > example qcow2 needs to write IN_USE flags into stored bitmaps
> > (currently it is done in a hacky way after commit and don't work).
> > So, let's introduce such logic.
> > 
> > The drawback is that in worst case bdrv_reopen_set_read_only may finish
> > with error and in some intermediate state: some nodes reopened RW and
> > some are not. But this is a way to fix bug around reopening qcow2
> > bitmaps in the following commits.
> 
> This commit message doesn’t really explain what this patch does.
> 
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >  include/block/block_int.h |   2 +
> >  block.c                   | 144 ++++++++++++++++++++++++++++++++++----
> >  2 files changed, 133 insertions(+), 13 deletions(-)
> > 
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index 3aa1e832a8..7bd6fd68dd 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -531,6 +531,8 @@ struct BlockDriver {
> >                               uint64_t parent_perm, uint64_t parent_shared,
> >                               uint64_t *nperm, uint64_t *nshared);
> >  
> > +     bool (*bdrv_need_rw_file_child_during_reopen_rw)(BlockDriverState *bs);
> > +
> >      /**
> >       * Bitmaps should be marked as 'IN_USE' in the image on reopening image
> >       * as rw. This handler should realize it. It also should unset readonly
> > diff --git a/block.c b/block.c
> > index cbd8da5f3b..3c8e1c59b4 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1715,10 +1715,12 @@ static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
> >                                       uint64_t *shared_perm);
> >  
> >  typedef struct BlockReopenQueueEntry {
> > -     bool prepared;
> > -     bool perms_checked;
> > -     BDRVReopenState state;
> > -     QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
> > +    bool reopened_file_child_rw;
> > +    bool changed_file_child_perm_rw;
> > +    bool prepared;
> > +    bool perms_checked;
> > +    BDRVReopenState state;
> > +    QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
> >  } BlockReopenQueueEntry;
> >  
> >  /*
> > @@ -3421,6 +3423,105 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
> >                                     keep_old_opts);
> >  }
> >  
> > +static int bdrv_reopen_set_read_only_drained(BlockDriverState *bs,
> > +                                             bool read_only,
> > +                                             Error **errp)
> > +{
> > +    BlockReopenQueue *queue;
> > +    QDict *opts = qdict_new();
> > +
> > +    qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
> > +
> > +    queue = bdrv_reopen_queue(NULL, bs, opts, true);
> > +
> > +    return bdrv_reopen_multiple(queue, errp);
> > +}
> > +
> > +/*
> > + * handle_recursive_reopens
> > + *
> > + * On fail it needs rollback_recursive_reopens to be called.
> 
> It would be nice if this description actually said anything about what
> the function is supposed to do.
> 
> > + */
> > +static int handle_recursive_reopens(BlockReopenQueueEntry *current,
> > +                                    Error **errp)
> > +{
> > +    int ret;
> > +    BlockDriverState *bs = current->state.bs;
> > +
> > +    /*
> > +     * We use the fact that in reopen-queue children are always following
> > +     * parents.
> > +     * TODO: Switch BlockReopenQueue to be QTAILQ and use
> > +     *       QTAILQ_FOREACH_REVERSE.
> 
> Why don’t you do that first?  It would make the code more obvious at
> least to me.
> 
> > +     */
> > +    if (QSIMPLEQ_NEXT(current, entry)) {
> > +        ret = handle_recursive_reopens(QSIMPLEQ_NEXT(current, entry), errp);
> > +        if (ret < 0) {
> > +            return ret;
> > +        }
> > +    }
> > +
> > +    if ((current->state.flags & BDRV_O_RDWR) && bs->file && bs->drv &&
> > +        bs->drv->bdrv_need_rw_file_child_during_reopen_rw &&
> > +        bs->drv->bdrv_need_rw_file_child_during_reopen_rw(bs))
> > +    {
> > +        if (!bdrv_is_writable(bs->file->bs)) {
> > +            ret = bdrv_reopen_set_read_only_drained(bs->file->bs, false, errp);
> 
> Hm.  Sorry, I find this all a bit hard to understand.  (No comments and
> all.)
> 
> I understand that this is for an RO -> RW transition?  Everything is
> still RO, but the parent will need an RW child before it transitions to
> RW itself.
> 
> 
> I’m going to be honest up front, I don’t like this very much.  But I
> think it may be a reasonable solution for now.
> 
> As I remember, the problem was that when reopening a qcow2 node from RO
> to RW, we need to write something in .prepare() (because it can fail),
> but naturally no .prepare() is called after any .commit(), so no matter
> the order of nodes in the ReopenQueue, the child node will never be RW
> by this point.
> 
> Hm.  To me that mostly means that making the whole reopen process a
> transaction was just a dream that turns out not to work.

This patch already looks somewhat complicated to me, and what you're
proposing below sounds another order of magnitude more complex.

But I think the important point is your last sentence above. Once we
acknowledge that we can't possibly maintain full transaction semantics,
I'll ask this naive question: What prevents us from just keeping the
additional write in .commit?

It is expected to work in the common case, so we're only talking about
the behaviour in error cases anyway. If something fails and we can't do
things in a transactionable way, we need to decide what the surprise
result will look like, because we can neither guarantee a rollback nor
successful completion.

If the write fails unexpectedly, and we end up with a qcow2 image that
is opened r/w, but has read-only bitmaps - wouldn't that be a reasonable
result? It seems much easier to explain than some dependency subchain
already being committed and the rest rolled back.

So, I guess my question is, what is the specifc scenario you're trying
to fix with this series (why isn't the final patch a test case that
would answer this question?), and are we sure that the cure isn't worse
than the disease?

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Vladimir Sementsov-Ogievskiy]

02.08.2019 18:42, Kevin Wolf wrote:
> Am 31.07.2019 um 14:09 hat Max Reitz geschrieben:
>> On 25.07.19 11:18, Vladimir Sementsov-Ogievskiy wrote:
>>> On reopen to rw parent may need rw access to child in .prepare, for
>>> example qcow2 needs to write IN_USE flags into stored bitmaps
>>> (currently it is done in a hacky way after commit and don't work).
>>> So, let's introduce such logic.
>>>
>>> The drawback is that in worst case bdrv_reopen_set_read_only may finish
>>> with error and in some intermediate state: some nodes reopened RW and
>>> some are not. But this is a way to fix bug around reopening qcow2
>>> bitmaps in the following commits.
>>
>> This commit message doesn’t really explain what this patch does.
>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   include/block/block_int.h |   2 +
>>>   block.c                   | 144 ++++++++++++++++++++++++++++++++++----
>>>   2 files changed, 133 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>> index 3aa1e832a8..7bd6fd68dd 100644
>>> --- a/include/block/block_int.h
>>> +++ b/include/block/block_int.h
>>> @@ -531,6 +531,8 @@ struct BlockDriver {
>>>                                uint64_t parent_perm, uint64_t parent_shared,
>>>                                uint64_t *nperm, uint64_t *nshared);
>>>   
>>> +     bool (*bdrv_need_rw_file_child_during_reopen_rw)(BlockDriverState *bs);
>>> +
>>>       /**
>>>        * Bitmaps should be marked as 'IN_USE' in the image on reopening image
>>>        * as rw. This handler should realize it. It also should unset readonly
>>> diff --git a/block.c b/block.c
>>> index cbd8da5f3b..3c8e1c59b4 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -1715,10 +1715,12 @@ static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
>>>                                        uint64_t *shared_perm);
>>>   
>>>   typedef struct BlockReopenQueueEntry {
>>> -     bool prepared;
>>> -     bool perms_checked;
>>> -     BDRVReopenState state;
>>> -     QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
>>> +    bool reopened_file_child_rw;
>>> +    bool changed_file_child_perm_rw;
>>> +    bool prepared;
>>> +    bool perms_checked;
>>> +    BDRVReopenState state;
>>> +    QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
>>>   } BlockReopenQueueEntry;
>>>   
>>>   /*
>>> @@ -3421,6 +3423,105 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>>>                                      keep_old_opts);
>>>   }
>>>   
>>> +static int bdrv_reopen_set_read_only_drained(BlockDriverState *bs,
>>> +                                             bool read_only,
>>> +                                             Error **errp)
>>> +{
>>> +    BlockReopenQueue *queue;
>>> +    QDict *opts = qdict_new();
>>> +
>>> +    qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
>>> +
>>> +    queue = bdrv_reopen_queue(NULL, bs, opts, true);
>>> +
>>> +    return bdrv_reopen_multiple(queue, errp);
>>> +}
>>> +
>>> +/*
>>> + * handle_recursive_reopens
>>> + *
>>> + * On fail it needs rollback_recursive_reopens to be called.
>>
>> It would be nice if this description actually said anything about what
>> the function is supposed to do.
>>
>>> + */
>>> +static int handle_recursive_reopens(BlockReopenQueueEntry *current,
>>> +                                    Error **errp)
>>> +{
>>> +    int ret;
>>> +    BlockDriverState *bs = current->state.bs;
>>> +
>>> +    /*
>>> +     * We use the fact that in reopen-queue children are always following
>>> +     * parents.
>>> +     * TODO: Switch BlockReopenQueue to be QTAILQ and use
>>> +     *       QTAILQ_FOREACH_REVERSE.
>>
>> Why don’t you do that first?  It would make the code more obvious at
>> least to me.
>>
>>> +     */
>>> +    if (QSIMPLEQ_NEXT(current, entry)) {
>>> +        ret = handle_recursive_reopens(QSIMPLEQ_NEXT(current, entry), errp);
>>> +        if (ret < 0) {
>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    if ((current->state.flags & BDRV_O_RDWR) && bs->file && bs->drv &&
>>> +        bs->drv->bdrv_need_rw_file_child_during_reopen_rw &&
>>> +        bs->drv->bdrv_need_rw_file_child_during_reopen_rw(bs))
>>> +    {
>>> +        if (!bdrv_is_writable(bs->file->bs)) {
>>> +            ret = bdrv_reopen_set_read_only_drained(bs->file->bs, false, errp);
>>
>> Hm.  Sorry, I find this all a bit hard to understand.  (No comments and
>> all.)
>>
>> I understand that this is for an RO -> RW transition?  Everything is
>> still RO, but the parent will need an RW child before it transitions to
>> RW itself.
>>
>>
>> I’m going to be honest up front, I don’t like this very much.  But I
>> think it may be a reasonable solution for now.
>>
>> As I remember, the problem was that when reopening a qcow2 node from RO
>> to RW, we need to write something in .prepare() (because it can fail),
>> but naturally no .prepare() is called after any .commit(), so no matter
>> the order of nodes in the ReopenQueue, the child node will never be RW
>> by this point.
>>
>> Hm.  To me that mostly means that making the whole reopen process a
>> transaction was just a dream that turns out not to work.
> 
> This patch already looks somewhat complicated to me, and what you're
> proposing below sounds another order of magnitude more complex.

Agree :) However, at this point I've almost implemented it (it's not a reason
to chose more complex way, of course).

> 
> But I think the important point is your last sentence above. Once we
> acknowledge that we can't possibly maintain full transaction semantics,
> I'll ask this naive question: What prevents us from just keeping the
> additional write in .commit?

Hmm, it's what we've started with. The only thing to do is to reverse order
of commits, to commit file child before parent (and this way it works in
Virtuozzo now).
And I proposed it long ago:
https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg04528.html


> 
> It is expected to work in the common case, so we're only talking about
> the behaviour in error cases anyway. If something fails and we can't do
> things in a transactionable way, we need to decide what the surprise
> result will look like, because we can neither guarantee a rollback nor
> successful completion.
> 
> If the write fails unexpectedly, and we end up with a qcow2 image that
> is opened r/w, but has read-only bitmaps - wouldn't that be a reasonable
> result? It seems much easier to explain than some dependency subchain
> already being committed and the rest rolled back.

And this is how it works now (except it doesn't work because of commit order).
In our long ago conversation (link above) you pointed that the problem here
is that we don't return an error from actually failed commit and it is
ignored..

> 
> So, I guess my question is, what is the specifc scenario you're trying
> to fix with this series (why isn't the final patch a test case that
> would answer this question?), and are we sure that the cure isn't worse
> than the disease?
> 

Test appears at 03 and tests what already works, and lacks test-cases which
are broken, and they are added in 08 when all is fixed.

And here are two things to fix:
First is that we lose bitmaps on reopening to RO and it is described and
fixed in 06.
Second is that we cant set IN_USE flag when reopening to RW and it is fixed
finally in 08.


=====

So, if we decide to keep things simple, what to do? Just reorder commits to
satisfy dependencies, if any?

Should we add return code to commit, which should always be 0 except very rare
case?


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

02.08.2019 19:23, Vladimir Sementsov-Ogievskiy wrote:
> 02.08.2019 18:42, Kevin Wolf wrote:
>> Am 31.07.2019 um 14:09 hat Max Reitz geschrieben:
>>> On 25.07.19 11:18, Vladimir Sementsov-Ogievskiy wrote:
>>>> On reopen to rw parent may need rw access to child in .prepare, for
>>>> example qcow2 needs to write IN_USE flags into stored bitmaps
>>>> (currently it is done in a hacky way after commit and don't work).
>>>> So, let's introduce such logic.
>>>>
>>>> The drawback is that in worst case bdrv_reopen_set_read_only may finish
>>>> with error and in some intermediate state: some nodes reopened RW and
>>>> some are not. But this is a way to fix bug around reopening qcow2
>>>> bitmaps in the following commits.
>>>
>>> This commit message doesn’t really explain what this patch does.
>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>   include/block/block_int.h |   2 +
>>>>   block.c                   | 144 ++++++++++++++++++++++++++++++++++----
>>>>   2 files changed, 133 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>>> index 3aa1e832a8..7bd6fd68dd 100644
>>>> --- a/include/block/block_int.h
>>>> +++ b/include/block/block_int.h
>>>> @@ -531,6 +531,8 @@ struct BlockDriver {
>>>>                                uint64_t parent_perm, uint64_t parent_shared,
>>>>                                uint64_t *nperm, uint64_t *nshared);
>>>> +     bool (*bdrv_need_rw_file_child_during_reopen_rw)(BlockDriverState *bs);
>>>> +
>>>>       /**
>>>>        * Bitmaps should be marked as 'IN_USE' in the image on reopening image
>>>>        * as rw. This handler should realize it. It also should unset readonly
>>>> diff --git a/block.c b/block.c
>>>> index cbd8da5f3b..3c8e1c59b4 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -1715,10 +1715,12 @@ static void bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
>>>>                                        uint64_t *shared_perm);
>>>>   typedef struct BlockReopenQueueEntry {
>>>> -     bool prepared;
>>>> -     bool perms_checked;
>>>> -     BDRVReopenState state;
>>>> -     QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
>>>> +    bool reopened_file_child_rw;
>>>> +    bool changed_file_child_perm_rw;
>>>> +    bool prepared;
>>>> +    bool perms_checked;
>>>> +    BDRVReopenState state;
>>>> +    QSIMPLEQ_ENTRY(BlockReopenQueueEntry) entry;
>>>>   } BlockReopenQueueEntry;
>>>>   /*
>>>> @@ -3421,6 +3423,105 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
>>>>                                      keep_old_opts);
>>>>   }
>>>> +static int bdrv_reopen_set_read_only_drained(BlockDriverState *bs,
>>>> +                                             bool read_only,
>>>> +                                             Error **errp)
>>>> +{
>>>> +    BlockReopenQueue *queue;
>>>> +    QDict *opts = qdict_new();
>>>> +
>>>> +    qdict_put_bool(opts, BDRV_OPT_READ_ONLY, read_only);
>>>> +
>>>> +    queue = bdrv_reopen_queue(NULL, bs, opts, true);
>>>> +
>>>> +    return bdrv_reopen_multiple(queue, errp);
>>>> +}
>>>> +
>>>> +/*
>>>> + * handle_recursive_reopens
>>>> + *
>>>> + * On fail it needs rollback_recursive_reopens to be called.
>>>
>>> It would be nice if this description actually said anything about what
>>> the function is supposed to do.
>>>
>>>> + */
>>>> +static int handle_recursive_reopens(BlockReopenQueueEntry *current,
>>>> +                                    Error **errp)
>>>> +{
>>>> +    int ret;
>>>> +    BlockDriverState *bs = current->state.bs;
>>>> +
>>>> +    /*
>>>> +     * We use the fact that in reopen-queue children are always following
>>>> +     * parents.
>>>> +     * TODO: Switch BlockReopenQueue to be QTAILQ and use
>>>> +     *       QTAILQ_FOREACH_REVERSE.
>>>
>>> Why don’t you do that first?  It would make the code more obvious at
>>> least to me.
>>>
>>>> +     */
>>>> +    if (QSIMPLEQ_NEXT(current, entry)) {
>>>> +        ret = handle_recursive_reopens(QSIMPLEQ_NEXT(current, entry), errp);
>>>> +        if (ret < 0) {
>>>> +            return ret;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    if ((current->state.flags & BDRV_O_RDWR) && bs->file && bs->drv &&
>>>> +        bs->drv->bdrv_need_rw_file_child_during_reopen_rw &&
>>>> +        bs->drv->bdrv_need_rw_file_child_during_reopen_rw(bs))
>>>> +    {
>>>> +        if (!bdrv_is_writable(bs->file->bs)) {
>>>> +            ret = bdrv_reopen_set_read_only_drained(bs->file->bs, false, errp);
>>>
>>> Hm.  Sorry, I find this all a bit hard to understand.  (No comments and
>>> all.)
>>>
>>> I understand that this is for an RO -> RW transition?  Everything is
>>> still RO, but the parent will need an RW child before it transitions to
>>> RW itself.
>>>
>>>
>>> I’m going to be honest up front, I don’t like this very much.  But I
>>> think it may be a reasonable solution for now.
>>>
>>> As I remember, the problem was that when reopening a qcow2 node from RO
>>> to RW, we need to write something in .prepare() (because it can fail),
>>> but naturally no .prepare() is called after any .commit(), so no matter
>>> the order of nodes in the ReopenQueue, the child node will never be RW
>>> by this point.
>>>
>>> Hm.  To me that mostly means that making the whole reopen process a
>>> transaction was just a dream that turns out not to work.
>>
>> This patch already looks somewhat complicated to me, and what you're
>> proposing below sounds another order of magnitude more complex.
> 
> Agree :) However, at this point I've almost implemented it (it's not a reason
> to chose more complex way, of course).
> 
>>
>> But I think the important point is your last sentence above. Once we
>> acknowledge that we can't possibly maintain full transaction semantics,
>> I'll ask this naive question: What prevents us from just keeping the
>> additional write in .commit?
> 
> Hmm, it's what we've started with. The only thing to do is to reverse order
> of commits, to commit file child before parent (and this way it works in
> Virtuozzo now).
> And I proposed it long ago:
> https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg04528.html
> 
> 
>>
>> It is expected to work in the common case, so we're only talking about
>> the behaviour in error cases anyway. If something fails and we can't do
>> things in a transactionable way, we need to decide what the surprise
>> result will look like, because we can neither guarantee a rollback nor
>> successful completion.
>>
>> If the write fails unexpectedly, and we end up with a qcow2 image that
>> is opened r/w, but has read-only bitmaps - wouldn't that be a reasonable
>> result? It seems much easier to explain than some dependency subchain
>> already being committed and the rest rolled back.
> 
> And this is how it works now (except it doesn't work because of commit order).
> In our long ago conversation (link above) you pointed that the problem here
> is that we don't return an error from actually failed commit and it is
> ignored..
> 
>>
>> So, I guess my question is, what is the specifc scenario you're trying
>> to fix with this series (why isn't the final patch a test case that
>> would answer this question?), and are we sure that the cure isn't worse
>> than the disease?
>>
> 
> Test appears at 03 and tests what already works, and lacks test-cases which
> are broken, and they are added in 08 when all is fixed.
> 
> And here are two things to fix:
> First is that we lose bitmaps on reopening to RO and it is described and
> fixed in 06.
> Second is that we cant set IN_USE flag when reopening to RW and it is fixed
> finally in 08.
> 
> 
> =====
> 
> So, if we decide to keep things simple, what to do? Just reorder commits to
> satisfy dependencies, if any?
> 
> Should we add return code to commit, which should always be 0 except very rare
> case?
> 
> 


And another idea is to postpone IN_USE setting until we really need to modify the
bitmap.. Not sure that it is simpler.

-- 
Best regards,
Vladimir