more libceph changes

PATCH In review Alex Elder 2 participants 16 messages Jun 21, 2012 Ceph common code (libceph)

Share & tools

Alex Elder

author

2012-06-21 14:15 UTC #0 lore raw

cover more libceph changes

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

Alex Elder

author

2012-06-21 14:26 UTC #1 lore raw

PATCH 1/12 libceph: SOCK_CLOSED is a flag, not a state

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 <[email protected]>

---
 net/ceph/messenger.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

net/ceph/messenger.c +2 -2

Index: 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;
 }

Alex Elder

author

2012-06-21 14:21 UTC #2 lore raw

PATCH 2/12 libceph: don't change socket state on sock event

In reply to 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 <[email protected]>

---
 net/ceph/messenger.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

net/ceph/messenger.c +9 -9

Index: 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;

Alex Elder

author

2012-06-21 14:21 UTC #3 lore raw

PATCH 3/12 libceph: just set SOCK_CLOSED when state changes

In reply to 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 <[email protected]>

---
 net/ceph/messenger.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

net/ceph/messenger.c +2 -2

Index: 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__);

Alex Elder

author

2012-06-21 14:27 UTC #4 lore raw

PATCH 4/12 libceph: don't touch con state in con_close_socket()

In reply to 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 <[email protected]>

---
 net/ceph/messenger.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

net/ceph/messenger.c +7 -1

Index: 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;

Alex Elder

author

2012-06-21 14:21 UTC #5 lore raw

PATCH 5/12 libceph: clear CONNECTING in ceph_con_close()

In reply to 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 <[email protected]>

---
 net/ceph/messenger.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

net/ceph/messenger.c +2 -1

Index: 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";

Alex Elder

author

2012-06-21 14:21 UTC #6 lore raw

PATCH 6/12 libceph: clear NEGOTIATING when done

In reply to 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 <[email protected]>

---
 net/ceph/messenger.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

net/ceph/messenger.c +5 -3

Index: 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;
 	}

Alex Elder

author

2012-06-21 14:22 UTC #7 lore raw

PATCH 7/12 libceph: define and use an explicit CONNECTED state

In reply to 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 <[email protected]>

---
 include/linux/ceph/messenger.h |    1 +
 net/ceph/messenger.c           |    9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

include/linux/ceph/messenger.h +1 -0

Index: 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. */

net/ceph/messenger.c +7 -2

Index: 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;
 	}

Alex Elder

author

2012-06-21 14:29 UTC #8 lore raw

PATCH 8/12 libceph: separate banner and connect writes

In reply to 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 <[email protected]>

---
 net/ceph/messenger.c |   20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

net/ceph/messenger.c +11 -9

Index: 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)

Alex Elder

author

2012-06-21 14:22 UTC #9 lore raw

PATCH 9/12 libceph: distinguish two phases of connect sequence

In reply to 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 <[email protected]>

---
 net/ceph/messenger.c |   48
++++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 22 deletions(-)

net/ceph/messenger.c +26 ~~-22~~

Index: 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;
 	}

Sage Weil

2012-06-21 18:44 UTC #10 lore raw

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

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

Quoted 109 lines · Alex Elder

Signed-off-by Alex Elder <[email protected]>

---
 net/ceph/messenger.c |   48
++++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 22 deletions(-)

Index: b/net/ceph/messenger.c
===================================================================

net/ceph/messenger.c +26 ~~-23~~

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alex Elder

author

2012-06-21 18:54 UTC #11 lore raw

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

Quoted 7 lines · 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)?

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

Quoted 116 lines

sage

Quoted 109 lines · Alex Elder

Signed-off-by Alex Elder <[email protected]>

---
 net/ceph/messenger.c |   48
++++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 22 deletions(-)

Index: b/net/ceph/messenger.c
===================================================================

net/ceph/messenger.c +26 ~~-23~~

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alex Elder

author

2012-06-21 14:22 UTC #12 lore raw

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

In reply to 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 <[email protected]>

---
 net/ceph/messenger.c |   63
+++++++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 32 deletions(-)

net/ceph/messenger.c +31 ~~-32~~

Index: 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));

Alex Elder

author

2012-06-21 14:22 UTC #13 lore raw

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

In reply to 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 <[email protected]>

---
 include/linux/ceph/messenger.h |    2 -
 net/ceph/messenger.c           |   48
++++++++++++++++++++---------------------
 2 files changed, 24 insertions(+), 26 deletions(-)

include/linux/ceph/messenger.h +0 -2

Index: 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);

net/ceph/messenger.c +24 ~~-24~~

60 lines not on this page · raw message

Sage Weil

2012-06-21 19:45 UTC #14 lore raw

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

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

sage

Quoted 97 lines · Alex Elder

Signed-off-by Alex Elder <[email protected]>

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

include/linux/ceph/messenger.h +0 -2

@@ -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
===================================================================

net/ceph/messenger.c +24 ~~-25~~

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alex Elder

author

2012-06-21 14:28 UTC #15 lore raw

PATCH 12/12 libceph: add some fine ASCII art

In reply to 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 <[email protected]>

---
 net/ceph/messenger.c |   42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

net/ceph/messenger.c +41 -1

Index: 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 */