On Wed, 6 Jun 2012 11:24:34 +0800 majianpeng <majianpeng@xxxxxxxxx> wrote:
> On Tue, 5 Jun 2012 18:28:13 neil wrote:
> >On Tue, 5 Jun 2012 15:32:56 +0800 majianpeng <majianpeng@xxxxxxxxx> 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@xxxxxxxxx>
> >> ---
> >> 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:
signature.asc
Description: PGP signature
