From ff19fad07e21bac2534edd867120645fa2a0d7d3 Mon Sep 17 00:00:00 2001

From: Kevin Wolf <kwolf@redhat.com>

Date: Fri, 14 Dec 2018 09:49:45 +0000

Subject: [PATCH 1/2] block: Don't inactivate children before parents

RH-Author: Kevin Wolf <kwolf@redhat.com>

Message-id: <20181214094946.26226-2-kwolf@redhat.com>

Patchwork-id: 83511

O-Subject: [RHEL-8.0 qemu-kvm PATCH 1/2] block: Don't inactivate children before parents

Bugzilla: 1659395

RH-Acked-by: Laurent Vivier <lvivier@redhat.com>

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

RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>

bdrv_child_cb_inactivate() asserts that parents are already inactive

when children get inactivated. This precondition is necessary because

parents could still issue requests in their inactivation code.

When block nodes are created individually with -blockdev, all of them

are monitor owned and will be returned by bdrv_next() in an undefined

order (in practice, in the order of their creation, which is usually

children before parents), which obviously fails the assertion:

qemu: block.c:899: bdrv_child_cb_inactivate: Assertion `bs->open_flags & BDRV_O_INACTIVE' failed.

This patch fixes the ordering by skipping nodes with still active

parents in bdrv_inactivate_recurse() because we know that they will be

covered by recursion when the last active parent becomes inactive.

With the correct parents-before-children ordering, we also got rid of

the reason why commit aad0b7a0bfb introduced two passes, so we can go

back to a single-pass recursion. This is necessary so we can rely on the

BDRV_O_INACTIVE flag to skip nodes with active parents (the flag used

to be set only in pass 2, so we would always skip non-root nodes in

pass 1 because all parents would still be considered active; setting the

flag in pass 1 would mean, that we never skip anything in pass 2 because

all parents are already considered inactive).

Because of the change to single pass, this patch is best reviewed with

whitespace changes ignored.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

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

(cherry picked from commit 9e37271f50ec2e95f299dc297ac08f9be0096b48)

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Signed-off-by: Danilo C. L. de Paula <ddepaula@redhat.com>

---

 block.c | 84 +++++++++++++++++++++++++++++++++++++++++------------------------

 1 file changed, 53 insertions(+), 31 deletions(-)

diff --git a/block.c b/block.c

index 18ae591..73f55a1 100644

--- a/block.c

+++ b/block.c

@@ -4442,45 +4442,68 @@ void bdrv_invalidate_cache_all(Error **errp)

     }

 }

-static int bdrv_inactivate_recurse(BlockDriverState *bs,

-                                   bool setting_flag)

+static bool bdrv_has_bds_parent(BlockDriverState *bs, bool only_active)

+{

+    BdrvChild *parent;

+

+    QLIST_FOREACH(parent, &bs->parents, next_parent) {

+        if (parent->role->parent_is_bds) {

+            BlockDriverState *parent_bs = parent->opaque;

+            if (!only_active || !(parent_bs->open_flags & BDRV_O_INACTIVE)) {

+                return true;

+            }

+        }

+    }

+

+    return false;

+}

+

+static int bdrv_inactivate_recurse(BlockDriverState *bs)

 {

     BdrvChild *child, *parent;

+    uint64_t perm, shared_perm;

     int ret;

     if (!bs->drv) {

         return -ENOMEDIUM;

     }

-    if (!setting_flag && bs->drv->bdrv_inactivate) {

+    /* Make sure that we don't inactivate a child before its parent.

+     * It will be covered by recursion from the yet active parent. */

+    if (bdrv_has_bds_parent(bs, true)) {

+        return 0;

+    }

+

+    assert(!(bs->open_flags & BDRV_O_INACTIVE));

+

+    /* Inactivate this node */

+    if (bs->drv->bdrv_inactivate) {

         ret = bs->drv->bdrv_inactivate(bs);

         if (ret < 0) {

             return ret;

         }

     }

-    if (setting_flag && !(bs->open_flags & BDRV_O_INACTIVE)) {

-        uint64_t perm, shared_perm;

-

-        QLIST_FOREACH(parent, &bs->parents, next_parent) {

-            if (parent->role->inactivate) {

-                ret = parent->role->inactivate(parent);

-                if (ret < 0) {

-                    return ret;

-                }

+    QLIST_FOREACH(parent, &bs->parents, next_parent) {

+        if (parent->role->inactivate) {

+            ret = parent->role->inactivate(parent);

+            if (ret < 0) {

+                return ret;

             }

         }

+    }

-        bs->open_flags |= BDRV_O_INACTIVE;

+    bs->open_flags |= BDRV_O_INACTIVE;

+

+    /* Update permissions, they may differ for inactive nodes */

+    bdrv_get_cumulative_perm(bs, &perm, &shared_perm);

+    bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &error_abort);

+    bdrv_set_perm(bs, perm, shared_perm);

-        /* Update permissions, they may differ for inactive nodes */

-        bdrv_get_cumulative_perm(bs, &perm, &shared_perm);

-        bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, &error_abort);

-        bdrv_set_perm(bs, perm, shared_perm);

-    }

+    /* Recursively inactivate children */

     QLIST_FOREACH(child, &bs->children, next) {

-        ret = bdrv_inactivate_recurse(child->bs, setting_flag);

+        ret = bdrv_inactivate_recurse(child->bs);

         if (ret < 0) {

             return ret;

         }

@@ -4494,7 +4517,6 @@ int bdrv_inactivate_all(void)

     BlockDriverState *bs = NULL;

     BdrvNextIterator it;

     int ret = 0;

-    int pass;

     GSList *aio_ctxs = NULL, *ctx;

     for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {

@@ -4506,17 +4528,17 @@ int bdrv_inactivate_all(void)

         }

     }

-    /* We do two passes of inactivation. The first pass calls to drivers'

-     * .bdrv_inactivate callbacks recursively so all cache is flushed to disk;

-     * the second pass sets the BDRV_O_INACTIVE flag so that no further write

-     * is allowed. */

-    for (pass = 0; pass < 2; pass++) {

-        for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {

-            ret = bdrv_inactivate_recurse(bs, pass);

-            if (ret < 0) {

-                bdrv_next_cleanup(&it);

-                goto out;

-            }

+    for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {

+        /* Nodes with BDS parents are covered by recursion from the last

+         * parent that gets inactivated. Don't inactivate them a second

+         * time if that has already happened. */

+        if (bdrv_has_bds_parent(bs, false)) {

+            continue;

+        }

+        ret = bdrv_inactivate_recurse(bs);

+        if (ret < 0) {

+            bdrv_next_cleanup(&it);

+            goto out;

         }

     }

-- 

1.8.3.1