ocfs2/dlm: fix race between purge and get lock resource
Subsystems (2)
PATCH 1/1 ocfs2/dlm: fix race between purge and get lock resource
There is a race between purge and get lock resource, which will lead to ast unfinished and system hung. The case is described below:
mkdir dlm_thread
-----------------------------------------------------------------------
o2cb_dlm_lock |
-> dlmlock |
-> dlm_get_lock_resource |
-> __dlm_lookup_lockres_full |
-> spin_unlock(&dlm->spinlock) |
| dlm_run_purge_list
| -> dlm_purge_lockres
| -> dlm_drop_lockres_ref
| -> spin_lock(&dlm->spinlock)
| -> spin_lock(&res->spinlock)
| -> ~DLM_LOCK_RES_DROPPING_REF
| -> spin_unlock(&res->spinlock)
| -> spin_unlock(&dlm->spinlock)
-> spin_lock(&tmpres->spinlock)|
DLM_LOCK_RES_DROPPING_REF cleared |
-> spin_unlock(&tmpres->spinlock) |
return the purged lockres |So after this, once ast comes, it will ingore the ast because the lockres cannot be found anymore. Thus the OCFS2_LOCK_BUSY won't be cleared and corresponding thread hangs.
The &dlm->spinlock was hold when checking DLM_LOCK_RES_DROPPING_REF at the very begining. And commit 7b791d6856 (ocfs2/dlm: Fix race during lockres mastery) moved it up because of the possible wait. So take the &dlm->spinlock and introduce a new wait function to fix the race.
--- fs/ocfs2/dlm/dlmcommon.h | 2 ++ fs/ocfs2/dlm/dlmmaster.c | 13 +++++++++---- fs/ocfs2/dlm/dlmthread.c | 23 +++++++++++++++++++++++ 3 files changed, 34 insertions(+), 4 deletions(-)
fs/ocfs2/dlm/dlmcommon.h +2 -0
@@ -1014,6 +1014,8 @@ 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_new (struct dlm_ctxt *dlm, +struct dlm_lock_resource *res,int flags);/* will exit holding res->spinlock, but may drop in function */ static inline void __dlm_wait_on_lockres (struct dlm_lock_resource *res)
fs/ocfs2/dlm/dlmmaster.c +9 -4
@@ -755,13 +755,16 @@ lookup:spin_lock (&dlm->spinlock); tmpres =__dlm_lookup_lockres_full (dlm, lockid, namelen, hash);if (tmpres) { -spin_unlock (&dlm->spinlock);spin_lock (&tmpres->spinlock);/* Wait on the thread that is mastering the resource */ if (tmpres->owner ==DLM_LOCK_RES_OWNER_UNKNOWN ) { -__dlm_wait_on_lockres (tmpres); +__dlm_wait_on_lockres_flags_new (dlm, tmpres, + (DLM_LOCK_RES_IN_PROGRESS | +DLM_LOCK_RES_RECOVERING | +DLM_LOCK_RES_MIGRATING ));BUG_ON (tmpres->owner ==DLM_LOCK_RES_OWNER_UNKNOWN );spin_unlock (&tmpres->spinlock); +spin_unlock (&dlm->spinlock);dlm_lockres_put (tmpres); tmpres =NULL ; goto lookup;
@@ -770,9 +773,10 @@ lookup:/* Wait on the resource purge to complete before continuing */ if (tmpres->state &DLM_LOCK_RES_DROPPING_REF ) {BUG_ON (tmpres->owner == dlm->node_num); -__dlm_wait_on_lockres_flags (tmpres, -DLM_LOCK_RES_DROPPING_REF ); +__dlm_wait_on_lockres_flags_new (dlm, tmpres, +DLM_LOCK_RES_DROPPING_REF );spin_unlock (&tmpres->spinlock); +spin_unlock (&dlm->spinlock);dlm_lockres_put (tmpres); tmpres =NULL ; goto lookup;
@@ -782,6 +786,7 @@ lookup:dlm_lockres_grab_inflight_ref (dlm, tmpres);spin_unlock (&tmpres->spinlock); +spin_unlock (&dlm->spinlock);if (res)dlm_lockres_put (res); res = tmpres;
fs/ocfs2/dlm/dlmthread.c +23 -0
@@ -77,6 +77,29 @@ repeat:__set_current_state (TASK_RUNNING ); } +void __dlm_wait_on_lockres_flags_new (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.8.4.3
Greg KH
maintainerPATCH 1/1 ocfs2/dlm: fix race between purge and get lock resource
Quoted 32 lines · Joseph Qi
There is a race between purge and get lock resource, which will lead to ast unfinished and system hung. The case is described below:
mkdir
dlm_thread
-----------------------------------------------------------------------o2cb_dlm_lock | -> dlmlock | -> dlm_get_lock_resource | -> __dlm_lookup_lockres_full | -> spin_unlock(&dlm->spinlock) | | dlm_run_purge_list | -> dlm_purge_lockres | -> dlm_drop_lockres_ref | -> spin_lock(&dlm->spinlock) | -> spin_lock(&res->spinlock) | -> ~DLM_LOCK_RES_DROPPING_REF | -> spin_unlock(&res->spinlock) | -> spin_unlock(&dlm->spinlock) -> spin_lock(&tmpres->spinlock)| DLM_LOCK_RES_DROPPING_REF cleared | -> spin_unlock(&tmpres->spinlock) | return the purged lockres |So after this, once ast comes, it will ingore the ast because the lockres cannot be found anymore. Thus the
OCFS2_LOCK_BUSYwon't be cleared and corresponding thread hangs.
The&dlm->spinlockwas hold when checkingDLM_LOCK_RES_DROPPING_REFat the very begining. And commit7b791d6856(ocfs2/dlm: Fix race during lockres mastery) moved it up because of the possible wait. So take the&dlm->spinlockand introduce a new wait function to fix the race.
Quoted 7 lines
Signed-off-by Joseph Qi <[email protected]>Reviewed-by joyce.xue <[email protected]>--- fs/ocfs2/dlm/dlmcommon.h | 2 ++ fs/ocfs2/dlm/dlmmaster.c | 13 +++++++++---- fs/ocfs2/dlm/dlmthread.c | 23 +++++++++++++++++++++++ 3 files changed, 34 insertions(+), 4 deletions(-)
<formletter>
This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read Documentation/stable_kernel_rules.txt for how to do this properly.
</formletter>
PATCH 1/1 ocfs2/dlm: fix race between purge and get lock resource
In reply to Joseph QiQuoted 37 lines · On Sat, 25 Apr 2015 15:05:15 +0800 Joseph Qi <[email protected]> wrote:
There is a race between purge and get lock resource, which will lead to ast unfinished and system hung. The case is described below:
mkdir
dlm_thread
-----------------------------------------------------------------------o2cb_dlm_lock | -> dlmlock | -> dlm_get_lock_resource | -> __dlm_lookup_lockres_full | -> spin_unlock(&dlm->spinlock) | | dlm_run_purge_list | -> dlm_purge_lockres | -> dlm_drop_lockres_ref | -> spin_lock(&dlm->spinlock) | -> spin_lock(&res->spinlock) | -> ~DLM_LOCK_RES_DROPPING_REF | -> spin_unlock(&res->spinlock) | -> spin_unlock(&dlm->spinlock) -> spin_lock(&tmpres->spinlock)| DLM_LOCK_RES_DROPPING_REF cleared | -> spin_unlock(&tmpres->spinlock) | return the purged lockres |So after this, once ast comes, it will ingore the ast because the lockres cannot be found anymore. Thus the
OCFS2_LOCK_BUSYwon't be cleared and corresponding thread hangs.
The&dlm->spinlockwas hold when checkingDLM_LOCK_RES_DROPPING_REFat the very begining. And commit7b791d6856(ocfs2/dlm: Fix race during lockres mastery) moved it up because of the possible wait. So take the&dlm->spinlockand introduce a new wait function to fix the race....
fs/ocfs2/dlm/dlmthread.c repeat: · line 86
Quoted 10 lines
fs/ocfs2/dlm/dlmthread.c +7
-0@@ -77,6 +77,29 @@ repeat:__set_current_state (TASK_RUNNING ); } +void __dlm_wait_on_lockres_flags_new (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);
Not strictly needed - lockdep will catch this. A minor thing.
Quoted 4 lines
fs/ocfs2/dlm/dlmthread.c +4
-0+add_wait_queue (&res->wq, &wait); +repeat: +set_current_state (TASK_UNINTERRUPTIBLE ); +if (res->state & flags) {
fs/ocfs2/dlm/dlmthread.c repeat: · line 101
Quoted 10 lines
fs/ocfs2/dlm/dlmthread.c +10
-0+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 ); +}
This is pretty nasty. Theoretically this could spin forever, if other tasks are setting the flag in a suitably synchronized fashion.
Is there no clean approach? A reorganization of the locking?
PATCH 1/1 ocfs2/dlm: fix race between purge and get lock resource
Hi Andrew,
Quoted 70 lines · Andrew Morton
Quoted 37 lines · On Sat, 25 Apr 2015 15:05:15 +0800 Joseph Qi <[email protected]> wrote:
There is a race between purge and get lock resource, which will lead to ast unfinished and system hung. The case is described below:
mkdir
dlm_thread
-----------------------------------------------------------------------o2cb_dlm_lock | -> dlmlock | -> dlm_get_lock_resource | -> __dlm_lookup_lockres_full | -> spin_unlock(&dlm->spinlock) | | dlm_run_purge_list | -> dlm_purge_lockres | -> dlm_drop_lockres_ref | -> spin_lock(&dlm->spinlock) | -> spin_lock(&res->spinlock) | -> ~DLM_LOCK_RES_DROPPING_REF | -> spin_unlock(&res->spinlock) | -> spin_unlock(&dlm->spinlock) -> spin_lock(&tmpres->spinlock)| DLM_LOCK_RES_DROPPING_REF cleared | -> spin_unlock(&tmpres->spinlock) | return the purged lockres |So after this, once ast comes, it will ingore the ast because the lockres cannot be found anymore. Thus the
OCFS2_LOCK_BUSYwon't be cleared and corresponding thread hangs.
The&dlm->spinlockwas hold when checkingDLM_LOCK_RES_DROPPING_REFat the very begining. And commit7b791d6856(ocfs2/dlm: Fix race during lockres mastery) moved it up because of the possible wait. So take the&dlm->spinlockand introduce a new wait function to fix the race....
fs/ocfs2/dlm/dlmthread.c repeat: · line 86
Quoted 10 lines
fs/ocfs2/dlm/dlmthread.c +7
-0@@ -77,6 +77,29 @@ repeat:__set_current_state (TASK_RUNNING ); } +void __dlm_wait_on_lockres_flags_new (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);Not strictly needed - lockdep will catch this. A minor thing.
Quoted 4 lines
fs/ocfs2/dlm/dlmthread.c +4
-0+add_wait_queue (&res->wq, &wait); +repeat: +set_current_state (TASK_UNINTERRUPTIBLE ); +if (res->state & flags) {fs/ocfs2/dlm/dlmthread.c repeat: · line 101
Quoted 10 lines
fs/ocfs2/dlm/dlmthread.c +10
-0+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 ); +}This is pretty nasty. Theoretically this could spin forever, if other tasks are setting the flag in a suitably synchronized fashion.
Quoted 2 lines
Is there no clean approach? A reorganization of the locking?
Do you mean the flag won't be cleared forever? If so, only taking &res->spinlock also has the same risk. But we haven't found this in our test/production environments so far.
To fix the race case above, I don't have another approach besides taking &dlm->spinlock.
Quoted 2 lines
.
PATCH 1/1 ocfs2/dlm: fix race between purge and get lock resource
Quoted 71 lines · Joseph Qi
Hi Andrew,
Quoted 68 lines · Andrew Morton
Quoted 37 lines · On Sat, 25 Apr 2015 15:05:15 +0800 Joseph Qi <[email protected]> wrote:
There is a race between purge and get lock resource, which will lead to ast unfinished and system hung. The case is described below:
mkdir
dlm_thread
-----------------------------------------------------------------------o2cb_dlm_lock|
-> dlmlock |-> dlm_get_lock_resource | -> __dlm_lookup_lockres_full | -> spin_unlock(&dlm->spinlock) | | dlm_run_purge_list | -> dlm_purge_lockres | -> dlm_drop_lockres_ref | -> spin_lock(&dlm->spinlock) | -> spin_lock(&res->spinlock) | -> ~DLM_LOCK_RES_DROPPING_REF | -> spin_unlock(&res->spinlock) | -> spin_unlock(&dlm->spinlock) -> spin_lock(&tmpres->spinlock)| DLM_LOCK_RES_DROPPING_REF cleared | -> spin_unlock(&tmpres->spinlock) | return the purged lockres |So after this, once ast comes, it will ingore the ast because the lockres cannot be found anymore. Thus the
OCFS2_LOCK_BUSYwon't be cleared and corresponding thread hangs.
The&dlm->spinlockwas hold when checkingDLM_LOCK_RES_DROPPING_REFat the very begining. And commit7b791d6856(ocfs2/dlm: Fix race during lockres mastery) moved it up because of the possible wait. So take the&dlm->spinlockand introduce a new wait function to fix the race....
fs/ocfs2/dlm/dlmthread.c repeat: · line 86
Quoted 10 lines
fs/ocfs2/dlm/dlmthread.c +7
-0@@ -77,6 +77,29 @@ repeat:__set_current_state (TASK_RUNNING ); } +void __dlm_wait_on_lockres_flags_new (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);Not strictly needed - lockdep will catch this. A minor thing.
Quoted 4 lines
fs/ocfs2/dlm/dlmthread.c +4
-0+add_wait_queue (&res->wq, &wait); +repeat: +set_current_state (TASK_UNINTERRUPTIBLE ); +if (res->state & flags) {fs/ocfs2/dlm/dlmthread.c repeat: · line 101
Quoted 10 lines
fs/ocfs2/dlm/dlmthread.c +10
-0+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 ); +}This is pretty nasty. Theoretically this could spin forever, if other tasks are setting the flag in a suitably synchronized fashion.
Quoted 7 lines
Quoted 2 lines
Is there no clean approach? A reorganization of the locking?
Do you mean the flag won't be cleared forever? If so, only taking
&res->spinlockalso has the same risk. But we haven't found this in our test/production environments so far.
To fix the race case above, I don't have another approach besides taking&dlm->spinlock.
Sorry, just found this patch was sent twice, i reply on the first one. This fix looks complicated. I posted another simpler fix on old thread.
Thanks,
Junxiao.
Quoted 8 lines
Quoted 2 lines
.
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
PATCH 1/1 ocfs2/dlm: fix race between purge and get lock resource
In reply to Joseph QiHi Joseph, thanks for finding and trying to fix this bug.
Quoted 23 lines · Joseph Qi
There is a race between purge and get lock resource, which will lead to ast unfinished and system hung. The case is described below:
mkdir
dlm_thread
-----------------------------------------------------------------------o2cb_dlm_lock | -> dlmlock | -> dlm_get_lock_resource | -> __dlm_lookup_lockres_full | -> spin_unlock(&dlm->spinlock) | | dlm_run_purge_list | -> dlm_purge_lockres | -> dlm_drop_lockres_ref | -> spin_lock(&dlm->spinlock) | -> spin_lock(&res->spinlock) | -> ~DLM_LOCK_RES_DROPPING_REF | -> spin_unlock(&res->spinlock) | -> spin_unlock(&dlm->spinlock) -> spin_lock(&tmpres->spinlock)| DLM_LOCK_RES_DROPPING_REF cleared | -> spin_unlock(&tmpres->spinlock) | return the purged lockres |
Quoted 8 lines
So after this, once ast comes, it will ingore the ast because the lockres cannot be found anymore. Thus the
OCFS2_LOCK_BUSYwon't be cleared and corresponding thread hangs.
The&dlm->spinlockwas hold when checkingDLM_LOCK_RES_DROPPING_REFat the very begining. And commit7b791d6856(ocfs2/dlm: Fix race during lockres mastery) moved it up because of the possible wait. So take the&dlm->spinlockand introduce a new wait function to fix the race.
Ok, I _think_ I understand the deadlock. But we can't say for sure who will come and wake up the sleeping process? If that's the case I don't think we want this for -stable right now.
Quoted 14 lines
Signed-off-by Joseph Qi <[email protected]>Reviewed-by joyce.xue <[email protected]>--- fs/ocfs2/dlm/dlmcommon.h | 2 ++ fs/ocfs2/dlm/dlmmaster.c | 13 +++++++++---- fs/ocfs2/dlm/dlmthread.c | 23 +++++++++++++++++++++++ 3 files changed, 34 insertions(+), 4 deletions(-)fs/ocfs2/dlm/dlmcommon.h +0
-0
fs/ocfs2/dlm/dlmcommon.h void dlm_move_lockres_to_recovery_list(struct dlm_ctxt *dlm, · line 1018
Quoted 5 lines
fs/ocfs2/dlm/dlmcommon.h +2
-0@@ -1014,6 +1014,8 @@ 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_new (struct dlm_ctxt *dlm, +struct dlm_lock_resource *res,int flags);
If we retain this function, it should have a more descriptive name than '_new'.
Quoted 66 lines
fs/ocfs2/dlm/dlmcommon.h +0
-0/* will exit holding res->spinlock, but may drop in function */ static inline void __dlm_wait_on_lockres (struct dlm_lock_resource *res)fs/ocfs2/dlm/dlmmaster.c +9
-4@@ -755,13 +755,16 @@ lookup:spin_lock (&dlm->spinlock); tmpres =__dlm_lookup_lockres_full (dlm, lockid, namelen, hash);if (tmpres) { -spin_unlock (&dlm->spinlock);spin_lock (&tmpres->spinlock);/* Wait on the thread that is mastering the resource */ if (tmpres->owner ==DLM_LOCK_RES_OWNER_UNKNOWN ) { -__dlm_wait_on_lockres (tmpres); +__dlm_wait_on_lockres_flags_new (dlm, tmpres, + (DLM_LOCK_RES_IN_PROGRESS | +DLM_LOCK_RES_RECOVERING | +DLM_LOCK_RES_MIGRATING ));BUG_ON (tmpres->owner ==DLM_LOCK_RES_OWNER_UNKNOWN );spin_unlock (&tmpres->spinlock); +spin_unlock (&dlm->spinlock);dlm_lockres_put (tmpres); tmpres =NULL ; goto lookup;@@ -770,9 +773,10 @@ lookup:/* Wait on the resource purge to complete before continuing */ if (tmpres->state &DLM_LOCK_RES_DROPPING_REF ) {BUG_ON (tmpres->owner == dlm->node_num); -__dlm_wait_on_lockres_flags (tmpres, -DLM_LOCK_RES_DROPPING_REF ); +__dlm_wait_on_lockres_flags_new (dlm, tmpres, +DLM_LOCK_RES_DROPPING_REF );spin_unlock (&tmpres->spinlock); +spin_unlock (&dlm->spinlock);dlm_lockres_put (tmpres); tmpres =NULL ; goto lookup;@@ -782,6 +786,7 @@ lookup:dlm_lockres_grab_inflight_ref (dlm, tmpres);spin_unlock (&tmpres->spinlock); +spin_unlock (&dlm->spinlock);if (res)dlm_lockres_put (res); res = tmpres;fs/ocfs2/dlm/dlmthread.c +12
-0@@ -77,6 +77,29 @@ repeat:__set_current_state (TASK_RUNNING ); } +void __dlm_wait_on_lockres_flags_new (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) {
fs/ocfs2/dlm/dlmthread.c repeat: · line 101
Quoted 10 lines
fs/ocfs2/dlm/dlmthread.c +10
-0+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 ); +}
Is it possible to rework this using wait_event()? The code you copied from __dlm_wait_on_lockres_flags() is seriously ugly :(
--Mark
--
Mark Fasheh
PATCH 1/1 ocfs2/dlm: fix race between purge and get lock resource
Hi Mark,
Quoted 35 lines · Mark Fasheh
Hi Joseph, thanks for finding and trying to fix this bug.
Quoted 23 lines · Joseph Qi
There is a race between purge and get lock resource, which will lead to ast unfinished and system hung. The case is described below:
mkdir
dlm_thread
-----------------------------------------------------------------------o2cb_dlm_lock | -> dlmlock | -> dlm_get_lock_resource | -> __dlm_lookup_lockres_full | -> spin_unlock(&dlm->spinlock) | | dlm_run_purge_list | -> dlm_purge_lockres | -> dlm_drop_lockres_ref | -> spin_lock(&dlm->spinlock) | -> spin_lock(&res->spinlock) | -> ~DLM_LOCK_RES_DROPPING_REF | -> spin_unlock(&res->spinlock) | -> spin_unlock(&dlm->spinlock) -> spin_lock(&tmpres->spinlock)| DLM_LOCK_RES_DROPPING_REF cleared | -> spin_unlock(&tmpres->spinlock) | return the purged lockres |Quoted 8 lines
So after this, once ast comes, it will ingore the ast because the lockres cannot be found anymore. Thus the
OCFS2_LOCK_BUSYwon't be cleared and corresponding thread hangs.
The&dlm->spinlockwas hold when checkingDLM_LOCK_RES_DROPPING_REFat the very begining. And commit7b791d6856(ocfs2/dlm: Fix race during lockres mastery) moved it up because of the possible wait. So take the&dlm->spinlockand introduce a new wait function to fix the race.
Quoted 4 lines
Ok, I _think_ I understand the deadlock. But we can't say for sure who will come and wake up the sleeping process? If that's the case I don't think we want this for -stable right now.
For DLM_LOCK_RES_DROPPING_REF, only dlm_thread will do it during purging lockres.
For others three (DLM_LOCK_RES_IN_PROGRESS|DLM_LOCK_RES_RECOVERING| DLM_LOCK_RES_MIGRATING), many flows are involved.
This bug has been found in our test environment when running race testcase. We can reproduce it in the following steps:
1) dreate a dir/file in N1, then N1 is the owner (take open lockres for example);
2) drop cache in N1 and then ls the dir/file in N2, so N2 is the owner now;
3) rm the dir/file in N1, the lockres is scheduled to be purged;
4) create the dir/file again in N1.
To 100% reproduce it, we can inject some delay right after lookup lockres and drop the &dlm->spinlock.
Quoted 104 lines
Quoted 15 lines
Signed-off-by Joseph Qi <[email protected]>Reviewed-by joyce.xue <[email protected]>--- fs/ocfs2/dlm/dlmcommon.h | 2 ++ fs/ocfs2/dlm/dlmmaster.c | 13 +++++++++---- fs/ocfs2/dlm/dlmthread.c | 23 +++++++++++++++++++++++ 3 files changed, 34 insertions(+), 4 deletions(-)fs/ocfs2/dlm/dlmcommon.h +0
-0fs/ocfs2/dlm/dlmcommon.h void dlm_move_lockres_to_recovery_list(struct dlm_ctxt *dlm, · line 1018
Quoted 5 lines
fs/ocfs2/dlm/dlmcommon.h +2
-0@@ -1014,6 +1014,8 @@ 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_new (struct dlm_ctxt *dlm, +struct dlm_lock_resource *res,int flags);If we retain this function, it should have a more descriptive name than '
_new'.Quoted 66 lines
fs/ocfs2/dlm/dlmcommon.h +0
-0/* will exit holding res->spinlock, but may drop in function */ static inline void __dlm_wait_on_lockres (struct dlm_lock_resource *res)fs/ocfs2/dlm/dlmmaster.c +9
-4@@ -755,13 +755,16 @@ lookup:spin_lock (&dlm->spinlock); tmpres =__dlm_lookup_lockres_full (dlm, lockid, namelen, hash);if (tmpres) { -spin_unlock (&dlm->spinlock);spin_lock (&tmpres->spinlock);/* Wait on the thread that is mastering the resource */ if (tmpres->owner ==DLM_LOCK_RES_OWNER_UNKNOWN ) { -__dlm_wait_on_lockres (tmpres); +__dlm_wait_on_lockres_flags_new (dlm, tmpres, + (DLM_LOCK_RES_IN_PROGRESS | +DLM_LOCK_RES_RECOVERING | +DLM_LOCK_RES_MIGRATING ));BUG_ON (tmpres->owner ==DLM_LOCK_RES_OWNER_UNKNOWN );spin_unlock (&tmpres->spinlock); +spin_unlock (&dlm->spinlock);dlm_lockres_put (tmpres); tmpres =NULL ; goto lookup;@@ -770,9 +773,10 @@ lookup:/* Wait on the resource purge to complete before continuing */ if (tmpres->state &DLM_LOCK_RES_DROPPING_REF ) {BUG_ON (tmpres->owner == dlm->node_num); -__dlm_wait_on_lockres_flags (tmpres, -DLM_LOCK_RES_DROPPING_REF ); +__dlm_wait_on_lockres_flags_new (dlm, tmpres, +DLM_LOCK_RES_DROPPING_REF );spin_unlock (&tmpres->spinlock); +spin_unlock (&dlm->spinlock);dlm_lockres_put (tmpres); tmpres =NULL ; goto lookup;@@ -782,6 +786,7 @@ lookup:dlm_lockres_grab_inflight_ref (dlm, tmpres);spin_unlock (&tmpres->spinlock); +spin_unlock (&dlm->spinlock);if (res)dlm_lockres_put (res); res = tmpres;fs/ocfs2/dlm/dlmthread.c +12
-0@@ -77,6 +77,29 @@ repeat:__set_current_state (TASK_RUNNING ); } +void __dlm_wait_on_lockres_flags_new (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) {fs/ocfs2/dlm/dlmthread.c repeat: · line 101
Quoted 10 lines
fs/ocfs2/dlm/dlmthread.c +10
-0+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 ); +}Is it possible to rework this using
wait_event()? The code you copied from__dlm_wait_on_lockres_flags()is seriously ugly :(
Quoted 2 lines
--Mark
Andrew has the same comments:)
Since __dlm_wait_on_lockres_flags is used in many other places, so I just copied the logic and add mine to limit the impact.
Reworking and then testing it may take much time:)
Quoted 5 lines
--
Mark Fasheh.