Skip to contentSkip to conversation

md/raid1:using else-if instead if.

PATCH In review majianpeng 3 participants 4 messages Apr 1, 2012 – Apr 2, 2012 MD RAID

lore
Share & tools
lore

majianpeng

author
#0 lore raw

PATCH 1/1 md/raid1:using else-if instead if.

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

Signed-off-by majianpeng <[email protected]>
---
 drivers/md/raid1.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
drivers/md/raid1.c +1 -2
index 4a40a20..a9de970 100644
@@ -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);
Signature · 6 lines
1.7.5.4

 				
--------------
majianpeng
2012-04-01
#1 lore raw
Quoted 15 lines · majianpeng

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

Signed-off-by majianpeng <[email protected]>
---
 drivers/md/raid1.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
drivers/md/raid1.c +0 -0
index 4a40a20..a9de970 100644

drivers/md/raid1.c static void handle_sync_write_finished(struct r1conf *conf, struct r1bio *r1_bio · line 2030

Quoted 9 lines
drivers/md/raid1.c +1 -2
@@ -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

Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.
#2 lore raw
Quoted 11 lines · On Sun, 01 Apr 2012 17:03:11 -0700 "H. Peter Anvin" <[email protected]> wrote:
Quoted 10 lines · majianpeng

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

Signed-off-by majianpeng <[email protected]>
---
 drivers/md/raid1.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
Quoted 14 lines
Quoted 5 lines
drivers/md/raid1.c +0 -0
index 4a40a20..a9de970 100644

drivers/md/raid1.c static void handle_sync_write_finished(struct r1conf *conf, struct r1bio *r1_bio · line 2030

Quoted 9 lines
drivers/md/raid1.c +1 -2
@@ -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..

Quoted 10 lines

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.

Quoted 2 lines

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

Quoted 4 lines

-hpa

#3 lore raw
Quoted 3 lines · NeilBrown

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

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