Re: [DLM] Add support for tcp communications [38/70]

[Pavel Machek]

Hi!

> From: Patrick Caulfield <pcaulfie@xxxxxxxxxx>
> Date: Thu, 2 Nov 2006 11:19:21 -0500
> Subject: [PATCH] [DLM] Add support for tcp communications
> 
> The following patch adds a TCP based communications layer
> to the DLM which is compile time selectable. The existing SCTP
> layer gives the advantage of allowing multihoming, whereas
> the TCP layer has been heavily tested in previous versions of
> the DLM and is known to be robust and therefore can be used as
> a baseline for performance testing.
> 
> Signed-off-by: Patrick Caulfield <pcaulfie@xxxxxxxxxx>
> Signed-off-by: Steven Whitehouse <swhiteho@xxxxxxxxxx>

Some comments from coding-style-police...

> +struct cbuf {
> +	unsigned		base;
> +	unsigned		len;
> +	unsigned		mask;
> +};

I'd prefer unsigned int here.

> +struct connection {
> +	struct socket          *sock;
> +	unsigned long		flags;
> +	struct page            *rx_page;
> +	atomic_t		waiting_requests;
> +	struct cbuf		cb;
> +	int                     eagain_flag;
> +};

Quite unusual indentiation style w.r.t. pointers...

> +static struct nodeinfo *nodeid2nodeinfo(int nodeid, gfp_t alloc)
> +{
> +	struct nodeinfo *ni;
> +	int r;
> +	int n;
> +
> +	down_read(&nodeinfo_lock);
> +	ni = idr_find(&nodeinfo_idr, nodeid);
> +	up_read(&nodeinfo_lock);
> +
> +	if (!ni && alloc) {
> +		down_write(&nodeinfo_lock);
> +
> +		ni = idr_find(&nodeinfo_idr, nodeid);
> +		if (ni)
> +			goto out_up;
...
> +		if (nodeid > max_nodeid)
> +			max_nodeid = nodeid;
> +	out_up:
> +		up_write(&nodeinfo_lock);
> +	}
> +
> +	return ni;

Can we do if (ni || !alloc) return ni, and avoid gotos inside of
block?


> +/* Something happened to an association */
> +static void process_sctp_notification(struct msghdr *msg, char *buf)
> +{
> +	union sctp_notification *sn = (union sctp_notification *)buf;
> +
> +	if (sn->sn_header.sn_type == SCTP_ASSOC_CHANGE) {
> +		switch (sn->sn_assoc_change.sac_state) {
> +
> +		case SCTP_COMM_UP:
> +		case SCTP_RESTART:
> +		{
...
> +				/* Retry INIT later */
> +				ni = > assoc2nodeinfo(sn->sn_assoc_change.sac_assoc_id);

Too long lines, and rgetting rid of outer if should be really easy.

> +
> +/* Initiate an SCTP association. In theory we could just use sendmsg() on
> +   the first IP address and it should work, but this allows us to set up the
> +   association before sending any valuable data that we can't afford to lose.
> +   It also keeps the send path clean as it can now always use the association ID */

Too long lines.

> +static void lowcomms_state_change(struct sock *sk)
> +{
> +/*	struct connection *con = sock2con(sk); */

Can we get commented code removed?

> +
> +	switch (sk->sk_state) {
> +	case TCP_ESTABLISHED:
> +		lowcomms_write_space(sk);
> +		break;
> +
> +	case TCP_FIN_WAIT1:
> +	case TCP_FIN_WAIT2:
> +	case TCP_TIME_WAIT:
> +	case TCP_CLOSE:
> +	case TCP_CLOSE_WAIT:
> +	case TCP_LAST_ACK:
> +	case TCP_CLOSING:
> +		/* FIXME: I think this causes more trouble than it solves.
> +		   lowcomms wil reconnect anyway when there is something to
> +		   send. This just attempts reconnection if a node goes down!
> +		*/
> +		/* lowcomms_connect_sock(con); */

Here too? Is the fixme still applicable?

> +{
> +        saddr->ss_family =  dlm_local_addr.ss_family;
> +        if (saddr->ss_family == AF_INET) {
> +		struct sockaddr_in *in4_addr = (struct sockaddr_in *)saddr;
> +		in4_addr->sin_port = cpu_to_be16(port);
> +		*addr_len = sizeof(struct sockaddr_in);
> +	}
> +	else {

} else { on one line, please.

...
> +	if (ret < 0)
> +		goto out_close;
> +	CBUF_EAT(&con->cb, ret);
> +
> +	if (CBUF_EMPTY(&con->cb) && !call_again_soon) {
> +		__free_page(con->rx_page);
> +		con->rx_page = NULL;
> +	}
> +
> +      out:
> +	if (call_again_soon)
> +		goto out_resched;
> +	up_read(&con->sock_sem);
> +	ret = 0;
> +	goto out_ret;

Could we just return 0 here?

> +      out_resched:
> +	lowcomms_data_ready(con->sock->sk, 0);
> +	up_read(&con->sock_sem);
> +	ret = 0;
> +	schedule();
> +	goto out_ret;

And here?

> +      out_close:
> +	up_read(&con->sock_sem);
> +	if (ret != -EAGAIN && !test_bit(CF_IS_OTHERCON, &con->flags)) {
> +		close_connection(con, FALSE);
> +		/* Reconnect when there is something to send */
> +	}
> +
> +      out_ret:
> +	return ret;

> +static struct socket *create_listen_sock(struct connection *con, struct sockaddr_storage *saddr)
> +{
> +        struct socket *sock = NULL;
> +	mm_segment_t fs;
> +	int result = 0;
> +	int one = 1;
> +	int addr_len;

Tabs vs spaces?

> +	result = sock->ops->listen(sock, 5);
> +	if (result < 0) {
> +		printk("dlm: Can't listen on port %d\n", dlm_config.tcp_port);
> +		sock_release(sock);
> +		sock = NULL;
> +		goto create_out;
> +	}
> +
> +      create_out:
> +	return sock;
> +}

I seriously think you are overdoing the gotos.


> +/* Called from recovery when it knows that a node has
> +   left the cluster */
> +int dlm_lowcomms_close(int nodeid)
> +{
> +	struct connection *con;
> +
> +	if (!connections)
> +		goto out;
> +
> +	log_print("closing connection to node %d", nodeid);
> +	con = nodeid2con(nodeid, 0);
> +	if (con) {
> +		clean_one_writequeue(con);
> +		close_connection(con, TRUE);
> +		atomic_set(&con->waiting_requests, 0);
> +	}
> +	return 0;
> +
> +      out:
> +	return -1;
> +}

Could we just return -1 directly? Should it follow 0/-errno convention
other functions here use? Can we get rid of TRUE/FALSE macros?

> +/*
> + * Return the largest buffer size we can cope with.
> + */
> +int lowcomms_max_buffer_size(void)
> +{
> +	return PAGE_CACHE_SIZE;
> +}

Uhuh.

> +/* This is quite likely to sleep... */
> +int dlm_lowcomms_start(void)
> +{
> +	int error = 0;
> +
> +	error = -ENOTCONN;

One statement?

> +	connections = kmalloc(sizeof(struct connection *) *
> +			      NODE_INCREMENT, GFP_KERNEL);
> +	if (!connections)
> +		goto out;
> +
> +	memset(connections, 0,
> +	       sizeof(struct connection *) * NODE_INCREMENT);

kzalloc?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/