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

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 4974 bytes --]

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?

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;
>  }
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]