ocfs2: fix a potential deadlock in dlm_reset_mleres_owner()
PATCH 1/1 ocfs2: fix a potential deadlock in dlm_reset_mleres_owner()
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.
--- fs/ocfs2/dlm/dlmmaster.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
fs/ocfs2/dlm/dlmmaster.c +11 -2
@@ -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);
Signature · 1 line
1.9.5.msysgit.1
PATCH 1/1 ocfs2: fix a potential deadlock in dlm_reset_mleres_owner()
Quoted 32 lines · alex chen
In
dlm_reset_mleres_owner(), we will lockdlm_lock_resource->spinlockafter lockingdlm_ctxt->master_lock, which breaks the spinlock lock ordering:
dlm_domain_lock
structdlm_ctxt->spinlock
structdlm_lock_resource->spinlock
structdlm_ctxt->master_lockFix it by unlocking
dlm_ctxt->master_lockbefore lockingdlm_lock_resource->spinlockand restarting to clean master list.Signed-off-by Alex Chen <[email protected]>Reviewed-by Jun Piao <[email protected]>--- fs/ocfs2/dlm/dlmmaster.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)fs/ocfs2/dlm/dlmmaster.c +4
-1@@ -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);
fs/ocfs2/dlm/dlmmaster.c static struct dlm_lock_resource *dlm_reset_mleres_owner(struct dlm_ctxt *dlm, · line 3304
Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +5
-1if (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
Quoted 6 lines
fs/ocfs2/dlm/dlmmaster.c +2
-0+ +/* 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);
PATCH 1/1 ocfs2: fix a potential deadlock in dlm_reset_mleres_owner()
Quoted 38 lines · Joseph Qi
Quoted 35 lines · alex chen
In
dlm_reset_mleres_owner(), we will lockdlm_lock_resource->spinlockafter lockingdlm_ctxt->master_lock, which breaks the spinlock lock ordering:
dlm_domain_lock
structdlm_ctxt->spinlock
structdlm_lock_resource->spinlock
structdlm_ctxt->master_lockFix it by unlocking
dlm_ctxt->master_lockbefore lockingdlm_lock_resource->spinlockand restarting to clean master list.Signed-off-by Alex Chen <[email protected]>Reviewed-by Jun Piao <[email protected]>--- fs/ocfs2/dlm/dlmmaster.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)fs/ocfs2/dlm/dlmmaster.c +4
-1@@ -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);
Quoted 12 lines
fs/ocfs2/dlm/dlmmaster.c static struct dlm_lock_resource *dlm_reset_mleres_owner(struct dlm_ctxt *dlm, · line 3304
Quoted 7 lines
fs/ocfs2/dlm/dlmmaster.c +5
-1-/* 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 withmaster_lockunlocked, or return NULL withmaster_lock.
You patch will introduce unlockingmaster_locktwice.
Agree.
Quoted 16 lines
Thanks,
Josephfs/ocfs2/dlm/dlmmaster.c static struct dlm_lock_resource *dlm_reset_mleres_owner(struct dlm_ctxt *dlm, · line 3309
Quoted 6 lines
fs/ocfs2/dlm/dlmmaster.c +2
-0+ +/* 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
PATCH 1/1 ocfs2: fix a potential deadlock in dlm_reset_mleres_owner()
In reply to Joseph QiHi Joseph,
Quoted 38 lines · Joseph Qi
Quoted 35 lines · alex chen
In
dlm_reset_mleres_owner(), we will lockdlm_lock_resource->spinlockafter lockingdlm_ctxt->master_lock, which breaks the spinlock lock ordering:
dlm_domain_lock
structdlm_ctxt->spinlock
structdlm_lock_resource->spinlock
structdlm_ctxt->master_lockFix it by unlocking
dlm_ctxt->master_lockbefore lockingdlm_lock_resource->spinlockand restarting to clean master list.Signed-off-by Alex Chen <[email protected]>Reviewed-by Jun Piao <[email protected]>--- fs/ocfs2/dlm/dlmmaster.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)fs/ocfs2/dlm/dlmmaster.c +4
-1@@ -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);
Quoted 13 lines
fs/ocfs2/dlm/dlmmaster.c static struct dlm_lock_resource *dlm_reset_mleres_owner(struct dlm_ctxt *dlm, · line 3304
Quoted 7 lines
fs/ocfs2/dlm/dlmmaster.c +5
-1-/* 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 withmaster_lockunlocked, or return NULL withmaster_lock.
You patch will introduce unlockingmaster_locktwice.
OK, my mistake.
I will modify it in the next patch.
Thanks,
Alex
Quoted 12 lines
Thanks,
Josephfs/ocfs2/dlm/dlmmaster.c static struct dlm_lock_resource *dlm_reset_mleres_owner(struct dlm_ctxt *dlm, · line 3309
Quoted 6 lines
fs/ocfs2/dlm/dlmmaster.c +2
-0+ +/* 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);.