remove lockres from purge list when we are getting it for creating lock
PATCH 1/1 remove lockres from purge list when we are getting it for creating lock
When we are to purge a lockres, we check if it's unused.
the check includes
1. no locks attached to the lockres.
2. not dirty.
3. not in recovery.
4. no interested nodes in refmap.
If the the above four are satisfied, we are going to purge it(remove it from hash table).
While, when a "create lock" is in progress especially when lockres is owned remotely(spinlocks are dropped when networking), the lockres can satisfy above four condition. If it's put to purge list, it can be purged out from hash table when we are still accessing on it(sending request to owner for example). That's not what we want.
I have met such a problem (orabug 12405575).
The lockres is in the purge list already(there is a delay for real purge work) and the create lock request comes. When we are sending network message to the owner in dlmlock_remote(), the owner crashed. So we get DLM_RECOVERING as return value and retries dlmlock_remote(). And before the owner crash, we have purged the lockres. So the lockres become stale(on lockres->onwer). Thus the code calls dlmlock_remote() infinitely.
fix:
we remove the lockres from purge list if it's there in dlm_get_lock_resource() which is called for only createlock case. So that the lockres can't be purged when we are in progress of createlock.
--- fs/ocfs2/dlm/dlmmaster.c | 41 +++++++++++++++++++++++++++++------------ 1 files changed, 29 insertions(+), 12 deletions(-)
fs/ocfs2/dlm/dlmmaster.c +29 -12
@@ -709,28 +709,27 @@ lookup:spin_lock (&dlm->spinlock); tmpres =__dlm_lookup_lockres_full (dlm, lockid, namelen, hash);if (tmpres) { -int dropping_ref =0 ; - -spin_unlock (&dlm->spinlock); -spin_lock (&tmpres->spinlock);/* We wait for the other thread that is mastering the resource */ if (tmpres->owner ==DLM_LOCK_RES_OWNER_UNKNOWN ) { +spin_unlock (&dlm->spinlock);__dlm_wait_on_lockres (tmpres);BUG_ON (tmpres->owner ==DLM_LOCK_RES_OWNER_UNKNOWN ); +spin_unlock (&tmpres->spinlock); +dlm_lockres_put (tmpres); + tmpres =NULL ; + goto lookup; }if (tmpres->owner == dlm->node_num) {BUG_ON (tmpres->state &DLM_LOCK_RES_DROPPING_REF );dlm_lockres_grab_inflight_ref (dlm, tmpres); - }else if (tmpres->state &DLM_LOCK_RES_DROPPING_REF ) - dropping_ref =1 ; -spin_unlock (&tmpres->spinlock); - -/* wait until done messaging the master, drop our ref to allow -* the lockres to be purged, start over. */ -if (dropping_ref) { -spin_lock (&tmpres->spinlock); + }else if (tmpres->state &DLM_LOCK_RES_DROPPING_REF ) { +/* +* wait until done messaging the master, drop our ref to +* allow the lockres to be purged, start over. +*/ +spin_unlock (&dlm->spinlock);__dlm_wait_on_lockres_flags (tmpres,DLM_LOCK_RES_DROPPING_REF );spin_unlock (&tmpres->spinlock);dlm_lockres_put (tmpres);
@@ -739,6 +738,24 @@ lookup: }mlog (0 ,"found in hash!\n" ); +/* +* we are going to do a create-lock next. so remove the lockres +* from purge list to avoid the case that we will access on the +* lockres which is already purged out from hash table in +* dlm_run_purge_list() path. +* otherwise, we could run into a problem: +* the owner dead(recovery didn't take care of this lockres +* since it's not in hashtable), and the code keeps sending +* request to the dead node and getting DLM_RECOVERING and +* then retrying infinitely. +*/ +if (!list_empty (&tmpres->purge)) { +list_del_init (&tmpres->purge); +dlm_lockres_put (tmpres); + } + +spin_unlock (&tmpres->spinlock); +spin_unlock (&dlm->spinlock);if (res)dlm_lockres_put (res); res = tmpres;
Signature · 1 line
1.7.2.3
PATCH 1/1 remove lockres from purge list when we are getting it for creating lock
I think I have seen this problem in ocfs2-1.2 and it was addressed by using a new state DLM_LOCK_RES_IN_USE. But we didn't merge into mainline as sunil suggested we need to look for a different approach
http://oss.oracle.com/pipermail/ocfs2-devel/2010-June/006669.html
http://www.mail-archive.com/ocfs2-devel at oss.oracle.com/msg05733.html
one comment inline
Quoted 89 lines · Wengang Wang
When we are to purge a lockres, we check if it's unused.
the check includes
1. no locks attached to the lockres.
2. not dirty.
3. not in recovery.
4. no interested nodes in refmap.
If the the above four are satisfied, we are going to purge it(remove it from hash table).While, when a "create lock" is in progress especially when lockres is owned remotely(spinlocks are dropped when networking), the lockres can satisfy above four condition. If it's put to purge list, it can be purged out from hash table when we are still accessing on it(sending request to owner for example). That's not what we want.
I have met such a problem (orabug 12405575).
The lockres is in the purge list already(there is a delay for real purge work) and the create lock request comes. When we are sending network message to the owner indlmlock_remote(), the owner crashed. So we getDLM_RECOVERINGas return value and retriesdlmlock_remote(). And before the owner crash, we have purged the lockres. So the lockres becomestale(on lockres->onwer). Thus the code callsdlmlock_remote()infinitely.fix:
we remove the lockres from purge list if it's there indlm_get_lock_resource()which is called for only createlock case. So that the lockres can't be purged when we are in progress of createlock.Signed-off-by Wengang Wang<[email protected]>--- fs/ocfs2/dlm/dlmmaster.c | 41 +++++++++++++++++++++++++++++------------ 1 files changed, 29 insertions(+), 12 deletions(-)fs/ocfs2/dlm/dlmmaster.c +19
-12@@ -709,28 +709,27 @@ lookup:spin_lock (&dlm->spinlock); tmpres =__dlm_lookup_lockres_full (dlm, lockid, namelen, hash);if (tmpres) { -int dropping_ref =0 ; - -spin_unlock (&dlm->spinlock); -spin_lock (&tmpres->spinlock);/* We wait for the other thread that is mastering the resource */ if (tmpres->owner ==DLM_LOCK_RES_OWNER_UNKNOWN ) { +spin_unlock (&dlm->spinlock);__dlm_wait_on_lockres (tmpres);BUG_ON (tmpres->owner ==DLM_LOCK_RES_OWNER_UNKNOWN ); +spin_unlock (&tmpres->spinlock); +dlm_lockres_put (tmpres); + tmpres =NULL ; + goto lookup; }if (tmpres->owner == dlm->node_num) {BUG_ON (tmpres->state&DLM_LOCK_RES_DROPPING_REF );dlm_lockres_grab_inflight_ref (dlm, tmpres); - }else if (tmpres->state&DLM_LOCK_RES_DROPPING_REF ) - dropping_ref =1 ; -spin_unlock (&tmpres->spinlock); - -/* wait until done messaging the master, drop our ref to allow -* the lockres to be purged, start over. */ -if (dropping_ref) { -spin_lock (&tmpres->spinlock); + }else if (tmpres->state&DLM_LOCK_RES_DROPPING_REF ) { +/* +* wait until done messaging the master, drop our ref to +* allow the lockres to be purged, start over. +*/ +spin_unlock (&dlm->spinlock);__dlm_wait_on_lockres_flags (tmpres,DLM_LOCK_RES_DROPPING_REF );spin_unlock (&tmpres->spinlock);dlm_lockres_put (tmpres);@@ -739,6 +738,24 @@ lookup: }mlog (0 ,"found in hash!\n" ); +/* +* we are going to do a create-lock next. so remove the lockres +* from purge list to avoid the case that we will access on the +* lockres which is already purged out from hash table in +* dlm_run_purge_list() path. +* otherwise, we could run into a problem: +* the owner dead(recovery didn't take care of this lockres +* since it's not in hashtable), and the code keeps sending
fs/ocfs2/dlm/dlmmaster.c lookup: · line 758
Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +10
-0+* request to the dead node and getting DLM_RECOVERING and +* then retrying infinitely. +*/ +if (!list_empty (&tmpres->purge)) { +list_del_init (&tmpres->purge); +dlm_lockres_put (tmpres); + } + +spin_unlock (&tmpres->spinlock); +spin_unlock (&dlm->spinlock);
lockres could still get added to purgelist at this point and we could still have the same problem? I think, here we need some mechanism that marks the lockres is in use that would protect it from adding to the purgelist?
Quoted 3 lines
fs/ocfs2/dlm/dlmmaster.c +0
-0if (res)dlm_lockres_put (res); res = tmpres;
PATCH 1/1 remove lockres from purge list when we are getting it for creating lock
Srini,
Quoted 6 lines · Srinivas Eeda
I think I have seen this problem in ocfs2-1.2 and it was addressed by using a new state
DLM_LOCK_RES_IN_USE. But we didn't merge into mainline as sunil suggested we need to look for a different approachhttp://oss.oracle.com/pipermail/ocfs2-devel/2010-June/006669.html
http://www.mail-archive.com/ocfs2-devel at oss.oracle.com/msg05733.html
Yes, this is the same problem.
Quoted 2 lines
one comment inline
<..snip>
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ spin_unlock(&tmpres->spinlock); + spin_unlock(&dlm->spinlock);lockres could still get added to purgelist at this point and we could still have the same problem? I think, here we need some mechanism that marks the lockres is in use that would protect it from adding to the purgelist?
Seems there shouldn't be the possibility that the lockres was moved to purge list again. We are in the case of a "create lock", there should not be any outstanding convert/unlock request in progress concurrently. If there is one, the there must be lock(s) attached to the lockres. But if there are locks attached, the lockres can't be purged. Also for concurrent "create lock", there is no problem either because there must be a lock attached to the lockres.
I think your patch works too. But changingDLM_LOCK_RES_IN_USE
toDLM_LOCK_RES_CREATING_LOCK
can help understand better.
I also thought about inflight_locks, but so far I failed to find correct places to drop it.
thanks,
wengang.
Quoted 3 lines
Quoted 3 lines
fs/ocfs2/dlm/dlmmaster.c +0
-0if (res)dlm_lockres_put (res); res = tmpres;
PATCH 1/1 remove lockres from purge list when we are getting it for creating lock
Quoted 9 lines
Quoted 7 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ spin_unlock(&tmpres->spinlock); + spin_unlock(&dlm->spinlock);lockres could still get added to purgelist at this point and we could still have the same problem? I think, here we need some mechanism that marks the lockres is in use that would protect it from adding to the purgelist?
Seems there shouldn't be the possibility that the lockres was moved to purge list again.
I think you are right about that once removed from the purge list it will not be put back again on the purgelist in this case :)
I still think that there is a hole here. Is it possible that the spinlocks are released here and right then dlm_thread was walking through dirty list and then it could put this on purge list for the first time?
If it's already on purgelist, then we seem to be safe by removing it. But what is preventing the lockres getting onto to purge list right at this point. locks are added to lockres a little later but till then it is not safe?(I think).
Quoted 11 lines
We are in the case of a "create lock", there should not be any
outstanding convert/unlock request in progress concurrently. If there is one, the there must be lock(s) attached to the lockres. But if there are locks attached, the lockres can't be purged. Also for concurrent "create lock", there is no problem either because there must be a lock attached to the lockres.I think your patch works too. But changing
DLM_LOCK_RES_IN_USE
toDLM_LOCK_RES_CREATING_LOCK
can help understand better.
I am ok with the name :). The thing we should do here is once we found a lockres and moving ahead with create lock request, we should be sure it's protected before releasing the spinlocks.
Quoted 8 lines
I also thought about
inflight_locks, but so far I failed to find correct places to drop it.thanks,
wengang.Quoted 3 lines
Quoted 3 lines
if (res) dlm_lockres_put(res); res = tmpres;
PATCH 1/1 remove lockres from purge list when we are getting it for creating lock
Srini,
Yes, you are right! Agree!
Sunil,
My privious patch which checks if lockres is unhashed can't fix the problem that "can't find lockres in ast handler". So I vote for Srini's patch.
what's your comment?
thanks,
wengang.
Quoted 44 lines · Srinivas Eeda
Quoted 9 lines
Quoted 7 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ spin_unlock(&tmpres->spinlock); + spin_unlock(&dlm->spinlock);lockres could still get added to purgelist at this point and we could still have the same problem? I think, here we need some mechanism that marks the lockres is in use that would protect it from adding to the purgelist?
Seems there shouldn't be the possibility that the lockres was moved to purge list again.
I think you are right about that once removed from the purge list it will not be put back again on the purgelist in this case :)
I still think that there is a hole here. Is it possible that the spinlocks are released here and right then
dlm_threadwas walking through dirty list and then it could put this on purge list for the first time?If it's already on purgelist, then we seem to be safe by removing it. But what is preventing the lockres getting onto to purge list right at this point. locks are added to lockres a little later but till then it is not safe?(I think).
Quoted 11 lines
We are in the case of a "create lock", there should not be any
outstanding convert/unlock request in progress concurrently. If there is one, the there must be lock(s) attached to the lockres. But if there are locks attached, the lockres can't be purged. Also for concurrent "create lock", there is no problem either because there must be a lock attached to the lockres.I think your patch works too. But changing
DLM_LOCK_RES_IN_USE
toDLM_LOCK_RES_CREATING_LOCK
can help understand better.I am ok with the name :). The thing we should do here is once we found a lockres and moving ahead with create lock request, we should be sure it's protected before releasing the spinlocks.
Quoted 8 lines
I also thought about
inflight_locks, but so far I failed to find correct places to drop it.thanks,
wengang.Quoted 3 lines
Quoted 3 lines
fs/ocfs2/dlm/dlmmaster.c +0
-0if (res)dlm_lockres_put (res); res = tmpres;
PATCH 1/1 remove lockres from purge list when we are getting it for creating lock
In reply to Srinivas EedaDifferent issue. That patch plugged a hole when the master was remote. In this case the master is local.
In this case, the window is very tiny as we not only need the recovery to kick-in in the that window, we also need the lockres to be moved to the purgelist in that window.
But it just so happens that ovm via userdlm hits createlock a lot making this issue more likely to trigger than with the fs.
Quoted 120 lines · Srinivas Eeda
I think I have seen this problem in ocfs2-1.2 and it was addressed by using a new state
DLM_LOCK_RES_IN_USE. But we didn't merge into mainline as sunil suggested we need to look for a different approachhttp://oss.oracle.com/pipermail/ocfs2-devel/2010-June/006669.html
http://www.mail-archive.com/ocfs2-devel at oss.oracle.com/msg05733.htmlone comment inline
Quoted 89 lines · Wengang Wang
When we are to purge a lockres, we check if it's unused.
the check includes
1. no locks attached to the lockres.
2. not dirty.
3. not in recovery.
4. no interested nodes in refmap.
If the the above four are satisfied, we are going to purge it(remove it from hash table).While, when a "create lock" is in progress especially when lockres is owned remotely(spinlocks are dropped when networking), the lockres can satisfy above four condition. If it's put to purge list, it can be purged out from hash table when we are still accessing on it(sending request to owner for example). That's not what we want.
I have met such a problem (orabug 12405575).
The lockres is in the purge list already(there is a delay for real purge work) and the create lock request comes. When we are sending network message to the owner indlmlock_remote(), the owner crashed. So we getDLM_RECOVERINGas return value and retriesdlmlock_remote(). And before the owner crash, we have purged the lockres. So the lockres becomestale(on lockres->onwer). Thus the code callsdlmlock_remote()infinitely.fix:
we remove the lockres from purge list if it's there indlm_get_lock_resource()which is called for only createlock case. So that the lockres can't be purged when we are in progress of createlock.Signed-off-by Wengang Wang<[email protected]>--- fs/ocfs2/dlm/dlmmaster.c | 41 +++++++++++++++++++++++++++++------------ 1 files changed, 29 insertions(+), 12 deletions(-)fs/ocfs2/dlm/dlmmaster.c +19
-12@@ -709,28 +709,27 @@ lookup:spin_lock (&dlm->spinlock); tmpres =__dlm_lookup_lockres_full (dlm, lockid, namelen, hash);if (tmpres) { -int dropping_ref =0 ; - -spin_unlock (&dlm->spinlock); -spin_lock (&tmpres->spinlock);/* We wait for the other thread that is mastering the resource */ if (tmpres->owner ==DLM_LOCK_RES_OWNER_UNKNOWN ) { +spin_unlock (&dlm->spinlock);__dlm_wait_on_lockres (tmpres);BUG_ON (tmpres->owner ==DLM_LOCK_RES_OWNER_UNKNOWN ); +spin_unlock (&tmpres->spinlock); +dlm_lockres_put (tmpres); + tmpres =NULL ; + goto lookup; }if (tmpres->owner == dlm->node_num) {BUG_ON (tmpres->state&DLM_LOCK_RES_DROPPING_REF );dlm_lockres_grab_inflight_ref (dlm, tmpres); - }else if (tmpres->state&DLM_LOCK_RES_DROPPING_REF ) - dropping_ref =1 ; -spin_unlock (&tmpres->spinlock); - -/* wait until done messaging the master, drop our ref to allow -* the lockres to be purged, start over. */ -if (dropping_ref) { -spin_lock (&tmpres->spinlock); + }else if (tmpres->state&DLM_LOCK_RES_DROPPING_REF ) { +/* +* wait until done messaging the master, drop our ref to +* allow the lockres to be purged, start over. +*/ +spin_unlock (&dlm->spinlock);__dlm_wait_on_lockres_flags (tmpres,DLM_LOCK_RES_DROPPING_REF );spin_unlock (&tmpres->spinlock);dlm_lockres_put (tmpres);@@ -739,6 +738,24 @@ lookup: }mlog (0 ,"found in hash!\n" ); +/* +* we are going to do a create-lock next. so remove the lockres +* from purge list to avoid the case that we will access on the +* lockres which is already purged out from hash table in +* dlm_run_purge_list() path. +* otherwise, we could run into a problem: +* the owner dead(recovery didn't take care of this lockres +* since it's not in hashtable), and the code keeps sending fs/ocfs2/dlm/dlmmaster.c lookup: · line 758
Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +10
-0+* request to the dead node and getting DLM_RECOVERING and +* then retrying infinitely. +*/ +if (!list_empty (&tmpres->purge)) { +list_del_init (&tmpres->purge); +dlm_lockres_put (tmpres); + } + +spin_unlock (&tmpres->spinlock); +spin_unlock (&dlm->spinlock);lockres could still get added to purgelist at this point and we could still have the same problem? I think, here we need some mechanism that marks the lockres is in use that would protect it from adding to the purgelist?
fs/ocfs2/dlm/dlmmaster.c lookup: · line 761
Quoted 3 lines
fs/ocfs2/dlm/dlmmaster.c +0
-0if (res)dlm_lockres_put (res); res = tmpres;_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
http://oss.oracle.com/mailman/listinfo/ocfs2-devel
PATCH 1/1 remove lockres from purge list when we are getting it for creating lock
Quoted 2 lines · Sunil Mushran
Different issue. That patch plugged a hole when the master was remote. In this case the master is local.
I think in my case, the master is remote too. The phenomemon is local node keeps sending messages infinitely to the master which is dead.
Quoted 7 lines
In this case, the window is very tiny as we not only need the recovery to kick-in in the that window, we also need the lockres to be moved to the purgelist in that window.
But it just so happens that ovm via userdlm hits createlock a lot making this issue more likely to trigger than with the fs.
Yes, OVM create the lock in every 5 seconds via dlmfs.
thanks,
wengang.
Quoted 122 lines · Srinivas Eeda
Quoted 120 lines
I think I have seen this problem in ocfs2-1.2 and it was addressed by using a new state
DLM_LOCK_RES_IN_USE. But we didn't merge into mainline as sunil suggested we need to look for a different approachhttp://oss.oracle.com/pipermail/ocfs2-devel/2010-June/006669.html
http://www.mail-archive.com/ocfs2-devel at oss.oracle.com/msg05733.htmlone comment inline
Quoted 89 lines · Wengang Wang
When we are to purge a lockres, we check if it's unused.
the check includes
1. no locks attached to the lockres.
2. not dirty.
3. not in recovery.
4. no interested nodes in refmap.
If the the above four are satisfied, we are going to purge it(remove it from hash table).While, when a "create lock" is in progress especially when lockres is owned remotely(spinlocks are dropped when networking), the lockres can satisfy above four condition. If it's put to purge list, it can be purged out from hash table when we are still accessing on it(sending request to owner for example). That's not what we want.
I have met such a problem (orabug 12405575).
The lockres is in the purge list already(there is a delay for real purge work) and the create lock request comes. When we are sending network message to the owner indlmlock_remote(), the owner crashed. So we getDLM_RECOVERINGas return value and retriesdlmlock_remote(). And before the owner crash, we have purged the lockres. So the lockres becomestale(on lockres->onwer). Thus the code callsdlmlock_remote()infinitely.fix:
we remove the lockres from purge list if it's there indlm_get_lock_resource()which is called for only createlock case. So that the lockres can't be purged when we are in progress of createlock.Signed-off-by Wengang Wang<[email protected]>--- fs/ocfs2/dlm/dlmmaster.c | 41 +++++++++++++++++++++++++++++------------ 1 files changed, 29 insertions(+), 12 deletions(-)fs/ocfs2/dlm/dlmmaster.c +19
-12@@ -709,28 +709,27 @@ lookup:spin_lock (&dlm->spinlock); tmpres =__dlm_lookup_lockres_full (dlm, lockid, namelen, hash);if (tmpres) { -int dropping_ref =0 ; - -spin_unlock (&dlm->spinlock); -spin_lock (&tmpres->spinlock);/* We wait for the other thread that is mastering the resource */ if (tmpres->owner ==DLM_LOCK_RES_OWNER_UNKNOWN ) { +spin_unlock (&dlm->spinlock);__dlm_wait_on_lockres (tmpres);BUG_ON (tmpres->owner ==DLM_LOCK_RES_OWNER_UNKNOWN ); +spin_unlock (&tmpres->spinlock); +dlm_lockres_put (tmpres); + tmpres =NULL ; + goto lookup; }if (tmpres->owner == dlm->node_num) {BUG_ON (tmpres->state&DLM_LOCK_RES_DROPPING_REF );dlm_lockres_grab_inflight_ref (dlm, tmpres); - }else if (tmpres->state&DLM_LOCK_RES_DROPPING_REF ) - dropping_ref =1 ; -spin_unlock (&tmpres->spinlock); - -/* wait until done messaging the master, drop our ref to allow -* the lockres to be purged, start over. */ -if (dropping_ref) { -spin_lock (&tmpres->spinlock); + }else if (tmpres->state&DLM_LOCK_RES_DROPPING_REF ) { +/* +* wait until done messaging the master, drop our ref to +* allow the lockres to be purged, start over. +*/ +spin_unlock (&dlm->spinlock);__dlm_wait_on_lockres_flags (tmpres,DLM_LOCK_RES_DROPPING_REF );spin_unlock (&tmpres->spinlock);dlm_lockres_put (tmpres);@@ -739,6 +738,24 @@ lookup: }mlog (0 ,"found in hash!\n" ); +/* +* we are going to do a create-lock next. so remove the lockres +* from purge list to avoid the case that we will access on the +* lockres which is already purged out from hash table in +* dlm_run_purge_list() path. +* otherwise, we could run into a problem: +* the owner dead(recovery didn't take care of this lockres +* since it's not in hashtable), and the code keeps sending fs/ocfs2/dlm/dlmmaster.c lookup: · line 758
Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +10
-0+* request to the dead node and getting DLM_RECOVERING and +* then retrying infinitely. +*/ +if (!list_empty (&tmpres->purge)) { +list_del_init (&tmpres->purge); +dlm_lockres_put (tmpres); + } + +spin_unlock (&tmpres->spinlock); +spin_unlock (&dlm->spinlock);lockres could still get added to purgelist at this point and we could still have the same problem? I think, here we need some mechanism that marks the lockres is in use that would protect it from adding to the purgelist?
fs/ocfs2/dlm/dlmmaster.c lookup: · line 761
Quoted 3 lines
fs/ocfs2/dlm/dlmmaster.c +0
-0if (res)dlm_lockres_put (res); res = tmpres;_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
http://oss.oracle.com/mailman/listinfo/ocfs2-devel
PATCH 1/1 remove lockres from purge list when we are getting it for creating lock
Quoted 5 lines · Wengang Wang
Quoted 2 lines · Sunil Mushran
Different issue. That patch plugged a hole when the master was remote. In this case the master is local.
I think in my case, the master is remote too. The phenomemon is local node keeps sending messages infinitely to the master which is dead.
Quoted 7 lines
Quoted 6 lines
In this case, the window is very tiny as we not only need the recovery to kick-in in the that window, we also need the lockres to be moved to the purgelist in that window.
But it just so happens that ovm via userdlm hits createlock a lot making this issue more likely to trigger than with the fs.
Yes, OVM create the lock in every 5 seconds via dlmfs.
Do you have a reproducible testcase? If not, work on that. That would be most helpful in fixing this issue.
Right now I am also working on a problem in this area. Not the same though. I can work on this issue too. But I need a testcase.
PATCH 1/1 remove lockres from purge list when we are getting it for creating lock
In reply to Wengang WangQuoted 92 lines · Wengang Wang
When we are to purge a lockres, we check if it's unused.
the check includes
1. no locks attached to the lockres.
2. not dirty.
3. not in recovery.
4. no interested nodes in refmap.
If the the above four are satisfied, we are going to purge it(remove it from hash table).While, when a "create lock" is in progress especially when lockres is owned remotely(spinlocks are dropped when networking), the lockres can satisfy above four condition. If it's put to purge list, it can be purged out from hash table when we are still accessing on it(sending request to owner for example). That's not what we want.
I have met such a problem (orabug 12405575).
The lockres is in the purge list already(there is a delay for real purge work) and the create lock request comes. When we are sending network message to the owner indlmlock_remote(), the owner crashed. So we getDLM_RECOVERINGas return value and retriesdlmlock_remote(). And before the owner crash, we have purged the lockres. So the lockres becomestale(on lockres->onwer). Thus the code callsdlmlock_remote()infinitely.fix:
we remove the lockres from purge list if it's there indlm_get_lock_resource()which is called for only createlock case. So that the lockres can't be purged when we are in progress of createlock.Signed-off-by Wengang Wang<[email protected]>--- fs/ocfs2/dlm/dlmmaster.c | 41 +++++++++++++++++++++++++++++------------ 1 files changed, 29 insertions(+), 12 deletions(-)fs/ocfs2/dlm/dlmmaster.c +22
-12@@ -709,28 +709,27 @@ lookup:spin_lock (&dlm->spinlock); tmpres =__dlm_lookup_lockres_full (dlm, lockid, namelen, hash);if (tmpres) { -int dropping_ref =0 ; - -spin_unlock (&dlm->spinlock); -spin_lock (&tmpres->spinlock);/* We wait for the other thread that is mastering the resource */ if (tmpres->owner ==DLM_LOCK_RES_OWNER_UNKNOWN ) { +spin_unlock (&dlm->spinlock);__dlm_wait_on_lockres (tmpres);BUG_ON (tmpres->owner ==DLM_LOCK_RES_OWNER_UNKNOWN ); +spin_unlock (&tmpres->spinlock); +dlm_lockres_put (tmpres); + tmpres =NULL ; + goto lookup; }if (tmpres->owner == dlm->node_num) {BUG_ON (tmpres->state&DLM_LOCK_RES_DROPPING_REF );dlm_lockres_grab_inflight_ref (dlm, tmpres); - }else if (tmpres->state&DLM_LOCK_RES_DROPPING_REF ) - dropping_ref =1 ; -spin_unlock (&tmpres->spinlock); - -/* wait until done messaging the master, drop our ref to allow -* the lockres to be purged, start over. */ -if (dropping_ref) { -spin_lock (&tmpres->spinlock); + }else if (tmpres->state&DLM_LOCK_RES_DROPPING_REF ) { +/* +* wait until done messaging the master, drop our ref to +* allow the lockres to be purged, start over. +*/ +spin_unlock (&dlm->spinlock);__dlm_wait_on_lockres_flags (tmpres,DLM_LOCK_RES_DROPPING_REF );spin_unlock (&tmpres->spinlock);dlm_lockres_put (tmpres);@@ -739,6 +738,24 @@ lookup: }mlog (0 ,"found in hash!\n" ); +/* +* we are going to do a create-lock next. so remove the lockres +* from purge list to avoid the case that we will access on the +* lockres which is already purged out from hash table in +* dlm_run_purge_list() path. +* otherwise, we could run into a problem: +* the owner dead(recovery didn't take care of this lockres +* since it's not in hashtable), and the code keeps sending +* request to the dead node and getting DLM_RECOVERING and +* then retrying infinitely. +*/
fs/ocfs2/dlm/dlmmaster.c lookup: · line 761
Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +7
-0+if (!list_empty (&tmpres->purge)) { +list_del_init (&tmpres->purge); +dlm_lockres_put (tmpres); + } + +spin_unlock (&tmpres->spinlock); +spin_unlock (&dlm->spinlock);if (res)dlm_lockres_put (res); res = tmpres;
In short, you are holding onto the dlm->spinlock a bit longer and forcibly removing the lockres from the purgelist.
I have two problems with this patch.
Firstly it ignores the fact that the resource can be added to the purgelist right after we drop the dlm->spinlock. There is nothing to protect against that. And I would think that is the more likely case. I had asked to you explore inflight_locks for that reason. Did you explore that option? Currently it is used for remote lock creates. That's why I suggested we use it for local creates too.
Secondly, we currently manipulate the purgelist in one function only. __dlm_calc_lockres_usage(). We should stick to that.
BTW, how are you testing this?
I would think this issue will be more of an issue for userdlm (ovm). Not the fs.
PATCH 1/1 remove lockres from purge list when we are getting it for creating lock
Quoted 98 lines · Sunil Mushran
Quoted 97 lines · Wengang Wang
When we are to purge a lockres, we check if it's unused.
the check includes
1. no locks attached to the lockres.
2. not dirty.
3. not in recovery.
4. no interested nodes in refmap.
If the the above four are satisfied, we are going to purge it(remove it from hash table).While, when a "create lock" is in progress especially when lockres is owned remotely(spinlocks are dropped when networking), the lockres can satisfy above four condition. If it's put to purge list, it can be purged out from hash table when we are still accessing on it(sending request to owner for example). That's not what we want.
I have met such a problem (orabug 12405575).
The lockres is in the purge list already(there is a delay for real purge work) and the create lock request comes. When we are sending network message to the owner indlmlock_remote(), the owner crashed. So we getDLM_RECOVERINGas return value and retriesdlmlock_remote(). And before the owner crash, we have purged the lockres. So the lockres becomestale(on lockres->onwer). Thus the code callsdlmlock_remote()infinitely.fix:
we remove the lockres from purge list if it's there indlm_get_lock_resource()which is called for only createlock case. So that the lockres can't be purged when we are in progress of createlock.Signed-off-by Wengang Wang<[email protected]>--- fs/ocfs2/dlm/dlmmaster.c | 41 +++++++++++++++++++++++++++++------------ 1 files changed, 29 insertions(+), 12 deletions(-)fs/ocfs2/dlm/dlmmaster.c +27
-12@@ -709,28 +709,27 @@ lookup:spin_lock (&dlm->spinlock); tmpres =__dlm_lookup_lockres_full (dlm, lockid, namelen, hash);if (tmpres) { -int dropping_ref =0 ; - -spin_unlock (&dlm->spinlock); -spin_lock (&tmpres->spinlock);/* We wait for the other thread that is mastering the resource */ if (tmpres->owner ==DLM_LOCK_RES_OWNER_UNKNOWN ) { +spin_unlock (&dlm->spinlock);__dlm_wait_on_lockres (tmpres);BUG_ON (tmpres->owner ==DLM_LOCK_RES_OWNER_UNKNOWN ); +spin_unlock (&tmpres->spinlock); +dlm_lockres_put (tmpres); + tmpres =NULL ; + goto lookup; }if (tmpres->owner == dlm->node_num) {BUG_ON (tmpres->state&DLM_LOCK_RES_DROPPING_REF );dlm_lockres_grab_inflight_ref (dlm, tmpres); - }else if (tmpres->state&DLM_LOCK_RES_DROPPING_REF ) - dropping_ref =1 ; -spin_unlock (&tmpres->spinlock); - -/* wait until done messaging the master, drop our ref to allow -* the lockres to be purged, start over. */ -if (dropping_ref) { -spin_lock (&tmpres->spinlock); + }else if (tmpres->state&DLM_LOCK_RES_DROPPING_REF ) { +/* +* wait until done messaging the master, drop our ref to +* allow the lockres to be purged, start over. +*/ +spin_unlock (&dlm->spinlock);__dlm_wait_on_lockres_flags (tmpres,DLM_LOCK_RES_DROPPING_REF );spin_unlock (&tmpres->spinlock);dlm_lockres_put (tmpres);@@ -739,6 +738,24 @@ lookup: }mlog (0 ,"found in hash!\n" ); +/* +* we are going to do a create-lock next. so remove the lockres +* from purge list to avoid the case that we will access on the +* lockres which is already purged out from hash table in +* dlm_run_purge_list() path. +* otherwise, we could run into a problem: +* the owner dead(recovery didn't take care of this lockres +* since it's not in hashtable), and the code keeps sending +* request to the dead node and getting DLM_RECOVERING and +* then retrying infinitely. +*/ +if (!list_empty (&tmpres->purge)) { +list_del_init (&tmpres->purge); +dlm_lockres_put (tmpres); + } +
Quoted 8 lines
fs/ocfs2/dlm/dlmmaster.c lookup: · line 761
Quoted 5 lines
fs/ocfs2/dlm/dlmmaster.c +2
-0+spin_unlock (&tmpres->spinlock); +spin_unlock (&dlm->spinlock);if (res)dlm_lockres_put (res); res = tmpres;In short, you are holding onto the
dlm->spinlocka bit longer and forcibly removing the lockres from the purgelist.
Yes. that's what the patch does.
Quoted 8 lines
I have two problems with this patch.
Firstly it ignores the fact that the resource can be added to the purgelist right after we drop the
dlm->spinlock. There is nothing to protect against that. And I would think that is the more likely case. I had asked to you exploreinflight_locksfor that reason. Did you explore that option? Currently it is used for remote lock creates. That's why I suggested we use it for local creates too.
Yes, you are right. There is such a problem that the lockres can be added to purge list after we drop dlm->spinlock. I am not sure if it's the more likely case since there is a 8 seconds delay between the the time the lockres is added to purge list and the time it is purged. I guess in normal case, the create- lock should already finished in the 8 seconds.
I considered about the inflight_locks. But I didn't think it well so far. Because simply moving the lockres out from purge list is not good enough, I will take a good think about making use of inflight_locks.
Quoted 2 lines
Secondly, we currently manipulate the purgelist in one function only.
__dlm_calc_lockres_usage(). We should stick to that.
If we make good use of inflight_locks, I think we can.
Quoted 2 lines
BTW, how are you testing this?
I tested with the steps which can cause the original problem to prove it works.(but without UT).
Also I made a regression test with the existing ocfs2-test suites(the multiple-xxx with reducing interations).
Quoted 1 line
I would think this issue will be more of an issue for userdlm (ovm). Not the fs.
Yes, agree.
thanks,
wengang.
PATCH 1/1 remove lockres from purge list when we are getting it for creating lock
Quoted 11 lines · Wengang Wang
Yes, you are right. There is such a problem that the lockres can be added to purge list after we drop
dlm->spinlock. I am not sure if it's the more likely case since there is a 8 seconds delay between the the time the lockres is added to purge list and the time it is purged. I guess in normal case, the create- lock should already finished in the 8 seconds.I considered about the
inflight_locks. But I didn't think it well so far. Because simply moving the lockres out from purge list is not good enough, I will take a good think about making use ofinflight_locks.
Quoted 8 lines
Quoted 2 lines
Secondly, we currently manipulate the purgelist in one function only.
__dlm_calc_lockres_usage(). We should stick to that.If we make good use of
inflight_locks, I think we can.Quoted 1 line
BTW, how are you testing this?
I tested with the steps which can cause the original problem to prove it works.(but without UT).
Also I made a regression test with the existing ocfs2-test suites(the multiple-xxx with reducing interations).
What's UT?
I am looking for is a testcase that reliably reproduces the problem. Not necessarily that can be checked in. But something that triggers the issue.
PATCH 1/1 remove lockres from purge list when we are getting it for creating lock
Quoted 14 lines · Sunil Mushran
Quoted 13 lines · Wengang Wang
Yes, you are right. There is such a problem that the lockres can be added to purge list after we drop
dlm->spinlock. I am not sure if it's the more likely case since there is a 8 seconds delay between the the time the lockres is added to purge list and the time it is purged. I guess in normal case, the create- lock should already finished in the 8 seconds.I considered about the
inflight_locks. But I didn't think it well so far. Because simply moving the lockres out from purge list is not good enough, I will take a good think about making use ofinflight_locks.Quoted 2 lines
Secondly, we currently manipulate the purgelist in one function only.
__dlm_calc_lockres_usage(). We should stick to that.
Quoted 8 lines
Quoted 6 lines
If we make good use of
inflight_locks, I think we can.Quoted 1 line
BTW, how are you testing this?
I tested with the steps which can cause the original problem to prove it works.(but without UT).
Also I made a regression test with the existing ocfs2-test suites(the multiple-xxx with reducing interations).What's UT?
unit test :)
Quoted 4 lines
I am looking for is a testcase that reliably reproduces the problem. Not necessarily that can be checked in. But something that triggers the issue.
sent via an another email.
thanks,
wengang.