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

PATCH In review Jack Wang 2 participants 4 messages Apr 17, 2013 – Apr 23, 2013 3 messages not in the archive bcache (block layer cache)

lore

Share & tools

lore

Jack Wang

author

2013-04-17 07:36 UTC #0 lore raw

PATCH 1/1 bcache : limit the bio max sectors to make request bug in raid0

We are using your bcache-testing branch.

From 20ad8cfb8047df2d09a5a960610f02c555a31a4f Mon Sep 17 00:00:00 2001
From: Jack Wang <[email protected]>
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 <[email protected]>

Signed-off-by Jack Wang <[email protected]>

Tested-by Dongsu Park <[email protected]>

---
 drivers/md/bcache/io.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/md/bcache/io.c +1 -1

index 9b63065..c5c36f7 100644
@@ -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;
 }

Signature · 1 line

1.7.9.5

[ 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 <[email protected]>

Signed-off-by Jack Wang <[email protected]>

Tested-by Dongsu Park <[email protected]>

---
 drivers/md/bcache/io.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/md/bcache/io.c +1 -1

index 9b63065..c5c36f7 100644
@@ -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;
 }

Signature · 1 line

1.7.9.5

Kent Overstreet

2013-04-22 21:46 UTC #2 lore raw

In reply to a message not in the archive

Quoted 8 lines · Jack Wang

We are using your bcache-testing branch.
From 20ad8cfb8047df2d09a5a960610f02c555a31a4f Mon Sep 17 00:00:00 2001
From: Jack Wang <[email protected]>
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:

Quoted 5 lines

[ 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 <[email protected]>
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 <[email protected]>

Signed-off-by Kent Overstreet <[email protected]>

drivers/md/bcache/io.c +8 -9

index 5304eaa..48efd4d 100644
@@ -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;
 		}
 	}

Jack Wang

author

2013-04-23 05:12 UTC #3 lore raw

Quoted 18 lines · Kent Overstreet

Quoted 8 lines · Jack Wang
We are using your bcache-testing branch.
From 20ad8cfb8047df2d09a5a960610f02c555a31a4f Mon Sep 17 00:00:00 2001
From: Jack Wang <[email protected]>
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:
Quoted 5 lines
[ 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.

Quoted 6 lines

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.

Quoted 53 lines

commit a09ded8edf9ed4009930713e101249084cbcea5c

Author: Kent Overstreet <[email protected]>
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 <[email protected]>

Signed-off-by Kent Overstreet <[email protected]>

drivers/md/bcache/io.c +8 -9

index 5304eaa..48efd4d 100644
@@ -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;
 		}
 	}

Jack Wang

author

2013-04-23 08:30 UTC #6 lore raw

In reply to a message not in the archive

Quoted 132 lines · Jinpu Wang

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 <[email protected] <mailto:[email protected]>> 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 <[email protected]
    <mailto:[email protected]>>
    >> 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 <[email protected]
    <mailto:[email protected]>>
    > 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 <[email protected]
    <mailto:[email protected]>>
    >     Signed-off-by: Kent Overstreet <[email protected]
    <mailto:[email protected]>>
    >
    > 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;
    >               }
    >       }
    >
    >

Signature · 22 lines

Mit freundlichen Grüßen,
Best Regards,

Jack Wang

Linux Kernel Developer Storage
ProfitBricks GmbH  The IaaS-Company.

Beschreibung: Beschreibung: Beschreibung:
cid:[email protected]

*ProfitBricks GmbH*
Greifswalder Str. 207
D - 10405 Berlin
Tel: +49 30 6098 56991-308
Fax: +49 30 6098 56992-203
Email: [email protected] <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.