Check memory allocations when cloning bio's.
--- linux-2.5.53a/drivers/md/dm.c Mon Dec 23 23:21:04 2002
+++ linux-2.5.53b/drivers/md/dm.c Fri Dec 27 14:50:29 2002
@@ -394,6 +393,10 @@
*/
clone = clone_bio(bio, ci->sector, ci->idx,
bio->bi_vcnt - ci->idx, ci->sector_count);
+ if (!clone) {
+ dec_pending(ci->io, -ENOMEM);
+ return;
+ }
__map_bio(ti, clone, ci->io);
ci->sector_count = 0;
@@ -417,6 +420,10 @@
}
clone = clone_bio(bio, ci->sector, ci->idx, i - ci->idx, len);
+ if (!clone) {
+ dec_pending(ci->io, -ENOMEM);
+ return;
+ }
__map_bio(ti, clone, ci->io);
ci->sector += len;
On Fri, Dec 27, 2002 at 04:55:31PM -0500, Kevin Corry wrote:
> Check memory allocations when cloning bio's.
>
> --- linux-2.5.53a/drivers/md/dm.c Mon Dec 23 23:21:04 2002
> +++ linux-2.5.53b/drivers/md/dm.c Fri Dec 27 14:50:29 2002
> @@ -394,6 +393,10 @@
> */
> clone = clone_bio(bio, ci->sector, ci->idx,
> bio->bi_vcnt - ci->idx, ci->sector_count);
> + if (!clone) {
> + dec_pending(ci->io, -ENOMEM);
> + return;
> + }
> __map_bio(ti, clone, ci->io);
> ci->sector_count = 0;
>
> @@ -417,6 +420,10 @@
> }
>
> clone = clone_bio(bio, ci->sector, ci->idx, i - ci->idx, len);
> + if (!clone) {
> + dec_pending(ci->io, -ENOMEM);
> + return;
> + }
> __map_bio(ti, clone, ci->io);
>
> ci->sector += len;
This seems a bit insufficient. Why is this error not propagated up
through to __split_bio ?
Jeff
> On Fri, Dec 27, 2002 at 04:55:31PM -0500, Kevin Corry wrote:
> > Check memory allocations when cloning bio's.
> >
> > --- linux-2.5.53a/drivers/md/dm.c Mon Dec 23 23:21:04 2002
> > +++ linux-2.5.53b/drivers/md/dm.c Fri Dec 27 14:50:29 2002
> > @@ -394,6 +393,10 @@
> > */
> > clone = clone_bio(bio, ci->sector, ci->idx,
> > bio->bi_vcnt - ci->idx, ci->sector_count);
> > + if (!clone) {
> > + dec_pending(ci->io, -ENOMEM);
> > + return;
> > + }
> > __map_bio(ti, clone, ci->io);
> > ci->sector_count = 0;
> >
> > @@ -417,6 +420,10 @@
> > }
> >
> > clone = clone_bio(bio, ci->sector, ci->idx, i - ci->idx, len);
> > + if (!clone) {
> > + dec_pending(ci->io, -ENOMEM);
> > + return;
> > + }
> > __map_bio(ti, clone, ci->io);
> >
> > ci->sector += len;
>
>
> This seems a bit insufficient. Why is this error not propagated up
> through to __split_bio ?
>
> Jeff
At second glance, it does seem insufficient. I had simply copied the
same check that was already used later in __clone_and_map().
What we should do is set ci->sector_count to zero. This will prevent
__split_bio() from performing any further processing on the bio.
Alternatively, we could change __clone_and_map() to return an int, and
return the error to __split_bio() and let it do the cleanup.
Here is a replacement patch based on the first suggestion.
-Kevin
--- linux-2.5.53a/drivers/md/dm.c Mon Dec 23 23:21:04 2002
+++ linux-2.5.53b/drivers/md/dm.c Fri Dec 27 15:18:19 2002
@@ -394,6 +393,11 @@
*/
clone = clone_bio(bio, ci->sector, ci->idx,
bio->bi_vcnt - ci->idx, ci->sector_count);
+ if (!clone) {
+ ci->sector_count = 0;
+ dec_pending(ci->io, -ENOMEM);
+ return;
+ }
__map_bio(ti, clone, ci->io);
ci->sector_count = 0;
@@ -417,6 +421,11 @@
}
clone = clone_bio(bio, ci->sector, ci->idx, i - ci->idx, len);
+ if (!clone) {
+ ci->sector_count = 0;
+ dec_pending(ci->io, -ENOMEM);
+ return;
+ }
__map_bio(ti, clone, ci->io);
ci->sector += len;
@@ -433,6 +442,7 @@
clone = split_bvec(bio, ci->sector, ci->idx,
bv->bv_offset, max);
if (!clone) {
+ ci->sector_count = 0;
dec_pending(ci->io, -ENOMEM);
return;
}
@@ -447,6 +457,7 @@
clone = split_bvec(bio, ci->sector, ci->idx,
bv->bv_offset + to_bytes(max), len);
if (!clone) {
+ ci->sector_count = 0;
dec_pending(ci->io, -ENOMEM);
return;
}
On Fri, Dec 27, 2002 at 04:55:31PM -0500, Kevin Corry wrote:
> Check memory allocations when cloning bio's.
Rejected, clone_bio() cannot fail since it's allocating from a mempool
with __GFP_WAIT set.
- Joe
>
> On Fri, Dec 27, 2002 at 04:55:31PM -0500, Kevin Corry wrote:
> > Check memory allocations when cloning bio's.
>
> Rejected, clone_bio() cannot fail since it's allocating from a mempool
> with __GFP_WAIT set.
>
> - Joe
Hmm. Yep. I must have mistaken GFP_NOIO for GFP_ATOMIC.
But based on that reasoning, shouldn't the bio_alloc() call
in split_bvec() always return a valid bio, and hence make the
checks in split_bvec() and __clone_and_map() unnecessary?
Kevin
On Tue, Dec 31, 2002 at 11:22:28AM -0500, Kevin Corry wrote:
> >
> > On Fri, Dec 27, 2002 at 04:55:31PM -0500, Kevin Corry wrote:
> > > Check memory allocations when cloning bio's.
> >
> > Rejected, clone_bio() cannot fail since it's allocating from a mempool
> > with __GFP_WAIT set.
> >
> > - Joe
>
> Hmm. Yep. I must have mistaken GFP_NOIO for GFP_ATOMIC.
>
> But based on that reasoning, shouldn't the bio_alloc() call
> in split_bvec() always return a valid bio, and hence make the
> checks in split_bvec() and __clone_and_map() unnecessary?
y, I was mistakenly under the impression that bio_alloc could fail
during the allocation of the bvec. I'll add the following patch.
- Joe
--- diff/drivers/md/dm.c 2002-12-30 11:40:30.000000000 +0000
+++ source/drivers/md/dm.c 2003-01-02 09:51:07.000000000 +0000
@@ -347,18 +347,15 @@
struct bio_vec *bv = bio->bi_io_vec + idx;
clone = bio_alloc(GFP_NOIO, 1);
+ memcpy(clone->bi_io_vec, bv, sizeof(*bv));
- if (clone) {
- memcpy(clone->bi_io_vec, bv, sizeof(*bv));
-
- clone->bi_sector = sector;
- clone->bi_bdev = bio->bi_bdev;
- clone->bi_rw = bio->bi_rw;
- clone->bi_vcnt = 1;
- clone->bi_size = to_bytes(len);
- clone->bi_io_vec->bv_offset = offset;
- clone->bi_io_vec->bv_len = clone->bi_size;
- }
+ clone->bi_sector = sector;
+ clone->bi_bdev = bio->bi_bdev;
+ clone->bi_rw = bio->bi_rw;
+ clone->bi_vcnt = 1;
+ clone->bi_size = to_bytes(len);
+ clone->bi_io_vec->bv_offset = offset;
+ clone->bi_io_vec->bv_len = clone->bi_size;
return clone;
}
@@ -432,11 +429,6 @@
clone = split_bvec(bio, ci->sector, ci->idx,
bv->bv_offset, max);
- if (!clone) {
- dec_pending(ci->io, -ENOMEM);
- return;
- }
-
__map_bio(ti, clone, ci->io);
ci->sector += max;
@@ -446,11 +438,6 @@
len = to_sector(bv->bv_len) - max;
clone = split_bvec(bio, ci->sector, ci->idx,
bv->bv_offset + to_bytes(max), len);
- if (!clone) {
- dec_pending(ci->io, -ENOMEM);
- return;
- }
-
__map_bio(ti, clone, ci->io);
ci->sector += len;