Re: [PATCH] md: do not use ++ in rcu_dereference() argument

[Neil Brown <neilb@suse.de>]

On Mon, 06 Sep 2010 09:43:32 +0200
walter harms <wharms@bfs.de> wrote:

> 
> 
> Neil Brown schrieb: 
> > I've taken the opportunity to substantially re-write that code.
> > 
> > Comments?
> > 
> > Thanks,
> > NeilBrown
> > 
> > commit e4062735c8f7233923df5858ed20f1278f3ee669
> > Author: NeilBrown <neilb@suse.de>
> > Date:   Mon Sep 6 14:10:08 2010 +1000
> > 
> >     md: tidy up device searches in read_balance.
> >     
> >     We have a pre-increment side-effect in the arg to a macro:
> >       rcu_dereference
> >     
> >     This is poor form and triggers a warning.  Rather than just fix that,
> >     take the opportunity to re-write the code it make it more readable.
> >     
> >     Reported-by: Kulikov Vasiliy <segooon@gmail.com>
> >     Signed-off-by: NeilBrown <neilb@suse.de>
> > 
> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> > index ad83a4d..e29e13f 100644
> > --- a/drivers/md/raid1.c
> > +++ b/drivers/md/raid1.c
> > @@ -420,11 +420,13 @@ static void raid1_end_write_request(struct bio *bio, int error)
> >  static int read_balance(conf_t *conf, r1bio_t *r1_bio)
> >  {
> >  	const sector_t this_sector = r1_bio->sector;
> > -	int new_disk = conf->last_used, disk = new_disk;
> > -	int wonly_disk = -1;
> > +	int new_disk = -1;
> > +	int start_disk;
> > +	int i;
> >  	const int sectors = r1_bio->sectors;
> >  	sector_t new_distance, current_distance;
> >  	mdk_rdev_t *rdev;
> > +	int choose_first;
> >  
> >  	rcu_read_lock();
> >  	/*
> > @@ -435,54 +437,35 @@ static int read_balance(conf_t *conf, r1bio_t *r1_bio)
> >   retry:
> >  	if (conf->mddev->recovery_cp < MaxSector &&
> >  	    (this_sector + sectors >= conf->next_resync)) {
> > -		/* Choose the first operational device, for consistancy */
> > -		new_disk = 0;
> > -
> > -		for (rdev = rcu_dereference(conf->mirrors[new_disk].rdev);
> > -		     r1_bio->bios[new_disk] == IO_BLOCKED ||
> > -		     !rdev || !test_bit(In_sync, &rdev->flags)
> > -			     || test_bit(WriteMostly, &rdev->flags);
> > -		     rdev = rcu_dereference(conf->mirrors[++new_disk].rdev)) {
> > -
> > -			if (rdev && test_bit(In_sync, &rdev->flags) &&
> > -				r1_bio->bios[new_disk] != IO_BLOCKED)
> > -				wonly_disk = new_disk;
> > -
> > -			if (new_disk == conf->raid_disks - 1) {
> > -				new_disk = wonly_disk;
> > -				break;
> > -			}
> > -		}
> > -		goto rb_out;
> > +		choose_first = 1;
> > +		start_disk = 0;
> > +	} else {
> > +		choose_first = 0;
> > +		start_disk = conf->last_used;
> >  	}
> >  
> 
> 
> perhaps you can drop the else when you init with
> choose_first = 0;
> start_disk = conf->last_used;

Perhaps.  Though given the 'retry' loop it isn't obvious that would do the
right thing.
I think I'll keep this bit as-is.  I think it helps see to two cases more
clearly.



> > +		if (r1_bio->bios[disk] == IO_BLOCKED
> > +		    || !(rdev = rcu_dereference(conf->mirrors[disk].rdev))
> > +		    || !test_bit(In_sync, &rdev->flags))
> > +			continue;
> 
> i think it is more readable to use:
> 
>   rdev = rcu_dereference(conf->mirrors[disk].rdev);
>   if ()
> 

I think assignments inside 'if' statements have their place, but it seems
that this is far from universal.  I've made this change, thanks.

> 
> 
> > +		if (test_bit(WriteMostly, &rdev->flags)) {
> > +			new_disk = disk;
> > +			continue;
> >  		}
> > +		new_disk = disk;
> > +		break;
> >  	}
> 
> to improve readability:
> 
> 	new_disk = disk;
> 	if ( ! test_bit(WriteMostly, &rdev->flags) )
> 		break;

Yes, that is a distinct improvement.  I've made that change too.

Thanks a lot for your review!!

NeilBrown