ocfs2/dlm: avoid incorrect bit set in refmap on recovery master
PATCH 1/1 ocfs2/dlm: avoid incorrect bit set in refmap on recovery master
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.
--- fs/ocfs2/dlm/dlmrecovery.c | 17 +++++++++++++++-- fs/ocfs2/dlm/dlmthread.c | 28 +++++++++++++++++----------- 2 files changed, 32 insertions(+), 13 deletions(-)
fs/ocfs2/dlm/dlmrecovery.c +15 -2
@@ -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);
fs/ocfs2/dlm/dlmthread.c +17 -11
@@ -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 ; }
Signature · 1 line
1.7.1.1
PATCH 1/1 ocfs2/dlm: avoid incorrect bit set in refmap on recovery master
thanks for making this patch, it looks good just few minor changes about comments
Quoted 37 lines · 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 <[email protected]>--- fs/ocfs2/dlm/dlmrecovery.c | 17 +++++++++++++++-- fs/ocfs2/dlm/dlmthread.c | 28 +++++++++++++++++----------- 2 files changed, 32 insertions(+), 13 deletions(-)fs/ocfs2/dlm/dlmrecovery.c +2
-0@@ -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,
fs/ocfs2/dlm/dlmrecovery.c static void dlm_do_local_recovery_cleanup(struct dlm_ctxt *dlm, u8 dead_node) · line 2341
Quoted 4 lines
fs/ocfs2/dlm/dlmrecovery.c +0
-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.
fs/ocfs2/dlm/dlmrecovery.c static void dlm_do_local_recovery_cleanup(struct dlm_ctxt *dlm, u8 dead_node) · line 2354
Quoted 7 lines
fs/ocfs2/dlm/dlmrecovery.c +5
-1- 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
fs/ocfs2/dlm/dlmrecovery.c static void dlm_do_local_recovery_cleanup(struct dlm_ctxt *dlm, u8 dead_node) · line 2354
Quoted 4 lines
fs/ocfs2/dlm/dlmrecovery.c +3
-0+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.
fs/ocfs2/dlm/dlmrecovery.c static void dlm_do_local_recovery_cleanup(struct dlm_ctxt *dlm, u8 dead_node) · line 2354
Quoted 3 lines
fs/ocfs2/dlm/dlmrecovery.c +2
-0+ res->state &= + ~DLM_LOCK_RES_DROPPING_REF ;
we don't need to clear this state as dlm_purge_lockres removes it.
Quoted 48 lines
fs/ocfs2/dlm/dlmrecovery.c +3
-1+ }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);fs/ocfs2/dlm/dlmthread.c +17
-11@@ -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 ; }
PATCH 1/1 ocfs2/dlm: avoid incorrect bit set in refmap on recovery master
Hi Srini,
Thanks for your review!
Quoted 27 lines · Srinivas Eeda
thanks for making this patch, it looks good just few minor changes about comments
Quoted 23 lines · 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 <[email protected]>---
fs/ocfs2/dlm/dlmrecovery.c | 17 +++++++++++++++-- fs/ocfs2/dlm/dlmthread.c | 28 +++++++++++++++++----------- 2 files changed, 32 insertions(+), 13 deletions(-)
Quoted 16 lines
Quoted 13 lines
fs/ocfs2/dlm/dlmrecovery.c +2
-0@@ -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,fs/ocfs2/dlm/dlmrecovery.c static void dlm_do_local_recovery_cleanup(struct dlm_ctxt *dlm, u8 dead_node) · line 2339
Quoted 2 lines
fs/ocfs2/dlm/dlmrecovery.c +0
-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.
Quoted 7 lines
fs/ocfs2/dlm/dlmrecovery.c static void dlm_do_local_recovery_cleanup(struct dlm_ctxt *dlm, u8 dead_node) · line 2345
Quoted 6 lines
fs/ocfs2/dlm/dlmrecovery.c +5
-1- 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.
Quoted 6 lines
fs/ocfs2/dlm/dlmrecovery.c static void dlm_do_local_recovery_cleanup(struct dlm_ctxt *dlm, u8 dead_node) · line 2348
Quoted 3 lines
fs/ocfs2/dlm/dlmrecovery.c +3
-0+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.
Quoted 3 lines
fs/ocfs2/dlm/dlmrecovery.c static void dlm_do_local_recovery_cleanup(struct dlm_ctxt *dlm, u8 dead_node) · line 2350
Quoted 2 lines
fs/ocfs2/dlm/dlmrecovery.c +2
-0+ res->state &= + ~DLM_LOCK_RES_DROPPING_REF ;we don't need to clear this state as
dlm_purge_lockresremoves it.
OK.
regards,
wengang.
Quoted 7 lines
Quoted 7 lines
fs/ocfs2/dlm/dlmrecovery.c +3
-1+ }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);
PATCH 1/1 ocfs2/dlm: avoid incorrect bit set in refmap on recovery master
In reply to Srinivas EedaIn 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.
--- fs/ocfs2/dlm/dlmrecovery.c | 17 +++++++++++++---- fs/ocfs2/dlm/dlmthread.c | 28 +++++++++++++++++----------- 2 files changed, 30 insertions(+), 15 deletions(-)
fs/ocfs2/dlm/dlmrecovery.c +13 -4
@@ -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);
fs/ocfs2/dlm/dlmthread.c +17 -11
@@ -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 ; }
Signature · 1 line
1.7.1.1
PATCH 1/1 ocfs2/dlm: avoid incorrect bit set in refmap on recovery master
Quoted 99 lines · 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 <[email protected]>--- fs/ocfs2/dlm/dlmrecovery.c | 17 +++++++++++++---- fs/ocfs2/dlm/dlmthread.c | 28 +++++++++++++++++----------- 2 files changed, 30 insertions(+), 15 deletions(-)fs/ocfs2/dlm/dlmrecovery.c +13
-4@@ -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);fs/ocfs2/dlm/dlmthread.c +17
-11@@ -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 ; }
PATCH 1/1 ocfs2/dlm: avoid incorrect bit set in refmap on recovery master
In reply to Wengang Wangcomments inlined
Quoted 50 lines · 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<[email protected]>--- fs/ocfs2/dlm/dlmrecovery.c | 17 +++++++++++++---- fs/ocfs2/dlm/dlmthread.c | 28 +++++++++++++++++----------- 2 files changed, 30 insertions(+), 15 deletions(-)fs/ocfs2/dlm/dlmrecovery.c +8
-3@@ -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 "
fs/ocfs2/dlm/dlmrecovery.c static void dlm_do_local_recovery_cleanup(struct dlm_ctxt *dlm, u8 dead_node) · line 2353
Quoted 10 lines
fs/ocfs2/dlm/dlmrecovery.c +5
-1+"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);
} elsedlm_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.
Quoted 30 lines
fs/ocfs2/dlm/dlmthread.c +10
-11@@ -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 ;
fs/ocfs2/dlm/dlmthread.c int __dlm_lockres_has_locks(struct dlm_lock_resource *res) · line 114
Quoted 10 lines
fs/ocfs2/dlm/dlmthread.c +7
-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;
PATCH 1/1 ocfs2/dlm: avoid incorrect bit set in refmap on recovery master
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.
--- fs/ocfs2/dlm/dlmrecovery.c | 22 ++++++++++------------ fs/ocfs2/dlm/dlmthread.c | 34 +++++++++++++++++++++------------- 2 files changed, 31 insertions(+), 25 deletions(-)
fs/ocfs2/dlm/dlmrecovery.c +10 -12
@@ -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);
fs/ocfs2/dlm/dlmthread.c +21 -13
@@ -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 ; }
Signature · 1 line
1.7.1.1
PATCH 1/1 ocfs2/dlm: avoid incorrect bit set in refmap on recovery master
Quoted 111 lines · 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<[email protected]>Acked-by Srinivas Eeda<[email protected]>--- fs/ocfs2/dlm/dlmrecovery.c | 22 ++++++++++------------ fs/ocfs2/dlm/dlmthread.c | 34 +++++++++++++++++++++------------- 2 files changed, 31 insertions(+), 25 deletions(-)fs/ocfs2/dlm/dlmrecovery.c +10
-12@@ -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);fs/ocfs2/dlm/dlmthread.c +21
-13@@ -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 ; }
PATCH 1/1 ocfs2/dlm: avoid incorrect bit set in refmap on recovery master
In reply to Wengang WangQuoted 13 lines · 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.
Quoted 6 lines
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 <[email protected]>Acked-by Srinivas Eeda <[email protected]>
This patch is now in the fixes branch of ocfs2.git.
Joel
Signature · 8 lines
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