[PATCH 0/5] libceph clean up con_work()

[PATCH 0/5] libceph clean up con_work()

[Alex Elder]

This series cleans up con_work() a bit.  The original motivation
was to get rid of a warning issued by the sparse utility, but
addressing that required a little rework and it was fairly
straightforward once that was done to make that function
fairly simple.

The problem sparse reported was really due to sparse not
being able to follow the logic between multiple functions
that together implement locking.  The result of these
changes makes both acquiring and releasing the connection
mutex occur in con_work().

					-Alex

[PATCH 1/5] libceph: encapsulate connection backoff
[PATCH 2/5] libceph: separate non-locked fault handling
[PATCH 3/5] libceph: use a flag to indicate a fault has occurred
[PATCH 4/5] libceph: use a do..while loop in con_work()
[PATCH 5/5] libceph: indent properly

[PATCH 1/5] libceph: encapsulate connection backoff

[Alex Elder]

Collect the code that tests for and implements a backoff delay for a
ceph connection into a new function, ceph_backoff().

Make the debug output messages in that part of the code report
things consistently by reporting a message in the socket closed
case, and by making the one for PREOPEN state report the connection
pointer like the rest.

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

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index ab702cd..686973c 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -2296,6 +2296,24 @@ static bool con_sock_closed(struct
ceph_connection *con)
 	return true;
 }

+static bool con_backoff(struct ceph_connection *con)
+{
+	int ret;
+
+	if (!test_and_clear_bit(CON_FLAG_BACKOFF, &con->flags))
+		return false;
+
+	ret = queue_con_delay(con, round_jiffies_relative(con->delay));
+	if (ret) {
+		dout("%s %p FAILED to back off %lu\n",
+			__func__, con, con->delay);
+		BUG_ON(ret == -ENOENT);
+		set_bit(CON_FLAG_BACKOFF, &con->flags);
+	}
+
+	return true;
+}
+
 /*
  * Do some work on a connection.  Drop a connection ref when we're done.
  */
@@ -2307,21 +2325,14 @@ static void con_work(struct work_struct *work)

 	mutex_lock(&con->mutex);
 restart:
-	if (con_sock_closed(con))
+	if (con_sock_closed(con)) {
+		dout("con_work %p SOCK_CLOSED\n", con);
 		goto fault;
-
-	if (test_and_clear_bit(CON_FLAG_BACKOFF, &con->flags)) {
-		dout("con_work %p backing off\n", con);
-		ret = queue_con_delay(con, round_jiffies_relative(con->delay));
-		if (ret) {
-			dout("con_work %p FAILED to back off %lu\n", con,
-			     con->delay);
-			BUG_ON(ret == -ENOENT);
-			set_bit(CON_FLAG_BACKOFF, &con->flags);
-		}
+	}
+	if (con_backoff(con)) {
+		dout("con_work %p BACKOFF\n", con);
 		goto done;
 	}
-
 	if (con->state == CON_STATE_STANDBY) {
 		dout("con_work %p STANDBY\n", con);
 		goto done;
@@ -2332,7 +2343,7 @@ restart:
 		goto done;
 	}
 	if (con->state == CON_STATE_PREOPEN) {
-		dout("con_work OPENING\n");
+		dout("con_work %p OPENING\n", con);
 		BUG_ON(con->sock);
 	}

-- 
1.7.9.5

[PATCH 2/5] libceph: separate non-locked fault handling

[Alex Elder]

An error occurring on a ceph connection is treated as a fault,
causing the connection to be reset.  The initial part of this fault
handling has to be done while holding the connection mutex, but
it must then be dropped for the last part.

Separate the part of this fault handling that executes without the
lock into its own function, con_fault_finish().  Move the call to
this new function, as well as call that drops the connection mutex,
into ceph_fault().  Rename that function con_fault() to reflect that
it's only handling the connection part of the fault handling.

The motivation for this was a warning from sparse about the locking
being done here.  Rearranging things this way keeps all the mutex
manipulation within ceph_fault(), and this stops sparse from
complaining.

This partially resolves:
    http://tracker.ceph.com/issues/4184

Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Signed-off-by: Alex Elder <elder@inktank.com>
---
 net/ceph/messenger.c |   42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 686973c..0fe9cbd 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -115,7 +115,7 @@ static struct lock_class_key socket_class;

 static void queue_con(struct ceph_connection *con);
 static void con_work(struct work_struct *);
-static void ceph_fault(struct ceph_connection *con);
+static void con_fault(struct ceph_connection *con);

 /*
  * Nicely render a sockaddr as a string.  An array of formatted
@@ -2314,6 +2314,23 @@ static bool con_backoff(struct ceph_connection *con)
 	return true;
 }

+/* Finish fault handling; con->mutex must *not* be held here */
+
+static void con_fault_finish(struct ceph_connection *con)
+{
+	/*
+	 * in case we faulted due to authentication, invalidate our
+	 * current tickets so that we can get new ones.
+	 */
+	if (con->auth_retry && con->ops->invalidate_authorizer) {
+		dout("calling invalidate_authorizer()\n");
+		con->ops->invalidate_authorizer(con);
+	}
+
+	if (con->ops->fault)
+		con->ops->fault(con);
+}
+
 /*
  * Do some work on a connection.  Drop a connection ref when we're done.
  */
@@ -2370,7 +2387,9 @@ done_unlocked:
 	return;

 fault:
-	ceph_fault(con);     /* error/fault path */
+	con_fault(con);
+	mutex_unlock(&con->mutex);
+	con_fault_finish(con);
 	goto done_unlocked;
 }

@@ -2379,8 +2398,7 @@ fault:
  * Generic error/fault handler.  A retry mechanism is used with
  * exponential backoff
  */
-static void ceph_fault(struct ceph_connection *con)
-	__releases(con->mutex)
+static void con_fault(struct ceph_connection *con)
 {
 	pr_warning("%s%lld %s %s\n", ENTITY_NAME(con->peer_name),
 	       ceph_pr_addr(&con->peer_addr.in_addr), con->error_msg);
@@ -2396,7 +2414,7 @@ static void ceph_fault(struct ceph_connection *con)
 	if (test_bit(CON_FLAG_LOSSYTX, &con->flags)) {
 		dout("fault on LOSSYTX channel, marking CLOSED\n");
 		con->state = CON_STATE_CLOSED;
-		goto out_unlock;
+		return;
 	}

 	if (con->in_msg) {
@@ -2427,20 +2445,6 @@ static void ceph_fault(struct ceph_connection *con)
 		set_bit(CON_FLAG_BACKOFF, &con->flags);
 		queue_con(con);
 	}
-
-out_unlock:
-	mutex_unlock(&con->mutex);
-	/*
-	 * in case we faulted due to authentication, invalidate our
-	 * current tickets so that we can get new ones.
-	 */
-	if (con->auth_retry && con->ops->invalidate_authorizer) {
-		dout("calling invalidate_authorizer()\n");
-		con->ops->invalidate_authorizer(con);
-	}
-
-	if (con->ops->fault)
-		con->ops->fault(con);
 }


-- 
1.7.9.5

[PATCH 3/5] libceph: use a flag to indicate a fault has occurred

[Alex Elder]

This just rearranges the logic in con_work() a little bit so that a
flag is used to indicate a fault has occurred.  This allows both the
fault and non-fault case to be handled the same way and avoids a
couple of nearly consecutive gotos.

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

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 0fe9cbd..1cf0e53 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -2338,13 +2338,15 @@ static void con_work(struct work_struct *work)
 {
 	struct ceph_connection *con = container_of(work, struct ceph_connection,
 						   work.work);
+	bool fault = false;
 	int ret;

 	mutex_lock(&con->mutex);
 restart:
 	if (con_sock_closed(con)) {
 		dout("con_work %p SOCK_CLOSED\n", con);
-		goto fault;
+		fault = true;
+		goto done;
 	}
 	if (con_backoff(con)) {
 		dout("con_work %p BACKOFF\n", con);
@@ -2369,7 +2371,8 @@ restart:
 		goto restart;
 	if (ret < 0) {
 		con->error_msg = "socket error on read";
-		goto fault;
+		fault = true;
+		goto done;
 	}

 	ret = try_write(con);
@@ -2377,20 +2380,17 @@ restart:
 		goto restart;
 	if (ret < 0) {
 		con->error_msg = "socket error on write";
-		goto fault;
+		fault = true;
 	}
-
 done:
+	if (fault)
+		con_fault(con);
 	mutex_unlock(&con->mutex);
-done_unlocked:
-	con->ops->put(con);
-	return;

-fault:
-	con_fault(con);
-	mutex_unlock(&con->mutex);
-	con_fault_finish(con);
-	goto done_unlocked;
+	if (fault)
+		con_fault_finish(con);
+
+	con->ops->put(con);
 }


-- 
1.7.9.5

[PATCH 4/5] libceph: use a do..while loop in con_work()

[Alex Elder]

This just converts a manually-implemented loop into a do..while loop
in con_work().  It also moves handling of EAGAIN inside the blocks
where it's already been determined an error code was returned.

NOTE:
    This was done in two steps in order to facilitate review.  The
    This patch will be squashed into the next one before commit.
    next patch simply indents the loop properly.

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

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 1cf0e53..609f2eb 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -2338,28 +2338,28 @@ static void con_work(struct work_struct *work)
 {
 	struct ceph_connection *con = container_of(work, struct ceph_connection,
 						   work.work);
-	bool fault = false;
-	int ret;
+	bool fault;

 	mutex_lock(&con->mutex);
-restart:
-	if (con_sock_closed(con)) {
+while (true) {
+	int ret;
+
+	if ((fault = con_sock_closed(con))) {
 		dout("con_work %p SOCK_CLOSED\n", con);
-		fault = true;
-		goto done;
+		break;
 	}
 	if (con_backoff(con)) {
 		dout("con_work %p BACKOFF\n", con);
-		goto done;
+		break;
 	}
 	if (con->state == CON_STATE_STANDBY) {
 		dout("con_work %p STANDBY\n", con);
-		goto done;
+		break;
 	}
 	if (con->state == CON_STATE_CLOSED) {
 		dout("con_work %p CLOSED\n", con);
 		BUG_ON(con->sock);
-		goto done;
+		break;
 	}
 	if (con->state == CON_STATE_PREOPEN) {
 		dout("con_work %p OPENING\n", con);
@@ -2367,22 +2367,24 @@ restart:
 	}

 	ret = try_read(con);
-	if (ret == -EAGAIN)
-		goto restart;
 	if (ret < 0) {
+		if (ret == -EAGAIN)
+			continue;
 		con->error_msg = "socket error on read";
 		fault = true;
-		goto done;
+		break;
 	}

 	ret = try_write(con);
-	if (ret == -EAGAIN)
-		goto restart;
 	if (ret < 0) {
+		if (ret == -EAGAIN)
+			continue;
 		con->error_msg = "socket error on write";
 		fault = true;
 	}
-done:
+
+	break;	/* If we make it to here, we're done */
+}
 	if (fault)
 		con_fault(con);
 	mutex_unlock(&con->mutex);
-- 
1.7.9.5

[PATCH 5/5] libceph: indent properly

[Alex Elder]

This is just the second part of a two-part patch.  It simply
indents a block of code.  This patch is going to be merged into
its predecessor following review.

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

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 609f2eb..8f1272d 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -2341,50 +2341,50 @@ static void con_work(struct work_struct *work)
 	bool fault;

 	mutex_lock(&con->mutex);
-while (true) {
-	int ret;
+	while (true) {
+		int ret;

-	if ((fault = con_sock_closed(con))) {
-		dout("con_work %p SOCK_CLOSED\n", con);
-		break;
-	}
-	if (con_backoff(con)) {
-		dout("con_work %p BACKOFF\n", con);
-		break;
-	}
-	if (con->state == CON_STATE_STANDBY) {
-		dout("con_work %p STANDBY\n", con);
-		break;
-	}
-	if (con->state == CON_STATE_CLOSED) {
-		dout("con_work %p CLOSED\n", con);
-		BUG_ON(con->sock);
-		break;
-	}
-	if (con->state == CON_STATE_PREOPEN) {
-		dout("con_work %p OPENING\n", con);
-		BUG_ON(con->sock);
-	}
+		if ((fault = con_sock_closed(con))) {
+			dout("con_work %p SOCK_CLOSED\n", con);
+			break;
+		}
+		if (con_backoff(con)) {
+			dout("con_work %p BACKOFF\n", con);
+			break;
+		}
+		if (con->state == CON_STATE_STANDBY) {
+			dout("con_work %p STANDBY\n", con);
+			break;
+		}
+		if (con->state == CON_STATE_CLOSED) {
+			dout("con_work %p CLOSED\n", con);
+			BUG_ON(con->sock);
+			break;
+		}
+		if (con->state == CON_STATE_PREOPEN) {
+			dout("con_work %p OPENING\n", con);
+			BUG_ON(con->sock);
+		}

-	ret = try_read(con);
-	if (ret < 0) {
-		if (ret == -EAGAIN)
-			continue;
-		con->error_msg = "socket error on read";
-		fault = true;
-		break;
-	}
+		ret = try_read(con);
+		if (ret < 0) {
+			if (ret == -EAGAIN)
+				continue;
+			con->error_msg = "socket error on read";
+			fault = true;
+			break;
+		}

-	ret = try_write(con);
-	if (ret < 0) {
-		if (ret == -EAGAIN)
-			continue;
-		con->error_msg = "socket error on write";
-		fault = true;
-	}
+		ret = try_write(con);
+		if (ret < 0) {
+			if (ret == -EAGAIN)
+				continue;
+			con->error_msg = "socket error on write";
+			fault = true;
+		}

-	break;	/* If we make it to here, we're done */
-}
+		break;	/* If we make it to here, we're done */
+	}
 	if (fault)
 		con_fault(con);
 	mutex_unlock(&con->mutex);
-- 
1.7.9.5