libceph: clean up some code involving CRCs

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

Share & tools

Alex Elder

author

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

cover libceph: clean up some code involving CRCs

Rename a few variables so it's clearer which indicate that a CRC should be computed and which hold an actual CRC value. Separate CRC calculation from byte swapping to improve readability. And move some code that runs on only on the last pass through a couple of loops outside the loop body.

-Alex

Alex Elder

author

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

PATCH 1/3 libceph: use "do" in CRC-related Boolean variables

Change the name (and type) of a few CRC-related Boolean local variables so they contain the word "do", to distingish their purpose from variables used for holding an actual CRC value.

Note that in the process of doing this I identified a fairly serious logic error in write_partial_msg_pages(): the value of "do_crc" assigned appears to be the opposite of what it should be. No attempt to fix this is made here; this change preserves the erroneous behavior. The problem I found is documented here:

    http://tracker.newdream.net/issues/2064

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

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

include/linux/ceph/messenger.h +1 -1

index 5ca0f82..3bff047 100644
@@ -98,7 +98,7 @@ struct ceph_msg {
 struct ceph_msg_pos {
 	int page, page_pos;  /* which page; offset in page */
 	int data_pos;        /* offset in data payload */
-	int did_page_crc;    /* true if we've calculated crc for current page */
+	bool did_page_crc;   /* true if we've calculated crc for current page */
 };
 
 /* ceph connection fault delay defaults, for exponential backoff */

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

index d793b9f..8ac2f33 100644
@@ -592,7 +592,7 @@ static void prepare_write_message(struct ceph_connection *con)
 		else
 			con->out_msg_pos.page_pos = 0;
 		con->out_msg_pos.data_pos = 0;
-		con->out_msg_pos.did_page_crc = 0;
+		con->out_msg_pos.did_page_crc = false;
 		con->out_more = 1;  /* data + footer will follow */
 	} else {
 		/* no, queue up footer too and be done */

@@ -802,7 +802,7 @@ static int write_partial_msg_pages(struct ceph_connection *con)
 	struct ceph_msg *msg = con->out_msg;
 	unsigned data_len = le32_to_cpu(msg->hdr.data_len);
 	size_t len;
-	int crc = con->msgr->nocrc;
+	bool do_crc = con->msgr->nocrc;
 	int ret;
 	int total_max_write;
 	int in_trail = 0;

@@ -840,17 +840,17 @@ static int write_partial_msg_pages(struct ceph_connection *con)
 
 			page = list_first_entry(&msg->trail->head,
 						struct page, lru);
-			if (crc)
+			if (do_crc)
 				kaddr = kmap(page);
 			max_write = PAGE_SIZE;
 		} else if (msg->pages) {
 			page = msg->pages[con->out_msg_pos.page];
-			if (crc)
+			if (do_crc)
 				kaddr = kmap(page);
 		} else if (msg->pagelist) {
 			page = list_first_entry(&msg->pagelist->head,
 						struct page, lru);
-			if (crc)
+			if (do_crc)
 				kaddr = kmap(page);
 #ifdef CONFIG_BLOCK
 		} else if (msg->bio) {

@@ -859,26 +859,26 @@ static int write_partial_msg_pages(struct ceph_connection *con)
 			bv = bio_iovec_idx(msg->bio_iter, msg->bio_seg);
 			page = bv->bv_page;
 			page_shift = bv->bv_offset;
-			if (crc)
+			if (do_crc)
 				kaddr = kmap(page) + page_shift;
 			max_write = bv->bv_len;
 #endif
 		} else {
 			page = zero_page;
-			if (crc)
+			if (do_crc)
 				kaddr = zero_page_address;
 		}
 		len = min_t(int, max_write - con->out_msg_pos.page_pos,
 			    total_max_write);
 
-		if (crc && !con->out_msg_pos.did_page_crc) {
+		if (do_crc && !con->out_msg_pos.did_page_crc) {
 			void *base = kaddr + con->out_msg_pos.page_pos;
 			u32 tmpcrc = le32_to_cpu(con->out_msg->footer.data_crc);
 
 			BUG_ON(kaddr == NULL);
 			con->out_msg->footer.data_crc =
 				cpu_to_le32(crc32c(tmpcrc, base, len));
-			con->out_msg_pos.did_page_crc = 1;
+			con->out_msg_pos.did_page_crc = true;
 		}
 		ret = kernel_sendpage(con->sock, page,
 				      con->out_msg_pos.page_pos + page_shift,

@@ -886,7 +886,7 @@ static int write_partial_msg_pages(struct ceph_connection *con)
 				      MSG_DONTWAIT | MSG_NOSIGNAL |
 				      MSG_MORE);
 
-		if (crc &&
+		if (do_crc &&
 		    (msg->pages || msg->pagelist || msg->bio || in_trail))
 			kunmap(page);

@@ -900,7 +900,7 @@ static int write_partial_msg_pages(struct ceph_connection *con)
 		if (ret == len) {
 			con->out_msg_pos.page_pos = 0;
 			con->out_msg_pos.page++;
-			con->out_msg_pos.did_page_crc = 0;
+			con->out_msg_pos.did_page_crc = false;
 			if (in_trail)
 				list_move_tail(&page->lru,
 					       &msg->trail->head);

@@ -917,7 +917,7 @@ static int write_partial_msg_pages(struct ceph_connection *con)
 	dout("write_partial_msg_pages %p msg %p done\n", con, msg);
 
 	/* prepare and queue up footer, too */
-	if (!crc)
+	if (!do_crc)
 		con->out_msg->footer.flags |= CEPH_MSG_FOOTER_NOCRC;
 	ceph_con_out_kvec_reset(con);
 	prepare_write_message_footer(con);

@@ -1554,7 +1554,7 @@ static struct ceph_msg *ceph_alloc_msg(struct ceph_connection *con,
 
 static int read_partial_message_pages(struct ceph_connection *con,
 				      struct page **pages,
-				      unsigned data_len, int datacrc)
+				      unsigned data_len, bool do_datacrc)
 {
 	void *p;
 	int ret;

@@ -1567,7 +1567,7 @@ static int read_partial_message_pages(struct ceph_connection *con,
 	p = kmap(pages[con->in_msg_pos.page]);
 	ret = ceph_tcp_recvmsg(con->sock, p + con->in_msg_pos.page_pos,
 			       left);
-	if (ret > 0 && datacrc)
+	if (ret > 0 && do_datacrc)
 		con->in_data_crc =
 			crc32c(con->in_data_crc,
 				  p + con->in_msg_pos.page_pos, ret);

@@ -1587,7 +1587,7 @@ static int read_partial_message_pages(struct ceph_connection *con,
 #ifdef CONFIG_BLOCK
 static int read_partial_message_bio(struct ceph_connection *con,
 				    struct bio **bio_iter, int *bio_seg,
-				    unsigned data_len, int datacrc)
+				    unsigned data_len, bool do_datacrc)
 {
 	struct bio_vec *bv = bio_iovec_idx(*bio_iter, *bio_seg);
 	void *p;

@@ -1603,7 +1603,7 @@ static int read_partial_message_bio(struct ceph_connection *con,
 
 	ret = ceph_tcp_recvmsg(con->sock, p + con->in_msg_pos.page_pos,
 			       left);
-	if (ret > 0 && datacrc)
+	if (ret > 0 && do_datacrc)
 		con->in_data_crc =
 			crc32c(con->in_data_crc,
 				  p + con->in_msg_pos.page_pos, ret);

@@ -1630,7 +1630,7 @@ static int read_partial_message(struct ceph_connection *con)
 	int ret;
 	int to, left;
 	unsigned front_len, middle_len, data_len;
-	int datacrc = con->msgr->nocrc;
+	bool do_datacrc = con->msgr->nocrc;
 	int skip;
 	u64 seq;

@@ -1741,7 +1741,7 @@ static int read_partial_message(struct ceph_connection *con)
 	while (con->in_msg_pos.data_pos < data_len) {
 		if (m->pages) {
 			ret = read_partial_message_pages(con, m->pages,
-						 data_len, datacrc);
+						 data_len, do_datacrc);
 			if (ret <= 0)
 				return ret;
 #ifdef CONFIG_BLOCK

@@ -1749,7 +1749,7 @@ static int read_partial_message(struct ceph_connection *con)
 
 			ret = read_partial_message_bio(con,
 						 &m->bio_iter, &m->bio_seg,
-						 data_len, datacrc);
+						 data_len, do_datacrc);
 			if (ret <= 0)
 				return ret;
 #endif

@@ -1784,7 +1784,7 @@ static int read_partial_message(struct ceph_connection *con)
 		       m, con->in_middle_crc, m->footer.middle_crc);
 		return -EBADMSG;
 	}
-	if (datacrc &&
+	if (do_datacrc &&
 	    (m->footer.flags & CEPH_MSG_FOOTER_NOCRC) == 0 &&
 	    con->in_data_crc != le32_to_cpu(m->footer.data_crc)) {
 		pr_err("read_partial_message %p data crc %u != exp. %u\n", m,

Signature · 1 line

1.7.5.4

Alex Elder

author

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

PATCH 2/3 libceph: separate CRC calculation from byte swapping

In reply to Alex Elder

Calculate CRC in a separate step from rearranging the byte order of the result, to improve clarity and readability.

Use offsetof() to determine the number of bytes to include in the CRC calculation.

In read_partial_message(), switch which value gets byte-swapped, since the just-computed CRC is already likely to be in a register.

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

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

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

index 8ac2f33..1df7e6f 100644
@@ -518,6 +518,7 @@ static void prepare_write_message_footer(struct ceph_connection *con)
 static void prepare_write_message(struct ceph_connection *con)
 {
 	struct ceph_msg *m;
+	u32 crc;
 
 	ceph_con_out_kvec_reset(con);
 	con->out_kvec_is_msg = true;

@@ -566,17 +567,17 @@ static void prepare_write_message(struct ceph_connection *con)
 			m->middle->vec.iov_base);
 
 	/* fill in crc (except data pages), footer */
-	con->out_msg->hdr.crc =
-		cpu_to_le32(crc32c(0, &m->hdr,
-				      sizeof(m->hdr) - sizeof(m->hdr.crc)));
+	crc = crc32c(0, &m->hdr, offsetof(struct ceph_msg_header, crc));
+	con->out_msg->hdr.crc = cpu_to_le32(crc);
 	con->out_msg->footer.flags = CEPH_MSG_FOOTER_COMPLETE;
-	con->out_msg->footer.front_crc =
-		cpu_to_le32(crc32c(0, m->front.iov_base, m->front.iov_len));
-	if (m->middle)
-		con->out_msg->footer.middle_crc =
-			cpu_to_le32(crc32c(0, m->middle->vec.iov_base,
-					   m->middle->vec.iov_len));
-	else
+
+	crc = crc32c(0, m->front.iov_base, m->front.iov_len);
+	con->out_msg->footer.front_crc = cpu_to_le32(crc);
+	if (m->middle) {
+		crc = crc32c(0, m->middle->vec.iov_base,
+				m->middle->vec.iov_len);
+		con->out_msg->footer.middle_crc = cpu_to_le32(crc);
+	} else
 		con->out_msg->footer.middle_crc = 0;
 	con->out_msg->footer.data_crc = 0;
 	dout("prepare_write_message front_crc %u data_crc %u\n",

@@ -872,12 +873,13 @@ static int write_partial_msg_pages(struct ceph_connection *con)
 			    total_max_write);
 
 		if (do_crc && !con->out_msg_pos.did_page_crc) {
+			u32 crc;
 			void *base = kaddr + con->out_msg_pos.page_pos;
 			u32 tmpcrc = le32_to_cpu(con->out_msg->footer.data_crc);
 
 			BUG_ON(kaddr == NULL);
-			con->out_msg->footer.data_crc =
-				cpu_to_le32(crc32c(tmpcrc, base, len));
+			crc = crc32c(tmpcrc, base, len);
+			con->out_msg->footer.data_crc = cpu_to_le32(crc);
 			con->out_msg_pos.did_page_crc = true;
 		}
 		ret = kernel_sendpage(con->sock, page,

@@ -1647,8 +1649,9 @@ static int read_partial_message(struct ceph_connection *con)
 		con->in_base_pos += ret;
 		if (con->in_base_pos == sizeof(con->in_hdr)) {
 			u32 crc = crc32c(0, &con->in_hdr,
-				 sizeof(con->in_hdr) - sizeof(con->in_hdr.crc));
-			if (crc != le32_to_cpu(con->in_hdr.crc)) {
+					offsetof(struct ceph_msg_header, crc));
+
+			if (cpu_to_le32(crc) != con->in_hdr.crc) {
 				pr_err("read_partial_message bad hdr "
 				       " crc %u != expected %u\n",
 				       crc, con->in_hdr.crc);

Signature · 1 line

1.7.5.4

Alex Elder

author

2012-02-29 04:50 UTC #3 lore raw

PATCH 3/3 libceph: do crc calculations outside loop

In reply to Alex Elder

Move blocks of code out of loops in read_partial_message_section() and read_partial_message(). They were only was getting called at the end of the last iteration of the loop anyway.

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

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

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

index 1df7e6f..693be64 100644
@@ -1541,10 +1541,9 @@ static int read_partial_message_section(struct ceph_connection *con,
 		if (ret <= 0)
 			return ret;
 		section->iov_len += ret;
-		if (section->iov_len == sec_len)
-			*crc = crc32c(0, section->iov_base,
-				      section->iov_len);
 	}
+	if (section->iov_len == sec_len)
+		*crc = crc32c(0, section->iov_base, section->iov_len);
 
 	return 1;
 }

@@ -1635,6 +1634,7 @@ static int read_partial_message(struct ceph_connection *con)
 	bool do_datacrc = con->msgr->nocrc;
 	int skip;
 	u64 seq;
+	u32 crc;
 
 	dout("read_partial_message con %p msg %p\n", con, m);

@@ -1647,18 +1647,16 @@ static int read_partial_message(struct ceph_connection *con)
 		if (ret <= 0)
 			return ret;
 		con->in_base_pos += ret;
-		if (con->in_base_pos == sizeof(con->in_hdr)) {
-			u32 crc = crc32c(0, &con->in_hdr,
-					offsetof(struct ceph_msg_header, crc));
-
-			if (cpu_to_le32(crc) != con->in_hdr.crc) {
-				pr_err("read_partial_message bad hdr "
-				       " crc %u != expected %u\n",
-				       crc, con->in_hdr.crc);
-				return -EBADMSG;
-			}
-		}
 	}
+
+	crc = crc32c(0, &con->in_hdr, offsetof(struct ceph_msg_header, crc));
+	if (cpu_to_le32(crc) != con->in_hdr.crc) {
+		pr_err("read_partial_message bad hdr "
+		       " crc %u != expected %u\n",
+		       crc, con->in_hdr.crc);
+		return -EBADMSG;
+	}
+
 	front_len = le32_to_cpu(con->in_hdr.front_len);
 	if (front_len > CEPH_MSG_MAX_FRONT_LEN)
 		return -EIO;

Signature · 1 line

1.7.5.4

Sage Weil

Reviewed-by on the v1 series

2012-03-02 21:47 UTC #4 lore raw

In reply to Alex Elder

These look good.. but where is the crc bug actually fixed? Not seeing that patch...

Reviewed-by Sage Weil <[email protected]>

Quoted 14 lines · Alex Elder

Rename a few variables so it's clearer which indicate that a CRC should be computed and which hold an actual CRC value. Separate CRC calculation from byte swapping to improve readability. And move some code that runs on only on the last pass through a couple of loops outside the loop body.
-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:30 UTC #5 lore raw

Quoted 2 lines · Sage Weil

These look good.. but where is the crc bug actually fixed? Not seeing that patch...

I haven't addressed it yet. Soon, very soon...

-Alex

Quoted 18 lines

Reviewed-by Sage Weil<[email protected]>
Quoted 14 lines · Alex Elder
Rename a few variables so it's clearer which indicate that a CRC should be computed and which hold an actual CRC value. Separate CRC calculation from byte swapping to improve readability. And move some code that runs on only on the last pass through a couple of loops outside the loop body.
-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