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

[Benoît Canet]

On Fri, Sep 19, 2014 at 12:35:36AM -0400, Jeff Cody wrote:

> 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 for the reply Jeff.

I think I'll drop the entry I CCed you, remove the assert then work to post
the new recursive blocker code.

Best regards

Benoît

> 
> Thanks,
> Jeff
>