Clean up glock layer

Cluster-devel New Steven Whitehouse 2 participants 7 messages Jul 30, 2007 – Aug 2, 2007 GFS2 file system

Share & tools

Steven Whitehouse

author

2007-07-30 09:32 UTC #0 lore raw

Hi,

This is a set of 5 patches which clean up parts of the glock layer. I spotted these while looking for a bug (which these patches don't fix) but hopefully they will make things a bit clearer.

Eventually there will be some follow on patches which change the way that GLF_LOCK is used, but these are standalone so I'm posting them anyway just to illuminate my current thoughts on this area,

Steve.

Steven Whitehouse

author

2007-07-30 09:32 UTC #1 lore raw

PATCH 1/1 Clean up arguments to rq_mutex/rq_promote

By making the arguments to rq_mutex and rq_promote into glocks and moving the asserts to those functions, run_queue becomes much simpler. Also rq_mutex always returned 1, so I've changed it to return void and it always breaks out of the loop.

Signed-off-by Steven Whitehouse <[email protected]>

fs/gfs2/glock.c +14 ~~-26~~

index e4bc8ae..65d6eba 100644
@@ -455,18 +455,18 @@ static void wait_on_demote(struct gfs2_glock *gl)
  * Returns: 1 if the queue is blocked
  */
 
-static int rq_mutex(struct gfs2_holder *gh)
+static void rq_mutex(struct gfs2_glock *gl)
 {
-	struct gfs2_glock *gl = gh->gh_gl;
+	struct gfs2_holder *gh = list_entry(gl->gl_waiters1.next,
+					    struct gfs2_holder, gh_list);
 
+	BUG_ON(test_bit(HIF_MUTEX, &gh->gh_iflags) == 0);
 	list_del_init(&gh->gh_list);
 	/*  gh->gh_error never examined.  */
 	set_bit(GLF_LOCK, &gl->gl_flags);
 	clear_bit(HIF_WAIT, &gh->gh_iflags);
 	smp_mb();
 	wake_up_bit(&gh->gh_iflags, HIF_WAIT);
-
-	return 1;
 }
 
 /**

@@ -478,11 +478,13 @@ static int rq_mutex(struct gfs2_holder *gh)
  * Returns: 1 if the queue is blocked
  */
 
-static int rq_promote(struct gfs2_holder *gh)
+static int rq_promote(struct gfs2_glock *gl)
 {
-	struct gfs2_glock *gl = gh->gh_gl;
+	struct gfs2_holder *gh = list_entry(gl->gl_waiters3.next,
+					    struct gfs2_holder, gh_list);
 	struct gfs2_sbd *sdp = gl->gl_sbd;
 
+	BUG_ON(test_bit(HIF_PROMOTE, &gh->gh_iflags) == 0);
 	if (!relaxed_state_ok(gl->gl_state, gh->gh_state, gh->gh_flags)) {
 		if (list_empty(&gl->gl_holders)) {
 			gl->gl_req_gh = gh;

@@ -562,35 +564,21 @@ static int rq_demote(struct gfs2_glock *gl)
  */
 static void run_queue(struct gfs2_glock *gl)
 {
-	struct gfs2_holder *gh;
-	int blocked = 1;
+	int blocked;
 
 	for (;;) {
+		blocked = 1;
 		if (test_bit(GLF_LOCK, &gl->gl_flags))
 			break;
 
 		if (!list_empty(&gl->gl_waiters1)) {
-			gh = list_entry(gl->gl_waiters1.next,
-					struct gfs2_holder, gh_list);
-
-			if (test_bit(HIF_MUTEX, &gh->gh_iflags))
-				blocked = rq_mutex(gh);
-			else
-				gfs2_assert_warn(gl->gl_sbd, 0);
-
+			rq_mutex(gl);
+			break;
 		} else if (test_bit(GLF_DEMOTE, &gl->gl_flags)) {
 			blocked = rq_demote(gl);
 		} else if (!list_empty(&gl->gl_waiters3)) {
-			gh = list_entry(gl->gl_waiters3.next,
-					struct gfs2_holder, gh_list);
-
-			if (test_bit(HIF_PROMOTE, &gh->gh_iflags))
-				blocked = rq_promote(gh);
-			else
-				gfs2_assert_warn(gl->gl_sbd, 0);
-
-		} else
-			break;
+			blocked = rq_promote(gl);
+		}
 
 		if (blocked)
 			break;

Signature · 1 line

1.5.1.2

Steven Whitehouse

author

2007-07-30 09:32 UTC #2 lore raw

PATCH 1/1 Clean up run_queue()

A further clean up to make the logic a bit more obvious and remove a local variable which is no longer required.

Signed-off-by Steven Whitehouse <[email protected]>

fs/gfs2/glock.c +8 ~~-13~~

index 65d6eba..79d61ff 100644
@@ -564,24 +564,19 @@ static int rq_demote(struct gfs2_glock *gl)
  */
 static void run_queue(struct gfs2_glock *gl)
 {
-	int blocked;
-
-	for (;;) {
-		blocked = 1;
-		if (test_bit(GLF_LOCK, &gl->gl_flags))
-			break;
-
+	while(test_bit(GLF_LOCK, &gl->gl_flags) == 0) {
 		if (!list_empty(&gl->gl_waiters1)) {
 			rq_mutex(gl);
 			break;
-		} else if (test_bit(GLF_DEMOTE, &gl->gl_flags)) {
-			blocked = rq_demote(gl);
+		}
+		if (test_bit(GLF_DEMOTE, &gl->gl_flags)) {
+			if (rq_demote(gl) == 0)
+				continue;
 		} else if (!list_empty(&gl->gl_waiters3)) {
-			blocked = rq_promote(gl);
+			if (rq_promote(gl) == 0)
+				continue;
 		}
-
-		if (blocked)
-			break;
+		break;
 	}
 }

Signature · 1 line

1.5.1.2

Steven Whitehouse

author

2007-07-30 09:32 UTC #3 lore raw

PATCH 1/1 Merge ->go_drop_th() with ->go_xmote_th()

The drop and xmote callbacks were functionally identical for glock types which had those callbacks. This patch merges the functions while at the same time adding an extra variable to the go_xmote_th() function for future use (the new state). Thus both the new and old states are now available in that callback.

This results in a fair amount of duplicate code being removed.

Signed-off-by Steven Whitehouse <[email protected]>

fs/gfs2/glock.c +3 -3

index 79d61ff..dd23ab1 100644
@@ -822,7 +822,7 @@ static void gfs2_glock_xmote_th(struct gfs2_glock *gl, struct gfs2_holder *gh)
 	unsigned int lck_ret;
 
 	if (glops->go_xmote_th)
-		glops->go_xmote_th(gl);
+		glops->go_xmote_th(gl, state);
 
 	gfs2_assert_warn(sdp, test_bit(GLF_LOCK, &gl->gl_flags));
 	gfs2_assert_warn(sdp, list_empty(&gl->gl_holders));

@@ -901,8 +901,8 @@ static void gfs2_glock_drop_th(struct gfs2_glock *gl)
 	const struct gfs2_glock_operations *glops = gl->gl_ops;
 	unsigned int ret;
 
-	if (glops->go_drop_th)
-		glops->go_drop_th(gl);
+	if (glops->go_xmote_th)
+		glops->go_xmote_th(gl, LM_ST_UNLOCKED);
 
 	gfs2_assert_warn(sdp, test_bit(GLF_LOCK, &gl->gl_flags));
 	gfs2_assert_warn(sdp, list_empty(&gl->gl_holders));

fs/gfs2/glops.c +3 ~~-41~~

98 lines not on this page · raw message

fs/gfs2/incore.h +1 -2

index 170ba93..8b91f83 100644
@@ -122,9 +122,8 @@ struct gfs2_bufdata {
 };
 
 struct gfs2_glock_operations {
-	void (*go_xmote_th) (struct gfs2_glock *gl);
+	void (*go_xmote_th) (struct gfs2_glock *gl, unsigned new_state);
 	void (*go_xmote_bh) (struct gfs2_glock *gl);
-	void (*go_drop_th) (struct gfs2_glock *gl);
 	void (*go_inval) (struct gfs2_glock *gl, int flags);
 	int (*go_demote_ok) (struct gfs2_glock *gl);
 	int (*go_lock) (struct gfs2_holder *gh);

Signature · 1 line

1.5.1.2

Steven Whitehouse

author

2007-07-30 09:32 UTC #4 lore raw

PATCH 1/1 Merge gfs2_glock_drop_th() into gfs2_glock_xmote_th()

These two functions were very similar in form. After the previous patch its fairly easy to merge them. Also there was some left over code in drop_bh() which was unused since we no longer use gfs2_holder structures for "drop"s.

Signed-off-by Steven Whitehouse <[email protected]>

fs/gfs2/glock.c +29 ~~-77~~

index dd23ab1..6324f57 100644
@@ -55,7 +55,6 @@ typedef void (*glock_examiner) (struct gfs2_glock * gl);
 static int gfs2_dump_lockstate(struct gfs2_sbd *sdp);
 static int dump_glock(struct glock_iter *gi, struct gfs2_glock *gl);
 static void gfs2_glock_xmote_th(struct gfs2_glock *gl, struct gfs2_holder *gh);
-static void gfs2_glock_drop_th(struct gfs2_glock *gl);
 static DECLARE_RWSEM(gfs2_umount_flush_sem);
 static struct dentry *gfs2_root;

@@ -544,16 +543,9 @@ static int rq_demote(struct gfs2_glock *gl)
 		return 0;
 	}
 	set_bit(GLF_LOCK, &gl->gl_flags);
-	if (gl->gl_demote_state == LM_ST_UNLOCKED ||
-	    gl->gl_state != LM_ST_EXCLUSIVE) {
-		spin_unlock(&gl->gl_spin);
-		gfs2_glock_drop_th(gl);
-	} else {
-		spin_unlock(&gl->gl_spin);
-		gfs2_glock_xmote_th(gl, NULL);
-	}
+	spin_unlock(&gl->gl_spin);
+	gfs2_glock_xmote_th(gl, NULL);
 	spin_lock(&gl->gl_spin);
-
 	return 0;
 }

@@ -750,7 +742,7 @@ static void xmote_bh(struct gfs2_glock *gl, unsigned int ret)
 			if (gl->gl_state != gl->gl_demote_state) {
 				gl->gl_req_bh = NULL;
 				spin_unlock(&gl->gl_spin);
-				gfs2_glock_drop_th(gl);
+				gfs2_glock_xmote_th(gl, NULL);
 				gfs2_glock_put(gl);
 				return;
 			}

@@ -803,47 +795,6 @@ out:
 }
 
 /**
- * gfs2_glock_xmote_th - Call into the lock module to acquire or change a glock
- * @gl: The glock in question
- * @state: the requested state
- * @flags: modifier flags to the lock call
- *
- */
-
-static void gfs2_glock_xmote_th(struct gfs2_glock *gl, struct gfs2_holder *gh)
-{
-	struct gfs2_sbd *sdp = gl->gl_sbd;
-	int flags = gh ? gh->gh_flags : 0;
-	unsigned state = gh ? gh->gh_state : gl->gl_demote_state;
-	const struct gfs2_glock_operations *glops = gl->gl_ops;
-	int lck_flags = flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB |
-				 LM_FLAG_NOEXP | LM_FLAG_ANY |
-				 LM_FLAG_PRIORITY);
-	unsigned int lck_ret;
-
-	if (glops->go_xmote_th)
-		glops->go_xmote_th(gl, state);
-
-	gfs2_assert_warn(sdp, test_bit(GLF_LOCK, &gl->gl_flags));
-	gfs2_assert_warn(sdp, list_empty(&gl->gl_holders));
-	gfs2_assert_warn(sdp, state != LM_ST_UNLOCKED);
-	gfs2_assert_warn(sdp, state != gl->gl_state);
-
-	gfs2_glock_hold(gl);
-	gl->gl_req_bh = xmote_bh;
-
-	lck_ret = gfs2_lm_lock(sdp, gl->gl_lock, gl->gl_state, state, lck_flags);
-
-	if (gfs2_assert_withdraw(sdp, !(lck_ret & LM_OUT_ERROR)))
-		return;
-
-	if (lck_ret & LM_OUT_ASYNC)
-		gfs2_assert_warn(sdp, lck_ret == LM_OUT_ASYNC);
-	else
-		xmote_bh(gl, lck_ret);
-}
-
-/**
  * drop_bh - Called after a lock module unlock completes
  * @gl: the glock
  * @ret: the return status

@@ -857,7 +808,6 @@ static void drop_bh(struct gfs2_glock *gl, unsigned int ret)
 {
 	struct gfs2_sbd *sdp = gl->gl_sbd;
 	const struct gfs2_glock_operations *glops = gl->gl_ops;
-	struct gfs2_holder *gh = gl->gl_req_gh;
 
 	gfs2_assert_warn(sdp, test_bit(GLF_LOCK, &gl->gl_flags));
 	gfs2_assert_warn(sdp, list_empty(&gl->gl_holders));

@@ -869,57 +819,59 @@ static void drop_bh(struct gfs2_glock *gl, unsigned int ret)
 	if (glops->go_inval)
 		glops->go_inval(gl, DIO_METADATA);
 
-	if (gh) {
-		spin_lock(&gl->gl_spin);
-		list_del_init(&gh->gh_list);
-		gh->gh_error = 0;
-		spin_unlock(&gl->gl_spin);
-	}
-
 	spin_lock(&gl->gl_spin);
-	gl->gl_req_gh = NULL;
 	gl->gl_req_bh = NULL;
 	clear_bit(GLF_LOCK, &gl->gl_flags);
 	run_queue(gl);
 	spin_unlock(&gl->gl_spin);
 
 	gfs2_glock_put(gl);
-
-	if (gh)
-		gfs2_holder_wake(gh);
 }
 
 /**
- * gfs2_glock_drop_th - call into the lock module to unlock a lock
- * @gl: the glock
+ * gfs2_glock_xmote_th - Call into the lock module to acquire or change a glock
+ * @gl: The glock in question
+ * @state: the requested state
+ * @flags: modifier flags to the lock call
  *
  */
 
-static void gfs2_glock_drop_th(struct gfs2_glock *gl)
+static void gfs2_glock_xmote_th(struct gfs2_glock *gl, struct gfs2_holder *gh)
 {
 	struct gfs2_sbd *sdp = gl->gl_sbd;
+	int flags = gh ? gh->gh_flags : 0;
+	unsigned state = gh ? gh->gh_state : gl->gl_demote_state;
 	const struct gfs2_glock_operations *glops = gl->gl_ops;
-	unsigned int ret;
+	int lck_flags = flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB |
+				 LM_FLAG_NOEXP | LM_FLAG_ANY |
+				 LM_FLAG_PRIORITY);
+	unsigned int lck_ret;
 
 	if (glops->go_xmote_th)
-		glops->go_xmote_th(gl, LM_ST_UNLOCKED);
+		glops->go_xmote_th(gl, state);
 
 	gfs2_assert_warn(sdp, test_bit(GLF_LOCK, &gl->gl_flags));
 	gfs2_assert_warn(sdp, list_empty(&gl->gl_holders));
-	gfs2_assert_warn(sdp, gl->gl_state != LM_ST_UNLOCKED);
+	gfs2_assert_warn(sdp, state != gl->gl_state);
 
 	gfs2_glock_hold(gl);
-	gl->gl_req_bh = drop_bh;
+	if (state != LM_ST_UNLOCKED) {
+		gl->gl_req_bh = xmote_bh;
+		lck_ret = gfs2_lm_lock(sdp, gl->gl_lock, gl->gl_state, state, lck_flags);
+	} else {
+		gl->gl_req_bh = drop_bh;
+		lck_ret = gfs2_lm_unlock(sdp, gl->gl_lock, gl->gl_state);
+	}
 
-	ret = gfs2_lm_unlock(sdp, gl->gl_lock, gl->gl_state);
+	if (gfs2_assert_withdraw(sdp, !(lck_ret & LM_OUT_ERROR)))
+		return;
 
-	if (gfs2_assert_withdraw(sdp, !(ret & LM_OUT_ERROR)))
+	if ((lck_ret & LM_OUT_ASYNC) == 0) {
+		gl->gl_req_bh(gl, lck_ret);
 		return;
+	}
 
-	if (!ret)
-		drop_bh(gl, ret);
-	else
-		gfs2_assert_warn(sdp, ret == LM_OUT_ASYNC);
+	BUG_ON(lck_ret != LM_OUT_ASYNC);
 }
 
 /**

Signature · 1 line

1.5.1.2

Steven Whitehouse

author

2007-07-30 09:32 UTC #5 lore raw

PATCH 1/1 Remove reclaim logic from rq_promote()

There is no need to reclaim glocks in this code path since the glockd does this for us anyway. Also the limit of 5000 is entirely aribtrary and it doesn't make sense to slow down this (fast path) when its not actually responsible for generating reclaimable glocks.

Signed-off-by Steven Whitehouse <[email protected]>

fs/gfs2/glock.c +0 -9

index 6324f57..be0368b 100644
@@ -481,7 +481,6 @@ static int rq_promote(struct gfs2_glock *gl)
 {
 	struct gfs2_holder *gh = list_entry(gl->gl_waiters3.next,
 					    struct gfs2_holder, gh_list);
-	struct gfs2_sbd *sdp = gl->gl_sbd;
 
 	BUG_ON(test_bit(HIF_PROMOTE, &gh->gh_iflags) == 0);
 	if (!relaxed_state_ok(gl->gl_state, gh->gh_state, gh->gh_flags)) {

@@ -489,14 +488,6 @@ static int rq_promote(struct gfs2_glock *gl)
 			gl->gl_req_gh = gh;
 			set_bit(GLF_LOCK, &gl->gl_flags);
 			spin_unlock(&gl->gl_spin);
-
-			if (atomic_read(&sdp->sd_reclaim_count) >
-			    gfs2_tune_get(sdp, gt_reclaim_limit) &&
-			    !(gh->gh_flags & LM_FLAG_PRIORITY)) {
-				gfs2_reclaim_glock(sdp);
-				gfs2_reclaim_glock(sdp);
-			}
-
 			gfs2_glock_xmote_th(gh->gh_gl, gh);
 			spin_lock(&gl->gl_spin);
 		}

fs/gfs2/incore.h +0 -1

index 8b91f83..4a48ded 100644
@@ -446,7 +446,6 @@ struct gfs2_tune {
 	unsigned int gt_lockdump_size;
 	unsigned int gt_stall_secs; /* Detects trouble! */
 	unsigned int gt_complain_secs;
-	unsigned int gt_reclaim_limit; /* Max num of glocks in reclaim list */
 	unsigned int gt_statfs_quantum;
 	unsigned int gt_statfs_slow;
 };

fs/gfs2/super.c +0 -1

index f916b97..78987db 100644
@@ -75,7 +75,6 @@ void gfs2_tune_init(struct gfs2_tune *gt)
 	gt->gt_lockdump_size = 131072;
 	gt->gt_stall_secs = 600;
 	gt->gt_complain_secs = 10;
-	gt->gt_reclaim_limit = 5000;
 	gt->gt_statfs_quantum = 30;
 	gt->gt_statfs_slow = 0;
 }

fs/gfs2/sys.c +0 -2

index c26c21b..b3ba9f1 100644
@@ -434,7 +434,6 @@ TUNE_ATTR(quota_quantum, 0);
 TUNE_ATTR(atime_quantum, 0);
 TUNE_ATTR(max_readahead, 0);
 TUNE_ATTR(complain_secs, 0);
-TUNE_ATTR(reclaim_limit, 0);
 TUNE_ATTR(statfs_slow, 0);
 TUNE_ATTR(new_files_jdata, 0);
 TUNE_ATTR(new_files_directio, 0);

@@ -458,7 +457,6 @@ static struct attribute *tune_attrs[] = {
 	&tune_attr_atime_quantum.attr,
 	&tune_attr_max_readahead.attr,
 	&tune_attr_complain_secs.attr,
-	&tune_attr_reclaim_limit.attr,
 	&tune_attr_statfs_slow.attr,
 	&tune_attr_quota_simul_sync.attr,
 	&tune_attr_quota_cache_secs.attr,

Signature · 1 line

1.5.1.2

David Teigland

2007-08-02 13:39 UTC #6 lore raw

PATCH 1/1 Clean up run_queue()

In reply to Steven Whitehouse

Quoted 11 lines · Steven Whitehouse

A further clean up to make the logic a bit more obvious and remove a local variable which is no longer required.
Signed-off-by Steven Whitehouse <[email protected]>
fs/gfs2/glock.c +0 -0
index 65d6eba..79d61ff 100644
@@ -564,24 +564,19 @@ static int rq_demote(struct gfs2_glock *gl)
  */

fs/gfs2/glock.c static int rq_demote(struct gfs2_glock *gl) · line 567

Quoted 10 lines

fs/gfs2/glock.c +1 -7

 static void run_queue(struct gfs2_glock *gl)
 {
-	int blocked;
-
-	for (;;) {
-		blocked = 1;
-		if (test_bit(GLF_LOCK, &gl->gl_flags))
-			break;
-
+	while(test_bit(GLF_LOCK, &gl->gl_flags) == 0) {

CodingStyle requires a space after while