libceph: miscellaneous cleanups

PATCH In review Alex Elder 2 participants 9 messages Feb 29, 2012 – Mar 2, 2012 Ceph common code (libceph)

lore

Share & tools

lore

Alex Elder

author

2012-02-29 04:44 UTC #0 lore raw

cover libceph: miscellaneous cleanups

More cleanups, this time in the messenger code. -Alex

Alex Elder

author

2012-02-29 04:46 UTC #1 lore raw

PATCH 1/4 libceph: make ceph_msgr_wq private

The messenger workqueue has no need to be public. So give it static scope.

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

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

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

index 6b5af5f..5ca0f82 100644
@@ -14,8 +14,6 @@
 struct ceph_msg;
 struct ceph_connection;
 
-extern struct workqueue_struct *ceph_msgr_wq;       /* receive work queue */
-
 /*
  * Ceph defines these callbacks for handling connection events.
  */

net/ceph/messenger.c +1 -1

index 03868cd..9c0f73a 100644
@@ -97,7 +97,7 @@ static void encode_my_addr(struct ceph_messenger *msgr)
 /*
  * work queue for all reading and writing to/from the socket.
  */
-struct workqueue_struct *ceph_msgr_wq;
+static struct workqueue_struct *ceph_msgr_wq;
 
 int ceph_msgr_init(void)
 {

Signature · 1 line

1.7.5.4

Alex Elder

author

2012-02-29 04:46 UTC #2 lore raw

PATCH 2/4 libceph: encapsulate some messenger cleanup code

In reply to Alex Elder

Define a helper function to perform various cleanup operations. Use it both in the exit routine and in the init routine in the event of an error.

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

---
 net/ceph/messenger.c |   38 ++++++++++++++++++++------------------
 1 files changed, 20 insertions(+), 18 deletions(-)

net/ceph/messenger.c +20 ~~-18~~

index 9c0f73a..2e496e2 100644
@@ -99,6 +99,20 @@ static void encode_my_addr(struct ceph_messenger *msgr)
  */
 static struct workqueue_struct *ceph_msgr_wq;
 
+void _ceph_msgr_exit(void)
+{
+	if (ceph_msgr_wq)
+		destroy_workqueue(ceph_msgr_wq);
+
+	BUG_ON(zero_page_address == NULL);
+	zero_page_address = NULL;
+
+	BUG_ON(zero_page == NULL);
+	kunmap(zero_page);
+	__free_page(zero_page);
+	zero_page = NULL;
+}
+
 int ceph_msgr_init(void)
 {
 	BUG_ON(zero_page != NULL);

@@ -111,33 +125,21 @@ int ceph_msgr_init(void)
 	zero_page_address = kmap(zero_page);
 
 	ceph_msgr_wq = alloc_workqueue("ceph-msgr", WQ_NON_REENTRANT, 0);
-	if (!ceph_msgr_wq) {
-		pr_err("msgr_init failed to create workqueue\n");
-
-		zero_page_address = NULL;
-		kunmap(zero_page);
-		__free_page(zero_page);
-		zero_page = NULL;
+	if (ceph_msgr_wq)
+		return 0;
 
-		return -ENOMEM;
-	}
+	pr_err("msgr_init failed to create workqueue\n");
+	_ceph_msgr_exit();
 
-	return 0;
+	return -ENOMEM;
 }
 EXPORT_SYMBOL(ceph_msgr_init);
 
 void ceph_msgr_exit(void)
 {
 	BUG_ON(ceph_msgr_wq == NULL);
-	destroy_workqueue(ceph_msgr_wq);
 
-	BUG_ON(zero_page_address == NULL);
-	zero_page_address = NULL;
-
-	BUG_ON(zero_page == NULL);
-	kunmap(zero_page);
-	__free_page(zero_page);
-	zero_page = NULL;
+	_ceph_msgr_exit();
 }
 EXPORT_SYMBOL(ceph_msgr_exit);

Signature · 1 line

1.7.5.4

Alex Elder

author

2012-02-29 04:46 UTC #4 lore raw

PATCH 3/4 libceph: make ceph_tcp_connect() return int

In reply to Alex Elder

There is no real need for ceph_tcp_connect() to return the socket pointer it creates, since it already assigns it to con->sock, which is visible to the caller. Instead, have it return an error code, which tidies things up a bit.

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

---
 net/ceph/messenger.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

net/ceph/messenger.c +6 -8

index 2e496e2..9a8a479 100644
@@ -236,7 +236,7 @@ static void set_sock_callbacks(struct socket *sock,
 /*
  * initiate connection to a remote socket.
  */
-static struct socket *ceph_tcp_connect(struct ceph_connection *con)
+static int ceph_tcp_connect(struct ceph_connection *con)
 {
 	struct sockaddr_storage *paddr = &con->peer_addr.in_addr;
 	struct socket *sock;

@@ -246,7 +246,7 @@ static struct socket *ceph_tcp_connect(struct ceph_connection *con)
 	ret = sock_create_kern(con->peer_addr.in_addr.ss_family, SOCK_STREAM,
 			       IPPROTO_TCP, &sock);
 	if (ret)
-		return ERR_PTR(ret);
+		return ret;
 	sock->sk->sk_allocation = GFP_NOFS;
 
 #ifdef CONFIG_LOCKDEP

@@ -269,11 +269,11 @@ static struct socket *ceph_tcp_connect(struct ceph_connection *con)
 		sock_release(sock);
 		con->error_msg = "connect error";
 
-		return ERR_PTR(ret);
+		return ret;
 	}
 	con->sock = sock;
 
-	return sock;
+	return 0;
 }
 
 static int ceph_tcp_recvmsg(struct socket *sock, void *buf, size_t len)

@@ -1850,11 +1850,9 @@ more:
 		con->in_tag = CEPH_MSGR_TAG_READY;
 		dout("try_write initiating connect on %p new state %lu\n",
 		     con, con->state);
-		con->sock = ceph_tcp_connect(con);
-		if (IS_ERR(con->sock)) {
-			con->sock = NULL;
+		ret = ceph_tcp_connect(con);
+		if (ret < 0) {
 			con->error_msg = "connect error";
-			ret = -1;
 			goto out;
 		}
 	}

Signature · 1 line

1.7.5.4

Alex Elder

author

2012-02-29 04:46 UTC #5 lore raw

PATCH 4/4 libceph: a few small changes

In reply to Alex Elder

This gathers a number of very minor changes:
- use %hu when formatting the a socket address's address family
- null out the ceph_msgr_wq pointer after the queue has been destroyed
- drop a needless cast in ceph_write_space()
- add a WARN() call in ceph_state_change() in the event an unrecognized socket state is encountered
- rearrange the logic in ceph_con_get() and ceph_con_put() so that:
- the reference counts are only atomically read once
- the values displayed via dout() calls are known to be meaningful at the time they are formatted

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

---
 net/ceph/messenger.c |   33 +++++++++++++++++++--------------
 1 files changed, 19 insertions(+), 14 deletions(-)

net/ceph/messenger.c +19 ~~-14~~

index 9a8a479..d793b9f 100644
@@ -80,8 +80,8 @@ const char *ceph_pr_addr(const struct sockaddr_storage *ss)
 		break;
 
 	default:
-		snprintf(s, MAX_ADDR_STR_LEN, "(unknown sockaddr family %d)",
-			 (int)ss->ss_family);
+		snprintf(s, MAX_ADDR_STR_LEN, "(unknown sockaddr family %hu)",
+			 ss->ss_family);
 	}
 
 	return s;

@@ -101,8 +101,10 @@ static struct workqueue_struct *ceph_msgr_wq;
 
 void _ceph_msgr_exit(void)
 {
-	if (ceph_msgr_wq)
+	if (ceph_msgr_wq) {
 		destroy_workqueue(ceph_msgr_wq);
+		ceph_msgr_wq = NULL;
+	}
 
 	BUG_ON(zero_page_address == NULL);
 	zero_page_address = NULL;

@@ -169,8 +171,7 @@ static void ceph_data_ready(struct sock *sk, int count_unused)
 /* socket has buffer space for writing */
 static void ceph_write_space(struct sock *sk)
 {
-	struct ceph_connection *con =
-		(struct ceph_connection *)sk->sk_user_data;
+	struct ceph_connection *con = sk->sk_user_data;
 
 	/* only queue to workqueue if there is data we want to write. */
 	if (test_bit(WRITE_PENDING, &con->state)) {

@@ -212,6 +213,9 @@ static void ceph_state_change(struct sock *sk)
 		dout("ceph_state_change TCP_ESTABLISHED\n");
 		queue_con(con);
 		break;
+	default:
+		WARN(1, "unexpected socket state (%d)", sk->sk_state);
+		break;
 	}
 }

@@ -416,22 +420,23 @@ bool ceph_con_opened(struct ceph_connection *con)
  */
 struct ceph_connection *ceph_con_get(struct ceph_connection *con)
 {
-	dout("con_get %p nref = %d -> %d\n", con,
-	     atomic_read(&con->nref), atomic_read(&con->nref) + 1);
-	if (atomic_inc_not_zero(&con->nref))
-		return con;
-	return NULL;
+	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)
 {
-	dout("con_put %p nref = %d -> %d\n", con,
-	     atomic_read(&con->nref), atomic_read(&con->nref) - 1);
-	BUG_ON(atomic_read(&con->nref) == 0);
-	if (atomic_dec_and_test(&con->nref)) {
+	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);
 }
 
 /*

Signature · 1 line

1.7.5.4

Alex Elder

author

2012-02-29 17:44 UTC #3 lore raw

PATCH v2 1/1 libceph: encapsulate some messenger cleanup code

In reply to Alex Elder

Define a helper function to perform various cleanup operations. Use it both in the exit routine and in the init routine in the event of an error.

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

---
v2: updated due to switch to using ZERO_PAGE(0) in an earlier patch
in the series.

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

net/ceph/messenger.c +20 ~~-18~~

Index: b/net/ceph/messenger.c
===================================================================
@@ -99,6 +99,20 @@ static void encode_my_addr(struct ceph_m
  */
 static struct workqueue_struct *ceph_msgr_wq;
 
+void _ceph_msgr_exit(void)
+{
+	if (ceph_msgr_wq)
+		destroy_workqueue(ceph_msgr_wq);
+
+	BUG_ON(zero_page_address == NULL);
+	zero_page_address = NULL;
+
+	BUG_ON(zero_page == NULL);
+	kunmap(zero_page);
+	page_cache_release(zero_page);
+	zero_page = NULL;
+}
+
 int ceph_msgr_init(void)
 {
 	BUG_ON(zero_page != NULL);

@@ -109,33 +123,21 @@ int ceph_msgr_init(void)
 	zero_page_address = kmap(zero_page);
 
 	ceph_msgr_wq = alloc_workqueue("ceph-msgr", WQ_NON_REENTRANT, 0);
-	if (!ceph_msgr_wq) {
-		pr_err("msgr_init failed to create workqueue\n");
-
-		zero_page_address = NULL;
-		kunmap(zero_page);
-		page_cache_release(zero_page);
-		zero_page = NULL;
+	if (ceph_msgr_wq)
+		return 0;
 
-		return -ENOMEM;
-	}
+	pr_err("msgr_init failed to create workqueue\n");
+	_ceph_msgr_exit();
 
-	return 0;
+	return -ENOMEM;
 }
 EXPORT_SYMBOL(ceph_msgr_init);
 
 void ceph_msgr_exit(void)
 {
 	BUG_ON(ceph_msgr_wq == NULL);
-	destroy_workqueue(ceph_msgr_wq);
-
-	BUG_ON(zero_page_address == NULL);
-	zero_page_address = NULL;
 
-	BUG_ON(zero_page == NULL);
-	kunmap(zero_page);
-	page_cache_release(zero_page);
-	zero_page = NULL;
+	_ceph_msgr_exit();
 }
 EXPORT_SYMBOL(ceph_msgr_exit);

Sage Weil

Reviewed-by on v1 4/4

2012-03-02 21:40 UTC #6 lore raw

Re: [PATCH 4/4] libceph: a few small changes

In reply to Alex Elder

Quoted 57 lines · Alex Elder

This gathers a number of very minor changes:
- use %hu when formatting the a socket address's address family
- null out the ceph_msgr_wq pointer after the queue has been destroyed
- drop a needless cast in ceph_write_space()
- add a WARN() call in ceph_state_change() in the event an unrecognized socket state is encountered
- rearrange the logic in ceph_con_get() and ceph_con_put() so that:
- the reference counts are only atomically read once
- the values displayed via dout() calls are known to be meaningful at the time they are formatted
Signed-off-by Alex Elder <[email protected]>
---
 net/ceph/messenger.c |   33 +++++++++++++++++++--------------
 1 files changed, 19 insertions(+), 14 deletions(-)
net/ceph/messenger.c +6 -5
index 9a8a479..d793b9f 100644
@@ -80,8 +80,8 @@ const char *ceph_pr_addr(const struct sockaddr_storage *ss)
 		break;
 
 	default:
-		snprintf(s, MAX_ADDR_STR_LEN, "(unknown sockaddr family %d)",
-			 (int)ss->ss_family);
+		snprintf(s, MAX_ADDR_STR_LEN, "(unknown sockaddr family %hu)",
+			 ss->ss_family);
 	}
 
 	return s;
@@ -101,8 +101,10 @@ static struct workqueue_struct *ceph_msgr_wq;
 
 void _ceph_msgr_exit(void)
 {
-	if (ceph_msgr_wq)
+	if (ceph_msgr_wq) {
 		destroy_workqueue(ceph_msgr_wq);
+		ceph_msgr_wq = NULL;
+	}
 
 	BUG_ON(zero_page_address == NULL);
 	zero_page_address = NULL;
@@ -169,8 +171,7 @@ static void ceph_data_ready(struct sock *sk, int
 count_unused)
 /* socket has buffer space for writing */
 static void ceph_write_space(struct sock *sk)
 {
-	struct ceph_connection *con =
-		(struct ceph_connection *)sk->sk_user_data;
+	struct ceph_connection *con = sk->sk_user_data;
 
 	/* only queue to workqueue if there is data we want to write. */
net/ceph/messenger.c +0 -0
 	if (test_bit(WRITE_PENDING, &con->state)) {

net/ceph/messenger.c static void ceph_state_change(struct sock *sk) · line 219

Quoted 7 lines

net/ceph/messenger.c +3 -0

@@ -212,6 +213,9 @@ static void ceph_state_change(struct sock *sk)
 		dout("ceph_state_change TCP_ESTABLISHED\n");
 		queue_con(con);
 		break;
+	default:
+		WARN(1, "unexpected socket state (%d)", sk->sk_state);
+		break;
 	}

This is a red herring... the switch isn't meant to be exhaustive, only to catch the interesting (shutdown) states. I think #2099 can be closed as well.

The rest looks good.

Reviewed-by Sage Weil <[email protected]>

Quoted 44 lines

net/ceph/messenger.c +10 -9

@@ -416,22 +420,23 @@ bool ceph_con_opened(struct ceph_connection *con)
  */
 struct ceph_connection *ceph_con_get(struct ceph_connection *con)
 {
-	dout("con_get %p nref = %d -> %d\n", con,
-	     atomic_read(&con->nref), atomic_read(&con->nref) + 1);
-	if (atomic_inc_not_zero(&con->nref))
-		return con;
-	return NULL;
+	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)
 {
-	dout("con_put %p nref = %d -> %d\n", con,
-	     atomic_read(&con->nref), atomic_read(&con->nref) - 1);
-	BUG_ON(atomic_read(&con->nref) == 0);
-	if (atomic_dec_and_test(&con->nref)) {
+	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);
 }
 
 /*

Signature · 6 lines

1.7.5.4

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

Sage Weil

Reviewed-by on the v1 series

2012-03-02 21:42 UTC #8 lore raw

In reply to Alex Elder

These look good, modulo the socket state switch() warning, and the rearranging of zero page addr code (which i think belongs inside write_partial_message_pages()).

Reviewed-by Sage Weil <[email protected]>

Quoted 7 lines · Alex Elder

More cleanups, this time in the messenger code. -Alex
--
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-03-02 23:29 UTC #7 lore raw

Re: [PATCH 4/4] libceph: a few small changes

In reply to Sage Weil

Quoted 7 lines · Sage Weil

Quoted 4 lines
net/ceph/messenger.c +1 -0
+	default:
Quoted 3 lines
+ WARN(1, "unexpected socket state (%d)", sk->sk_state);
 +		break;
   	}
This is a red herring... the switch isn't meant to be exhaustive, only to catch the interesting (shutdown) states. I think #2099 can be closed as well.

That's pretty funny. Didn't I open that bug?

I will drop the WARN() but will add a comment to indicate it's explicitly not meant to cover all expected cases.

-Alex