From mboxrd@z Thu Jan  1 00:00:00 1970
From: Bob Peterson <rpeterso@redhat.com>
Date: Mon, 19 Dec 2011 11:15:51 -0500 (EST)
Subject: [Cluster-devel] [PATCH 3/5] dlm: add node slots and generation
In-Reply-To: <1324072991-30729-4-git-send-email-teigland@redhat.com>
Message-ID: <734c9d9e-b58e-46b3-b1fb-122c721a59d6@zmail16.collab.prod.int.phx2.redhat.com>
List-Id: <cluster-devel.redhat.com>
To: cluster-devel.redhat.com
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit

----- Original Message -----
| Slot numbers are assigned to nodes when they join the lockspace.
| The slot number chosen is the minimum unused value starting at 1.
| Once a node is assigned a slot, that slot number will not change
| while the node remains a lockspace member.  If the node leaves
| and rejoins it can be assigned a new slot number.
| 
| A new generation number is also added to a lockspace.  It is
| set and incremented during each recovery along with the slot
| collection/assignment.
| 
| The slot numbers will be passed to gfs2 which will use them as
| journal id's.
| 
| Signed-off-by: David Teigland <teigland@redhat.com>
| ---
|  fs/dlm/dlm_internal.h |   48 ++++++++-
|  fs/dlm/lockspace.c    |    5 +
|  fs/dlm/member.c       |  274
|  ++++++++++++++++++++++++++++++++++++++++++++++++-
|  fs/dlm/member.h       |    7 ++
|  fs/dlm/rcom.c         |   99 +++++++++++++++---
|  fs/dlm/rcom.h         |    2 +-
|  fs/dlm/recover.c      |   64 ++++++++++--
|  7 files changed, 470 insertions(+), 29 deletions(-)
(snip)
| +int dlm_slots_assign(struct dlm_ls *ls, int *num_slots, int
(snip)
| +			memb->slot = i+1;

Nit, but this should have some spaces, iow, "i + 1;"

| @@ -161,8 +180,11 @@ int dlm_rcom_status(struct dlm_ls *ls, int
| nodeid)
|  		/* we pretend the remote lockspace exists with 0 status */
|  		log_debug(ls, "remote node %d not ready", nodeid);
|  		rc->rc_result = 0;
| -	} else
| -		error = check_config(ls, rc, nodeid);
| +		goto out;
| +	}
| +
| +	error = check_rcom_config(ls, rc, nodeid);
| +
|  	/* the caller looks at rc_result for the remote recovery status */
|   out:
|  	return error;

IMHO, this is messy and I don't see the point, other than to change the
function name. Maybe I'm not following it, but adding the goto only
seems to unnecessarily complicate things. My source is a bit outdated, but
it looks to me like you can accomplish the same thing with:

  		/* we pretend the remote lockspace exists with 0 status */
  		log_debug(ls, "remote node %d not ready", nodeid);
  		rc->rc_result = 0;
 	} else
 -		error = check_config(ls, rc, nodeid);
 +		error = check_rcom_config(ls, rc, nodeid);
 +
  	/* the caller looks at rc_result for the remote recovery status */
   out:
  	return error;

Other than that, ACK.

Regards,

Bob Peterson
Red Hat File Systems