Re: [PATCH 2/3] libceph: define mds_alloc_msg() method
This looks like a faithful reshuffling to me.
But...
On Mon, Mar 4, 2013 at 10:12 AM, Alex Elder el...@inktank.com wrote:
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 6ec6051..c7d4278 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -2804,55 +2804,34 @@ static int ceph_alloc_middle(struct
ceph_connection *con, struct ceph_msg *msg)
static int ceph_con_in_msg_alloc(struct ceph_connection *con, int *skip)
{
struct ceph_msg_header *hdr = con-in_hdr;
- int type = le16_to_cpu(hdr-type);
- int front_len = le32_to_cpu(hdr-front_len);
int middle_len = le32_to_cpu(hdr-middle_len);
struct ceph_msg *msg;
int ret = 0;
BUG_ON(con-in_msg != NULL);
+ BUG_ON(!con-ops-alloc_msg);
- if (con-ops-alloc_msg) {
- mutex_unlock(con-mutex);
- msg = con-ops-alloc_msg(con, hdr, skip);
- mutex_lock(con-mutex);
- if (con-state != CON_STATE_OPEN) {
- if (msg)
- ceph_msg_put(msg);
- return -EAGAIN;
- }
- con-in_msg = msg;
- if (con-in_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;
- return -ENOMEM;
- }
- }
- if (!con-in_msg) {
- mutex_unlock(con-mutex);
- msg = ceph_msg_new(type, front_len, GFP_NOFS, false);
- mutex_lock(con-mutex);
- if (!msg) {
- pr_err(unable to allocate msg type %d len %d\n,
- type, front_len);
- return -ENOMEM;
- }
- if (con-state != CON_STATE_OPEN) {
+ mutex_unlock(con-mutex);
+ msg = con-ops-alloc_msg(con, hdr, skip);
+ mutex_lock(con-mutex);
+ if (con-state != CON_STATE_OPEN) {
+ if (msg)
ceph_msg_put(msg);
- return -EAGAIN;
- }
- con-in_msg = msg;
+ return -EAGAIN;
+ }
+ con-in_msg = msg;
+ if (con-in_msg) {
con-in_msg-con = con-ops-get(con);
BUG_ON(con-in_msg-con == NULL);
- con-in_msg-page_alignment = le16_to_cpu(hdr-data_off);
+ }
+ if (*skip) {
+ con-in_msg = NULL;
+ return 0;
This looks like a memory leak to me? I'm not familiar with the skip
infrastructure; is there something that cleans up those messages in
the background, or should this actually be a BUG_ON?
-Greg
--
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 2/3] libceph: define mds_alloc_msg() method
On 03/04/2013 01:05 PM, Gregory Farnum wrote:
This looks like a faithful reshuffling to me.
But...
On Mon, Mar 4, 2013 at 10:12 AM, Alex Elder el...@inktank.com wrote:
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index 6ec6051..c7d4278 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -2804,55 +2804,34 @@ static int ceph_alloc_middle(struct
ceph_connection *con, struct ceph_msg *msg)
static int ceph_con_in_msg_alloc(struct ceph_connection *con, int *skip)
{
struct ceph_msg_header *hdr = con-in_hdr;
- int type = le16_to_cpu(hdr-type);
- int front_len = le32_to_cpu(hdr-front_len);
int middle_len = le32_to_cpu(hdr-middle_len);
struct ceph_msg *msg;
int ret = 0;
BUG_ON(con-in_msg != NULL);
+ BUG_ON(!con-ops-alloc_msg);
- if (con-ops-alloc_msg) {
- mutex_unlock(con-mutex);
- msg = con-ops-alloc_msg(con, hdr, skip);
- mutex_lock(con-mutex);
- if (con-state != CON_STATE_OPEN) {
- if (msg)
- ceph_msg_put(msg);
- return -EAGAIN;
- }
- con-in_msg = msg;
- if (con-in_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;
- return -ENOMEM;
- }
- }
- if (!con-in_msg) {
- mutex_unlock(con-mutex);
- msg = ceph_msg_new(type, front_len, GFP_NOFS, false);
- mutex_lock(con-mutex);
- if (!msg) {
- pr_err(unable to allocate msg type %d len %d\n,
- type, front_len);
- return -ENOMEM;
- }
- if (con-state != CON_STATE_OPEN) {
+ mutex_unlock(con-mutex);
+ msg = con-ops-alloc_msg(con, hdr, skip);
+ mutex_lock(con-mutex);
+ if (con-state != CON_STATE_OPEN) {
+ if (msg)
ceph_msg_put(msg);
- return -EAGAIN;
- }
- con-in_msg = msg;
+ return -EAGAIN;
+ }
+ con-in_msg = msg;
+ if (con-in_msg) {
con-in_msg-con = con-ops-get(con);
BUG_ON(con-in_msg-con == NULL);
- con-in_msg-page_alignment = le16_to_cpu(hdr-data_off);
+ }
+ if (*skip) {
+ con-in_msg = NULL;
+ return 0;
This looks like a memory leak to me? I'm not familiar with the skip
infrastructure; is there something that cleans up those messages in
the background, or should this actually be a BUG_ON?
It looks that way to me too, but the con-ops-get(con) call before
it makes it a little tricky. I will investigate and get back
to you, and if it is a leak I'll insert a fix for it ahead of this
in the series.
I really appreciate your reviewing these.
-Alex
--
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
