Skip to contentSkip to conversation

RHEL fix for bz428751

PATCH New Benjamin Marzinski 2 participants 5 messages Mar 5, 2008 – Mar 7, 2008

lore
Share & tools
lore

Benjamin Marzinski

author
#0 lore raw

PATCH 1/1 RHEL fix for bz428751

Here is my proposed fix for 428751. It needs some heavy testing, both for correctness and performance, before it gets pushed.

-Ben
-------------- next part --------------

glock.c +22 -1
diff -urpN gfs2/glock.c gfs2-patched/glock.c
@@ -42,6 +42,7 @@
 #include "quota.h"
 #include "super.h"
 #include "util.h"
+#include "lm_deadlk.h"
 
 struct gfs2_gl_hash_bucket {
         struct hlist_head hb_list;

@@ -785,7 +786,8 @@ static void xmote_bh(struct gfs2_glock *
 
 	state_change(gl, ret & LM_OUT_ST_MASK);
 
-	if (prev_state != LM_ST_UNLOCKED && !(ret & LM_OUT_CACHEABLE)) {
+	if ((prev_state != LM_ST_UNLOCKED && !(ret & LM_OUT_CACHEABLE)) ||
+	    ret & LM_OUT_DEADLK) {
 		if (glops->go_inval)
 			glops->go_inval(gl, DIO_METADATA);
 	} else if (gl->gl_state == LM_ST_DEFERRED) {

@@ -815,6 +817,15 @@ static void xmote_bh(struct gfs2_glock *
 		}
 	} else {
 		spin_lock(&gl->gl_spin);
+		if (ret & LM_OUT_DEADLK) {
+			gh->gh_error = 0;
+			gl->gl_req_bh = NULL;
+			set_bit(GLF_DEADLK, &gl->gl_flags);
+			spin_unlock(&gl->gl_spin);
+			gfs2_glock_drop_th(gl);
+			gfs2_glock_put(gl);
+			return;
+		}
 		list_del_init(&gh->gh_list);
 		gh->gh_error = -EIO;
 		if (unlikely(test_bit(SDF_SHUTDOWN, &sdp->sd_flags))) 

@@ -920,6 +931,16 @@ static void drop_bh(struct gfs2_glock *g
 
 	state_change(gl, LM_ST_UNLOCKED);
 
+	if (test_and_clear_bit(GLF_DEADLK, &gl->gl_flags)) {
+		spin_lock(&gl->gl_spin);
+		gh->gh_error = 0;
+		gl->gl_req_bh = NULL;
+		spin_unlock(&gl->gl_spin);
+		gfs2_glock_xmote_th(gl, gl->gl_req_gh);
+		gfs2_glock_put(gl);
+		return;
+	}
+
 	if (glops->go_inval)
 		glops->go_inval(gl, DIO_METADATA);
incore.h +1 -0
diff -urpN gfs2/incore.h gfs2-patched/incore.h
@@ -172,6 +172,7 @@ enum {
 	GLF_PENDING_DEMOTE	= 4,
 	GLF_DIRTY		= 5,
 	GLF_DEMOTE_IN_PROGRESS	= 6,
+	GLF_DEADLK		= 7,
 };
 
 struct gfs2_glock {
lm.c +2 -1
diff -urpN gfs2/lm.c gfs2-patched/lm.c
@@ -21,6 +21,7 @@
 #include "lm.h"
 #include "super.h"
 #include "util.h"
+#include "lm_deadlk.h"
 
 /**
  * gfs2_lm_mount - mount a locking protocol

@@ -35,7 +36,7 @@ int gfs2_lm_mount(struct gfs2_sbd *sdp, 
 {
 	char *proto = sdp->sd_proto_name;
 	char *table = sdp->sd_table_name;
-	int flags = 0;
+	int flags = LM_MFLAG_ALLOW_DEADLK;
 	int error;
 
 	if (sdp->sd_args.ar_spectator)
lm_deadlk.h +16 -0
diff -urpN gfs2/lm_deadlk.h gfs2-patched/lm_deadlk.h
@@ -0,0 +1,16 @@
+/*
+ * Copyright (C) 2008 Red Hat, Inc.  All rights reserved.
+ *
+ * This copyrighted material is made available to anyone wishing to use,
+ * modify, copy, or redistribute it subject to the terms and conditions
+ * of the GNU General Public License version 2.
+ */
+
+#ifndef __LM_DEADLK_DOT_H__
+#define __LM_DEADLK_DOT_H__
+
+/* This is a hack. These flags really belong in lm_interface.h */
+#define LM_OUT_DEADLK    0x00000200
+#define LM_MFLAG_ALLOW_DEADLK 0x00000002
+
+#endif /* __LM_DEADLK_DOT_H__ */
locking/dlm/lock.c +3 -1
diff -urpN gfs2/locking/dlm/lock.c gfs2-patched/locking/dlm/lock.c
@@ -8,6 +8,7 @@
  */
 
 #include "lock_dlm.h"
+#include "../../lm_deadlk.h"
 
 static char junk_lvb[GDLM_LVB_SIZE];
 

@@ -137,7 +138,8 @@ static inline unsigned int make_flags(st
 
 		/* Conversion deadlock avoidance by DLM */
 
-		if (!test_bit(LFL_FORCE_PROMOTE, &lp->flags) &&
+		if (!(lp->ls->fsflags & LM_MFLAG_ALLOW_DEADLK) &&
+		    !test_bit(LFL_FORCE_PROMOTE, &lp->flags) &&
 		    !(lkf & DLM_LKF_NOQUEUE) &&
 		    cur > DLM_LOCK_NL && req > DLM_LOCK_NL && cur != req)
 			lkf |= DLM_LKF_CONVDEADLK;
locking/dlm/thread.c +10 -1
diff -urpN gfs2/locking/dlm/thread.c gfs2-patched/locking/dlm/thread.c
@@ -8,6 +8,7 @@
  */
 
 #include "lock_dlm.h"
+#include "../../lm_deadlk.h"
 
 /* A lock placed on this queue is re-submitted to DLM as soon as the lock_dlm
    thread gets to it. */

@@ -135,7 +136,15 @@ static void process_complete(struct gdlm
 			 lp->lksb.sb_status, lp->lockname.ln_type,
 			 (unsigned long long)lp->lockname.ln_number,
 			 lp->flags);
-		return;
+		if (lp->lksb.sb_status == -EDEADLOCK &&
+		    lp->ls->fsflags & LM_MFLAG_ALLOW_DEADLK) {
+			lp->req = lp->cur;
+			acb.lc_ret |= LM_OUT_DEADLK;
+			if (lp->cur == DLM_LOCK_IV)
+				lp->lksb.sb_lkid = 0;
+			goto out;
+		} else
+			return;
 	}
 
 	/*
#1 lore raw

PATCH 1/1 RHEL fix for bz428751

Hi,

Some comments...

Quoted 4 lines · Benjamin Marzinski

Here is my proposed fix for 428751. It needs some heavy testing, both for correctness and performance, before it gets pushed.

-Ben

glock.c +3 -1
diff -urpN gfs2/glock.c gfs2-patched/glock.c
@@ -42,6 +42,7 @@
 #include "quota.h"
 #include "super.h"
 #include "util.h"
+#include "lm_deadlk.h"
 
 struct gfs2_gl_hash_bucket {
         struct hlist_head hb_list;

@@ -785,7 +786,8 @@ static void xmote_bh(struct gfs2_glock *
 
 	state_change(gl, ret & LM_OUT_ST_MASK);
 
-	if (prev_state != LM_ST_UNLOCKED && !(ret & LM_OUT_CACHEABLE)) {
+	if ((prev_state != LM_ST_UNLOCKED && !(ret & LM_OUT_CACHEABLE)) ||
+	    ret & LM_OUT_DEADLK) {
 		if (glops->go_inval)
 			glops->go_inval(gl, DIO_METADATA);

If we move the ->go_inval() as suggested below, then I don't think this change is required. In fact I think we can ignore the
(ret & LM_OUT_CACHABLE) test since this should never be true any more, since if it is, then we've not eliminated the bug that you are trying to fix. So that should remove one branch from this "if" clause. Also, if we are moving to the DEFERRED state, then we need to invalidate earlier (in the "th" routine) in order to avoid the bug too, so that moves the remaining branch of the "if" clause from this function as well.

} else if (gl->gl_state == LM_ST_DEFERRED) {
@@ -815,6 +817,15 @@ static void xmote_bh(struct gfs2_glock *
}
} else {
spin_lock(&gl->gl_spin);
+ if (ret & LM_OUT_DEADLK) {
+ gh->gh_error = 0;
+ gl->gl_req_bh = NULL;
+ set_bit(GLF_DEADLK, &gl->gl_flags);
+ spin_unlock(&gl->gl_spin);
+ gfs2_glock_drop_th(gl);
+ gfs2_glock_put(gl);
+ return;
+ }
list_del_init(&gh->gh_list);
gh->gh_error = -EIO;
if (unlikely(test_bit(SDF_SHUTDOWN, &sdp->sd_flags)))
@@ -920,6 +931,16 @@ static void drop_bh(struct gfs2_glock *g

state_change(gl, LM_ST_UNLOCKED);

+ if (test_and_clear_bit(GLF_DEADLK, &gl->gl_flags)) {
+ spin_lock(&gl->gl_spin);
+ gh->gh_error = 0;
+ gl->gl_req_bh = NULL;
+ spin_unlock(&gl->gl_spin);
+ gfs2_glock_xmote_th(gl, gl->gl_req_gh);
+ gfs2_glock_put(gl);
+ return;
+ }
+
if (glops->go_inval)
glops->go_inval(gl, DIO_METADATA);

This doesn't look like it solves the problem... don't we need to move the ->go_inval call into gfs2_glock_drop_th() ? After all we know at that point that we'll be dropping the lock, so there is no reason not to invalidate there.

incore.h +1 -0
diff -urpN gfs2/incore.h gfs2-patched/incore.h
@@ -172,6 +172,7 @@ enum {
 	GLF_PENDING_DEMOTE	= 4,
 	GLF_DIRTY		= 5,
 	GLF_DEMOTE_IN_PROGRESS	= 6,
+	GLF_DEADLK		= 7,
 };
 
 struct gfs2_glock {
lm.c +2 -1
diff -urpN gfs2/lm.c gfs2-patched/lm.c
@@ -21,6 +21,7 @@
 #include "lm.h"
 #include "super.h"
 #include "util.h"
+#include "lm_deadlk.h"
 
 /**
  * gfs2_lm_mount - mount a locking protocol

@@ -35,7 +36,7 @@ int gfs2_lm_mount(struct gfs2_sbd *sdp, 
 {
 	char *proto = sdp->sd_proto_name;
 	char *table = sdp->sd_table_name;
-	int flags = 0;
+	int flags = LM_MFLAG_ALLOW_DEADLK;
 	int error;
 
 	if (sdp->sd_args.ar_spectator)
lm_deadlk.h +16 -0
diff -urpN gfs2/lm_deadlk.h gfs2-patched/lm_deadlk.h
@@ -0,0 +1,16 @@
+/*
+ * Copyright (C) 2008 Red Hat, Inc.  All rights reserved.
+ *
+ * This copyrighted material is made available to anyone wishing to use,
+ * modify, copy, or redistribute it subject to the terms and conditions
+ * of the GNU General Public License version 2.
+ */
+
+#ifndef __LM_DEADLK_DOT_H__
+#define __LM_DEADLK_DOT_H__
+
+/* This is a hack. These flags really belong in lm_interface.h */
+#define LM_OUT_DEADLK    0x00000200
+#define LM_MFLAG_ALLOW_DEADLK 0x00000002
+
+#endif /* __LM_DEADLK_DOT_H__ */

So let me ask the obvious question here... why not put them in lm_interface.h in that case? I don't think we need to add an extra header just for two constants.

I'm also not convinced about the naming of the constants. I can see why you chose those names, but I don't think its very helpful to someone looking at the code for the first time. Could we have LM_MFLAG_CONV_NODROP for example, which might make it a bit clearer? Some brief comments explaining what they mean would be good too.

locking/dlm/lock.c +1 -0
diff -urpN gfs2/locking/dlm/lock.c gfs2-patched/locking/dlm/lock.c
@@ -8,6 +8,7 @@
  */
 
 #include "lock_dlm.h"
+#include "../../lm_deadlk.h"

If we really do need to have an extra header, the please put it in include/linux and then you can use <> rather than needing to specify paths like this.

static char junk_lvb[GDLM_LVB_SIZE];

@@ -137,7 +138,8 @@ static inline unsigned int make_flags(st

/* Conversion deadlock avoidance by DLM */

- if (!test_bit(LFL_FORCE_PROMOTE, &lp->flags) &&
+ if (!(lp->ls->fsflags & LM_MFLAG_ALLOW_DEADLK) &&
+ !test_bit(LFL_FORCE_PROMOTE, &lp->flags) &&
!(lkf & DLM_LKF_NOQUEUE) &&
cur > DLM_LOCK_NL && req > DLM_LOCK_NL && cur != req)
lkf |= DLM_LKF_CONVDEADLK;

locking/dlm/thread.c +10 -1
diff -urpN gfs2/locking/dlm/thread.c gfs2-patched/locking/dlm/thread.c
@@ -8,6 +8,7 @@
  */
 
 #include "lock_dlm.h"
+#include "../../lm_deadlk.h"
 
 /* A lock placed on this queue is re-submitted to DLM as soon as the lock_dlm
    thread gets to it. */

@@ -135,7 +136,15 @@ static void process_complete(struct gdlm
 			 lp->lksb.sb_status, lp->lockname.ln_type,
 			 (unsigned long long)lp->lockname.ln_number,
 			 lp->flags);
-		return;
+		if (lp->lksb.sb_status == -EDEADLOCK &&
+		    lp->ls->fsflags & LM_MFLAG_ALLOW_DEADLK) {
+			lp->req = lp->cur;
+			acb.lc_ret |= LM_OUT_DEADLK;
+			if (lp->cur == DLM_LOCK_IV)
+				lp->lksb.sb_lkid = 0;
+			goto out;
+		} else
+			return;
 	}
 
 	/*
author
#2 lore raw

PATCH 1/1 RHEL fix for bz428751

Quoted 4 lines · Steven Whitehouse

This doesn't look like it solves the problem... don't we need to move the ->go_inval call into gfs2_glock_drop_th() ? After all we know at that point that we'll be dropping the lock, so there is no reason not to invalidate there.

Moving the invalidation into the gfs2_glock_drop_th() causes problems. If the page is already locked when gfs2_glock_drop_th() is called, you deadlock trying to lock the pages when you invalidate the lock.

Looking through the code, it seems like the only time this should happen is during a gfs2_readpage() call, like this.

#0 [f1d6ec2c] schedule at c06072d9
#1 [f1d6ec94] io_schedule at c0607974
#2 [f1d6eca0] sync_page at c0455074
#3 [f1d6eca4] __wait_on_bit_lock at c0607a89 #4 [f1d6ecb8] __lock_page at c0454fbf
#5 [f1d6ece4] truncate_inode_pages_range at c045be78 #6 [f1d6ed50] truncate_inode_pages at c045bed6 #7 [f1d6ed5c] inode_go_inval at f8e4fba2
#8 [f1d6ed64] gfs2_glock_drop_th at f8e4ed97 #9 [f1d6ed80] run_queue at f8e4ef40
#10 [f1d6ed9c] gfs2_glock_nq at f8e4f432
#11 [f1d6edb8] gfs2_glock_nq_atime at f8e5060b #12 [f1d6edfc] gfs2_readpage at f8e56c95

From the best I can tell, it looks like it would be O.K. to unlock the page before calling glops->go_inval() in this case, assuming that you knew that you were the process that is holding the lock to the page and which page was actually locked, and you had a way to tell gfs2_readpage not to bother unlocking the page once you were finished.

Unfortunately, coming up with a good way to pass that information back and forth isn't straightforward. As soon as I come up with a decent answer, I'll post the modified fix.

-Ben

#3 lore raw

PATCH 1/1 RHEL fix for bz428751

Hi,

Quoted 34 lines · Benjamin Marzinski
Quoted 4 lines · Steven Whitehouse

This doesn't look like it solves the problem... don't we need to move the ->go_inval call into gfs2_glock_drop_th() ? After all we know at that point that we'll be dropping the lock, so there is no reason not to invalidate there.

Moving the invalidation into the gfs2_glock_drop_th() causes problems. If the page is already locked when gfs2_glock_drop_th() is called, you deadlock trying to lock the pages when you invalidate the lock.

Looking through the code, it seems like the only time this should happen is during a gfs2_readpage() call, like this.

#0 [f1d6ec2c] schedule at c06072d9
#1 [f1d6ec94] io_schedule at c0607974
#2 [f1d6eca0] sync_page at c0455074
#3 [f1d6eca4] __wait_on_bit_lock at c0607a89 #4 [f1d6ecb8] __lock_page at c0454fbf
#5 [f1d6ece4] truncate_inode_pages_range at c045be78 #6 [f1d6ed50] truncate_inode_pages at c045bed6 #7 [f1d6ed5c] inode_go_inval at f8e4fba2
#8 [f1d6ed64] gfs2_glock_drop_th at f8e4ed97 #9 [f1d6ed80] run_queue at f8e4ef40
#10 [f1d6ed9c] gfs2_glock_nq at f8e4f432
#11 [f1d6edb8] gfs2_glock_nq_atime at f8e5060b #12 [f1d6edfc] gfs2_readpage at f8e56c95

From the best I can tell, it looks like it would be O.K. to unlock the page before calling glops->go_inval() in this case, assuming that you knew that you were the process that is holding the lock to the page and which page was actually locked, and you had a way to tell gfs2_readpage not to bother unlocking the page once you were finished.

Quoted 6 lines

Unfortunately, coming up with a good way to pass that information back and forth isn't straightforward. As soon as I come up with a decent answer, I'll post the modified fix.

-Ben

Nothing ought to be holding the page lock while we are unlocking the glock, otherwise problems will occur whichever version of the code you use. Are you missing the fix for #432057 perhaps?

Steve.

author
#4 lore raw

PATCH 1/1 RHEL fix for bz428751

Quoted 41 lines · Steven Whitehouse

Hi,

Quoted 38 lines · Benjamin Marzinski
Quoted 4 lines · Steven Whitehouse

This doesn't look like it solves the problem... don't we need to move the ->go_inval call into gfs2_glock_drop_th() ? After all we know at that point that we'll be dropping the lock, so there is no reason not to invalidate there.

Moving the invalidation into the gfs2_glock_drop_th() causes problems. If the page is already locked when gfs2_glock_drop_th() is called, you deadlock trying to lock the pages when you invalidate the lock.

Looking through the code, it seems like the only time this should happen is during a gfs2_readpage() call, like this.

#0 [f1d6ec2c] schedule at c06072d9
#1 [f1d6ec94] io_schedule at c0607974
#2 [f1d6eca0] sync_page at c0455074
#3 [f1d6eca4] __wait_on_bit_lock at c0607a89 #4 [f1d6ecb8] __lock_page at c0454fbf
#5 [f1d6ece4] truncate_inode_pages_range at c045be78 #6 [f1d6ed50] truncate_inode_pages at c045bed6 #7 [f1d6ed5c] inode_go_inval at f8e4fba2
#8 [f1d6ed64] gfs2_glock_drop_th at f8e4ed97 #9 [f1d6ed80] run_queue at f8e4ef40
#10 [f1d6ed9c] gfs2_glock_nq at f8e4f432
#11 [f1d6edb8] gfs2_glock_nq_atime at f8e5060b #12 [f1d6edfc] gfs2_readpage at f8e56c95

From the best I can tell, it looks like it would be O.K. to unlock the page before calling glops->go_inval() in this case, assuming that you knew that you were the process that is holding the lock to the page and which page was actually locked, and you had a way to tell gfs2_readpage not to bother unlocking the page once you were finished.

Unfortunately, coming up with a good way to pass that information back and forth isn't straightforward. As soon as I come up with a decent answer, I'll post the modified fix.

Quoted 7 lines
Quoted 2 lines

-Ben

Nothing ought to be holding the page lock while we are unlocking the glock, otherwise problems will occur whichever version of the code you use. Are you missing the fix for #432057 perhaps?

Yes I am. Thanks. That should make things work much better. -Ben

Quoted 2 lines

Steve.