Skip to contentSkip to conversation

dlm: recovery

PATCH In review David Teigland 3 participants 6 messages Apr 25, 2005 – Apr 27, 2005

lore
Share & tools
lore
#0 lore raw

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__ */
+
#1 lore raw
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.

<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

#5 lore raw
In reply to David Teigland
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.

---

[...]

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
#2 lore raw
In reply to Nish Aravamudan
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 not test_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_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

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 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.

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

#3 lore raw
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_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

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 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.

Quoted 6 lines

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;

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_timeout is 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

#4 lore raw
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