On Mon, 06 Sep 2010 09:43:32 +0200
walter harms [off-list ref] wrote:

Neil Brown schrieb: 

quoted

I've taken the opportunity to substantially re-write that code.

Comments?

Thanks,
NeilBrown

commit e4062735c8f7233923df5858ed20f1278f3ee669
Author: NeilBrown [off-list ref]
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 [off-list ref]
    Signed-off-by: NeilBrown [off-list ref]

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.

quoted

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

quoted

+		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