On 04/29/2015 08:51 AM, Joseph Qi wrote:
Hi Andrew,
On 2015/4/29 4:32, Andrew Morton wrote:
On Sat, 25 Apr 2015 15:05:15 +0800 Joseph Qi <joseph.qi@xxxxxxxxxx> 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_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.
...
--- a/fs/ocfs2/dlm/dlmthread.c
+++ b/fs/ocfs2/dlm/dlmthread.c
@@ -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.
+ 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);
+}
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?
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.
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.
.
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@xxxxxxxxxxxxxx
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html