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>

> >@@ -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.
> 
> >          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.

While trying to fix up this part of the series so I can send out a v2,
I noticed that I'm not sure what exactly you mean.

You are aware that bdrv_swap() doesn't change any pointers between
BDSes, but just swaps the _contents_ of them, right? That is, the extent
BDS is changed under the feet of VMDK, without it having code for it.
This is what makes bdrv_swap() work at all, and also what makes it so
ugly.

Do you think that there is still something left that would refer from
VMDK to the removed extent BDS? Where would that pointer come from?

Kevin