[Ocfs2-devel] [PATCH] ocfs2/dlm: avoid incorrect bit set in refmap on recovery master

[Ocfs2-devel] [PATCH] ocfs2/dlm: avoid incorrect bit set in refmap on recovery master

[Wengang Wang]

In the following situation, there remains an incorrect bit in refmap on the
recovery master. Finally the recovery master will fail at purging the lockres
due to the incorrect bit in refmap.

1) node A has no interest on lockres A any longer, so it is purging it.
2) the owner of lockres A is node B, so node A is sending de-ref message
to node B.
3) at this time, node B crashed. node C becomes the recovery master. it recovers
lockres A(because the master is the dead node B).
4) node A migrated lockres A to node C with a refbit there.
5) node A failed to send de-ref message to node B because it crashed. The failure
is ignored. no other action is done for lockres A any more.

For mormal, re-send the deref message to it to recovery master can fix it. Well,
ignoring the failure of deref to the original master and not recovering the lockres
to recovery master has the same effect. And the later is simpler.

Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
---
 fs/ocfs2/dlm/dlmrecovery.c |   17 +++++++++++++++--
 fs/ocfs2/dlm/dlmthread.c   |   28 +++++++++++++++++-----------
 2 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/fs/ocfs2/dlm/dlmrecovery.c b/fs/ocfs2/dlm/dlmrecovery.c
index 9dfaac7..06640f6 100644
--- a/fs/ocfs2/dlm/dlmrecovery.c
+++ b/fs/ocfs2/dlm/dlmrecovery.c
@@ -1997,6 +1997,8 @@ void dlm_move_lockres_to_recovery_list(struct dlm_ctxt *dlm,
 	struct list_head *queue;
 	struct dlm_lock *lock, *next;
 
+	assert_spin_locked(&dlm->spinlock);
+	assert_spin_locked(&res->spinlock);
 	res->state |= DLM_LOCK_RES_RECOVERING;
 	if (!list_empty(&res->recovering)) {
 		mlog(0,
@@ -2336,9 +2338,20 @@ static void dlm_do_local_recovery_cleanup(struct dlm_ctxt *dlm, u8 dead_node)
 
 				/* the wake_up for this will happen when the
 				 * RECOVERING flag is dropped later */
-				res->state &= ~DLM_LOCK_RES_DROPPING_REF;
+				if (res->state & DLM_LOCK_RES_DROPPING_REF) {
+					/*
+					 * don't migrate a lockres which is in
+					 * progress of dropping ref
+					 */
+					mlog(ML_NOTICE, "%.*s ignored for "
+					     "migration\n", res->lockname.len,
+					     res->lockname.name);
+					res->state &=
+						~DLM_LOCK_RES_DROPPING_REF;
+				} else
+					dlm_move_lockres_to_recovery_list(dlm,
+									  res);
 
-				dlm_move_lockres_to_recovery_list(dlm, res);
 			} else if (res->owner == dlm->node_num) {
 				dlm_free_dead_locks(dlm, res, dead_node);
 				__dlm_lockres_calc_usage(dlm, res);
diff --git a/fs/ocfs2/dlm/dlmthread.c b/fs/ocfs2/dlm/dlmthread.c
index dd78ca3..47420ce 100644
--- a/fs/ocfs2/dlm/dlmthread.c
+++ b/fs/ocfs2/dlm/dlmthread.c
@@ -92,17 +92,23 @@ int __dlm_lockres_has_locks(struct dlm_lock_resource *res)
  * truly ready to be freed. */
 int __dlm_lockres_unused(struct dlm_lock_resource *res)
 {
-	if (!__dlm_lockres_has_locks(res) &&
-	    (list_empty(&res->dirty) && !(res->state & DLM_LOCK_RES_DIRTY))) {
-		/* try not to scan the bitmap unless the first two
-		 * conditions are already true */
-		int bit = find_next_bit(res->refmap, O2NM_MAX_NODES, 0);
-		if (bit >= O2NM_MAX_NODES) {
-			/* since the bit for dlm->node_num is not
-			 * set, inflight_locks better be zero */
-			BUG_ON(res->inflight_locks != 0);
-			return 1;
-		}
+	int bit;
+
+	if (__dlm_lockres_has_locks(res))
+		return 0;
+
+	if (!list_empty(&res->dirty) || res->state & DLM_LOCK_RES_DIRTY)
+		return 0;
+
+	if (res->state & DLM_LOCK_RES_RECOVERING)
+		return 0;
+
+	bit = find_next_bit(res->refmap, O2NM_MAX_NODES, 0);
+	if (bit >= O2NM_MAX_NODES) {
+		/* since the bit for dlm->node_num is not
+		 * set, inflight_locks better be zero */
+		BUG_ON(res->inflight_locks != 0);
+		return 1;
 	}
 	return 0;
 }
-- 
1.7.1.1

[Ocfs2-devel] [PATCH] ocfs2/dlm: avoid incorrect bit set in refmap on recovery master

[Srinivas Eeda]

thanks for making this patch, it looks good just few minor changes about 
comments

On 7/23/2010 5:15 AM, Wengang Wang wrote:
> In the following situation, there remains an incorrect bit in refmap on the
> recovery master. Finally the recovery master will fail at purging the lockres
> due to the incorrect bit in refmap.
>
> 1) node A has no interest on lockres A any longer, so it is purging it.
> 2) the owner of lockres A is node B, so node A is sending de-ref message
> to node B.
> 3) at this time, node B crashed. node C becomes the recovery master. it recovers
> lockres A(because the master is the dead node B).
> 4) node A migrated lockres A to node C with a refbit there.
> 5) node A failed to send de-ref message to node B because it crashed. The failure
> is ignored. no other action is done for lockres A any more.
>
> For mormal, re-send the deref message to it to recovery master can fix it. Well,
> ignoring the failure of deref to the original master and not recovering the lockres
> to recovery master has the same effect. And the later is simpler.
>
> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> ---
>  fs/ocfs2/dlm/dlmrecovery.c |   17 +++++++++++++++--
>  fs/ocfs2/dlm/dlmthread.c   |   28 +++++++++++++++++-----------
>  2 files changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/fs/ocfs2/dlm/dlmrecovery.c b/fs/ocfs2/dlm/dlmrecovery.c
> index 9dfaac7..06640f6 100644
> --- a/fs/ocfs2/dlm/dlmrecovery.c
> +++ b/fs/ocfs2/dlm/dlmrecovery.c
> @@ -1997,6 +1997,8 @@ void dlm_move_lockres_to_recovery_list(struct dlm_ctxt *dlm,
>  	struct list_head *queue;
>  	struct dlm_lock *lock, *next;
>  
> +	assert_spin_locked(&dlm->spinlock);
> +	assert_spin_locked(&res->spinlock);
>  	res->state |= DLM_LOCK_RES_RECOVERING;
>  	if (!list_empty(&res->recovering)) {
>  		mlog(0,
> @@ -2336,9 +2338,20 @@ static void dlm_do_local_recovery_cleanup(struct dlm_ctxt *dlm, u8 dead_node)
>  
>  				/* the wake_up for this will happen when the
>  				 * RECOVERING flag is dropped later */
>   
remove above comment as it doesn't seem to be relevant anymore.
> -				res->state &= ~DLM_LOCK_RES_DROPPING_REF;
> +				if (res->state & DLM_LOCK_RES_DROPPING_REF) {
> +					/*
> +					 * don't migrate a lockres which is in
> +					 * progress of dropping ref
> +					 */
>   
move this comment to before the if condition
> +					mlog(ML_NOTICE, "%.*s ignored for "
> +					     "migration\n", res->lockname.len,
> +					     res->lockname.name);
>   
This information only helps us in diagnosing any related issue and is 
not helpful in normal cases. So it should be 0 instead of ML_NOTICE.
> +					res->state &=
> +						~DLM_LOCK_RES_DROPPING_REF;
>   
we don't need to clear this state as dlm_purge_lockres removes it.
> +				} else
> +					dlm_move_lockres_to_recovery_list(dlm,
> +									  res);
>  
> -				dlm_move_lockres_to_recovery_list(dlm, res);
>  			} else if (res->owner == dlm->node_num) {
>  				dlm_free_dead_locks(dlm, res, dead_node);
>  				__dlm_lockres_calc_usage(dlm, res);
> diff --git a/fs/ocfs2/dlm/dlmthread.c b/fs/ocfs2/dlm/dlmthread.c
> index dd78ca3..47420ce 100644
> --- a/fs/ocfs2/dlm/dlmthread.c
> +++ b/fs/ocfs2/dlm/dlmthread.c
> @@ -92,17 +92,23 @@ int __dlm_lockres_has_locks(struct dlm_lock_resource *res)
>   * truly ready to be freed. */
>  int __dlm_lockres_unused(struct dlm_lock_resource *res)
>  {
> -	if (!__dlm_lockres_has_locks(res) &&
> -	    (list_empty(&res->dirty) && !(res->state & DLM_LOCK_RES_DIRTY))) {
> -		/* try not to scan the bitmap unless the first two
> -		 * conditions are already true */
> -		int bit = find_next_bit(res->refmap, O2NM_MAX_NODES, 0);
> -		if (bit >= O2NM_MAX_NODES) {
> -			/* since the bit for dlm->node_num is not
> -			 * set, inflight_locks better be zero */
> -			BUG_ON(res->inflight_locks != 0);
> -			return 1;
> -		}
> +	int bit;
> +
> +	if (__dlm_lockres_has_locks(res))
> +		return 0;
> +
> +	if (!list_empty(&res->dirty) || res->state & DLM_LOCK_RES_DIRTY)
> +		return 0;
> +
> +	if (res->state & DLM_LOCK_RES_RECOVERING)
> +		return 0;
> +
> +	bit = find_next_bit(res->refmap, O2NM_MAX_NODES, 0);
> +	if (bit >= O2NM_MAX_NODES) {
> +		/* since the bit for dlm->node_num is not
> +		 * set, inflight_locks better be zero */
> +		BUG_ON(res->inflight_locks != 0);
> +		return 1;
>  	}
>  	return 0;
>  }
>   

[Ocfs2-devel] [PATCH] ocfs2/dlm: avoid incorrect bit set in refmap on recovery master

[Wengang Wang]

Hi Srini,

Thanks for your review!

On 10-07-23 15:27, Srinivas Eeda wrote:
> thanks for making this patch, it looks good just few minor changes
> about comments
> 
> On 7/23/2010 5:15 AM, Wengang Wang wrote:
> >In the following situation, there remains an incorrect bit in refmap on the
> >recovery master. Finally the recovery master will fail at purging the lockres
> >due to the incorrect bit in refmap.
> >
> >1) node A has no interest on lockres A any longer, so it is purging it.
> >2) the owner of lockres A is node B, so node A is sending de-ref message
> >to node B.
> >3) at this time, node B crashed. node C becomes the recovery master. it recovers
> >lockres A(because the master is the dead node B).
> >4) node A migrated lockres A to node C with a refbit there.
> >5) node A failed to send de-ref message to node B because it crashed. The failure
> >is ignored. no other action is done for lockres A any more.
> >
> >For mormal, re-send the deref message to it to recovery master can fix it. Well,
> >ignoring the failure of deref to the original master and not recovering the lockres
> >to recovery master has the same effect. And the later is simpler.
> >
> >Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> >---
> > fs/ocfs2/dlm/dlmrecovery.c |   17 +++++++++++++++--
> > fs/ocfs2/dlm/dlmthread.c   |   28 +++++++++++++++++-----------
> > 2 files changed, 32 insertions(+), 13 deletions(-)
> >
> >diff --git a/fs/ocfs2/dlm/dlmrecovery.c b/fs/ocfs2/dlm/dlmrecovery.c
> >index 9dfaac7..06640f6 100644
> >--- a/fs/ocfs2/dlm/dlmrecovery.c
> >+++ b/fs/ocfs2/dlm/dlmrecovery.c
> >@@ -1997,6 +1997,8 @@ void dlm_move_lockres_to_recovery_list(struct dlm_ctxt *dlm,
> > 	struct list_head *queue;
> > 	struct dlm_lock *lock, *next;
> >+	assert_spin_locked(&dlm->spinlock);
> >+	assert_spin_locked(&res->spinlock);
> > 	res->state |= DLM_LOCK_RES_RECOVERING;
> > 	if (!list_empty(&res->recovering)) {
> > 		mlog(0,
> >@@ -2336,9 +2338,20 @@ static void dlm_do_local_recovery_cleanup(struct dlm_ctxt *dlm, u8 dead_node)
> > 				/* the wake_up for this will happen when the
> > 				 * RECOVERING flag is dropped later */
> remove above comment as it doesn't seem to be relevant anymore.

OK.
> >-				res->state &= ~DLM_LOCK_RES_DROPPING_REF;
> >+				if (res->state & DLM_LOCK_RES_DROPPING_REF) {
> >+					/*
> >+					 * don't migrate a lockres which is in
> >+					 * progress of dropping ref
> >+					 */
> move this comment to before the if condition

OK.
> >+					mlog(ML_NOTICE, "%.*s ignored for "
> >+					     "migration\n", res->lockname.len,
> >+					     res->lockname.name);
> This information only helps us in diagnosing any related issue and
> is not helpful in normal cases. So it should be 0 instead of
> ML_NOTICE.

I think this happens in a rare case. So it won't write many such logs but
it could help in diagnosing in case something wroing take place.
mlog(0, ..) almost help nothing.

> >+					res->state &=
> >+						~DLM_LOCK_RES_DROPPING_REF;
> we don't need to clear this state as dlm_purge_lockres removes it.

OK.

regards,
wengang.
> >+				} else
> >+					dlm_move_lockres_to_recovery_list(dlm,
> >+									  res);
> >-				dlm_move_lockres_to_recovery_list(dlm, res);
> > 			} else if (res->owner == dlm->node_num) {
> > 				dlm_free_dead_locks(dlm, res, dead_node);
> > 				__dlm_lockres_calc_usage(dlm, res);

[Ocfs2-devel] [PATCH] ocfs2/dlm: avoid incorrect bit set in refmap on recovery master

[Wengang Wang]

In the following situation, there remains an incorrect bit in refmap on the
recovery master. Finally the recovery master will fail at purging the lockres
due to the incorrect bit in refmap.

1) node A has no interest on lockres A any longer, so it is purging it.
2) the owner of lockres A is node B, so node A is sending de-ref message
to node B.
3) at this time, node B crashed. node C becomes the recovery master. it recovers
lockres A(because the master is the dead node B).
4) node A migrated lockres A to node C with a refbit there.
5) node A failed to send de-ref message to node B because it crashed. The failure
is ignored. no other action is done for lockres A any more.

For mormal, re-send the deref message to it to recovery master can fix it. Well,
ignoring the failure of deref to the original master and not recovering the lockres
to recovery master has the same effect. And the later is simpler.

Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
---
 fs/ocfs2/dlm/dlmrecovery.c |   17 +++++++++++++----
 fs/ocfs2/dlm/dlmthread.c   |   28 +++++++++++++++++-----------
 2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/fs/ocfs2/dlm/dlmrecovery.c b/fs/ocfs2/dlm/dlmrecovery.c
index 9dfaac7..2b57cc4 100644
--- a/fs/ocfs2/dlm/dlmrecovery.c
+++ b/fs/ocfs2/dlm/dlmrecovery.c
@@ -1997,6 +1997,8 @@ void dlm_move_lockres_to_recovery_list(struct dlm_ctxt *dlm,
 	struct list_head *queue;
 	struct dlm_lock *lock, *next;
 
+	assert_spin_locked(&dlm->spinlock);
+	assert_spin_locked(&res->spinlock);
 	res->state |= DLM_LOCK_RES_RECOVERING;
 	if (!list_empty(&res->recovering)) {
 		mlog(0,
@@ -2334,11 +2336,18 @@ static void dlm_do_local_recovery_cleanup(struct dlm_ctxt *dlm, u8 dead_node)
 					     dlm->name, res->lockname.len,
 					     res->lockname.name, dead_node);
 
-				/* the wake_up for this will happen when the
-				 * RECOVERING flag is dropped later */
-				res->state &= ~DLM_LOCK_RES_DROPPING_REF;
+				/*
+				 * don't migrate a lockres which is in progress
+				 * of dropping ref
+				 */
+				if (res->state & DLM_LOCK_RES_DROPPING_REF) {
+					mlog(ML_NOTICE, "%.*s ignored for "
+					     "migration\n", res->lockname.len,
+					     res->lockname.name);
+				} else
+					dlm_move_lockres_to_recovery_list(dlm,
+									  res);
 
-				dlm_move_lockres_to_recovery_list(dlm, res);
 			} else if (res->owner == dlm->node_num) {
 				dlm_free_dead_locks(dlm, res, dead_node);
 				__dlm_lockres_calc_usage(dlm, res);
diff --git a/fs/ocfs2/dlm/dlmthread.c b/fs/ocfs2/dlm/dlmthread.c
index dd78ca3..47420ce 100644
--- a/fs/ocfs2/dlm/dlmthread.c
+++ b/fs/ocfs2/dlm/dlmthread.c
@@ -92,17 +92,23 @@ int __dlm_lockres_has_locks(struct dlm_lock_resource *res)
  * truly ready to be freed. */
 int __dlm_lockres_unused(struct dlm_lock_resource *res)
 {
-	if (!__dlm_lockres_has_locks(res) &&
-	    (list_empty(&res->dirty) && !(res->state & DLM_LOCK_RES_DIRTY))) {
-		/* try not to scan the bitmap unless the first two
-		 * conditions are already true */
-		int bit = find_next_bit(res->refmap, O2NM_MAX_NODES, 0);
-		if (bit >= O2NM_MAX_NODES) {
-			/* since the bit for dlm->node_num is not
-			 * set, inflight_locks better be zero */
-			BUG_ON(res->inflight_locks != 0);
-			return 1;
-		}
+	int bit;
+
+	if (__dlm_lockres_has_locks(res))
+		return 0;
+
+	if (!list_empty(&res->dirty) || res->state & DLM_LOCK_RES_DIRTY)
+		return 0;
+
+	if (res->state & DLM_LOCK_RES_RECOVERING)
+		return 0;
+
+	bit = find_next_bit(res->refmap, O2NM_MAX_NODES, 0);
+	if (bit >= O2NM_MAX_NODES) {
+		/* since the bit for dlm->node_num is not
+		 * set, inflight_locks better be zero */
+		BUG_ON(res->inflight_locks != 0);
+		return 1;
 	}
 	return 0;
 }
-- 
1.7.1.1

[Ocfs2-devel] [PATCH] ocfs2/dlm: avoid incorrect bit set in refmap on recovery master

[Srinivas Eeda]

Acked-by: Srinivas Eeda <srinivas.eeda@oracle.com>

Wengang Wang wrote:

> In the following situation, there remains an incorrect bit in refmap on the
> recovery master. Finally the recovery master will fail at purging the lockres
> due to the incorrect bit in refmap.
>
> 1) node A has no interest on lockres A any longer, so it is purging it.
> 2) the owner of lockres A is node B, so node A is sending de-ref message
> to node B.
> 3) at this time, node B crashed. node C becomes the recovery master. it recovers
> lockres A(because the master is the dead node B).
> 4) node A migrated lockres A to node C with a refbit there.
> 5) node A failed to send de-ref message to node B because it crashed. The failure
> is ignored. no other action is done for lockres A any more.
>
> For mormal, re-send the deref message to it to recovery master can fix it. Well,
> ignoring the failure of deref to the original master and not recovering the lockres
> to recovery master has the same effect. And the later is simpler.
>
> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> ---
>  fs/ocfs2/dlm/dlmrecovery.c |   17 +++++++++++++----
>  fs/ocfs2/dlm/dlmthread.c   |   28 +++++++++++++++++-----------
>  2 files changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/fs/ocfs2/dlm/dlmrecovery.c b/fs/ocfs2/dlm/dlmrecovery.c
> index 9dfaac7..2b57cc4 100644
> --- a/fs/ocfs2/dlm/dlmrecovery.c
> +++ b/fs/ocfs2/dlm/dlmrecovery.c
> @@ -1997,6 +1997,8 @@ void dlm_move_lockres_to_recovery_list(struct dlm_ctxt *dlm,
>  	struct list_head *queue;
>  	struct dlm_lock *lock, *next;
>  
> +	assert_spin_locked(&dlm->spinlock);
> +	assert_spin_locked(&res->spinlock);
>  	res->state |= DLM_LOCK_RES_RECOVERING;
>  	if (!list_empty(&res->recovering)) {
>  		mlog(0,
> @@ -2334,11 +2336,18 @@ static void dlm_do_local_recovery_cleanup(struct dlm_ctxt *dlm, u8 dead_node)
>  					     dlm->name, res->lockname.len,
>  					     res->lockname.name, dead_node);
>  
> -				/* the wake_up for this will happen when the
> -				 * RECOVERING flag is dropped later */
> -				res->state &= ~DLM_LOCK_RES_DROPPING_REF;
> +				/*
> +				 * don't migrate a lockres which is in progress
> +				 * of dropping ref
> +				 */
> +				if (res->state & DLM_LOCK_RES_DROPPING_REF) {
> +					mlog(ML_NOTICE, "%.*s ignored for "
> +					     "migration\n", res->lockname.len,
> +					     res->lockname.name);
> +				} else
> +					dlm_move_lockres_to_recovery_list(dlm,
> +									  res);
>  
> -				dlm_move_lockres_to_recovery_list(dlm, res);
>  			} else if (res->owner == dlm->node_num) {
>  				dlm_free_dead_locks(dlm, res, dead_node);
>  				__dlm_lockres_calc_usage(dlm, res);
> diff --git a/fs/ocfs2/dlm/dlmthread.c b/fs/ocfs2/dlm/dlmthread.c
> index dd78ca3..47420ce 100644
> --- a/fs/ocfs2/dlm/dlmthread.c
> +++ b/fs/ocfs2/dlm/dlmthread.c
> @@ -92,17 +92,23 @@ int __dlm_lockres_has_locks(struct dlm_lock_resource *res)
>   * truly ready to be freed. */
>  int __dlm_lockres_unused(struct dlm_lock_resource *res)
>  {
> -	if (!__dlm_lockres_has_locks(res) &&
> -	    (list_empty(&res->dirty) && !(res->state & DLM_LOCK_RES_DIRTY))) {
> -		/* try not to scan the bitmap unless the first two
> -		 * conditions are already true */
> -		int bit = find_next_bit(res->refmap, O2NM_MAX_NODES, 0);
> -		if (bit >= O2NM_MAX_NODES) {
> -			/* since the bit for dlm->node_num is not
> -			 * set, inflight_locks better be zero */
> -			BUG_ON(res->inflight_locks != 0);
> -			return 1;
> -		}
> +	int bit;
> +
> +	if (__dlm_lockres_has_locks(res))
> +		return 0;
> +
> +	if (!list_empty(&res->dirty) || res->state & DLM_LOCK_RES_DIRTY)
> +		return 0;
> +
> +	if (res->state & DLM_LOCK_RES_RECOVERING)
> +		return 0;
> +
> +	bit = find_next_bit(res->refmap, O2NM_MAX_NODES, 0);
> +	if (bit >= O2NM_MAX_NODES) {
> +		/* since the bit for dlm->node_num is not
> +		 * set, inflight_locks better be zero */
> +		BUG_ON(res->inflight_locks != 0);
> +		return 1;
>  	}
>  	return 0;
>  }
>   

[Ocfs2-devel] [PATCH] ocfs2/dlm: avoid incorrect bit set in refmap on recovery master

[Sunil Mushran]

comments inlined

On 07/29/2010 05:37 AM, Wengang Wang wrote:
> In the following situation, there remains an incorrect bit in refmap on the
> recovery master. Finally the recovery master will fail at purging the lockres
> due to the incorrect bit in refmap.
>
> 1) node A has no interest on lockres A any longer, so it is purging it.
> 2) the owner of lockres A is node B, so node A is sending de-ref message
> to node B.
> 3) at this time, node B crashed. node C becomes the recovery master. it recovers
> lockres A(because the master is the dead node B).
> 4) node A migrated lockres A to node C with a refbit there.
> 5) node A failed to send de-ref message to node B because it crashed. The failure
> is ignored. no other action is done for lockres A any more.
>
> For mormal, re-send the deref message to it to recovery master can fix it. Well,
> ignoring the failure of deref to the original master and not recovering the lockres
> to recovery master has the same effect. And the later is simpler.
>
> Signed-off-by: Wengang Wang<wen.gang.wang@oracle.com>
> ---
>   fs/ocfs2/dlm/dlmrecovery.c |   17 +++++++++++++----
>   fs/ocfs2/dlm/dlmthread.c   |   28 +++++++++++++++++-----------
>   2 files changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/fs/ocfs2/dlm/dlmrecovery.c b/fs/ocfs2/dlm/dlmrecovery.c
> index 9dfaac7..2b57cc4 100644
> --- a/fs/ocfs2/dlm/dlmrecovery.c
> +++ b/fs/ocfs2/dlm/dlmrecovery.c
> @@ -1997,6 +1997,8 @@ void dlm_move_lockres_to_recovery_list(struct dlm_ctxt *dlm,
>   	struct list_head *queue;
>   	struct dlm_lock *lock, *next;
>
> +	assert_spin_locked(&dlm->spinlock);
> +	assert_spin_locked(&res->spinlock);
>   	res->state |= DLM_LOCK_RES_RECOVERING;
>   	if (!list_empty(&res->recovering)) {
>   		mlog(0,
> @@ -2334,11 +2336,18 @@ static void dlm_do_local_recovery_cleanup(struct dlm_ctxt *dlm, u8 dead_node)
>   					     dlm->name, res->lockname.len,
>   					     res->lockname.name, dead_node);
>
> -				/* the wake_up for this will happen when the
> -				 * RECOVERING flag is dropped later */
> -				res->state&= ~DLM_LOCK_RES_DROPPING_REF;
> +				/*
> +				 * don't migrate a lockres which is in progress
> +				 * of dropping ref
> +				 */
> +				if (res->state&  DLM_LOCK_RES_DROPPING_REF) {
> +					mlog(ML_NOTICE, "%.*s ignored for "
> +					     "migration\n", res->lockname.len,
> +					     res->lockname.name);
> +				} else
> +					dlm_move_lockres_to_recovery_list(dlm,
> +									  res);
>
> -				dlm_move_lockres_to_recovery_list(dlm, res);
>   			} else if (res->owner == dlm->node_num) {
>   				dlm_free_dead_locks(dlm, res, dead_node);
>   				__dlm_lockres_calc_usage(dlm, res);
>    

So the code reads like this.

                                 if (res->state & DLM_LOCK_RES_DROPPING_REF)
                                         mlog(0, "%s:%.*s: owned by "
                                              "dead node %u, this node was "
                                              "dropping its ref when it 
died. "
                                              "continue, dropping the 
flag.\n",
                                              dlm->name, res->lockname.len,
                                              res->lockname.name, 
dead_node);

                                 /*
                                  * don't migrate a lockres which is in 
progress
                                  * of dropping ref
                                  */
                                 if (res->state & 
DLM_LOCK_RES_DROPPING_REF) {
                                         mlog(ML_NOTICE, "%.*s ignored for "
                                              "migration\n", 
res->lockname.len,
                                              res->lockname.name);
                                 } else
                                         
dlm_move_lockres_to_recovery_list(dlm,
                                                                           res);

The first mlog should be removed. It is incorrect. The second mlog
is more appropriate. Could be reworded ("Ignore %.*s for recovery as it is
being freed").

The comment can just be removed. The mlog says it all.

> diff --git a/fs/ocfs2/dlm/dlmthread.c b/fs/ocfs2/dlm/dlmthread.c
> index dd78ca3..47420ce 100644
> --- a/fs/ocfs2/dlm/dlmthread.c
> +++ b/fs/ocfs2/dlm/dlmthread.c
> @@ -92,17 +92,23 @@ int __dlm_lockres_has_locks(struct dlm_lock_resource *res)
>    * truly ready to be freed. */
>   int __dlm_lockres_unused(struct dlm_lock_resource *res)
>   {
> -	if (!__dlm_lockres_has_locks(res)&&
> -	    (list_empty(&res->dirty)&&  !(res->state&  DLM_LOCK_RES_DIRTY))) {
> -		/* try not to scan the bitmap unless the first two
> -		 * conditions are already true */
> -		int bit = find_next_bit(res->refmap, O2NM_MAX_NODES, 0);
> -		if (bit>= O2NM_MAX_NODES) {
> -			/* since the bit for dlm->node_num is not
> -			 * set, inflight_locks better be zero */
> -			BUG_ON(res->inflight_locks != 0);
> -			return 1;
> -		}
> +	int bit;
> +
> +	if (__dlm_lockres_has_locks(res))
> +		return 0;
> +
> +	if (!list_empty(&res->dirty) || res->state&  DLM_LOCK_RES_DIRTY)
> +		return 0;
> +
> +	if (res->state&  DLM_LOCK_RES_RECOVERING)
> +		return 0;
> +
> +	bit = find_next_bit(res->refmap, O2NM_MAX_NODES, 0);
> +	if (bit>= O2NM_MAX_NODES) {
> +		/* since the bit for dlm->node_num is not
> +		 * set, inflight_locks better be zero */
> +		BUG_ON(res->inflight_locks != 0);
> +		return 1;
>   	}
>   	return 0;
>   }
>    


I like it. But you reversed the flow at the end. How about...

	bit = find_next_bit(res->refmap, O2NM_MAX_NODES, 0);
	if (bit<  O2NM_MAX_NODES)
		return 0;

	/*
	 * Since the bit for dlm->node_num is not set, inflight_locks
	 * better be zero
	 */
	BUG_ON(res->inflight_locks != 0);

	return 1;

[Ocfs2-devel] [PATCH] ocfs2/dlm: avoid incorrect bit set in refmap on recovery master

[Wengang Wang]

In the following situation, there remains an incorrect bit in refmap on the
recovery master. Finally the recovery master will fail at purging the lockres
due to the incorrect bit in refmap.

1) node A has no interest on lockres A any longer, so it is purging it.
2) the owner of lockres A is node B, so node A is sending de-ref message
to node B.
3) at this time, node B crashed. node C becomes the recovery master. it recovers
lockres A(because the master is the dead node B).
4) node A migrated lockres A to node C with a refbit there.
5) node A failed to send de-ref message to node B because it crashed. The failure
is ignored. no other action is done for lockres A any more.

For mormal, re-send the deref message to it to recovery master can fix it. Well,
ignoring the failure of deref to the original master and not recovering the lockres
to recovery master has the same effect. And the later is simpler.

Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
Acked-by: Srinivas Eeda <srinivas.eeda@oracle.com>
---
 fs/ocfs2/dlm/dlmrecovery.c |   22 ++++++++++------------
 fs/ocfs2/dlm/dlmthread.c   |   34 +++++++++++++++++++++-------------
 2 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/fs/ocfs2/dlm/dlmrecovery.c b/fs/ocfs2/dlm/dlmrecovery.c
index 9dfaac7..aaaffbc 100644
--- a/fs/ocfs2/dlm/dlmrecovery.c
+++ b/fs/ocfs2/dlm/dlmrecovery.c
@@ -1997,6 +1997,8 @@ void dlm_move_lockres_to_recovery_list(struct dlm_ctxt *dlm,
 	struct list_head *queue;
 	struct dlm_lock *lock, *next;
 
+	assert_spin_locked(&dlm->spinlock);
+	assert_spin_locked(&res->spinlock);
 	res->state |= DLM_LOCK_RES_RECOVERING;
 	if (!list_empty(&res->recovering)) {
 		mlog(0,
@@ -2326,19 +2328,15 @@ static void dlm_do_local_recovery_cleanup(struct dlm_ctxt *dlm, u8 dead_node)
 			/* zero the lvb if necessary */
 			dlm_revalidate_lvb(dlm, res, dead_node);
 			if (res->owner == dead_node) {
-				if (res->state & DLM_LOCK_RES_DROPPING_REF)
-					mlog(0, "%s:%.*s: owned by "
-					     "dead node %u, this node was "
-					     "dropping its ref when it died. "
-					     "continue, dropping the flag.\n",
-					     dlm->name, res->lockname.len,
-					     res->lockname.name, dead_node);
-
-				/* the wake_up for this will happen when the
-				 * RECOVERING flag is dropped later */
-				res->state &= ~DLM_LOCK_RES_DROPPING_REF;
+				if (res->state & DLM_LOCK_RES_DROPPING_REF) {
+					mlog(ML_NOTICE, "Ignore %.*s for "
+					     "recovery as it is being freed\n",
+					     res->lockname.len,
+					     res->lockname.name);
+				} else
+					dlm_move_lockres_to_recovery_list(dlm,
+									  res);
 
-				dlm_move_lockres_to_recovery_list(dlm, res);
 			} else if (res->owner == dlm->node_num) {
 				dlm_free_dead_locks(dlm, res, dead_node);
 				__dlm_lockres_calc_usage(dlm, res);
diff --git a/fs/ocfs2/dlm/dlmthread.c b/fs/ocfs2/dlm/dlmthread.c
index dd78ca3..c737021 100644
--- a/fs/ocfs2/dlm/dlmthread.c
+++ b/fs/ocfs2/dlm/dlmthread.c
@@ -92,19 +92,27 @@ int __dlm_lockres_has_locks(struct dlm_lock_resource *res)
  * truly ready to be freed. */
 int __dlm_lockres_unused(struct dlm_lock_resource *res)
 {
-	if (!__dlm_lockres_has_locks(res) &&
-	    (list_empty(&res->dirty) && !(res->state & DLM_LOCK_RES_DIRTY))) {
-		/* try not to scan the bitmap unless the first two
-		 * conditions are already true */
-		int bit = find_next_bit(res->refmap, O2NM_MAX_NODES, 0);
-		if (bit >= O2NM_MAX_NODES) {
-			/* since the bit for dlm->node_num is not
-			 * set, inflight_locks better be zero */
-			BUG_ON(res->inflight_locks != 0);
-			return 1;
-		}
-	}
-	return 0;
+	int bit;
+
+	if (__dlm_lockres_has_locks(res))
+		return 0;
+
+	if (!list_empty(&res->dirty) || res->state & DLM_LOCK_RES_DIRTY)
+		return 0;
+
+	if (res->state & DLM_LOCK_RES_RECOVERING)
+		return 0;
+
+	bit = find_next_bit(res->refmap, O2NM_MAX_NODES, 0);
+	if (bit < O2NM_MAX_NODES)
+		return 0;
+
+	/* 
+	 * since the bit for dlm->node_num is not set, inflight_locks better
+	 * be zero
+	 */
+	BUG_ON(res->inflight_locks != 0);
+	return 1;
 }
 
 
-- 
1.7.1.1

[Ocfs2-devel] [PATCH] ocfs2/dlm: avoid incorrect bit set in refmap on recovery master

[Sunil Mushran]

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

On 07/30/2010 01:14 AM, Wengang Wang wrote:
> In the following situation, there remains an incorrect bit in refmap on the
> recovery master. Finally the recovery master will fail at purging the lockres
> due to the incorrect bit in refmap.
>
> 1) node A has no interest on lockres A any longer, so it is purging it.
> 2) the owner of lockres A is node B, so node A is sending de-ref message
> to node B.
> 3) at this time, node B crashed. node C becomes the recovery master. it recovers
> lockres A(because the master is the dead node B).
> 4) node A migrated lockres A to node C with a refbit there.
> 5) node A failed to send de-ref message to node B because it crashed. The failure
> is ignored. no other action is done for lockres A any more.
>
> For mormal, re-send the deref message to it to recovery master can fix it. Well,
> ignoring the failure of deref to the original master and not recovering the lockres
> to recovery master has the same effect. And the later is simpler.
>
> Signed-off-by: Wengang Wang<wen.gang.wang@oracle.com>
> Acked-by: Srinivas Eeda<srinivas.eeda@oracle.com>
> ---
>   fs/ocfs2/dlm/dlmrecovery.c |   22 ++++++++++------------
>   fs/ocfs2/dlm/dlmthread.c   |   34 +++++++++++++++++++++-------------
>   2 files changed, 31 insertions(+), 25 deletions(-)
>
> diff --git a/fs/ocfs2/dlm/dlmrecovery.c b/fs/ocfs2/dlm/dlmrecovery.c
> index 9dfaac7..aaaffbc 100644
> --- a/fs/ocfs2/dlm/dlmrecovery.c
> +++ b/fs/ocfs2/dlm/dlmrecovery.c
> @@ -1997,6 +1997,8 @@ void dlm_move_lockres_to_recovery_list(struct dlm_ctxt *dlm,
>   	struct list_head *queue;
>   	struct dlm_lock *lock, *next;
>
> +	assert_spin_locked(&dlm->spinlock);
> +	assert_spin_locked(&res->spinlock);
>   	res->state |= DLM_LOCK_RES_RECOVERING;
>   	if (!list_empty(&res->recovering)) {
>   		mlog(0,
> @@ -2326,19 +2328,15 @@ static void dlm_do_local_recovery_cleanup(struct dlm_ctxt *dlm, u8 dead_node)
>   			/* zero the lvb if necessary */
>   			dlm_revalidate_lvb(dlm, res, dead_node);
>   			if (res->owner == dead_node) {
> -				if (res->state&  DLM_LOCK_RES_DROPPING_REF)
> -					mlog(0, "%s:%.*s: owned by "
> -					     "dead node %u, this node was "
> -					     "dropping its ref when it died. "
> -					     "continue, dropping the flag.\n",
> -					     dlm->name, res->lockname.len,
> -					     res->lockname.name, dead_node);
> -
> -				/* the wake_up for this will happen when the
> -				 * RECOVERING flag is dropped later */
> -				res->state&= ~DLM_LOCK_RES_DROPPING_REF;
> +				if (res->state&  DLM_LOCK_RES_DROPPING_REF) {
> +					mlog(ML_NOTICE, "Ignore %.*s for "
> +					     "recovery as it is being freed\n",
> +					     res->lockname.len,
> +					     res->lockname.name);
> +				} else
> +					dlm_move_lockres_to_recovery_list(dlm,
> +									  res);
>
> -				dlm_move_lockres_to_recovery_list(dlm, res);
>   			} else if (res->owner == dlm->node_num) {
>   				dlm_free_dead_locks(dlm, res, dead_node);
>   				__dlm_lockres_calc_usage(dlm, res);
> diff --git a/fs/ocfs2/dlm/dlmthread.c b/fs/ocfs2/dlm/dlmthread.c
> index dd78ca3..c737021 100644
> --- a/fs/ocfs2/dlm/dlmthread.c
> +++ b/fs/ocfs2/dlm/dlmthread.c
> @@ -92,19 +92,27 @@ int __dlm_lockres_has_locks(struct dlm_lock_resource *res)
>    * truly ready to be freed. */
>   int __dlm_lockres_unused(struct dlm_lock_resource *res)
>   {
> -	if (!__dlm_lockres_has_locks(res)&&
> -	    (list_empty(&res->dirty)&&  !(res->state&  DLM_LOCK_RES_DIRTY))) {
> -		/* try not to scan the bitmap unless the first two
> -		 * conditions are already true */
> -		int bit = find_next_bit(res->refmap, O2NM_MAX_NODES, 0);
> -		if (bit>= O2NM_MAX_NODES) {
> -			/* since the bit for dlm->node_num is not
> -			 * set, inflight_locks better be zero */
> -			BUG_ON(res->inflight_locks != 0);
> -			return 1;
> -		}
> -	}
> -	return 0;
> +	int bit;
> +
> +	if (__dlm_lockres_has_locks(res))
> +		return 0;
> +
> +	if (!list_empty(&res->dirty) || res->state&  DLM_LOCK_RES_DIRTY)
> +		return 0;
> +
> +	if (res->state&  DLM_LOCK_RES_RECOVERING)
> +		return 0;
> +
> +	bit = find_next_bit(res->refmap, O2NM_MAX_NODES, 0);
> +	if (bit<  O2NM_MAX_NODES)
> +		return 0;
> +
> +	/*
> +	 * since the bit for dlm->node_num is not set, inflight_locks better
> +	 * be zero
> +	 */
> +	BUG_ON(res->inflight_locks != 0);
> +	return 1;
>   }
>
>
>    

[Ocfs2-devel] [PATCH] ocfs2/dlm: avoid incorrect bit set in refmap on recovery master

[Joel Becker]

On Fri, Jul 30, 2010 at 04:14:44PM +0800, Wengang Wang wrote:
> In the following situation, there remains an incorrect bit in refmap on the
> recovery master. Finally the recovery master will fail at purging the lockres
> due to the incorrect bit in refmap.
> 
> 1) node A has no interest on lockres A any longer, so it is purging it.
> 2) the owner of lockres A is node B, so node A is sending de-ref message
> to node B.
> 3) at this time, node B crashed. node C becomes the recovery master. it recovers
> lockres A(because the master is the dead node B).
> 4) node A migrated lockres A to node C with a refbit there.
> 5) node A failed to send de-ref message to node B because it crashed. The failure
> is ignored. no other action is done for lockres A any more.
> 
> For mormal, re-send the deref message to it to recovery master can fix it. Well,
> ignoring the failure of deref to the original master and not recovering the lockres
> to recovery master has the same effect. And the later is simpler.
> 
> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> Acked-by: Srinivas Eeda <srinivas.eeda@oracle.com>

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

Joel

-- 

 The zen have a saying:
 "When you learn how to listen, ANYONE can be your teacher."

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