FUJITA Tomonori wrote:
> On Mon, 01 Dec 2008 12:19:09 +0200
> Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote:
>
>> FUJITA Tomonori wrote:
>>> On Mon, 01 Dec 2008 11:42:48 +0200
>>> Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote:
>>>
>>>>>> No. I do not want a loop to calculate the size here. This is ugly.
>>>>>> I have this information, and I just lost it do to BAD API.
>>>>> IMO, being ugly is better than confusing people.
>>>> Come on man. Ugly was an understatement. Can't you see?
>>>> Make a loop for information we have and decided to drop on the floor.
>>> Honestly, I can't see. Whatever you call the patch, the current API is
>>> worse than it for me.
>>>
>>>
>>>>> Do you think that the current API is good, which make developers
>>>>> always wrongly use it and feel that they have to ask how to use and
>>>>> add comments about necessary tricks to use it?
>>>>>
>>>> No I think (and said) that the API is crap. But not that bad that I want to
>>>> take the effort and change it.
>>>>
>>>>>> Since we do not want to change all users of blk_end_request. (We should,
>>>>>> but we are too lazy. I'm). Then please just let blk_end_bidi_request()
>>>>>> be. Confused users will be corrected.
>>>>> No. blk_end_bidi_request interface and the name is confusing. So
>>>>> forcing everyone to use blk_end_bidi_request doesn't help.
>>>>>
>>>> We should rename blk_end_bidi_request => blk_end_request() and
>>>> change all users to add an extra 0, and the original blk_end_request()
>>>> should be dropped.
>>> That might be better than the current situation but why do we need
>>> bidi_bytes (number of bytes to complete for bidi) in blk_end_request?
>>>
>>> Having bidi_bytes is theoretically correct but we always must complete
>>> all the bytes with bidi. Do you say that someone might need it in the
>>> future?
>> I will try to explain. The situation was bad for a long time. Not at
>> all because of the amount of bytes to complete but because of the residual
>> count. Look at current code that wants to return residual. It needs to save
>> on the side what ever was there in req->data_len then set req->data_len
>> to residual and call blk_end_request with the saved count. Now the thing
>> became worse because we have two of these.
>>
>> Yes sure I want to complete both sides and I don't mind calling
>> blk_end_io( req, error, ~0U, ~0U, ...) so lets make a compromise
>> in your patch, don't loop on the bio's just set bidi_bytes = ~0U;
>> that is:
>>
>> int blk_end_request(struct request *rq, int error, unsigned int nr_bytes)
>> {
>> - return blk_end_io(rq, error, nr_bytes, 0, NULL);
>> + unsigned int bidi_bytes = 0;
>> +
>> + if (blk_bidi_rq(rq))
>> + bidi_bytes = ~0U;
>> +
>> + return blk_end_io(rq, error, nr_bytes, bidi_bytes, NULL);
>> }
>> EXPORT_SYMBOL_GPL(blk_end_request);
>
> Well, I also thought about the exact same approach.
>
> I guess that this taints part_stat_add() in
> __end_that_request_first()? Well, bidi is not fs requests for now (or
> forever?) so the patch should be fine now.
Thanks TOMO. I appreciate this very much.
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
