Re: [Qemu-devel] [PATCH 00/17] block: Convert common I/O path to BdrvChild

[Kevin Wolf <kwolf@redhat.com>]

Am 21.06.2016 um 11:47 hat Paolo Bonzini geschrieben:
> 
> 
> On 21/06/2016 11:21, Kevin Wolf wrote:
> > This series converts all I/O function in the core block layer up to
> > bdrv_co_preadv/pwritev() to taking a BdrvChild as their first parameter
> > instead of a BlockDriverState.
> > 
> > The original motivation for this change were op blockers, where one of
> > the biggest problems is making sure that every user of block devices
> > actually registers correctly with the op blockers system. If the I/O
> > functions know which parent a request comes from (BdrvChild basically
> > corresponds to an edge in our block device graph), it can use assertions
> > to make sure that that parent has actually registered its activities and
> > thereby ensured that it doesn't conflict with other users.
> > 
> > There are, however, more benefits we get from this change. The most
> > important one is probably that it enforces important aspects of the
> > block layer design like that external users go through a BlockBackend
> > and request are internally routed along the edges of the graph. Accesses
> > to random BDSes are no longer possible, you need to own an actual child
> > reference so you can make a request.
> 
> I still fail to understand what is the rationale for this change.  The
> API is weird; you read from a disk, not from an edge, and in fact the
> first thing all the APIs do is dereference the BdrvChild...
> 
> The assertions are nice, but I'm not sure it's a good idea to design a
> whole API around them.

Do you see a problem with such an API, though? If there is no reason not
to have the advantages, as small as they may seem, why not take them?

(By the way, I disagree that assertions for op blockers are just "nice
to have". I would never be confident that we covered everything without
them.)

Apart from that, I actually think the conversion has already done its
most important job, which is uncovering the places where we employed
hacks that violate our design. With this series compiling and working,
I'm even confident to say that everything that should be using
BlockBackend, does so by now (now = with the series applied).

But now that the patches exist, even if you dismiss the value of op
blocker assertions, applying them in order to keep us honest to the
design even in the future seems appealing to me.

Well, unless you show me an example of something that makes sense, is
compatible with the block layer design and would still be incorrectly
blocked by the requirement to have a BdrvChild. Then I might change my
opinion.

Kevin