Re: [RFC] FC pass thru - Rev V

[Boaz Harrosh <bharrosh@panasas.com>]

FUJITA Tomonori wrote:
> On Wed, 11 Feb 2009 18:15:07 +0200
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>> James Smart wrote:
>>> Trying to kick-start this again...  
>>> I've updated the prior RFC with the comments from Seokmann,
>>> SvenFujita, and Boaz. I would still like review on the
>>> blk_xxx completion calls in the std and error paths.
>>>
>>> It currently expects that blk_end_request() has been updated
>>> by Fujita's patch to incorporate blk_end_bidi_request()
>>> functionality :
>>> http://marc.info/?l-linux-scsi&m=122785157116659&w=2
>>>
>> I did not accept this patch and it did not go in right?
> 
> I think that Jens has not merged mine or yours. I don't care about
> either but I still think that it's better to kill
> blk_end_bidi_request(). It's really confusing API.
> 
> 
>> I still don't like it, it's a performance regression.
> 
> Hmm, I've not seen the figures. Please show the figures if you insist
> a performance regression.
> 

Come on man. this discussion all over again. You do a loop to find
information that was in a CPU register 10 cycles before. That is
plain bad programing, and just a cover up of bad API.

In my book it is: Someone got lazy.

> It's about a bidi request. We already have tons of loops, memory
> allocations, etc in the path. Do you think that adding one more loop
> leads to a notable performance regression?
> 
> Well, if you say that it's hacky then I would agree. But your patch
> using ~0 is hacky too.

It is an hack if used by an outside user, because it assumes knowledge
of block-internals. It is much less of an hack if done by block-internals
which knows for sure that this has no side effects.

But I agree that this is not clean. The clean solution is to add an extra
parameter to blk_end_request() and change all callers.

Or even cleaner is to add a new request->residual member and leave
request->data_len be in peace. Then change the few users that care
about residual, and one caller that sets it. I'll prepare a patch.

Boaz