Re: [PATCH] dm: Fix deadlock under high i/o load in raid1 setup.

[Heiko Carstens <heiko.carstens@de.ibm.com>]

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@de.ibm.com> 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?