ocfs2/dlm: remove lockres from purge when a lock is added
PATCH 1/1 ocfs2/dlm: remove lockres from purge when a lock is added
dlm_thread(when purges a lockres) races with another thread that is running on dlmlock_remote(). dlmlock_remote() can add a lock to the blocked list of the lockres without taking dlm->spinlock. This can lead a BUG() in dlm_thread because of the added lock.
Fix
We take dlm->spinlock when adding a lock to the blocked list and make sure the lockres is not in purge list. If we removed the lockres from purge list, try to add it back.
--- fs/ocfs2/dlm/dlmcommon.h | 15 +++++++++++++++ fs/ocfs2/dlm/dlmlock.c | 18 +++++++++++++++--- fs/ocfs2/dlm/dlmthread.c | 28 ++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 3 deletions(-)
fs/ocfs2/dlm/dlmcommon.h +15 -0
@@ -1002,6 +1002,9 @@ void dlm_move_lockres_to_recovery_list(struct dlm_ctxt *dlm,/* will exit holding res->spinlock, but may drop in function */ void __dlm_wait_on_lockres_flags (struct dlm_lock_resource *res,int flags); +void __dlm_wait_on_lockres_flags_lock_dlm (struct dlm_ctxt *dlm, +struct dlm_lock_resource *res, +int flags);void __dlm_wait_on_lockres_flags_set (struct dlm_lock_resource *res,int flags);/* will exit holding res->spinlock, but may drop in function */
@@ -1012,6 +1015,18 @@ static inline void __dlm_wait_on_lockres(struct dlm_lock_resource *res) DLM_LOCK_RES_MIGRATING)); } +/* +* will exit holding dlm->spinlock and res->spinlock, but may drop in +* function +*/ +static inline void __dlm_wait_on_lockres_lock_dlm (struct dlm_ctxt *dlm, +struct dlm_lock_resource *res) +{ +__dlm_wait_on_lockres_flags_lock_dlm (dlm, res, + (DLM_LOCK_RES_IN_PROGRESS | +DLM_LOCK_RES_RECOVERING | +DLM_LOCK_RES_MIGRATING )); +}void __dlm_unlink_mle (struct dlm_ctxt *dlm,struct dlm_master_list_entry *mle);void __dlm_insert_mle (struct dlm_ctxt *dlm,struct dlm_master_list_entry *mle);
fs/ocfs2/dlm/dlmlock.c +15 -3
@@ -222,23 +222,35 @@ static enum dlm_status dlmlock_remote(struct dlm_ctxt *dlm, struct dlm_lock *lock, int flags) {enum dlm_status status =DLM_DENIED ; -int lockres_changed =1 ; +int lockres_changed =1 , recalc;mlog_entry ("type=%d\n" , lock->ml.type);mlog (0 ,"lockres %.*s, flags = 0x%x\n" , res->lockname.len, res->lockname.name, flags); +spin_lock (&dlm->spinlock);spin_lock (&res->spinlock);/* will exit this call with spinlock held */ -__dlm_wait_on_lockres (res); +__dlm_wait_on_lockres_lock_dlm (dlm, res); res->state |=DLM_LOCK_RES_IN_PROGRESS ;/* add lock to local (secondary) queue */ dlm_lock_get (lock);list_add_tail (&lock->list, &res->blocked); lock->lock_pending =1 ; +/* +* make sure this lockres is not in purge list if we remove the +* lockres from purge list, we try to add it back if locking failed. +* +* there is a race with dlm_thread purging this lockres. +* in this case, it can trigger a BUG() because we added a lock to the +* blocked list. +*/ + recalc = !list_empty (&res->purge); +__dlm_lockres_calc_usage (dlm, res);spin_unlock (&res->spinlock); +spin_unlock (&dlm->spinlock);/* spec seems to say that you will get DLM_NORMAL when the lock * has been queued, meaning we need to wait for a reply here. */
@@ -282,7 +294,7 @@ static enum dlm_status dlmlock_remote(struct dlm_ctxt *dlm, }spin_unlock (&res->spinlock); -if (lockres_changed) +if (recalc || lockres_changed)dlm_lockres_calc_usage (dlm, res);wake_up (&res->wq);
fs/ocfs2/dlm/dlmthread.c +28 -0
@@ -77,6 +77,34 @@ repeat:__set_current_state (TASK_RUNNING ); } +/* +* same as __dlm_wait_on_lockres_flags, but also hold dlm->spinlock on exit and +* may drop it in function +*/ +void __dlm_wait_on_lockres_flags_lock_dlm (struct dlm_ctxt *dlm, +struct dlm_lock_resource *res, +int flags) +{ +DECLARE_WAITQUEUE (wait, current); + +assert_spin_locked (&dlm->spinlock); +assert_spin_locked (&res->spinlock); + +add_wait_queue (&res->wq, &wait); +repeat: +set_current_state (TASK_UNINTERRUPTIBLE ); +if (res->state & flags) { +spin_unlock (&res->spinlock); +spin_unlock (&dlm->spinlock); +schedule (); +spin_lock (&dlm->spinlock); +spin_lock (&res->spinlock); + goto repeat; + } +remove_wait_queue (&res->wq, &wait); +__set_current_state (TASK_RUNNING ); +} +int __dlm_lockres_has_locks (struct dlm_lock_resource *res) { if (list_empty(&res->granted) &&
Signature · 1 line
1.6.6.1
PATCH 1/1 ocfs2/dlm: remove lockres from purge when a lock is added
Wengang, thanks for the patch. My comments are inline :)
Quoted 78 lines · Wengang Wang
dlm_thread(when purges a lockres)races with another thread that is running ondlmlock_remote().dlmlock_remote()can add a lock to the blocked list of the lockres without takingdlm->spinlock. This can lead aBUG()indlm_threadbecause of the added lock.Fix
We takedlm->spinlockwhen adding a lock to the blocked list and make sure the lockres is not in purge list. If we removed the lockres from purge list, try to add it back.Signed-off-by Wengang Wang <[email protected]>--- fs/ocfs2/dlm/dlmcommon.h | 15 +++++++++++++++ fs/ocfs2/dlm/dlmlock.c | 18 +++++++++++++++--- fs/ocfs2/dlm/dlmthread.c | 28 ++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 3 deletions(-)fs/ocfs2/dlm/dlmcommon.h +15
-0@@ -1002,6 +1002,9 @@ void dlm_move_lockres_to_recovery_list(struct dlm_ctxt *dlm,/* will exit holding res->spinlock, but may drop in function */ void __dlm_wait_on_lockres_flags (struct dlm_lock_resource *res,int flags); +void __dlm_wait_on_lockres_flags_lock_dlm (struct dlm_ctxt *dlm, +struct dlm_lock_resource *res, +int flags);void __dlm_wait_on_lockres_flags_set (struct dlm_lock_resource *res,int flags);/* will exit holding res->spinlock, but may drop in function */ @@ -1012,6 +1015,18 @@ static inline void __dlm_wait_on_lockres(struct dlm_lock_resource *res) DLM_LOCK_RES_MIGRATING)); } +/* +* will exit holding dlm->spinlock and res->spinlock, but may drop in +* function +*/ +static inline void __dlm_wait_on_lockres_lock_dlm (struct dlm_ctxt *dlm, +struct dlm_lock_resource *res) +{ +__dlm_wait_on_lockres_flags_lock_dlm (dlm, res, + (DLM_LOCK_RES_IN_PROGRESS | +DLM_LOCK_RES_RECOVERING | +DLM_LOCK_RES_MIGRATING )); +}void __dlm_unlink_mle (struct dlm_ctxt *dlm,struct dlm_master_list_entry *mle);void __dlm_insert_mle (struct dlm_ctxt *dlm,struct dlm_master_list_entry *mle);fs/ocfs2/dlm/dlmlock.c +4
-2@@ -222,23 +222,35 @@ static enum dlm_status dlmlock_remote(struct dlm_ctxt *dlm, struct dlm_lock *lock, int flags) {enum dlm_status status =DLM_DENIED ; -int lockres_changed =1 ; +int lockres_changed =1 , recalc;mlog_entry ("type=%d\n" , lock->ml.type);mlog (0 ,"lockres %.*s, flags = 0x%x\n" , res->lockname.len, res->lockname.name, flags); +spin_lock (&dlm->spinlock);spin_lock (&res->spinlock);/* will exit this call with spinlock held */ -__dlm_wait_on_lockres (res); +__dlm_wait_on_lockres_lock_dlm (dlm, res); res->state |=DLM_LOCK_RES_IN_PROGRESS ;/* add lock to local (secondary) queue */ dlm_lock_get (lock);list_add_tail (&lock->list, &res->blocked); lock->lock_pending =1 ; +/*
fs/ocfs2/dlm/dlmlock.c static enum dlm_status dlmlock_remote(struct dlm_ctxt *dlm, · line 252
Quoted 10 lines
fs/ocfs2/dlm/dlmlock.c +9
-0+* make sure this lockres is not in purge list if we remove the +* lockres from purge list, we try to add it back if locking failed. +* +* there is a race with dlm_thread purging this lockres. +* in this case, it can trigger a BUG() because we added a lock to the +* blocked list. +*/ + recalc = !list_empty (&res->purge); +__dlm_lockres_calc_usage (dlm, res);
Thanks for this patch, looks good. A few comments on another approach, let me know your thoughts.
The patch I sent for review marks the lockres to be in use (before dlmlock_remote is called) in dlm_get_lockresource. I have to do that to protect the lockres from unhashing once it's being used. But since lockres is still in purgelist the BUG you mentioned can still trigger in dlm_thread.
once a lockres is on purge list there are quite a few cases(case this patch is addressing, case the patch I just sent for review is addresing, and the case of the recovery) that can trigger the lockres to be reused, before or while it is getting purged. dlm_thread(dlm_run_purge_list) already expects that a lockres on purge list could just getting started to be reused. However it doesn't handle the above cases. I think it's better to enhance dlm_run_purge_list/dlm_purge_lockres code to handle these cases. I'am working on this change.
There also seems to be a case thats not handled. Assume dlm_thread sent deref message and waiting for response, now dlmlock_remote removed it from purge list. dlm_thread got response from master and continues and unhashes the lockres.
Quoted 54 lines
fs/ocfs2/dlm/dlmlock.c +1
-0spin_unlock (&res->spinlock); +spin_unlock (&dlm->spinlock);/* spec seems to say that you will get DLM_NORMAL when the lock fs/ocfs2/dlm/dlmlock.c +1
-1* has been queued, meaning we need to wait for a reply here. */ @@ -282,7 +294,7 @@ static enum dlm_status dlmlock_remote(struct dlm_ctxt *dlm, }spin_unlock (&res->spinlock); -if (lockres_changed) +if (recalc || lockres_changed)dlm_lockres_calc_usage (dlm, res);wake_up (&res->wq);fs/ocfs2/dlm/dlmthread.c +28
-0@@ -77,6 +77,34 @@ repeat:__set_current_state (TASK_RUNNING ); } +/* +* same as __dlm_wait_on_lockres_flags, but also hold dlm->spinlock on exit and +* may drop it in function +*/ +void __dlm_wait_on_lockres_flags_lock_dlm (struct dlm_ctxt *dlm, +struct dlm_lock_resource *res, +int flags) +{ +DECLARE_WAITQUEUE (wait, current); + +assert_spin_locked (&dlm->spinlock); +assert_spin_locked (&res->spinlock); + +add_wait_queue (&res->wq, &wait); +repeat: +set_current_state (TASK_UNINTERRUPTIBLE ); +if (res->state & flags) { +spin_unlock (&res->spinlock); +spin_unlock (&dlm->spinlock); +schedule (); +spin_lock (&dlm->spinlock); +spin_lock (&res->spinlock); + goto repeat; + } +remove_wait_queue (&res->wq, &wait); +__set_current_state (TASK_RUNNING ); +} +int __dlm_lockres_has_locks (struct dlm_lock_resource *res) { if (list_empty(&res->granted) &&
PATCH 1/1 ocfs2/dlm: remove lockres from purge when a lock is added
Srini, thanks for review!
Quoted 85 lines · Srinivas Eeda
Wengang, thanks for the patch. My comments are inline :)
Quoted 69 lines · Wengang Wang
dlm_thread(when purges a lockres)races with another thread that is running ondlmlock_remote().dlmlock_remote()can add a lock to the blocked list of the lockres without takingdlm->spinlock. This can lead aBUG()indlm_threadbecause of the added lock.Fix
We takedlm->spinlockwhen adding a lock to the blocked list and make sure the lockres is not in purge list. If we removed the lockres from purge list, try to add it back.Signed-off-by Wengang Wang <[email protected]>---
fs/ocfs2/dlm/dlmcommon.h | 15 +++++++++++++++ fs/ocfs2/dlm/dlmlock.c | 18 +++++++++++++++--- fs/ocfs2/dlm/dlmthread.c | 28 ++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 3 deletions(-)fs/ocfs2/dlm/dlmcommon.h +15
-0@@ -1002,6 +1002,9 @@ void dlm_move_lockres_to_recovery_list(struct dlm_ctxt *dlm,/* will exit holding res->spinlock, but may drop in function */ void __dlm_wait_on_lockres_flags (struct dlm_lock_resource *res,int flags); +void __dlm_wait_on_lockres_flags_lock_dlm (struct dlm_ctxt *dlm, +struct dlm_lock_resource *res, +int flags);void __dlm_wait_on_lockres_flags_set (struct dlm_lock_resource *res,int flags);/* will exit holding res->spinlock, but may drop in function */ @@ -1012,6 +1015,18 @@ static inline void __dlm_wait_on_lockres(struct dlm_lock_resource *res) DLM_LOCK_RES_MIGRATING)); } +/* +* will exit holding dlm->spinlock and res->spinlock, but may drop in +* function +*/ +static inline void __dlm_wait_on_lockres_lock_dlm (struct dlm_ctxt *dlm, +struct dlm_lock_resource *res) +{ +__dlm_wait_on_lockres_flags_lock_dlm (dlm, res, + (DLM_LOCK_RES_IN_PROGRESS | +DLM_LOCK_RES_RECOVERING | +DLM_LOCK_RES_MIGRATING )); +}void __dlm_unlink_mle (struct dlm_ctxt *dlm,struct dlm_master_list_entry *mle);void __dlm_insert_mle (struct dlm_ctxt *dlm,struct dlm_master_list_entry *mle);fs/ocfs2/dlm/dlmlock.c +3
-2@@ -222,23 +222,35 @@ static enum dlm_status dlmlock_remote(struct dlm_ctxt *dlm, struct dlm_lock *lock, int flags) {enum dlm_status status =DLM_DENIED ; -int lockres_changed =1 ; +int lockres_changed =1 , recalc;mlog_entry ("type=%d\n" , lock->ml.type);mlog (0 ,"lockres %.*s, flags = 0x%x\n" , res->lockname.len, res->lockname.name, flags); +spin_lock (&dlm->spinlock);spin_lock (&res->spinlock);/* will exit this call with spinlock held */ -__dlm_wait_on_lockres (res); +__dlm_wait_on_lockres_lock_dlm (dlm, res); res->state |=DLM_LOCK_RES_IN_PROGRESS ;/* add lock to local (secondary) queue */ dlm_lock_get (lock);list_add_tail (&lock->list, &res->blocked); lock->lock_pending =1 ;fs/ocfs2/dlm/dlmlock.c static enum dlm_status dlmlock_remote(struct dlm_ctxt *dlm, · line 247
Quoted 10 lines
fs/ocfs2/dlm/dlmlock.c +10
-0+/* +* make sure this lockres is not in purge list if we remove the +* lockres from purge list, we try to add it back if locking failed. +* +* there is a race with dlm_thread purging this lockres. +* in this case, it can trigger a BUG() because we added a lock to the +* blocked list. +*/ + recalc = !list_empty (&res->purge); +__dlm_lockres_calc_usage (dlm, res);Thanks for this patch, looks good. A few comments on another approach, let me know your thoughts.
Quoted 5 lines
The patch I sent for review marks the lockres to be in use (before
dlmlock_remoteis called) indlm_get_lockresource. I have to do that to protect the lockres from unhashing once it's being used. But since lockres is still in purgelist the BUG you mentioned can still trigger indlm_thread.
Yes.
Quoted 9 lines
once a lockres is on purge list there are quite a few cases(case this patch is addressing, case the patch I just sent for review is addresing, and the case of the recovery) that can trigger the lockres to be reused, before or while it is getting purged.
dlm_thread(dlm_run_purge_list)already expects that a lockres on purge list could just getting started to be reused. However it doesn't handle the above cases. I think it's better to enhance dlm_run_purge_list/dlm_purge_lockres code to handle these cases. I'am working on this change.
My thought may be incorrect, just for discuss.
I think we'd better keep the dlm_thread as simple as possible and take care in the problematic cases. That is remove the lockres from purge list it's "re-used" and put it back later if needed.
Putting all things to dlm_run_purge_list() will make it complex for understanding. Say, for the three cases you mentioned above, they involves AST/"remote lock"/recovery. If we simply remove the BUG()(retry to purge instead), we can't see such three problems are involved from the code any longer. And doing well in different threads is "clean" way :)
Quoted 4 lines
There also seems to be a case thats not handled. Assume
dlm_threadsent deref message and waiting for response, nowdlmlock_remoteremoved it from purge list.dlm_threadgot response from master and continues and unhashes the lockres.
Yes, that's a problem. Avoid unhashing the lockres is not hard. The problem is what will happen we dropped reference on the master node, but the lockres on this node is still "in use". The lockres on master node can disappear and we send request against this lockres. --that's bad.
Also, if the lockres is still in use, in your patch, you leave it in the purgelist. This can lead to secondary sendings of deref request to the master. Though this won't cause a BUG() on master or on this node, it's an incorrect behaviour.
So I think sending deref and unhashing the lockres should be in same "atomic" action?
regards,
wengang.
Quoted 49 lines
Quoted 49 lines
fs/ocfs2/dlm/dlmlock.c +2
-1spin_unlock (&res->spinlock); +spin_unlock (&dlm->spinlock);/* spec seems to say that you will get DLM_NORMAL when the lock * has been queued, meaning we need to wait for a reply here. */ @@ -282,7 +294,7 @@ static enum dlm_status dlmlock_remote(struct dlm_ctxt *dlm, }spin_unlock (&res->spinlock); -if (lockres_changed) +if (recalc || lockres_changed)dlm_lockres_calc_usage (dlm, res);wake_up (&res->wq);fs/ocfs2/dlm/dlmthread.c +28
-0@@ -77,6 +77,34 @@ repeat:__set_current_state (TASK_RUNNING ); } +/* +* same as __dlm_wait_on_lockres_flags, but also hold dlm->spinlock on exit and +* may drop it in function +*/ +void __dlm_wait_on_lockres_flags_lock_dlm (struct dlm_ctxt *dlm, +struct dlm_lock_resource *res, +int flags) +{ +DECLARE_WAITQUEUE (wait, current); + +assert_spin_locked (&dlm->spinlock); +assert_spin_locked (&res->spinlock); + +add_wait_queue (&res->wq, &wait); +repeat: +set_current_state (TASK_UNINTERRUPTIBLE ); +if (res->state & flags) { +spin_unlock (&res->spinlock); +spin_unlock (&dlm->spinlock); +schedule (); +spin_lock (&dlm->spinlock); +spin_lock (&res->spinlock); + goto repeat; + } +remove_wait_queue (&res->wq, &wait); +__set_current_state (TASK_RUNNING ); +} +int __dlm_lockres_has_locks (struct dlm_lock_resource *res) { if (list_empty(&res->granted) &&