[PATCH 0/5] ceph: abstract message data information setting

[PATCH 0/5] ceph: abstract message data information setting

[Alex Elder]

The first two of these were posted before, but they ran into
trouble because assertions that data information fields in
ceph messages got set only once were failing.  Now those
assertions are commented out--initially.

The third and fourth patches in this series address the
reasons the assertions could fail, by changing the osd
client to only set message data fields once, and by changing
the mds client to only set them if there is data to be sent.

The final patch activates those commented-out assertions.

These are available in the branch "review/wip-abstract_data"
in the ceph-client git repository.

					-Alex

[PATCH 1/5] libceph: isolate message page field manipulation
[PATCH 2/5] libceph: isolate other message data fields
[PATCH 3/5] ceph: only set message data pointers if non-empty
[PATCH 4/5] libceph: set response data fields earlier
[PATCH 5/5] libceph: activate message data assignment checks

[PATCH 1/5] libceph: isolate message page field manipulation

[Alex Elder]

Define a function ceph_msg_data_set_pages(), which more clearly
abstracts the assignment page-related fields for data in a ceph
message structure.  Use this new function in the osd client and mds
client.

Ideally, these fields would never be set more than once (with
BUG_ON() calls to guarantee that).  At the moment though the osd
client sets these every time it recieves a message, and in the event
of a communication problem this can happen more than once.  (This
will be resolved shortly, but setting up these helpers first makes
it all a bit easier to work with.)

Rearrange the field order in a ceph_msg structure to group those
that are used to define the possible data payloads.

This partially resolves:
    http://tracker.ceph.com/issues/4263

Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Josh Durgin <josh.durgin@inktank.com>
---
 fs/ceph/mds_client.c           |    4 ++--
 include/linux/ceph/messenger.h |   22 +++++++++++++---------
 net/ceph/messenger.c           |   13 +++++++++++++
 net/ceph/osd_client.c          |   11 +++++------
 4 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index afb9dc9..c8f6be9 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -1718,8 +1718,8 @@ static struct ceph_msg
*create_request_message(struct ceph_mds_client *mdsc,
 	msg->front.iov_len = p - msg->front.iov_base;
 	msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);

-	msg->pages = req->r_pages;
-	msg->page_count = req->r_num_pages;
+	ceph_msg_data_set_pages(msg, req->r_pages, req->r_num_pages, 0);
+
 	msg->hdr.data_len = cpu_to_le32(req->r_data_len);
 	msg->hdr.data_off = cpu_to_le16(0);

diff --git a/include/linux/ceph/messenger.h b/include/linux/ceph/messenger.h
index 6c11874..aa463b9 100644
--- a/include/linux/ceph/messenger.h
+++ b/include/linux/ceph/messenger.h
@@ -74,21 +74,22 @@ struct ceph_msg {
 	struct ceph_msg_footer footer;	/* footer */
 	struct kvec front;              /* unaligned blobs of message */
 	struct ceph_buffer *middle;
-	struct page **pages;            /* data payload.  NOT OWNER. */
-	unsigned page_count;		/* size of page array */
-	unsigned page_alignment;        /* io offset in first page */
-	struct ceph_pagelist *pagelist; /* instead of pages */
-
-	struct ceph_connection *con;
-	struct list_head list_head;

-	struct kref kref;
+	struct page **pages;		/* data payload.  NOT OWNER. */
+	unsigned int page_alignment;	/* io offset in first page */
+	unsigned int page_count;	/* # pages in array or list */
+	struct ceph_pagelist *pagelist; /* instead of pages */
 #ifdef CONFIG_BLOCK
+	unsigned int bio_seg;		/* current bio segment */
 	struct bio  *bio;		/* instead of pages/pagelist */
 	struct bio  *bio_iter;		/* bio iterator */
-	unsigned int bio_seg;		/* current bio segment */
 #endif /* CONFIG_BLOCK */
 	struct ceph_pagelist *trail;	/* the trailing part of the data */
+
+	struct ceph_connection *con;
+	struct list_head list_head;	/* links for connection lists */
+
+	struct kref kref;
 	bool front_is_vmalloc;
 	bool more_to_follow;
 	bool needs_out_seq;
@@ -218,6 +219,9 @@ extern void ceph_msg_revoke_incoming(struct ceph_msg
*msg);

 extern void ceph_con_keepalive(struct ceph_connection *con);

+extern void ceph_msg_data_set_pages(struct ceph_msg *msg, struct page
**pages,
+				unsigned int page_count, size_t alignment);
+
 extern struct ceph_msg *ceph_msg_new(int type, int front_len, gfp_t flags,
 				     bool can_fail);
 extern void ceph_msg_kfree(struct ceph_msg *m);
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index c7d4278..a23faba 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -2674,6 +2674,19 @@ void ceph_con_keepalive(struct ceph_connection *con)
 }
 EXPORT_SYMBOL(ceph_con_keepalive);

+void ceph_msg_data_set_pages(struct ceph_msg *msg, struct page **pages,
+		unsigned int page_count, size_t alignment)
+{
+	/* BUG_ON(!pages); */
+	/* BUG_ON(!page_count); */
+	/* BUG_ON(msg->pages); */
+	/* BUG_ON(msg->page_count); */
+
+	msg->pages = pages;
+	msg->page_count = page_count;
+	msg->page_alignment = alignment & PAGE_SIZE;
+}
+EXPORT_SYMBOL(ceph_msg_data_set_pages);

 /*
  * construct a new message with given type, size
diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index f9cf445..db777a4 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -1753,9 +1753,8 @@ int ceph_osdc_start_request(struct ceph_osd_client
*osdc,

 	osd_data = &req->r_data_out;
 	if (osd_data->type == CEPH_OSD_DATA_TYPE_PAGES) {
-		req->r_request->pages = osd_data->pages;
-		req->r_request->page_count = osd_data->num_pages;
-		req->r_request->page_alignment = osd_data->alignment;
+		ceph_msg_data_set_pages(req->r_request, osd_data->pages,
+			osd_data->num_pages, osd_data->alignment);
 #ifdef CONFIG_BLOCK
 	} else if (osd_data->type == CEPH_OSD_DATA_TYPE_BIO) {
 		req->r_request->bio = osd_data->bio;
@@ -2127,9 +2126,9 @@ static struct ceph_msg *get_reply(struct
ceph_connection *con,
 				m = NULL;
 				goto out;
 			}
-			m->pages = osd_data->pages;
-			m->page_count = osd_data->num_pages;
-			m->page_alignment = osd_data->alignment;
+
+			ceph_msg_data_set_pages(m, osd_data->pages,
+				osd_data->num_pages, osd_data->alignment);
 #ifdef CONFIG_BLOCK
 		} else if (osd_data->type == CEPH_OSD_DATA_TYPE_BIO) {
 			m->bio = osd_data->bio;
-- 
1.7.9.5

Re: [PATCH 1/5] libceph: isolate message page field manipulation

[Alex Elder]

On 03/05/2013 07:52 AM, Alex Elder wrote:
> +void ceph_msg_data_set_pages(struct ceph_msg *msg, struct page **pages,
> +		unsigned int page_count, size_t alignment)
> +{
> +	/* BUG_ON(!pages); */
> +	/* BUG_ON(!page_count); */
> +	/* BUG_ON(msg->pages); */
> +	/* BUG_ON(msg->page_count); */
> +
> +	msg->pages = pages;
> +	msg->page_count = page_count;
> +	msg->page_alignment = alignment & PAGE_SIZE;

Whoops, this is supposed to be:
	msg->page_alignment = alignment & ~PAGE_MASK;

I think it works despite the bug, it just isn't very
good for performance on the other end...

I will fix this before I commit anything (and after
review).

					-Alex

> +}
> +EXPORT_SYMBOL(ceph_msg_data_set_pages);

[PATCH 2/5] libceph: isolate other message data fields

[Alex Elder]

Define ceph_msg_data_set_pagelist(), ceph_msg_data_set_bio(), and
ceph_msg_data_set_trail() to clearly abstract the assignment the
remaining data-related fields in a ceph message structure.  Use the
new functions in the osd client and mds client.

This partially resolves:
    http://tracker.ceph.com/issues/4263

Signed-off-by: Alex Elder <elder@inktank.com>
Reviewed-by: Josh Durgin <josh.durgin@inktank.com>
---
 fs/ceph/mds_client.c           |    2 +-
 include/linux/ceph/messenger.h |    5 +++++
 net/ceph/messenger.c           |   28 ++++++++++++++++++++++++++++
 net/ceph/osd_client.c          |    6 +++---
 4 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index c8f6be9..42400ce 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -2599,7 +2599,7 @@ static void send_mds_reconnect(struct
ceph_mds_client *mdsc,
 			goto fail;
 	}

-	reply->pagelist = pagelist;
+	ceph_msg_data_set_pagelist(reply, pagelist);
 	if (recon_state.flock)
 		reply->hdr.version = cpu_to_le16(2);
 	reply->hdr.data_len = cpu_to_le32(pagelist->length);
diff --git a/include/linux/ceph/messenger.h b/include/linux/ceph/messenger.h
index aa463b9..8e55d86 100644
--- a/include/linux/ceph/messenger.h
+++ b/include/linux/ceph/messenger.h
@@ -221,6 +221,11 @@ extern void ceph_con_keepalive(struct
ceph_connection *con);

 extern void ceph_msg_data_set_pages(struct ceph_msg *msg, struct page
**pages,
 				unsigned int page_count, size_t alignment);
+extern void ceph_msg_data_set_pagelist(struct ceph_msg *msg,
+				struct ceph_pagelist *pagelist);
+extern void ceph_msg_data_set_bio(struct ceph_msg *msg, struct bio *bio);
+extern void ceph_msg_data_set_trail(struct ceph_msg *msg,
+				struct ceph_pagelist *trail);

 extern struct ceph_msg *ceph_msg_new(int type, int front_len, gfp_t flags,
 				     bool can_fail);
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index a23faba..97506ac 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -2688,6 +2688,34 @@ void ceph_msg_data_set_pages(struct ceph_msg
*msg, struct page **pages,
 }
 EXPORT_SYMBOL(ceph_msg_data_set_pages);

+void ceph_msg_data_set_pagelist(struct ceph_msg *msg,
+				struct ceph_pagelist *pagelist)
+{
+	/* BUG_ON(!pagelist); */
+	/* BUG_ON(msg->pagelist); */
+
+	msg->pagelist = pagelist;
+}
+EXPORT_SYMBOL(ceph_msg_data_set_pagelist);
+
+void ceph_msg_data_set_bio(struct ceph_msg *msg, struct bio *bio)
+{
+	/* BUG_ON(!bio); */
+	/* BUG_ON(msg->bio); */
+
+	msg->bio = bio;
+}
+EXPORT_SYMBOL(ceph_msg_data_set_bio);
+
+void ceph_msg_data_set_trail(struct ceph_msg *msg, struct ceph_pagelist
*trail)
+{
+	/* BUG_ON(!trail); */
+	/* BUG_ON(msg->trail); */
+
+	msg->trail = trail;
+}
+EXPORT_SYMBOL(ceph_msg_data_set_trail);
+
 /*
  * construct a new message with given type, size
  * the new msg has a ref count of 1.
diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index db777a4..c19188a 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -1757,12 +1757,12 @@ int ceph_osdc_start_request(struct
ceph_osd_client *osdc,
 			osd_data->num_pages, osd_data->alignment);
 #ifdef CONFIG_BLOCK
 	} else if (osd_data->type == CEPH_OSD_DATA_TYPE_BIO) {
-		req->r_request->bio = osd_data->bio;
+		ceph_msg_data_set_bio(req->r_request, osd_data->bio);
 #endif
 	} else {
 		BUG_ON(osd_data->type != CEPH_OSD_DATA_TYPE_NONE);
 	}
-	req->r_request->trail = &req->r_trail;
+	ceph_msg_data_set_trail(req->r_request, &req->r_trail);

 	register_request(osdc, req);

@@ -2131,7 +2131,7 @@ static struct ceph_msg *get_reply(struct
ceph_connection *con,
 				osd_data->num_pages, osd_data->alignment);
 #ifdef CONFIG_BLOCK
 		} else if (osd_data->type == CEPH_OSD_DATA_TYPE_BIO) {
-			m->bio = osd_data->bio;
+			ceph_msg_data_set_bio(m, osd_data->bio);
 #endif
 		}
 	}
-- 
1.7.9.5

[PATCH 3/5] ceph: only set message data pointers if non-empty

[Alex Elder]

The ceph file system doesn't typically send information in the
data portion of a message.  (It relies on some functionality
exported by the osd client to read and write page data.)

There are two spots it does send data though.  The value assigned to
an extended attribute is held in one or more pages allocated by
ceph_sync_setxattr().  Eventually those pages are assigned to a
request message in create_request_message().

The second spot is when sending a reconnect message, where a
ceph pagelist is used to build up an array of snaprealm_reconnect
structures to send to the mds.

Change it so we only assign the outgoing data information for
these messages if there is outgoing data to send.

This is related to:
    http://tracker.ceph.com/issues/4284

Signed-off-by: Alex Elder <elder@inktank.com>
---
 fs/ceph/mds_client.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
index 42400ce..ae83aa9 100644
--- a/fs/ceph/mds_client.c
+++ b/fs/ceph/mds_client.c
@@ -1718,7 +1718,12 @@ static struct ceph_msg
*create_request_message(struct ceph_mds_client *mdsc,
 	msg->front.iov_len = p - msg->front.iov_base;
 	msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);

-	ceph_msg_data_set_pages(msg, req->r_pages, req->r_num_pages, 0);
+	if (req->r_num_pages) {
+		/* outbound data set only by ceph_sync_setxattr() */
+		BUG_ON(!req->r_pages);
+		ceph_msg_data_set_pages(msg, req->r_pages,
+				req->r_num_pages, 0);
+	}

 	msg->hdr.data_len = cpu_to_le32(req->r_data_len);
 	msg->hdr.data_off = cpu_to_le16(0);
@@ -2599,10 +2604,13 @@ static void send_mds_reconnect(struct
ceph_mds_client *mdsc,
 			goto fail;
 	}

-	ceph_msg_data_set_pagelist(reply, pagelist);
 	if (recon_state.flock)
 		reply->hdr.version = cpu_to_le16(2);
-	reply->hdr.data_len = cpu_to_le32(pagelist->length);
+	if (pagelist->length) {
+		/* set up outbound data if we have any */
+		reply->hdr.data_len = cpu_to_le32(pagelist->length);
+		ceph_msg_data_set_pagelist(reply, pagelist);
+	}
 	ceph_con_send(&session->s_con, reply);

 	mutex_unlock(&session->s_mutex);
-- 
1.7.9.5

Re: [PATCH 3/5] ceph: only set message data pointers if non-empty

[Greg Farnum]

On Tuesday, March 5, 2013 at 5:53 AM, Alex Elder wrote:
> The ceph file system doesn't typically send information in the
> data portion of a message. (It relies on some functionality
> exported by the osd client to read and write page data.)
> 
> There are two spots it does send data though. The value assigned to
> an extended attribute is held in one or more pages allocated by
> ceph_sync_setxattr(). Eventually those pages are assigned to a
> request message in create_request_message().
> 
> The second spot is when sending a reconnect message, where a
> ceph pagelist is used to build up an array of snaprealm_reconnect
> structures to send to the mds.
> 
> Change it so we only assign the outgoing data information for
> these messages if there is outgoing data to send.
> 
> This is related to:
> http://tracker.ceph.com/issues/4284
> 
> Signed-off-by: Alex Elder <elder@inktank.com>
> ---
> fs/ceph/mds_client.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 42400ce..ae83aa9 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -1718,7 +1718,12 @@ static struct ceph_msg
> *create_request_message(struct ceph_mds_client *mdsc,
> msg->front.iov_len = p - msg->front.iov_base;
> msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);
> 
> - ceph_msg_data_set_pages(msg, req->r_pages, req->r_num_pages, 0);
> + if (req->r_num_pages) {
> + /* outbound data set only by ceph_sync_setxattr() */
> + BUG_ON(!req->r_pages);
> + ceph_msg_data_set_pages(msg, req->r_pages,
> + req->r_num_pages, 0);
> + }
> 
> msg->hdr.data_len = cpu_to_le32(req->r_data_len);
> msg->hdr.data_off = cpu_to_le16(0);
> @@ -2599,10 +2604,13 @@ static void send_mds_reconnect(struct
> ceph_mds_client *mdsc,
> goto fail;
> }
> 
> - ceph_msg_data_set_pagelist(reply, pagelist);
> if (recon_state.flock)
> reply->hdr.version = cpu_to_le16(2);
> - reply->hdr.data_len = cpu_to_le32(pagelist->length);
> + if (pagelist->length) {
> + /* set up outbound data if we have any */
> + reply->hdr.data_len = cpu_to_le32(pagelist->length);
> + ceph_msg_data_set_pagelist(reply, pagelist);
> + }
> ceph_con_send(&session->s_con, reply);
> 
> mutex_unlock(&session->s_mutex);
> 
Reviewed-by: Greg Farnum <greg@inktank.com>
Software Engineer #42 @ http://inktank.com | http://ceph.com

[PATCH 4/5] libceph: set response data fields earlier

[Alex Elder]

When an incoming message is destined for the osd client, the
messenger calls the osd client's alloc_msg method.  That function
looks up which request has the tid matching the incoming message,
and returns the request message that was preallocated to receive the
response.  The response message is therefore known before the
request is even started.

Between the start of the request and the receipt of the response,
the request and its data fields will not change, so there's no
reason we need to hold off setting them.  In fact it's preferable
to set them just once because it's more obvious that they're
unchanging.

So set up the fields describing where incoming data is to land in a
response message at the beginning of ceph_osdc_start_request().
Define a helper function that sets these fields, and use it to
set the fields for both outgoing data in the request message and
incoming data in the response.

This resolves:
    http://tracker.ceph.com/issues/4284

Signed-off-by: Alex Elder <elder@inktank.com>
---
 net/ceph/osd_client.c |   38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c
index c19188a..1bb2be9 100644
--- a/net/ceph/osd_client.c
+++ b/net/ceph/osd_client.c
@@ -1739,6 +1739,21 @@ bad:
 	return;
 }

+static void ceph_osdc_msg_data_set(struct ceph_msg *msg,
+				struct ceph_osd_data *osd_data)
+{
+	if (osd_data->type == CEPH_OSD_DATA_TYPE_PAGES) {
+		ceph_msg_data_set_pages(msg, osd_data->pages,
+			osd_data->num_pages, osd_data->alignment);
+#ifdef CONFIG_BLOCK
+	} else if (osd_data->type == CEPH_OSD_DATA_TYPE_BIO) {
+		ceph_msg_data_set_bio(msg, osd_data->bio);
+#endif
+	} else {
+		BUG_ON(osd_data->type != CEPH_OSD_DATA_TYPE_NONE);
+	}
+}
+
 /*
  * Register request, send initial attempt.
  */
@@ -1747,22 +1762,12 @@ int ceph_osdc_start_request(struct
ceph_osd_client *osdc,
 			    bool nofail)
 {
 	int rc = 0;
-	struct ceph_osd_data *osd_data;

-	/* Set up outgoing data */
+	/* Set up request outgoing data and response incoming data fields */

-	osd_data = &req->r_data_out;
-	if (osd_data->type == CEPH_OSD_DATA_TYPE_PAGES) {
-		ceph_msg_data_set_pages(req->r_request, osd_data->pages,
-			osd_data->num_pages, osd_data->alignment);
-#ifdef CONFIG_BLOCK
-	} else if (osd_data->type == CEPH_OSD_DATA_TYPE_BIO) {
-		ceph_msg_data_set_bio(req->r_request, osd_data->bio);
-#endif
-	} else {
-		BUG_ON(osd_data->type != CEPH_OSD_DATA_TYPE_NONE);
-	}
+	ceph_osdc_msg_data_set(req->r_request, &req->r_data_out);
 	ceph_msg_data_set_trail(req->r_request, &req->r_trail);
+	ceph_osdc_msg_data_set(req->r_reply, &req->r_data_in);

 	register_request(osdc, req);

@@ -2126,13 +2131,6 @@ static struct ceph_msg *get_reply(struct
ceph_connection *con,
 				m = NULL;
 				goto out;
 			}
-
-			ceph_msg_data_set_pages(m, osd_data->pages,
-				osd_data->num_pages, osd_data->alignment);
-#ifdef CONFIG_BLOCK
-		} else if (osd_data->type == CEPH_OSD_DATA_TYPE_BIO) {
-			ceph_msg_data_set_bio(m, osd_data->bio);
-#endif
 		}
 	}
 	*skip = 0;
-- 
1.7.9.5

[PATCH 5/5] libceph: activate message data assignment checks

[Alex Elder]

The mds client no longer tries to assign zero-length message data,
and the osd client no longer sets its data info more than once.
This allows us to activate assertions in the messenger to verify
these things never happen.

This resolves both of these:
    http://tracker.ceph.com/issues/4263
    http://tracker.ceph.com/issues/4284

Signed-off-by: Alex Elder <elder@inktank.com>
---
 net/ceph/messenger.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 97506ac..5bf1bb5 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -2677,10 +2677,10 @@ EXPORT_SYMBOL(ceph_con_keepalive);
 void ceph_msg_data_set_pages(struct ceph_msg *msg, struct page **pages,
 		unsigned int page_count, size_t alignment)
 {
-	/* BUG_ON(!pages); */
-	/* BUG_ON(!page_count); */
-	/* BUG_ON(msg->pages); */
-	/* BUG_ON(msg->page_count); */
+	BUG_ON(!pages);
+	BUG_ON(!page_count);
+	BUG_ON(msg->pages);
+	BUG_ON(msg->page_count);

 	msg->pages = pages;
 	msg->page_count = page_count;
@@ -2691,8 +2691,8 @@ EXPORT_SYMBOL(ceph_msg_data_set_pages);
 void ceph_msg_data_set_pagelist(struct ceph_msg *msg,
 				struct ceph_pagelist *pagelist)
 {
-	/* BUG_ON(!pagelist); */
-	/* BUG_ON(msg->pagelist); */
+	BUG_ON(!pagelist);
+	BUG_ON(msg->pagelist);

 	msg->pagelist = pagelist;
 }
@@ -2700,8 +2700,8 @@ EXPORT_SYMBOL(ceph_msg_data_set_pagelist);

 void ceph_msg_data_set_bio(struct ceph_msg *msg, struct bio *bio)
 {
-	/* BUG_ON(!bio); */
-	/* BUG_ON(msg->bio); */
+	BUG_ON(!bio);
+	BUG_ON(msg->bio);

 	msg->bio = bio;
 }
@@ -2709,8 +2709,8 @@ EXPORT_SYMBOL(ceph_msg_data_set_bio);

 void ceph_msg_data_set_trail(struct ceph_msg *msg, struct ceph_pagelist
*trail)
 {
-	/* BUG_ON(!trail); */
-	/* BUG_ON(msg->trail); */
+	BUG_ON(!trail);
+	BUG_ON(msg->trail);

 	msg->trail = trail;
 }
-- 
1.7.9.5

Re: [PATCH 5/5] libceph: activate message data assignment checks

[Greg Farnum]

On Tuesday, March 5, 2013 at 5:53 AM, Alex Elder wrote:
> The mds client no longer tries to assign zero-length message data,
> and the osd client no longer sets its data info more than once.
> This allows us to activate assertions in the messenger to verify
> these things never happen.
> 
> This resolves both of these:
> http://tracker.ceph.com/issues/4263
> http://tracker.ceph.com/issues/4284
> 
> Signed-off-by: Alex Elder <elder@inktank.com (mailto:elder@inktank.com)>
> ---
> net/ceph/messenger.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
> index 97506ac..5bf1bb5 100644
> --- a/net/ceph/messenger.c
> +++ b/net/ceph/messenger.c
> @@ -2677,10 +2677,10 @@ EXPORT_SYMBOL(ceph_con_keepalive);
> void ceph_msg_data_set_pages(struct ceph_msg *msg, struct page **pages,
> unsigned int page_count, size_t alignment)
> {
> - /* BUG_ON(!pages); */
> - /* BUG_ON(!page_count); */
> - /* BUG_ON(msg->pages); */
> - /* BUG_ON(msg->page_count); */
> + BUG_ON(!pages);
> + BUG_ON(!page_count);
> + BUG_ON(msg->pages);
> + BUG_ON(msg->page_count);
> 
> msg->pages = pages;
> msg->page_count = page_count;
> @@ -2691,8 +2691,8 @@ EXPORT_SYMBOL(ceph_msg_data_set_pages);
> void ceph_msg_data_set_pagelist(struct ceph_msg *msg,
> struct ceph_pagelist *pagelist)
> {
> - /* BUG_ON(!pagelist); */
> - /* BUG_ON(msg->pagelist); */
> + BUG_ON(!pagelist);
> + BUG_ON(msg->pagelist);
> 
> msg->pagelist = pagelist;
> }
> @@ -2700,8 +2700,8 @@ EXPORT_SYMBOL(ceph_msg_data_set_pagelist);
> 
> void ceph_msg_data_set_bio(struct ceph_msg *msg, struct bio *bio)
> {
> - /* BUG_ON(!bio); */
> - /* BUG_ON(msg->bio); */
> + BUG_ON(!bio);
> + BUG_ON(msg->bio);
> 
> msg->bio = bio;
> }
> @@ -2709,8 +2709,8 @@ EXPORT_SYMBOL(ceph_msg_data_set_bio);
> 
> void ceph_msg_data_set_trail(struct ceph_msg *msg, struct ceph_pagelist
> *trail)
> {
> - /* BUG_ON(!trail); */
> - /* BUG_ON(msg->trail); */
> + BUG_ON(!trail);
> + BUG_ON(msg->trail);
> 
> msg->trail = trail;
> }
> -- 
> 1.7.9.5
> 


Reviewed-by: Greg Farnum <greg@inktank.com>

I'll leave #4 for Josh to review. :)

Software Engineer #42 @ http://inktank.com | http://ceph.com