[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 <address@hidden>
---
 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