[PATCH]bcache : limit the bio max sectors to make request bug in raid0

[PATCH]bcache : limit the bio max sectors to make request bug in raid0

[Jack Wang]

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

We are using your bcache-testing branch.

From 20ad8cfb8047df2d09a5a960610f02c555a31a4f Mon Sep 17 00:00:00 2001
From: Jack Wang <jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
Date: Tue, 16 Apr 2013 14:59:04 +0200
Subject: [PATCH] limit the max sectors in bcache to fix the make request bug
 in raid10
During test bcache with raid1+0, we saw a lot of complain as below:
[ 2766.555172] md/raid0:md400: make_request bug: can't convert block
across chunks or bigger than 512k 953328 144

when the using dd or fio with bigger blocksize like 512k, limited the
bio_max_sectors resolve this issue.

Reported-by: Michael Balser <michael.balser-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
Signed-off-by: Jack Wang <jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
Tested-by: Dongsu Park <dongsu.park-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
---
 drivers/md/bcache/io.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/bcache/io.c b/drivers/md/bcache/io.c
index 9b63065..c5c36f7 100644
--- a/drivers/md/bcache/io.c
+++ b/drivers/md/bcache/io.c
@@ -181,7 +181,7 @@ static unsigned bch_bio_max_sectors(struct bio *bio)
 	ret = min(ret, queue_max_sectors(q));

 	WARN_ON(!ret);
-	ret = max_t(int, ret, bio_iovec(bio)->bv_len >> 9);
+	ret = min_t(int, ret, bio_iovec(bio)->bv_len >> 9);

 	return ret;
 }
-- 
1.7.9.5

[-- Attachment #2: 0001-limit-the-max-sectors-in-bcache-to-fix-the-make-requ.patch --]
[-- Type: text/x-patch, Size: 1303 bytes --]

From 20ad8cfb8047df2d09a5a960610f02c555a31a4f Mon Sep 17 00:00:00 2001
From: Jack Wang <jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
Date: Tue, 16 Apr 2013 14:59:04 +0200
Subject: [PATCH] limit the max sectors in bcache to fix the make request bug
 in raid10
During test bcache with raid1+0, we saw a lot of complain as below:
[ 2766.555172] md/raid0:md400: make_request bug: can't convert block across chunks or bigger than 512k 953328 144

when the using dd or fio with bigger blocksize like 512k, limited the bio_max_sectors resolve this issue.

Reported-by: Michael Balser <michael.balser-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
Signed-off-by: Jack Wang <jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
Tested-by: Dongsu Park <dongsu.park-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
---
 drivers/md/bcache/io.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/bcache/io.c b/drivers/md/bcache/io.c
index 9b63065..c5c36f7 100644
--- a/drivers/md/bcache/io.c
+++ b/drivers/md/bcache/io.c
@@ -181,7 +181,7 @@ static unsigned bch_bio_max_sectors(struct bio *bio)
 	ret = min(ret, queue_max_sectors(q));
 
 	WARN_ON(!ret);
-	ret = max_t(int, ret, bio_iovec(bio)->bv_len >> 9);
+	ret = min_t(int, ret, bio_iovec(bio)->bv_len >> 9);
 
 	return ret;
 }
-- 
1.7.9.5

Re: [PATCH]bcache : limit the bio max sectors to make request bug in raid0

[Kent Overstreet]

On Wed, Apr 17, 2013 at 09:36:11AM +0200, Jack Wang wrote:
> We are using your bcache-testing branch.
> 
> From 20ad8cfb8047df2d09a5a960610f02c555a31a4f Mon Sep 17 00:00:00 2001
> From: Jack Wang <jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
> Date: Tue, 16 Apr 2013 14:59:04 +0200
> Subject: [PATCH] limit the max sectors in bcache to fix the make request bug
>  in raid10
> During test bcache with raid1+0, we saw a lot of complain as below:
> [ 2766.555172] md/raid0:md400: make_request bug: can't convert block
> across chunks or bigger than 512k 953328 144
> 
> when the using dd or fio with bigger blocksize like 512k, limited the
> bio_max_sectors resolve this issue.

The fix is wrong - it'll have the effect of limiting _every_ bio bcache
emits to a single page, which will be bad for performance.

I think I just found the problem though - looking at the raid0
merge_bvec_fn, when it's stacked on top of devices with their own
merge_bvec_fns, it reuses the bvm was passed to it and modifies the
bi_bdev and bi_sector. Ew.

Can you try this patch and see if it fixes it?

commit a09ded8edf9ed4009930713e101249084cbcea5c
Author: Kent Overstreet <koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Date:   Mon Apr 22 14:44:24 2013 -0700

    bcache: Fix merge_bvec_fn usage for when it modifies the bvm
    
    Stacked md devices reuse the bvm for the subordinate device, causing
    problems...
    
    Reported-by: Michael Balser <michael.balser-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
    Signed-off-by: Kent Overstreet <koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

diff --git a/drivers/md/bcache/io.c b/drivers/md/bcache/io.c
index 5304eaa..48efd4d 100644
--- a/drivers/md/bcache/io.c
+++ b/drivers/md/bcache/io.c
@@ -163,13 +163,6 @@ static unsigned bch_bio_max_sectors(struct bio *bio)
 	struct bio_vec *bv, *end = bio_iovec(bio) +
 		min_t(int, bio_segments(bio), max_segments);
 
-	struct bvec_merge_data bvm = {
-		.bi_bdev	= bio->bi_bdev,
-		.bi_sector	= bio->bi_sector,
-		.bi_size	= 0,
-		.bi_rw		= bio->bi_rw,
-	};
-
 	if (bio->bi_rw & REQ_DISCARD)
 		return min(ret, q->limits.max_discard_sectors);
 
@@ -178,12 +171,18 @@ static unsigned bch_bio_max_sectors(struct bio *bio)
 		ret = 0;
 
 		for (bv = bio_iovec(bio); bv < end; bv++) {
+			struct bvec_merge_data bvm = {
+				.bi_bdev	= bio->bi_bdev,
+				.bi_sector	= bio->bi_sector,
+				.bi_size	= ret << 9,
+				.bi_rw		= bio->bi_rw,
+			};
+
 			if (q->merge_bvec_fn &&
 			    q->merge_bvec_fn(q, &bvm, bv) < (int) bv->bv_len)
 				break;
 
-			ret		+= bv->bv_len >> 9;
-			bvm.bi_size	+= bv->bv_len;
+			ret += bv->bv_len >> 9;
 		}
 	}
 

Re: [PATCH]bcache : limit the bio max sectors to make request bug in raid0

[Jack Wang]

On 2013年04月22日 23:46, Kent Overstreet wrote:
> On Wed, Apr 17, 2013 at 09:36:11AM +0200, Jack Wang wrote:
>> We are using your bcache-testing branch.
>>
>> From 20ad8cfb8047df2d09a5a960610f02c555a31a4f Mon Sep 17 00:00:00 2001
>> From: Jack Wang <jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
>> Date: Tue, 16 Apr 2013 14:59:04 +0200
>> Subject: [PATCH] limit the max sectors in bcache to fix the make request bug
>>  in raid10
>> During test bcache with raid1+0, we saw a lot of complain as below:
>> [ 2766.555172] md/raid0:md400: make_request bug: can't convert block
>> across chunks or bigger than 512k 953328 144
>>
>> when the using dd or fio with bigger blocksize like 512k, limited the
>> bio_max_sectors resolve this issue.
> 
> The fix is wrong - it'll have the effect of limiting _every_ bio bcache
> emits to a single page, which will be bad for performance.
> 
> I think I just found the problem though - looking at the raid0
> merge_bvec_fn, when it's stacked on top of devices with their own
> merge_bvec_fns, it reuses the bvm was passed to it and modifies the
> bi_bdev and bi_sector. Ew.
> 
> Can you try this patch and see if it fixes it?
Sure, thanks for looking into it, will test it today.
> 
> commit a09ded8edf9ed4009930713e101249084cbcea5c
> Author: Kent Overstreet <koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Date:   Mon Apr 22 14:44:24 2013 -0700
> 
>     bcache: Fix merge_bvec_fn usage for when it modifies the bvm
>     
>     Stacked md devices reuse the bvm for the subordinate device, causing
>     problems...
>     
>     Reported-by: Michael Balser <michael.balser-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
>     Signed-off-by: Kent Overstreet <koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> 
> diff --git a/drivers/md/bcache/io.c b/drivers/md/bcache/io.c
> index 5304eaa..48efd4d 100644
> --- a/drivers/md/bcache/io.c
> +++ b/drivers/md/bcache/io.c
> @@ -163,13 +163,6 @@ static unsigned bch_bio_max_sectors(struct bio *bio)
>  	struct bio_vec *bv, *end = bio_iovec(bio) +
>  		min_t(int, bio_segments(bio), max_segments);
>  
> -	struct bvec_merge_data bvm = {
> -		.bi_bdev	= bio->bi_bdev,
> -		.bi_sector	= bio->bi_sector,
> -		.bi_size	= 0,
> -		.bi_rw		= bio->bi_rw,
> -	};
> -
>  	if (bio->bi_rw & REQ_DISCARD)
>  		return min(ret, q->limits.max_discard_sectors);
>  
> @@ -178,12 +171,18 @@ static unsigned bch_bio_max_sectors(struct bio *bio)
>  		ret = 0;
>  
>  		for (bv = bio_iovec(bio); bv < end; bv++) {
> +			struct bvec_merge_data bvm = {
> +				.bi_bdev	= bio->bi_bdev,
> +				.bi_sector	= bio->bi_sector,
> +				.bi_size	= ret << 9,
> +				.bi_rw		= bio->bi_rw,
> +			};
> +
>  			if (q->merge_bvec_fn &&
>  			    q->merge_bvec_fn(q, &bvm, bv) < (int) bv->bv_len)
>  				break;
>  
> -			ret		+= bv->bv_len >> 9;
> -			bvm.bi_size	+= bv->bv_len;
> +			ret += bv->bv_len >> 9;
>  		}
>  	}
>  
> 

Re: [PATCH]bcache : limit the bio max sectors to make request bug in raid0

[Jack Wang]

On 04/23/2013 10:28 AM, Jinpu Wang wrote:
> Hi Kent,
> 
> I tested your patch today, it works well, and you're right, my patch
> limits every BIO in a single page:(
> 
> Thanks for your patch and share.
> 
> Regards,
> Jack
> 
> 
> On Tue, Apr 23, 2013 at 7:12 AM, Jack Wang <jinpu.wang@profitbricks.com
> <mailto:jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>> wrote:
> 
>     On 2013年04月22日 23:46, Kent Overstreet wrote:
>     > On Wed, Apr 17, 2013 at 09:36:11AM +0200, Jack Wang wrote:
>     >> We are using your bcache-testing branch.
>     >>
>     >> From 20ad8cfb8047df2d09a5a960610f02c555a31a4f Mon Sep 17 00:00:00
>     2001
>     >> From: Jack Wang <jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org
>     <mailto:jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>>
>     >> Date: Tue, 16 Apr 2013 14:59:04 +0200
>     >> Subject: [PATCH] limit the max sectors in bcache to fix the make
>     request bug
>     >>  in raid10
>     >> During test bcache with raid1+0, we saw a lot of complain as below:
>     >> [ 2766.555172] md/raid0:md400: make_request bug: can't convert block
>     >> across chunks or bigger than 512k 953328 144
>     >>
>     >> when the using dd or fio with bigger blocksize like 512k, limited the
>     >> bio_max_sectors resolve this issue.
>     >
>     > The fix is wrong - it'll have the effect of limiting _every_ bio
>     bcache
>     > emits to a single page, which will be bad for performance.
>     >
>     > I think I just found the problem though - looking at the raid0
>     > merge_bvec_fn, when it's stacked on top of devices with their own
>     > merge_bvec_fns, it reuses the bvm was passed to it and modifies the
>     > bi_bdev and bi_sector. Ew.
>     >
>     > Can you try this patch and see if it fixes it?
>     Sure, thanks for looking into it, will test it today.
>     >
>     > commit a09ded8edf9ed4009930713e101249084cbcea5c
>     > Author: Kent Overstreet <koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org
>     <mailto:koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>>
>     > Date:   Mon Apr 22 14:44:24 2013 -0700
>     >
>     >     bcache: Fix merge_bvec_fn usage for when it modifies the bvm
>     >
>     >     Stacked md devices reuse the bvm for the subordinate device,
>     causing
>     >     problems...
>     >
>     >     Reported-by: Michael Balser <michael.balser-EIkl63zCoXbZz8UjPX3odA@public.gmane.orgm
>     <mailto:michael.balser-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>>
>     >     Signed-off-by: Kent Overstreet <koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org
>     <mailto:koverstreet-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>>
>     >
>     > diff --git a/drivers/md/bcache/io.c b/drivers/md/bcache/io.c
>     > index 5304eaa..48efd4d 100644
>     > --- a/drivers/md/bcache/io.c
>     > +++ b/drivers/md/bcache/io.c
>     > @@ -163,13 +163,6 @@ static unsigned bch_bio_max_sectors(struct
>     bio *bio)
>     >       struct bio_vec *bv, *end = bio_iovec(bio) +
>     >               min_t(int, bio_segments(bio), max_segments);
>     >
>     > -     struct bvec_merge_data bvm = {
>     > -             .bi_bdev        = bio->bi_bdev,
>     > -             .bi_sector      = bio->bi_sector,
>     > -             .bi_size        = 0,
>     > -             .bi_rw          = bio->bi_rw,
>     > -     };
>     > -
>     >       if (bio->bi_rw & REQ_DISCARD)
>     >               return min(ret, q->limits.max_discard_sectors);
>     >
>     > @@ -178,12 +171,18 @@ static unsigned bch_bio_max_sectors(struct
>     bio *bio)
>     >               ret = 0;
>     >
>     >               for (bv = bio_iovec(bio); bv < end; bv++) {
>     > +                     struct bvec_merge_data bvm = {
>     > +                             .bi_bdev        = bio->bi_bdev,
>     > +                             .bi_sector      = bio->bi_sector,
>     > +                             .bi_size        = ret << 9,
>     > +                             .bi_rw          = bio->bi_rw,
>     > +                     };
>     > +
>     >                       if (q->merge_bvec_fn &&
>     >                           q->merge_bvec_fn(q, &bvm, bv) < (int)
>     bv->bv_len)
>     >                               break;
>     >
>     > -                     ret             += bv->bv_len >> 9;
>     > -                     bvm.bi_size     += bv->bv_len;
>     > +                     ret += bv->bv_len >> 9;
>     >               }
>     >       }
>     >
>     >
> 
> 
> 
> 
> -- 
> 
> Mit freundlichen Grüßen,
> Best Regards,
> 
> Jack Wang
> 
> Linux Kernel Developer Storage
> ProfitBricks GmbH  The IaaS-Company.
> 
> Beschreibung: Beschreibung: Beschreibung:
> cid:part1.00030702.00050402-EIkl63zCoXaH+58JC4qpiA@public.gmane.org
> 
> *ProfitBricks GmbH*
> Greifswalder Str. 207
> D - 10405 Berlin
> Tel: +49 30 6098 56991-308
> Fax: +49 30 6098 56992-203
> Email: jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org <http://profitbricks.com>
> URL: http://www.profitbricks.com <http://www.profitbricks.com/>
> 
> Sitz der Gesellschaft: Berlin.
> Registergericht: Amtsgericht Charlottenburg, HRB 125506 B.
> Geschäftsführer: Andreas Gauger, Achim Weiss.