[dm-devel] mirror patches - device failure detection/race fix/deadlock fix

[Alasdair G Kergon]

On Tue, Aug 02, 2005 at 01:02:02PM -0500, Jon Brassow wrote:
> That's what the mempool is all about.  :)  I suppose, if you were under 
> memory pressure long enough and the kernel never successfully reclaimed 
> memory, it *might* return NULL -- in which case, the kernel oops's.  I 
> think this is so unlikely, that I would be willing to put in a BUG() 
> call if nreg is found to be NULL.  

> BTW, the scenario is exactly the 
> same as if the gfp_flag was GFP_NOIO -- if the kernel can't get us 
> memory, it can't get us memory.
 
The two cases are quite different.

With NOIO - how it currently works - it'll sleep if there isn't enough 
memory and *never* return NULL.

> >Doesn't it need to be able to sleep to get the benefit of the mempool?
> No.  mempool will try to allocate memory honoring the flags you have 
> set (in this case GFP_ATOMIC).  If unsuccessful, it will tap into the 
> memory reserve you have created when you did a mempool_create.  So, 
> when there is no memory left to give us, we have 64 _regions_ left in 
> our repository.

But that's a finite reserve.  Consider what happens if mempool_allocs 
occur faster than mempool_frees: eventually your reserve drops to 0
and mempool_alloc returns NULL - or have I missed some rate-limiting
properties of the code that would prevent this?

> Are you asking me to "combine" the patches?  I can do that.

I think the irqsaves are only necessary because of locking bugs
in race.patch: We know the irq state throughout the code, and there's 
only a small section that gets used in both states and genuinely
needs irqsave.

This one is self-evidently unnecessary, for example:
        write_lock_irq(&rh->hash_lock);
-       spin_lock(&rh->region_lock);
+       spin_lock_irqsave(&rh->region_lock, flags);

Alasdair
-- 
agk at redhat.com