[PATCH] raid1: use __GFP_NOFAIL

[PATCH] raid1: use __GFP_NOFAIL

[Mikulas Patocka]

Hi

This is a bug I found when implementing the barriers. There should be 
__GFP_NOFAIL if the code can't handle an allocation failure.

Mikulas

--

If the code can't handle allocation failures, use __GFP_NOFAIL - so that
in case of memory pressure the allocator will retry indefinitely and
won't return NULL which would cause a crash in the function.

This is still not a correct fix, it may cause a classic deadlock when
memory manager waits for I/O being done and I/O waits for some free memory.
I/O code shouldn't allocate any memory. But in this case it probably
doesn't matter much in practice, people usually do not swap on RAID.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-region-hash.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.30-rc2-devel/drivers/md/dm-region-hash.c
===================================================================
--- linux-2.6.30-rc2-devel.orig/drivers/md/dm-region-hash.c	2009-04-24 09:38:43.000000000 +0200
+++ linux-2.6.30-rc2-devel/drivers/md/dm-region-hash.c	2009-04-24 09:39:56.000000000 +0200
@@ -292,7 +292,7 @@ static struct dm_region *__rh_alloc(stru
 
 	nreg = mempool_alloc(rh->region_pool, GFP_ATOMIC);
 	if (unlikely(!nreg))
-		nreg = kmalloc(sizeof(*nreg), GFP_NOIO);
+		nreg = kmalloc(sizeof(*nreg), GFP_NOIO | __GFP_NOFAIL);
 
 	nreg->state = rh->log->type->in_sync(rh->log, region, 1) ?
 		      DM_RH_CLEAN : DM_RH_NOSYNC;

Re: [PATCH] raid1: use __GFP_NOFAIL

[Alasdair G Kergon]

On Mon, Apr 27, 2009 at 07:01:22AM -0400, Mikulas Patocka wrote:
> This is a bug I found when implementing the barriers. There should be 
> __GFP_NOFAIL if the code can't handle an allocation failure.
 
See also this one:
  dm-raid1-mempool-bug-workaround-reversion.patch

Has the original reason for that workaround gone away now or is that
mempool still being used incorrectly?

Alasdair


From: Alasdair G Kergon <agk@redhat.com>

Implement private fallback if immediate allocation from mempool fails.
Standard mempool_alloc() fallback can yield a deadlock when only the
calling process is able to refill the pool. In out-of-memory situations,
instead of waiting for itself, kmirrord now waits for someone else to
free some space, using a standard blocking allocation.

Signed-off-by: Daniel Kobras <kobras@linux.de>
Signed-off-by: Alasdair G Kergon <agk@redhat.com>
---
[AGK - This patch reverses the mempool workaround described above.]

 drivers/md/dm-raid1.c |    4 +---
 1 files changed, 1 insertion(+), 3 deletions(-)

Index: linux-2.6.19/drivers/md/dm-raid1.c
===================================================================
--- linux-2.6.19.orig/drivers/md/dm-raid1.c	2006-12-06 20:49:41.000000000 +0000
+++ linux-2.6.19/drivers/md/dm-raid1.c	2006-12-06 20:49:44.000000000 +0000
@@ -261,9 +261,7 @@ static struct region *__rh_alloc(struct 
 	struct region *reg, *nreg;
 
 	read_unlock(&rh->hash_lock);
-	nreg = mempool_alloc(rh->region_pool, GFP_ATOMIC);
-	if (unlikely(!nreg))
-		nreg = kmalloc(sizeof(struct region), GFP_NOIO);
+	nreg = mempool_alloc(rh->region_pool, GFP_NOIO);
 	nreg->state = rh->log->type->in_sync(rh->log, region, 1) ?
 		RH_CLEAN : RH_NOSYNC;
 	nreg->rh = rh;

Re: [PATCH] raid1: use __GFP_NOFAIL

[Mikulas Patocka]

On Fri, 1 May 2009, Alasdair G Kergon wrote:

> On Mon, Apr 27, 2009 at 07:01:22AM -0400, Mikulas Patocka wrote:
> > This is a bug I found when implementing the barriers. There should be 
> > __GFP_NOFAIL if the code can't handle an allocation failure.
>  
> See also this one:
>   dm-raid1-mempool-bug-workaround-reversion.patch
> 
> Has the original reason for that workaround gone away now or is that
> mempool still being used incorrectly?
> 
> Alasdair

As I said on chat, don't remove that GFP_IO allocation. The code is 
deadlocky anyway and allocation from a mempool with GFP_ATOMIC and, in 
case of failure, allocation from memory with GFP_IO is the best that can 
be done to reduce the probability of the deadlock.

Mikulas

Re: Re: [PATCH] raid1: use __GFP_NOFAIL

[Alasdair G Kergon]

On Fri, May 01, 2009 at 08:04:12AM -0400, Mikulas Patocka wrote:
> As I said on chat, don't remove that GFP_IO allocation. The code is 
> deadlocky anyway and allocation from a mempool with GFP_ATOMIC and, in 
> case of failure, allocation from memory with GFP_IO is the best that can 
> be done to reduce the probability of the deadlock.
 
OK - I'll apply this one pending a more-complicated fix that uses the
mempool correctly.

Alasdair
-- 
agk@redhat.com