On 10.06.2015 14:09, Kevin Wolf wrote:
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 <kw...@redhat.com>
@@ -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?
My point wasn't about switching the extent BDS, but the VMDK BDS itself,
which has multiple extent BDSs as children.
My mistake was somehow assuming this list of children would not be moved
during bdrv_swap() (what I meant by "to leave the extents behind"), but
of course it is. So on bdrv_swap(), all the children get swapped, too
(which is correct: protocol and backing BDS, as well as all BDSs
referred to through the opaque structure, are indeed swapped).
So, no objections remaining.
Max
