* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).