[PATCH 0/2] libceph: more miscellaneous cleanups

[PATCH 0/2] libceph: more miscellaneous cleanups

[Alex Elder]

Messaging code again.  The biggest improvement here is encapsulating
the code that puts data into the write vectors.  The second patch just
adds more simple cleanups.

					-Alex

Re: [PATCH 0/2] libceph: more miscellaneous cleanups

[Alex Elder]

On 02/28/2012 08:52 PM, Alex Elder wrote:
> Messaging code again. The biggest improvement here is encapsulating
> the code that puts data into the write vectors. The second patch just
> adds more simple cleanups.

What I meant to say was...

These are two simple patches, the first is just a simple logic change
whose result I thought was slightly more readable.

					-Alex

[PATCH 1/2] libceph: small refactor in write_partial_kvec()

[Alex Elder]

Make a small change in the code that counts down kvecs consumed by
a ceph_tcp_sendmsg() call.  Same functionality, just blocked out
a little differently.

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

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 693be64..63f281f 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -744,17 +744,18 @@ static int write_partial_kvec(struct 
ceph_connection *con)
  		con->out_kvec_bytes -= ret;
  		if (con->out_kvec_bytes == 0)
  			break;            /* done */
-		while (ret > 0) {
-			if (ret >= con->out_kvec_cur->iov_len) {
-				ret -= con->out_kvec_cur->iov_len;
-				con->out_kvec_cur++;
-				con->out_kvec_left--;
-			} else {
-				con->out_kvec_cur->iov_len -= ret;
-				con->out_kvec_cur->iov_base += ret;
-				ret = 0;
-				break;
-			}
+
+		/* account for full iov entries consumed */
+		while (ret >= con->out_kvec_cur->iov_len) {
+			BUG_ON(!con->out_kvec_left);
+			ret -= con->out_kvec_cur->iov_len;
+			con->out_kvec_cur++;
+			con->out_kvec_left--;
+		}
+		/* and for a partially-consumed entry */
+		if (ret) {
+			con->out_kvec_cur->iov_len -= ret;
+			con->out_kvec_cur->iov_base += ret;
  		}
  	}
  	con->out_kvec_left = 0;
-- 
1.7.5.4

[PATCH 2/2] libceph: some simple changes

[Alex Elder]

Nothing too big here.
     - define the size of the buffer used for consuming ignored
       incoming data using a symbolic constant
     - simplify the condition determining whether to unmap the page
       in write_partial_msg_pages(): do it for crc but not if the
       page is the zero page

Signed-off-by: Alex Elder <elder@dreamhost.com>
---
  net/ceph/messenger.c |   13 +++++++++----
  1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 63f281f..27a1364 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -38,6 +38,11 @@ static char tag_keepalive = CEPH_MSGR_TAG_KEEPALIVE;
  static struct lock_class_key socket_class;
  #endif

+/*
+ * When skipping (ignoring) a block of input we read it into a "skip
+ * buffer," which is this many bytes in size.
+ */
+#define SKIP_BUF_SIZE	1024

  static void queue_con(struct ceph_connection *con);
  static void con_work(struct work_struct *);
@@ -889,8 +894,7 @@ static int write_partial_msg_pages(struct 
ceph_connection *con)
  				      MSG_DONTWAIT | MSG_NOSIGNAL |
  				      MSG_MORE);

-		if (do_crc &&
-		    (msg->pages || msg->pagelist || msg->bio || in_trail))
+		if (do_crc && kaddr != zero_page_address)
  			kunmap(page);

  		if (ret == -EAGAIN)
@@ -1979,8 +1983,9 @@ more:
  		 *
  		 * FIXME: there must be a better way to do this!
  		 */
-		static char buf[1024];
-		int skip = min(1024, -con->in_base_pos);
+		static char buf[SKIP_BUF_SIZE];
+		int skip = min((int) sizeof buf, -con->in_base_pos);
+
  		dout("skipping %d / %d bytes\n", skip, -con->in_base_pos);
  		ret = ceph_tcp_recvmsg(con->sock, buf, skip);
  		if (ret <= 0)
-- 
1.7.5.4

Re: [PATCH 2/2] libceph: some simple changes

[Sage Weil]

Looks ok, as long as this isn't broken if the moving zero_page_address 
inside write_partial_msg_pages().

Reviewed-by: Sage Weil <sage@newdream.net>

On Tue, 28 Feb 2012, Alex Elder wrote:

> Nothing too big here.
>     - define the size of the buffer used for consuming ignored
>       incoming data using a symbolic constant
>     - simplify the condition determining whether to unmap the page
>       in write_partial_msg_pages(): do it for crc but not if the
>       page is the zero page
> 
> Signed-off-by: Alex Elder <elder@dreamhost.com>
> ---
>  net/ceph/messenger.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> index 63f281f..27a1364 100644
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -38,6 +38,11 @@ static char tag_keepalive = CEPH_MSGR_TAG_KEEPALIVE;
>  static struct lock_class_key socket_class;
>  #endif
> 
> +/*
> + * When skipping (ignoring) a block of input we read it into a "skip
> + * buffer," which is this many bytes in size.
> + */
> +#define SKIP_BUF_SIZE	1024
> 
>  static void queue_con(struct ceph_connection *con);
>  static void con_work(struct work_struct *);
> @@ -889,8 +894,7 @@ static int write_partial_msg_pages(struct ceph_connection
> *con)
>  				      MSG_DONTWAIT | MSG_NOSIGNAL |
>  				      MSG_MORE);
> 
> -		if (do_crc &&
> -		    (msg->pages || msg->pagelist || msg->bio || in_trail))
> +		if (do_crc && kaddr != zero_page_address)
>  			kunmap(page);
> 
>  		if (ret == -EAGAIN)
> @@ -1979,8 +1983,9 @@ more:
>  		 *
>  		 * FIXME: there must be a better way to do this!
>  		 */
> -		static char buf[1024];
> -		int skip = min(1024, -con->in_base_pos);
> +		static char buf[SKIP_BUF_SIZE];
> +		int skip = min((int) sizeof buf, -con->in_base_pos);
> +
>  		dout("skipping %d / %d bytes\n", skip, -con->in_base_pos);
>  		ret = ceph_tcp_recvmsg(con->sock, buf, skip);
>  		if (ret <= 0)
> -- 
> 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
> 
>