[PATCH 1/4] md/raid5: Reduce one write-operation when handle badsector

[PATCH 1/4] md/raid5: Reduce one write-operation when handle badsector

[majianpeng]

For badsector,it took two steps:rewrite and reread.So for
write-operation,it make no sense to rewrite after write-operation.

Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
---
 drivers/md/raid5.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 19d77a0..59c0569 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -1921,8 +1921,10 @@ static void raid5_end_write_request(struct bio *bi, int error)
 					&rdev->mddev->recovery);
 		} else if (is_badblock(rdev, sh->sector,
 				       STRIPE_SECTORS,
-				       &first_bad, &bad_sectors))
+				       &first_bad, &bad_sectors)) {
 			set_bit(R5_MadeGood, &sh->dev[i].flags);
+			set_bit(R5_ReWrite, &sh->dev[i].flags);
+		}
 	}
 	rdev_dec_pending(rdev, conf->mddev);
 
-- 
1.7.9.5

Re: [PATCH 1/4] md/raid5: Reduce one write-operation when handle badsector

[NeilBrown]

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

On Thu, 28 Feb 2013 15:51:01 +0800 majianpeng <majianpeng@gmail.com> wrote:

> For badsector,it took two steps:rewrite and reread.So for
> write-operation,it make no sense to rewrite after write-operation.

Please please please please please try to explain yourself better.

This patch doesn't make any sense.  The write succeeded.  We set the MadeGood
flag so that later code will remove the block from the bad-block list.
There will be no re-read or re-write.

NeilBrown


> 
> Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
> ---
>  drivers/md/raid5.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 19d77a0..59c0569 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -1921,8 +1921,10 @@ static void raid5_end_write_request(struct bio *bi, int error)
>  					&rdev->mddev->recovery);
>  		} else if (is_badblock(rdev, sh->sector,
>  				       STRIPE_SECTORS,
> -				       &first_bad, &bad_sectors))
> +				       &first_bad, &bad_sectors)) {
>  			set_bit(R5_MadeGood, &sh->dev[i].flags);
> +			set_bit(R5_ReWrite, &sh->dev[i].flags);
> +		}
>  	}
>  	rdev_dec_pending(rdev, conf->mddev);
>  


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

Re: Re: [PATCH 1/4] md/raid5: Reduce one write-operation when handle badsector

[majianpeng]

>On Thu, 28 Feb 2013 15:51:01 +0800 majianpeng <majianpeng@gmail.com> wrote:
>
>> For badsector,it took two steps:rewrite and reread.So for
>> write-operation,it make no sense to rewrite after write-operation.
>
>Please please please please please try to explain yourself better.
>
Sorry for that. 
>This patch doesn't make any sense.  The write succeeded.  We set the MadeGood
>flag so that later code will remove the block from the bad-block list.
>There will be no re-read or re-write.
>
For a striep which contain badsector is doing write-operation.
In analyse_stripe, it check badsector and set R5_ReadError.
 write--->MadeGood->clear badblock log. But it can't clear R5_ReadError flag.
So it will exec those code in func handle_stripe:
>        /* If the failed drives are just a ReadError, then we might need
>         * to progress the repair/check process
>         */
>        if (s.failed <= conf->max_degraded && !conf->mddev->ro)
>                for (i = 0; i < s.failed; i++) {
>                        struct r5dev *dev = &sh->dev[s.failed_num[i]];
>                        if (test_bit(R5_ReadError, &dev->flags)
>                            && !test_bit(R5_LOCKED, &dev->flags)
>                            && test_bit(R5_UPTODATE, &dev->flags)
>                                ) {  
>                                if (!test_bit(R5_ReWrite, &dev->flags)) {
>                                        set_bit(R5_Wantwrite, &dev->flags);
>                                        set_bit(R5_ReWrite, &dev->flags);
>                                        set_bit(R5_LOCKED, &dev->flags);
>                                        s.locked++;
>                                } else {
>                                        /* let's read it back */
>                                        set_bit(R5_Wantread, &dev->flags);
>                                       set_bit(R5_LOCKED, &dev->flags);
>                                       s.locked++;
>                                }    
>                        }    
>                }    

But the flag ReWrite can't set. So it do write-operatoin again.Then do read-operation.

Thanks!
Jianpeng Ma
>NeilBrown
>
>
>> 
>> Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
>> ---
>>  drivers/md/raid5.c |    4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index 19d77a0..59c0569 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -1921,8 +1921,10 @@ static void raid5_end_write_request(struct bio *bi, int error)
>>  					&rdev->mddev->recovery);
>>  		} else if (is_badblock(rdev, sh->sector,
>>  				       STRIPE_SECTORS,
>> -				       &first_bad, &bad_sectors))
>> +				       &first_bad, &bad_sectors)) {
>>  			set_bit(R5_MadeGood, &sh->dev[i].flags);
>> +			set_bit(R5_ReWrite, &sh->dev[i].flags);
>> +		}
>>  	}
>>  	rdev_dec_pending(rdev, conf->mddev);
>>  
>
>

Re: [PATCH 1/4] md/raid5: Reduce one write-operation when handle badsector

[NeilBrown]

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

On Mon, 4 Mar 2013 09:52:57 +0800 majianpeng <majianpeng@gmail.com> wrote:

> >On Thu, 28 Feb 2013 15:51:01 +0800 majianpeng <majianpeng@gmail.com> wrote:
> >
> >> For badsector,it took two steps:rewrite and reread.So for
> >> write-operation,it make no sense to rewrite after write-operation.
> >
> >Please please please please please try to explain yourself better.
> >
> Sorry for that. 
> >This patch doesn't make any sense.  The write succeeded.  We set the MadeGood
> >flag so that later code will remove the block from the bad-block list.
> >There will be no re-read or re-write.
> >
> For a striep which contain badsector is doing write-operation.
> In analyse_stripe, it check badsector and set R5_ReadError.
>  write--->MadeGood->clear badblock log. But it can't clear R5_ReadError flag.
> So it will exec those code in func handle_stripe:

After you wrote that, did you read it again to see if it makes any sense?  It
does but only just.

I'd rather have a good explanation in the code so it is clear what is going
on.

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 3e0ec16..277d9c2 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -1911,8 +1911,15 @@ static void raid5_end_write_request(struct bio *bi, int error)
 					&rdev->mddev->recovery);
 		} else if (is_badblock(rdev, sh->sector,
 				       STRIPE_SECTORS,
-				       &first_bad, &bad_sectors))
+				       &first_bad, &bad_sectors)) {
 			set_bit(R5_MadeGood, &sh->dev[i].flags);
+			if (test_bit(R5_ReadError, &sh->dev[i].flags))
+				/* That was a successful write so make
+				 * sure it looks like we already did
+				 * a re-write.
+				 */
+				set_bit(R5_ReWrite, &sh->dev[i].flags);
+		}
 	}
 	rdev_dec_pending(rdev, conf->mddev);
 
Thanks,
NeilBrown

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

Re: Re: [PATCH 1/4] md/raid5: Reduce one write-operation when handle badsector

[majianpeng]

>On Mon, 4 Mar 2013 09:52:57 +0800 majianpeng <majianpeng@gmail.com> wrote:
>
>> >On Thu, 28 Feb 2013 15:51:01 +0800 majianpeng <majianpeng@gmail.com> wrote:
>> >
>> >> For badsector,it took two steps:rewrite and reread.So for
>> >> write-operation,it make no sense to rewrite after write-operation.
>> >
>> >Please please please please please try to explain yourself better.
>> >
>> Sorry for that. 
>> >This patch doesn't make any sense.  The write succeeded.  We set the MadeGood
>> >flag so that later code will remove the block from the bad-block list.
>> >There will be no re-read or re-write.
>> >
>> For a striep which contain badsector is doing write-operation.
>> In analyse_stripe, it check badsector and set R5_ReadError.
>>  write--->MadeGood->clear badblock log. But it can't clear R5_ReadError flag.
>> So it will exec those code in func handle_stripe:
>
>After you wrote that, did you read it again to see if it makes any sense?  It
>does but only just.
>
>I'd rather have a good explanation in the code so it is clear what is going
>on.
>
>diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>index 3e0ec16..277d9c2 100644
>--- a/drivers/md/raid5.c
>+++ b/drivers/md/raid5.c
>@@ -1911,8 +1911,15 @@ static void raid5_end_write_request(struct bio *bi, int error)
> 					&rdev->mddev->recovery);
> 		} else if (is_badblock(rdev, sh->sector,
> 				       STRIPE_SECTORS,
>-				       &first_bad, &bad_sectors))
>+				       &first_bad, &bad_sectors)) {
> 			set_bit(R5_MadeGood, &sh->dev[i].flags);
>+			if (test_bit(R5_ReadError, &sh->dev[i].flags))
Why do it test R5_ReadError?
Even if  it didn't set R5_ReadError, it also do in analyse_stripe.

Thanks!
Jianpeng Ma
>+				/* That was a successful write so make
>+				 * sure it looks like we already did
>+				 * a re-write.
>+				 */
>+				set_bit(R5_ReWrite, &sh->dev[i].flags);
>+		}
> 	}
> 	rdev_dec_pending(rdev, conf->mddev);
> 
>Thanks,
>NeilBrown
>