[PATCH] md/raid1:using else-if instead if.

[PATCH] md/raid1:using else-if instead if.

[majianpeng]

From 798f3fce3d077db049a44d0d2434261c937796e9 Mon Sep 17 00:00:00 2001
From: majianpeng <majianpeng@gmail.com>
Date: Sun, 1 Apr 2012 10:23:56 +0800
Subject: [PATCH] md/raid1:using else-if instead if.


Signed-off-by: majianpeng <majianpeng@gmail.com>
---
 drivers/md/raid1.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 4a40a20..a9de970 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -2024,8 +2024,7 @@ static void handle_sync_write_finished(struct r1conf *conf, struct r1bio *r1_bio
 		if (test_bit(BIO_UPTODATE, &bio->bi_flags) &&
 		    test_bit(R1BIO_MadeGood, &r1_bio->state)) {
 			rdev_clear_badblocks(rdev, r1_bio->sector, s);
-		}
-		if (!test_bit(BIO_UPTODATE, &bio->bi_flags) &&
+		} else if (!test_bit(BIO_UPTODATE, &bio->bi_flags) &&
 		    test_bit(R1BIO_WriteError, &r1_bio->state)) {
 			if (!rdev_set_badblocks(rdev, r1_bio->sector, s, 0))
 				md_error(conf->mddev, rdev);
-- 
1.7.5.4

 				
--------------
majianpeng
2012-04-01

Re: [PATCH] md/raid1:using else-if instead if.

[H. Peter Anvin]

On 03/31/2012 07:29 PM, majianpeng wrote:
> From 798f3fce3d077db049a44d0d2434261c937796e9 Mon Sep 17 00:00:00 2001
> From: majianpeng <majianpeng@gmail.com>
> Date: Sun, 1 Apr 2012 10:23:56 +0800
> Subject: [PATCH] md/raid1:using else-if instead if.
> 
> Signed-off-by: majianpeng <majianpeng@gmail.com>
> ---
>  drivers/md/raid1.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 4a40a20..a9de970 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -2024,8 +2024,7 @@ static void handle_sync_write_finished(struct r1conf *conf, struct r1bio *r1_bio
>  		if (test_bit(BIO_UPTODATE, &bio->bi_flags) &&
>  		    test_bit(R1BIO_MadeGood, &r1_bio->state)) {
>  			rdev_clear_badblocks(rdev, r1_bio->sector, s);
> -		}
> -		if (!test_bit(BIO_UPTODATE, &bio->bi_flags) &&
> +		} else if (!test_bit(BIO_UPTODATE, &bio->bi_flags) &&
>  		    test_bit(R1BIO_WriteError, &r1_bio->state)) {
>  			if (!rdev_set_badblocks(rdev, r1_bio->sector, s, 0))
>  				md_error(conf->mddev, rdev);

It would be even better to:

if (test_bit(BIO_UPDATE, &bio->bi_flags)) {
	if (test_bit(R1BIO_MadeGood, &r1_bio->state))
  			rdev_clear_badblocks(rdev, r1_bio->sector, s);
} else {
	if (test_bit(R1BIO_WriteError, &r1_bio->state)) {
	...


... rather than testing the bit twice.

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] md/raid1:using else-if instead if.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2447 bytes --]

On Sun, 01 Apr 2012 17:03:11 -0700 "H. Peter Anvin" <hpa@zytor.com> wrote:

> On 03/31/2012 07:29 PM, majianpeng wrote:
> > From 798f3fce3d077db049a44d0d2434261c937796e9 Mon Sep 17 00:00:00 2001
> > From: majianpeng <majianpeng@gmail.com>
> > Date: Sun, 1 Apr 2012 10:23:56 +0800
> > Subject: [PATCH] md/raid1:using else-if instead if.
> > 
> > Signed-off-by: majianpeng <majianpeng@gmail.com>
> > ---
> >  drivers/md/raid1.c |    3 +--
> >  1 files changed, 1 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> > index 4a40a20..a9de970 100644
> > --- a/drivers/md/raid1.c
> > +++ b/drivers/md/raid1.c
> > @@ -2024,8 +2024,7 @@ static void handle_sync_write_finished(struct r1conf *conf, struct r1bio *r1_bio
> >  		if (test_bit(BIO_UPTODATE, &bio->bi_flags) &&
> >  		    test_bit(R1BIO_MadeGood, &r1_bio->state)) {
> >  			rdev_clear_badblocks(rdev, r1_bio->sector, s);
> > -		}
> > -		if (!test_bit(BIO_UPTODATE, &bio->bi_flags) &&
> > +		} else if (!test_bit(BIO_UPTODATE, &bio->bi_flags) &&
> >  		    test_bit(R1BIO_WriteError, &r1_bio->state)) {
> >  			if (!rdev_set_badblocks(rdev, r1_bio->sector, s, 0))
> >  				md_error(conf->mddev, rdev);

I don't like this option as it confuses the logic..

> 
> It would be even better to:
> 
> if (test_bit(BIO_UPDATE, &bio->bi_flags)) {
> 	if (test_bit(R1BIO_MadeGood, &r1_bio->state))
>   			rdev_clear_badblocks(rdev, r1_bio->sector, s);
> } else {
> 	if (test_bit(R1BIO_WriteError, &r1_bio->state)) {
> 	...
> 

and I don't really like adding unnecessary indentation.

> 
> ... rather than testing the bit twice.

I'm really surprised that the compiler doesn't optimise that out.

I see:

   0x0000000000004fb1 <+113>:	mov    0x18(%rcx),%rax
   0x0000000000004fb5 <+117>:	test   $0x1,%al
   0x0000000000004fb7 <+119>:	je     0x4f70 <handle_sync_write_finished+48>

which is the first test_bit(BIO_UPTODATE), then

   0x0000000000004f70 <+48>:	mov    0x18(%rcx),%rax
   0x0000000000004f74 <+52>:	test   $0x1,%al
   0x0000000000004f76 <+54>:	jne    0x4f82 <handle_sync_write_finished+66>

so it is repeating a test that it already knows the answer too.
Why not just "je 0x4f78 <handle_sync_write_finished+56> I wonder.

Still, I'm much more interested in readability than this sort of micro
optimisation, so I'll leave the code as it is.

Thanks,
NeilBrown


> 
> 	-hpa
> 
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] md/raid1:using else-if instead if.

[H. Peter Anvin]

On 04/01/2012 06:58 PM, NeilBrown wrote:
> 
> I'm really surprised that the compiler doesn't optimise that out.
> 

It depends partly on what it can depend on ... it probably isn't
allowed to because of alias analysis, at least not without the else if.

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.