[PATCH] libceph: clean up skipped message logic

[Alex Elder]

(This patch is available as the top commit in branch
review/wip-4324 in the ceph-client git repository.)

In ceph_con_in_msg_alloc() it is possible for a connection's
alloc_msg method to indicate an incoming message should be skipped.
By default, read_partial_message() initializes the skip variable
to 0 before it gets provided to ceph_con_in_msg_alloc().

The osd client, mon client, and mds client each supply an alloc_msg
method.  The mds client always assigns skip to be 0.

The other two leave the skip value of as-is, or assigns it to zero,
except:
- if no (osd or mon) request having the given tid is found, in
  which case skip is set to 1 and NULL is returned; or
- in the osd client, if the data of the reply message is not
  adequate to hold the message to be read, it assigns skip
  value 1 and returns NULL.
So the returned message pointer will always be NULL if skip is ever
non-zero.

Clean up the logic a bit in ceph_con_in_msg_alloc() to make this
state of affairs more obvious.  Add a comment explaining how a null
message pointer can mean either a message that should be skipped or
a problem allocating a message.

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

Reported-by: Greg Farnum g...@inktank.com
Signed-off-by: Alex Elder el...@inktank.com
---
 net/ceph/messenger.c |   23 +--
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 5bf1bb5..644cb6c 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -2860,18 +2860,21 @@ static int ceph_con_in_msg_alloc(struct
ceph_connection *con, int *skip)
ceph_msg_put(msg);
return -EAGAIN;
}
-   con-in_msg = msg;
-   if (con-in_msg) {
+   if (msg) {
+   BUG_ON(*skip);
+   con-in_msg = msg;
con-in_msg-con = con-ops-get(con);
BUG_ON(con-in_msg-con == NULL);
-   }
-   if (*skip) {
-   con-in_msg = NULL;
-   return 0;
-   }
-   if (!con-in_msg) {
-   con-error_msg =
-   error allocating memory for incoming message;
+   } else {
+   /*
+* Null message pointer means either we should skip
+* this message or we couldn't allocate memory.  The
+* former is not an error.
+*/
+   if (*skip)
+   return 0;
+   con-error_msg = error allocating memory for incoming message;
+
return -ENOMEM;
}
memcpy(con-in_msg-hdr, con-in_hdr, sizeof(con-in_hdr));
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libceph: clean up skipped message logic

[Greg Farnum]

On Tuesday, March 5, 2013 at 7:33 AM, Alex Elder wrote:
 (This patch is available as the top commit in branch
 review/wip-4324 in the ceph-client git repository.)
 
 In ceph_con_in_msg_alloc() it is possible for a connection's
 alloc_msg method to indicate an incoming message should be skipped.
 By default, read_partial_message() initializes the skip variable
 to 0 before it gets provided to ceph_con_in_msg_alloc().
 
 The osd client, mon client, and mds client each supply an alloc_msg
 method. The mds client always assigns skip to be 0.
 
 The other two leave the skip value of as-is, or assigns it to zero,
 except:
 - if no (osd or mon) request having the given tid is found, in
 which case skip is set to 1 and NULL is returned; or
 - in the osd client, if the data of the reply message is not
 adequate to hold the message to be read, it assigns skip
 value 1 and returns NULL.
 So the returned message pointer will always be NULL if skip is ever
 non-zero.
 
 Clean up the logic a bit in ceph_con_in_msg_alloc() to make this
 state of affairs more obvious. Add a comment explaining how a null
 message pointer can mean either a message that should be skipped or
 a problem allocating a message.
 
 This resolves:
 http://tracker.ceph.com/issues/4324
 
 Reported-by: Greg Farnum g...@inktank.com (mailto:g...@inktank.com)
 Signed-off-by: Alex Elder el...@inktank.com (mailto:el...@inktank.com)
 ---
 net/ceph/messenger.c | 23 +--
 1 file changed, 13 insertions(+), 10 deletions(-)
 
 diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
 index 5bf1bb5..644cb6c 100644
 --- a/net/ceph/messenger.c
 +++ b/net/ceph/messenger.c
 @@ -2860,18 +2860,21 @@ static int ceph_con_in_msg_alloc(struct
 ceph_connection *con, int *skip)
 ceph_msg_put(msg);
 return -EAGAIN;
 }
 - con-in_msg = msg;
 - if (con-in_msg) {
 + if (msg) {
 + BUG_ON(*skip);
 + con-in_msg = msg;
 con-in_msg-con = con-ops-get(con);
 BUG_ON(con-in_msg-con == NULL);
 - }
 - if (*skip) {
 - con-in_msg = NULL;
 - return 0;
 - }
 - if (!con-in_msg) {
 - con-error_msg =
 - error allocating memory for incoming message;
 + } else {
 + /*
 + * Null message pointer means either we should skip
 + * this message or we couldn't allocate memory. The
 + * former is not an error.
 + */
 + if (*skip)
 + return 0;
 + con-error_msg = error allocating memory for incoming message;
 +
 return -ENOMEM;
 }
 memcpy(con-in_msg-hdr, con-in_hdr, sizeof(con-in_hdr));
 -- 
 1.7.9.5

Reviewed-by: Greg Farnum g...@inktank.com 

--
To unsubscribe from this list: send the line unsubscribe ceph-devel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html