On Mon, 4 Mar 2013 10:24:16 +0800 majianpeng <majianpeng@xxxxxxxxx> wrote:
> >On Thu, 28 Feb 2013 15:50:37 +0800 majianpeng <majianpeng@xxxxxxxxx> wrote:
> >
> >> Replacement is a fullsync which don't depent on bitmap.So regardless of
> >> the presence and absence of bitmap, it do full resync.
> >> If offset of normal io is larger than offset of resync,it will write
> >> again when resync arrived this offset.
> >
> >This might be OK for RAID1 and RAID10 as recover is paused when writes
> >happen, but that is not the case for RAID5, so it isn't safe to test against
> >curr_resync - it gets updated a bit too later.
> >
> ->curr_resync + STRIPE_SECTORS is the next stripe which willbe replaced.
> How about the ->curr_resync+STRIPE_SECTORS?
If you aren't certain, then neither am I.
As resync and normal writes can be intermingled you would need some guarantee
that a write wouldn't be missed, and that almost certainly means a test under
a lock against some value which is updated under a lock.
>
> >Also you messed up the formatting in raid10.c
> >
> Can you explain in detail?
>> if (rrdev && (test_bit(Faulty, &rrdev->flags)
>> - || test_bit(Unmerged, &rrdev->flags)))
>> + || test_bit(Unmerged, &rrdev->flags) ||
>> + (test_bit(Replacement, &rrdev->flags) &&
>> + conf->mddev->curr_resync < r10_bio->sector)))
Text that is inside parentheses (or other brackets) should never be to the
left of the opening parenthesis unless that parenthesis is at the end of a
line.
In the original code, the "|| test_bit(Unmerged....." was to the right of the
'('. In your version it starts to the left, and the lines you added also
start to the left.
> >I'm not convinced this optimisation is really worth it.
> >
> Maybe for HDD disk, it only improve speed by reducing some write operation.
> But for ssd disk, it can reduce one write.
Still doesn't sound convincing.
NeilBrown
>
> Thanks!
> Jianpeng Ma
> >NeilBrown
> >
> >
> >>
> >> Signed-off-by: Jianpeng Ma <majianpeng@xxxxxxxxx>
> >> ---
> >> drivers/md/raid1.c | 4 +++-
> >> drivers/md/raid10.c | 4 +++-
> >> drivers/md/raid5.c | 3 ++-
> >> 3 files changed, 8 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> >> index d5bddfc..142a5fa 100644
> >> --- a/drivers/md/raid1.c
> >> +++ b/drivers/md/raid1.c
> >> @@ -1173,7 +1173,9 @@ read_again:
> >> set_bit(R1BIO_Degraded, &r1_bio->state);
> >> continue;
> >> }
> >> -
> >> + if (test_bit(Replacement, &rdev->flags) &&
> >> + conf->mddev->curr_resync < r1_bio->sector)
> >> + continue;
> >> atomic_inc(&rdev->nr_pending);
> >> if (test_bit(WriteErrorSeen, &rdev->flags)) {
> >> sector_t first_bad;
> >> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> >> index 64d4824..bb11cfb 100644
> >> --- a/drivers/md/raid10.c
> >> +++ b/drivers/md/raid10.c
> >> @@ -1337,7 +1337,9 @@ retry_write:
> >> || test_bit(Unmerged, &rdev->flags)))
> >> rdev = NULL;
> >> if (rrdev && (test_bit(Faulty, &rrdev->flags)
> >> - || test_bit(Unmerged, &rrdev->flags)))
> >> + || test_bit(Unmerged, &rrdev->flags) ||
> >> + (test_bit(Replacement, &rrdev->flags) &&
> >> + conf->mddev->curr_resync < r10_bio->sector)))
> >> rrdev = NULL;
> >>
> >> r10_bio->devs[i].bio = NULL;
> >> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> >> index bd49623..e0a2a39 100644
> >> --- a/drivers/md/raid5.c
> >> +++ b/drivers/md/raid5.c
> >> @@ -602,7 +602,8 @@ static void ops_run_io(struct stripe_head *sh, struct stripe_head_state *s)
> >> rdev = NULL;
> >> if (rdev)
> >> atomic_inc(&rdev->nr_pending);
> >> - if (rrdev && test_bit(Faulty, &rrdev->flags))
> >> + if (rrdev && (test_bit(Faulty, &rrdev->flags) ||
> >> + conf->mddev->curr_resync < sh->sector))
> >> rrdev = NULL;
> >> if (rrdev)
> >> atomic_inc(&rrdev->nr_pending);
> >
> >
Attachment:
signature.asc
Description: PGP signature
