[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] Consolidate reads and writes in nbd block de
From: |
Michael Tokarev |
Subject: |
Re: [Qemu-devel] [PATCH v2] Consolidate reads and writes in nbd block device into one common routine |
Date: |
Tue, 28 Feb 2012 17:16:46 +0400 |
User-agent: |
Mozilla/5.0 (X11; Linux i686 on x86_64; rv:8.0) Gecko/20120104 Icedove/8.0 |
On 28.02.2012 17:03, Paolo Bonzini wrote:
> Il 28/02/2012 13:35, Michael Tokarev ha scritto:
>> On 28.02.2012 15:35, Paolo Bonzini wrote:
>>> Il 28/02/2012 11:24, Michael Tokarev ha scritto:
>>>> This removes quite some duplicated code.
>> []
>>>> +static int nbd_co_rwv(BlockDriverState *bs, int64_t sector_num,
>>>> + int nb_sectors, QEMUIOVector *qiov, int iswrite)
>>>
>>> Call this nbd_co_rw, and please pass the whole request.type down.
>>
>> Originally it is readV and writeV, so why it should not be rwV ?
>
> It's more consistent with block.c.
>
>> By passing whole request.type (NBD_CMD_WRITE or
>> NBD_CMD_WRITE|NBD_CMD_FLAG_FUA
>> or NBD_CMD_READ) the condition (iswrite currently) will be larger
>> (request.type
>> != NBD_CMD_READ). Also, if someday we'll have additional flag for READ as we
>> already do for write, whole thing will be even more difficult to read.
>
> Sure, but why should a generic function deal with NBD_CMD_FLAG_FUA?
I can pass both "iswrite" and request.type here - to avoid possible
complications in the area of adding more nbd-specific meanings/flags
to request.type. But that becomes more ugly.
[]
>>> ... but thinking more about it, why don't you leave
>>> nbd_co_readv_1/nbd_co_writev_1 alone, and create a nbd_split_rw function
>>> that takes a function pointer?
>>
>> Because each of these nbd_co_*_1 does the same thing, the diff. is
>> only quiv->iov vs NULL. While reading the original code it was the
>> first thing I did - consolidated nbd_co_*_1 into nbd_co_* ;)
>
> And offset. I needed to check that non-0 offsets are fine when the iov
> is NULL; it's not obvious.
It isn't indeed. Both send_request and recv_reply only checks iov
and ignores offset if iov is NULL.
>> Actually it might be a good idea to have single bdrv->bdrv_co_readwritev
>> method instead of two -- the path of each read and write jumps between
>> specific read-or-write routine and common readwrite routine several
>> times.
>
> Small amounts of duplicate code can be better than functions with many
> ifs or complicated conditions.
Sure thing. But when the code path is so twisted - common->specific->
common-> specific - it makes very difficult to get the bigger picture.
>> I see only one correction which needs (really!) to be done - that's
>> fixing the bug with offset. Do you still not agree?
>
> I still disagree. :) I will accept the patch with the function pointer
> though.
With separate nbd_co_*_1 it isn't worth the effort. When it all is in a
_small_ single routine (the resulting function is actually very small),
whole logic is immediately visible. In particular, the FUA bit which
is set for every _part_ of write request - it wasn't visible till I
integrated nbd_co_writev_1 into nbd_co_writev.
Thanks,
/mjt