Re: Re: [PATCH] md/raid5:Choose to replacing or recoverying when raid degraded and had a want_replacement disk at the same time.

[majianpeng <majianpeng@gmail.com>]

>On Wed, 6 Jun 2012 11:24:34 +0800 majianpeng <majianpeng@gmail.com> wrote:
>
>> On  Tue, 5 Jun 2012 18:28:13 neil wrote:
>> >On Tue, 5 Jun 2012 15:32:56 +0800 majianpeng <majianpeng@gmail.com> wrote:
>> >
>> >> In Commit 7bfec5f35c68121e7b1849f3f4166dd96c8da5b3:
>> >> "if there is a spare and a want_replacement device, start replacement."
>> >> But it did not consider the raid was degraded at the same time.
>> >> When we add spare disk in order to recovery, unless raid was ok and then
>> >> started replacement or vice versa.
>> >> 
>> >> Signed-off-by: majianpeng <majianpeng@gmail.com>
>> >> ---
>> >>  drivers/md/raid5.c |   48 +++++++++++++++++++++++++++++-------------------
>> >>  1 files changed, 29 insertions(+), 19 deletions(-)
>> >> 
>> >> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> >> index d267672..f74c9a5 100644
>> >> --- a/drivers/md/raid5.c
>> >> +++ b/drivers/md/raid5.c
>> >
>> >Good point, but the code feels a little ... clumsy.
>> >
>> >How about this?
>> >
>> >NeilBrown
>> >
>> >
>> >diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> >index d267672..4f0861e 100644
>> >--- a/drivers/md/raid5.c
>> >+++ b/drivers/md/raid5.c
>> >@@ -5465,10 +5465,9 @@ static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev)
>> > 	if (rdev->saved_raid_disk >= 0 &&
>> > 	    rdev->saved_raid_disk >= first &&
>> > 	    conf->disks[rdev->saved_raid_disk].rdev == NULL)
>> >-		disk = rdev->saved_raid_disk;
>> >-	else
>> >-		disk = first;
>> >-	for ( ; disk <= last ; disk++) {
>> >+		first = rdev->saved_raid_disk;
>> >+
>> >+	for (disk = first; disk <= last; disk++) {
>> > 		p = conf->disks + disk;
>> > 		if (p->rdev == NULL) {
>> > 			clear_bit(In_sync, &rdev->flags);
>> >@@ -5477,8 +5476,10 @@ static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev)
>> > 			if (rdev->saved_raid_disk != disk)
>> > 				conf->fullsync = 1;
>> > 			rcu_assign_pointer(p->rdev, rdev);
>> >-			break;
>> >+			goto out;
>> > 		}
>> >+	}
>> >+	for (disk = first; disk <= last; disk++) {
>> > 		if (test_bit(WantReplacement, &p->rdev->flags) &&
>> > 		    p->replacement == NULL) {
>> > 			clear_bit(In_sync, &rdev->flags);
>> >@@ -5490,6 +5491,7 @@ static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev)
>> > 			break;
>> > 		}
>> > 	}
>> >+out:
>> > 	print_raid5_conf(conf);
>> > 	return err;
>> > }
>> >
>> >
>> I tested and found a bug.I corrected it like this.
>
>You've added a test for 'p->rdev != NULL' - is that all?
>
No, I also add 
>> +       for (disk = first; disk <= last; disk++) {
 +               p = conf->disks + disk;
>> +               if (p->rdev != NULL &&
>> +                       test_bit(WantReplacement, &p->rdev->flags) &&
>> +                       p->replacement == NULL) {
You lost :
 p = conf->disks + disk;
in next loop.
													Thanks!
>That isn't necessary.  If any p->rdev were NULL then the first loop would
>find it and the second loop would never be entered.
>
>
>> But I had a question:why  p->rdev not protect by rcu_read_lock?
>> I think it should be.
>
>rcu is not necessary here.  We hold mddev->mutex as does the code which
>removes devices, so we cannot race with it.  We only need rcu when not
>holding the mutex, and when not performing resync/recovery/etc as that
>prevents ->rdev from being removed too.
>
>Thanks,
>NeilBrown
>
>
>
>> 
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index d267672..24162c1 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -5465,10 +5465,9 @@ static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev)
>>         if (rdev->saved_raid_disk >= 0 &&
>>             rdev->saved_raid_disk >= first &&
>>             conf->disks[rdev->saved_raid_disk].rdev == NULL)
>> -               disk = rdev->saved_raid_disk;
>> -       else
>> -               disk = first;
>> -       for ( ; disk <= last ; disk++) {
>> +               first = rdev->saved_raid_disk;
>> +
>> +       for (disk = first; disk <= last; disk++) {
>>                 p = conf->disks + disk;
>>                 if (p->rdev == NULL) {
>>                         clear_bit(In_sync, &rdev->flags);
>> @@ -5477,10 +5476,15 @@ static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev)
>>                         if (rdev->saved_raid_disk != disk)
>>                                 conf->fullsync = 1;
>>                         rcu_assign_pointer(p->rdev, rdev);
>> -                       break;
>> +                       goto out;
>>                 }
>> -               if (test_bit(WantReplacement, &p->rdev->flags) &&
>> -                   p->replacement == NULL) {
>> +       }
>> +
>> +       for (disk = first; disk <= last; disk++) {
>> +               p = conf->disks + disk;
>> +               if (p->rdev != NULL &&
>> +                       test_bit(WantReplacement, &p->rdev->flags) &&
>> +                       p->replacement == NULL) {
>>                         clear_bit(In_sync, &rdev->flags);
>>                         set_bit(Replacement, &rdev->flags);
>>                         rdev->raid_disk = disk;
>> @@ -5490,6 +5494,7 @@ static int raid5_add_disk(struct mddev *mddev, struct md_rdev *rdev)
>>                         break;
>>                 }
>>         }
>> +out:
>>         print_raid5_conf(conf);
>>         return err;
>>  }
>> 
>
>


--------------
majianpeng