From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60973) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S2mek-00006T-4N for qemu-devel@nongnu.org; Wed, 29 Feb 2012 11:45:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S2mei-0005wG-1H for qemu-devel@nongnu.org; Wed, 29 Feb 2012 11:45:33 -0500 Received: from isrv.corpit.ru ([86.62.121.231]:43878) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S2meh-0005vh-Lj for qemu-devel@nongnu.org; Wed, 29 Feb 2012 11:45:31 -0500 Message-ID: <4F4E5628.7020002@msgid.tls.msk.ru> Date: Wed, 29 Feb 2012 20:45:28 +0400 From: Michael Tokarev MIME-Version: 1.0 References: <1330473276-8975-1-git-send-email-mjt@tls.msk.ru> <1330473276-8975-4-git-send-email-mjt@msgid.tls.msk.ru> <4F4E4BDA.9000304@redhat.com> <4F4E4E7F.8050202@msgid.tls.msk.ru> <4F4E5140.5030600@redhat.com> In-Reply-To: <4F4E5140.5030600@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/3] Combine bdrv_co_readv and bdrv_co_writev into bdrv_co_rw_vector List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, qemu-devel@nongnu.org On 29.02.2012 20:24, Paolo Bonzini wrote: > Il 29/02/2012 17:12, Michael Tokarev ha scritto: >> On 29.02.2012 20:01, Paolo Bonzini wrote: >>> Il 29/02/2012 00:54, Michael Tokarev ha scritto: >>>> BlockDriver *drv = bs->drv; >>>> BdrvTrackedRequest req; >>>> + bool is_write = flags & (BDRV_REQ_WRITE|BDRV_REQ_ZERO_WRITE); >>>> int ret; >>> >>> You can do BDRV_REQ_WRITE|BDRV_REQ_ZERO_WRITE, but not >>> BDRV_REQ_READ|BDRV_REQ_COPY_ON_READ. That's ugly. >> >> BDRV_REQ_READ is zero. This is just mnemonic to avoid "magic >> numbers" elsewhere in the code. This is an internal function >> and the comment above it says just that, and it is always >> called with just ONE value. It is not a bitmask, it is used >> as such inside this very routine ONLY. The argument is declared >> as enum too, -- this should tell something. In the function >> prototype it should have been named "opcode" or "request", >> not "flags". It is used as flags only inside this function. >> >> This code isn't written by me, it was this way before. >> I just added 2 more possible values for this parameter. > > If you have 4 values, make them 1/2/4/8 or 0/1/2/3. Not 0/1/2/4. You didn't read what I wrote, did you? In the _original_ code, this very enum was ALREADY used as a bitmask, but only inside this routine. I can change that, but it has nothing to do with the patch in question. Making it 0/1/2/3 will break that. And yet again, I dislike this code myself, and I mentioned this already, but that's why I put the "RFC" in the original subject -- RFC for the general "idea". >> No block driver -- at least currently -- needs any other value >> here except of read-or-write (or is_write). COPY_ON_READ is >> not a business of the individual block drivers currently. > > Sure, but ZERO_WRITES is (we have a separate callback). I already replied about this. To me it looks like it is better to keep it as separate, for several reasons: very few drivers will actually implement it in a reasonable way. By turning it into "request type" of a common dispatcher method (like bdrv_rw) means that each driver will have to recognize it and call some common emulation routine, instead of letting the upper layer to deal with it. it matches much better the discard method instead of rw method, so if we want to combine, lets' go there instead. it does not accept data, just like discard. So we still end up with reads or writes. And using bool instead of a enum for these is easier. > >> These defines are _only_ to make some code a bit more readable, >> in a very few places where it necessary to call individual >> read or write block driver method. So that the construct: >> >> ret - s->bdrv_co_rw_vector(bs, ..., true) >> >> becomes >> >> ret - s->bdrv_co_rw_vector(bs, ..., BDRV_WRITE) >> >> and it is immediately obvious that it is write. The prototype >> of the method has "bool is_write" here. > > If you use an enum, the prototype shouldn't be bool. I use a bool, not an enum. The parameter is called "is_write". It is also used in lots of other places. >>> But I'm skeptical, the >>> actual amount of unification is not that large. >> >> This is not about unification. This is, as described in the introduction >> email, about removing complexity of a very twisted nature of read and >> write code paths, for a start. > > The patches are a balance of removing duplication and adding > conditionals, no? Removing duplication is unification. The unification is just a (good) side effect. Thanks, /mjt