On Wed, Aug 15, 2007 at 03:56:04PM -0700, Andrew Morton wrote: > On Mon, 13 Aug 2007 13:33:40 +0200 > Heiko Carstens <heiko.carstens@xxxxxxxxxx> wrote: > > the patch below went into 2.6.18. Now my question is: why doesn't it check > > if kmalloc(..., GFP_NOIO) returns with a NULL pointer? > > Did I miss anything that guarentees that this will always succeed or is it > > just a bug? > > --- a/drivers/md/dm-raid1.c > > +++ b/drivers/md/dm-raid1.c > > @@ -255,7 +255,9 @@ static struct region *__rh_alloc(struct region_hash *rh, region_t region) > > struct region *reg, *nreg; > > > > read_unlock(&rh->hash_lock); > > - nreg = mempool_alloc(rh->region_pool, GFP_NOIO); > > + nreg = mempool_alloc(rh->region_pool, GFP_ATOMIC); > > + if (unlikely(!nreg)) > > + nreg = kmalloc(sizeof(struct region), GFP_NOIO); > > nreg->state = rh->log->type->in_sync(rh->log, region, 1) ? > > RH_CLEAN : RH_NOSYNC; > > nreg->rh = rh; > > > > Yeah, that's a bug. > > kmalloc(small_amount, GFP_NOIO) can fail if the calling process gets > oom-killed, and it can fail if the system is using fault-injection. > > One could say "don't use fault injection" and, perhaps, "this is only > ever called by a kernel thread and kernel threads don't get oom-killed". > But the former is lame and the latter assumes current implementation > details which could change (and indeed have in the past). Thanks for clarifying! > So yes, I'd say this is a bug in DM. > > Also, __rh_alloc() is called under read_lock(), via __rh_find(). If > __rh_alloc()'s mempool_alloc() fails, it will perform a sleeping allocation > under read_lock(), which is deadlockable and will generate might_sleep() > warnings The read_lock() is unlocked at the beginning of the function. Unless you're talking of a different lock, but I couldn't find any. So at least _currently_ this should work unless somebody uses fault injection. Would it make sense then to add the __GFP_NOFAIL flag to the kmalloc call? -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
