| Message ID | 5134E3F1.70008@inktank.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 4, 2013 at 10:12 AM, Alex Elder <elder@inktank.com> wrote: > In ceph_con_in_msg_alloc(), if no alloc_msg method is defined for a > connection a new message is allocated with ceph_msg_new(). > > Drop the mutex before making this call, and make sure we're still > connected when we get it back again. Why do we need to drop the mutex at all? The mds_alloc_msg() that you're about to define doesn't seem to need it. -Greg -- 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
On 03/04/2013 01:07 PM, Gregory Farnum wrote: > On Mon, Mar 4, 2013 at 10:12 AM, Alex Elder <elder@inktank.com> wrote: >> In ceph_con_in_msg_alloc(), if no alloc_msg method is defined for a >> connection a new message is allocated with ceph_msg_new(). >> >> Drop the mutex before making this call, and make sure we're still >> connected when we get it back again. > > Why do we need to drop the mutex at all? The mds_alloc_msg() that > you're about to define doesn't seem to need it. > -Greg > My purpose in doing this is to make the third patch in this series trivial to review. That is, I make the code here match what will happen when I unify how all alloc_msg calls get made, as a pre-step, and then it's easier to see that the later patch is correct. I agree, it's not needed for that allocator. By the same token, it almost certainly causes no harm, and the end result will be consistent handling by all users of this interface. -Alex -- 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
On Mon, Mar 4, 2013 at 11:25 AM, Alex Elder <elder@inktank.com> wrote: > On 03/04/2013 01:07 PM, Gregory Farnum wrote: >> On Mon, Mar 4, 2013 at 10:12 AM, Alex Elder <elder@inktank.com> wrote: >>> In ceph_con_in_msg_alloc(), if no alloc_msg method is defined for a >>> connection a new message is allocated with ceph_msg_new(). >>> >>> Drop the mutex before making this call, and make sure we're still >>> connected when we get it back again. >> >> Why do we need to drop the mutex at all? The mds_alloc_msg() that >> you're about to define doesn't seem to need it. >> -Greg >> > > My purpose in doing this is to make the third patch in > this series trivial to review. The one that doesn't set the page_alignment for mds messages? *confused* > That is, I make the code > here match what will happen when I unify how all alloc_msg > calls get made, as a pre-step, and then it's easier to see > that the later patch is correct. Oh, and we do do the mutex_lock and unlock pair around alloc_msg. Right, looks good! :) Reviewed-by: Greg Farnum <greg@inktank.com> > > I agree, it's not needed for that allocator. By the same > token, it almost certainly causes no harm, and the end result > will be consistent handling by all users of this interface. > > -Alex -- 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
On 03/04/2013 01:39 PM, Gregory Farnum wrote: >> My purpose in doing this is to make the third patch in >> > this series trivial to review. > The one that doesn't set the page_alignment for mds messages? *confused* > Sorry, second patch in the series. -Alex -- 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
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c index 0f9933a..6ec6051 100644 --- a/net/ceph/messenger.c +++ b/net/ceph/messenger.c @@ -2807,13 +2807,12 @@ static int ceph_con_in_msg_alloc(struct ceph_connection *con, int *skip) 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); if (con->ops->alloc_msg) { - struct ceph_msg *msg; - mutex_unlock(&con->mutex); msg = con->ops->alloc_msg(con, hdr, skip);
In ceph_con_in_msg_alloc(), if no alloc_msg method is defined for a connection a new message is allocated with ceph_msg_new(). Drop the mutex before making this call, and make sure we're still connected when we get it back again. This is preparing for the next patch, which ensures all connections define an alloc_msg method, and then handles them all the same way. Signed-off-by: Alex Elder <elder@inktank.com> --- net/ceph/messenger.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) mutex_lock(&con->mutex); @@ -2838,12 +2837,19 @@ static int ceph_con_in_msg_alloc(struct ceph_connection *con, int *skip) } } if (!con->in_msg) { - con->in_msg = ceph_msg_new(type, front_len, GFP_NOFS, false); - 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) { + ceph_msg_put(msg); + return -EAGAIN; + } + con->in_msg = 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);