ocfs2: fix a potential deadlock in dlm_reset_mleres_owner()
PATCH v2 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 | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
fs/ocfs2/dlm/dlmmaster.c +10 -4
@@ -3287,16 +3287,22 @@ 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); -dlm_set_lockres_owner (dlm, res,DLM_LOCK_RES_OWNER_UNKNOWN ); -dlm_move_lockres_to_recovery_list (dlm, res); +if (!(res->state &DLM_LOCK_RES_DROPPING_REF )) { +/* 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);dlm_lockres_put (res);
Signature · 1 line
1.9.5.msysgit.1
PATCH v2 1/1 ocfs2: fix a potential deadlock in dlm_reset_mleres_owner()
Hi Alex,
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 | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)fs/ocfs2/dlm/dlmmaster.c +4
-1@@ -3287,16 +3287,22 @@ 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);
fs/ocfs2/dlm/dlmmaster.c static struct dlm_lock_resource *dlm_reset_mleres_owner(struct dlm_ctxt *dlm, · line 3305
Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +6
-3-/* move lockres onto recovery list */ spin_lock (&res->spinlock); -dlm_set_lockres_owner (dlm, res,DLM_LOCK_RES_OWNER_UNKNOWN ); -dlm_move_lockres_to_recovery_list (dlm, res); +if (!(res->state &DLM_LOCK_RES_DROPPING_REF )) { +/* move lockres onto recovery list */ +dlm_set_lockres_owner (dlm, res,DLM_LOCK_RES_OWNER_UNKNOWN ); +dlm_move_lockres_to_recovery_list (dlm, res); + } +
I don't think this change is lock re-ordering *only*. It definitely changes the logic of resetting mle resource owner.
Why do you detach mle from heartbeat if lock resource is in the process of dropping its mastery reference? And why we have to restart in this case?
Thanks,
Joseph
Quoted 3 lines
fs/ocfs2/dlm/dlmmaster.c +0
-0spin_unlock (&res->spinlock);dlm_lockres_put (res);
PATCH v2 1/1 ocfs2: fix a potential deadlock in dlm_reset_mleres_owner()
Hi Joseph,
Quoted 38 lines · Joseph Qi
Hi Alex,
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 | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)fs/ocfs2/dlm/dlmmaster.c +4
-1@@ -3287,16 +3287,22 @@ 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 15 lines
fs/ocfs2/dlm/dlmmaster.c static struct dlm_lock_resource *dlm_reset_mleres_owner(struct dlm_ctxt *dlm, · line 3305
Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +6
-3-/* move lockres onto recovery list */ spin_lock (&res->spinlock); -dlm_set_lockres_owner (dlm, res,DLM_LOCK_RES_OWNER_UNKNOWN ); -dlm_move_lockres_to_recovery_list (dlm, res); +if (!(res->state &DLM_LOCK_RES_DROPPING_REF )) { +/* move lockres onto recovery list */ +dlm_set_lockres_owner (dlm, res,DLM_LOCK_RES_OWNER_UNKNOWN ); +dlm_move_lockres_to_recovery_list (dlm, res); + } +I don't think this change is lock re-ordering *only*. It definitely changes the logic of resetting mle resource owner.
Why do you detach mle from heartbeat if lock resource is in the process of dropping its mastery reference? And why we have to restart in this case?
I think if the lock resource is being purge we don't need to set its owner to UNKNOWN and it is the same as the original logic. We should drop the master lock if we want to judge if the state of the lock resource is DLM_LOCK_RES_DROPPING_REF. Once we drop the master lock we should restart to clean master list.
Here the mle is not useful and will be released, so we detach it from heartbeat. In fact, the mle has been detached in dlm_clean_migration_mle().
Thanks,
Alex
Quoted 10 lines
Thanks,
Josephfs/ocfs2/dlm/dlmmaster.c static struct dlm_lock_resource *dlm_reset_mleres_owner(struct dlm_ctxt *dlm, · line 3308
Quoted 3 lines
fs/ocfs2/dlm/dlmmaster.c +0
-0spin_unlock (&res->spinlock);dlm_lockres_put (res);.
PATCH v2 1/1 ocfs2: fix a potential deadlock in dlm_reset_mleres_owner()
Quoted 56 lines · alex chen
Hi Joseph,
Quoted 38 lines · Joseph Qi
Hi Alex,
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 | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)fs/ocfs2/dlm/dlmmaster.c +4
-1@@ -3287,16 +3287,22 @@ 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 15 lines
fs/ocfs2/dlm/dlmmaster.c static struct dlm_lock_resource *dlm_reset_mleres_owner(struct dlm_ctxt *dlm, · line 3305
Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +6
-3-/* move lockres onto recovery list */ spin_lock (&res->spinlock); -dlm_set_lockres_owner (dlm, res,DLM_LOCK_RES_OWNER_UNKNOWN ); -dlm_move_lockres_to_recovery_list (dlm, res); +if (!(res->state &DLM_LOCK_RES_DROPPING_REF )) { +/* move lockres onto recovery list */ +dlm_set_lockres_owner (dlm, res,DLM_LOCK_RES_OWNER_UNKNOWN ); +dlm_move_lockres_to_recovery_list (dlm, res); + } +I don't think this change is lock re-ordering *only*. It definitely changes the logic of resetting mle resource owner.
Why do you detach mle from heartbeat if lock resource is in the process of dropping its mastery reference? And why we have to restart in this case?
Quoted 6 lines
I think if the lock resource is being purge we don't need to set its owner to UNKNOWN and it is the same as the original logic. We should drop the master lock if we want to judge if the state of the lock resource is
DLM_LOCK_RES_DROPPING_REF. Once we drop the master lock we should restart to clean master list.
Here the mle is not useful and will be released, so we detach it from heartbeat. In fact, the mle has been detached indlm_clean_migration_mle().
Hi Alex,
Perhaps, you can just judge if lock resource is marked DLM_LOCK_RES_DROPPING_REF and if so directly return NULL with ::master_lock locked :)
Thanks,
Changwei
Quoted 20 lines
Thanks,
AlexQuoted 10 lines
Thanks,
Josephfs/ocfs2/dlm/dlmmaster.c static struct dlm_lock_resource *dlm_reset_mleres_owner(struct dlm_ctxt *dlm, · line 3308
Quoted 3 lines
fs/ocfs2/dlm/dlmmaster.c +0
-0spin_unlock (&res->spinlock);dlm_lockres_put (res);.
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
PATCH v2 1/1 ocfs2: fix a potential deadlock in dlm_reset_mleres_owner()
Quoted 63 lines · Changwei Ge
Quoted 56 lines · alex chen
Hi Joseph,
Quoted 38 lines · Joseph Qi
Hi Alex,
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 | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)fs/ocfs2/dlm/dlmmaster.c +4
-1@@ -3287,16 +3287,22 @@ 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 15 lines
fs/ocfs2/dlm/dlmmaster.c static struct dlm_lock_resource *dlm_reset_mleres_owner(struct dlm_ctxt *dlm, · line 3305
Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +6
-3-/* move lockres onto recovery list */ spin_lock (&res->spinlock); -dlm_set_lockres_owner (dlm, res,DLM_LOCK_RES_OWNER_UNKNOWN ); -dlm_move_lockres_to_recovery_list (dlm, res); +if (!(res->state &DLM_LOCK_RES_DROPPING_REF )) { +/* move lockres onto recovery list */ +dlm_set_lockres_owner (dlm, res,DLM_LOCK_RES_OWNER_UNKNOWN ); +dlm_move_lockres_to_recovery_list (dlm, res); + } +I don't think this change is lock re-ordering *only*. It definitely changes the logic of resetting mle resource owner.
Why do you detach mle from heartbeat if lock resource is in the process of dropping its mastery reference? And why we have to restart in this case?Quoted 6 lines
I think if the lock resource is being purge we don't need to set its owner to UNKNOWN and it is the same as the original logic. We should drop the master lock if we want to judge if the state of the lock resource is
DLM_LOCK_RES_DROPPING_REF. Once we drop the master lock we should restart to clean master list.
Here the mle is not useful and will be released, so we detach it from heartbeat. In fact, the mle has been detached indlm_clean_migration_mle().
Quoted 5 lines
Hi Alex,
Perhaps, you can just judge if lock resource is marked
DLM_LOCK_RES_DROPPING_REFand if so directly return NULL with ::master_lock locked :)
Umm... We can't do this with unlocking master lock first and then
re-taking it. It breaks the logic.
What my concern is the behavior change. e.g. Currently for lock resource in the process of dropping mastery reference, we just ignore it and continue with the next. But after this patch, we have to restart at the beginning.
Quoted 24 lines
Thanks,
ChangweiQuoted 20 lines
Thanks,
AlexQuoted 10 lines
Thanks,
Josephfs/ocfs2/dlm/dlmmaster.c static struct dlm_lock_resource *dlm_reset_mleres_owner(struct dlm_ctxt *dlm, · line 3308
Quoted 3 lines
fs/ocfs2/dlm/dlmmaster.c +0
-0spin_unlock (&res->spinlock);dlm_lockres_put (res);.
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
PATCH v2 1/1 ocfs2: fix a potential deadlock in dlm_reset_mleres_owner()
Hi Joseph and Changwei,
Quoted 68 lines · Joseph Qi
Quoted 65 lines · Changwei Ge
Quoted 56 lines · alex chen
Hi Joseph,
Quoted 38 lines · Joseph Qi
Hi Alex,
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 | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)fs/ocfs2/dlm/dlmmaster.c +4
-1@@ -3287,16 +3287,22 @@ 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 15 lines
fs/ocfs2/dlm/dlmmaster.c static struct dlm_lock_resource *dlm_reset_mleres_owner(struct dlm_ctxt *dlm, · line 3305
Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +6
-3-/* move lockres onto recovery list */ spin_lock (&res->spinlock); -dlm_set_lockres_owner (dlm, res,DLM_LOCK_RES_OWNER_UNKNOWN ); -dlm_move_lockres_to_recovery_list (dlm, res); +if (!(res->state &DLM_LOCK_RES_DROPPING_REF )) { +/* move lockres onto recovery list */ +dlm_set_lockres_owner (dlm, res,DLM_LOCK_RES_OWNER_UNKNOWN ); +dlm_move_lockres_to_recovery_list (dlm, res); + } +I don't think this change is lock re-ordering *only*. It definitely changes the logic of resetting mle resource owner.
Why do you detach mle from heartbeat if lock resource is in the process of dropping its mastery reference? And why we have to restart in this case?Quoted 6 lines
I think if the lock resource is being purge we don't need to set its owner to UNKNOWN and it is the same as the original logic. We should drop the master lock if we want to judge if the state of the lock resource is
DLM_LOCK_RES_DROPPING_REF. Once we drop the master lock we should restart to clean master list.
Here the mle is not useful and will be released, so we detach it from heartbeat. In fact, the mle has been detached indlm_clean_migration_mle().Hi Alex,
Quoted 8 lines
Quoted 3 lines
Perhaps, you can just judge if lock resource is marked
DLM_LOCK_RES_DROPPING_REFand if so directly return NULL with ::master_lock locked :)
Umm... We can't do this with unlocking master lock first and thenre-taking it. It breaks the logic.
What my concern is the behavior change. e.g. Currently for lock resource in the process of dropping mastery reference, we just ignore it and continue with the next. But after this patch, we have to restart at the beginning.
Before this patch, we can continue when we find the lock resource is marked DLM_LOCK_RES_DROPPING_REF, that is because we use a wrong spin lock ordering.
Once we drop the master lock we should restart to iterate over the master list, otherwise, we may miss some mles which are inserted during we are dropping the master lock.
In this patch, we may increase the times to restart, but we can not avoid it for solving the deadlock.
Thanks,
Alex
Quoted 28 lines
Quoted 25 lines
Thanks,
ChangweiQuoted 20 lines
Thanks,
AlexQuoted 10 lines
Thanks,
Josephfs/ocfs2/dlm/dlmmaster.c static struct dlm_lock_resource *dlm_reset_mleres_owner(struct dlm_ctxt *dlm, · line 3308
Quoted 3 lines
fs/ocfs2/dlm/dlmmaster.c +0
-0spin_unlock (&res->spinlock);dlm_lockres_put (res);.
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel.