| Message ID | CAOi1vP_rt7TJbmpqVCokdS_r9UrcvvU5rjHpv+-iN=7nMRB_Gg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> On 26 Jul 2017, at 16:04, Ilya Dryomov <idryomov@gmail.com> wrote: > > On Tue, Jul 25, 2017 at 3:33 PM, Ilya Dryomov <idryomov@gmail.com> wrote: >> On Tue, Jul 25, 2017 at 3:14 PM, Yan, Zheng <zyan@redhat.com> wrote: >>> >>>> On 25 Jul 2017, at 21:01, Ilya Dryomov <idryomov@gmail.com> wrote: >>>> >>>> On Mon, Jul 24, 2017 at 3:28 PM, Yan, Zheng <zyan@redhat.com> wrote: >>>>> The new BUG_ON in encode_request_partial() verifies that space used >>>>> by encoding request front is exactly equal to request message size. >>>>> This is wrong because request messages allocated from mempool always >>>>> have size PAGE_SIZE. >>>>> >>>>> Signed-off-by: "Yan, Zheng" <zyan@redhat.com> >>>>> --- >>>>> net/ceph/osd_client.c | 17 +++++++++-------- >>>>> 1 file changed, 9 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c >>>>> index 5c9d696..81f6199 100644 >>>>> --- a/net/ceph/osd_client.c >>>>> +++ b/net/ceph/osd_client.c >>>>> @@ -1913,10 +1913,11 @@ static void encode_request_partial(struct ceph_osd_request *req, >>>>> } >>>>> >>>>> ceph_encode_32(&p, req->r_attempts); /* retry_attempt */ >>>>> - BUG_ON(p != end - 8); /* space for features */ >>>>> + BUG_ON(p + 8 > end); /* space for features */ >>>>> >>>>> msg->hdr.version = cpu_to_le16(8); /* MOSDOp v8 */ >>>>> - /* front_len is finalized in encode_request_finish() */ >>>>> + msg->front.iov_len = p + 8 - msg->front.iov_base; >>>>> + msg->hdr.front_len = cpu_to_le32(msg->front.iov_len); >>>>> msg->hdr.data_len = cpu_to_le32(data_len); >>>>> /* >>>>> * The header "data_off" is a hint to the receiver allowing it >>>>> @@ -1932,7 +1933,7 @@ static void encode_request_partial(struct ceph_osd_request *req, >>>>> static void encode_request_finish(struct ceph_msg *msg) >>>>> { >>>>> void *p = msg->front.iov_base; >>>>> - void *const end = p + msg->front_alloc_len; >>>>> + void *const end = p + msg->front.iov_len; >>>>> >>>>> if (CEPH_HAVE_FEATURE(msg->con->peer_features, RESEND_ON_SPLIT)) { >>>>> /* luminous OSD -- encode features and be done */ >>>>> @@ -2008,11 +2009,11 @@ static void encode_request_finish(struct ceph_msg *msg) >>>>> p += tail_len; >>>>> >>>>> msg->hdr.version = cpu_to_le16(4); /* MOSDOp v4 */ >>>>> - } >>>>> >>>>> - BUG_ON(p > end); >>>>> - msg->front.iov_len = p - msg->front.iov_base; >>>>> - msg->hdr.front_len = cpu_to_le32(msg->front.iov_len); >>>>> + BUG_ON(p > end); >>>>> + msg->front.iov_len = p - msg->front.iov_base; >>>>> + msg->hdr.front_len = cpu_to_le32(msg->front.iov_len); >>>>> + } >>>>> >>>>> dout("%s msg %p tid %llu %u+%u+%u v%d\n", __func__, msg, >>>>> le64_to_cpu(msg->hdr.tid), le32_to_cpu(msg->hdr.front_len), >>>>> @@ -3981,7 +3982,7 @@ static struct ceph_msg *create_backoff_message( >>>>> return NULL; >>>>> >>>>> p = msg->front.iov_base; >>>>> - end = p + msg->front_alloc_len; >>>>> + end = p + msg->front.iov_len; >>>>> >>>>> encode_spgid(&p, &backoff->spgid); >>>>> ceph_encode_32(&p, map_epoch); >>>> >>>> Hi Zheng, >>>> >>>> How about the attached patch instead? It's shorter and more >>>> importantly preserves the existing structure. >>> >>> does encode_request_finish() get called each time we re-send a message? >>> If it does, your patch seems incorrect. encode_request_finish() appends extra >>> 8 bytes to the message each time it get called. >> >> Hrm, it is. But then your patch doesn't fix it either because the >> problem is the ->reencode_message() call itself -- I didn't intend it >> to be called on every resend. Let me run some tests... > > OK, so ->reencode_message() being called more than once could affect > the MDS client, but since only the OSD client is using it, there are no > immediate issues. I'll post the attached patch for the sake of > robustness though. > With this new patch, your previous patch looks good. Regards Yan, Zheng > Thanks, > > Ilya > <0001-libceph-don-t-call-reencode_message-more-than-once-p.patch> -- 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
From 8524b57a5881b548ecb17b150aaf599e647b8982 Mon Sep 17 00:00:00 2001
From: Ilya Dryomov <idryomov@gmail.com>
Date: Wed, 26 Jul 2017 09:59:15 +0200
Subject: [PATCH] libceph: don't call ->reencode_message() more than once per
message
Reencoding an already reencoded message is a bad idea. This could
happen on Policy::stateful_server connections (!CEPH_MSG_CONNECT_LOSSY),
such as MDS sessions.
This didn't pop up in testing because currently only OSD requests are
reencoded and OSD sessions are always lossy.
Fixes: 98ad5ebd1505 ("libceph: ceph_connection_operations::reencode_message() method")
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
net/ceph/messenger.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index b7cc615d42ef..a67298c7e0cd 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -1287,10 +1287,10 @@ static void prepare_write_message(struct ceph_connection *con)
if (m->needs_out_seq) {
m->hdr.seq = cpu_to_le64(++con->out_seq);
m->needs_out_seq = false;
- }
- if (con->ops->reencode_message)
- con->ops->reencode_message(m);
+ if (con->ops->reencode_message)
+ con->ops->reencode_message(m);
+ }
dout("prepare_write_message %p seq %lld type %d len %d+%d+%zd\n",
m, con->out_seq, le16_to_cpu(m->hdr.type),
--
2.4.3