All of lore.kernel.org
 help / color / mirror / Atom feed
* [Cluster-devel] [GFS2 PATCH] GFS2: Change recovery to use mutex instead of spin_lock
       [not found] <2144052809.17222072.1492627907851.JavaMail.zimbra@redhat.com>
@ 2017-04-19 18:52 ` Bob Peterson
  2017-04-26 16:15   ` Steven Whitehouse
  0 siblings, 1 reply; 3+ messages in thread
From: Bob Peterson @ 2017-04-19 18:52 UTC (permalink / raw)
  To: cluster-devel.redhat.com

Hi,

GFS2's recovery procedure relied heavily on spin_locks to protect
certain variables, but spin_locks consume CPU time that is better
spent allowing things like corosync to run. This patch switches it
to a mutex, which will sleep until it becomes available.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/incore.h   |  2 +-
 fs/gfs2/lock_dlm.c | 74 +++++++++++++++++++++++++++---------------------------
 2 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index b7cf65d..b8049be 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -661,7 +661,7 @@ struct lm_lockstruct {
 	struct completion ls_sync_wait; /* {control,mounted}_{lock,unlock} */
 	char *ls_lvb_bits;
 
-	spinlock_t ls_recover_spin; /* protects following fields */
+	struct mutex ls_recover_mutex; /* protects following fields */
 	unsigned long ls_recover_flags; /* DFL_ */
 	uint32_t ls_recover_mount; /* gen in first recover_done cb */
 	uint32_t ls_recover_start; /* gen in last recover_done cb */
diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c
index 0515f0a..b2fd828 100644
--- a/fs/gfs2/lock_dlm.c
+++ b/fs/gfs2/lock_dlm.c
@@ -585,7 +585,7 @@ static void gfs2_control_func(struct work_struct *work)
 	int recover_size;
 	int i, error;
 
-	spin_lock(&ls->ls_recover_spin);
+	mutex_lock(&ls->ls_recover_mutex);
 	/*
 	 * No MOUNT_DONE means we're still mounting; control_mount()
 	 * will set this flag, after which this thread will take over
@@ -597,12 +597,12 @@ static void gfs2_control_func(struct work_struct *work)
 	 */
 	if (!test_bit(DFL_MOUNT_DONE, &ls->ls_recover_flags) ||
 	     test_bit(DFL_FIRST_MOUNT, &ls->ls_recover_flags)) {
-		spin_unlock(&ls->ls_recover_spin);
+		mutex_unlock(&ls->ls_recover_mutex);
 		return;
 	}
 	block_gen = ls->ls_recover_block;
 	start_gen = ls->ls_recover_start;
-	spin_unlock(&ls->ls_recover_spin);
+	mutex_unlock(&ls->ls_recover_mutex);
 
 	/*
 	 * Equal block_gen and start_gen implies we are between
@@ -634,12 +634,12 @@ static void gfs2_control_func(struct work_struct *work)
 
 	control_lvb_read(ls, &lvb_gen, ls->ls_lvb_bits);
 
-	spin_lock(&ls->ls_recover_spin);
+	mutex_lock(&ls->ls_recover_mutex);
 	if (block_gen != ls->ls_recover_block ||
 	    start_gen != ls->ls_recover_start) {
 		fs_info(sdp, "recover generation %u block1 %u %u\n",
 			start_gen, block_gen, ls->ls_recover_block);
-		spin_unlock(&ls->ls_recover_spin);
+		mutex_unlock(&ls->ls_recover_mutex);
 		control_lock(sdp, DLM_LOCK_NL, DLM_LKF_CONVERT);
 		return;
 	}
@@ -700,7 +700,7 @@ static void gfs2_control_func(struct work_struct *work)
 		 * we should be getting a recover_done() for lvb_gen soon
 		 */
 	}
-	spin_unlock(&ls->ls_recover_spin);
+	mutex_unlock(&ls->ls_recover_mutex);
 
 	if (write_lvb) {
 		control_lvb_write(ls, start_gen, ls->ls_lvb_bits);
@@ -739,17 +739,17 @@ static void gfs2_control_func(struct work_struct *work)
 	 * again while working above)
 	 */
 
-	spin_lock(&ls->ls_recover_spin);
+	mutex_lock(&ls->ls_recover_mutex);
 	if (ls->ls_recover_block == block_gen &&
 	    ls->ls_recover_start == start_gen) {
 		clear_bit(DFL_BLOCK_LOCKS, &ls->ls_recover_flags);
-		spin_unlock(&ls->ls_recover_spin);
+		mutex_unlock(&ls->ls_recover_mutex);
 		fs_info(sdp, "recover generation %u done\n", start_gen);
 		gfs2_glock_thaw(sdp);
 	} else {
 		fs_info(sdp, "recover generation %u block2 %u %u\n",
 			start_gen, block_gen, ls->ls_recover_block);
-		spin_unlock(&ls->ls_recover_spin);
+		mutex_unlock(&ls->ls_recover_mutex);
 	}
 }
 
@@ -865,11 +865,11 @@ static int control_mount(struct gfs2_sbd *sdp)
 
 	if (mounted_mode == DLM_LOCK_EX) {
 		/* first mounter, keep both EX while doing first recovery */
-		spin_lock(&ls->ls_recover_spin);
+		mutex_lock(&ls->ls_recover_mutex);
 		clear_bit(DFL_BLOCK_LOCKS, &ls->ls_recover_flags);
 		set_bit(DFL_MOUNT_DONE, &ls->ls_recover_flags);
 		set_bit(DFL_FIRST_MOUNT, &ls->ls_recover_flags);
-		spin_unlock(&ls->ls_recover_spin);
+		mutex_unlock(&ls->ls_recover_mutex);
 		fs_info(sdp, "first mounter control generation %u\n", lvb_gen);
 		return 0;
 	}
@@ -890,7 +890,7 @@ static int control_mount(struct gfs2_sbd *sdp)
 		goto restart;
 	}
 
-	spin_lock(&ls->ls_recover_spin);
+	mutex_lock(&ls->ls_recover_mutex);
 	block_gen = ls->ls_recover_block;
 	start_gen = ls->ls_recover_start;
 	mount_gen = ls->ls_recover_mount;
@@ -901,7 +901,7 @@ static int control_mount(struct gfs2_sbd *sdp)
 		fs_info(sdp, "control_mount wait1 block %u start %u mount %u "
 			"lvb %u flags %lx\n", block_gen, start_gen, mount_gen,
 			lvb_gen, ls->ls_recover_flags);
-		spin_unlock(&ls->ls_recover_spin);
+		mutex_unlock(&ls->ls_recover_mutex);
 		goto restart;
 	}
 
@@ -911,7 +911,7 @@ static int control_mount(struct gfs2_sbd *sdp)
 		fs_info(sdp, "control_mount wait2 block %u start %u mount %u "
 			"lvb %u flags %lx\n", block_gen, start_gen, mount_gen,
 			lvb_gen, ls->ls_recover_flags);
-		spin_unlock(&ls->ls_recover_spin);
+		mutex_unlock(&ls->ls_recover_mutex);
 		goto restart;
 	}
 
@@ -920,7 +920,7 @@ static int control_mount(struct gfs2_sbd *sdp)
 		fs_info(sdp, "control_mount wait3 block %u start %u mount %u "
 			"lvb %u flags %lx\n", block_gen, start_gen, mount_gen,
 			lvb_gen, ls->ls_recover_flags);
-		spin_unlock(&ls->ls_recover_spin);
+		mutex_unlock(&ls->ls_recover_mutex);
 		goto restart;
 	}
 
@@ -928,7 +928,7 @@ static int control_mount(struct gfs2_sbd *sdp)
 	set_bit(DFL_MOUNT_DONE, &ls->ls_recover_flags);
 	memset(ls->ls_recover_submit, 0, ls->ls_recover_size*sizeof(uint32_t));
 	memset(ls->ls_recover_result, 0, ls->ls_recover_size*sizeof(uint32_t));
-	spin_unlock(&ls->ls_recover_spin);
+	mutex_unlock(&ls->ls_recover_mutex);
 	return 0;
 
 fail:
@@ -944,7 +944,7 @@ static int control_first_done(struct gfs2_sbd *sdp)
 	int error;
 
 restart:
-	spin_lock(&ls->ls_recover_spin);
+	mutex_lock(&ls->ls_recover_mutex);
 	start_gen = ls->ls_recover_start;
 	block_gen = ls->ls_recover_block;
 
@@ -954,7 +954,7 @@ static int control_first_done(struct gfs2_sbd *sdp)
 		/* sanity check, should not happen */
 		fs_err(sdp, "control_first_done start %u block %u flags %lx\n",
 		       start_gen, block_gen, ls->ls_recover_flags);
-		spin_unlock(&ls->ls_recover_spin);
+		mutex_unlock(&ls->ls_recover_mutex);
 		control_unlock(sdp);
 		return -1;
 	}
@@ -967,7 +967,7 @@ static int control_first_done(struct gfs2_sbd *sdp)
 		 * because we are still the first mounter and any failed nodes
 		 * have not fully mounted, so they don't need recovery.
 		 */
-		spin_unlock(&ls->ls_recover_spin);
+		mutex_unlock(&ls->ls_recover_mutex);
 		fs_info(sdp, "control_first_done wait gen %u\n", start_gen);
 
 		wait_on_bit(&ls->ls_recover_flags, DFL_DLM_RECOVERY,
@@ -979,7 +979,7 @@ static int control_first_done(struct gfs2_sbd *sdp)
 	set_bit(DFL_FIRST_MOUNT_DONE, &ls->ls_recover_flags);
 	memset(ls->ls_recover_submit, 0, ls->ls_recover_size*sizeof(uint32_t));
 	memset(ls->ls_recover_result, 0, ls->ls_recover_size*sizeof(uint32_t));
-	spin_unlock(&ls->ls_recover_spin);
+	mutex_unlock(&ls->ls_recover_mutex);
 
 	memset(ls->ls_lvb_bits, 0, GDLM_LVB_SIZE);
 	control_lvb_write(ls, start_gen, ls->ls_lvb_bits);
@@ -1039,7 +1039,7 @@ static int set_recover_size(struct gfs2_sbd *sdp, struct dlm_slot *slots,
 		return -ENOMEM;
 	}
 
-	spin_lock(&ls->ls_recover_spin);
+	mutex_lock(&ls->ls_recover_mutex);
 	memcpy(submit, ls->ls_recover_submit, old_size * sizeof(uint32_t));
 	memcpy(result, ls->ls_recover_result, old_size * sizeof(uint32_t));
 	kfree(ls->ls_recover_submit);
@@ -1047,7 +1047,7 @@ static int set_recover_size(struct gfs2_sbd *sdp, struct dlm_slot *slots,
 	ls->ls_recover_submit = submit;
 	ls->ls_recover_result = result;
 	ls->ls_recover_size = new_size;
-	spin_unlock(&ls->ls_recover_spin);
+	mutex_unlock(&ls->ls_recover_mutex);
 	return 0;
 }
 
@@ -1068,17 +1068,17 @@ static void gdlm_recover_prep(void *arg)
 	struct gfs2_sbd *sdp = arg;
 	struct lm_lockstruct *ls = &sdp->sd_lockstruct;
 
-	spin_lock(&ls->ls_recover_spin);
+	mutex_lock(&ls->ls_recover_mutex);
 	ls->ls_recover_block = ls->ls_recover_start;
 	set_bit(DFL_DLM_RECOVERY, &ls->ls_recover_flags);
 
 	if (!test_bit(DFL_MOUNT_DONE, &ls->ls_recover_flags) ||
 	     test_bit(DFL_FIRST_MOUNT, &ls->ls_recover_flags)) {
-		spin_unlock(&ls->ls_recover_spin);
+		mutex_unlock(&ls->ls_recover_mutex);
 		return;
 	}
 	set_bit(DFL_BLOCK_LOCKS, &ls->ls_recover_flags);
-	spin_unlock(&ls->ls_recover_spin);
+	mutex_unlock(&ls->ls_recover_mutex);
 }
 
 /* dlm calls after recover_prep has been completed on all lockspace members;
@@ -1090,11 +1090,11 @@ static void gdlm_recover_slot(void *arg, struct dlm_slot *slot)
 	struct lm_lockstruct *ls = &sdp->sd_lockstruct;
 	int jid = slot->slot - 1;
 
-	spin_lock(&ls->ls_recover_spin);
+	mutex_lock(&ls->ls_recover_mutex);
 	if (ls->ls_recover_size < jid + 1) {
 		fs_err(sdp, "recover_slot jid %d gen %u short size %d",
 		       jid, ls->ls_recover_block, ls->ls_recover_size);
-		spin_unlock(&ls->ls_recover_spin);
+		mutex_unlock(&ls->ls_recover_mutex);
 		return;
 	}
 
@@ -1103,7 +1103,7 @@ static void gdlm_recover_slot(void *arg, struct dlm_slot *slot)
 			jid, ls->ls_recover_block, ls->ls_recover_submit[jid]);
 	}
 	ls->ls_recover_submit[jid] = ls->ls_recover_block;
-	spin_unlock(&ls->ls_recover_spin);
+	mutex_unlock(&ls->ls_recover_mutex);
 }
 
 /* dlm calls after recover_slot and after it completes lock recovery */
@@ -1117,7 +1117,7 @@ static void gdlm_recover_done(void *arg, struct dlm_slot *slots, int num_slots,
 	/* ensure the ls jid arrays are large enough */
 	set_recover_size(sdp, slots, num_slots);
 
-	spin_lock(&ls->ls_recover_spin);
+	mutex_lock(&ls->ls_recover_mutex);
 	ls->ls_recover_start = generation;
 
 	if (!ls->ls_recover_mount) {
@@ -1131,7 +1131,7 @@ static void gdlm_recover_done(void *arg, struct dlm_slot *slots, int num_slots,
 	clear_bit(DFL_DLM_RECOVERY, &ls->ls_recover_flags);
 	smp_mb__after_atomic();
 	wake_up_bit(&ls->ls_recover_flags, DFL_DLM_RECOVERY);
-	spin_unlock(&ls->ls_recover_spin);
+	mutex_unlock(&ls->ls_recover_mutex);
 }
 
 /* gfs2_recover thread has a journal recovery result */
@@ -1148,15 +1148,15 @@ static void gdlm_recovery_result(struct gfs2_sbd *sdp, unsigned int jid,
 	if (jid == ls->ls_jid)
 		return;
 
-	spin_lock(&ls->ls_recover_spin);
+	mutex_lock(&ls->ls_recover_mutex);
 	if (test_bit(DFL_FIRST_MOUNT, &ls->ls_recover_flags)) {
-		spin_unlock(&ls->ls_recover_spin);
+		mutex_unlock(&ls->ls_recover_mutex);
 		return;
 	}
 	if (ls->ls_recover_size < jid + 1) {
 		fs_err(sdp, "recovery_result jid %d short size %d",
 		       jid, ls->ls_recover_size);
-		spin_unlock(&ls->ls_recover_spin);
+		mutex_unlock(&ls->ls_recover_mutex);
 		return;
 	}
 
@@ -1172,7 +1172,7 @@ static void gdlm_recovery_result(struct gfs2_sbd *sdp, unsigned int jid,
 	if (!test_bit(DFL_UNMOUNT, &ls->ls_recover_flags))
 		queue_delayed_work(gfs2_control_wq, &sdp->sd_control_work,
 				   result == LM_RD_GAVEUP ? HZ : 0);
-	spin_unlock(&ls->ls_recover_spin);
+	mutex_unlock(&ls->ls_recover_mutex);
 }
 
 const struct dlm_lockspace_ops gdlm_lockspace_ops = {
@@ -1194,7 +1194,7 @@ static int gdlm_mount(struct gfs2_sbd *sdp, const char *table)
 	 */
 
 	INIT_DELAYED_WORK(&sdp->sd_control_work, gfs2_control_func);
-	spin_lock_init(&ls->ls_recover_spin);
+	mutex_init(&ls->ls_recover_mutex);
 	ls->ls_recover_flags = 0;
 	ls->ls_recover_mount = 0;
 	ls->ls_recover_start = 0;
@@ -1300,9 +1300,9 @@ static void gdlm_unmount(struct gfs2_sbd *sdp)
 
 	/* wait for gfs2_control_wq to be done with this mount */
 
-	spin_lock(&ls->ls_recover_spin);
+	mutex_lock(&ls->ls_recover_mutex);
 	set_bit(DFL_UNMOUNT, &ls->ls_recover_flags);
-	spin_unlock(&ls->ls_recover_spin);
+	mutex_unlock(&ls->ls_recover_mutex);
 	flush_delayed_work(&sdp->sd_control_work);
 
 	/* mounted_lock and control_lock will be purged in dlm recovery */



^ permalink raw reply related	[flat|nested] 3+ messages in thread
* [Cluster-devel] [GFS2 PATCH] GFS2: Change recovery to use mutex instead of spin_lock
  2017-04-19 18:52 ` [Cluster-devel] [GFS2 PATCH] GFS2: Change recovery to use mutex instead of spin_lock Bob Peterson
@ 2017-04-26 16:15   ` Steven Whitehouse
  2017-04-26 20:28     ` Bob Peterson
  0 siblings, 1 reply; 3+ messages in thread
From: Steven Whitehouse @ 2017-04-26 16:15 UTC (permalink / raw)
  To: cluster-devel.redhat.com

Hi,



On 19/04/17 19:52, Bob Peterson wrote:
> Hi,
>
> GFS2's recovery procedure relied heavily on spin_locks to protect
> certain variables, but spin_locks consume CPU time that is better
> spent allowing things like corosync to run. This patch switches it
> to a mutex, which will sleep until it becomes available.
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
If this is a spin lock, then it should not be covering anything that 
blocks. Looking down lock_dlm.c, I don't see anything that obviously is 
going to take a long time, so I wonder what is the real problem here? Is 
there something under the spinlock taking a long time, or is one of the 
bits of code it protects being called a very large number of times?

Mostly it seems to be covering the recovery state, and that shouldn't 
take very long to update, particularly compared with longer running 
operations such as journal recovery, so I think we need to look a bit 
harder at what is going on here,

Steve.

> ---
>   fs/gfs2/incore.h   |  2 +-
>   fs/gfs2/lock_dlm.c | 74 +++++++++++++++++++++++++++---------------------------
>   2 files changed, 38 insertions(+), 38 deletions(-)
>
> diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
> index b7cf65d..b8049be 100644
> --- a/fs/gfs2/incore.h
> +++ b/fs/gfs2/incore.h
> @@ -661,7 +661,7 @@ struct lm_lockstruct {
>   	struct completion ls_sync_wait; /* {control,mounted}_{lock,unlock} */
>   	char *ls_lvb_bits;
>   
> -	spinlock_t ls_recover_spin; /* protects following fields */
> +	struct mutex ls_recover_mutex; /* protects following fields */
>   	unsigned long ls_recover_flags; /* DFL_ */
>   	uint32_t ls_recover_mount; /* gen in first recover_done cb */
>   	uint32_t ls_recover_start; /* gen in last recover_done cb */
> diff --git a/fs/gfs2/lock_dlm.c b/fs/gfs2/lock_dlm.c
> index 0515f0a..b2fd828 100644
> --- a/fs/gfs2/lock_dlm.c
> +++ b/fs/gfs2/lock_dlm.c
> @@ -585,7 +585,7 @@ static void gfs2_control_func(struct work_struct *work)
>   	int recover_size;
>   	int i, error;
>   
> -	spin_lock(&ls->ls_recover_spin);
> +	mutex_lock(&ls->ls_recover_mutex);
>   	/*
>   	 * No MOUNT_DONE means we're still mounting; control_mount()
>   	 * will set this flag, after which this thread will take over
> @@ -597,12 +597,12 @@ static void gfs2_control_func(struct work_struct *work)
>   	 */
>   	if (!test_bit(DFL_MOUNT_DONE, &ls->ls_recover_flags) ||
>   	     test_bit(DFL_FIRST_MOUNT, &ls->ls_recover_flags)) {
> -		spin_unlock(&ls->ls_recover_spin);
> +		mutex_unlock(&ls->ls_recover_mutex);
>   		return;
>   	}
>   	block_gen = ls->ls_recover_block;
>   	start_gen = ls->ls_recover_start;
> -	spin_unlock(&ls->ls_recover_spin);
> +	mutex_unlock(&ls->ls_recover_mutex);
>   
>   	/*
>   	 * Equal block_gen and start_gen implies we are between
> @@ -634,12 +634,12 @@ static void gfs2_control_func(struct work_struct *work)
>   
>   	control_lvb_read(ls, &lvb_gen, ls->ls_lvb_bits);
>   
> -	spin_lock(&ls->ls_recover_spin);
> +	mutex_lock(&ls->ls_recover_mutex);
>   	if (block_gen != ls->ls_recover_block ||
>   	    start_gen != ls->ls_recover_start) {
>   		fs_info(sdp, "recover generation %u block1 %u %u\n",
>   			start_gen, block_gen, ls->ls_recover_block);
> -		spin_unlock(&ls->ls_recover_spin);
> +		mutex_unlock(&ls->ls_recover_mutex);
>   		control_lock(sdp, DLM_LOCK_NL, DLM_LKF_CONVERT);
>   		return;
>   	}
> @@ -700,7 +700,7 @@ static void gfs2_control_func(struct work_struct *work)
>   		 * we should be getting a recover_done() for lvb_gen soon
>   		 */
>   	}
> -	spin_unlock(&ls->ls_recover_spin);
> +	mutex_unlock(&ls->ls_recover_mutex);
>   
>   	if (write_lvb) {
>   		control_lvb_write(ls, start_gen, ls->ls_lvb_bits);
> @@ -739,17 +739,17 @@ static void gfs2_control_func(struct work_struct *work)
>   	 * again while working above)
>   	 */
>   
> -	spin_lock(&ls->ls_recover_spin);
> +	mutex_lock(&ls->ls_recover_mutex);
>   	if (ls->ls_recover_block == block_gen &&
>   	    ls->ls_recover_start == start_gen) {
>   		clear_bit(DFL_BLOCK_LOCKS, &ls->ls_recover_flags);
> -		spin_unlock(&ls->ls_recover_spin);
> +		mutex_unlock(&ls->ls_recover_mutex);
>   		fs_info(sdp, "recover generation %u done\n", start_gen);
>   		gfs2_glock_thaw(sdp);
>   	} else {
>   		fs_info(sdp, "recover generation %u block2 %u %u\n",
>   			start_gen, block_gen, ls->ls_recover_block);
> -		spin_unlock(&ls->ls_recover_spin);
> +		mutex_unlock(&ls->ls_recover_mutex);
>   	}
>   }
>   
> @@ -865,11 +865,11 @@ static int control_mount(struct gfs2_sbd *sdp)
>   
>   	if (mounted_mode == DLM_LOCK_EX) {
>   		/* first mounter, keep both EX while doing first recovery */
> -		spin_lock(&ls->ls_recover_spin);
> +		mutex_lock(&ls->ls_recover_mutex);
>   		clear_bit(DFL_BLOCK_LOCKS, &ls->ls_recover_flags);
>   		set_bit(DFL_MOUNT_DONE, &ls->ls_recover_flags);
>   		set_bit(DFL_FIRST_MOUNT, &ls->ls_recover_flags);
> -		spin_unlock(&ls->ls_recover_spin);
> +		mutex_unlock(&ls->ls_recover_mutex);
>   		fs_info(sdp, "first mounter control generation %u\n", lvb_gen);
>   		return 0;
>   	}
> @@ -890,7 +890,7 @@ static int control_mount(struct gfs2_sbd *sdp)
>   		goto restart;
>   	}
>   
> -	spin_lock(&ls->ls_recover_spin);
> +	mutex_lock(&ls->ls_recover_mutex);
>   	block_gen = ls->ls_recover_block;
>   	start_gen = ls->ls_recover_start;
>   	mount_gen = ls->ls_recover_mount;
> @@ -901,7 +901,7 @@ static int control_mount(struct gfs2_sbd *sdp)
>   		fs_info(sdp, "control_mount wait1 block %u start %u mount %u "
>   			"lvb %u flags %lx\n", block_gen, start_gen, mount_gen,
>   			lvb_gen, ls->ls_recover_flags);
> -		spin_unlock(&ls->ls_recover_spin);
> +		mutex_unlock(&ls->ls_recover_mutex);
>   		goto restart;
>   	}
>   
> @@ -911,7 +911,7 @@ static int control_mount(struct gfs2_sbd *sdp)
>   		fs_info(sdp, "control_mount wait2 block %u start %u mount %u "
>   			"lvb %u flags %lx\n", block_gen, start_gen, mount_gen,
>   			lvb_gen, ls->ls_recover_flags);
> -		spin_unlock(&ls->ls_recover_spin);
> +		mutex_unlock(&ls->ls_recover_mutex);
>   		goto restart;
>   	}
>   
> @@ -920,7 +920,7 @@ static int control_mount(struct gfs2_sbd *sdp)
>   		fs_info(sdp, "control_mount wait3 block %u start %u mount %u "
>   			"lvb %u flags %lx\n", block_gen, start_gen, mount_gen,
>   			lvb_gen, ls->ls_recover_flags);
> -		spin_unlock(&ls->ls_recover_spin);
> +		mutex_unlock(&ls->ls_recover_mutex);
>   		goto restart;
>   	}
>   
> @@ -928,7 +928,7 @@ static int control_mount(struct gfs2_sbd *sdp)
>   	set_bit(DFL_MOUNT_DONE, &ls->ls_recover_flags);
>   	memset(ls->ls_recover_submit, 0, ls->ls_recover_size*sizeof(uint32_t));
>   	memset(ls->ls_recover_result, 0, ls->ls_recover_size*sizeof(uint32_t));
> -	spin_unlock(&ls->ls_recover_spin);
> +	mutex_unlock(&ls->ls_recover_mutex);
>   	return 0;
>   
>   fail:
> @@ -944,7 +944,7 @@ static int control_first_done(struct gfs2_sbd *sdp)
>   	int error;
>   
>   restart:
> -	spin_lock(&ls->ls_recover_spin);
> +	mutex_lock(&ls->ls_recover_mutex);
>   	start_gen = ls->ls_recover_start;
>   	block_gen = ls->ls_recover_block;
>   
> @@ -954,7 +954,7 @@ static int control_first_done(struct gfs2_sbd *sdp)
>   		/* sanity check, should not happen */
>   		fs_err(sdp, "control_first_done start %u block %u flags %lx\n",
>   		       start_gen, block_gen, ls->ls_recover_flags);
> -		spin_unlock(&ls->ls_recover_spin);
> +		mutex_unlock(&ls->ls_recover_mutex);
>   		control_unlock(sdp);
>   		return -1;
>   	}
> @@ -967,7 +967,7 @@ static int control_first_done(struct gfs2_sbd *sdp)
>   		 * because we are still the first mounter and any failed nodes
>   		 * have not fully mounted, so they don't need recovery.
>   		 */
> -		spin_unlock(&ls->ls_recover_spin);
> +		mutex_unlock(&ls->ls_recover_mutex);
>   		fs_info(sdp, "control_first_done wait gen %u\n", start_gen);
>   
>   		wait_on_bit(&ls->ls_recover_flags, DFL_DLM_RECOVERY,
> @@ -979,7 +979,7 @@ static int control_first_done(struct gfs2_sbd *sdp)
>   	set_bit(DFL_FIRST_MOUNT_DONE, &ls->ls_recover_flags);
>   	memset(ls->ls_recover_submit, 0, ls->ls_recover_size*sizeof(uint32_t));
>   	memset(ls->ls_recover_result, 0, ls->ls_recover_size*sizeof(uint32_t));
> -	spin_unlock(&ls->ls_recover_spin);
> +	mutex_unlock(&ls->ls_recover_mutex);
>   
>   	memset(ls->ls_lvb_bits, 0, GDLM_LVB_SIZE);
>   	control_lvb_write(ls, start_gen, ls->ls_lvb_bits);
> @@ -1039,7 +1039,7 @@ static int set_recover_size(struct gfs2_sbd *sdp, struct dlm_slot *slots,
>   		return -ENOMEM;
>   	}
>   
> -	spin_lock(&ls->ls_recover_spin);
> +	mutex_lock(&ls->ls_recover_mutex);
>   	memcpy(submit, ls->ls_recover_submit, old_size * sizeof(uint32_t));
>   	memcpy(result, ls->ls_recover_result, old_size * sizeof(uint32_t));
>   	kfree(ls->ls_recover_submit);
> @@ -1047,7 +1047,7 @@ static int set_recover_size(struct gfs2_sbd *sdp, struct dlm_slot *slots,
>   	ls->ls_recover_submit = submit;
>   	ls->ls_recover_result = result;
>   	ls->ls_recover_size = new_size;
> -	spin_unlock(&ls->ls_recover_spin);
> +	mutex_unlock(&ls->ls_recover_mutex);
>   	return 0;
>   }
>   
> @@ -1068,17 +1068,17 @@ static void gdlm_recover_prep(void *arg)
>   	struct gfs2_sbd *sdp = arg;
>   	struct lm_lockstruct *ls = &sdp->sd_lockstruct;
>   
> -	spin_lock(&ls->ls_recover_spin);
> +	mutex_lock(&ls->ls_recover_mutex);
>   	ls->ls_recover_block = ls->ls_recover_start;
>   	set_bit(DFL_DLM_RECOVERY, &ls->ls_recover_flags);
>   
>   	if (!test_bit(DFL_MOUNT_DONE, &ls->ls_recover_flags) ||
>   	     test_bit(DFL_FIRST_MOUNT, &ls->ls_recover_flags)) {
> -		spin_unlock(&ls->ls_recover_spin);
> +		mutex_unlock(&ls->ls_recover_mutex);
>   		return;
>   	}
>   	set_bit(DFL_BLOCK_LOCKS, &ls->ls_recover_flags);
> -	spin_unlock(&ls->ls_recover_spin);
> +	mutex_unlock(&ls->ls_recover_mutex);
>   }
>   
>   /* dlm calls after recover_prep has been completed on all lockspace members;
> @@ -1090,11 +1090,11 @@ static void gdlm_recover_slot(void *arg, struct dlm_slot *slot)
>   	struct lm_lockstruct *ls = &sdp->sd_lockstruct;
>   	int jid = slot->slot - 1;
>   
> -	spin_lock(&ls->ls_recover_spin);
> +	mutex_lock(&ls->ls_recover_mutex);
>   	if (ls->ls_recover_size < jid + 1) {
>   		fs_err(sdp, "recover_slot jid %d gen %u short size %d",
>   		       jid, ls->ls_recover_block, ls->ls_recover_size);
> -		spin_unlock(&ls->ls_recover_spin);
> +		mutex_unlock(&ls->ls_recover_mutex);
>   		return;
>   	}
>   
> @@ -1103,7 +1103,7 @@ static void gdlm_recover_slot(void *arg, struct dlm_slot *slot)
>   			jid, ls->ls_recover_block, ls->ls_recover_submit[jid]);
>   	}
>   	ls->ls_recover_submit[jid] = ls->ls_recover_block;
> -	spin_unlock(&ls->ls_recover_spin);
> +	mutex_unlock(&ls->ls_recover_mutex);
>   }
>   
>   /* dlm calls after recover_slot and after it completes lock recovery */
> @@ -1117,7 +1117,7 @@ static void gdlm_recover_done(void *arg, struct dlm_slot *slots, int num_slots,
>   	/* ensure the ls jid arrays are large enough */
>   	set_recover_size(sdp, slots, num_slots);
>   
> -	spin_lock(&ls->ls_recover_spin);
> +	mutex_lock(&ls->ls_recover_mutex);
>   	ls->ls_recover_start = generation;
>   
>   	if (!ls->ls_recover_mount) {
> @@ -1131,7 +1131,7 @@ static void gdlm_recover_done(void *arg, struct dlm_slot *slots, int num_slots,
>   	clear_bit(DFL_DLM_RECOVERY, &ls->ls_recover_flags);
>   	smp_mb__after_atomic();
>   	wake_up_bit(&ls->ls_recover_flags, DFL_DLM_RECOVERY);
> -	spin_unlock(&ls->ls_recover_spin);
> +	mutex_unlock(&ls->ls_recover_mutex);
>   }
>   
>   /* gfs2_recover thread has a journal recovery result */
> @@ -1148,15 +1148,15 @@ static void gdlm_recovery_result(struct gfs2_sbd *sdp, unsigned int jid,
>   	if (jid == ls->ls_jid)
>   		return;
>   
> -	spin_lock(&ls->ls_recover_spin);
> +	mutex_lock(&ls->ls_recover_mutex);
>   	if (test_bit(DFL_FIRST_MOUNT, &ls->ls_recover_flags)) {
> -		spin_unlock(&ls->ls_recover_spin);
> +		mutex_unlock(&ls->ls_recover_mutex);
>   		return;
>   	}
>   	if (ls->ls_recover_size < jid + 1) {
>   		fs_err(sdp, "recovery_result jid %d short size %d",
>   		       jid, ls->ls_recover_size);
> -		spin_unlock(&ls->ls_recover_spin);
> +		mutex_unlock(&ls->ls_recover_mutex);
>   		return;
>   	}
>   
> @@ -1172,7 +1172,7 @@ static void gdlm_recovery_result(struct gfs2_sbd *sdp, unsigned int jid,
>   	if (!test_bit(DFL_UNMOUNT, &ls->ls_recover_flags))
>   		queue_delayed_work(gfs2_control_wq, &sdp->sd_control_work,
>   				   result == LM_RD_GAVEUP ? HZ : 0);
> -	spin_unlock(&ls->ls_recover_spin);
> +	mutex_unlock(&ls->ls_recover_mutex);
>   }
>   
>   const struct dlm_lockspace_ops gdlm_lockspace_ops = {
> @@ -1194,7 +1194,7 @@ static int gdlm_mount(struct gfs2_sbd *sdp, const char *table)
>   	 */
>   
>   	INIT_DELAYED_WORK(&sdp->sd_control_work, gfs2_control_func);
> -	spin_lock_init(&ls->ls_recover_spin);
> +	mutex_init(&ls->ls_recover_mutex);
>   	ls->ls_recover_flags = 0;
>   	ls->ls_recover_mount = 0;
>   	ls->ls_recover_start = 0;
> @@ -1300,9 +1300,9 @@ static void gdlm_unmount(struct gfs2_sbd *sdp)
>   
>   	/* wait for gfs2_control_wq to be done with this mount */
>   
> -	spin_lock(&ls->ls_recover_spin);
> +	mutex_lock(&ls->ls_recover_mutex);
>   	set_bit(DFL_UNMOUNT, &ls->ls_recover_flags);
> -	spin_unlock(&ls->ls_recover_spin);
> +	mutex_unlock(&ls->ls_recover_mutex);
>   	flush_delayed_work(&sdp->sd_control_work);
>   
>   	/* mounted_lock and control_lock will be purged in dlm recovery */
>



^ permalink raw reply	[flat|nested] 3+ messages in thread
* [Cluster-devel] [GFS2 PATCH] GFS2: Change recovery to use mutex instead of spin_lock
  2017-04-26 16:15   ` Steven Whitehouse
@ 2017-04-26 20:28     ` Bob Peterson
  0 siblings, 0 replies; 3+ messages in thread
From: Bob Peterson @ 2017-04-26 20:28 UTC (permalink / raw)
  To: cluster-devel.redhat.com

----- Original Message -----
| If this is a spin lock, then it should not be covering anything that
| blocks. Looking down lock_dlm.c, I don't see anything that obviously is
| going to take a long time, so I wonder what is the real problem here? Is
| there something under the spinlock taking a long time, or is one of the
| bits of code it protects being called a very large number of times?
| 
| Mostly it seems to be covering the recovery state, and that shouldn't
| take very long to update, particularly compared with longer running
| operations such as journal recovery, so I think we need to look a bit
| harder at what is going on here,
| 
| Steve.

Based on your email, I studied how the spin_lock was used.
It turns out that it's not called that often, but there are some places
where recovery functions take the spin_lock then call queue_delayed_work,
which can block on its own spin_lock.

In my case, I've got 60 GFS2 mount points, and 16 cpus, and therefore
60 of everything. So maybe that's a better thing to address here.
I'll see if I can rework it to queue the work after the spin_lock is
released without harm.

Regards,

Bob Peterson
Red Hat File Systems



^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-04-26 20:28 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <2144052809.17222072.1492627907851.JavaMail.zimbra@redhat.com>
2017-04-19 18:52 ` [Cluster-devel] [GFS2 PATCH] GFS2: Change recovery to use mutex instead of spin_lock Bob Peterson
2017-04-26 16:15   ` Steven Whitehouse
2017-04-26 20:28     ` Bob Peterson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.