From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1039658AbdDUNDA (ORCPT ); Fri, 21 Apr 2017 09:03:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49376 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1038986AbdDUNCz (ORCPT ); Fri, 21 Apr 2017 09:02:55 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4E3A78C1A4 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=mpatocka@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 4E3A78C1A4 Date: Fri, 21 Apr 2017 09:02:46 -0400 (EDT) From: Mikulas Patocka X-X-Sender: mpatocka@file01.intranet.prod.int.rdu2.redhat.com To: NeilBrown cc: Alasdair Kergon , Mike Snitzer , dm-devel@redhat.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] dm-region-hash: fix strange usage of mempool_alloc. In-Reply-To: <87efx1xb9h.fsf@notabene.neil.brown.name> Message-ID: References: <87efx1xb9h.fsf@notabene.neil.brown.name> User-Agent: Alpine 2.02 (LRH 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Fri, 21 Apr 2017 13:02:50 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 10 Apr 2017, NeilBrown wrote: > mempool_alloc() should only be called with GFP_ATOMIC when > it is not safe to wait. Passing __GFP_NOFAIL to kmalloc() > says that it is safe to wait indefinitely. So this code is > inconsistent. > > Clearly it is OK to wait indefinitely in this code, and > mempool_alloc() is able to do that. So just use > mempool_alloc, and allow it to sleep. If no memory is > available it will wait for something to be returned to the > pool, and will retry a normal allocation regularly. The region hash code is buggy anyway, because it may allocate more entries than the size of the pool and not give them back. That kmalloc was introduced in the commit c06aad854 to work around a deadlock due to incorrect mempool usage. Your patch slightly increases the probability of the deadlock because mempool_alloc does all allocations with __GFP_NOMEMALLOC, so it uses higher limit than kmalloc(GFP_NOIO). Mikulas > Signed-off-by: NeilBrown > --- > drivers/md/dm-region-hash.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/md/dm-region-hash.c b/drivers/md/dm-region-hash.c > index 85c32b22a420..a6279f5d779e 100644 > --- a/drivers/md/dm-region-hash.c > +++ b/drivers/md/dm-region-hash.c > @@ -287,9 +287,7 @@ static struct dm_region *__rh_alloc(struct dm_region_hash *rh, region_t region) > { > struct dm_region *reg, *nreg; > > - nreg = mempool_alloc(rh->region_pool, GFP_ATOMIC); > - if (unlikely(!nreg)) > - nreg = kmalloc(sizeof(*nreg), GFP_NOIO | __GFP_NOFAIL); > + nreg = mempool_alloc(rh->region_pool, GFP_NOIO); > > nreg->state = rh->log->type->in_sync(rh->log, region, 1) ? > DM_RH_CLEAN : DM_RH_NOSYNC; > -- > 2.12.2 > >