[PATCH 0/4] libceph: miscellaneous cleanups

[PATCH 0/4] libceph: miscellaneous cleanups

[Alex Elder]

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

[PATCH 1/4] libceph: make ceph_msgr_wq private

[Alex Elder]

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

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

diff --git a/include/linux/ceph/messenger.h b/include/linux/ceph/messenger.h
index 6b5af5f..5ca0f82 100644
--- a/include/linux/ceph/messenger.h
+++ b/include/linux/ceph/messenger.h
@@ -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.
   */
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 03868cd..9c0f73a 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -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)
  {
-- 
1.7.5.4

[PATCH 2/4] libceph: encapsulate some messenger cleanup code

[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 <elder@dreamhost.com>
---
  net/ceph/messenger.c |   38 ++++++++++++++++++++------------------
  1 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 9c0f73a..2e496e2 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -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);

-- 
1.7.5.4

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

[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 <elder@dreamhost.com>
---
  net/ceph/messenger.c |   14 ++++++--------
  1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 2e496e2..9a8a479 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -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;
  		}
  	}
-- 
1.7.5.4

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

[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 <elder@dreamhost.com>
---
  net/ceph/messenger.c |   33 +++++++++++++++++++--------------
  1 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 9a8a479..d793b9f 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -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);
  }

  /*
-- 
1.7.5.4

[PATCH, v2] libceph: encapsulate some messenger cleanup code

[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 <elder@dreamhost.com>

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

Index: b/net/ceph/messenger.c
===================================================================
--- a/net/ceph/messenger.c
+++ 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);

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

[Sage Weil]

On Tue, 28 Feb 2012, Alex Elder wrote:

> 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 <elder@dreamhost.com>
> ---
>  net/ceph/messenger.c |   33 +++++++++++++++++++--------------
>  1 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> index 9a8a479..d793b9f 100644
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -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;
>  	}

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 <sage@newdream.net>


>  }
> 
> @@ -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);
>  }
> 
>  /*
> -- 
> 1.7.5.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 0/4] libceph: miscellaneous cleanups

[Sage Weil]

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 <sage@newdream.net>


On Tue, 28 Feb 2012, Alex Elder wrote:

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

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

[Alex Elder]

On 03/02/2012 01:40 PM, Sage Weil wrote:
>> +	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.

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