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

[Max Reitz <mreitz@redhat.com>]

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