md/raid1:Fix a logic bug in fix_sync_read_error().

md/raid1:Fix a logic bug in fix_sync_read_error().

[kedacomkernel]

From 0fe15c8e1bd5e46234d37573f3322312d8da325d Mon Sep 17 00:00:00 2001
From: majianpeng <majianpeng@gmail.com>
Date: Sat, 31 Mar 2012 10:27:33 +0800
Subject: [PATCH] md/raid1:Fix a logic bug in fix_sync_read_error(). 
If  d==read_disk && success == 1 and then break, so d =
 read_disk. When exec this judgement: >>start = d; >>/*
 write it back and re-read */ >>while (d !=
 r1_bio->read_disk) { Because d == read_disk,so write and
 re-add did not exec.


Signed-off-by: majianpeng <majianpeng@gmail.com>
---
 drivers/md/raid1.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 4a40a20..3a133ff 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1618,7 +1618,6 @@ static int fix_sync_read_error(struct r1bio *r1_bio)
 						 bio->bi_io_vec[idx].bv_page,
 						 READ, false)) {
 					success = 1;
-					break;
 				}
 			}
 			d++;
-- 
1.7.5.4

kedacomkernel
2012-03-31

Re: md/raid1:Fix a logic bug in fix_sync_read_error().

[NeilBrown]

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

On Sat, 31 Mar 2012 10:38:01 +0800 "kedacomkernel" <kedacomkernel@gmail.com>
wrote:

> >From 0fe15c8e1bd5e46234d37573f3322312d8da325d Mon Sep 17 00:00:00 2001
> From: majianpeng <majianpeng@gmail.com>
> Date: Sat, 31 Mar 2012 10:27:33 +0800
> Subject: [PATCH] md/raid1:Fix a logic bug in fix_sync_read_error(). 
> If  d==read_disk && success == 1 and then break, so d =
>  read_disk. When exec this judgement: >>start = d; >>/*
>  write it back and re-read */ >>while (d !=
>  r1_bio->read_disk) { Because d == read_disk,so write and
>  re-add did not exec.

That is correct, and that is how it should be.

If we get a read error, then try again and get a successful read, then there
is nothing more that we need to do.  No need to write or anything.

So: current code is correct.

Thanks,
NeilBrown


> 
> 
> Signed-off-by: majianpeng <majianpeng@gmail.com>
> ---
>  drivers/md/raid1.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 4a40a20..3a133ff 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1618,7 +1618,6 @@ static int fix_sync_read_error(struct r1bio *r1_bio)
>  						 bio->bi_io_vec[idx].bv_page,
>  						 READ, false)) {
>  					success = 1;
> -					break;
>  				}
>  			}
>  			d++;


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

Re: Re: md/raid1:Fix a logic bug in fix_sync_read_error().

[kedacomkernel]

>>That is correct, and that is how it should be.

>>If we get a read error, then try again and get a successful read, then there
>>is nothing more that we need to do.  No need to write or anything.

>>So: current code is correct.

>>Thanks,
>>NeilBrown
Sorry Neil,I don't understand what's your mean?
I think this function is want to do something like follow when all sync_read failed.
1:want to read partly by PAGE_SIZE from all read devices.
2:if read success and write write to other and read back, in order to repair those disks.

for example :
disk A,B ,read 64k from 0 to 128 sector.
supposed: disk A read failed because sector 0-7 was bad sector.
			   disk B  read failed because sector 8-15 was bad sector.
by this function,we can fix sector 0-7 of A and sector 8-15 of B.
Is ok?

If B is read_disk,so read 0-7 is ok and want to write it to A.But becasue the code ,do not write to disk A.

------------------				 
kedacomkernel
2012-04-05

-------------------------------------------------------------
发件人：NeilBrown
发送日期：2012-04-02 10:05:34
收件人：kedacomkernel
抄送：linux-raid
主题：Re: md/raid1:Fix a logic bug in fix_sync_read_error().

On Sat, 31 Mar 2012 10:38:01 +0800 "kedacomkernel" <kedacomkernel@gmail.com>
wrote:

> >From 0fe15c8e1bd5e46234d37573f3322312d8da325d Mon Sep 17 00:00:00 2001
> From: majianpeng <majianpeng@gmail.com>
> Date: Sat, 31 Mar 2012 10:27:33 +0800
> Subject: [PATCH] md/raid1:Fix a logic bug in fix_sync_read_error(). 
> If  d==read_disk && success == 1 and then break, so d =
>  read_disk. When exec this judgement: >>start = d; >>/*
>  write it back and re-read */ >>while (d !=
>  r1_bio->read_disk) { Because d == read_disk,so write and
>  re-add did not exec.

That is correct, and that is how it should be.

If we get a read error, then try again and get a successful read, then there
is nothing more that we need to do.  No need to write or anything.

So: current code is correct.

Thanks,
NeilBrown


> 
> 
> Signed-off-by: majianpeng <majianpeng@gmail.com>
> ---
>  drivers/md/raid1.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 4a40a20..3a133ff 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1618,7 +1618,6 @@ static int fix_sync_read_error(struct r1bio *r1_bio)
>  						 bio->bi_io_vec[idx].bv_page,
>  						 READ, false)) {
>  					success = 1;
> -					break;
>  				}
>  			}
>  			d++;

Re: md/raid1:Fix a logic bug in fix_sync_read_error().

[NeilBrown]

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

On Thu, 5 Apr 2012 10:23:47 +0800 "kedacomkernel" <kedacomkernel@gmail.com>
wrote:

> >>That is correct, and that is how it should be.
> 
> >>If we get a read error, then try again and get a successful read, then there
> >>is nothing more that we need to do.  No need to write or anything.
> 
> >>So: current code is correct.
> 
> >>Thanks,
> >>NeilBrown
> Sorry Neil,I don't understand what's your mean?
> I think this function is want to do something like follow when all sync_read failed.
> 1:want to read partly by PAGE_SIZE from all read devices.

Not exactly.  fix_sync_read_error() only gets called if all reads that were
attempted (often just one, but for 'check' or 'repair' it will have tried all
devicess) have failed.
So what we need to do in this case is just to get the valid data from
somewhere.  As soon as we have found it we can be happy.

> 2:if read success and write write to other and read back, in order to repair those disks.
 
yes ... and no.  The code might be a bit confusing.  There may even be room
for improvement.

But after fix_sync_read_error succeeds, the rest of sync_request_write will
write out the data that it recovered to other devices if necessary.

so it doesn't matter that fix_sync_read_error() doesn't write out.

> 
> for example :
> disk A,B ,read 64k from 0 to 128 sector.
> supposed: disk A read failed because sector 0-7 was bad sector.
> 			   disk B  read failed because sector 8-15 was bad sector.
> by this function,we can fix sector 0-7 of A and sector 8-15 of B.
> Is ok?

Hmm... not sure.
fix_sync_read_error should read all the data from wherever it is.
I guess I'm not certain that sync_request_write() will write it out
everywhere.

So maybe there is a problem there.

However you should make sure that your analysis covered both
fix_sync_read_error and sync_request_write.

NeilBrown



> 
> If B is read_disk,so read 0-7 is ok and want to write it to A.But becasue the code ,do not write to disk A.
> 
> ------------------				 
> kedacomkernel
> 2012-04-05
> 
> -------------------------------------------------------------
> 发件人：NeilBrown
> 发送日期：2012-04-02 10:05:34
> 收件人：kedacomkernel
> 抄送：linux-raid
> 主题：Re: md/raid1:Fix a logic bug in fix_sync_read_error().
> 
> On Sat, 31 Mar 2012 10:38:01 +0800 "kedacomkernel" <kedacomkernel@gmail.com>
> wrote:
> 
> > >From 0fe15c8e1bd5e46234d37573f3322312d8da325d Mon Sep 17 00:00:00 2001
> > From: majianpeng <majianpeng@gmail.com>
> > Date: Sat, 31 Mar 2012 10:27:33 +0800
> > Subject: [PATCH] md/raid1:Fix a logic bug in fix_sync_read_error(). 
> > If  d==read_disk && success == 1 and then break, so d =
> >  read_disk. When exec this judgement: >>start = d; >>/*
> >  write it back and re-read */ >>while (d !=
> >  r1_bio->read_disk) { Because d == read_disk,so write and
> >  re-add did not exec.
> 
> That is correct, and that is how it should be.
> 
> If we get a read error, then try again and get a successful read, then there
> is nothing more that we need to do.  No need to write or anything.
> 
> So: current code is correct.
> 
> Thanks,
> NeilBrown
> 
> 
> > 
> > 
> > Signed-off-by: majianpeng <majianpeng@gmail.com>
> > ---
> >  drivers/md/raid1.c |    1 -
> >  1 files changed, 0 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> > index 4a40a20..3a133ff 100644
> > --- a/drivers/md/raid1.c
> > +++ b/drivers/md/raid1.c
> > @@ -1618,7 +1618,6 @@ static int fix_sync_read_error(struct r1bio *r1_bio)
> >  						 bio->bi_io_vec[idx].bv_page,
> >  						 READ, false)) {
> >  					success = 1;
> > -					break;
> >  				}
> >  			}
> >  			d++;
> 


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