[PATCH 00/12] more libceph changes

[PATCH 00/12] more libceph changes

[Alex Elder]

I'm trying to wrap up a lot of work I've done recently on the
Ceph client messenger code.  I had a number of loose ends that
never got committed along the way as I narrowed my focus to
things I discovered in the course of development.

I still have a few more, but here is another batch which are
ready to go, having undergone several hours of testing without
any problems.  They are based on the code resulting from this
series, which I submitted the other day:
    [PATCH 0/6] ceph: a few more messenger cleanups

They're a bit of a random collection, but I've put them into
several sort of related groups in the descriptions below.

					-Alex

[PATCH 01/12] libceph: SOCK_CLOSED is a flag, not a state
    This fixes a bug in which the SOCK_CLOSED bit was erroneously
    being manipulated in a ceph connection's "state" field.

[PATCH 02/12] libceph: don't change socket state on sock event
    This moves the setting of a ceph connection's error_msg
    value out of the socket event handler.

[PATCH 03/12] libceph: just set SOCK_CLOSED when state changes
    This makes it so the SOCK_CLOSED bit is set, rather than
    test-and-set, when the underlying Linux socket has closed
[PATCH 04/12] libceph: don't touch con state in con_close_socket()
    This drops the slightly puzzling setting and clearing of
    the SOCK_CLOSED bit in con_close_socket()

[PATCH 05/12] libceph: clear CONNECTING in ceph_con_close()
    This takes a step toward making the state bits always
    reflect the real state of a connection.
[PATCH 06/12] libceph: clear NEGOTIATING when done
    The NEGOTIATING state was never getting cleared until just
    before a new connection attempt was initiated.
[PATCH 07/12] libceph: define and use an explicit CONNECTED state
    This just defines a state to reflect when a ceph connection
    is fully connected to its peer.

[PATCH 08/12] libceph: separate banner and connect writes
    This splits up how some connection negotiation information
    gets sent when a client initiates a new connect sequence.
[PATCH 09/12] libceph: distinguish two phases of connect sequence
    This makes the banner and IP exchange portion of a connect
    sequence get treated as distinct state (NEGOTIATING), rather
    than being a sort of sub-state of CONNECTING.

[PATCH 10/12] libceph: small changes to messenger.c
    This just gathers a number of minor cleanups.
[PATCH 11/12] libceph: make ceph_con_get() (etc.) private
    This makes ceph_con_get() and ceph_con_put() have static
    scope, since they are only used in "messenger.c".
[PATCH 12/12] libceph: add some fine ASCII art
    This inserts a state diagram for a ceph connection socket
    into comments in "messenger.c".

[PATCH 01/12] libceph: SOCK_CLOSED is a flag, not a state

[Alex Elder]

The following commit changed it so SOCK_CLOSED bit was stored in
a connection's new "flags" field rather than its "state" field.

    libceph: start separating connection flags from state
    commit 928443cd

That bit is used in con_close_socket() to protect against setting an
error message more than once in the socket event handler function.

Unfortunately, the field being operated on in that function was not
updated to be "flags" as it should have been.  This fixes that
error.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 net/ceph/messenger.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: b/net/ceph/messenger.c
===================================================================
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -397,11 +397,11 @@ static int con_close_socket(struct ceph_
 	dout("con_close_socket on %p sock %p\n", con, con->sock);
 	if (!con->sock)
 		return 0;
-	set_bit(SOCK_CLOSED, &con->state);
+	set_bit(SOCK_CLOSED, &con->flags);
 	rc = con->sock->ops->shutdown(con->sock, SHUT_RDWR);
 	sock_release(con->sock);
 	con->sock = NULL;
-	clear_bit(SOCK_CLOSED, &con->state);
+	clear_bit(SOCK_CLOSED, &con->flags);
 	con_sock_state_closed(con);
 	return rc;
 }

[PATCH 02/12] libceph: don't change socket state on sock event

[Alex Elder]

Currently the socket state change event handler records an error
message on a connection to distinguish a close while connecting from
a close while a connection was already established.

Changing connection information during handling of a socket event is
not very clean, so instead move this assignment inside con_work(),
where it can be done during normal connection-level processing (and
under protection of the connection mutex as well).

Move the handling of a socket closed event up to the top of the
processing loop in con_work(); there's no point in handling backoff
etc. if we have a newly-closed socket to take care of.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 net/ceph/messenger.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Index: b/net/ceph/messenger.c
===================================================================
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -261,13 +261,8 @@ static void ceph_sock_state_change(struc
 	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";
-			else
-				con->error_msg = "socket closed";
+		if (!test_and_set_bit(SOCK_CLOSED, &con->flags))
 			queue_con(con);
-		}
 		break;
 	case TCP_ESTABLISHED:
 		dout("%s TCP_ESTABLISHED\n", __func__);
@@ -2212,6 +2207,14 @@ static void con_work(struct work_struct

 	mutex_lock(&con->mutex);
 restart:
+	if (test_and_clear_bit(SOCK_CLOSED, &con->flags)) {
+		if (test_bit(CONNECTING, &con->state))
+			con->error_msg = "connection failed";
+		else
+			con->error_msg = "socket closed";
+		goto fault;
+	}
+
 	if (test_and_clear_bit(BACKOFF, &con->flags)) {
 		dout("con_work %p backing off\n", con);
 		if (queue_delayed_work(ceph_msgr_wq, &con->work,
@@ -2241,9 +2244,6 @@ restart:
 		con_close_socket(con);
 	}

-	if (test_and_clear_bit(SOCK_CLOSED, &con->flags))
-		goto fault;
-
 	ret = try_read(con);
 	if (ret == -EAGAIN)
 		goto restart;

[PATCH 03/12] libceph: just set SOCK_CLOSED when state changes

[Alex Elder]

When a TCP_CLOSE or TCP_CLOSE_WAIT event occurs, the SOCK_CLOSED
connection flag bit is set, and if it had not been previously set
queue_con() is called to ensure con_work() will get a chance to
handle the changed state.

con_work() atomically checks--and if set, clears--the SOCK_CLOSED
bit if it was set.  This means that even if the bit were set
repeatedly, the related processing in con_work() only gets called
once per transition of the bit from 0 to 1.

What's important then is that we ensure con_work() gets called *at
least* once when a socket close event occurs, not that it gets
called *exactly* once.

The work queue mechanism already takes care of queueing work
only if it is not already queued, so there's no need for us
to call queue_con() conditionally.

So this patch just makes it so the SOCK_CLOSED flag gets set
unconditionally in ceph_sock_state_change().

Signed-off-by: Alex Elder <elder@inktank.com>
---
 net/ceph/messenger.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: b/net/ceph/messenger.c
===================================================================
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -261,8 +261,8 @@ static void ceph_sock_state_change(struc
 	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))
-			queue_con(con);
+		set_bit(SOCK_CLOSED, &con->flags);
+		queue_con(con);
 		break;
 	case TCP_ESTABLISHED:
 		dout("%s TCP_ESTABLISHED\n", __func__);

[PATCH 04/12] libceph: don't touch con state in con_close_socket()

[Alex Elder]

In con_close_socket(), a connection's SOCK_CLOSED flag gets set and
then cleared while its shutdown method is called and its reference
gets dropped.

Previously, that flag got set only if it had not already been set,
so setting it in con_close_socket() might have prevented additional
processing being done on a socket being shut down.  We no longer set
SOCK_CLOSED in the socket event routine conditionally, so setting
that bit here no longer provides whatever benefit it might have
provided before.

A race condition could still leave the SOCK_CLOSED bit set even
after we've issued the call to con_close_socket(), so we still clear
that bit after shutting the socket down.  Add a comment explaining
the reason for this.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 net/ceph/messenger.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Index: b/net/ceph/messenger.c
===================================================================
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -392,10 +392,16 @@ static int con_close_socket(struct ceph_
 	dout("con_close_socket on %p sock %p\n", con, con->sock);
 	if (!con->sock)
 		return 0;
-	set_bit(SOCK_CLOSED, &con->flags);
 	rc = con->sock->ops->shutdown(con->sock, SHUT_RDWR);
 	sock_release(con->sock);
 	con->sock = NULL;
+
+	/*
+	 * Forcibly clear the SOCK_CLOSE flag.  It gets set
+	 * independent of the connection mutex, and we could have
+	 * received a socket close event before we had the chance to
+	 * shut the socket down.
+	 */
 	clear_bit(SOCK_CLOSED, &con->flags);
 	con_sock_state_closed(con);
 	return rc;

[PATCH 05/12] libceph: clear CONNECTING in ceph_con_close()

[Alex Elder]

A connection that is closed will no longer be connecting.  So
clear the CONNECTING state bit in ceph_con_close().  Similarly,
if the socket has been closed we no longer are in connecting
state (a new connect sequence will need to be initiated).

Signed-off-by: Alex Elder <elder@inktank.com>
---
 net/ceph/messenger.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: b/net/ceph/messenger.c
===================================================================
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -462,6 +462,7 @@ void ceph_con_close(struct ceph_connecti
 	dout("con_close %p peer %s\n", con,
 	     ceph_pr_addr(&con->peer_addr.in_addr));
 	clear_bit(NEGOTIATING, &con->state);
+	clear_bit(CONNECTING, &con->state);
 	clear_bit(STANDBY, &con->state);  /* avoid connect_seq bump */
 	set_bit(CLOSED, &con->state);

@@ -2214,7 +2215,7 @@ static void con_work(struct work_struct
 	mutex_lock(&con->mutex);
 restart:
 	if (test_and_clear_bit(SOCK_CLOSED, &con->flags)) {
-		if (test_bit(CONNECTING, &con->state))
+		if (test_and_clear_bit(CONNECTING, &con->state))
 			con->error_msg = "connection failed";
 		else
 			con->error_msg = "socket closed";

[PATCH 06/12] libceph: clear NEGOTIATING when done

[Alex Elder]

A connection state's NEGOTIATING bit gets set while in CONNECTING
state after we have successfully exchanged a ceph banner and IP
addresses with the connection's peer (the server).  But that bit
is not cleared again--at least not until another connection attempt
is initiated.

Instead, clear it as soon as the connection is fully established.
Also, clear it when a socket connection gets prematurely closed
in the midst of establishing a ceph connection (in case we had
reached the point where it was set).

Signed-off-by: Alex Elder <elder@inktank.com>
---
 net/ceph/messenger.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Index: b/net/ceph/messenger.c
===================================================================
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -1586,6 +1586,7 @@ static int process_connect(struct ceph_c
 			fail_protocol(con);
 			return -1;
 		}
+		clear_bit(NEGOTIATING, &con->state);
 		clear_bit(CONNECTING, &con->state);
 		con->peer_global_seq = le32_to_cpu(con->in_reply.global_seq);
 		con->connect_seq++;
@@ -1976,7 +1977,6 @@ more:

 	/* open the socket first? */
 	if (con->sock == NULL) {
-		clear_bit(NEGOTIATING, &con->state);
 		set_bit(CONNECTING, &con->state);

 		con_out_kvec_reset(con);
@@ -2215,10 +2215,12 @@ static void con_work(struct work_struct
 	mutex_lock(&con->mutex);
 restart:
 	if (test_and_clear_bit(SOCK_CLOSED, &con->flags)) {
-		if (test_and_clear_bit(CONNECTING, &con->state))
+		if (test_and_clear_bit(CONNECTING, &con->state)) {
+			clear_bit(NEGOTIATING, &con->state);
 			con->error_msg = "connection failed";
-		else
+		} else {
 			con->error_msg = "socket closed";
+		}
 		goto fault;
 	}

[PATCH 07/12] libceph: define and use an explicit CONNECTED state

[Alex Elder]

There is no state explicitly defined when a ceph connection is fully
operational.  So define one.

It's set when the connection sequence completes successfully, and is
cleared when the connection gets closed.

Be a little more careful when examining the old state when a socket
disconnect event is reported.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 include/linux/ceph/messenger.h |    1 +
 net/ceph/messenger.c           |    9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

Index: b/include/linux/ceph/messenger.h
===================================================================
--- a/include/linux/ceph/messenger.h
+++ b/include/linux/ceph/messenger.h
@@ -120,6 +120,7 @@ struct ceph_msg_pos {
  */
 #define CONNECTING	1
 #define NEGOTIATING	2
+#define CONNECTED	5
 #define STANDBY		8  /* no outgoing messages, socket closed.  we keep
 			    * the ceph_connection around to maintain shared
 			    * state with the peer. */
Index: b/net/ceph/messenger.c
===================================================================
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -463,6 +463,7 @@ void ceph_con_close(struct ceph_connecti
 	     ceph_pr_addr(&con->peer_addr.in_addr));
 	clear_bit(NEGOTIATING, &con->state);
 	clear_bit(CONNECTING, &con->state);
+	clear_bit(CONNECTED, &con->state);
 	clear_bit(STANDBY, &con->state);  /* avoid connect_seq bump */
 	set_bit(CLOSED, &con->state);

@@ -1588,6 +1589,7 @@ static int process_connect(struct ceph_c
 		}
 		clear_bit(NEGOTIATING, &con->state);
 		clear_bit(CONNECTING, &con->state);
+		set_bit(CONNECTED, &con->state);
 		con->peer_global_seq = le32_to_cpu(con->in_reply.global_seq);
 		con->connect_seq++;
 		con->peer_features = server_feat;
@@ -2139,6 +2141,7 @@ more:
 			prepare_read_ack(con);
 			break;
 		case CEPH_MSGR_TAG_CLOSE:
+			clear_bit(CONNECTED, &con->state);
 			set_bit(CLOSED, &con->state);   /* fixme */
 			goto out;
 		default:
@@ -2215,11 +2218,13 @@ static void con_work(struct work_struct
 	mutex_lock(&con->mutex);
 restart:
 	if (test_and_clear_bit(SOCK_CLOSED, &con->flags)) {
-		if (test_and_clear_bit(CONNECTING, &con->state)) {
+		if (test_and_clear_bit(CONNECTED, &con->state))
+			con->error_msg = "socket closed";
+		else if (test_and_clear_bit(CONNECTING, &con->state)) {
 			clear_bit(NEGOTIATING, &con->state);
 			con->error_msg = "connection failed";
 		} else {
-			con->error_msg = "socket closed";
+			con->error_msg = "unrecognized con state";
 		}
 		goto fault;
 	}

[PATCH 08/12] libceph: separate banner and connect writes

[Alex Elder]

There are two phases in the process of linking together the two ends
of a ceph connection.  The first involves exchanging a banner and
IP addresses, and if that is successful a second phase exchanges
some detail about each side's connection capabilities.

When initiating a connection, the client side now queues to send
its information for both phases of this process at the same time.
This is probably a bit more efficient, but it is slightly messier
from a layering perspective in the code.

So rearrange things so that the client doesn't send the connection
information until it has received and processed the response in the
initial banner phase (in process_banner()).

Move the code (in the (con->sock == NULL) case in try_write()) that
prepares for writing the connection information, delaying doing that
until the banner exchange has completed.  Move the code that begins
the transition to this second "NEGOTIATING" phase out of
process_banner() and into its caller, so preparing to write the
connection information and preparing to read the response are
adjacent to each other.

Finally, preparing to write the connection information now requires
the output kvec to be reset in all cases, so move that into the
prepare_write_connect() and delete it from all callers.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 net/ceph/messenger.c |   20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

Index: b/net/ceph/messenger.c
===================================================================
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -865,6 +865,7 @@ static int prepare_write_connect(struct
 	con->out_connect.authorizer_len = auth ?
 		cpu_to_le32(auth->authorizer_buf_len) : 0;

+	con_out_kvec_reset(con);
 	con_out_kvec_add(con, sizeof (con->out_connect),
 					&con->out_connect);
 	if (auth && auth->authorizer_buf_len)
@@ -1454,8 +1455,6 @@ static int process_banner(struct ceph_co
 		     ceph_pr_addr(&con->msgr->inst.addr.in_addr));
 	}

-	set_bit(NEGOTIATING, &con->state);
-	prepare_read_connect(con);
 	return 0;
 }

@@ -1505,7 +1504,6 @@ static int process_connect(struct ceph_c
 			return -1;
 		}
 		con->auth_retry = 1;
-		con_out_kvec_reset(con);
 		ret = prepare_write_connect(con);
 		if (ret < 0)
 			return ret;
@@ -1526,7 +1524,6 @@ static int process_connect(struct ceph_c
 		       ENTITY_NAME(con->peer_name),
 		       ceph_pr_addr(&con->peer_addr.in_addr));
 		reset_connection(con);
-		con_out_kvec_reset(con);
 		ret = prepare_write_connect(con);
 		if (ret < 0)
 			return ret;
@@ -1552,7 +1549,6 @@ static int process_connect(struct ceph_c
 		     le32_to_cpu(con->out_connect.connect_seq),
 		     le32_to_cpu(con->in_connect.connect_seq));
 		con->connect_seq = le32_to_cpu(con->in_connect.connect_seq);
-		con_out_kvec_reset(con);
 		ret = prepare_write_connect(con);
 		if (ret < 0)
 			return ret;
@@ -1569,7 +1565,6 @@ static int process_connect(struct ceph_c
 		     le32_to_cpu(con->in_connect.global_seq));
 		get_global_seq(con->msgr,
 			       le32_to_cpu(con->in_connect.global_seq));
-		con_out_kvec_reset(con);
 		ret = prepare_write_connect(con);
 		if (ret < 0)
 			return ret;
@@ -1983,9 +1978,6 @@ more:

 		con_out_kvec_reset(con);
 		prepare_write_banner(con);
-		ret = prepare_write_connect(con);
-		if (ret < 0)
-			goto out;
 		prepare_read_banner(con);

 		BUG_ON(con->in_msg);
@@ -2098,6 +2090,16 @@ more:
 			ret = process_banner(con);
 			if (ret < 0)
 				goto out;
+
+			/* Banner is good, exchange connection info */
+			ret = prepare_write_connect(con);
+			if (ret < 0)
+				goto out;
+			prepare_read_connect(con);
+			set_bit(NEGOTIATING, &con->state);
+
+			/* Send connection info before awaiting response */
+			goto out;
 		}
 		ret = read_partial_connect(con);
 		if (ret <= 0)

[PATCH 09/12] libceph: distinguish two phases of connect sequence

[Alex Elder]

Currently a ceph connection enters a "CONNECTING" state when it
begins the process of (re-)connecting with its peer.  Once the two
ends have successfully exchanged their banner and addresses, an
additional NEGOTIATING bit is set in the ceph connection's state to
indicate the connection information exhange has begun.  The
CONNECTING bit/state continues to be set during this phase.

Rather than have the CONNECTING state continue while the NEGOTIATING
bit is set, interpret these two phases as distinct states.  In other
words, when NEGOTIATING is set, clear CONNECTING.  That way only
one of them will be active at a time.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 net/ceph/messenger.c |   48
++++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 22 deletions(-)

Index: b/net/ceph/messenger.c
===================================================================
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -1583,7 +1583,6 @@ static int process_connect(struct ceph_c
 			return -1;
 		}
 		clear_bit(NEGOTIATING, &con->state);
-		clear_bit(CONNECTING, &con->state);
 		set_bit(CONNECTED, &con->state);
 		con->peer_global_seq = le32_to_cpu(con->in_reply.global_seq);
 		con->connect_seq++;
@@ -2025,7 +2024,8 @@ more_kvec:
 	}

 do_next:
-	if (!test_bit(CONNECTING, &con->state)) {
+	if (!test_bit(CONNECTING, &con->state) &&
+			!test_bit(NEGOTIATING, &con->state)) {
 		/* is anything else pending? */
 		if (!list_empty(&con->out_queue)) {
 			prepare_write_message(con);
@@ -2082,25 +2082,29 @@ more:
 	}

 	if (test_bit(CONNECTING, &con->state)) {
-		if (!test_bit(NEGOTIATING, &con->state)) {
-			dout("try_read connecting\n");
-			ret = read_partial_banner(con);
-			if (ret <= 0)
-				goto out;
-			ret = process_banner(con);
-			if (ret < 0)
-				goto out;
+		dout("try_read connecting\n");
+		ret = read_partial_banner(con);
+		if (ret <= 0)
+			goto out;
+		ret = process_banner(con);
+		if (ret < 0)
+			goto out;

-			/* Banner is good, exchange connection info */
-			ret = prepare_write_connect(con);
-			if (ret < 0)
-				goto out;
-			prepare_read_connect(con);
-			set_bit(NEGOTIATING, &con->state);
+		clear_bit(CONNECTING, &con->state);
+		set_bit(NEGOTIATING, &con->state);

-			/* Send connection info before awaiting response */
+		/* Banner is good, exchange connection info */
+		ret = prepare_write_connect(con);
+		if (ret < 0)
 			goto out;
-		}
+		prepare_read_connect(con);
+
+		/* Send connection info before awaiting response */
+		goto out;
+	}
+
+	if (test_bit(NEGOTIATING, &con->state)) {
+		dout("try_read negotiating\n");
 		ret = read_partial_connect(con);
 		if (ret <= 0)
 			goto out;
@@ -2222,12 +2226,12 @@ restart:
 	if (test_and_clear_bit(SOCK_CLOSED, &con->flags)) {
 		if (test_and_clear_bit(CONNECTED, &con->state))
 			con->error_msg = "socket closed";
-		else if (test_and_clear_bit(CONNECTING, &con->state)) {
-			clear_bit(NEGOTIATING, &con->state);
+		else if (test_and_clear_bit(NEGOTIATING, &con->state))
+			con->error_msg = "negotiation failed";
+		else if (test_and_clear_bit(CONNECTING, &con->state))
 			con->error_msg = "connection failed";
-		} else {
+		else
 			con->error_msg = "unrecognized con state";
-		}
 		goto fault;
 	}

Re: [PATCH 09/12] libceph: distinguish two phases of connect sequence

[Sage Weil]

I get the feeling that all of these lists of clear/set_bit calls would go 
away if we

- verify we are under the mutex at each of these sites
- replace con->state with an enum

Is there a reason you stopped short of doing that (besides time)?

sage


On Thu, 21 Jun 2012, Alex Elder wrote:

> Currently a ceph connection enters a "CONNECTING" state when it
> begins the process of (re-)connecting with its peer.  Once the two
> ends have successfully exchanged their banner and addresses, an
> additional NEGOTIATING bit is set in the ceph connection's state to
> indicate the connection information exhange has begun.  The
> CONNECTING bit/state continues to be set during this phase.
> 
> Rather than have the CONNECTING state continue while the NEGOTIATING
> bit is set, interpret these two phases as distinct states.  In other
> words, when NEGOTIATING is set, clear CONNECTING.  That way only
> one of them will be active at a time.
> 
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>  net/ceph/messenger.c |   48
> ++++++++++++++++++++++++++----------------------
>  1 file changed, 26 insertions(+), 22 deletions(-)
> 
> Index: b/net/ceph/messenger.c
> ===================================================================
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -1583,7 +1583,6 @@ static int process_connect(struct ceph_c
>  			return -1;
>  		}
>  		clear_bit(NEGOTIATING, &con->state);
> -		clear_bit(CONNECTING, &con->state);
>  		set_bit(CONNECTED, &con->state);
>  		con->peer_global_seq = le32_to_cpu(con->in_reply.global_seq);
>  		con->connect_seq++;
> @@ -2025,7 +2024,8 @@ more_kvec:
>  	}
> 
>  do_next:
> -	if (!test_bit(CONNECTING, &con->state)) {
> +	if (!test_bit(CONNECTING, &con->state) &&
> +			!test_bit(NEGOTIATING, &con->state)) {
>  		/* is anything else pending? */
>  		if (!list_empty(&con->out_queue)) {
>  			prepare_write_message(con);
> @@ -2082,25 +2082,29 @@ more:
>  	}
> 
>  	if (test_bit(CONNECTING, &con->state)) {
> -		if (!test_bit(NEGOTIATING, &con->state)) {
> -			dout("try_read connecting\n");
> -			ret = read_partial_banner(con);
> -			if (ret <= 0)
> -				goto out;
> -			ret = process_banner(con);
> -			if (ret < 0)
> -				goto out;
> +		dout("try_read connecting\n");
> +		ret = read_partial_banner(con);
> +		if (ret <= 0)
> +			goto out;
> +		ret = process_banner(con);
> +		if (ret < 0)
> +			goto out;
> 
> -			/* Banner is good, exchange connection info */
> -			ret = prepare_write_connect(con);
> -			if (ret < 0)
> -				goto out;
> -			prepare_read_connect(con);
> -			set_bit(NEGOTIATING, &con->state);
> +		clear_bit(CONNECTING, &con->state);
> +		set_bit(NEGOTIATING, &con->state);
> 
> -			/* Send connection info before awaiting response */
> +		/* Banner is good, exchange connection info */
> +		ret = prepare_write_connect(con);
> +		if (ret < 0)
>  			goto out;
> -		}
> +		prepare_read_connect(con);
> +
> +		/* Send connection info before awaiting response */
> +		goto out;
> +	}
> +
> +	if (test_bit(NEGOTIATING, &con->state)) {
> +		dout("try_read negotiating\n");
>  		ret = read_partial_connect(con);
>  		if (ret <= 0)
>  			goto out;
> @@ -2222,12 +2226,12 @@ restart:
>  	if (test_and_clear_bit(SOCK_CLOSED, &con->flags)) {
>  		if (test_and_clear_bit(CONNECTED, &con->state))
>  			con->error_msg = "socket closed";
> -		else if (test_and_clear_bit(CONNECTING, &con->state)) {
> -			clear_bit(NEGOTIATING, &con->state);
> +		else if (test_and_clear_bit(NEGOTIATING, &con->state))
> +			con->error_msg = "negotiation failed";
> +		else if (test_and_clear_bit(CONNECTING, &con->state))
>  			con->error_msg = "connection failed";
> -		} else {
> +		else
>  			con->error_msg = "unrecognized con state";
> -		}
>  		goto fault;
>  	}
> 
> --
> 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
> 
> 

Re: [PATCH 09/12] libceph: distinguish two phases of connect sequence

[Alex Elder]

On 06/21/2012 01:44 PM, Sage Weil wrote:
> I get the feeling that all of these lists of clear/set_bit calls would go 
> away if we
> 
> - verify we are under the mutex at each of these sites
> - replace con->state with an enum
> 
> Is there a reason you stopped short of doing that (besides time)?

Time.

Where I was/am headed is to have functions just like I did with
the connection socket states, in which a function would contain
the state change itself and would verify we're transitioning
from a valid previous state.

I also have been working toward having distinct, non-overlapping
states, which has not really been the case here.

But my approach has been to do all this incrementally, so at each
step along the way it is easy to understand that a particular
change is doing the right thing, and at with each change we can
do regression tests to verify nothing is broken as a result.

I probably have another dozen or more small patches that bring
it even closer, but at this point I'll probably try to trickle
them in as I get little blocks of time.  They need to be looked
at again individually to make sure they still make sense before
I send them out for review.

					-Alex

> sage
> 
> 
> On Thu, 21 Jun 2012, Alex Elder wrote:
> 
>> Currently a ceph connection enters a "CONNECTING" state when it
>> begins the process of (re-)connecting with its peer.  Once the two
>> ends have successfully exchanged their banner and addresses, an
>> additional NEGOTIATING bit is set in the ceph connection's state to
>> indicate the connection information exhange has begun.  The
>> CONNECTING bit/state continues to be set during this phase.
>>
>> Rather than have the CONNECTING state continue while the NEGOTIATING
>> bit is set, interpret these two phases as distinct states.  In other
>> words, when NEGOTIATING is set, clear CONNECTING.  That way only
>> one of them will be active at a time.
>>
>> Signed-off-by: Alex Elder <elder@inktank.com>
>> ---
>>  net/ceph/messenger.c |   48
>> ++++++++++++++++++++++++++----------------------
>>  1 file changed, 26 insertions(+), 22 deletions(-)
>>
>> Index: b/net/ceph/messenger.c
>> ===================================================================
>> --- a/net/ceph/messenger.c
>> +++ b/net/ceph/messenger.c
>> @@ -1583,7 +1583,6 @@ static int process_connect(struct ceph_c
>>  			return -1;
>>  		}
>>  		clear_bit(NEGOTIATING, &con->state);
>> -		clear_bit(CONNECTING, &con->state);
>>  		set_bit(CONNECTED, &con->state);
>>  		con->peer_global_seq = le32_to_cpu(con->in_reply.global_seq);
>>  		con->connect_seq++;
>> @@ -2025,7 +2024,8 @@ more_kvec:
>>  	}
>>
>>  do_next:
>> -	if (!test_bit(CONNECTING, &con->state)) {
>> +	if (!test_bit(CONNECTING, &con->state) &&
>> +			!test_bit(NEGOTIATING, &con->state)) {
>>  		/* is anything else pending? */
>>  		if (!list_empty(&con->out_queue)) {
>>  			prepare_write_message(con);
>> @@ -2082,25 +2082,29 @@ more:
>>  	}
>>
>>  	if (test_bit(CONNECTING, &con->state)) {
>> -		if (!test_bit(NEGOTIATING, &con->state)) {
>> -			dout("try_read connecting\n");
>> -			ret = read_partial_banner(con);
>> -			if (ret <= 0)
>> -				goto out;
>> -			ret = process_banner(con);
>> -			if (ret < 0)
>> -				goto out;
>> +		dout("try_read connecting\n");
>> +		ret = read_partial_banner(con);
>> +		if (ret <= 0)
>> +			goto out;
>> +		ret = process_banner(con);
>> +		if (ret < 0)
>> +			goto out;
>>
>> -			/* Banner is good, exchange connection info */
>> -			ret = prepare_write_connect(con);
>> -			if (ret < 0)
>> -				goto out;
>> -			prepare_read_connect(con);
>> -			set_bit(NEGOTIATING, &con->state);
>> +		clear_bit(CONNECTING, &con->state);
>> +		set_bit(NEGOTIATING, &con->state);
>>
>> -			/* Send connection info before awaiting response */
>> +		/* Banner is good, exchange connection info */
>> +		ret = prepare_write_connect(con);
>> +		if (ret < 0)
>>  			goto out;
>> -		}
>> +		prepare_read_connect(con);
>> +
>> +		/* Send connection info before awaiting response */
>> +		goto out;
>> +	}
>> +
>> +	if (test_bit(NEGOTIATING, &con->state)) {
>> +		dout("try_read negotiating\n");
>>  		ret = read_partial_connect(con);
>>  		if (ret <= 0)
>>  			goto out;
>> @@ -2222,12 +2226,12 @@ restart:
>>  	if (test_and_clear_bit(SOCK_CLOSED, &con->flags)) {
>>  		if (test_and_clear_bit(CONNECTED, &con->state))
>>  			con->error_msg = "socket closed";
>> -		else if (test_and_clear_bit(CONNECTING, &con->state)) {
>> -			clear_bit(NEGOTIATING, &con->state);
>> +		else if (test_and_clear_bit(NEGOTIATING, &con->state))
>> +			con->error_msg = "negotiation failed";
>> +		else if (test_and_clear_bit(CONNECTING, &con->state))
>>  			con->error_msg = "connection failed";
>> -		} else {
>> +		else
>>  			con->error_msg = "unrecognized con state";
>> -		}
>>  		goto fault;
>>  	}
>>
>> --
>> 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
>>
>>
> 
> 

[PATCH 10/12] libceph: small changes to messenger.c

[Alex Elder]

This patch gathers a few small changes in "net/ceph/messenger.c":
  out_msg_pos_next()
    - small logic change that mostly affects indentation
  write_partial_msg_pages().
    - use a local variable trail_off to represent the offset into
      a message of the trail portion of the data (if present)
    - once we are in the trail portion we will always be there, so we
      don't always need to check against our data position
    - avoid computing len twice after we've reached the trail
    - get rid of the variable tmpcrc, which is not needed
    - trail_off and trail_len never change so mark them const
    - update some comments
  read_partial_message_bio()
    - bio_iovec_idx() will never return an error, so don't bother
      checking for it

Signed-off-by: Alex Elder <elder@inktank.com>
---
 net/ceph/messenger.c |   63
+++++++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 32 deletions(-)

Index: b/net/ceph/messenger.c
===================================================================
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -931,21 +931,23 @@ static void out_msg_pos_next(struct ceph

 	con->out_msg_pos.data_pos += sent;
 	con->out_msg_pos.page_pos += sent;
-	if (sent == len) {
-		con->out_msg_pos.page_pos = 0;
-		con->out_msg_pos.page++;
-		con->out_msg_pos.did_page_crc = false;
-		if (in_trail)
-			list_move_tail(&page->lru,
-				       &msg->trail->head);
-		else if (msg->pagelist)
-			list_move_tail(&page->lru,
-				       &msg->pagelist->head);
+	if (sent < len)
+		return;
+
+	BUG_ON(sent != len);
+	con->out_msg_pos.page_pos = 0;
+	con->out_msg_pos.page++;
+	con->out_msg_pos.did_page_crc = false;
+	if (in_trail)
+		list_move_tail(&page->lru,
+			       &msg->trail->head);
+	else if (msg->pagelist)
+		list_move_tail(&page->lru,
+			       &msg->pagelist->head);
 #ifdef CONFIG_BLOCK
-		else if (msg->bio)
-			iter_bio_next(&msg->bio_iter, &msg->bio_seg);
+	else if (msg->bio)
+		iter_bio_next(&msg->bio_iter, &msg->bio_seg);
 #endif
-	}
 }

 /*
@@ -964,30 +966,31 @@ static int write_partial_msg_pages(struc
 	int ret;
 	int total_max_write;
 	bool in_trail = false;
-	size_t trail_len = (msg->trail ? msg->trail->length : 0);
+	const size_t trail_len = (msg->trail ? msg->trail->length : 0);
+	const size_t trail_off = data_len - trail_len;

 	dout("write_partial_msg_pages %p msg %p page %d/%d offset %d\n",
 	     con, msg, con->out_msg_pos.page, msg->nr_pages,
 	     con->out_msg_pos.page_pos);

+	/*
+	 * Iterate through each page that contains data to be
+	 * written, and send as much as possible for each.
+	 *
+	 * If we are calculating the data crc (the default), we will
+	 * need to map the page.  If we have no pages, they have
+	 * been revoked, so use the zero page.
+	 */
 	while (data_len > con->out_msg_pos.data_pos) {
 		struct page *page = NULL;
 		int max_write = PAGE_SIZE;
 		int bio_offset = 0;

-		total_max_write = data_len - trail_len -
-			con->out_msg_pos.data_pos;
-
-		/*
-		 * if we are calculating the data crc (the default), we need
-		 * to map the page.  if our pages[] has been revoked, use the
-		 * zero page.
-		 */
-
-		/* have we reached the trail part of the data? */
-		if (con->out_msg_pos.data_pos >= data_len - trail_len) {
-			in_trail = true;
+		in_trail = in_trail || con->out_msg_pos.data_pos >= trail_off;
+		if (!in_trail)
+			total_max_write = trail_off - con->out_msg_pos.data_pos;

+		if (in_trail) {
 			total_max_write = data_len - con->out_msg_pos.data_pos;

 			page = list_first_entry(&msg->trail->head,
@@ -1014,14 +1017,13 @@ static int write_partial_msg_pages(struc

 		if (do_datacrc && !con->out_msg_pos.did_page_crc) {
 			void *base;
-			u32 crc;
-			u32 tmpcrc = le32_to_cpu(msg->footer.data_crc);
+			u32 crc = le32_to_cpu(msg->footer.data_crc);
 			char *kaddr;

 			kaddr = kmap(page);
 			BUG_ON(kaddr == NULL);
 			base = kaddr + con->out_msg_pos.page_pos + bio_offset;
-			crc = crc32c(tmpcrc, base, len);
+			crc = crc32c(crc, base, len);
 			msg->footer.data_crc = cpu_to_le32(crc);
 			con->out_msg_pos.did_page_crc = true;
 		}
@@ -1726,9 +1728,6 @@ static int read_partial_message_bio(stru
 	void *p;
 	int ret, left;

-	if (IS_ERR(bv))
-		return PTR_ERR(bv);
-
 	left = min((int)(data_len - con->in_msg_pos.data_pos),
 		   (int)(bv->bv_len - con->in_msg_pos.page_pos));

[PATCH 11/12] libceph: make ceph_con_get() (etc.) private

[Alex Elder]

The functions ceph_con_get() and ceph_con_put() are both only ever
used in "net/ceph/messenger.c", so change them to have static scope.
Move their definition up in the source file so they're both defined
before their first use.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 include/linux/ceph/messenger.h |    2 -
 net/ceph/messenger.c           |   48
++++++++++++++++++++---------------------
 2 files changed, 24 insertions(+), 26 deletions(-)

Index: b/include/linux/ceph/messenger.h
===================================================================
--- a/include/linux/ceph/messenger.h
+++ b/include/linux/ceph/messenger.h
@@ -245,8 +245,6 @@ extern void ceph_msg_revoke(struct ceph_
 extern void ceph_msg_revoke_incoming(struct ceph_msg *msg);

 extern void ceph_con_keepalive(struct ceph_connection *con);
-extern struct ceph_connection *ceph_con_get(struct ceph_connection *con);
-extern void ceph_con_put(struct ceph_connection *con);

 extern struct ceph_msg *ceph_msg_new(int type, int front_len, gfp_t flags,
 				     bool can_fail);
Index: b/net/ceph/messenger.c
===================================================================
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -408,6 +408,30 @@ static int con_close_socket(struct ceph_
 }

 /*
+ * generic get/put
+ */
+static struct ceph_connection *ceph_con_get(struct ceph_connection *con)
+{
+	int nref = __atomic_add_unless(&con->nref, 1, 0);
+
+	dout("con_get %p nref = %d -> %d\n", con, nref, nref + 1);
+
+	return nref ? con : NULL;
+}
+
+static void ceph_con_put(struct ceph_connection *con)
+{
+	int nref = atomic_dec_return(&con->nref);
+
+	BUG_ON(nref < 0);
+	if (nref == 0) {
+		BUG_ON(con->sock);
+		kfree(con);
+	}
+	dout("con_put %p nref = %d -> %d\n", con, nref + 1, nref);
+}
+
+/*
  * Reset a connection.  Discard all incoming and outgoing messages
  * and clear *_seq state.
  */
@@ -504,30 +528,6 @@ bool ceph_con_opened(struct ceph_connect
 }

 /*
- * generic get/put
- */
-struct ceph_connection *ceph_con_get(struct ceph_connection *con)
-{
-	int nref = __atomic_add_unless(&con->nref, 1, 0);
-
-	dout("con_get %p nref = %d -> %d\n", con, nref, nref + 1);
-
-	return nref ? con : NULL;
-}
-
-void ceph_con_put(struct ceph_connection *con)
-{
-	int nref = atomic_dec_return(&con->nref);
-
-	BUG_ON(nref < 0);
-	if (nref == 0) {
-		BUG_ON(con->sock);
-		kfree(con);
-	}
-	dout("con_put %p nref = %d -> %d\n", con, nref + 1, nref);
-}
-
-/*
  * initialize a new connection.
  */
 void ceph_con_init(struct ceph_connection *con, void *private,

Re: [PATCH 11/12] libceph: make ceph_con_get() (etc.) private

[Sage Weil]

These should actually go away entirely; pushed a couple patches that do 
that, and remove the now-unused con->nref member.

sage

On Thu, 21 Jun 2012, Alex Elder wrote:

> The functions ceph_con_get() and ceph_con_put() are both only ever
> used in "net/ceph/messenger.c", so change them to have static scope.
> Move their definition up in the source file so they're both defined
> before their first use.
> 
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
>  include/linux/ceph/messenger.h |    2 -
>  net/ceph/messenger.c           |   48
> ++++++++++++++++++++---------------------
>  2 files changed, 24 insertions(+), 26 deletions(-)
> 
> Index: b/include/linux/ceph/messenger.h
> ===================================================================
> --- a/include/linux/ceph/messenger.h
> +++ b/include/linux/ceph/messenger.h
> @@ -245,8 +245,6 @@ extern void ceph_msg_revoke(struct ceph_
>  extern void ceph_msg_revoke_incoming(struct ceph_msg *msg);
> 
>  extern void ceph_con_keepalive(struct ceph_connection *con);
> -extern struct ceph_connection *ceph_con_get(struct ceph_connection *con);
> -extern void ceph_con_put(struct ceph_connection *con);
> 
>  extern struct ceph_msg *ceph_msg_new(int type, int front_len, gfp_t flags,
>  				     bool can_fail);
> Index: b/net/ceph/messenger.c
> ===================================================================
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -408,6 +408,30 @@ static int con_close_socket(struct ceph_
>  }
> 
>  /*
> + * generic get/put
> + */
> +static struct ceph_connection *ceph_con_get(struct ceph_connection *con)
> +{
> +	int nref = __atomic_add_unless(&con->nref, 1, 0);
> +
> +	dout("con_get %p nref = %d -> %d\n", con, nref, nref + 1);
> +
> +	return nref ? con : NULL;
> +}
> +
> +static void ceph_con_put(struct ceph_connection *con)
> +{
> +	int nref = atomic_dec_return(&con->nref);
> +
> +	BUG_ON(nref < 0);
> +	if (nref == 0) {
> +		BUG_ON(con->sock);
> +		kfree(con);
> +	}
> +	dout("con_put %p nref = %d -> %d\n", con, nref + 1, nref);
> +}
> +
> +/*
>   * Reset a connection.  Discard all incoming and outgoing messages
>   * and clear *_seq state.
>   */
> @@ -504,30 +528,6 @@ bool ceph_con_opened(struct ceph_connect
>  }
> 
>  /*
> - * generic get/put
> - */
> -struct ceph_connection *ceph_con_get(struct ceph_connection *con)
> -{
> -	int nref = __atomic_add_unless(&con->nref, 1, 0);
> -
> -	dout("con_get %p nref = %d -> %d\n", con, nref, nref + 1);
> -
> -	return nref ? con : NULL;
> -}
> -
> -void ceph_con_put(struct ceph_connection *con)
> -{
> -	int nref = atomic_dec_return(&con->nref);
> -
> -	BUG_ON(nref < 0);
> -	if (nref == 0) {
> -		BUG_ON(con->sock);
> -		kfree(con);
> -	}
> -	dout("con_put %p nref = %d -> %d\n", con, nref + 1, nref);
> -}
> -
> -/*
>   * initialize a new connection.
>   */
>  void ceph_con_init(struct ceph_connection *con, void *private,
> --
> 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
> 
> 

[PATCH 12/12] libceph: add some fine ASCII art

[Alex Elder]

Sage liked the state diagram I put in my commit description so
I'm putting it in with the code.

Signed-off-by: Alex Elder <elder@inktank.com>
---
 net/ceph/messenger.c |   42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

Index: b/net/ceph/messenger.c
===================================================================
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -29,7 +29,47 @@
  * the sender.
  */

-/* State values for ceph_connection->sock_state; NEW is assumed to be 0 */
+/*
+ * We track the state of the socket on a given connection using
+ * values defined below.  The transition to a new socket state is
+ * handled by a function which verifies we aren't coming from an
+ * unexpected state.
+ *
+ *      --------
+ *      | 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
+ *      -------------
+ *
+ * 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 */