From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51010) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WS6Fi-0005Vl-Uh for qemu-devel@nongnu.org; Mon, 24 Mar 2014 10:53:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WS6Fc-0005uB-UD for qemu-devel@nongnu.org; Mon, 24 Mar 2014 10:53:26 -0400 Received: from 28-113-190-109.dsl.ovh.fr ([109.190.113.28]:42355 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WS6Fc-0005u3-Gf for qemu-devel@nongnu.org; Mon, 24 Mar 2014 10:53:20 -0400 Date: Mon, 24 Mar 2014 15:53:19 +0100 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140324145319.GG3071@irqsave.net> References: <20140314155756.GC3324@irqsave.net> <20140317031231.GA28582@T430.nay.redhat.com> <20140318132747.GN4607@noname.str.redhat.com> <20140320140509.GA3205@irqsave.net> <20140320151234.GC3450@noname.redhat.com> <20140320160626.GB3045@irqsave.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140320160626.GB3045@irqsave.net> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] n ways block filters List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Beno=EEt?= Canet Cc: Kevin Wolf , Fam Zheng , Stefan Hajnoczi , qemu-devel , Max Reitz , Stefan Hajnoczi , armbru@redhat.com The Thursday 20 Mar 2014 =E0 17:06:26 (+0100), Beno=EEt Canet wrote : > The Thursday 20 Mar 2014 =E0 16:12:34 (+0100), Kevin Wolf wrote : > > Am 20.03.2014 um 15:05 hat Beno=EEt Canet geschrieben: > > > The Tuesday 18 Mar 2014 =E0 14:27:47 (+0100), Kevin Wolf wrote : > > > > Am 17.03.2014 um 17:02 hat Stefan Hajnoczi geschrieben: > > > > > On Mon, Mar 17, 2014 at 4:12 AM, Fam Zheng wr= ote: > > > > > > On Fri, 03/14 16:57, Beno=EEt Canet wrote: > > > > > >> I discussed a bit with Stefan on the list and we came to the= conclusion that the > > > > > >> block filter API need group support. > > > > > >> > > > > > >> filter group: > > > > > >> ------------- > > > > > >> > > > > > >> My current plan to implement this is to add the following fi= elds to the BlockDriver > > > > > >> structure. > > > > > >> > > > > > >> int bdrv_add_filter_group(const char *name, QDict options); > > > > > >> int bdrv_reconfigure_filter_group(const char *name, QDict op= tions); > > > > > >> int bdrv_destroy_filter_group(const char *name); > > > >=20 > > > > Beno=EEt, your mail left me puzzled. You didn't really describe t= he > > > > problem that you're solving, nor what the QDict options actually > > > > contains or what a filter group even is. > > > >=20 > > > > > >> These three extra method would allow to create, reconfigure = or destroy a block > > > > > >> filter group. A block filter group contain the shared or non= shared state of the > > > > > >> blockfilter. For throttling it would contains the ThrottleSt= ate structure. > > > > > >> > > > > > >> Each block filter driver would contains a linked list of lin= ked list where the > > > > > >> BDS are registered grouped by filter groups state. > > > > > > > > > > > > Sorry I don't fully understand this. Does a filter group cont= ain multiple block > > > > > > filters, and every block filter has effect on multiple BDSes?= Could you give an > > > > > > example? > > > > >=20 > > > > > Just to why a "group" mechanism is useful: > > > > >=20 > > > > > You want to impose a 2000 IOPS limit for the entire VM. Curren= tly > > > > > this is not possible because each drive has its own throttling = state. > > > > >=20 > > > > > We need a way to say certain drives are part of a group. All d= rives > > > > > in a group share the same throttling state and therefore a 2000= IOPS > > > > > limit is shared amongst them. > > > >=20 > > > > Now at least I have an idea what you're all talking about, but it= 's > > > > still not obvious to me how the three functions from above solve = your > > > > problem or how they work in detail. > > > >=20 > > > > The obvious solution, using often discussed blockdev-add concepts= , is: > > > > ______________ > > > > virtio-blk_A --> | | --> qcow2_A --> raw-posix_A > > > > | throttling | > > > > virtio_blk_B --> |____________| --> qcow2_B --> nbd_B > > >=20 > > > My proposal would be: > > > ______________ > > > virtio-blk_A --> | BDS 1 | --> qcow2_A --> raw-posix_A > > > |____________| > > > | > > > _____|________ > > > | | The shared state is the state of a= BDS group > > > | Shared | It's stored in a static linked lis= t of the > > > | State | block/throttle.c module. It has a = name and contains a > > > |____________| throttle state structure. > > > | > > > _____|________ > > > | BDS 2 | > > > virtio_blk_B --> |____________| --> qcow2_B --> nbd_B > >=20 > > Okay. I think your proposal might be easier to implement in the short > > run, but it introduces an additional type of nodes to the graph (so f= ar > > we have only one type, BlockDriverStates) with their own set of > > functions, and I assume monitor commands, for management. > >=20 > > This makes the whole graph less uniform and consistent. There may be > > cases where this is necessary or at least tolerable because the fully > > generic alternativ isn't doable. I'm not convinced yet that this is t= he > > case here. > >=20 > > In contrast, my approach would require considerable infrastructure wo= rk > > (you somehow seem to attract that kind of things ;-)), but it's merel= y a > > generalisation of what we already have and as such fits nicely in the > > graph. > >=20 > > We already have multiple children of BDS nodes. And we take it for > > granted that they don't refer to the same data, but that bs->file and > > bs->backing_hd have actually different semantics. > >=20 > > We have recently introduced refcounts for BDSes so that one BDS can n= ow > > have multiple parents, too, as a first step towards symmetry. The > > logical extension is that these parent get different semantics, just > > like the children have different semantics. > >=20 > > Doing the abstraction in one model right instead of adding hacks that > > don't really fit in but are easy to implement has paid off in the pas= t. > > I'm pretty sure that extending the infrastructure this way will find > > more users than just I/O throttling, and that having different parent= s > > in different roles is universally useful. With qcow2 exposing the > > snapshots, too, I already named a second potential user of the > > infrastructure. > >=20 > > > The name of the shared state is the throttle group name. > > > The three added methods are used to add, configure and destroy such= shared > > > states. > > >=20 > > > The benefit of this aproach is that we don't need to add a special = slot mechanism > > > and that removing BDS 2 would be easy. > > > Your approach don't deal with the fact that the throttling group me= mbership can > > > be changed dynamically while the vm is running: for example adding = qcow2_C and > > > removing qcow2_B should be made easy. > >=20 > > Yes, this is right. But then, the nice thing about it is that I staye= d > > fully within the one uniform graph. We just need a way to modify the > > edges in this graph (and we already need that to insert/delete filter= s) > > and you get this special case and many others for free. > >=20 > > So, I vote for investing into a uniform infrastructure here instead o= f > > adding new one-off node types. >=20 > Maybe parents BDS could use a generic block function to get a cookie wh= en they > start to use a children BDS. >=20 > The parent would to >=20 > bs->file_cookie =3D bdrv_get_cookie(file); > bs->file =3D file; >=20 > when choosing to use file as bs file. >=20 > The get cookie method would be >=20 > uint64_t bdrv_get_cookie(bs) { > bs->cookie =3D gen_uuid(bs); > return bs->cookie; > } >=20 > gen_uuid would combine a random 64 bit number with a registry to preven= t > identical cookie generation. >=20 > After this step every BlockDriver method would receive the cookie as se= cond > parameter. >=20 > For example bdrv_read(bs, cookie, ...) >=20 > So it's easy for a block driver to discriminate based on the cookie and= even to > look up which of his own child is associated to this cookie. >=20 > Best regards >=20 > Beno=EEt >=20 > >=20 > > Kevin Kevin: what do you think of this cookie idea ? It seems something doable with reasonable small steps. Best regards Beno=EEt > >=20 > > > > That is, the I/O throttling BDS is referenced by two devices inst= ead of > > > > just one and it associates one 'input' with one 'output'. Once we= have > > > > BlockBackend, we would have two BBs, but still only one throttlin= g > > > > BDS. > > > >=20 > > > > The new thing that you get there is that the throttling driver ha= s > > > > not only multiple parents (that part exists today), but it behave= s > > > > differently depending on who called it. So we need to provide som= e way > > > > for one BDS to expose multiple slots or whatever you want to call= them > > > > that users can attach to. > > > >=20 > > > > This is, by the way, the very same thing as would be required for > > > > exposing qcow2 internal snapshots (read-only) while the VM is run= ning. > > > >=20 > > > > Kevin > > > >=20