Re: [Qemu-devel] [PATCH 08/34] block: Add list of children to BlockDriverState

[Kevin Wolf]

Am 11.05.2015 um 17:45 hat Max Reitz geschrieben:
> On 08.05.2015 19:21, Kevin Wolf wrote:
> >This allows iterating over all children of a given BDS, not only
> >including bs->file and bs->backing_hd, but also driver-specific
> >ones like VMDK extents or Quorum children.
> >
> >Signed-off-by: Kevin Wolf <address@hidden>
> >---
> >  block.c                   | 27 +++++++++++++++++++++++++++
> >  include/block/block_int.h |  8 ++++++++
> >  2 files changed, 35 insertions(+)
> >
> >diff --git a/block.c b/block.c
> >index c4f0fb4..59f54ed 100644
> >--- a/block.c
> >+++ b/block.c
> >@@ -1301,6 +1301,19 @@ out:
> >      return ret;
> >  }
> >+static void bdrv_attach_child(BlockDriverState *parent_bs,
> >+                              BlockDriverState *child_bs,
> >+                              const BdrvChildRole *child_role)
> >+{
> >+    BdrvChild *child = g_new(BdrvChild, 1);
> >+    *child = (BdrvChild) {
> >+        .bs     = child_bs,
> >+        .role   = child_role,
> >+    };
> >+
> >+    QLIST_INSERT_HEAD(&parent_bs->children, child, next);
> >+}
> >+
> >  /*
> >   * Opens a disk image (raw, qcow2, vmdk, ...)
> >   *
> >@@ -1353,6 +1366,9 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> >const char *filename,
> >              return -ENODEV;
> >          }
> >          bdrv_ref(bs);
> >+        if (child_role) {
> >+            bdrv_attach_child(parent, bs, child_role);
> >+        }
> >          *pbs = bs;
> >          return 0;
> >      }
> >@@ -1495,6 +1511,10 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> >const char *filename,
> >          goto close_and_fail;
> >      }
> >+    if (child_role) {
> >+        bdrv_attach_child(parent, bs, child_role);
> >+    }
> >+
> >      QDECREF(options);
> >      *pbs = bs;
> >      return 0;
> >@@ -1789,6 +1809,12 @@ void bdrv_close(BlockDriverState *bs)
> >      notifier_list_notify(&bs->close_notifiers, bs);
> >      if (bs->drv) {
> >+        BdrvChild *child, *next;
> >+
> >+        QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
> >+            g_free(child);
> >+        }
> >+
> 
> Not considering the case where the child is closed before the parent
> assumes all children are reference-counted from the parent and they
> won't be closed (and maybe replaced with another BDS) on purpose.
> The first seems reasonable, the second one I'm not so sure about. It
> works for now, but I could imagine that we want to modify children
> of a Quorum instance at runtime.
> 
> But I can't imagine any case where this would break right now, so I
> guess I'm fine with it.

We don't have that yet, but I suppose to remove a child you would modify
the Quorum node to drop the child reference, which should at the same
time remove it from the children list.

What we currently can do (I think) is replacing a node with another
node. In that case, I thought bdrv_swap() would do the right thing, but
maybe it doesn't.

> >          if (bs->backing_hd) {
> >              BlockDriverState *backing_hd = bs->backing_hd;
> >              bdrv_set_backing_hd(bs, NULL);
> >@@ -1999,6 +2025,7 @@ void bdrv_append(BlockDriverState *bs_new, 
> >BlockDriverState *bs_top)
> >      /* The contents of 'tmp' will become bs_top, as we are
> >       * swapping bs_new and bs_top contents. */
> >      bdrv_set_backing_hd(bs_top, bs_new);
> >+    bdrv_attach_child(bs_top, bs_new, &child_backing);
> >  }
> >  static void bdrv_delete(BlockDriverState *bs)
> 
> Using a mirror block job, we can force bdrv_swap() on arbitrary
> nodes, right? What happens if you swap e.g. a VMDK and a quorum
> node? Well, maybe one simply cannot swap a quorum node due to
> blockers, but I guess one can swap a VMDK node with some non-VMDK
> node. It is actually correct to leave the extents behind; but the
> other node cannot do anything with them, so because they are part of
> the opaque VMDK structure, they will de-facto remain with VMDK,
> while being counted as children of the other node. But I try to keep
> so far away from bdrv_swap() that I don't even know whether this
> case is even possible.

I suspect that instead of doing bdrv_attach_child() here, the child list
must be handled in bdrv_move_feature_fields(), so that the swapped BDSes
effectively swap their roles.

bs->inherits_from (next patch) might be similar. I'm not completely sure
yet what the ideal behaviour would be there. Or perhaps just set it to
NULL for both swapped BDSes.

Kevin