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

[NeilBrown <neilb@xxxxxxx>]

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