fs/ocfs2/dlm: Use GFP_ATOMIC under spin_lock

PATCH David Sterba 5 participants 8 messages Nov 2, 2010 – Jan 28, 2011 OCFS2

Share & tools

David Sterba

author

2010-11-02 22:36 UTC #0 lore raw

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 <[email protected]>

CC Joel Becker <[email protected]>

CC Mark Fasheh <[email protected]>

CC [email protected]

--
Exists in v2.6.37-rc1 and current linux-next.

fs/ocfs2/dlm/dlmdomain.c 2010-11-02 17:11:06.000000000 +0100 renamed from fs/ocfs2/dlm/dlmdomain.c 2010-10-22 10:23:23.502434402 +0200 +1 -1

diff -u -p a/fs/ocfs2/dlm/dlmdomain.c b/fs/ocfs2/dlm/dlmdomain.c
@@ -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;

Joel Becker

2010-11-18 23:42 UTC #1 lore raw

Quoted 9 lines · 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)

Quoted 6 lines

Change it to GFP_ATOMIC.
Signed-off-by David Sterba <[email protected]>
CC Joel Becker <[email protected]>
CC Mark Fasheh <[email protected]>
CC ocfs2-devel at oss.oracle.com

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

Joel

Signature · 8 lines

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

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

Andrew Morton

2011-01-28 01:09 UTC #2 lore raw

In reply to David Sterba

Blast from the past...

Quoted 24 lines · On Tue, 2 Nov 2010 23:36:02 +0100 David Sterba <[email protected]> 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 <[email protected]>
CC Joel Becker <[email protected]>
CC Mark Fasheh <[email protected]>
CC ocfs2-devel at 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

fs/ocfs2/dlm/dlmdomain.c 2010-11-02 17:11:06.000000000 +0100 static int dlm_match_regions(struct dlm_ · line 965

Quoted 8 lines

fs/ocfs2/dlm/dlmdomain.c 2010-11-02 17:11:06.000000000 +0100 renamed from fs/ocfs2/dlm/dlmdomain.c 2010-10-22 10:23:23.502434402 +0200 +1 -1

@@ -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.

fs/ocfs2/dlm/dlmdomain.c renamed from fs/ocfs2/dlm/dlmdomain.c~a +4 ~~-11~~

@@ -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)

Sunil Mushran

2011-01-28 01:28 UTC #3 lore raw

Quoted 55 lines · Andrew Morton

A reliable way of fixing this bug is below. As an added bonus, it makes the fs faster.

fs/ocfs2/dlm/dlmdomain.c renamed from fs/ocfs2/dlm/dlmdomain.c~a +3 ~~-10~~

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

fs/ocfs2/dlm/dlmdomain.c static int dlm_query_region_handler(stru · line 1111

Quoted 8 lines

fs/ocfs2/dlm/dlmdomain.c renamed from fs/ocfs2/dlm/dlmdomain.c~a +1 -1

@@ -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.

Andrew Morton

2011-01-28 01:48 UTC #4 lore raw

Quoted 15 lines · On Thu, 27 Jan 2011 17:28:53 -0800 Sunil Mushran <[email protected]>…

Quoted 15 lines

  	return status;
  }

fs/ocfs2/dlm/dlmdomain.c renamed from fs/ocfs2/dlm/dlmdomain.c~a +1 -0

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

Quoted 7 lines

fs/ocfs2/dlm/dlmdomain.c static int dlm_query_region_handler(stru · line 1111
Quoted 5 lines
fs/ocfs2/dlm/dlmdomain.c renamed from fs/ocfs2/dlm/dlmdomain.c~a +1 -1
-	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().

Joel Becker

maintainer

2011-01-28 01:35 UTC #5 lore raw

In reply to Andrew Morton

Quoted 7 lines · Andrew Morton

Review

Quoted 6 lines

(quoted diff) +1 -1

-       local = kmalloc(sizeof(qr->qr_regions), GFP_KERNEL);
+       local = kmalloc(sizeof(qr->qr_regions), GFP_ATOMIC);
        if (!local) {
                status = -ENOMEM;
                goto bail;

Quoted 5 lines

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.

Quoted 6 lines

A reliable way of fixing this bug is below. As an added bonus, it makes the fs faster.

fs/ocfs2/dlm/dlmdomain.c static int dlm_assert_joined_handler(str · line 931

Quoted 8 lines

fs/ocfs2/dlm/dlmdomain.c renamed from fs/ocfs2/dlm/dlmdomain.c~a +2 -2

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

Signature · 5 lines

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

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

Andrew Morton

2011-01-28 01:48 UTC #6 lore raw

Quoted 5 lines · On Thu, 27 Jan 2011 17:35:53 -0800 Joel Becker <[email protected]> wrote:

Quoted 5 lines
fs/ocfs2/dlm/dlmdomain.c renamed from fs/ocfs2/dlm/dlmdomain.c~a +0 -0
@@ -926,9 +926,9 @@ static int dlm_assert_joined_handler(str
 }
 

Quoted 9 lines

fs/ocfs2/dlm/dlmdomain.c static int dlm_assert_joined_handler(str · line 931
Quoted 6 lines
fs/ocfs2/dlm/dlmdomain.c renamed from fs/ocfs2/dlm/dlmdomain.c~a +2 -2
 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 */

        ^^^^^^

Joel Becker

maintainer

2011-01-28 02:33 UTC #7 lore raw

Quoted 17 lines · Andrew Morton

Quoted 14 lines · On Thu, 27 Jan 2011 17:35:53 -0800 Joel Becker <[email protected]> wrote:
fs/ocfs2/dlm/dlmdomain.c static int dlm_assert_joined_handler(str · line 931
Quoted 11 lines
fs/ocfs2/dlm/dlmdomain.c renamed from fs/ocfs2/dlm/dlmdomain.c~a +2 -2
@@ -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.

Review

Quoted 2 lines

(quoted diff) +1 -0

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

I can read C, I swear!

Joel

Signature · 7 lines

"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 at evilplan.org