Re: [Qemu-devel] [PATCH 3/3] Combine bdrv_co_readv and bdrv_co_writev into bdrv_co_rw_vector

[Michael Tokarev]

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