* [PATCH] md/raid1:using else-if instead if. @ 2012-04-01 2:29 majianpeng 2012-04-02 0:03 ` H. Peter Anvin 0 siblings, 1 reply; 4+ messages in thread From: majianpeng @ 2012-04-01 2:29 UTC (permalink / raw) To: Neil Brown; +Cc: linux-raid 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] md/raid1:using else-if instead if. 2012-04-01 2:29 [PATCH] md/raid1:using else-if instead if majianpeng @ 2012-04-02 0:03 ` H. Peter Anvin 2012-04-02 1:58 ` NeilBrown 0 siblings, 1 reply; 4+ messages in thread From: H. Peter Anvin @ 2012-04-02 0:03 UTC (permalink / raw) To: majianpeng; +Cc: Neil Brown, linux-raid 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] md/raid1:using else-if instead if. 2012-04-02 0:03 ` H. Peter Anvin @ 2012-04-02 1:58 ` NeilBrown 2012-04-02 2:11 ` H. Peter Anvin 0 siblings, 1 reply; 4+ messages in thread From: NeilBrown @ 2012-04-02 1:58 UTC (permalink / raw) To: H. Peter Anvin; +Cc: majianpeng, linux-raid [-- 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 --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] md/raid1:using else-if instead if. 2012-04-02 1:58 ` NeilBrown @ 2012-04-02 2:11 ` H. Peter Anvin 0 siblings, 0 replies; 4+ messages in thread From: H. Peter Anvin @ 2012-04-02 2:11 UTC (permalink / raw) To: NeilBrown; +Cc: majianpeng, linux-raid 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-04-02 2:11 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-04-01 2:29 [PATCH] md/raid1:using else-if instead if majianpeng 2012-04-02 0:03 ` H. Peter Anvin 2012-04-02 1:58 ` NeilBrown 2012-04-02 2:11 ` H. Peter Anvin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).