From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753976Ab1A1BJ5 (ORCPT ); Thu, 27 Jan 2011 20:09:57 -0500 Received: from smtp1.linux-foundation.org ([140.211.169.13]:49889 "EHLO smtp1.linux-foundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752960Ab1A1BJ4 (ORCPT ); Thu, 27 Jan 2011 20:09:56 -0500 Date: Thu, 27 Jan 2011 17:09:48 -0800 From: Andrew Morton To: dsterba@suse.cz Cc: ocfs2-devel@oss.oracle.com, linux-kernel@vger.kernel.org, mfasheh@suse.com, joel.becker@oracle.com Subject: Re: fs/ocfs2/dlm: Use GFP_ATOMIC under spin_lock Message-Id: <20110127170948.9a0d8b60.akpm@linux-foundation.org> In-Reply-To: <20101102223601.GA27513@ds.suse.cz> References: <20101102223601.GA27513@ds.suse.cz> X-Mailer: Sylpheed 3.0.2 (GTK+ 2.20.1; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Blast from the past... On Tue, 2 Nov 2010 23:36:02 +0100 David Sterba wrote: > coccinelle check scripts/coccinelle/locks/call_kern.cocci found that > in fs/ocfs2/dlm/dlmdomain.c an allocation with GFP_KERNEL is done > with locks held: > > dlm_query_region_handler > spin_lock(dlm_domain_lock) > dlm_match_regions > kmalloc(GFP_KERNEL) > > Change it to GFP_ATOMIC. > > Signed-off-by: David Sterba > CC: Joel Becker > CC: Mark Fasheh > CC: ocfs2-devel@oss.oracle.com > > -- > Exists in v2.6.37-rc1 and current linux-next. > > diff -u -p a/fs/ocfs2/dlm/dlmdomain.c b/fs/ocfs2/dlm/dlmdomain.c > --- a/fs/ocfs2/dlm/dlmdomain.c 2010-10-22 10:23:23.502434402 +0200 > +++ b/fs/ocfs2/dlm/dlmdomain.c 2010-11-02 17:11:06.000000000 +0100 > @@ -959,7 +959,7 @@ static int dlm_match_regions(struct dlm_ > r += O2HB_MAX_REGION_NAME_LEN; > } > > - local = kmalloc(sizeof(qr->qr_regions), GFP_KERNEL); > + local = kmalloc(sizeof(qr->qr_regions), GFP_ATOMIC); > if (!local) { > status = -ENOMEM; > goto bail; > Switching to GFP_ATOMIC is the worst of all possible ways of "fixing" this bug. GFP_ATOMIC is *unreliable*. We don't want to introduce unreliability deep down inside our filesytems. And fs maintainers who don't want to make their filesystems less reliable shouldn't blindly apply patches that do so :( A reliable way of fixing this bug is below. As an added bonus, it makes the fs faster. --- a/fs/ocfs2/dlm/dlmdomain.c~a +++ a/fs/ocfs2/dlm/dlmdomain.c @@ -926,9 +926,9 @@ static int dlm_assert_joined_handler(str } static int dlm_match_regions(struct dlm_ctxt *dlm, - struct dlm_query_region *qr) + struct dlm_query_region *qr, u8 *local) { - char *local = NULL, *remote = qr->qr_regions; + char *remote = qr->qr_regions; char *l, *r; int localnr, i, j, foundit; int status = 0; @@ -957,12 +957,6 @@ static int dlm_match_regions(struct dlm_ r += O2HB_MAX_REGION_NAME_LEN; } - local = kmalloc(sizeof(qr->qr_regions), GFP_ATOMIC); - if (!local) { - status = -ENOMEM; - goto bail; - } - localnr = o2hb_get_all_regions(local, O2NM_MAX_REGIONS); /* compare local regions with remote */ @@ -1012,8 +1006,6 @@ static int dlm_match_regions(struct dlm_ } bail: - kfree(local); - return status; } @@ -1077,6 +1069,7 @@ static int dlm_query_region_handler(stru struct dlm_ctxt *dlm = NULL; int status = 0; int locked = 0; + static u8 local[sizeof(qr->qr_regions)]; /* locked by dlm_domain_lock */ qr = (struct dlm_query_region *) msg->buf; @@ -1112,7 +1105,7 @@ static int dlm_query_region_handler(stru goto bail; } - status = dlm_match_regions(dlm, qr); + status = dlm_match_regions(dlm, qr, local); bail: if (locked) _