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...@huawei.com> 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@oss.oracle.com > https://oss.oracle.com/mailman/listinfo/ocfs2-devel _______________________________________________ Ocfs2-devel mailing list Ocfs2-devel@oss.oracle.com https://oss.oracle.com/mailman/listinfo/ocfs2-devel