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.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 5ba3435f8f..e9181c3be7 100644
--- a/block.c
+++ b/block.c
@@ -4612,6 +4612,22 @@ void bdrv_invalidate_cache_all(Error **errp)
     }
 }
 
+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,
                                    bool setting_flag)
 {
@@ -4622,6 +4638,14 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
         return -ENOMEDIUM;
     }
 
+    /* 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));
+
     if (!setting_flag && bs->drv->bdrv_inactivate) {
         ret = bs->drv->bdrv_inactivate(bs);
         if (ret < 0) {
@@ -4629,7 +4653,7 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
         }
     }
 
-    if (setting_flag && !(bs->open_flags & BDRV_O_INACTIVE)) {
+    if (setting_flag) {
         uint64_t perm, shared_perm;
 
         QLIST_FOREACH(parent, &bs->parents, next_parent) {
@@ -4682,6 +4706,12 @@ int bdrv_inactivate_all(void)
      * is allowed. */
     for (pass = 0; pass < 2; pass++) {
         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, pass);
             if (ret < 0) {
                 bdrv_next_cleanup(&it);
-- 
2.19.1

On 26.11.18 15:16, Kevin Wolf wrote:
> 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.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index 5ba3435f8f..e9181c3be7 100644
> --- a/block.c
> +++ b/block.c

[...]

> @@ -4622,6 +4638,14 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
>          return -ENOMEDIUM;
>      }
>  
> +    /* 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)) {

Overall the patch looks good to me, apart from the fact that I still
think this should only be checked if setting_flag is true.

(Because BDRV_O_INACTIVE is only set during the second pass, so this
check will lead to the first pass not doing anything (and thus not
calling bs->drv->bdrv_inactivate()) on any non-root bs.)

Max

> +        return 0;
> +    }
> +
> +    assert(!(bs->open_flags & BDRV_O_INACTIVE));
> +
>      if (!setting_flag && bs->drv->bdrv_inactivate) {
>          ret = bs->drv->bdrv_inactivate(bs);
>          if (ret < 0) {