Re: [PATCH 09/13] libceph: start tracking connection socket state

[Alex Elder <elder@inktank.com>]

On 05/31/2012 11:28 PM, Sage Weil wrote:
> On Wed, 30 May 2012, Alex Elder wrote:
>> Start explicitly keeping track of the state of a ceph connection's
>> socket, separate from the state of the connection itself.  Create
>> placeholder functions to encapsulate the state transitions.
>>
>>      --------
>>      | NEW* |  transient initial state
>>      --------
>>          | con_sock_state_init()
>>          v
>>      ----------
>>      | CLOSED |  initialized, but no socket (and no
>>      ----------  TCP connection)
>>       ^      \
>>       |       \ con_sock_state_connecting()
>>       |        ----------------------
>>       |                              \
>>       + con_sock_state_closed()       \
>>       |\                               \
>>       | \                               \
>>       |  -----------                     \
>>       |  | CLOSING |  socket event;       \
>>       |  -----------  await close          \
>>       |       ^                            |
>>       |       |                            |
>>       |       + con_sock_state_closing()   |
>>       |      / \                           |
>>       |     /   ---------------            |
>>       |    /                   \           v
>>       |   /                    --------------
>>       |  /    -----------------| CONNECTING |  socket created, TCP
>>       |  |   /                 --------------  connect initiated
>>       |  |   | con_sock_state_connected()
>>       |  |   v
>>      -------------
>>      | CONNECTED |  TCP connection established
>>      -------------
>
> Can we put this beautiful pictures in the header next to the states?

I can be quite the ASCII artist.  Yes, I will add this, when I update
the state definitions with better names and numbers.

					-Alex

> Reviewed-by: Sage Weil<sage@inktank.com>
>
>>
>> Make the socket state an atomic variable, reinforcing that it's a
>> distinct transtion with no possible "intermediate/both" states.
>> This is almost certainly overkill at this point, though the
>> transitions into CONNECTED and CLOSING state do get called via
>> socket callback (the rest of the transitions occur with the
>> connection mutex held).  We can back out the atomicity later.
>>
>> Signed-off-by: Alex Elder<elder@inktank.com>
>> ---
>>   include/linux/ceph/messenger.h |    8 ++++-
>>   net/ceph/messenger.c           |   63
>> ++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 69 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/ceph/messenger.h b/include/linux/ceph/messenger.h
>> index 920235e..5e852f4 100644
>> --- a/include/linux/ceph/messenger.h
>> +++ b/include/linux/ceph/messenger.h
>> @@ -137,14 +137,18 @@ struct ceph_connection {
>>   	const struct ceph_connection_operations *ops;
>>
>>   	struct ceph_messenger *msgr;
>> +
>> +	atomic_t sock_state;
>>   	struct socket *sock;
>> +	struct ceph_entity_addr peer_addr; /* peer address */
>> +	struct ceph_entity_addr peer_addr_for_me;
>> +
>>   	unsigned long flags;
>>   	unsigned long state;
>>   	const char *error_msg;  /* error message, if any */
>>
>> -	struct ceph_entity_addr peer_addr; /* peer address */
>>   	struct ceph_entity_name peer_name; /* peer name */
>> -	struct ceph_entity_addr peer_addr_for_me;
>> +
>>   	unsigned peer_features;
>>   	u32 connect_seq;      /* identify the most recent connection
>>   				 attempt for this connection, client */
>> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
>> index 29055df..7e11b07 100644
>> --- a/net/ceph/messenger.c
>> +++ b/net/ceph/messenger.c
>> @@ -29,6 +29,14 @@
>>    * the sender.
>>    */
>>
>> +/* State values for ceph_connection->sock_state; NEW is assumed to be 0 */
>> +
>> +#define CON_SOCK_STATE_NEW		0	/* ->  CLOSED */
>> +#define CON_SOCK_STATE_CLOSED		1	/* ->  CONNECTING */
>> +#define CON_SOCK_STATE_CONNECTING	2	/* ->  CONNECTED or ->  CLOSING
>> */
>> +#define CON_SOCK_STATE_CONNECTED	3	/* ->  CLOSING or ->  CLOSED */
>> +#define CON_SOCK_STATE_CLOSING		4	/* ->  CLOSED */
>> +
>>   /* static tag bytes (protocol control messages) */
>>   static char tag_msg = CEPH_MSGR_TAG_MSG;
>>   static char tag_ack = CEPH_MSGR_TAG_ACK;
>> @@ -147,6 +155,54 @@ void ceph_msgr_flush(void)
>>   }
>>   EXPORT_SYMBOL(ceph_msgr_flush);
>>
>> +/* Connection socket state transition functions */
>> +
>> +static void con_sock_state_init(struct ceph_connection *con)
>> +{
>> +	int old_state;
>> +
>> +	old_state = atomic_xchg(&con->sock_state, CON_SOCK_STATE_CLOSED);
>> +	if (WARN_ON(old_state != CON_SOCK_STATE_NEW))
>> +		printk("%s: unexpected old state %d\n", __func__, old_state);
>> +}
>> +
>> +static void con_sock_state_connecting(struct ceph_connection *con)
>> +{
>> +	int old_state;
>> +
>> +	old_state = atomic_xchg(&con->sock_state, CON_SOCK_STATE_CONNECTING);
>> +	if (WARN_ON(old_state != CON_SOCK_STATE_CLOSED))
>> +		printk("%s: unexpected old state %d\n", __func__, old_state);
>> +}
>> +
>> +static void con_sock_state_connected(struct ceph_connection *con)
>> +{
>> +	int old_state;
>> +
>> +	old_state = atomic_xchg(&con->sock_state, CON_SOCK_STATE_CONNECTED);
>> +	if (WARN_ON(old_state != CON_SOCK_STATE_CONNECTING))
>> +		printk("%s: unexpected old state %d\n", __func__, old_state);
>> +}
>> +
>> +static void con_sock_state_closing(struct ceph_connection *con)
>> +{
>> +	int old_state;
>> +
>> +	old_state = atomic_xchg(&con->sock_state, CON_SOCK_STATE_CLOSING);
>> +	if (WARN_ON(old_state != CON_SOCK_STATE_CONNECTING&&
>> +			old_state != CON_SOCK_STATE_CONNECTED))
>> +		printk("%s: unexpected old state %d\n", __func__, old_state);
>> +}
>> +
>> +static void con_sock_state_closed(struct ceph_connection *con)
>> +{
>> +	int old_state;
>> +
>> +	old_state = atomic_xchg(&con->sock_state, CON_SOCK_STATE_CLOSED);
>> +	if (WARN_ON(old_state != CON_SOCK_STATE_CONNECTED&&
>> +			old_state != CON_SOCK_STATE_CLOSING))
>> +		printk("%s: unexpected old state %d\n", __func__, old_state);
>> +}
>>
>>   /*
>>    * socket callback functions
>> @@ -203,6 +259,7 @@ static void ceph_sock_state_change(struct sock *sk)
>>   		dout("%s TCP_CLOSE\n", __func__);
>>   	case TCP_CLOSE_WAIT:
>>   		dout("%s TCP_CLOSE_WAIT\n", __func__);
>> +		con_sock_state_closing(con);
>>   		if (test_and_set_bit(SOCK_CLOSED,&con->flags) == 0) {
>>   			if (test_bit(CONNECTING,&con->state))
>>   				con->error_msg = "connection failed";
>> @@ -213,6 +270,7 @@ static void ceph_sock_state_change(struct sock *sk)
>>   		break;
>>   	case TCP_ESTABLISHED:
>>   		dout("%s TCP_ESTABLISHED\n", __func__);
>> +		con_sock_state_connected(con);
>>   		queue_con(con);
>>   		break;
>>   	default:	/* Everything else is uninteresting */
>> @@ -277,6 +335,7 @@ static int ceph_tcp_connect(struct ceph_connection *con)
>>   		return ret;
>>   	}
>>   	con->sock = sock;
>> +	con_sock_state_connecting(con);
>>
>>   	return 0;
>>   }
>> @@ -341,6 +400,7 @@ static int con_close_socket(struct ceph_connection *con)
>>   	rc = con->sock->ops->shutdown(con->sock, SHUT_RDWR);
>>   	sock_release(con->sock);
>>   	con->sock = NULL;
>> +	con_sock_state_closed(con);
>>   	return rc;
>>   }
>>
>> @@ -460,6 +520,9 @@ void ceph_con_init(struct ceph_messenger *msgr, struct
>> ceph_connection *con)
>>   	memset(con, 0, sizeof(*con));
>>   	atomic_set(&con->nref, 1);
>>   	con->msgr = msgr;
>> +
>> +	con_sock_state_init(con);
>> +
>>   	mutex_init(&con->mutex);
>>   	INIT_LIST_HEAD(&con->out_queue);
>>   	INIT_LIST_HEAD(&con->out_sent);
>> --
>> 1.7.5.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>