md/raid1:If r1bio->sectors % 8 != 0,then the memcmp and later memcpy will omit the last bio_vec.

md/raid1:If r1bio->sectors % 8 != 0,then the memcmp and later memcpy will omit the last bio_vec.

[majianpeng]

From 72e5ad9da511689f7efe330ce5093a3df2c92738 Mon Sep 17 00:00:00 2001
From: majianpeng <majianpeng@gmail.com>
Date: Sat, 31 Mar 2012 15:36:01 +0800
Subject: [PATCH] md/raid1:If r1bio->sectors % 8 != 0,then the memcmp and
 later memcpy will omit the last bio_vec.


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

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 4a40a20..11a28ca 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1711,7 +1711,7 @@ static int process_checks(struct r1bio *r1_bio)
 	struct mddev *mddev = r1_bio->mddev;
 	struct r1conf *conf = mddev->private;
 	int primary;
-	int i;
+	int i, vcnt;
 
 	for (primary = 0; primary < conf->raid_disks * 2; primary++)
 		if (r1_bio->bios[primary]->bi_end_io == end_sync_read &&
@@ -1721,9 +1721,9 @@ static int process_checks(struct r1bio *r1_bio)
 			break;
 		}
 	r1_bio->read_disk = primary;
+	vcnt = (r1_bio->sectors + PAGE_SIZE / 512 - 1) >> (PAGE_SHIFT - 9);
 	for (i = 0; i < conf->raid_disks * 2; i++) {
 		int j;
-		int vcnt = r1_bio->sectors >> (PAGE_SHIFT- 9);
 		struct bio *pbio = r1_bio->bios[primary];
 		struct bio *sbio = r1_bio->bios[i];
 		int size;
@@ -1736,9 +1736,16 @@ static int process_checks(struct r1bio *r1_bio)
 				struct page *p, *s;
 				p = pbio->bi_io_vec[j].bv_page;
 				s = sbio->bi_io_vec[j].bv_page;
+				if ((j == vcnt - 1) &&
+					(r1_bio->sectors &
+					 (PAGE_SIZE / 512 - 1)))
+					size = (r1_bio->sectors << 9)
+						- (PAGE_SIZE * j);
+				else
+					size = PAGE_SIZE;
 				if (memcmp(page_address(p),
 					   page_address(s),
-					   PAGE_SIZE))
+					   size))
 					break;
 			}
 		} else
@@ -2480,7 +2487,6 @@ static sector_t sync_request(struct mddev *mddev, sector_t sector_nr, int *skipp
 	} while (r1_bio->bios[disk]->bi_vcnt < RESYNC_PAGES);
  bio_full:
 	r1_bio->sectors = nr_sectors;
-
 	/* For a user-requested sync, we read all readable devices and do a
 	 * compare
 	 */
-- 
1.7.5.4

 				
--------------
majianpeng
2012-03-31

Re: md/raid1:If r1bio->sectors % 8 != 0,then the memcmp and later memcpy will omit the last bio_vec.

[NeilBrown]

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

On Sat, 31 Mar 2012 15:44:14 +0800 "majianpeng" <majianpeng@gmail.com> wrote:

> >From 72e5ad9da511689f7efe330ce5093a3df2c92738 Mon Sep 17 00:00:00 2001
> From: majianpeng <majianpeng@gmail.com>
> Date: Sat, 31 Mar 2012 15:36:01 +0800
> Subject: [PATCH] md/raid1:If r1bio->sectors % 8 != 0,then the memcmp and
>  later memcpy will omit the last bio_vec.

Thanks for this.

There is a bug here. However I think your fix is overly complex.

Also the 'memcpy' doesn't need to be changed - it doesn't hurt if it copies
more bytes than necessary.
However it could hurt if we compare more bytes than necessary.

I've applied a very different patch as below.

Thanks,
NeilBrown


From 9d49d24d665146d121d3ae349adc92e83460d75d Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.de>
Date: Mon, 2 Apr 2012 11:39:05 +1000
Subject: [PATCH] md/raid1,raid10: don't compare excess byte during
 consistency check.

When comparing two pages read from different legs of a mirror, only
compare the bytes that were read, not the whole page.

I most cases were read a whole page, but in some cases with
bad blocks or odd sizes devices we might read fewer than that.

This bug has been present "forever" but at worst it might cause
a report of two many mismatches and generate a little bit
extra resync IO, so there is no need to back-port to -stable
kernels.

Reported-by: majianpeng <majianpeng@gmail.com>
Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 8c420f1..d35e4c9 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1738,7 +1738,7 @@ static int process_checks(struct r1bio *r1_bio)
 				s = sbio->bi_io_vec[j].bv_page;
 				if (memcmp(page_address(p),
 					   page_address(s),
-					   PAGE_SIZE))
+					   sbio->bi_io_vec[j].bv_len))
 					break;
 			}
 		} else
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 3540316..fff7821 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1821,7 +1821,7 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
 			for (j = 0; j < vcnt; j++)
 				if (memcmp(page_address(fbio->bi_io_vec[j].bv_page),
 					   page_address(tbio->bi_io_vec[j].bv_page),
-					   PAGE_SIZE))
+					   fbio->bi_io_vec[j].bv_len))
 					break;
 			if (j == vcnt)
 				continue;



> 
> 
> Signed-off-by: majianpeng <majianpeng@gmail.com>
> ---
>  drivers/md/raid1.c |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 4a40a20..11a28ca 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1711,7 +1711,7 @@ static int process_checks(struct r1bio *r1_bio)
>  	struct mddev *mddev = r1_bio->mddev;
>  	struct r1conf *conf = mddev->private;
>  	int primary;
> -	int i;
> +	int i, vcnt;
>  
>  	for (primary = 0; primary < conf->raid_disks * 2; primary++)
>  		if (r1_bio->bios[primary]->bi_end_io == end_sync_read &&
> @@ -1721,9 +1721,9 @@ static int process_checks(struct r1bio *r1_bio)
>  			break;
>  		}
>  	r1_bio->read_disk = primary;
> +	vcnt = (r1_bio->sectors + PAGE_SIZE / 512 - 1) >> (PAGE_SHIFT - 9);
>  	for (i = 0; i < conf->raid_disks * 2; i++) {
>  		int j;
> -		int vcnt = r1_bio->sectors >> (PAGE_SHIFT- 9);
>  		struct bio *pbio = r1_bio->bios[primary];
>  		struct bio *sbio = r1_bio->bios[i];
>  		int size;
> @@ -1736,9 +1736,16 @@ static int process_checks(struct r1bio *r1_bio)
>  				struct page *p, *s;
>  				p = pbio->bi_io_vec[j].bv_page;
>  				s = sbio->bi_io_vec[j].bv_page;
> +				if ((j == vcnt - 1) &&
> +					(r1_bio->sectors &
> +					 (PAGE_SIZE / 512 - 1)))
> +					size = (r1_bio->sectors << 9)
> +						- (PAGE_SIZE * j);
> +				else
> +					size = PAGE_SIZE;
>  				if (memcmp(page_address(p),
>  					   page_address(s),
> -					   PAGE_SIZE))
> +					   size))
>  					break;
>  			}
>  		} else
> @@ -2480,7 +2487,6 @@ static sector_t sync_request(struct mddev *mddev, sector_t sector_nr, int *skipp
>  	} while (r1_bio->bios[disk]->bi_vcnt < RESYNC_PAGES);
>   bio_full:
>  	r1_bio->sectors = nr_sectors;
> -
>  	/* For a user-requested sync, we read all readable devices and do a
>  	 * compare
>  	 */


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

Re: Re: md/raid1:If r1bio->sectors % 8 != 0,then the memcmp and latermemcpy will omit the last bio_vec.

[kedacomkernel]

I readed your patch and found it wrong.My patch had two point
1:memcmp probem.You patch can correct it.
2:omit the last bio_vec.You patch did not correct it.
If r1bio->sectors %8 != 0,supposed sectors = 18,then bio cover three pages. 
But:
	>>int vcnt = r1_bio->sectors >> (PAGE_SHIFT- 9); vcnt=2
then
    >>for (j = vcnt; j-- ; )  
      j = 1, 0


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

-------------------------------------------------------------
发件人：NeilBrown
发送日期：2012-04-02 09:41:29
收件人：majianpeng
抄送：linux-raid
主题：Re: md/raid1:If r1bio->sectors % 8 != 0,then the memcmp and  latermemcpy will omit the last bio_vec.

On Sat, 31 Mar 2012 15:44:14 +0800 "majianpeng" <majianpeng@gmail.com> wrote:

> >From 72e5ad9da511689f7efe330ce5093a3df2c92738 Mon Sep 17 00:00:00 2001
> From: majianpeng <majianpeng@gmail.com>
> Date: Sat, 31 Mar 2012 15:36:01 +0800
> Subject: [PATCH] md/raid1:If r1bio->sectors % 8 != 0,then the memcmp and
>  later memcpy will omit the last bio_vec.

Thanks for this.

There is a bug here. However I think your fix is overly complex.

Also the 'memcpy' doesn't need to be changed - it doesn't hurt if it copies
more bytes than necessary.
However it could hurt if we compare more bytes than necessary.

I've applied a very different patch as below.

Thanks,
NeilBrown


From 9d49d24d665146d121d3ae349adc92e83460d75d Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.de>
Date: Mon, 2 Apr 2012 11:39:05 +1000
Subject: [PATCH] md/raid1,raid10: don't compare excess byte during
 consistency check.

When comparing two pages read from different legs of a mirror, only
compare the bytes that were read, not the whole page.

I most cases were read a whole page, but in some cases with
bad blocks or odd sizes devices we might read fewer than that.

This bug has been present "forever" but at worst it might cause
a report of two many mismatches and generate a little bit
extra resync IO, so there is no need to back-port to -stable
kernels.

Reported-by: majianpeng <majianpeng@gmail.com>
Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 8c420f1..d35e4c9 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1738,7 +1738,7 @@ static int process_checks(struct r1bio *r1_bio)
 				s = sbio->bi_io_vec[j].bv_page;
 				if (memcmp(page_address(p),
 					   page_address(s),
-					   PAGE_SIZE))
+					   sbio->bi_io_vec[j].bv_len))
 					break;
 			}
 		} else
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 3540316..fff7821 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1821,7 +1821,7 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio)
 			for (j = 0; j < vcnt; j++)
 				if (memcmp(page_address(fbio->bi_io_vec[j].bv_page),
 					   page_address(tbio->bi_io_vec[j].bv_page),
-					   PAGE_SIZE))
+					   fbio->bi_io_vec[j].bv_len))
 					break;
 			if (j == vcnt)
 				continue;



> 
> 
> Signed-off-by: majianpeng <majianpeng@gmail.com>
> ---
>  drivers/md/raid1.c |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 4a40a20..11a28ca 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1711,7 +1711,7 @@ static int process_checks(struct r1bio *r1_bio)
>  	struct mddev *mddev = r1_bio->mddev;
>  	struct r1conf *conf = mddev->private;
>  	int primary;
> -	int i;
> +	int i, vcnt;
>  
>  	for (primary = 0; primary < conf->raid_disks * 2; primary++)
>  		if (r1_bio->bios[primary]->bi_end_io == end_sync_read &&
> @@ -1721,9 +1721,9 @@ static int process_checks(struct r1bio *r1_bio)
>  			break;
>  		}
>  	r1_bio->read_disk = primary;
> +	vcnt = (r1_bio->sectors + PAGE_SIZE / 512 - 1) >> (PAGE_SHIFT - 9);
>  	for (i = 0; i < conf->raid_disks * 2; i++) {
>  		int j;
> -		int vcnt = r1_bio->sectors >> (PAGE_SHIFT- 9);
>  		struct bio *pbio = r1_bio->bios[primary];
>  		struct bio *sbio = r1_bio->bios[i];
>  		int size;
> @@ -1736,9 +1736,16 @@ static int process_checks(struct r1bio *r1_bio)
>  				struct page *p, *s;
>  				p = pbio->bi_io_vec[j].bv_page;
>  				s = sbio->bi_io_vec[j].bv_page;
> +				if ((j == vcnt - 1) &&
> +					(r1_bio->sectors &
> +					 (PAGE_SIZE / 512 - 1)))
> +					size = (r1_bio->sectors << 9)
> +						- (PAGE_SIZE * j);
> +				else
> +					size = PAGE_SIZE;
>  				if (memcmp(page_address(p),
>  					   page_address(s),
> -					   PAGE_SIZE))
> +					   size))
>  					break;
>  			}
>  		} else
> @@ -2480,7 +2487,6 @@ static sector_t sync_request(struct mddev *mddev, sector_t sector_nr, int *skipp
>  	} while (r1_bio->bios[disk]->bi_vcnt < RESYNC_PAGES);
>   bio_full:
>  	r1_bio->sectors = nr_sectors;
> -
>  	/* For a user-requested sync, we read all readable devices and do a
>  	 * compare
>  	 */

Re: md/raid1:If r1bio->sectors % 8 != 0,then the memcmp and latermemcpy will omit the last bio_vec.

[NeilBrown]

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

On Thu, 5 Apr 2012 14:12:25 +0800 "kedacomkernel" <kedacomkernel@gmail.com>
wrote:

> I readed your patch and found it wrong.My patch had two point
> 1:memcmp probem.You patch can correct it.
> 2:omit the last bio_vec.You patch did not correct it.
> If r1bio->sectors %8 != 0,supposed sectors = 18,then bio cover three pages. 
> But:
> 	>>int vcnt = r1_bio->sectors >> (PAGE_SHIFT- 9); vcnt=2
> then
>     >>for (j = vcnt; j-- ; )  
>       j = 1, 0
> 

Ahhh, yes of course.
Thanks.

> > @@ -1711,7 +1711,7 @@ static int process_checks(struct r1bio *r1_bio)
> >  	struct mddev *mddev = r1_bio->mddev;
> >  	struct r1conf *conf = mddev->private;
> >  	int primary;
> > -	int i;
> > +	int i, vcnt;
> >  
> >  	for (primary = 0; primary < conf->raid_disks * 2; primary++)
> >  		if (r1_bio->bios[primary]->bi_end_io == end_sync_read &&
> > @@ -1721,9 +1721,9 @@ static int process_checks(struct r1bio *r1_bio)
> >  			break;
> >  		}
> >  	r1_bio->read_disk = primary;
> > +	vcnt = (r1_bio->sectors + PAGE_SIZE / 512 - 1) >> (PAGE_SHIFT - 9);
> >  	for (i = 0; i < conf->raid_disks * 2; i++) {
> >  		int j;
> > -		int vcnt = r1_bio->sectors >> (PAGE_SHIFT- 9);
> >  		struct bio *pbio = r1_bio->bios[primary];
> >  		struct bio *sbio = r1_bio->bios[i];
> >  		int size;

This bit.

I'll make sure that gets applied.

Thanks,
NeilBrown



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