yeahuh / rpms / qemu-kvm

Forked from rpms/qemu-kvm 2 years ago
Clone

Blame SOURCES/kvm-block-Don-t-inactivate-children-before-parents.patch

26ba25
From ff19fad07e21bac2534edd867120645fa2a0d7d3 Mon Sep 17 00:00:00 2001
26ba25
From: Kevin Wolf <kwolf@redhat.com>
26ba25
Date: Fri, 14 Dec 2018 09:49:45 +0000
26ba25
Subject: [PATCH 1/2] block: Don't inactivate children before parents
26ba25
26ba25
RH-Author: Kevin Wolf <kwolf@redhat.com>
26ba25
Message-id: <20181214094946.26226-2-kwolf@redhat.com>
26ba25
Patchwork-id: 83511
26ba25
O-Subject: [RHEL-8.0 qemu-kvm PATCH 1/2] block: Don't inactivate children before parents
26ba25
Bugzilla: 1659395
26ba25
RH-Acked-by: Laurent Vivier <lvivier@redhat.com>
26ba25
RH-Acked-by: Max Reitz <mreitz@redhat.com>
26ba25
RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
26ba25
26ba25
bdrv_child_cb_inactivate() asserts that parents are already inactive
26ba25
when children get inactivated. This precondition is necessary because
26ba25
parents could still issue requests in their inactivation code.
26ba25
26ba25
When block nodes are created individually with -blockdev, all of them
26ba25
are monitor owned and will be returned by bdrv_next() in an undefined
26ba25
order (in practice, in the order of their creation, which is usually
26ba25
children before parents), which obviously fails the assertion:
26ba25
26ba25
qemu: block.c:899: bdrv_child_cb_inactivate: Assertion `bs->open_flags & BDRV_O_INACTIVE' failed.
26ba25
26ba25
This patch fixes the ordering by skipping nodes with still active
26ba25
parents in bdrv_inactivate_recurse() because we know that they will be
26ba25
covered by recursion when the last active parent becomes inactive.
26ba25
26ba25
With the correct parents-before-children ordering, we also got rid of
26ba25
the reason why commit aad0b7a0bfb introduced two passes, so we can go
26ba25
back to a single-pass recursion. This is necessary so we can rely on the
26ba25
BDRV_O_INACTIVE flag to skip nodes with active parents (the flag used
26ba25
to be set only in pass 2, so we would always skip non-root nodes in
26ba25
pass 1 because all parents would still be considered active; setting the
26ba25
flag in pass 1 would mean, that we never skip anything in pass 2 because
26ba25
all parents are already considered inactive).
26ba25
26ba25
Because of the change to single pass, this patch is best reviewed with
26ba25
whitespace changes ignored.
26ba25
26ba25
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
26ba25
Reviewed-by: Max Reitz <mreitz@redhat.com>
26ba25
(cherry picked from commit 9e37271f50ec2e95f299dc297ac08f9be0096b48)
26ba25
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
26ba25
Signed-off-by: Danilo C. L. de Paula <ddepaula@redhat.com>
26ba25
---
26ba25
 block.c | 84 +++++++++++++++++++++++++++++++++++++++++------------------------
26ba25
 1 file changed, 53 insertions(+), 31 deletions(-)
26ba25
26ba25
diff --git a/block.c b/block.c
26ba25
index 18ae591..73f55a1 100644
26ba25
--- a/block.c
26ba25
+++ b/block.c
26ba25
@@ -4442,45 +4442,68 @@ void bdrv_invalidate_cache_all(Error **errp)
26ba25
     }
26ba25
 }
26ba25
 
26ba25
-static int bdrv_inactivate_recurse(BlockDriverState *bs,
26ba25
-                                   bool setting_flag)
26ba25
+static bool bdrv_has_bds_parent(BlockDriverState *bs, bool only_active)
26ba25
+{
26ba25
+    BdrvChild *parent;
26ba25
+
26ba25
+    QLIST_FOREACH(parent, &bs->parents, next_parent) {
26ba25
+        if (parent->role->parent_is_bds) {
26ba25
+            BlockDriverState *parent_bs = parent->opaque;
26ba25
+            if (!only_active || !(parent_bs->open_flags & BDRV_O_INACTIVE)) {
26ba25
+                return true;
26ba25
+            }
26ba25
+        }
26ba25
+    }
26ba25
+
26ba25
+    return false;
26ba25
+}
26ba25
+
26ba25
+static int bdrv_inactivate_recurse(BlockDriverState *bs)
26ba25
 {
26ba25
     BdrvChild *child, *parent;
26ba25
+    uint64_t perm, shared_perm;
26ba25
     int ret;
26ba25
 
26ba25
     if (!bs->drv) {
26ba25
         return -ENOMEDIUM;
26ba25
     }
26ba25
 
26ba25
-    if (!setting_flag && bs->drv->bdrv_inactivate) {
26ba25
+    /* Make sure that we don't inactivate a child before its parent.
26ba25
+     * It will be covered by recursion from the yet active parent. */
26ba25
+    if (bdrv_has_bds_parent(bs, true)) {
26ba25
+        return 0;
26ba25
+    }
26ba25
+
26ba25
+    assert(!(bs->open_flags & BDRV_O_INACTIVE));
26ba25
+
26ba25
+    /* Inactivate this node */
26ba25
+    if (bs->drv->bdrv_inactivate) {
26ba25
         ret = bs->drv->bdrv_inactivate(bs);
26ba25
         if (ret < 0) {
26ba25
             return ret;
26ba25
         }
26ba25
     }
26ba25
 
26ba25
-    if (setting_flag && !(bs->open_flags & BDRV_O_INACTIVE)) {
26ba25
-        uint64_t perm, shared_perm;
26ba25
-
26ba25
-        QLIST_FOREACH(parent, &bs->parents, next_parent) {
26ba25
-            if (parent->role->inactivate) {
26ba25
-                ret = parent->role->inactivate(parent);
26ba25
-                if (ret < 0) {
26ba25
-                    return ret;
26ba25
-                }
26ba25
+    QLIST_FOREACH(parent, &bs->parents, next_parent) {
26ba25
+        if (parent->role->inactivate) {
26ba25
+            ret = parent->role->inactivate(parent);
26ba25
+            if (ret < 0) {
26ba25
+                return ret;
26ba25
             }
26ba25
         }
26ba25
+    }
26ba25
 
26ba25
-        bs->open_flags |= BDRV_O_INACTIVE;
26ba25
+    bs->open_flags |= BDRV_O_INACTIVE;
26ba25
+
26ba25
+    /* Update permissions, they may differ for inactive nodes */
26ba25
+    bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
26ba25
+    bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &error_abort);
26ba25
+    bdrv_set_perm(bs, perm, shared_perm);
26ba25
 
26ba25
-        /* Update permissions, they may differ for inactive nodes */
26ba25
-        bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
26ba25
-        bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &error_abort);
26ba25
-        bdrv_set_perm(bs, perm, shared_perm);
26ba25
-    }
26ba25
 
26ba25
+    /* Recursively inactivate children */
26ba25
     QLIST_FOREACH(child, &bs->children, next) {
26ba25
-        ret = bdrv_inactivate_recurse(child->bs, setting_flag);
26ba25
+        ret = bdrv_inactivate_recurse(child->bs);
26ba25
         if (ret < 0) {
26ba25
             return ret;
26ba25
         }
26ba25
@@ -4494,7 +4517,6 @@ int bdrv_inactivate_all(void)
26ba25
     BlockDriverState *bs = NULL;
26ba25
     BdrvNextIterator it;
26ba25
     int ret = 0;
26ba25
-    int pass;
26ba25
     GSList *aio_ctxs = NULL, *ctx;
26ba25
 
26ba25
     for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
26ba25
@@ -4506,17 +4528,17 @@ int bdrv_inactivate_all(void)
26ba25
         }
26ba25
     }
26ba25
 
26ba25
-    /* We do two passes of inactivation. The first pass calls to drivers'
26ba25
-     * .bdrv_inactivate callbacks recursively so all cache is flushed to disk;
26ba25
-     * the second pass sets the BDRV_O_INACTIVE flag so that no further write
26ba25
-     * is allowed. */
26ba25
-    for (pass = 0; pass < 2; pass++) {
26ba25
-        for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
26ba25
-            ret = bdrv_inactivate_recurse(bs, pass);
26ba25
-            if (ret < 0) {
26ba25
-                bdrv_next_cleanup(&it);
26ba25
-                goto out;
26ba25
-            }
26ba25
+    for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
26ba25
+        /* Nodes with BDS parents are covered by recursion from the last
26ba25
+         * parent that gets inactivated. Don't inactivate them a second
26ba25
+         * time if that has already happened. */
26ba25
+        if (bdrv_has_bds_parent(bs, false)) {
26ba25
+            continue;
26ba25
+        }
26ba25
+        ret = bdrv_inactivate_recurse(bs);
26ba25
+        if (ret < 0) {
26ba25
+            bdrv_next_cleanup(&it);
26ba25
+            goto out;
26ba25
         }
26ba25
     }
26ba25
 
26ba25
-- 
26ba25
1.8.3.1
26ba25