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
