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

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

[Tejun Heo]

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.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <jens.axboe@oracle.com>
Cc: Boaz Harrosh <bharrosh@panasas.com>
Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Jeff Garzik <jeff@garzik.org>
---
Eh... talk about being obscure. :-( I've put this onto the following
temp block tree I'm running and will push toward Linus in a few days.

  git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git tj-block-for-linus

Thanks.

 block/elevator.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index 6f23753..977aa7c 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -103,9 +103,9 @@ int elv_rq_merge_ok(struct request *rq, struct bio *bio)
 	/*
 	 * Don't merge if failfast settings don't match
 	 */
-	if (bio_failfast_dev(bio)	!= blk_failfast_dev(rq)		||
-	    bio_failfast_transport(bio)	!= blk_failfast_transport(rq)	||
-	    bio_failfast_driver(bio)	!= blk_failfast_driver(rq))
+	if (!bio_failfast_dev(bio)	 != !blk_failfast_dev(rq)	||
+	    !bio_failfast_transport(bio) != !blk_failfast_transport(rq)	||
+	    !bio_failfast_driver(bio)	 != !blk_failfast_driver(rq))
 		return 0;

 	if (!elv_iosched_allow_merge(rq, bio))
-- 
1.6.0.2

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

[Boaz Harrosh]

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.

if you are concerned with performance don't
an if(flag & bit) is even slightly slower then
   if(0 != (flag & bit)) on some processors

> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Jens Axboe <jens.axboe@oracle.com>
> Cc: Boaz Harrosh <bharrosh@panasas.com>
> Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Cc: Jeff Garzik <jeff@garzik.org>
> ---
> Eh... talk about being obscure. :-( I've put this onto the following
> temp block tree I'm running and will push toward Linus in a few days.
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git tj-block-for-linus
> 
> Thanks.
> 
>  block/elevator.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/elevator.c b/block/elevator.c
> index 6f23753..977aa7c 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -103,9 +103,9 @@ int elv_rq_merge_ok(struct request *rq, struct bio *bio)
>  	/*
>  	 * Don't merge if failfast settings don't match
>  	 */
> -	if (bio_failfast_dev(bio)	!= blk_failfast_dev(rq)		||
> -	    bio_failfast_transport(bio)	!= blk_failfast_transport(rq)	||
> -	    bio_failfast_driver(bio)	!= blk_failfast_driver(rq))
> +	if (!bio_failfast_dev(bio)	 != !blk_failfast_dev(rq)	||
> +	    !bio_failfast_transport(bio) != !blk_failfast_transport(rq)	||
> +	    !bio_failfast_driver(bio)	 != !blk_failfast_driver(rq))
>  		return 0;
> 
>  	if (!elv_iosched_allow_merge(rq, bio))

Boaz

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

[Tejun Heo]

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

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.

-- 
tejun

Re: [PATCH #tj-block-for-linus] block: fix failfast merge testing in 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

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

[Tejun Heo]

Boaz Harrosh wrote:
> OK So could you put a FIXME: and fat comment, on that weird "!"(s)
> everywhere?

Yeap, sure.  How about something like the following?

	/*
	 * Don't merge if failfast settings don't match.  The negation
	 * in front of each condition is necessary because bio and
	 * request flags use different bit positions and the accessors
	 * return those bits directly.  This ugliness will soon go
	 * away.
	 */

-- 
tejun

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

[Boaz Harrosh]

On 07/16/2009 11:35 AM, Tejun Heo wrote:
> Boaz Harrosh wrote:
>> OK So could you put a FIXME: and fat comment, on that weird "!"(s)
>> everywhere?
> 
> Yeap, sure.  How about something like the following?
> 
> 	/*
> 	 * Don't merge if failfast settings don't match.  The negation

- 	 * Don't merge if failfast settings don't match.  The negation
+ 	 * Don't merge if failfast settings don't match.
+	 * FIXME:   The negation ...

> 	 * in front of each condition is necessary because bio and
> 	 * request flags use different bit positions and the accessors
> 	 * return those bits directly.  This ugliness will soon go
> 	 * away.
> 	 */
> 

Boaz

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

[Tejun Heo]

Boaz Harrosh wrote:
> On 07/16/2009 11:35 AM, Tejun Heo wrote:
>> Boaz Harrosh wrote:
>>> OK So could you put a FIXME: and fat comment, on that weird "!"(s)
>>> everywhere?
>> Yeap, sure.  How about something like the following?
>>
>> 	/*
>> 	 * Don't merge if failfast settings don't match.  The negation
> 
> - 	 * Don't merge if failfast settings don't match.  The negation
> + 	 * Don't merge if failfast settings don't match.
> +	 * FIXME:   The negation ...

So updated.  Thanks.

-- 
tejun