[lkml]   [2009]   [Jul]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH #tj-block-for-linus] block: fix failfast merge testing in elv_rq_merge_ok()
On 07/16/2009 11:06 AM, Tejun Heo wrote:
> Boaz Harrosh wrote:
>> On 07/16/2009 09:44 AM, Tejun Heo wrote:
>>> Commit ab0fd1debe730ec9998678a0c53caefbd121ed10 tries to prevent merge
>>> of requests with different failfast settings. In elv_rq_merge_ok(),
>>> it compares new bio's failfast flags against the merge target
>>> request's. However, the flag testing accessors for bio and blk don't
>>> return boolean but the tested bit value directly and FAILFAST on bio
>>> and blk don't match, so directly comparing them with == results in
>>> false negative unnecessary preventing merge of readahead requests.
>>> This patch convert the results to boolean by negating them before
>>> comparison.
>> I don't like that at all. Please fix the accessors to return
>> boolean. They look and regarded as boolean. I've never seen
>> them used as their bit value.
> Yeah, I'll be happier that way but please note that this patch is only
> for 2.6.31. 2.6.32 won't have this code at all and we're past the
> merge window, so the smallest fix wins in this case, I think. Also,
> changing only some of the accessors will increase the level of
> confusion while changing all of them for 2.6.31 at this point is way
> too invasive (there can be cases where the bit mask return value is
> depended upon).

OK So could you put a FIXME: and fat comment, on that weird "!"(s)

> Looks like the flags are gonna go through considerable cleanup pretty
> soon, so let's postpone small things till then.
>> if you are concerned with performance don't
>> an if(flag & bit) is even slightly slower then
>> if(0 != (flag & bit)) on some processors
> I wasn't worried about the performance at all.
> Thanks.


 \ /
  Last update: 2009-07-16 10:23    [W:0.057 / U:5.944 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site