dlm: recovery
PATCH 3/7 dlm: recovery
When a node is removed from a lockspace, recovery is required for that lockspace on all the remaining lockspace members. Recovery involves: a full rebuild of the distributed resource directory, selecting a new master node for locks/resources previously mastered on the removed node, and rebuilding master-copy locks on newly selected masters.
---
drivers/dlm/member.c | 347 ++++++++++++++++++++++ drivers/dlm/member.h | 29 + drivers/dlm/rcom.c | 413 ++++++++++++++++++++++++++ drivers/dlm/rcom.h | 44 ++ drivers/dlm/recover.c | 706 +++++++++++++++++++++++++++++++++++++++++++++ drivers/dlm/recover.h | 31 + drivers/dlm/recoverd.c | 705 ++++++++++++++++++++++++++++++++++++++++++++ drivers/dlm/recoverd.h | 23 + drivers/dlm/requestqueue.c | 144 +++++++++ drivers/dlm/requestqueue.h | 21 + 10 files changed, 2463 insertions(+)
drivers/dlm/rcom.c +413 -0
@@ -0,0 +1,413 @@ +/****************************************************************************** +******************************************************************************* +** +** Copyright (C) Sistina Software, Inc. 1997-2003 All rights reserved. +** Copyright (C) 2005 Red Hat, Inc. All rights reserved. +** +** This copyrighted material is made available to anyone wishing to use, +** modify, copy, or redistribute it subject to the terms and conditions +** of the GNU General Public License v.2. +** +******************************************************************************* +******************************************************************************/ + +#include "dlm_internal.h" +#include "lockspace.h" +#include "member.h" +#include "lowcomms.h" +#include "midcomms.h" +#include "rcom.h" +#include "recover.h" +#include "dir.h" +#include "config.h" +#include "memory.h" +#include "lock.h" +#include "util.h" + + +static int rcom_response (struct dlm_ls *ls) +{ +return test_bit (LSFL_RCOM_READY , &ls->ls_flags); +} + +static int create_rcom (struct dlm_ls *ls,int to_nodeid,int type,int len, +struct dlm_rcom **rc_ret,struct dlm_mhandle **mh_ret) +{ +struct dlm_rcom *rc; +struct dlm_mhandle *mh; +char *mb; +int mb_len =sizeof (struct dlm_rcom ) + len; + + mh =dlm_lowcomms_get_buffer (to_nodeid, mb_len,GFP_KERNEL , &mb); +if (!mh) +return -ENOBUFS ; +memset (mb,0 , mb_len); + + rc = (struct dlm_rcom *) mb; + + rc->rc_header.h_version = (DLM_HEADER_MAJOR |DLM_HEADER_MINOR ); + rc->rc_header.h_lockspace = ls->ls_global_id; + rc->rc_header.h_nodeid =dlm_our_nodeid (); + rc->rc_header.h_length = mb_len; + rc->rc_header.h_cmd =DLM_RCOM ; + + rc->rc_type = type; + + *mh_ret = mh; + *rc_ret = rc; +return 0 ; +} + +static int send_rcom (struct dlm_ls *ls,struct dlm_mhandle *mh, +struct dlm_rcom *rc) +{ +dlm_rcom_out (rc); +dlm_lowcomms_commit_buffer (mh); +return 0 ; +} + +static int make_status (struct dlm_ls *ls) +{ +int status =0 ; + +if (test_bit (LSFL_DIR_VALID , &ls->ls_flags)) + status |=DIR_VALID ; + +if (test_bit (LSFL_ALL_DIR_VALID , &ls->ls_flags)) + status |=DIR_ALL_VALID ; + +if (test_bit (LSFL_NODES_VALID , &ls->ls_flags)) + status |=NODES_VALID ; + +if (test_bit (LSFL_ALL_NODES_VALID , &ls->ls_flags)) + status |=NODES_ALL_VALID ; + +return status; +} + +int dlm_rcom_status (struct dlm_ls *ls,int nodeid) +{ +struct dlm_rcom *rc; +struct dlm_mhandle *mh; +int error; + +memset (ls->ls_recover_buf,0 , dlm_config.buffer_size); + +if (nodeid ==dlm_our_nodeid ()) { + rc = (struct dlm_rcom *) ls->ls_recover_buf; + rc->rc_result =make_status (ls); +return 0 ; + } + + error =create_rcom (ls, nodeid,DLM_RCOM_STATUS ,0 , &rc, &mh); + + error =send_rcom (ls, mh, rc); + + error =dlm_wait_function (ls, &rcom_response); +clear_bit (LSFL_RCOM_READY , &ls->ls_flags); +return error; +} + +static void receive_rcom_status (struct dlm_ls *ls,struct dlm_rcom *rc_in) +{ +struct dlm_rcom *rc; +struct dlm_mhandle *mh; +int error, nodeid = rc_in->rc_header.h_nodeid; + + error =create_rcom (ls, nodeid,DLM_RCOM_STATUS_REPLY ,0 , &rc, &mh); + rc->rc_result =make_status (ls); + + error =send_rcom (ls, mh, rc); +} + +static void receive_rcom_status_reply (struct dlm_ls *ls,struct dlm_rcom *rc_in) +{ +memcpy (ls->ls_recover_buf, rc_in, rc_in->rc_header.h_length); +set_bit (LSFL_RCOM_READY , &ls->ls_flags); +wake_up (&ls->ls_wait_general); +} + +int dlm_rcom_names (struct dlm_ls *ls,int nodeid,char *last_name,int last_len) +{ +struct dlm_rcom *rc; +struct dlm_mhandle *mh; +int error, len =sizeof (struct dlm_rcom ); + +memset (ls->ls_recover_buf,0 , dlm_config.buffer_size); + +if (nodeid ==dlm_our_nodeid ()) { +dlm_copy_master_names (ls, last_name, last_len, + ls->ls_recover_buf + len, + dlm_config.buffer_size - len, nodeid); +return 0 ; + } + + error =create_rcom (ls, nodeid,DLM_RCOM_NAMES , last_len, &rc, &mh); +memcpy (rc->rc_buf, last_name, last_len); + + error =send_rcom (ls, mh, rc); + + error =dlm_wait_function (ls, &rcom_response); +clear_bit (LSFL_RCOM_READY , &ls->ls_flags); +return error; +} + +static void receive_rcom_names (struct dlm_ls *ls,struct dlm_rcom *rc_in) +{ +struct dlm_rcom *rc; +struct dlm_mhandle *mh; +int error, inlen, outlen; +int nodeid = rc_in->rc_header.h_nodeid; + +/* +* We can't run dlm_dir_rebuild_send (which uses ls_nodes) while +* dlm_recoverd is running ls_nodes_reconfig (which changes ls_nodes). +* It could only happen in rare cases where we get a late NAMES +* message from a previous instance of recovery. +*/ + +if (!test_bit (LSFL_NODES_VALID , &ls->ls_flags)) { +log_debug (ls,"ignoring RCOM_NAMES from %u" , nodeid); +return ; + } + + nodeid = rc_in->rc_header.h_nodeid; + inlen = rc_in->rc_header.h_length -sizeof (struct dlm_rcom ); + outlen = dlm_config.buffer_size -sizeof (struct dlm_rcom ); + + error =create_rcom (ls, nodeid,DLM_RCOM_NAMES_REPLY , outlen, &rc, &mh); + + error =dlm_copy_master_names (ls, rc_in->rc_buf, inlen, rc->rc_buf, + outlen, nodeid); + + error =send_rcom (ls, mh, rc); +} + +static void receive_rcom_names_reply (struct dlm_ls *ls,struct dlm_rcom *rc_in) +{ +memcpy (ls->ls_recover_buf, rc_in, rc_in->rc_header.h_length); +set_bit (LSFL_RCOM_READY , &ls->ls_flags); +wake_up (&ls->ls_wait_general); +} + +int dlm_send_rcom_lookup (struct dlm_rsb *r,int dir_nodeid) +{ +struct dlm_rcom *rc; +struct dlm_mhandle *mh; +struct dlm_ls *ls = r->res_ls; +int error; + + error =create_rcom (ls, dir_nodeid,DLM_RCOM_LOOKUP , r->res_length, + &rc, &mh); +memcpy (rc->rc_buf, r->res_name, r->res_length); + rc->rc_id = (unsigned long ) r; + + error =send_rcom (ls, mh, rc); +return 0 ; +} + +static void receive_rcom_lookup (struct dlm_ls *ls,struct dlm_rcom *rc_in) +{ +struct dlm_rcom *rc; +struct dlm_mhandle *mh; +int error, ret_nodeid, nodeid = rc_in->rc_header.h_nodeid; +int len = rc_in->rc_header.h_length -sizeof (struct dlm_rcom ); + + error =dlm_dir_lookup (ls, nodeid, rc_in->rc_buf, len, &ret_nodeid); + + error =create_rcom (ls, nodeid,DLM_RCOM_LOOKUP_REPLY ,0 , &rc, &mh); + rc->rc_result = ret_nodeid; + rc->rc_id = rc_in->rc_id; + + error =send_rcom (ls, mh, rc); +} + +static void receive_rcom_lookup_reply (struct dlm_ls *ls,struct dlm_rcom *rc_in) +{ +dlm_recover_master_reply (ls, rc_in); +} + +static void pack_rcom_lock (struct dlm_rsb *r,struct dlm_lkb *lkb, +struct rcom_lock *rl) +{ +memset (rl,0 ,sizeof (*rl)); + + rl->rl_ownpid = lkb->lkb_ownpid; + rl->rl_lkid = lkb->lkb_id; + rl->rl_exflags = lkb->lkb_exflags; + rl->rl_flags = lkb->lkb_flags; + rl->rl_lvbseq = lkb->lkb_lvbseq; + rl->rl_rqmode = lkb->lkb_rqmode; + rl->rl_grmode = lkb->lkb_grmode; + rl->rl_status = lkb->lkb_status; + rl->rl_wait_type = lkb->lkb_wait_type; + +if (lkb->lkb_bastaddr) + rl->rl_asts |=AST_BAST ; +if (lkb->lkb_astaddr) + rl->rl_asts |=AST_COMP ; + +if (lkb->lkb_range) +memcpy (rl->rl_range, lkb->lkb_range,4 *sizeof (uint64_t )); + +/* FIXME: might we have an lvb without DLM_LKF_VALBLK set ? +If so, receive_rcom_lock_args() won't take this copy. */ + +if (lkb->lkb_lvbptr) +memcpy (rl->rl_lvb, lkb->lkb_lvbptr,DLM_LVB_LEN ); + + rl->rl_namelen = r->res_length; +memcpy (rl->rl_name, r->res_name, r->res_length); +} + +int dlm_send_rcom_lock (struct dlm_rsb *r,struct dlm_lkb *lkb) +{ +struct dlm_ls *ls = r->res_ls; +struct dlm_rcom *rc; +struct dlm_mhandle *mh; +struct rcom_lock *rl; +int error; + + error =create_rcom (ls, r->res_nodeid,DLM_RCOM_LOCK , +sizeof (struct rcom_lock ), &rc, &mh); + + rl = (struct rcom_lock *) rc->rc_buf; +pack_rcom_lock (r, lkb, rl); + rc->rc_id = (unsigned long ) r; + + error =send_rcom (ls, mh, rc); + +return error; +} + +static void receive_rcom_lock (struct dlm_ls *ls,struct dlm_rcom *rc_in) +{ +struct dlm_rcom *rc; +struct dlm_mhandle *mh; +int error, nodeid = rc_in->rc_header.h_nodeid; + +dlm_recover_master_copy (ls, rc_in); + + error =create_rcom (ls, nodeid,DLM_RCOM_LOCK_REPLY , +sizeof (struct rcom_lock ), &rc, &mh); + +/* We send back the same rcom_lock struct we received, but +dlm_recover_master_copy() has filled in rl_remid and rl_result */ + +memcpy (rc->rc_buf, rc_in->rc_buf,sizeof (struct rcom_lock )); + rc->rc_id = rc_in->rc_id; + + error =send_rcom (ls, mh, rc); +} + +static void receive_rcom_lock_reply (struct dlm_ls *ls,struct dlm_rcom *rc_in) +{ +if (!test_bit (LSFL_DIR_VALID , &ls->ls_flags)) { +log_debug (ls,"ignoring RCOM_LOCK_REPLY from %u" , + rc_in->rc_header.h_nodeid); +return ; + } + +dlm_recover_process_copy (ls, rc_in); +} + +static int send_ls_not_ready (int nodeid,struct dlm_rcom *rc_in) +{ +struct dlm_rcom *rc; +struct dlm_mhandle *mh; +char *mb; +int mb_len =sizeof (struct dlm_rcom ); + + mh =dlm_lowcomms_get_buffer (nodeid, mb_len,GFP_KERNEL , &mb); +if (!mh) +return -ENOBUFS ; +memset (mb,0 , mb_len); + + rc = (struct dlm_rcom *) mb; + + rc->rc_header.h_version = (DLM_HEADER_MAJOR |DLM_HEADER_MINOR ); + rc->rc_header.h_lockspace = rc_in->rc_header.h_lockspace; + rc->rc_header.h_nodeid =dlm_our_nodeid (); + rc->rc_header.h_length = mb_len; + rc->rc_header.h_cmd =DLM_RCOM ; + + rc->rc_type =DLM_RCOM_STATUS_REPLY ; + rc->rc_result =0 ; + +dlm_rcom_out (rc); +dlm_lowcomms_commit_buffer (mh); + +return 0 ; +} + +/* Called by dlm_recvd; corresponds to dlm_receive_message() but special +recovery-only comms are sent through here. */ + +void dlm_receive_rcom (struct dlm_header *hd,int nodeid) +{ +struct dlm_rcom *rc = (struct dlm_rcom *) hd; +struct dlm_ls *ls; + +dlm_rcom_in (rc); + +/* If the lockspace doesn't exist then still send a status message +back; it's possible that it just doesn't have its global_id yet. */ + + ls =dlm_find_lockspace_global (hd->h_lockspace); +if (!ls) { +send_ls_not_ready (nodeid, rc); +return ; + } + +if (dlm_recovery_stopped (ls) && (rc->rc_type !=DLM_RCOM_STATUS )) { +log_error (ls,"ignoring recovery message %x from %d" , + rc->rc_type, nodeid); +return ; + } + +if (nodeid != rc->rc_header.h_nodeid) { +log_error (ls,"bad rcom nodeid %d from %d" , + rc->rc_header.h_nodeid, nodeid); +return ; + } + +switch (rc->rc_type) { +case DLM_RCOM_STATUS : +receive_rcom_status (ls, rc); +break ; + +case DLM_RCOM_NAMES : +receive_rcom_names (ls, rc); +break ; + +case DLM_RCOM_LOOKUP : +receive_rcom_lookup (ls, rc); +break ; + +case DLM_RCOM_LOCK : +receive_rcom_lock (ls, rc); +break ; + +case DLM_RCOM_STATUS_REPLY : +receive_rcom_status_reply (ls, rc); +break ; + +case DLM_RCOM_NAMES_REPLY : +receive_rcom_names_reply (ls, rc); +break ; + +case DLM_RCOM_LOOKUP_REPLY : +receive_rcom_lookup_reply (ls, rc); +break ; + +case DLM_RCOM_LOCK_REPLY : +receive_rcom_lock_reply (ls, rc); +break ; + +default : + DLM_ASSERT(0, printk("rc_type=%x\n", rc->rc_type);); + } + +dlm_put_lockspace (ls); +} +
drivers/dlm/rcom.h +44 -0
44 lines not on this page · raw message
drivers/dlm/recover.c +706 -0
706 lines not on this page · raw message
drivers/dlm/recover.h +31 -0
@@ -0,0 +1,31 @@ +/****************************************************************************** +******************************************************************************* +** +** Copyright (C) Sistina Software, Inc. 1997-2003 All rights reserved. +** Copyright (C) 2004-2005 Red Hat, Inc. All rights reserved. +** +** This copyrighted material is made available to anyone wishing to use, +** modify, copy, or redistribute it subject to the terms and conditions +** of the GNU General Public License v.2. +** +******************************************************************************* +******************************************************************************/ + +#ifndef __RECOVER_DOT_H__ +#define __RECOVER_DOT_H__ + +int dlm_recovery_stopped (struct dlm_ls *ls); +int dlm_wait_function (struct dlm_ls *ls,int (*testfn) (struct dlm_ls *ls)); +int dlm_wait_status_all (struct dlm_ls *ls,unsigned int wait_status); +int dlm_wait_status_low (struct dlm_ls *ls,unsigned int wait_status); +int dlm_recover_masters (struct dlm_ls *ls); +int dlm_recover_master_reply (struct dlm_ls *ls,struct dlm_rcom *rc); +int dlm_recover_locks (struct dlm_ls *ls); +void dlm_recovered_lock (struct dlm_rsb *r); +int dlm_recover_lvbs (struct dlm_ls *ls); +int dlm_create_root_list (struct dlm_ls *ls); +void dlm_release_root_list (struct dlm_ls *ls); +void dlm_clear_toss_list (struct dlm_ls *ls); +void dlm_recover_conversions (struct dlm_ls *ls); + +#endif /* __RECOVER_DOT_H__ */
drivers/dlm/recoverd.c +705 -0
705 lines not on this page · raw message
drivers/dlm/recoverd.h +23 -0
@@ -0,0 +1,23 @@ +/****************************************************************************** +******************************************************************************* +** +** Copyright (C) Sistina Software, Inc. 1997-2003 All rights reserved. +** Copyright (C) 2004-2005 Red Hat, Inc. All rights reserved. +** +** This copyrighted material is made available to anyone wishing to use, +** modify, copy, or redistribute it subject to the terms and conditions +** of the GNU General Public License v.2. +** +******************************************************************************* +******************************************************************************/ + +#ifndef __RECOVERD_DOT_H__ +#define __RECOVERD_DOT_H__ + +void dlm_recoverd_kick (struct dlm_ls *ls); +void dlm_recoverd_stop (struct dlm_ls *ls); +int dlm_recoverd_start (struct dlm_ls *ls); +void dlm_recoverd_suspend (struct dlm_ls *ls); +void dlm_recoverd_resume (struct dlm_ls *ls); + +#endif /* __RECOVERD_DOT_H__ */
drivers/dlm/requestqueue.c +144 -0
144 lines not on this page · raw message
drivers/dlm/requestqueue.h +21 -0
@@ -0,0 +1,21 @@ +/****************************************************************************** +******************************************************************************* +** +** Copyright (C) 2005 Red Hat, Inc. All rights reserved. +** +** This copyrighted material is made available to anyone wishing to use, +** modify, copy, or redistribute it subject to the terms and conditions +** of the GNU General Public License v.2. +** +******************************************************************************* +******************************************************************************/ + +#ifndef __REQUESTQUEUE_DOT_H__ +#define __REQUESTQUEUE_DOT_H__ + +void dlm_add_requestqueue (struct dlm_ls *ls,int nodeid,struct dlm_header *hd); +int dlm_process_requestqueue (struct dlm_ls *ls); +void dlm_wait_requestqueue (struct dlm_ls *ls); +void dlm_purge_requestqueue (struct dlm_ls *ls); + +#endif
drivers/dlm/member.c +347 -0
347 lines not on this page · raw message
drivers/dlm/member.h +29 -0
@@ -0,0 +1,29 @@ +/****************************************************************************** +******************************************************************************* +** +** Copyright (C) 2005 Red Hat, Inc. All rights reserved. +** +** This copyrighted material is made available to anyone wishing to use, +** modify, copy, or redistribute it subject to the terms and conditions +** of the GNU General Public License v.2. +** +******************************************************************************* +******************************************************************************/ + +#ifndef __MEMBER_DOT_H__ +#define __MEMBER_DOT_H__ + +int dlm_ls_terminate (struct dlm_ls *ls); +int dlm_ls_stop (struct dlm_ls *ls); +int dlm_ls_start (struct dlm_ls *ls,int event_nr); +int dlm_ls_finish (struct dlm_ls *ls,int event_nr); + +void dlm_clear_members (struct dlm_ls *ls); +void dlm_clear_members_gone (struct dlm_ls *ls); +void dlm_clear_members_finish (struct dlm_ls *ls,int finish_event); +int dlm_recover_members_first (struct dlm_ls *ls,struct dlm_recover *rv); +int dlm_recover_members (struct dlm_ls *ls,struct dlm_recover *rv,int *neg_out); +int dlm_is_removed (struct dlm_ls *ls,int nodeid); + +#endif /* __MEMBER_DOT_H__ */ +
Quoted 9 lines · David Teigland
When a node is removed from a lockspace, recovery is required for that lockspace on all the remaining lockspace members. Recovery involves: a full rebuild of the distributed resource directory, selecting a new master node for locks/resources previously mastered on the removed node, and rebuilding master-copy locks on newly selected masters.
Signed-off-by Dave Teigland <[email protected]>Signed-off-by Patrick Caulfield <[email protected]>
<snip>
drivers/dlm/recover.c line 56
Quoted 8 lines
drivers/dlm/recover.c +8
-0+int dlm_wait_function (struct dlm_ls *ls,int (*testfn) (struct dlm_ls *ls)) +{ +int error =0 ; + +for (;;) { + wait_event_interruptible_timeout(ls->ls_wait_general, + testfn(ls) || + test_bit(LSFL_LS_STOP, &ls->ls_flags),
Shouldn't this be
dlm_recover_stopped(ls) and not test_bit()?
?
drivers/dlm/recover.c line 67
Quoted 11 lines
drivers/dlm/recover.c +11
-0+ (dlm_config.recover_timer * HZ)); +if (testfn (ls)) +break ; +if (dlm_recovery_stopped (ls)) { + error =-1 ; +break ; + } + } + +return error; +}
Rather than wrap an infinite loop in an infinite loop, since all you really want the second loop for is a periodic gurantee of recover_timer seconds, wouldn't it be easier to have a sof-timer set up to go off in that amount of time, and have it's callback do an unconditional wake-up on the local wait-queue then mod_timer the timer back in? Or might there be other tasks sleeping on the same wait-queue? At that point, since your code does not seem to care about signals (uses interruptible version, but doesn't check for signal_pending() return value from wait_event), you probably could just use the stock version of wait_event(). The conditions would be checked in wait_event() on the desired periodic basis, just as they are now, but maybe slightly more efficiently (and cleaner code, IMO :) ). If you do *need* interruptible, please put in a comment as to why.
Maybe we need yet another interface? :)
<snip>
Quoted 21 lines
drivers/dlm/recover.c +21
-0+int dlm_wait_status_all (struct dlm_ls *ls,unsigned int wait_status) +{ +struct dlm_rcom *rc = (struct dlm_rcom *) ls->ls_recover_buf; +struct dlm_member *memb; +int error =0 ; + +list_for_each_entry (memb, &ls->ls_nodes, list) { +for (;;) { + error =dlm_recovery_stopped (ls); +if (error) + goto out; + + error =dlm_rcom_status (ls, memb->nodeid); +if (error) + goto out; + +if (rc->rc_result & wait_status) +break ; +else { +set_current_state (TASK_INTERRUPTIBLE ); +schedule_timeout (HZ >>1 );
<snip>
Quoted 9 lines
drivers/dlm/recover.c +9
-0+int dlm_wait_status_low (struct dlm_ls *ls,unsigned int wait_status) +{ +struct dlm_rcom *rc = (struct dlm_rcom *) ls->ls_recover_buf; +int error =0 , nodeid = ls->ls_low_nodeid; + +for (;;) { + error =dlm_recovery_stopped (ls); +if (error) + goto out;
drivers/dlm/recover.c line 122
Quoted 10 lines
drivers/dlm/recover.c +10
-0+ + error =dlm_rcom_status (ls, nodeid); +if (error) +break ; + +if (rc->rc_result & wait_status) +break ; +else { +set_current_state (TASK_INTERRUPTIBLE ); +schedule_timeout (HZ >>1 );
500 ms is a long time! What's the justification? No comments? Especially considering you will wake up on *every* signal. It's unlikely you'll actually sleep for 500 ms. Also, please use msleep_interruptible(), unless you expect to be woken by wait-queues (I didn't have enough time to trace all the possible code paths :) ) -- in which case, make it explicit with comments, please.
Thanks,
Nish
Quoted 13 lines · Nish Aravamudan
drivers/dlm/recover.c line 56
Quoted 8 lines · David Teigland
drivers/dlm/recover.c +8
-0+int dlm_wait_function (struct dlm_ls *ls,int (*testfn) (struct dlm_ls *ls)) +{ +int error =0 ; + +for (;;) { + wait_event_interruptible_timeout(ls->ls_wait_general, + testfn(ls) || + test_bit(LSFL_LS_STOP, &ls->ls_flags),Shouldn't this be
dlm_recover_stopped(ls)and nottest_bit()?
yes, done
Quoted 17 lines
drivers/dlm/recover.c line 67
Quoted 11 lines
drivers/dlm/recover.c +11
-0+ (dlm_config.recover_timer * HZ)); +if (testfn (ls)) +break ; +if (dlm_recovery_stopped (ls)) { + error =-1 ; +break ; + } + } + +return error; +}Rather than wrap an infinite loop in an infinite loop, since all you really want the second loop for is a periodic gurantee of
recover_timerseconds, wouldn't it be easier to have a sof-timer set up to go off in that amount of time, and have it's callback do an unconditional wake-up on the local wait-queue thenmod_timerthe timer
Quoted 8 lines
back in? Or might there be other tasks sleeping on the same wait-queue? At that point, since your code does not seem to care about signals (uses interruptible version, but doesn't check for
signal_pending()return value fromwait_event), you probably could just use the stock version ofwait_event(). The conditions would be checked inwait_event()on the desired periodic basis, just as they are now, but maybe slightly more efficiently (and cleaner code, IMO :) ). If you do *need* interruptible, please put in a comment as to why.
Don't need inter, wait_event_timeout should work. Let's see if I followed all that; are you suggesting:
static void dlm_wait_timer_fn(unsigned long data)
{
struct dlm_ls *ls = (struct dlm_ls *) data;
wake_up(&ls->ls_wait_general);
}int dlm_wait_function(struct dlm_ls *ls, int (*testfn) (struct dlm_ls *ls))
{
struct timer_list timer;
int error = 0;
init_timer(&timer);
timer.function = dlm_wait_timer_fn;
timer.data = (long) ls;
for (;;) {
mod_timer(&timer, jiffies + (dlm_config.recover_timer * HZ));
wait_event(ls->ls_wait_general,
testfn(ls) || dlm_recovery_stopped(ls));
if (timer_pending(&timer))
del_timer(&timer);
if (testfn(ls))
break;
if (dlm_recovery_stopped(ls)) {
error = -1;
break;
}
}
return error;
}[Another thread should usually be calling wake_up().]
This is actually how it was some months ago, but I thought the timers made it more complicated given that wait_event_timeout is available. Do you really think the timers are nicer if we don't use interruptible?
Quoted 18 lines
Quoted 18 lines
drivers/dlm/recover.c +18
-0+int dlm_wait_status_low (struct dlm_ls *ls,unsigned int wait_status) +{ +struct dlm_rcom *rc = (struct dlm_rcom *) ls->ls_recover_buf; +int error =0 , nodeid = ls->ls_low_nodeid; + +for (;;) { + error =dlm_recovery_stopped (ls); +if (error) + goto out; + + error =dlm_rcom_status (ls, nodeid); +if (error) +break ; + +if (rc->rc_result & wait_status) +break ; +else { +set_current_state (TASK_INTERRUPTIBLE );
Quoted 8 lines
Quoted 1 line
+
schedule_timeout(HZ >> 1);500 ms is a long time! What's the justification? No comments? Especially considering you will wake up on *every* signal. It's unlikely you'll actually sleep for 500 ms. Also, please use
msleep_interruptible(), unless you expect to be woken by wait-queues (I didn't have enough time to trace all the possible code paths :) ) -- in which case, make it explicit with comments, please.
This is polling the status of a remote node. All I'm after here is a delay between the dlm_rcom_status() calls so that repeated status messages aren't flooding the network. We keep sending status messages until the remote node reports the status we want to see.
When the remote node has _failed_ we're repeating these status requests hopelessly until the membership system detects the node failure and recovery is aborted (dlm_recovery_stopped) -- up to several seconds. When the remote node is just _slower_ at doing recovery, I'm guessing the difference is on average a second.
Given that info, do you have a suggested delay? I've already switched to msleep.
Dave
Quoted 2 lines · David Teigland
Quoted 1 line · Nish Aravamudan
On 4/25/05, David Teigland <[email protected]> wrote:
<snip>
Quoted 14 lines
Quoted 5 lines
Rather than wrap an infinite loop in an infinite loop, since all you really want the second loop for is a periodic gurantee of
recover_timerseconds, wouldn't it be easier to have a sof-timer set up to go off in that amount of time, and have it's callback do an unconditional wake-up on the local wait-queue thenmod_timerthe timerQuoted 8 lines
back in? Or might there be other tasks sleeping on the same wait-queue? At that point, since your code does not seem to care about signals (uses interruptible version, but doesn't check for
signal_pending()return value fromwait_event), you probably could just use the stock version ofwait_event(). The conditions would be checked inwait_event()on the desired periodic basis, just as they are now, but maybe slightly more efficiently (and cleaner code, IMO :) ). If you do *need* interruptible, please put in a comment as to why.
Quoted 6 lines
Don't need inter,
wait_event_timeoutshould work. Let's see if I followed all that; are you suggesting:static void
dlm_wait_timer_fn(unsigned long data){ struct dlm_ls *ls = (struct dlm_ls *) data;
You can do the mod_timer() here if you make the timer_list global (dlm_timer or something).
mod_timer(&dlm_timer, jiffies + dlm_config.recover_timer * HZ;
Quoted 11 lines
wake_up(&ls->ls_wait_general); }int
dlm_wait_function(struct dlm_ls *ls, int (*testfn) (struct dlm_ls *ls)){ struct timer_list timer; int error = 0; init_timer(&timer); timer.function = dlm_wait_timer_fn; timer.data = (long) ls;
do an
timer.expires = jiffies + (dlm_config.recover_timer * HZ); add_timer(&dlm_timer);
before wait_event().
Quoted 1 line
for (;;) {Should not be need anymore.
Quoted 1 line
mod_timer(&timer, jiffies + (dlm_config.recover_timer * HZ));
Neither should this.
Quoted 2 lines
wait_event(ls->ls_wait_general,
testfn(ls) || dlm_recovery_stopped(ls));So, now, you get woken up every so often to check the conditions *in* wait_event() and if either is set you'll return to this context. Otherwise, you'll go back to sleep in wait_event() -- that's why you shouldn't need the infinite loop here.
Quoted 2 lines
if (timer_pending(&timer))
del_timer(&timer);Good, this makes sure that you won't have the callback happening again :) Do you need the sync version?
Quoted 2 lines
if (testfn(ls))
break;Since you don't modify error in this case, you won't need this anymore
Quoted 4 lines
if (dlm_recovery_stopped(ls)) {
error = -1;
break;
}Shouldn't need the break anymore, since you there is no loop.
Quoted 9 lines
} return error; }[Another thread should usually be calling
wake_up().]This is actually how it was some months ago, but I thought the timers made it more complicated given that
wait_event_timeoutis available. Do you really think the timers are nicer if we don't use interruptible?
The reason I think timers are nicer is that you don't need to nest wait_event*() in a loop (and really, it looks strange when you do). Complicated or not, I think they are the "right thing" for this case.
Quoted 32 lines
Quoted 18 lines
Quoted 18 lines
drivers/dlm/recover.c +18
-0+int dlm_wait_status_low (struct dlm_ls *ls,unsigned int wait_status) +{ +struct dlm_rcom *rc = (struct dlm_rcom *) ls->ls_recover_buf; +int error =0 , nodeid = ls->ls_low_nodeid; + +for (;;) { + error =dlm_recovery_stopped (ls); +if (error) + goto out; + + error =dlm_rcom_status (ls, nodeid); +if (error) +break ; + +if (rc->rc_result & wait_status) +break ; +else { +set_current_state (TASK_INTERRUPTIBLE );Quoted 8 lines
Quoted 1 line
+
schedule_timeout(HZ >> 1);500 ms is a long time! What's the justification? No comments? Especially considering you will wake up on *every* signal. It's unlikely you'll actually sleep for 500 ms. Also, please use
msleep_interruptible(), unless you expect to be woken by wait-queues (I didn't have enough time to trace all the possible code paths :) ) -- in which case, make it explicit with comments, please.This is polling the status of a remote node. All I'm after here is a delay between the
dlm_rcom_status()calls so that repeated status messages aren't flooding the network. We keep sending status messages until the remote node reports the status we want to see.
Quoted 8 lines
When the remote node has _failed_ we're repeating these status requests hopelessly until the membership system detects the node failure and recovery is aborted (
dlm_recovery_stopped) -- up to several seconds. When the remote node is just _slower_ at doing recovery, I'm guessing the difference is on average a second.Given that info, do you have a suggested delay? I've already switched to msleep.
Nope, that seems reasonable (500ms).
Thanks,
Nish
Quoted 3 lines · Nish Aravamudan
The reason I think timers are nicer is that you don't need to nest wait_event*() in a loop (and really, it looks strange when you do). Complicated or not, I think they are the "right thing" for this case.
OK, that makes sense, I have it changed now.
Thanks for the help.
Dave
Quoted 11 lines · David Teigland
When a node is removed from a lockspace, recovery is required for that lockspace on all the remaining lockspace members. Recovery involves: a full rebuild of the distributed resource directory, selecting a new master node for locks/resources previously mastered on the removed node, and rebuilding master-copy locks on newly selected masters.
Signed-off-by Dave Teigland <[email protected]>Signed-off-by Patrick Caulfield <[email protected]>---
[...]
drivers/dlm/rcom.c line 121
Quoted 11 lines
drivers/dlm/rcom.c +11
-0+static void receive_rcom_status (struct dlm_ls *ls,struct dlm_rcom *rc_in) +{ +struct dlm_rcom *rc; +struct dlm_mhandle *mh; +int error, nodeid = rc_in->rc_header.h_nodeid; + + error =create_rcom (ls, nodeid,DLM_RCOM_STATUS_REPLY ,0 , &rc, &mh); + rc->rc_result =make_status (ls); + + error =send_rcom (ls, mh, rc); +}
This last assignment seems a bit pointless since you never use the value stored in error' for anything. Shouldn't you be testing error' at this point and take appropriate action? if not, then why bother assigning the value in the first place. The same comment goes for the assignment a few lines above. Either use the return value or just kill off the local variable `error' alltogether and just call create_rcom() and send_rcom() and throw away the result... I don't know what's appropriate here, but in any case the current code is a bit silly.
Quoted 5 lines
drivers/dlm/rcom.c +5
-0+static void receive_rcom_names (struct dlm_ls *ls,struct dlm_rcom *rc_in) +{ +struct dlm_rcom *rc; +struct dlm_mhandle *mh; +int error, inlen, outlen;
[...]
drivers/dlm/rcom.c line 184
Quoted 7 lines
drivers/dlm/rcom.c +7
-0+ error =create_rcom (ls, nodeid,DLM_RCOM_NAMES_REPLY , outlen, &rc, &mh); + + error =dlm_copy_master_names (ls, rc_in->rc_buf, inlen, rc->rc_buf, + outlen, nodeid); + + error =send_rcom (ls, mh, rc); +}
Some more seemingly pointless assignments of values to `error' that are never used.
Quoted 5 lines
drivers/dlm/rcom.c +5
-0+int dlm_send_rcom_lookup (struct dlm_rsb *r,int dir_nodeid) +{ +struct dlm_rcom *rc; +struct dlm_mhandle *mh; +struct dlm_ls *ls = r->res_ls;
drivers/dlm/rcom.c line 207
Quoted 10 lines
drivers/dlm/rcom.c +10
-0+int error; + + error =create_rcom (ls, dir_nodeid,DLM_RCOM_LOOKUP , r->res_length, + &rc, &mh); +memcpy (rc->rc_buf, r->res_name, r->res_length); + rc->rc_id = (unsigned long ) r; + + error =send_rcom (ls, mh, rc); +return 0 ; +}
Again these assignments to a local `error' variable that's never used.
Quoted 5 lines
drivers/dlm/rcom.c +5
-0+static void receive_rcom_lookup (struct dlm_ls *ls,struct dlm_rcom *rc_in) +{ +struct dlm_rcom *rc; +struct dlm_mhandle *mh; +int error, ret_nodeid, nodeid = rc_in->rc_header.h_nodeid;
drivers/dlm/rcom.c line 223
Quoted 10 lines
drivers/dlm/rcom.c +10
-0+int len = rc_in->rc_header.h_length -sizeof (struct dlm_rcom ); + + error =dlm_dir_lookup (ls, nodeid, rc_in->rc_buf, len, &ret_nodeid); + + error =create_rcom (ls, nodeid,DLM_RCOM_LOOKUP_REPLY ,0 , &rc, &mh); + rc->rc_result = ret_nodeid; + rc->rc_id = rc_in->rc_id; + + error =send_rcom (ls, mh, rc); +}
Yet more error assignments that I don't see the point of. There are more of these, but you can find the rest yourself :)
Don't have time right now to look at more code, so I'll stop here for now...
Signature · 1 line
Jesper Juhl