From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33857) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFJM6-0008IU-EY for qemu-devel@nongnu.org; Tue, 21 Jun 2016 06:56:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bFJM4-0004Y5-DB for qemu-devel@nongnu.org; Tue, 21 Jun 2016 06:56:29 -0400 Date: Tue, 21 Jun 2016 12:56:20 +0200 From: Kevin Wolf Message-ID: <20160621105620.GD4520@noname.redhat.com> References: <1466500894-9710-1-git-send-email-kwolf@redhat.com> <3f872c08-06d8-d755-9369-02ecd0d6d000@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3f872c08-06d8-d755-9369-02ecd0d6d000@redhat.com> Subject: Re: [Qemu-devel] [PATCH 00/17] block: Convert common I/O path to BdrvChild List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-block@nongnu.org, famz@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org, mreitz@redhat.com, stefanha@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