From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:47985) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rzok7-00076V-1T for qemu-devel@nongnu.org; Tue, 21 Feb 2012 07:22:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rzojz-0001Yl-1t for qemu-devel@nongnu.org; Tue, 21 Feb 2012 07:22:50 -0500 Received: from mail-lpp01m010-f45.google.com ([209.85.215.45]:52281) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rzojy-0001YS-Kq for qemu-devel@nongnu.org; Tue, 21 Feb 2012 07:22:43 -0500 Received: by lahi5 with SMTP id i5so8194602lah.4 for ; Tue, 21 Feb 2012 04:22:40 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <4F4381CE.70604@redhat.com> References: <73865e0ce364c40e0eb65ec6b22b819d@mail.gmail.com> <4F31153E.9010205@codemonkey.ws> <4F311839.9030709@redhat.com> <4F311BBA.8000708@codemonkey.ws> <4F312FD3.5020206@zerto.com> <4F3137DB.1040503@redhat.com> <4F3139CE.4040103@zerto.com> <4F314798.8010009@redhat.com> <4F3211D0.3070502@zerto.com> <4F323875.1000000@redhat.com> <4F3244C2.1040604@zerto.com> <4F32489A.80307@redhat.com> <4F32788C.60904@zerto.com> <4F40FBD6.2000500@zerto.com> <4F425987.20103@redhat.com> <4F435DD2.8080600@redhat.com> <4F4360C7.5080806@redhat.com> <4F4368BF.4040707@redhat.com> <4F436D76.6090206@redhat.com> <4F43773B.6060109@redhat.com> <4F4381CE.70604@redhat.com> Date: Tue, 21 Feb 2012 12:22:40 +0000 Message-ID: From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] BlockDriverState stack and BlockListeners List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Kevin Wolf , =?UTF-8?B?16rXldee16gg15HXnyDXkNeV16g=?= , =?UTF-8?B?16LXldeT15Mg16fXk9ed?= , Jeff Cody , dlaor@redhat.com, qemu-devel@nongnu.org, Markus Armbruster , Zhi Yong Wu , Federico Simoncelli , Ori Mamluk , Yair Kuszpet On Tue, Feb 21, 2012 at 11:36 AM, Paolo Bonzini wrote= : > On 02/21/2012 11:51 AM, Kevin Wolf wrote: >> And even protocols and protocols don't. Compare file to blkdebug, for >> example. In fact, blkdebug and blkverify are already very close to what >> BlockListeners would be. > > Yes, and I think considering blkdebug and blkverify help in the design. > They provide the difference between views and listeners: listeners can > be applied to both a protocol and a view, while views can only be > applied to a format. > >> > - Protocols -- Protocols tell you where to get the raw bits. >> > - Formats -- Formats transform those raw bits into a block device. >> > - Views -- Views can move from a format to another. =A0A format can us= e a >> > default view implementation, or provide its own (e.g. to access >> > different snapshots). >> > - Listeners -- I think a view can have-a listener? >> >> Where protocols, formats and listeners are-a image (Not the best name, >> I'm open for suggestions. Something like "BDS stack building block" >> would be most accurate...). Or actually not is-a in terms of >> inheritance, but I think it would be the very same thing without any >> subclassing, implemented by a BlockDriver. >> >> > with the following relationship: >> > >> > - A format has-a protocol for the raw bits and has-a view for the >> > backing file. >> >> An image has-a image from which it takes its data (bs->file). > > No. A protocol has neither an image below it, nor a backing file. =A0I > think a view has no backing file either (except as provided by the > format). =A0I'm not sure that a listener can have a backing file, either; > you could say that blkverify has one, but it could just as well have > three or four, so it's not the same thing. > > A format does not do much more than create views, do snapshots, and hold > state that is common to all of its views. > > So, let's put aside listeners for a moment, and consider this > alternative hierarchy: > > BlockDriver > =A0 =A0Protocol > =A0 =A0 =A0 =A0FileProtocol > =A0 =A0 =A0 =A0... > =A0 =A0View > =A0 =A0 =A0 =A0QCOW2View > =A0 =A0 =A0 =A0RawView > =A0 =A0 =A0 =A0... > BlockFormat > =A0 =A0QCOW2Format > =A0 =A0RawFormat > =A0 =A0... > > Now we have to figure out how to fit listeners in this picture. > >> And it has-a view for the backing file, yes. Both could be a listener. >> >>> > - A view has-a format, a device has-a view. >>> > - A view can have-a listener? =A0Or is it formats? >> A view has-a image. This may happen to be a listener, which in turn >> has-a image (could be another listener, a format or a protocol). >> >> The question is what the semantics is with live snapshots (there are >> probably similar problems, but this is the obvious one). For example we >> could now have: >> >> mirror -> qcow2 -> blkdebug -> file > > Let's be precise here: > > mirror -> currentSnapshot -> qcow2 -> blkdebug -> file > > - file is a protocol. > > - blkdebug is a listener > > - qcow2 is a format > > - currentSnapshot is a view > > - mirror is a listener > > The difference between blkdebug/mirror on one side, and currentSnapshot > on the other, is that (as you said) blkdebug/mirror are always stacked > on top of something else. A driver that references currentSnapshot > actually gets mirror. > > So we get to the actual hierarchy I'm proposing: > > BlockDriver > =A0 =A0BlockSource (see below) > =A0 =A0Protocol (bdrv_file_open) > =A0 =A0 =A0 =A0FileProtocol > =A0 =A0 =A0 =A0... > =A0 =A0View (has-a BlockFormat) > =A0 =A0 =A0 =A0QCOW2View > =A0 =A0 =A0 =A0RawView > =A0 =A0 =A0 =A0... > =A0 =A0BlockListener (has-a BlockDriver) > =A0 =A0 =A0 =A0MirrorListener > =A0 =A0 =A0 =A0BlkDebugListener > BlockFormat (has-a BlockSource) > =A0 =A0QCOW2Format > =A0 =A0RawFormat > =A0 =A0... > > Protocols and views are only internal. =A0Formats and devices in practice > will only ever see BlockSources. =A0A BlockSource is a reference a stack = of > BlockDrivers, where the base must be a protocol or view and there can > be a number of listeners stacked on top of it. =A0Listeners can be > added or removed from the stack, and the bottom driver can be swapped > for another (for snapshots). > > So, here is how it would look: > > =A0.=3D=3D BlockSource =3D=3D. =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 .=3D= =3D BlockSource =3D=3D=3D. > =A0| MirrorListener =A0| =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | BlkDebugLi= stener | > =A0| QCOW2View ------+--> QCOW2Format -> | FileProtocol =A0 =A0 | > =A0'=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D' =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 '=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= ' > > >> There are two listeners here, mirror and blkdebug. (Things like blkdebug >> are why view has-a listener isn't enough). After creating an external >> snapshot, we expect the graph to look like this (the arrow down is the >> backing file): >> >> mirror -> qcow2 -> file >> =A0 =A0 =A0 =A0 =A0 =A0 | >> =A0 =A0 =A0 =A0 =A0 =A0 +-> qcow2 -> blkdebug -> file > > And here: > > =A0.=3D=3D BlockSource =3D=3D. > =A0| MirrorListener =A0| =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 .=3D=3D Bloc= kSource =3D=3D. > =A0| QCOW2View ------+--> QCOW2Format -> | FileProtocol =A0 =A0| > =A0'=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D' =A0 =A0| =A0 =A0= =A0 =A0 =A0 =A0 =A0'=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D' > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0.=3D=3D BlockSo= urce =3D=3D=3D. > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 | =A0 .=3D=3D BlockSource= =3D=3D. =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| BlkDebugListener | > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 '-> | QCOW2View ------+--= > QCOW2Format --> | FileProtocol =A0 =A0 | > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 '=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D' =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0'=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D' > > Does it seem sane? This is a good discussion because BlockDriverState has become bloated and complex. A lot of fields only apply to sub-cases and we should really split this struct. Fields like "backing_file" *should* be in generic code, not duplicated in each Format. But BlockDriverState is too generic since it also encompasses Protocols and Listeners. You mentioned that some of these classes would be "internal". I think everything should be exposed in the QOM just like Linux exposes kernel objects in sysfs. It makes troubleshooting and debugging easier. As has been pointed out, "Listener" suggests a passive role. Perhaps BlockFilter, BlockProcessor, or BlockModule is a better name. Ideally Formats can be isolated from the rest of the block layer so that it becomes easy to create a libimageformat. If we bake coroutines, I/O APIs, and memory allocation functions too deeply into Formats then they are hard to test and impossible to use outside of QEMU. Stefan