[Ocfs2-devel] [PATCH] ocfs2/dlm: remove potential deadlock -V3

[Ocfs2-devel] [PATCH] ocfs2/dlm: remove potential deadlock -V3

[Wengang Wang]

When we need to take both dlm_domain_lock and dlm->spinlock, we should take
them in order of: dlm_domain_lock then dlm->spinlock.

There is pathes disobey this order. That is calling dlm_lockres_put() with
dlm->spinlock held in dlm_run_purge_list. dlm_lockres_put() calls dlm_put() at
the ref and dlm_put() locks on dlm_domain_lock.

Fix:
Don't grab/put the dlm when the initialising/releasing lockres.
That grab is not required because we don't call dlm_unregister_domain()
based on refcount.

Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
---
 fs/ocfs2/dlm/dlmmaster.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c
index 94b97fc..8408465 100644
--- a/fs/ocfs2/dlm/dlmmaster.c
+++ b/fs/ocfs2/dlm/dlmmaster.c
@@ -511,8 +511,6 @@ static void dlm_lockres_release(struct kref *kref)
 
 	atomic_dec(&dlm->res_cur_count);
 
-	dlm_put(dlm);
-
 	if (!hlist_unhashed(&res->hash_node) ||
 	    !list_empty(&res->granted) ||
 	    !list_empty(&res->converting) ||
@@ -586,7 +584,6 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,
 	res->inflight_locks = 0;
 
 	/* put in dlm_lockres_release */
-	dlm_grab(dlm);
 	res->dlm = dlm;
 
 	kref_init(&res->refs);
-- 
1.7.1.1

[Ocfs2-devel] [PATCH] ocfs2/dlm: remove potential deadlock -V3

[Sunil Mushran]

You can also remove the comment that talks about the put. 

On Jul 30, 2010, at 6:53 AM, Wengang Wang <wen.gang.wang@oracle.com> wrote:

> When we need to take both dlm_domain_lock and dlm->spinlock, we should take
> them in order of: dlm_domain_lock then dlm->spinlock.
> 
> There is pathes disobey this order. That is calling dlm_lockres_put() with
> dlm->spinlock held in dlm_run_purge_list. dlm_lockres_put() calls dlm_put() at
> the ref and dlm_put() locks on dlm_domain_lock.
> 
> Fix:
> Don't grab/put the dlm when the initialising/releasing lockres.
> That grab is not required because we don't call dlm_unregister_domain()
> based on refcount.
> 
> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> ---
> fs/ocfs2/dlm/dlmmaster.c |    3 ---
> 1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c
> index 94b97fc..8408465 100644
> --- a/fs/ocfs2/dlm/dlmmaster.c
> +++ b/fs/ocfs2/dlm/dlmmaster.c
> @@ -511,8 +511,6 @@ static void dlm_lockres_release(struct kref *kref)
> 
>    atomic_dec(&dlm->res_cur_count);
> 
> -    dlm_put(dlm);
> -
>    if (!hlist_unhashed(&res->hash_node) ||
>        !list_empty(&res->granted) ||
>        !list_empty(&res->converting) ||
> @@ -586,7 +584,6 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,
>    res->inflight_locks = 0;
> 
>    /* put in dlm_lockres_release */
> -    dlm_grab(dlm);
>    res->dlm = dlm;
> 
>    kref_init(&res->refs);
> -- 
> 1.7.1.1
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> http://oss.oracle.com/mailman/listinfo/ocfs2-devel

[Ocfs2-devel] [PATCH] ocfs2/dlm: remove potential deadlock -V3

[Wengang Wang]

On 10-07-30 07:45, Sunil Mushran wrote:
> You can also remove the comment that talks about the put. 

Yes, I should.

regards,
wengang.
> 
> On Jul 30, 2010, at 6:53 AM, Wengang Wang <wen.gang.wang@oracle.com> wrote:
> 
> > When we need to take both dlm_domain_lock and dlm->spinlock, we should take
> > them in order of: dlm_domain_lock then dlm->spinlock.
> > 
> > There is pathes disobey this order. That is calling dlm_lockres_put() with
> > dlm->spinlock held in dlm_run_purge_list. dlm_lockres_put() calls dlm_put() at
> > the ref and dlm_put() locks on dlm_domain_lock.
> > 
> > Fix:
> > Don't grab/put the dlm when the initialising/releasing lockres.
> > That grab is not required because we don't call dlm_unregister_domain()
> > based on refcount.
> > 
> > Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> > ---
> > fs/ocfs2/dlm/dlmmaster.c |    3 ---
> > 1 files changed, 0 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c
> > index 94b97fc..8408465 100644
> > --- a/fs/ocfs2/dlm/dlmmaster.c
> > +++ b/fs/ocfs2/dlm/dlmmaster.c
> > @@ -511,8 +511,6 @@ static void dlm_lockres_release(struct kref *kref)
> > 
> >    atomic_dec(&dlm->res_cur_count);
> > 
> > -    dlm_put(dlm);
> > -
> >    if (!hlist_unhashed(&res->hash_node) ||
> >        !list_empty(&res->granted) ||
> >        !list_empty(&res->converting) ||
> > @@ -586,7 +584,6 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,
> >    res->inflight_locks = 0;
> > 
> >    /* put in dlm_lockres_release */
> > -    dlm_grab(dlm);
> >    res->dlm = dlm;
> > 
> >    kref_init(&res->refs);
> > -- 
> > 1.7.1.1
> > 
> > 
> > _______________________________________________
> > Ocfs2-devel mailing list
> > Ocfs2-devel at oss.oracle.com
> > http://oss.oracle.com/mailman/listinfo/ocfs2-devel

[Ocfs2-devel] [PATCH] ocfs2/dlm: remove potential deadlock -V3

[Wengang Wang]

When we need to take both dlm_domain_lock and dlm->spinlock, we should take
them in order of: dlm_domain_lock then dlm->spinlock.

There is pathes disobey this order. That is calling dlm_lockres_put() with
dlm->spinlock held in dlm_run_purge_list. dlm_lockres_put() calls dlm_put() at
the ref and dlm_put() locks on dlm_domain_lock.

Fix:
Don't grab/put the dlm when the initialising/releasing lockres.
That grab is not required because we don't call dlm_unregister_domain()
based on refcount.

Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
---
 fs/ocfs2/dlm/dlmmaster.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c
index 94b97fc..0fe7c4d 100644
--- a/fs/ocfs2/dlm/dlmmaster.c
+++ b/fs/ocfs2/dlm/dlmmaster.c
@@ -511,8 +511,6 @@ static void dlm_lockres_release(struct kref *kref)
 
 	atomic_dec(&dlm->res_cur_count);
 
-	dlm_put(dlm);
-
 	if (!hlist_unhashed(&res->hash_node) ||
 	    !list_empty(&res->granted) ||
 	    !list_empty(&res->converting) ||
@@ -585,8 +583,6 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,
 	res->migration_pending = 0;
 	res->inflight_locks = 0;
 
-	/* put in dlm_lockres_release */
-	dlm_grab(dlm);
 	res->dlm = dlm;
 
 	kref_init(&res->refs);
-- 
1.7.1.1

[Ocfs2-devel] [PATCH] ocfs2/dlm: remove potential deadlock -V3

[Sunil Mushran]

Acked-by: Sunil Mushran <sunil.mushran@oracle.com>

On 07/30/2010 08:18 AM, Wengang Wang wrote:
> When we need to take both dlm_domain_lock and dlm->spinlock, we should take
> them in order of: dlm_domain_lock then dlm->spinlock.
>
> There is pathes disobey this order. That is calling dlm_lockres_put() with
> dlm->spinlock held in dlm_run_purge_list. dlm_lockres_put() calls dlm_put() at
> the ref and dlm_put() locks on dlm_domain_lock.
>
> Fix:
> Don't grab/put the dlm when the initialising/releasing lockres.
> That grab is not required because we don't call dlm_unregister_domain()
> based on refcount.
>
> Signed-off-by: Wengang Wang<wen.gang.wang@oracle.com>
> ---
>   fs/ocfs2/dlm/dlmmaster.c |    4 ----
>   1 file changed, 4 deletions(-)
>
> diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c
> index 94b97fc..0fe7c4d 100644
> --- a/fs/ocfs2/dlm/dlmmaster.c
> +++ b/fs/ocfs2/dlm/dlmmaster.c
> @@ -511,8 +511,6 @@ static void dlm_lockres_release(struct kref *kref)
>
>   	atomic_dec(&dlm->res_cur_count);
>
> -	dlm_put(dlm);
> -
>   	if (!hlist_unhashed(&res->hash_node) ||
>   	    !list_empty(&res->granted) ||
>   	    !list_empty(&res->converting) ||
> @@ -585,8 +583,6 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,
>   	res->migration_pending = 0;
>   	res->inflight_locks = 0;
>
> -	/* put in dlm_lockres_release */
> -	dlm_grab(dlm);
>   	res->dlm = dlm;
>
>   	kref_init(&res->refs);
>    

[Ocfs2-devel] [PATCH] ocfs2/dlm: remove potential deadlock -V3

[Joel Becker]

On Fri, Jul 30, 2010 at 11:18:00PM +0800, Wengang Wang wrote:
> When we need to take both dlm_domain_lock and dlm->spinlock, we should take
> them in order of: dlm_domain_lock then dlm->spinlock.
> 
> There is pathes disobey this order. That is calling dlm_lockres_put() with
> dlm->spinlock held in dlm_run_purge_list. dlm_lockres_put() calls dlm_put() at
> the ref and dlm_put() locks on dlm_domain_lock.
> 
> Fix:
> Don't grab/put the dlm when the initialising/releasing lockres.
> That grab is not required because we don't call dlm_unregister_domain()
> based on refcount.
> 
> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>

This patch is now in the fixes branch of ocfs2.git.

Joel
-- 

"If the human brain were so simple we could understand it, we would
 be so simple that we could not."
	- W. A. Clouston

Joel Becker
Consulting Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127