2006-03-20 19:22:09

by Alasdair G Kergon

[permalink] [raw]
Subject: dm: bio split bvec fix

The code that handles bios that span table target boundaries by breaking
them up into smaller bios will not split an individual struct bio_vec
into more than two pieces. Sometimes more than that are required.

This patch adds a loop to break the second piece up into as many
pieces as are necessary.

Cc: "Abhishek Gupta" <[email protected]>
Cc: Dan Smith <[email protected]>
Signed-Off-By: Alasdair G Kergon <[email protected]>

Index: linux-2.6.16-rc5/drivers/md/dm.c
===================================================================
--- linux-2.6.16-rc5.orig/drivers/md/dm.c 2006-03-20 16:00:11.000000000 +0000
+++ linux-2.6.16-rc5/drivers/md/dm.c 2006-03-20 18:38:19.000000000 +0000
@@ -573,30 +573,35 @@ static void __clone_and_map(struct clone

} else {
/*
- * Create two copy bios to deal with io that has
- * been split across a target.
+ * Handle a bvec that must be split between two or more targets.
*/
struct bio_vec *bv = bio->bi_io_vec + ci->idx;
+ sector_t remaining = to_sector(bv->bv_len);
+ unsigned int offset = 0;

- clone = split_bvec(bio, ci->sector, ci->idx,
- bv->bv_offset, max);
- __map_bio(ti, clone, tio);
-
- ci->sector += max;
- ci->sector_count -= max;
- ti = dm_table_find_target(ci->map, ci->sector);
-
- len = to_sector(bv->bv_len) - max;
- clone = split_bvec(bio, ci->sector, ci->idx,
- bv->bv_offset + to_bytes(max), len);
- tio = alloc_tio(ci->md);
- tio->io = ci->io;
- tio->ti = ti;
- memset(&tio->info, 0, sizeof(tio->info));
- __map_bio(ti, clone, tio);
+ do {
+ if (offset) {
+ ti = dm_table_find_target(ci->map, ci->sector);
+ max = max_io_len(ci->md, ci->sector, ti);
+
+ tio = alloc_tio(ci->md);
+ tio->io = ci->io;
+ tio->ti = ti;
+ memset(&tio->info, 0, sizeof(tio->info));
+ }
+
+ len = (max > remaining) ? remaining : max;
+
+ clone = split_bvec(bio, ci->sector, ci->idx,
+ bv->bv_offset + offset, len);
+
+ __map_bio(ti, clone, tio);
+
+ ci->sector += len;
+ ci->sector_count -= len;
+ offset += to_bytes(len);
+ } while (remaining -= len);

- ci->sector += len;
- ci->sector_count -= len;
ci->idx++;
}
}


2006-03-20 22:11:46

by Andrew Morton

[permalink] [raw]
Subject: Re: dm: bio split bvec fix

Alasdair G Kergon <[email protected]> wrote:
>
> The code that handles bios that span table target boundaries by breaking
> them up into smaller bios will not split an individual struct bio_vec
> into more than two pieces. Sometimes more than that are required.
>
> This patch adds a loop to break the second piece up into as many
> pieces as are necessary.

Should this go into 2.6.16.1?

2006-03-22 11:32:36

by Jens Axboe

[permalink] [raw]
Subject: Re: dm: bio split bvec fix

On Mon, Mar 20 2006, Alasdair G Kergon wrote:
> The code that handles bios that span table target boundaries by breaking
> them up into smaller bios will not split an individual struct bio_vec
> into more than two pieces. Sometimes more than that are required.
>
> This patch adds a loop to break the second piece up into as many
> pieces as are necessary.

Why isn't this just handled in the merge callback? Can a single page bio
span > 2 targets?

--
Jens Axboe

2006-03-22 12:20:08

by Alasdair G Kergon

[permalink] [raw]
Subject: Re: [dm-devel] Re: dm: bio split bvec fix

On Wed, Mar 22, 2006 at 12:32:35PM +0100, Jens Axboe wrote:
> Why isn't this just handled in the merge callback? Can a single page bio
> span > 2 targets?

Yes. (Unit of size if the sector - and things don't have to
be aligned nicely, just aligned to sector.)

IIRC the merge function assumes the number of bytes that can
be added is only a function of the offset: but in our case
it's also a function of time. To make this work it should
reserve those bytes with device-mapper, and guarantee either to
supply them to us subsequently (and preferably quickly) or to
cancel that reservation. Device-mapper for its part would
guarantee to accept the bio without needing to split it.
Or dm could have a rejection mechanism that refuses bios
that are too big (because the max number of bytes we accept
got reduced between the initial call and the bio actually being
presented) and they go back and get processed again.

Alasdair
--
[email protected]

2006-03-22 12:56:31

by Jens Axboe

[permalink] [raw]
Subject: Re: [dm-devel] Re: dm: bio split bvec fix

On Wed, Mar 22 2006, Alasdair G Kergon wrote:
> On Wed, Mar 22, 2006 at 12:32:35PM +0100, Jens Axboe wrote:
> > Why isn't this just handled in the merge callback? Can a single page bio
> > span > 2 targets?
>
> Yes. (Unit of size if the sector - and things don't have to
> be aligned nicely, just aligned to sector.)
>
> IIRC the merge function assumes the number of bytes that can
> be added is only a function of the offset: but in our case
> it's also a function of time. To make this work it should
> reserve those bytes with device-mapper, and guarantee either to
> supply them to us subsequently (and preferably quickly) or to
> cancel that reservation. Device-mapper for its part would
> guarantee to accept the bio without needing to split it.
> Or dm could have a rejection mechanism that refuses bios
> that are too big (because the max number of bytes we accept
> got reduced between the initial call and the bio actually being
> presented) and they go back and get processed again.

Ok, thanks for the followup explanation, I guess there's no way around
that then.

--
Jens Axboe

2006-03-22 13:08:31

by Jens Axboe

[permalink] [raw]
Subject: Re: [dm-devel] Re: dm: bio split bvec fix

On Wed, Mar 22 2006, Alasdair G Kergon wrote:
> On Wed, Mar 22, 2006 at 12:32:35PM +0100, Jens Axboe wrote:
> > Why isn't this just handled in the merge callback? Can a single page bio
> > span > 2 targets?
>
> Yes. (Unit of size if the sector - and things don't have to
> be aligned nicely, just aligned to sector.)
>
> IIRC the merge function assumes the number of bytes that can
> be added is only a function of the offset: but in our case
> it's also a function of time. To make this work it should
> reserve those bytes with device-mapper, and guarantee either to
> supply them to us subsequently (and preferably quickly) or to
> cancel that reservation. Device-mapper for its part would
> guarantee to accept the bio without needing to split it.
> Or dm could have a rejection mechanism that refuses bios
> that are too big (because the max number of bytes we accept
> got reduced between the initial call and the bio actually being
> presented) and they go back and get processed again.
>
> Alasdair
> --
> [email protected]
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

--
Jens Axboe