On Sun, 01 Apr 2012 17:03:11 -0700 "H. Peter Anvin" <hpa@xxxxxxxxx> wrote:
> On 03/31/2012 07:29 PM, majianpeng wrote:
> > From 798f3fce3d077db049a44d0d2434261c937796e9 Mon Sep 17 00:00:00 2001
> > From: majianpeng <majianpeng@xxxxxxxxx>
> > Date: Sun, 1 Apr 2012 10:23:56 +0800
> > Subject: [PATCH] md/raid1:using else-if instead if.
> >
> > Signed-off-by: majianpeng <majianpeng@xxxxxxxxx>
> > ---
> > 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:
signature.asc
Description: PGP signature
