[PATCH] dm.c : Check memory allocations

[PATCH] dm.c : Check memory allocations

[Kevin Corry]

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;

Re: [PATCH] dm.c : Check memory allocations

[Jeff Garzik]

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

Re: [PATCH] dm.c : Check memory allocations

[Kevin Corry]

> 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;
 		}

Re: [PATCH] dm.c : Check memory allocations

[Joe Thornber]

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

Re: [PATCH] dm.c : Check memory allocations

[Kevin Corry]

> 
> 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

Re: [PATCH] dm.c : Check memory allocations

[Joe Thornber]

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;