* [Ocfs2-devel] [PATCH] ocfs2: fix a potential deadlock in dlm_reset_mleres_owner()
@ 2017-12-18 10:22 alex chen
2017-12-18 11:52 ` Joseph Qi
0 siblings, 1 reply; 4+ messages in thread
From: alex chen @ 2017-12-18 10:22 UTC (permalink / raw)
To: ocfs2-devel
In dlm_reset_mleres_owner(), we will lock
dlm_lock_resource->spinlock after locking dlm_ctxt->master_lock,
which breaks the spinlock lock ordering:
dlm_domain_lock
struct dlm_ctxt->spinlock
struct dlm_lock_resource->spinlock
struct dlm_ctxt->master_lock
Fix it by unlocking dlm_ctxt->master_lock before locking
dlm_lock_resource->spinlock and restarting to clean master list.
Signed-off-by: Alex Chen <alex.chen@huawei.com>
Reviewed-by: Jun Piao <piaojun@huawei.com>
---
fs/ocfs2/dlm/dlmmaster.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c
index 3e04279..0df939a 100644
--- a/fs/ocfs2/dlm/dlmmaster.c
+++ b/fs/ocfs2/dlm/dlmmaster.c
@@ -3287,14 +3287,23 @@ static struct dlm_lock_resource *dlm_reset_mleres_owner(struct dlm_ctxt *dlm,
{
struct dlm_lock_resource *res;
+ assert_spin_locked(&dlm->spinlock);
+ assert_spin_locked(&dlm->master_lock);
+
/* Find the lockres associated to the mle and set its owner to UNK */
- res = __dlm_lookup_lockres(dlm, mle->mname, mle->mnamelen,
+ res = __dlm_lookup_lockres_full(dlm, mle->mname, mle->mnamelen,
mle->mnamehash);
if (res) {
spin_unlock(&dlm->master_lock);
- /* move lockres onto recovery list */
spin_lock(&res->spinlock);
+ if (res->state & DLM_LOCK_RES_DROPPING_REF) {
+ spin_unlock(&res->spinlock);
+ dlm_lockres_put(res);
+ return NULL;
+ }
+
+ /* move lockres onto recovery list */
dlm_set_lockres_owner(dlm, res, DLM_LOCK_RES_OWNER_UNKNOWN);
dlm_move_lockres_to_recovery_list(dlm, res);
spin_unlock(&res->spinlock);
--
1.9.5.msysgit.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: fix a potential deadlock in dlm_reset_mleres_owner()
2017-12-18 10:22 [Ocfs2-devel] [PATCH] ocfs2: fix a potential deadlock in dlm_reset_mleres_owner() alex chen
@ 2017-12-18 11:52 ` Joseph Qi
2017-12-18 12:08 ` Changwei Ge
2017-12-19 3:13 ` alex chen
0 siblings, 2 replies; 4+ messages in thread
From: Joseph Qi @ 2017-12-18 11:52 UTC (permalink / raw)
To: ocfs2-devel
On 17/12/18 18:22, alex chen wrote:
> In dlm_reset_mleres_owner(), we will lock
> dlm_lock_resource->spinlock after locking dlm_ctxt->master_lock,
> which breaks the spinlock lock ordering:
> dlm_domain_lock
> struct dlm_ctxt->spinlock
> struct dlm_lock_resource->spinlock
> struct dlm_ctxt->master_lock
>
> Fix it by unlocking dlm_ctxt->master_lock before locking
> dlm_lock_resource->spinlock and restarting to clean master list.
>
> Signed-off-by: Alex Chen <alex.chen@huawei.com>
> Reviewed-by: Jun Piao <piaojun@huawei.com>
> ---
> fs/ocfs2/dlm/dlmmaster.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c
> index 3e04279..0df939a 100644
> --- a/fs/ocfs2/dlm/dlmmaster.c
> +++ b/fs/ocfs2/dlm/dlmmaster.c
> @@ -3287,14 +3287,23 @@ static struct dlm_lock_resource *dlm_reset_mleres_owner(struct dlm_ctxt *dlm,
> {
> struct dlm_lock_resource *res;
>
> + assert_spin_locked(&dlm->spinlock);
> + assert_spin_locked(&dlm->master_lock);
> +
> /* Find the lockres associated to the mle and set its owner to UNK */
> - res = __dlm_lookup_lockres(dlm, mle->mname, mle->mnamelen,
> + res = __dlm_lookup_lockres_full(dlm, mle->mname, mle->mnamelen,
> mle->mnamehash);
> if (res) {
> spin_unlock(&dlm->master_lock);
>
> - /* move lockres onto recovery list */
> spin_lock(&res->spinlock);
> + if (res->state & DLM_LOCK_RES_DROPPING_REF) {
> + spin_unlock(&res->spinlock);
> + dlm_lockres_put(res);
> + return NULL;
> + }
We can't just return NULL here. Please note that we have to:
return a valid lock resource with master_lock unlocked, or return NULL
with master_lock.
You patch will introduce unlocking master_lock twice.
Thanks,
Joseph
> +
> + /* move lockres onto recovery list */
> dlm_set_lockres_owner(dlm, res, DLM_LOCK_RES_OWNER_UNKNOWN);
> dlm_move_lockres_to_recovery_list(dlm, res);
> spin_unlock(&res->spinlock);
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: fix a potential deadlock in dlm_reset_mleres_owner()
2017-12-18 11:52 ` Joseph Qi
@ 2017-12-18 12:08 ` Changwei Ge
2017-12-19 3:13 ` alex chen
1 sibling, 0 replies; 4+ messages in thread
From: Changwei Ge @ 2017-12-18 12:08 UTC (permalink / raw)
To: ocfs2-devel
On 2017/12/18 19:52, Joseph Qi wrote:
>
>
> On 17/12/18 18:22, alex chen wrote:
>> In dlm_reset_mleres_owner(), we will lock
>> dlm_lock_resource->spinlock after locking dlm_ctxt->master_lock,
>> which breaks the spinlock lock ordering:
>> dlm_domain_lock
>> struct dlm_ctxt->spinlock
>> struct dlm_lock_resource->spinlock
>> struct dlm_ctxt->master_lock
>>
>> Fix it by unlocking dlm_ctxt->master_lock before locking
>> dlm_lock_resource->spinlock and restarting to clean master list.
>>
>> Signed-off-by: Alex Chen <alex.chen@huawei.com>
>> Reviewed-by: Jun Piao <piaojun@huawei.com>
>> ---
>> fs/ocfs2/dlm/dlmmaster.c | 13 +++++++++++--
>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c
>> index 3e04279..0df939a 100644
>> --- a/fs/ocfs2/dlm/dlmmaster.c
>> +++ b/fs/ocfs2/dlm/dlmmaster.c
>> @@ -3287,14 +3287,23 @@ static struct dlm_lock_resource *dlm_reset_mleres_owner(struct dlm_ctxt *dlm,
>> {
>> struct dlm_lock_resource *res;
>>
>> + assert_spin_locked(&dlm->spinlock);
>> + assert_spin_locked(&dlm->master_lock);
>> +
>> /* Find the lockres associated to the mle and set its owner to UNK */
>> - res = __dlm_lookup_lockres(dlm, mle->mname, mle->mnamelen,
>> + res = __dlm_lookup_lockres_full(dlm, mle->mname, mle->mnamelen,
>> mle->mnamehash);
>> if (res) {
>> spin_unlock(&dlm->master_lock);
>>
>> - /* move lockres onto recovery list */
>> spin_lock(&res->spinlock);
>> + if (res->state & DLM_LOCK_RES_DROPPING_REF) {
>> + spin_unlock(&res->spinlock);
>> + dlm_lockres_put(res);
>> + return NULL;
>> + }
>
> We can't just return NULL here. Please note that we have to:
> return a valid lock resource with master_lock unlocked, or return NULL
> with master_lock.
> You patch will introduce unlocking master_lock twice.
Agree.
>
> Thanks,
> Joseph
>
>> +
>> + /* move lockres onto recovery list */
>> dlm_set_lockres_owner(dlm, res, DLM_LOCK_RES_OWNER_UNKNOWN);
>> dlm_move_lockres_to_recovery_list(dlm, res);
>> spin_unlock(&res->spinlock);
>>
>
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: fix a potential deadlock in dlm_reset_mleres_owner()
2017-12-18 11:52 ` Joseph Qi
2017-12-18 12:08 ` Changwei Ge
@ 2017-12-19 3:13 ` alex chen
1 sibling, 0 replies; 4+ messages in thread
From: alex chen @ 2017-12-19 3:13 UTC (permalink / raw)
To: ocfs2-devel
Hi Joseph,
On 2017/12/18 19:52, Joseph Qi wrote:
>
>
> On 17/12/18 18:22, alex chen wrote:
>> In dlm_reset_mleres_owner(), we will lock
>> dlm_lock_resource->spinlock after locking dlm_ctxt->master_lock,
>> which breaks the spinlock lock ordering:
>> dlm_domain_lock
>> struct dlm_ctxt->spinlock
>> struct dlm_lock_resource->spinlock
>> struct dlm_ctxt->master_lock
>>
>> Fix it by unlocking dlm_ctxt->master_lock before locking
>> dlm_lock_resource->spinlock and restarting to clean master list.
>>
>> Signed-off-by: Alex Chen <alex.chen@huawei.com>
>> Reviewed-by: Jun Piao <piaojun@huawei.com>
>> ---
>> fs/ocfs2/dlm/dlmmaster.c | 13 +++++++++++--
>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c
>> index 3e04279..0df939a 100644
>> --- a/fs/ocfs2/dlm/dlmmaster.c
>> +++ b/fs/ocfs2/dlm/dlmmaster.c
>> @@ -3287,14 +3287,23 @@ static struct dlm_lock_resource *dlm_reset_mleres_owner(struct dlm_ctxt *dlm,
>> {
>> struct dlm_lock_resource *res;
>>
>> + assert_spin_locked(&dlm->spinlock);
>> + assert_spin_locked(&dlm->master_lock);
>> +
>> /* Find the lockres associated to the mle and set its owner to UNK */
>> - res = __dlm_lookup_lockres(dlm, mle->mname, mle->mnamelen,
>> + res = __dlm_lookup_lockres_full(dlm, mle->mname, mle->mnamelen,
>> mle->mnamehash);
>> if (res) {
>> spin_unlock(&dlm->master_lock);
>>
>> - /* move lockres onto recovery list */
>> spin_lock(&res->spinlock);
>> + if (res->state & DLM_LOCK_RES_DROPPING_REF) {
>> + spin_unlock(&res->spinlock);
>> + dlm_lockres_put(res);
>> + return NULL;
>> + }
>
> We can't just return NULL here. Please note that we have to:
> return a valid lock resource with master_lock unlocked, or return NULL
> with master_lock.
> You patch will introduce unlocking master_lock twice.
>
OK, my mistake.
I will modify it in the next patch.
Thanks,
Alex
> Thanks,
> Joseph
>
>> +
>> + /* move lockres onto recovery list */
>> dlm_set_lockres_owner(dlm, res, DLM_LOCK_RES_OWNER_UNKNOWN);
>> dlm_move_lockres_to_recovery_list(dlm, res);
>> spin_unlock(&res->spinlock);
>>
>
> .
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-12-19 3:13 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-18 10:22 [Ocfs2-devel] [PATCH] ocfs2: fix a potential deadlock in dlm_reset_mleres_owner() alex chen
2017-12-18 11:52 ` Joseph Qi
2017-12-18 12:08 ` Changwei Ge
2017-12-19 3:13 ` alex chen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.