[Qemu-devel] [PATCH 0/1] A couple of problems with BlockDriverState's children list

[Alberto Garcia <berto@igalia.com>]

I've been debugging a couple of problems related to the recently
merged bdrv_reopen() overhaul code.

1. bs->children is not updated correctly
----------------------------------------
The problem is described in this e-mail:

   https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg06813.html

In short, changing an image's backing hd does not replace the pointer
in the bs->children list.

The children list is a feature added recently in 6e93e7c41fdfdee30.
In addition to bs->backing_hd and bs->file it also includes other
driver-specific children for cases like Quorum.

However there's no way to remove items from that list. It seems that
this was discussed when the patch was first published, but no one saw
a case where this could break:

   https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02284.html

The problem is that it can break: the block-stream command removes a
BDS's backing image (optionally replacing it with a new one), so the
pointer in bs->children becomes invalid.

I wrote a patch that updates bs->children when bdrv_set_backing_hd()
is called. It also makes sure that the same children is not added
twice to the same parent (that can happen due to bdrv_set_backing_hd()
being called in bdrv_open_backing_file()).

I think this is enough to solve this problem, but I haven't checked
all other cases of chidren added using bdrv_attach_child(). Anyway the
assumption that once a BDS is added to that list it will always be
valid seems very broad to me.


2. bdrv_reopen_queue() includes the complete backing chain
----------------------------------------------------------
Calling bdrv_reopen() on a particular BlockDriverState now adds its
whole backing chain to the queue (formerly I think it was just
bs->file).

I don't know why this behavior is necessary, but there are surely
things that I'm overlooking.

However this breaks one of the features of my intermediate block
streaming patchset: the ability to start several block-stream
operations in parallel as long as the affected chains don't overlap.

This breaks iotest 030, as described here:

   https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg06273.html

Now, this feature was just a nice side effect of the ability to stream
to intermediate images, and is of secondary importance to me; so if I
can no longer assume that bdrv_reopen() is not going to touch the
whole backing chain, I can just remove it very easily and still leave
the main functionality intact.

Comments are welcome.

Thanks,

Berto

Alberto Garcia (1):
  block: update BlockDriverState's children in bdrv_set_backing_hd()

 block.c | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

-- 
2.1.4