Updated sparse warning message patches

Updated sparse warning message patches

[Alex Elder]

I'm re-posting these patches because I've updated them to be
based on the patches I just posted ("Four miscellaneous patches").

These patches are available in the branch "test/wip-4184" in
the ceph-client git repository.  That branch is based on
branch "test/wip-4234,5,7,8".

(Here's the original description)

What follows is a few series of patches that get rid of code
issues that lead to warnings from the sparse utility.

The first three patches address the warnings in the rbd, ceph
file system, and libceph code respectively.  After that, one
warning remains in libceph, and that is addressed by a series
of five patches which both address the underlying problem and
reorganized and clean up the surrounding code.  The last two
are meant to be combined into one before commit; they've been
posted as two separate patches to make them easier to review.

					-Alex

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

[PATCH, v2] rbd: eliminate sparse warnings

[Alex Elder]

Fengguang Wu reminded me that there were outstanding sparse reports
in the ceph and rbd code.  This patch fixes these problems in rbd
that lead to those reports:
    - Convert functions that are never referenced externally to have
      static scope.
    - Add a lockdep annotation to rbd_request_fn(), because it
      releases a lock before acquiring it again.

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>
---
v2: rebased

 drivers/block/rbd.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
index a9c86ca..c6b15d4 100644
--- a/drivers/block/rbd.c
+++ b/drivers/block/rbd.c
@@ -1141,7 +1141,7 @@ static bool obj_request_type_valid(enum
obj_request_type type)
 	}
 }

-struct ceph_osd_req_op *rbd_osd_req_op_create(u16 opcode, ...)
+static struct ceph_osd_req_op *rbd_osd_req_op_create(u16 opcode, ...)
 {
 	struct ceph_osd_req_op *op;
 	va_list args;
@@ -1537,7 +1537,8 @@ static void rbd_obj_request_destroy(struct kref *kref)
  * that comprises the image request, and the Linux request pointer
  * (if there is one).
  */
-struct rbd_img_request *rbd_img_request_create(struct rbd_device *rbd_dev,
+static struct rbd_img_request *rbd_img_request_create(
+					struct rbd_device *rbd_dev,
 					u64 offset, u64 length,
 					bool write_request)
 {
@@ -1971,6 +1972,7 @@ out:
 }

 static void rbd_request_fn(struct request_queue *q)
+		__releases(q->queue_lock) __acquires(q->queue_lock)
 {
 	struct rbd_device *rbd_dev = q->queuedata;
 	bool read_only = rbd_dev->mapping.read_only;
@@ -2705,7 +2707,7 @@ static void rbd_spec_free(struct kref *kref)
 	kfree(spec);
 }

-struct rbd_device *rbd_dev_create(struct rbd_client *rbdc,
+static struct rbd_device *rbd_dev_create(struct rbd_client *rbdc,
 				struct rbd_spec *spec)
 {
 	struct rbd_device *rbd_dev;
@@ -4256,7 +4258,7 @@ static void rbd_sysfs_cleanup(void)
 	device_unregister(&rbd_root_dev);
 }

-int __init rbd_init(void)
+static int __init rbd_init(void)
 {
 	int rc;

@@ -4272,7 +4274,7 @@ int __init rbd_init(void)
 	return 0;
 }

-void __exit rbd_exit(void)
+static void __exit rbd_exit(void)
 {
 	rbd_sysfs_cleanup();
 }
-- 
1.7.9.5

[PATCH, v2] ceph: eliminate sparse warnings in fs code

[Alex Elder]

Fix the causes for sparse warnings reported in the ceph file system
code.  Here there are only two (and they're sort of silly but
they're easy to fix).

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>
---
v2: rebased

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

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index 2135817..9b6b2b6 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -213,7 +213,7 @@ static struct ceph_vxattr ceph_dir_vxattrs[] = {
 	XATTR_NAME_CEPH(dir, rsubdirs),
 	XATTR_NAME_CEPH(dir, rbytes),
 	XATTR_NAME_CEPH(dir, rctime),
-	{ 0 }	/* Required table terminator */
+	{ .name = NULL, 0 }	/* Required table terminator */
 };
 static size_t ceph_dir_vxattrs_name_size;	/* total size of all names */

@@ -232,7 +232,7 @@ static struct ceph_vxattr ceph_file_vxattrs[] = {
 	XATTR_LAYOUT_FIELD(file, layout, stripe_count),
 	XATTR_LAYOUT_FIELD(file, layout, object_size),
 	XATTR_LAYOUT_FIELD(file, layout, pool),
-	{ 0 }	/* Required table terminator */
+	{ .name = NULL, 0 }	/* Required table terminator */
 };
 static size_t ceph_file_vxattrs_name_size;	/* total size of all names */

-- 
1.7.9.5

[PATCH, v2] libceph: eliminate sparse warnings

[Alex Elder]

Eliminate most of the problems in the libceph code that cause sparse
to issue warnings.
    - Convert functions that are never referenced externally to have
      static scope.
    - Pass NULL rather than 0 for a pointer argument in one spot in
      ceph_monc_delete_snapid()

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>
---
v2: rebased

 net/ceph/crypto.c     |    7 ++++---
 net/ceph/messenger.c  |    2 +-
 net/ceph/mon_client.c |    2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/ceph/crypto.c b/net/ceph/crypto.c
index af14cb4..6e7a236 100644
--- a/net/ceph/crypto.c
+++ b/net/ceph/crypto.c
@@ -423,7 +423,8 @@ int ceph_encrypt2(struct ceph_crypto_key *secret,
void *dst, size_t *dst_len,
 	}
 }

-int ceph_key_instantiate(struct key *key, struct key_preparsed_payload
*prep)
+static int ceph_key_instantiate(struct key *key,
+				struct key_preparsed_payload *prep)
 {
 	struct ceph_crypto_key *ckey;
 	size_t datalen = prep->datalen;
@@ -458,12 +459,12 @@ err:
 	return ret;
 }

-int ceph_key_match(const struct key *key, const void *description)
+static int ceph_key_match(const struct key *key, const void *description)
 {
 	return strcmp(key->description, description) == 0;
 }

-void ceph_key_destroy(struct key *key) {
+static void ceph_key_destroy(struct key *key) {
 	struct ceph_crypto_key *ckey = key->payload.data;

 	ceph_crypto_key_destroy(ckey);
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 771d4c9..ed9e237 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -223,7 +223,7 @@ static void encode_my_addr(struct ceph_messenger *msgr)
  */
 static struct workqueue_struct *ceph_msgr_wq;

-void _ceph_msgr_exit(void)
+static void _ceph_msgr_exit(void)
 {
 	if (ceph_msgr_wq) {
 		destroy_workqueue(ceph_msgr_wq);
diff --git a/net/ceph/mon_client.c b/net/ceph/mon_client.c
index 812eb3b..aef5b10 100644
--- a/net/ceph/mon_client.c
+++ b/net/ceph/mon_client.c
@@ -697,7 +697,7 @@ int ceph_monc_delete_snapid(struct ceph_mon_client
*monc,
 			    u32 pool, u64 snapid)
 {
 	return do_poolop(monc,  POOL_OP_CREATE_UNMANAGED_SNAP,
-				   pool, snapid, 0, 0);
+				   pool, snapid, NULL, 0);

 }

-- 
1.7.9.5

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

[Alex Elder]

(Re-posting because these changes have been rebased.)

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>
---
v2: rebased

 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 ed9e237..9a29d8a 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -2345,6 +2345,24 @@ static bool con_sock_closed(struct
ceph_connection *con)
 	return true;
 }

+static bool con_backoff(struct ceph_connection *con)
+{
+	int ret;
+
+	if (!con_flag_test_and_clear(con, CON_FLAG_BACKOFF))
+		return false;
+
+	ret = queue_con_delay(con, round_jiffies_relative(con->delay));
+	if (ret) {
+		dout("%s: con %p FAILED to back off %lu\n", __func__,
+			con, con->delay);
+		BUG_ON(ret == -ENOENT);
+		con_flag_set(con, CON_FLAG_BACKOFF);
+	}
+
+	return true;
+}
+
 /*
  * Do some work on a connection.  Drop a connection ref when we're done.
  */
@@ -2356,21 +2374,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("%s: con %p SOCK_CLOSED\n", __func__, con);
 		goto fault;
-
-	if (con_flag_test_and_clear(con, CON_FLAG_BACKOFF)) {
-		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);
-			con_flag_set(con, CON_FLAG_BACKOFF);
-		}
+	}
+	if (con_backoff(con)) {
+		dout("%s: con %p BACKOFF\n", __func__, con);
 		goto done;
 	}
-
 	if (con->state == CON_STATE_STANDBY) {
 		dout("con_work %p STANDBY\n", con);
 		goto done;
@@ -2381,7 +2392,7 @@ restart:
 		goto done;
 	}
 	if (con->state == CON_STATE_PREOPEN) {
-		dout("con_work OPENING\n");
+		dout("%s: con %p OPENING\n", __func__, 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>
---
v2: rebased

 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 9a29d8a..c3b9060 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -166,7 +166,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
@@ -2363,6 +2363,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.
  */
@@ -2419,7 +2436,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;
 }

@@ -2428,8 +2447,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);
@@ -2445,7 +2463,7 @@ static void ceph_fault(struct ceph_connection *con)
 	if (con_flag_test(con, CON_FLAG_LOSSYTX)) {
 		dout("fault on LOSSYTX channel, marking CLOSED\n");
 		con->state = CON_STATE_CLOSED;
-		goto out_unlock;
+		return;
 	}

 	if (con->in_msg) {
@@ -2476,20 +2494,6 @@ static void ceph_fault(struct ceph_connection *con)
 		con_flag_set(con, CON_FLAG_BACKOFF);
 		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 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>
---
v2: rebased

 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 9a29d8a..c3b9060 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -166,7 +166,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
@@ -2363,6 +2363,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.
  */
@@ -2419,7 +2436,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;
 }

@@ -2428,8 +2447,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);
@@ -2445,7 +2463,7 @@ static void ceph_fault(struct ceph_connection *con)
 	if (con_flag_test(con, CON_FLAG_LOSSYTX)) {
 		dout("fault on LOSSYTX channel, marking CLOSED\n");
 		con->state = CON_STATE_CLOSED;
-		goto out_unlock;
+		return;
 	}

 	if (con->in_msg) {
@@ -2476,20 +2494,6 @@ static void ceph_fault(struct ceph_connection *con)
 		con_flag_set(con, CON_FLAG_BACKOFF);
 		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

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

[Alex Elder]

On 02/22/2013 11:26 AM, Alex Elder wrote:
> 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.
. . .

Sorry about the duplicate(s) and the messed up subject
lines.  I got a little trouble from gmail in the middle
of posting these.

					-Alex

[PATCH 3/5, v2] 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>
---
v2: rebased

 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 c3b9060..18eb788 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -2387,13 +2387,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("%s: con %p SOCK_CLOSED\n", __func__, con);
-		goto fault;
+		fault = true;
+		goto done;
 	}
 	if (con_backoff(con)) {
 		dout("%s: con %p BACKOFF\n", __func__, con);
@@ -2418,7 +2420,8 @@ restart:
 		goto restart;
 	if (ret < 0) {
 		con->error_msg = "socket error on read";
-		goto fault;
+		fault = true;
+		goto done;
 	}

 	ret = try_write(con);
@@ -2426,20 +2429,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, v2] 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.

Also update a few dout() calls near the affected code for
consistency.

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>
---
v2: rebased

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

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 18eb788..223406f 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -2387,51 +2387,53 @@ 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("%s: con %p SOCK_CLOSED\n", __func__, con);
-		fault = true;
-		goto done;
+		break;
 	}
 	if (con_backoff(con)) {
 		dout("%s: con %p BACKOFF\n", __func__, con);
-		goto done;
+		break;
 	}
 	if (con->state == CON_STATE_STANDBY) {
-		dout("con_work %p STANDBY\n", con);
-		goto done;
+		dout("%s: con %p STANDBY\n", __func__, con);
+		break;
 	}
 	if (con->state == CON_STATE_CLOSED) {
-		dout("con_work %p CLOSED\n", con);
+		dout("%s: con %p CLOSED\n", __func__, con);
 		BUG_ON(con->sock);
-		goto done;
+		break;
 	}
 	if (con->state == CON_STATE_PREOPEN) {
-		dout("%s: con %p OPENING\n", __func__, con);
+		dout("%s: con %p PREOPEN\n", __func__, con);
 		BUG_ON(con->sock);
 	}

 	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);
@@ -2442,7 +2444,6 @@ done:
 	con->ops->put(con);
 }

-
 /*
  * Generic error/fault handler.  A retry mechanism is used with
  * exponential backoff
-- 
1.7.9.5

[PATCH 5/5, v2] 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>
---
v2: rebased

 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 223406f..2c0669f 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -2390,50 +2390,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("%s: con %p SOCK_CLOSED\n", __func__, con);
-		break;
-	}
-	if (con_backoff(con)) {
-		dout("%s: con %p BACKOFF\n", __func__, con);
-		break;
-	}
-	if (con->state == CON_STATE_STANDBY) {
-		dout("%s: con %p STANDBY\n", __func__, con);
-		break;
-	}
-	if (con->state == CON_STATE_CLOSED) {
-		dout("%s: con %p CLOSED\n", __func__, con);
-		BUG_ON(con->sock);
-		break;
-	}
-	if (con->state == CON_STATE_PREOPEN) {
-		dout("%s: con %p PREOPEN\n", __func__, con);
-		BUG_ON(con->sock);
-	}
+		if ((fault = con_sock_closed(con))) {
+			dout("%s: con %p SOCK_CLOSED\n", __func__, con);
+			break;
+		}
+		if (con_backoff(con)) {
+			dout("%s: con %p BACKOFF\n", __func__, con);
+			break;
+		}
+		if (con->state == CON_STATE_STANDBY) {
+			dout("%s: con %p STANDBY\n", __func__, con);
+			break;
+		}
+		if (con->state == CON_STATE_CLOSED) {
+			dout("%s: con %p CLOSED\n", __func__, con);
+			BUG_ON(con->sock);
+			break;
+		}
+		if (con->state == CON_STATE_PREOPEN) {
+			dout("%s: con %p PREOPEN\n", __func__, 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

Re: Updated sparse warning message patches

[Josh Durgin]

On 02/22/2013 09:18 AM, Alex Elder wrote:
> I'm re-posting these patches because I've updated them to be
> based on the patches I just posted ("Four miscellaneous patches").
>
> These patches are available in the branch "test/wip-4184" in
> the ceph-client git repository.  That branch is based on
> branch "test/wip-4234,5,7,8".
>
> (Here's the original description)
>
> What follows is a few series of patches that get rid of code
> issues that lead to warnings from the sparse utility.
>
> The first three patches address the warnings in the rbd, ceph
> file system, and libceph code respectively.  After that, one
> warning remains in libceph, and that is addressed by a series
> of five patches which both address the underlying problem and
> reorganized and clean up the surrounding code.  The last two
> are meant to be combined into one before commit; they've been
> posted as two separate patches to make them easier to review.
>
> 					-Alex
>
> [PATCH, v2] rbd: eliminate sparse warnings
> [PATCH, v2] ceph: eliminate sparse warnings in fs code
> [PATCH, v2] libceph: eliminate sparse warnings
> [PATCH 1/5, v2] libceph: encapsulate connection backoff
> [PATCH 2/5, v2] libceph: separate non-locked fault handling
> [PATCH 3/5, v2] libceph: use a flag to indicate a fault has occurred
> [PATCH 4/5, v2] libceph: use a do..while loop in con_work()
> [PATCH 5/5, v2] libceph: indent properly

These all look good too.
Reviewed-by: Josh Durgin <josh.durgin@inktank.com>

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