Re: [Qemu-devel] [PATCH 0/2] Handle interferences between multiple children drivers and stream and commit

[Jeff Cody <jcody@redhat.com>]

On Mon, Sep 15, 2014 at 04:21:18PM +0200, Benoît Canet wrote:
> We do not want to try to stream or commit with a base argument through
> a multiple children driver.
> 
> Handle this case.
> 
> Benoît Canet (2):
>   block: Introduce a BlockDriver field to flag drivers supporting
>     multiple children
>   block: commit and stream return an error when a subtree is found
> 
>  block.c                   | 17 +++++++++++++++--
>  block/blkverify.c         |  1 +
>  block/quorum.c            |  1 +
>  block/vmdk.c              |  1 +
>  blockdev.c                | 18 ++++++++++++------
>  include/block/block.h     |  3 ++-
>  include/block/block_int.h |  6 ++++++
>  7 files changed, 38 insertions(+), 9 deletions(-)
> 
> -- 
> 2.1.0
>


I had some specific comments on the implementation, but once I stepped
back some I am not sure this series is needed.

When we talked on irc, you mentioned it was needed because your
recursive blocker code would assert in quorum/blkverify/vmdk - but I
think that might mean the recursive code needs refactoring.

Here is why I am not sure this is needed: we should be able to commit
through/to an image that has multiple child nodes, if we can treat
that image as an actual BDS.

I view these as a black box, like so:

               (imageE)
             ------------
             | [childA] |
             | [childB] |
[base] <---- |          | <----- [imageF] <--- [active]
             | [childC] |
 ^           | [childD] |
 |           ------------
 |
 |
(Perhaps base isn't support by the format, in which case it will always
be NULL anyway)


ImageE may have multiple children (and perhaps each of those children
are an entire chain themselves), but ultimately imageE is a 'BDS'
entry.

Perhaps the format, like quorum, will not have a backing_hd pointer.
But in that case, backing_hd will be NULL.

Thinking about your recursive blocker code, why assert when the 'base'
termination argument is NULL?  If the multi-child format does not
support a backing_hd, then bdrv_find_base_image() will just find it as
the end.

The .bdrv_op_recursive_block() in that case could still navigate down
each private child node, because if the original 'base' blocker
applied to the multi-child parent (imageE), then that same blocker
should apply to each private child (and their children), regardless of
'base' - because they are essentially part of the pseudo-atomic entity
of (imageE).

Does this make sense, or am I missing a piece?

Thanks,
Jeff