fs/ocfs2/dlm: Use GFP_ATOMIC under spin_lock

fs/ocfs2/dlm: Use GFP_ATOMIC under spin_lock

[David Sterba]

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 <dsterba@suse.cz>
CC: Joel Becker <joel.becker@oracle.com>
CC: Mark Fasheh <mfasheh@suse.com>
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;

Re: fs/ocfs2/dlm: Use GFP_ATOMIC under spin_lock

[Joel Becker]

On Tue, Nov 02, 2010 at 11:36:02PM +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 <dsterba@suse.cz>
> CC: Joel Becker <joel.becker@oracle.com>
> CC: Mark Fasheh <mfasheh@suse.com>
> CC: ocfs2-devel@oss.oracle.com

	This patch is now in the fixes branch of ocfs2.git.

Joel

-- 

"Behind every successful man there's a lot of unsuccessful years."
        - Bob Brown

Joel Becker
Senior Development Manager
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: fs/ocfs2/dlm: Use GFP_ATOMIC under spin_lock

[Andrew Morton]

Blast from the past...

On Tue, 2 Nov 2010 23:36:02 +0100
David Sterba <dsterba@suse.cz> 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 <dsterba@suse.cz>
> CC: Joel Becker <joel.becker@oracle.com>
> CC: Mark Fasheh <mfasheh@suse.com>
> 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)
_

Re: [Ocfs2-devel] fs/ocfs2/dlm: Use GFP_ATOMIC under spin_lock

[Sunil Mushran]

On 01/27/2011 05:09 PM, Andrew Morton wrote:
> 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)

That sizeof() is 1K. It maybe better if we moved the kmalloc() here.

Note that this handler is only called during mount. If the alloc fails, the
mount fails. It's not the end of the world.

Re: [Ocfs2-devel] fs/ocfs2/dlm: Use GFP_ATOMIC under spin_lock

[Andrew Morton]

On Thu, 27 Jan 2011 17:28:53 -0800 Sunil Mushran <sunil.mushran@oracle.com> wrote:

> >   	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)
> 
> That sizeof() is 1K. It maybe better if we moved the kmalloc() here.

That would work too.  As it's only called at mount time, the
speed/space tradeoff favours kmalloc().

Re: [Ocfs2-devel] fs/ocfs2/dlm: Use GFP_ATOMIC under spin_lock

[Joel Becker]

On Thu, Jan 27, 2011 at 05:09:48PM -0800, Andrew Morton wrote:
> > -       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 :(

	Fair enough.

> 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;

	Won't the stack-depth busybodies hate us for this?  I realize we
don't go much deeper from here, but it still is 1K of stack.

Joel

-- 

"When arrows don't penetrate, see...
 Cupid grabs the pistol."

			http://www.jlbec.org/
			jlbec@evilplan.org

Re: [Ocfs2-devel] fs/ocfs2/dlm: Use GFP_ATOMIC under spin_lock

[Andrew Morton]

On Thu, 27 Jan 2011 17:35:53 -0800 Joel Becker <jlbec@evilplan.org> wrote:

> > --- 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;
> 
> 	Won't the stack-depth busybodies hate us for this?  I realize we
> don't go much deeper from here, but it still is 1K of stack.

+	static u8 local[sizeof(qr->qr_regions)]; /* locked by dlm_domain_lock */
        ^^^^^^

Re: [Ocfs2-devel] fs/ocfs2/dlm: Use GFP_ATOMIC under spin_lock

[Joel Becker]

On Thu, Jan 27, 2011 at 05:48:57PM -0800, Andrew Morton wrote:
> On Thu, 27 Jan 2011 17:35:53 -0800 Joel Becker <jlbec@evilplan.org> wrote:
> 
> > > --- 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;
> > 
> > 	Won't the stack-depth busybodies hate us for this?  I realize we
> > don't go much deeper from here, but it still is 1K of stack.
> 
> +	static u8 local[sizeof(qr->qr_regions)]; /* locked by dlm_domain_lock */
>         ^^^^^^

	I can read C, I swear!

Joel

-- 

"And yet I find,
 And yet I find repeating in my head.
 If I can't be my own, 
 I'd feel better dead."

			http://www.jlbec.org/
			jlbec@evilplan.org