* [PATCH] dm.c : Check memory allocations
@ 2002-12-27 21:55 Kevin Corry
2002-12-27 22:00 ` Jeff Garzik
2002-12-30 10:51 ` Joe Thornber
0 siblings, 2 replies; 6+ messages in thread
From: Kevin Corry @ 2002-12-27 21:55 UTC (permalink / raw)
To: Joe Thornber; +Cc: dm-devel, linux-kernel
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;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] dm.c : Check memory allocations 2002-12-27 21:55 [PATCH] dm.c : Check memory allocations Kevin Corry @ 2002-12-27 22:00 ` Jeff Garzik 2002-12-27 22:16 ` Kevin Corry 2002-12-30 10:51 ` Joe Thornber 1 sibling, 1 reply; 6+ messages in thread From: Jeff Garzik @ 2002-12-27 22:00 UTC (permalink / raw) To: Kevin Corry; +Cc: Joe Thornber, dm-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dm.c : Check memory allocations 2002-12-27 22:00 ` Jeff Garzik @ 2002-12-27 22:16 ` Kevin Corry 0 siblings, 0 replies; 6+ messages in thread From: Kevin Corry @ 2002-12-27 22:16 UTC (permalink / raw) To: Jeff Garzik; +Cc: Joe Thornber, dm-devel, linux-kernel > 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; } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dm.c : Check memory allocations 2002-12-27 21:55 [PATCH] dm.c : Check memory allocations Kevin Corry 2002-12-27 22:00 ` Jeff Garzik @ 2002-12-30 10:51 ` Joe Thornber 2002-12-31 16:22 ` Kevin Corry 1 sibling, 1 reply; 6+ messages in thread From: Joe Thornber @ 2002-12-30 10:51 UTC (permalink / raw) To: Kevin Corry; +Cc: Joe Thornber, dm-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dm.c : Check memory allocations 2002-12-30 10:51 ` Joe Thornber @ 2002-12-31 16:22 ` Kevin Corry 2003-01-02 9:55 ` Joe Thornber 0 siblings, 1 reply; 6+ messages in thread From: Kevin Corry @ 2002-12-31 16:22 UTC (permalink / raw) To: Joe Thornber; +Cc: dm-devel, linux-kernel > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] dm.c : Check memory allocations 2002-12-31 16:22 ` Kevin Corry @ 2003-01-02 9:55 ` Joe Thornber 0 siblings, 0 replies; 6+ messages in thread From: Joe Thornber @ 2003-01-02 9:55 UTC (permalink / raw) To: Kevin Corry; +Cc: Joe Thornber, dm-devel, linux-kernel 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; ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2003-01-02 9:46 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2002-12-27 21:55 [PATCH] dm.c : Check memory allocations Kevin Corry 2002-12-27 22:00 ` Jeff Garzik 2002-12-27 22:16 ` Kevin Corry 2002-12-30 10:51 ` Joe Thornber 2002-12-31 16:22 ` Kevin Corry 2003-01-02 9:55 ` Joe Thornber
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox