Re: [PATCH #tj-block-for-linus] block: fix failfast merge testingin elv_rq_merge_ok()

[Boaz Harrosh]

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)
everywhere?

> 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.
> 

Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/