ocfs2: fix __ocfs2_cluster_lock() dead lock
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
there is deadlock possibility in __ocfs2_cluster_lock(). the case is like following.
#in time order
1) node A(the lock owner) is doing an up-convert(UC, PR->EX). it got the lock but didn't check BUSY flag again.
2) node B requested an UC on the same lock resource. since node A has an EX on the lockres, a bast is issued and OCFS2_LOCK_BLOCKED flag is set to the lockres meanting that a DC is should be done. the DC asks for agreement of node A to release EX. and for now node A refuses that(still in process of getting that lock:) ).
3) the UC on node A runs into the phrase of checking OCFS2_LOCK_BLOCKED flag and if it can continue without waiting for any DC. code is:
if (lockres->l_flags & OCFS2_LOCK_BLOCKED &&
!ocfs2_may_continue_on_blocked_lock(lockres, level)) {analysis:
the BLOCKED flag is set in 2), and the UC can't continue to get the lock( ocfs2_may_continue_on_blocked_lock() returns false), so it the DC to finish.
so both UC and DC are waiting for each other --a dead lock.
the deadlock happens when the UC is a PR->EX and there happen to be a DC queued setting OCFS2_LOCK_BLOCKED flag when the UC checks that flag again.
fix:
fix in this patch is that if the EX lock is got, we don't check OCFS2_LOCK_BLOCKED flag again. since we have got the EX lock, we don't need to wait for any DC to finish. and we don't agree to DC the EX lock, so it' can't be DCed. --it's safe.
another solution could be in ocfs2_may_continue_on_blocked_lock(). currently it compares the wanted level and blocking level. we can change that to check each dlm_lock attached to the lockres and ignore the one for local then it works. but it's too complex and affects a lot. --give it up.
fs/ocfs2/dlmglue.c +15 -1
@@ -1313,6 +1313,7 @@ static int __ocfs2_cluster_lock(struct ocfs2_super *osb,unsigned long flags;unsigned int gen;int noqueue_attempted =0 ; +int lock_done =0 ;mlog_entry_void ();
@@ -1347,6 +1348,18 @@ again: goto unlock; } +if (lock_done) { +/* cluster lock completed already, don't wait for down- +* convertion to finish. +* if we do, in case another DC happen to be there(not on behalf +* of this lock request), and ocfs2_may_continue_on_blocked_lock() +* returns false(in case we want an EX), we go into deadlock: +* DC is waiting for our release of the EX(check_downconvert()) +* and we are waiting for that DC. +*/ + goto update_holders; + } +if (lockres->l_flags &OCFS2_LOCK_BLOCKED && !ocfs2_may_continue_on_blocked_lock (lockres, level)) {/* is the lock is currently blocked on behalf of
@@ -1414,9 +1427,11 @@ again: catch_signals =0 ;/* wait for busy to clear and carry on */ + lock_done =1 ; goto again; } +update_holders:/* Ok, if we get here then we're good to go. */ ocfs2_inc_holders (lockres, level); --
1.6.2.5
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
Quoted 16 lines · Wengang Wang
there is deadlock possibility in
__ocfs2_cluster_lock(). the case is like following.#in time order
1) node A(the lock owner) is doing an up-convert(UC,PR->EX). it got the lock but didn't check BUSY flag again.2) node B requested an UC on the same lock resource. since node A has an EX on the lockres, a bast is issued and
OCFS2_LOCK_BLOCKEDflag is set to the lockres meanting that a DC is should be done. the DC asks for agreement of node A to release EX. and for now node A refuses that(still in process of getting that lock:) ).3) the UC on node A runs into the phrase of checking
OCFS2_LOCK_BLOCKEDflag and if it can continue without waiting for any DC. code is:
Quoted 8 lines
if (lockres->l_flags & OCFS2_LOCK_BLOCKED && !ocfs2_may_continue_on_blocked_lock(lockres, level)) {analysis:
the BLOCKED flag is set in 2), and the UC can't continue to get thelock( ocfs2_may_continue_on_blocked_lock() returns false), so it the DC to finish.so both UC and DC are waiting for each other --a dead lock.
1) Do you have a test case? Have you seen this happen, or are you just postulating from reading the code?
2) It sure looks like you're right.
3) I hate our locking spaghetti!
Joel
Signature · 9 lines
"I don't know anything about music. In my line you don't have
to."
- Elvis Presley
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
Hi Joel,
Quoted 20 lines · Joel Becker
Quoted 19 lines · Wengang Wang
there is deadlock possibility in
__ocfs2_cluster_lock(). the case is like following.#in time order
1) node A(the lock owner) is doing an up-convert(UC,PR->EX). it got the lock but didn't check BUSY flag again.2) node B requested an UC on the same lock resource. since node A has an EX on the lockres, a bast is issued and
OCFS2_LOCK_BLOCKEDflag is set to the lockres meanting that a DC is should be done. the DC asks for agreement of node A to release EX. and for now node A refuses that(still in process of getting that lock:) ).3) the UC on node A runs into the phrase of checking
OCFS2_LOCK_BLOCKEDflag and if it can continue without waiting for any DC. code is:if (lockres->l_flags & OCFS2_LOCK_BLOCKED && !ocfs2_may_continue_on_blocked_lock(lockres, level)) {
Quoted 8 lines
Quoted 5 lines
analysis:
the BLOCKED flag is set in 2), and the UC can't continue to get thelock( ocfs2_may_continue_on_blocked_lock() returns false), so it the DC to finish.so both UC and DC are waiting for each other --a dead lock.
1) Do you have a test case? Have you seen this happen, or are you just postulating from reading the code?
yes, I have test case for this. when I was testing freeze/thaw, it was hit. while, the code for freeze/thaw is not sent out yet :P
the main cluster lock iterations is like this:
0) a cluster lock for freeze/thaw is added. it works like open lock on inode. so node A and node B both have PR lock on it.
1) node A want to freeze the fs, so requested for a EX.
2) node B released PR. and very immediately requests a PR waiting for thawing.
3) things described above happened.
regards,
wengang.
Quoted 2 lines
2) It sure looks like you're right.
3) I hate our locking spaghetti!
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
In reply to Joel BeckerQuoted 24 lines · Joel Becker
Quoted 23 lines · Wengang Wang
there is deadlock possibility in
__ocfs2_cluster_lock(). the case is like following.#in time order
1) node A(the lock owner) is doing an up-convert(UC,PR->EX). it got the lock but didn't check BUSY flag again.2) node B requested an UC on the same lock resource. since node A has an EX on the lockres, a bast is issued and
OCFS2_LOCK_BLOCKEDflag is set to the lockres meanting that a DC is should be done. the DC asks for agreement of node A to release EX. and for now node A refuses that(still in process of getting that lock:) ).3) the UC on node A runs into the phrase of checking
OCFS2_LOCK_BLOCKEDflag and if it can continue without waiting for any DC. code is:if (lockres->l_flags & OCFS2_LOCK_BLOCKED && !ocfs2_may_continue_on_blocked_lock(lockres, level)) {analysis:
the BLOCKED flag is set in 2), and the UC can't continue to get thelock( ocfs2_may_continue_on_blocked_lock() returns false), so it the DC to finish.
Quoted 5 lines
Quoted 1 line
so both UC and DC are waiting for each other --a dead lock.
1) Do you have a test case? Have you seen this happen, or are you just postulating from reading the code?
2) It sure looks like you're right.
well, this doesn't sounds like a sob :)
regards,
wengang.
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
This needs testing. Joel has forwarded it to Fabbione who has a testcase that we believe triggers this issue. If that works, I'll push this in the 1.6 testing tree. That way we'll know (a) the patch fixes the problem, and (b) does not break other things. This is a tricky function. We are just being careful.
Quoted 48 lines · Wengang Wang
Quoted 32 lines · Joel Becker
Quoted 31 lines · Wengang Wang
there is deadlock possibility in
__ocfs2_cluster_lock(). the case is like following.#in time order
1) node A(the lock owner) is doing an up-convert(UC,PR->EX). it got the lock
but didn't check BUSY flag again.2) node B requested an UC on the same lock resource. since node A has an EX on
the lockres, a bast is issued andOCFS2_LOCK_BLOCKEDflag is set to the lockres
meanting that a DC is should be done. the DC asks for agreement of node A to
release EX. and for now node A refuses that(still in process of getting that
lock:) ).3) the UC on node A runs into the phrase of checking
OCFS2_LOCK_BLOCKEDflag and
if it can continue without waiting for any DC. code is:if (lockres->l_flags & OCFS2_LOCK_BLOCKED && !ocfs2_may_continue_on_blocked_lock(lockres, level)) {analysis:
the BLOCKED flag is set in 2), and the UC can't continue to get the lock(ocfs2_may_continue_on_blocked_lock()returns false), so it the DC to finish.Quoted 6 lines
Quoted 1 line
so both UC and DC are waiting for each other --a dead lock.
1) Do you have a test case? Have you seen this happen, or are you just
postulating from reading the code?
2) It sure looks like you're right.well, this doesn't sounds like a sob :)
regards,
wengang._______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
http://oss.oracle.com/mailman/listinfo/ocfs2-devel
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
Quoted 5 lines · Sunil Mushran
This needs testing. Joel has forwarded it to Fabbione who has a testcase that we believe triggers this issue. If that works, I'll push this in the 1.6 testing tree. That way we'll know (a) the patch fixes the problem, and (b) does not break other things. This is a tricky function. We are just being careful.
Wengang,
Like Sunil said, this is a very critical part of the code. Our
locking code is the biggest place for lockups and corruptions to happen. We always take the safe route here. We're working to test both of your patches.
Joel
Signature · 9 lines
Life's Little Instruction Book #198 "Feed a stranger's expired parking meter." Joel Becker Principal Software Developer Oracle E-mail: joel.becker at oracle.com Phone: (650) 506-8127
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
Hi Joel and Sunil,
Sure I agree with what is doing with the patches --It's just a reminder not to forget it :).
regards,
wengang.
Quoted 26 lines · Joel Becker
Quoted 5 lines · Sunil Mushran
This needs testing. Joel has forwarded it to Fabbione who has a testcase that we believe triggers this issue. If that works, I'll push this in the 1.6 testing tree. That way we'll know (a) the patch fixes the problem, and (b) does not break other things. This is a tricky function. We are just being careful.
Wengang,
Like Sunil said, this is a very critical part of the code. Our
locking code is the biggest place for lockups and corruptions to happen. We always take the safe route here. We're working to test both of your patches.Joel
Signature · 9 lines
Life's Little Instruction Book #198 "Feed a stranger's expired parking meter." Joel Becker Principal Software Developer Oracle E-mail: joel.becker at oracle.com Phone: (650) 506-8127
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
In reply to Wengang Wang[Cc'd Mark and Dave]
Quoted 2 lines · Wengang Wang
Quoted 1 line
2) It sure looks like you're right.
well, this doesn't sounds like a sob :)
With good reason, too. You've got a bug. Dave Teigland (copied
now) found it while testing with fs/dlm.
The short description is that you didn't notice the handling of
async -EAGAIN. Not really your fault, there are no obvious comments. With o2dlm, you get -EAGAIN from the call to ocfs2_dlm_lock(). That's safe with your patch. However, with fs/dlm, EAGAIN comes asynchronously. ocfs2_dlm_lock() will return 0. When the ast fires, the ast notices the -EAGAIN and clears the locking state for a retry. In your patch, however, you take the 'lock_done' + !BUSY and immediately increment the holders - even though the lock hasn't been acquired!
The fix is pretty simple, though - when lock_done is set, you
know we've at least tried to upconvert. If the locking level is good, we know the upconvert succeeded. We can then go to inc_holders. I've attached a new version of your patch that does this.
I've also modified the patch to reflect that the original
problem you found is actually a livelock, not a deadlock. And I added a little comment about the async -EAGAIN.
I've attached the full patch with my changes. Dave, please test
my version (the attached one) instead of Wengang's. Mark, can you do a sanity check here?
Here's the diff between your patch and mine:
fs/ocfs2/dlmglue.c +24 -10
@@ -1313,7 +1313,7 @@ static int __ocfs2_cluster_lock(struct ocfs2_super *osb,unsigned long flags;unsigned int gen;int noqueue_attempted =0 ; -int lock_done =0 ; +int lock_attempted =0 ;mlog_entry_void ();
@@ -1348,18 +1348,25 @@ again: goto unlock; } -if (lock_done) { -/* cluster lock completed already, don't wait for down- -* convertion to finish. -* if we do, in case another DC happen to be there(not on behalf -* of this lock request), and ocfs2_may_continue_on_blocked_lock() -* returns false(in case we want an EX), we go into deadlock: -* DC is waiting for our release of the EX(check_downconvert()) -* and we are waiting for that DC. +if (lock_attempted && (lockres->l_level >= level)) { +/* +* We've attempted to upconvert, and the lock now has +* a level we can work with. If we fell through to the +* next checks, we could spin in an upconvert->downconvert +* cycle as other nodes pounded us. Instead, we jump +* out and let the caller do some work. If a downconvert +* has come in, it will do its thing as soon as the caller +* is done. */ goto update_holders; } +/* +* This is either the first pass or our level wasn't good enough. +* Treat both cases as a first pass. +*/ + lock_attempted =0 ; +if (lockres->l_flags &OCFS2_LOCK_BLOCKED && !ocfs2_may_continue_on_blocked_lock (lockres, level)) {/* is the lock is currently blocked on behalf of
@@ -1370,6 +1377,13 @@ again: }if (level > lockres->l_level) { +/* +* If the dlm notifies us of -EAGAIN asynchronously, it +* will have returned 0 from the call to ocfs2_dlm_lock() +* and given us -EAGAIN in the ast. The ast will clear +* out our state. We notice that here and return -EAGAIN +* to the caller. +*/ if (noqueue_attempted >0 ) { ret = -EAGAIN ; goto unlock;
@@ -1427,7 +1441,7 @@ again: catch_signals =0 ;/* wait for busy to clear and carry on */ - lock_done =1 ; + lock_attempted =1 ; goto again; }
Joel
Signature · 16 lines
"If you are ever in doubt as to whether or not to kiss a pretty girl,
give her the benefit of the doubt"
-Thomas Carlyle
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ocfs2-fix-__ocfs2_cluster_lock-livelock.patch
Type: text/x-diff
Size: 3646 bytes
Desc: not available
Url : http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20100111/1264b243/attachment.bin PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
Hi Joel,
Quoted 6 lines · Joel Becker
[Cc'd Mark and Dave]
Quoted 2 lines · Wengang Wang
Quoted 1 line
2) It sure looks like you're right.
well, this doesn't sounds like a sob :)
Quoted 10 lines
With good reason, too. You've got a bug. Dave Teigland (copied
now) found it while testing with fs/dlm.
The short description is that you didn't notice the handling of
async-EAGAIN. Not really your fault, there are no obvious comments. With o2dlm, you get-EAGAINfrom the call toocfs2_dlm_lock(). That's safe with your patch. However, with fs/dlm, EAGAIN comes asynchronously.ocfs2_dlm_lock()will return 0. When the ast fires, the ast notices the-EAGAINand clears the locking state for a retry. In your patch, however, you take the 'lock_done' + !BUSY and immediately increment the holders - even though the lock hasn't been acquired!
ah, yes. I fault.
I should check more deeper.
Quoted 11 lines
The fix is pretty simple, though - when
lock_doneis set, you
know we've at least tried to upconvert. If the locking level is good, we know the upconvert succeeded. We can then go toinc_holders. I've attached a new version of your patch that does this.
I've also modified the patch to reflect that the original
problem you found is actually a livelock, not a deadlock. And I added a little comment about the async-EAGAIN.
I've attached the full patch with my changes. Dave, please test
my version (the attached one) instead of Wengang's. Mark, can you do a sanity check here?
Here's the diff between your patch and mine:
snip...
Quoted 20 lines
Date: Wed, 6 Jan 2010 16:34:44 +0800
Subject: [PATCH] ocfs2: fix__ocfs2_cluster_lock()livelockThere is livelock possibility in
__ocfs2_cluster_lock(). Here's what happens:1) node A(the lock owner) is doing an up-convert(UC,
PR->EX). it gets the lock but doesn't check BUSY again because the level is right.2) node B requested an UC on the same lock resource. since node A has an EX on the lockres, a bast is issued and
OCFS2_LOCK_BLOCKEDflag is set to the lockres meanting that a DC is should be done. the DC asks for agreement of node A to release EX. The downconvert starts because BUSY is cleared by the ast.3) the UC on node A runs into the check of
OCFS2_LOCK_BLOCKED:if (lockres->l_flags & OCFS2_LOCK_BLOCKED && !ocfs2_may_continue_on_blocked_lock(lockres, level)) {
Quoted 7 lines
analysis:
the BLOCKED flag is set in 2), and the UC can't continue to get the lock (ocfs2_may_continue_on_blocked_lock()returns false), so it waits on the DC to finish.4) The DC finishes, and BLOCKED is cleared. The UC on node A starts over getting the UC, now from
NL->EX. It asks node B for the lock.
I am wondering why DC can finish.
although BUSY is cleared. I think the check_downconvert() should return false since we have not done whatever we want to do. and then the DC should be requeued.
#don't hate me if I am wrong/stupid :)
regards,
wengang.
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
Quoted 26 lines · Wengang Wang
Quoted 25 lines · Joel Becker
Date: Wed, 6 Jan 2010 16:34:44 +0800
Subject: [PATCH] ocfs2: fix__ocfs2_cluster_lock()livelockThere is livelock possibility in
__ocfs2_cluster_lock(). Here's what happens:1) node A(the lock owner) is doing an up-convert(UC,
PR->EX). it gets the lock but doesn't check BUSY again because the level is right.2) node B requested an UC on the same lock resource. since node A has an EX on the lockres, a bast is issued and
OCFS2_LOCK_BLOCKEDflag is set to the lockres meanting that a DC is should be done. the DC asks for agreement of node A to release EX. The downconvert starts because BUSY is cleared by the ast.3) the UC on node A runs into the check of
OCFS2_LOCK_BLOCKED:if (lockres->l_flags & OCFS2_LOCK_BLOCKED && !ocfs2_may_continue_on_blocked_lock(lockres, level)) {analysis:
the BLOCKED flag is set in 2), and the UC can't continue to get the lock (ocfs2_may_continue_on_blocked_lock()returns false), so it waits on the DC to finish.
Quoted 7 lines
Quoted 2 lines
4) The DC finishes, and BLOCKED is cleared. The UC on node A starts over getting the UC, now from
NL->EX. It asks node B for the lock.I am wondering why DC can finish.
although BUSY is cleared. I think thecheck_downconvert()should return false since we have not done whatever we want to do. and then the DC should be requeued.
DC thread only waits on PENDING and holders. If BUSY, it will
cancel the upconvert. If not BUSY, it will schedule a downconvert. There's nothing stopping the downconvert, in other words.
Quoted 1 line
#don't hate me if I am wrong/stupid :)
Won't hate you at all. And you're not stupid even if you are
wrong. It's not easy code. Heck, you may be right and I might be wrong in the end. Remember, you found the bug in the first place!
Joel
Signature · 9 lines
Life's Little Instruction Book #450 "Don't be afraid to say, 'I need help.'" Joel Becker Principal Software Developer Oracle E-mail: joel.becker at oracle.com Phone: (650) 506-8127
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
Hi Joel,
Quoted 28 lines · Joel Becker
Quoted 28 lines
Quoted 27 lines
Date: Wed, 6 Jan 2010 16:34:44 +0800
Subject: [PATCH] ocfs2: fix__ocfs2_cluster_lock()livelockThere is livelock possibility in
__ocfs2_cluster_lock(). Here's what happens:1) node A(the lock owner) is doing an up-convert(UC,
PR->EX). it gets the lock but doesn't check BUSY again because the level is right.2) node B requested an UC on the same lock resource. since node A has an EX on the lockres, a bast is issued and
OCFS2_LOCK_BLOCKEDflag is set to the lockres meanting that a DC is should be done. the DC asks for agreement of node A to release EX. The downconvert starts because BUSY is cleared by the ast.3) the UC on node A runs into the check of
OCFS2_LOCK_BLOCKED:if (lockres->l_flags & OCFS2_LOCK_BLOCKED && !ocfs2_may_continue_on_blocked_lock(lockres, level)) {analysis:
the BLOCKED flag is set in 2), and the UC can't continue to get the lock (ocfs2_may_continue_on_blocked_lock()returns false), so it waits on the DC to finish.4) The DC finishes, and BLOCKED is cleared. The UC on node A starts over getting the UC, now from
NL->EX. It asks node B for the lock.
Quoted 8 lines
Quoted 4 lines
I am wondering why DC can finish.
although BUSY is cleared. I think thecheck_downconvert()should return false since we have not done whatever we want to do. and then the DC should be requeued.DC thread only waits on PENDING and holders. If BUSY, it will
cancel the upconvert. If not BUSY, it will schedule a downconvert. There's nothing stopping the downconvert, in other words.
I think I am talking about the scheduled downconvert. let me copy src here. the downconvert kernel thread does a call stack like this:ocfs2_downconvert_thread_do_work()
-->ocfs2_process_blocked_lock
-->ocfs2_unblock_lock()
in ocfs2_unblock_lock() there are lines after checking for BUSY flag:
if (lockres->l_ops->check_downconvert
&& !lockres->l_ops->check_downconvert(lockres, new_level))
goto leave_requeue;[snip...]leave_requeue:
spin_unlock_irqrestore(&lockres->l_lock, flags);
ctl->requeue = 1;if ctl->requeue is set, ocfs2_schedule_blocked_lock() will add the lockres to blocked_lock_list. the downconvert will then be rescheduled only if ocfs2_wake_downconvert_thread() is called somewhere.
in this simple case:
the "somewhere", I think, is in __ocfs2_cluster_unlock().
so before __ocfs2_cluster_unlock() is called(surely after
__ocfs2_cluster_lock() returns), the downconvert couldn't
be rescheduled.
thus __ocfs2_cluster_lock() is waiting for DC to finish and the DC is waiting __ocfs2_cluster_unlock() to be called. --deadlock.
so maybe you don't notice the lockres->l_ops->check_downconvert()?
Quoted 6 lines
Quoted 1 line
#don't hate me if I am wrong/stupid :)
Won't hate you at all. And you're not stupid even if you are
wrong. It's not easy code. Heck, you may be right and I might be wrong in the end. Remember, you found the bug in the first place!
great!
regards,
wengang.
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
Quoted 4 lines · Wengang Wang
Quoted 3 lines · Joel Becker
DC thread only waits on PENDING and holders. If BUSY, it will
cancel the upconvert. If not BUSY, it will schedule a downconvert. There's nothing stopping the downconvert, in other words.
...
Quoted 5 lines
in
ocfs2_unblock_lock()there are lines after checking for BUSY flag:if (lockres->l_ops->check_downconvert && !lockres->l_ops->check_downconvert(lockres, new_level)) goto leave_requeue;
...
Quoted 11 lines
if
ctl->requeueis set,ocfs2_schedule_blocked_lock()will add the lockres toblocked_lock_list. the downconvert will then be rescheduled only ifocfs2_wake_downconvert_thread()is called somewhere.
in this simple case:
the "somewhere", I think, is in__ocfs2_cluster_unlock().
so before__ocfs2_cluster_unlock()is called(surely after
__ocfs2_cluster_lock()returns), the downconvert couldn't
be rescheduled.
thus__ocfs2_cluster_lock()is waiting for DC to finish and the DC is waiting__ocfs2_cluster_unlock()to be called. --deadlock.
Quoted 1 line
so maybe you don't notice the
lockres->l_ops->check_downconvert()?
I notice it. I know what it checks. If the lock is not
currently taken it will return "go ahead and downconvert". So we won't requeue, we'll downconvert. Specifically, look at ocfs2_check_meta_downconvert(). It merely checks the caching info. We can't have any cached data if we don't have the EX yet.
Sunil has observed the livelock behavior (pinging between UC and
DC) in the wild via traces. The UC process never leaves ocfs2_cluster_lock(). Your change fixes that.
Joel
Signature · 8 lines
"I don't even butter my bread; I consider that cooking."
- Katherine Cebrian
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
Quoted 7 lines · Joel Becker
Quoted 1 line
so maybe you don't notice the
lockres->l_ops->check_downconvert()?I notice it. I know what it checks. If the lock is not
currently taken it will return "go ahead and downconvert". So we won't requeue, we'll downconvert. Specifically, look atocfs2_check_meta_downconvert(). It merely checks the caching info. We can't have any cached data if we don't have the EX yet.
Is it a rule that If the lock is not currently taken it will return "go ahead and downconvert"?
by checking ocfs2_check_meta_downconvert(), I think it ensures cache is checkpointed by JBD(2) before returning "go ahead and downconvert". maybe yes that you noticed that we can't have cache if we don't have EX, I think the main purpose is that it ensures that we finish things which we should finish before we release the lock. another evidence is that it doesn't check the ex_holders.
another question is that you can find out ocfs2_check_meta_downconvert() checks if lock is taken(not explicitly), can you make such a conclusion that all check_downconvert() follows it? --sorry, I didn't check them and pushed it to you :P --I will check them too later.
so still the question, is it a rule that check_downconvert() should return "go ahead and downconvert" if the lock is not currently taken? and also, what means "currently taken"? --ocfs2 layer(after ocfs2_cluster_lock() returns) or dlm level? though I guess you meant ocfs2 layer.
regards,
wengang.
Quoted 4 lines
Sunil has observed the livelock behavior (pinging between UC and
DC) in the wild via traces. The UC process never leavesocfs2_cluster_lock(). Your change fixes that.
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
Quoted 2 lines · Wengang Wang
Is it a rule that If the lock is not currently taken it will return "go ahead and downconvert"?
In short, yes, because of fairness.
Not having holders is one of the checks. If there are incompatible holders then it will requeue the lockres in the blocked queue. The lockres will be re-processed when the dc thread is kicked the next time. If there are no holders and the journal is checkpointed (or check_downconvert succeeds), then yes, it will be downconvert.
Quoted 6 lines
by checking
ocfs2_check_meta_downconvert(), I think it ensures cache is checkpointed by JBD(2) before returning "go ahead and downconvert". maybe yes that you noticed that we can't have cache if we don't have EX, I think the main purpose is that it ensures that we finish things which we should finish before we release the lock. another evidence is that it doesn't check theex_holders.
check_downconvert() is an additional check. The holders check tells the fs whether it can and whether it needs to down convert or not. If answer to both qs is yes, then it sees whether there are any lock type specific checks. M and T lock types want the journal check pointed before a dc.
Quoted 10 lines
another question is that you can find out
ocfs2_check_meta_downconvert()checks if lock is taken(not explicitly), can you make such a conclusion that allcheck_downconvert()follows it? --sorry, I didn't check them and pushed it to you :P --I will check them too later.so still the question, is it a rule that
check_downconvert()should return "go ahead and downconvert" if the lock is not currently taken? and also, what means "currently taken"? --ocfs2layer(after ocfs2_cluster_lock() returns)or dlm level? though I guess you meant ocfs2 layer.
I am confused by your qs. Have you read the description of check_downconvert()?
/*
* Allow a lock type to add checks to determine whether it is
* safe to downconvert a lock. Return 0 to re-queue the
* downconvert at a later time, nonzero to continue. *
* For most locks, the default checks that there are no
* incompatible holders are sufficient.
*
* Called with the lockres spinlock held.
*/
int (*check_downconvert)(struct ocfs2_lock_res *, int);
Sunil
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
Quoted 4 lines · Sunil Mushran
Quoted 2 lines · Wengang Wang
Is it a rule that If the lock is not currently taken it will return "go ahead and downconvert"?
Quoted 8 lines
In short, yes, because of fairness.
Not having holders is one of the checks. If there are incompatible holders then it will requeue the lockres in the blocked queue. The lockres will be re-processed when the dc thread is kicked the next time. If there are no holders and the journal is checkpointed (or
check_downconvertsucceeds), then yes, it will be downconvert.
Ok, got it.
Quoted 11 lines
Quoted 6 lines
by checking
ocfs2_check_meta_downconvert(), I think it ensures cache is checkpointed by JBD(2) before returning "go ahead and downconvert". maybe yes that you noticed that we can't have cache if we don't have EX, I think the main purpose is that it ensures that we finish things which we should finish before we release the lock. another evidence is that it doesn't check theex_holders.
check_downconvert()is an additional check. The holders check tells the fs whether it can and whether it needs to down convert or not. If answer to both qs is yes, then it sees whether there are any lock type specific checks. M and T lock types want the journal check pointed before a dc.
yes, understood.
the holders check is done in ocfs2_unblock_lock() and the check_downconvert() checks specific things.
whether the problem is a deadlock or a livelock depends on whether the check_downconvert()(if there is) doesn't agree to dc in some condition. if it doesn't agree, it's a deadlock. otherwise, it's a livelock.
Quoted 28 lines
Quoted 11 lines
another question is that you can find out
ocfs2_check_meta_downconvert()checks if lock is taken(not explicitly), can you make such a conclusion that allcheck_downconvert()follows it? --sorry, I didn't check them and pushed it to you :P --I will check them too later.so still the question, is it a rule that
check_downconvert()should return "go ahead and downconvert" if the lock is not currently taken? and also, what means "currently taken"? --ocfs2layer(after ocfs2_cluster_lock() returns)or dlm level? though I guess you meant ocfs2 layer.I am confused by your qs. Have you read the description of
check_downconvert()?/*
* Allow a lock type to add checks to determine whether it is
* safe to downconvert a lock. Return 0 to re-queue the
* downconvert at a later time, nonzero to continue. *
* For most locks, the default checks that there are no
* incompatible holders are sufficient.
*
* Called with the lockres spinlock held.
*/
int (*check_downconvert)(structocfs2_lock_res*, int);
Quoted 1 line
Sunil
got, please ignore the above part.
regards,
wengang.
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
In reply to Joel BeckerQuoted 2 lines · Joel Becker
I've attached the full patch with my changes. Dave, please test
my version (the attached one) instead of Wengang's.
Your new patch fixes the mount, so I went on to test make_panic which is the test we never got to work:
http://oss.oracle.com/pipermail/ocfs2-devel/2009-April/004313.html
https://bugzilla.novell.com/show_bug.cgi?id=492055
It ran on three nodes for several minutes; much longer than it ever had before. It eventually triggered different BUG's on two of the nodes, rather than just getting stuck as it used to. I wasn't watching, so I don't know which of these came first.
One node had:
kernel BUG at fs/ocfs2/dlmglue.c:3567!
invalid opcode: 0000 [#1] SMP
last sysfs file: /sys/devices/system/cpu/cpu3/cache/index2/shared_cpu_map CPU 3
Modules linked in: ocfs2_stack_user dlm ocfs2 jbd2 ocfs2_nodemanager
configfs ocfs2_stackglue ipt_REJECT xt_tcpudp iptable_filter ip_tables x_tables bridge stp autofs4 sunrpc ipv6 iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi cpufreq_ondemand dm_multipath video output sbs sbshc battery ac parport_pc lp parport sg serio_raw button tg3 libphy i2c_nforce2 i2c_core pcspkr dm_snapshot dm_zero dm_mirror dm_region_hash dm_log dm_mod qla2xxx scsi_transport_fc shpchp mptspi mptscsih mptbase scsi_transport_spi sd_mod scsi_mod ext3 jbd uhci_hcd ohci_hcd ehci_hcd
Pid: 4019, comm: ocfs2dc Not tainted 2.6.32.3 #2 ProLiant DL145 G2
RIP: 0010:[<ffffffffa03a8148>] [<ffffffffa03a8148>] ocfs2_ci_checkpointed+0x7c/0xcb [ocfs2] RSP: 0018:ffff8800785b7dc0 EFLAGS: 00010002 RAX: 0000000000000001 RBX: 0000000000000411 RCX: ffff8800779b53b8 RDX: 000000000000d0cf RSI: 0000000000000001 RDI: 0000000000000000 RBP: ffff8800785b7df0 R08: ffffffffa03a810a R09: ffffffffa03aa010 R10: ffff88013f421e68 R11: ffff8800839d36c0 R12: ffffffffffffffff R13: ffff880078b73938 R14: 0000000000000000 R15: ffff880078b73368 FS: 00007f589e5126e0(0000) GS:ffff880083a00000(0000)
knlGS:0000000000000000
CS: 0010 DS: 0018 ES: 0018 CR0: 000000008005003b
CR2: 00007f0598be6000 CR3: 0000000133608000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 Process ocfs2dc (pid: 4019, threadinfo ffff8800785b6000, task ffff8800779b4cc0)
Stack: ffff88007856c000 0000000000000282 ffff880078b73368 0000000000000000 <0> ffff88007856c000 0000000000000282 ffff8800785b7e00 ffffffffa03ab4b0 <0> ffff8800785b7eb0 ffffffffa03aa16d ffff8800785b7eb0 ffffffff813528ed Call Trace: [<ffffffffa03ab4b0>] ocfs2_check_meta_downconvert+0x32/0x34 [ocfs2] [<ffffffffa03aa16d>] ocfs2_downconvert_thread+0x470/0x869 [ocfs2] [<ffffffff813528ed>] ? thread_return+0x3e/0xee [<ffffffff8105ff63>] ? autoremove_wake_function+0x0/0x3d [<ffffffffa03a9cfd>] ? ocfs2_downconvert_thread+0x0/0x869 [ocfs2] [<ffffffff8105fe55>] kthread+0x82/0x8d [<ffffffff8100cc1a>] child_rip+0xa/0x20 [<ffffffff8105fdb2>] ? kthreadd+0xc7/0xe8 [<ffffffff8105fdd3>] ? kthread+0x0/0x8d [<ffffffff8100cc10>] ? child_rip+0x0/0x20 Code: e0 45 85 f6 74 0a 41 83 fe 03 74 04 0f 0b eb fe 49 29 dc 49 f7 d4 4c
89 e0 48 c1 e8 3f 41 83 bf a0 00 00 00 05 74 08 84 c0 74 08 <0f> 0b eb fe 84 c0 75 07 b8 01 00 00 00 eb 33 4c 89 ef e8 07 ac
RIP [<ffffffffa03a8148>] ocfs2_ci_checkpointed+0x7c/0xcb [ocfs2]
RSP <ffff8800785b7dc0>
Another node had:
(2881,2):ocfs2_inode_lock_update:2224 ERROR: bug expression:inode->i_generation != le32_to_cpu(fe->i_generation) (2881,2):ocfs2_inode_lock_update:2224 ERROR: Invalid dinode 420382 disk
generation: 1523484106 inode->i_generation: 1523484094
------------[ cut here ]------------ kernel BUG at fs/ocfs2/dlmglue.c:2224! invalid opcode: 0000 [#1] SMP last sysfs file: /sys/devices/system/cpu/cpu3/cache/index2/shared_cpu_map CPU 2 Modules linked in: ocfs2_stack_user dlm ocfs2 jbd2 ocfs2_nodemanager
configfs ocfs2_stackglue ipt_REJECT xt_tcpudp iptable_filter ip_tables x_tables bridge stp sunrpc ipv6 cpufreq_ondemand dm_multipath uinput serio_raw pcspkr sg qla2xxx scsi_transport_fc tg3 libphy i2c_nforce2 i2c_core button dm_snapshot dm_zero dm_mirror dm_region_hash dm_log dm_mod shpchp mptspi mptscsih mptbase scsi_transport_spi sd_mod scsi_mod ext3 jbd uhci_hcd ohci_hcd ehci_hcd [last unloaded: scsi_wait_scan]
Pid: 2881, comm: make_panic Not tainted 2.6.32.3 #1 ProLiant DL145 G2
RIP: 0010:[<ffffffffa032e9a0>] [<ffffffffa032e9a0>] ocfs2_inode_lock_full_nested+0x850/0xd00 [ocfs2] RSP: 0018:ffff88007b55bce8 EFLAGS: 00010296 RAX: 0000000000000085 RBX: ffff880067c95000 RCX: 000000000000be01 RDX: ffff880083800000 RSI: 0000000000000001 RDI: 0000000000000003 RBP: ffff88007b55bd58 R08: 0000000000000092 R09: 0000000000000000 R10: 0000000000000003 R11: 0000000000018600 R12: ffff880066a8b2d8 R13: ffff880066a8b6e8 R14: ffff880066a8b840 R15: ffff880066a8b040 FS: 00007fa9c96b66f0(0000) GS:ffff880083800000(0000)
knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00007f54f5021000 CR3: 000000007e82a000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 Process make_panic (pid: 2881, threadinfo ffff88007b55a000, task ffff88007e9d40c0)
Stack: ffffffff5ace85ca 000001015ace85be 00000000810f4897 0000000000000000 <0> 0000000000000000 000000017f59ece0 ffff880066a8b228 ffffffff81103794 <0> ffff880068122cb0 ffff880066a8b840 ffff880066a8b840 0000000000000026 Call Trace: [<ffffffff81103794>] ? mntput_no_expire+0x29/0xfd [<ffffffffa03354f1>] ocfs2_permission+0x78/0x16f [ocfs2] [<ffffffff810f3ea5>] inode_permission+0x6e/0x9e [<ffffffff810f593c>] may_open+0x9e/0x252 [<ffffffff810f8389>] do_filp_open+0x51f/0xa55 [<ffffffff81101c14>] ? alloc_fd+0x122/0x133 [<ffffffff810ea0b0>] do_sys_open+0x62/0x109 [<ffffffff810ea18a>] sys_open+0x20/0x22 [<ffffffff8100bc5b>] system_call_fastpath+0x16/0x1b Code: ff 48 c7 c1 b0 eb 37 a0 48 c7 c7 0b 6d 38 a0 65 8b 14 25 08 cd 00 00
89 44 24 08 8b 43 08 48 63 d2 89 04 24 31 c0 e8 37 cc 02 e1 <0f> 0b eb fe 48 83 7b 48 00 75 0a f6 43 2c 01 0f 85 bc 00 00 00
RIP [<ffffffffa032e9a0>] ocfs2_inode_lock_full_nested+0x850/0xd00 [ocfs2]
RSP <ffff88007b55bce8>
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
In reply to Joel BeckerHi Joel and David,
I think the patch to fix the deadlock(or livelock) is not good enough yet.
my original patch is based on that when we have requested the lock, we don't allow to downconvert it before we release it.
but per the communications with Joel and Sunil, I found my base is wrong. so my original patch is not good. and Joel's patch has problem either.
Quoted 24 lines · Joel Becker
[Cc'd Mark and Dave]Signed-off-by Joel Becker <[email protected]>--- fs/ocfs2/dlmglue.c | 29 +++++++++++++++++++++++++++++ 1 files changed, 29 insertions(+), 0 deletions(-)fs/ocfs2/dlmglue.c +3
-0@@ -1313,6 +1313,7 @@ static int __ocfs2_cluster_lock(struct ocfs2_super *osb,unsigned long flags;unsigned int gen;int noqueue_attempted =0 ; +int lock_attempted =0 ;mlog_entry_void ();@@ -1347,6 +1348,25 @@ again: goto unlock; } +if (lock_attempted && (lockres->l_level >= level)) { +/*
fs/ocfs2/dlmglue.c again: · line 1362
Quoted 10 lines
fs/ocfs2/dlmglue.c +10
-0+* We've attempted to upconvert, and the lock now has +* a level we can work with. If we fell through to the +* next checks, we could spin in an upconvert->downconvert +* cycle as other nodes pounded us. Instead, we jump +* out and let the caller do some work. If a downconvert +* has come in, it will do its thing as soon as the caller +* is done. +*/ + goto update_holders; + }
before update_holders, the lock could be DCed(since no BUSY flag set by here).
and even after update_holders, the lock could be DCed too.
so that we get ocfs2_cluster_lock()(with holders increased) returned sucessfully but actually we don't hold the dlm lock. --thus more than one node is considering that they have the (EX) lock.
that's could be the cause of what is happening observed by David.
regards,
wengang.
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
Quoted 49 lines · Wengang Wang
Hi Joel and David,
I think the patch to fix the deadlock(or livelock) is not good enough yet.
my original patch is based on that when we have requested the lock, we don't allow to downconvert it before we release it.
but per the communications with Joel and Sunil, I found my base is wrong. so my original patch is not good. and Joel's patch has problem either.Quoted 24 lines · Joel Becker
[Cc'd Mark and Dave]Signed-off-by Joel Becker <[email protected]>--- fs/ocfs2/dlmglue.c | 29 +++++++++++++++++++++++++++++ 1 files changed, 29 insertions(+), 0 deletions(-)fs/ocfs2/dlmglue.c +3
-0@@ -1313,6 +1313,7 @@ static int __ocfs2_cluster_lock(struct ocfs2_super *osb,unsigned long flags;unsigned int gen;int noqueue_attempted =0 ; +int lock_attempted =0 ;mlog_entry_void ();@@ -1347,6 +1348,25 @@ again: goto unlock; } +if (lock_attempted && (lockres->l_level >= level)) { +/* fs/ocfs2/dlmglue.c again: · line 1362
Quoted 10 lines
fs/ocfs2/dlmglue.c +10
-0+* We've attempted to upconvert, and the lock now has +* a level we can work with. If we fell through to the +* next checks, we could spin in an upconvert->downconvert +* cycle as other nodes pounded us. Instead, we jump +* out and let the caller do some work. If a downconvert +* has come in, it will do its thing as soon as the caller +* is done. +*/ + goto update_holders; + }before
update_holders, the lock could be DCed(since no BUSY flag set by here).
Quoted 7 lines
and even after
update_holders, the lock could be DCed too.so that we get ocfs2_cluster_lock()(with holders increased) returned sucessfully but actually we don't hold the dlm lock. --thus more than one node is considering that they have the (EX) lock.
that's could be the cause of what is happening observed by David.
I think the main problem is that though the lock is got, the holders is not updated in time(within BUSY flag protection).
a BUSY flag protects a single ocfs2_dlm_lock(). if a BUSY_INC_HD(also busy with updating holders) is used to protect a single ocfs2_dlm_lock() + holders updates against dc thread, so far I think it's the fix. comments?
#patch attached.
-------------- next part --------------
fs/ocfs2/dlmglue.c +6 -2
@@ -1360,6 +1360,8 @@ again: goto update_holders; } +lockres_clear_flags (lockres,OCFS2_LOCK_BUSY_INC_HD ); +/* * This is either the first pass or our level wasn't good enough. * Treat both cases as a first pass.
@@ -1403,7 +1405,7 @@ again: } lockres->l_requested = level; -lockres_or_flags (lockres,OCFS2_LOCK_BUSY ); +lockres_or_flags (lockres,OCFS2_LOCK_BUSY |OCFS2_LOCK_BUSY_INC_HD ); gen =lockres_set_pending (lockres);spin_unlock_irqrestore (&lockres->l_lock, flags);
@@ -1447,6 +1449,8 @@ again: update_holders:/* Ok, if we get here then we're good to go. */ ocfs2_inc_holders (lockres, level); +/* no harm even double clear OCFS2_LOCK_BUSY_INC_HD */ +lockres_clear_flags (lockres,OCFS2_LOCK_BUSY_INC_HD ); ret =0 ; unlock:
@@ -3392,7 +3396,7 @@ static int ocfs2_unblock_lock(struct ocfs2_super *osb,BUG_ON (!(lockres->l_flags &OCFS2_LOCK_BLOCKED )); recheck: -if (lockres->l_flags &OCFS2_LOCK_BUSY ) { +if (lockres->l_flags & (OCFS2_LOCK_BUSY |OCFS2_LOCK_BUSY_INC_HD )) {/* XXX * This is a *big* race. The OCFS2_LOCK_PENDING flag * exists entirely for one reason - another thread has set
fs/ocfs2/ocfs2.h +1 -0
@@ -136,6 +136,7 @@ enum ocfs2_unlock_action {#define OCFS2_LOCK_PENDING (0x00000400)/* This lockres is pending a call to dlm_lock. Only exists with BUSY set. */ +#define OCFS2_LOCK_BUSY_INC_HD (0x00000800)/* busy with increase holders */ struct ocfs2_lock_res_ops ;
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
In reply to Wengang WangQuoted 8 lines · Wengang Wang
before
update_holders, the lock could be DCed(since no BUSY flag set by here).and even after
update_holders, the lock could be DCed too.so that we get ocfs2_cluster_lock()(with holders increased) returned sucessfully but actually we don't hold the dlm lock. --thus more than one node is considering that they have the (EX) lock.
You make a good point. I don't like the solution you propose,
though. Another flag that's almost the same? Eww! There's got to be a better way.
Joel
Signature · 9 lines
Life's Little Instruction Book #80 "Slow dance" Joel Becker Principal Software Developer Oracle E-mail: joel.becker at oracle.com Phone: (650) 506-8127
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
Quoted 6 lines · Joel Becker
Quoted 5 lines · Wengang Wang
before
update_holders, the lock could be DCed(since no BUSY flag set by here).and even after
update_holders, the lock could be DCed too.
Quoted 7 lines
Quoted 3 lines
so that we get ocfs2_cluster_lock()(with holders increased) returned sucessfully but actually we don't hold the dlm lock. --thus more than one node is considering that they have the (EX) lock.
You make a good point. I don't like the solution you propose,
though. Another flag that's almost the same? Eww! There's got to be a better way.
cool! expecting your patch...
regards,
wengang.
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
Quoted 7 lines · Wengang Wang
Quoted 6 lines · Joel Becker
Quoted 5 lines · Wengang Wang
before
update_holders, the lock could be DCed(since no BUSY flag set by here).and even after
update_holders, the lock could be DCed too.
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
so that we get ocfs2_cluster_lock()(with holders increased) returned sucessfully but actually we don't hold the dlm lock. --thus more than one node is considering that they have the (EX) lock.
You make a good point. I don't like the solution you propose,
though. Another flag that's almost the same? Eww! There's got to be a better way.cool! expecting your patch...
Don't have one yet. We can't rely on the spinlock, because we
have to sleep on BUSY. We relied on the spinblock before because we allowed the UC->DC->UC->DC livelock to happen.
Joel
Signature · 8 lines
"Up and down that road in our worn out shoes, Talking bout good things and singing the blues." Joel Becker Principal Software Developer Oracle E-mail: joel.becker at oracle.com Phone: (650) 506-8127
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
Quoted 9 lines · Joel Becker
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
before
update_holders, the lock could be DCed(since no BUSY flag set by here).and even after
update_holders, the lock could be DCed too.so that we get ocfs2_cluster_lock()(with holders increased) returned sucessfully but actually we don't hold the dlm lock. --thus more than one node is considering that they have the (EX) lock.
Quoted 9 lines
Quoted 4 lines
Quoted 3 lines
You make a good point. I don't like the solution you propose,
though. Another flag that's almost the same? Eww! There's got to be a better way.cool! expecting your patch...
Don't have one yet. We can't rely on the spinlock, because we
have to sleep on BUSY. We relied on the spinblock before because we allowed theUC->DC->UC->DClivelock to happen.
then I feel it's going to be a big change.
so that the author shouldn't be me, add me as something like bug-found-by if you like :)
regards,
wengang.
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
In reply to Joel BeckerQuoted 6 lines · Joel Becker
Quoted 5 lines · Wengang Wang
before
update_holders, the lock could be DCed(since no BUSY flag set by here).and even after
update_holders, the lock could be DCed too.
Quoted 7 lines
Quoted 3 lines
so that we get ocfs2_cluster_lock()(with holders increased) returned sucessfully but actually we don't hold the dlm lock. --thus more than one node is considering that they have the (EX) lock.
You make a good point. I don't like the solution you propose,
though. Another flag that's almost the same? Eww! There's got to be a better way.
I don't see how we can resolve this without another flag. It could be a mirror image of the PENDING flag. PENDING takes care of the gap between BUSY set and calling dlm_lock(). This flag has to take care of the gap between BUSY clear and inc_holders.
Am working on improving Wengang's patch.
Mark is strangely silent.
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
Quoted 15 lines · Sunil Mushran
Quoted 6 lines · Joel Becker
Quoted 5 lines · Wengang Wang
before
update_holders, the lock could be DCed(since no BUSY flag set by here).and even after
update_holders, the lock could be DCed too.Quoted 7 lines
Quoted 3 lines
so that we get ocfs2_cluster_lock()(with holders increased) returned sucessfully but actually we don't hold the dlm lock. --thus more than one node is considering that they have the (EX) lock.
You make a good point. I don't like the solution you propose,
though. Another flag that's almost the same? Eww! There's got to be a better way.
Quoted 6 lines
I don't see how we can resolve this without another flag. It could be a mirror image of the PENDING flag. PENDING takes care of the gap between BUSY set and calling
dlm_lock(). This flag has to take care of the gap between BUSY clear andinc_holders.Am working on improving Wengang's patch.
I don't see anything yet, but PENDING is a complex enough flag
as-is. Another flag and we're even less understandable ;-( If we have to do it, and I assume you'll set the flag in the ast when you clear BUSY, I say call it UPCONVERT_FINISHING.
Oh, wait. This means that we can drop the
lock_attemped/lock_done state variable from wengang's original change. Right before ocfs2_cluster_lock checks for BLOCKING, it can just check flags&UPCONVERT_FINISHING and jump to inc_holders. Maybe this is in wengang's update and I didn't notice.
Quoted 1 line
Mark is strangely silent.
He's probably busy.
Joel
Signature · 10 lines
One look at the From: understanding has blossomed .procmailrc grows - Alexander Viro Joel Becker Principal Software Developer Oracle E-mail: joel.becker at oracle.com Phone: (650) 506-8127
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
Hi,
Quoted 16 lines · Joel Becker
Quoted 15 lines · Sunil Mushran
Quoted 13 lines · Joel Becker
Quoted 8 lines · Wengang Wang
before
update_holders, the lock could be DCed(since no BUSY flag set by here).and even after
update_holders, the lock could be DCed too.so that we get ocfs2_cluster_lock()(with holders increased) returned sucessfully but actually we don't hold the dlm lock. --thus more than one node is considering that they have the (EX) lock.
You make a good point. I don't like the solution you propose,
though. Another flag that's almost the same? Eww! There's got to be a better way.
Quoted 7 lines
Quoted 6 lines
I don't see how we can resolve this without another flag. It could be a mirror image of the PENDING flag. PENDING takes care of the gap between BUSY set and calling
dlm_lock(). This flag has to take care of the gap between BUSY clear andinc_holders.Am working on improving Wengang's patch.
thanks!
Quoted 10 lines
I don't see anything yet, but PENDING is a complex enough flag
as-is. Another flag and we're even less understandable ;-( If we have to do it, and I assume you'll set the flag in the ast when you clear BUSY, I say call itUPCONVERT_FINISHING.
Oh, wait. This means that we can drop the
lock_attemped/lock_done state variable from wengang's original change. Right beforeocfs2_cluster_lockchecks for BLOCKING, it can just check flags&UPCONVERT_FINISHING and jump toinc_holders. Maybe this is in wengang's update and I didn't notice.
yes, we can drop that variable. and I didn't notice it in my update :P
regards,
wengang.
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
In reply to Sunil MushranQuoted 16 lines · Sunil Mushran
Quoted 6 lines · Joel Becker
Quoted 5 lines · Wengang Wang
before
update_holders, the lock could be DCed(since no BUSY flag set by here).and even after
update_holders, the lock could be DCed too.Quoted 8 lines
Quoted 4 lines
so that we get ocfs2_cluster_lock()(with holders increased) returned sucessfully
but actually we don't hold the dlm lock. --thus more than one node is considering that they have the (EX) lock.You make a good point. I don't like the solution you propose,
though. Another flag that's almost the same? Eww! There's got to be a better way.
Quoted 6 lines
I don't see how we can resolve this without another flag. It could be a mirror image of the PENDING flag. PENDING takes care of the gap between BUSY set and calling
dlm_lock(). This flag has to take care of the gap between BUSY clear andinc_holders.Am working on improving Wengang's patch.
I'm afraid I lost track of which patches I should be trying. Is there something that we expect should solve the known problems, even if it's not a final version?
Dave
PATCH 1/1 ocfs2: fix __ocfs2_cluster_lock() dead lock
Yes, we expect it to fix the issue. I'll send the patches after I get to work.
Quoted 34 lines · David Teigland
Quoted 20 lines · Sunil Mushran
Quoted 14 lines · Joel Becker
Quoted 4 lines · Wengang Wang
before
update_holders, the lock could be DCed(since no BUSY flag set by
here).Quoted 8 lines
and even after
update_holders, the lock could be DCed too.so that we get ocfs2_cluster_lock()(with holders increased) returned
sucessfully
but actually we don't hold the dlm lock. --thus more than one node is
considering that they have the (EX) lock.Quoted 4 lines
You make a good point. I don't like the solution you propose,
though. Another flag that's almost the same? Eww! There's got to be a
better way.Quoted 6 lines
I don't see how we can resolve this without another flag. It could be a mirror image of the PENDING flag. PENDING takes care of the gap between BUSY set and calling
dlm_lock(). This flag has to take care of the gap between BUSY clear andinc_holders.Am working on improving Wengang's patch.
I'm afraid I lost track of which patches I should be trying. Is there something that we expect should solve the known problems, even if it's not
a final version?
Dave