On 12/8/23 19:58, Ilya Dryomov wrote:
On Fri, Dec 8, 2023 at 5:34 AM <xiubli@xxxxxxxxxx> wrote:
From: Xiubo Li <xiubli@xxxxxxxxxx>
The messages from ceph maybe split into multiple socket packages
and we just need to wait for all the data to be availiable on the
sokcet.
URL: https://tracker.ceph.com/issues/63586
Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
---
net/ceph/messenger_v1.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/net/ceph/messenger_v1.c b/net/ceph/messenger_v1.c
index f9a50d7f0d20..aff81fef932f 100644
--- a/net/ceph/messenger_v1.c
+++ b/net/ceph/messenger_v1.c
@@ -1160,15 +1160,17 @@ static int read_partial_message(struct ceph_connection *con)
/* header */
size = sizeof(con->v1.in_hdr);
end = size;
- ret = read_partial(con, end, size, &con->v1.in_hdr);
- if (ret <= 0)
- return ret;
+ if (con->v1.in_base_pos < end) {
+ ret = read_partial(con, end, size, &con->v1.in_hdr);
+ if (ret <= 0)
+ return ret;
- crc = crc32c(0, &con->v1.in_hdr, offsetof(struct ceph_msg_header, crc));
- if (cpu_to_le32(crc) != con->v1.in_hdr.crc) {
- pr_err("read_partial_message bad hdr crc %u != expected %u\n",
- crc, con->v1.in_hdr.crc);
- return -EBADMSG;
+ crc = crc32c(0, &con->v1.in_hdr, offsetof(struct ceph_msg_header, crc));
+ if (cpu_to_le32(crc) != con->v1.in_hdr.crc) {
+ pr_err("read_partial_message bad hdr crc %u != expected %u\n",
+ crc, con->v1.in_hdr.crc);
+ return -EBADMSG;
+ }
}
front_len = le32_to_cpu(con->v1.in_hdr.front_len);
--
2.43.0
Hi Xiubo,
This doesn't seem right to me. read_partial() is supposed to be called
unconditionally. On a short read (i.e. when it's unable to fill the
destination buffer -- in this case the header), it returns 0 and the
stack is supposed to unroll all the way up to ceph_con_workfn().
If the destination buffer is already filled, read_partial() does
nothing and returns 1. Recomputing the header crc in case
read_partial_message() is called repeatedly shouldn't be an issue
because con->v1.in_hdr shouldn't be modified in the interim. If it
gets modified, it's a bug.
It might help if you provide a step-by-step breakdown of the scenario
that you are trying to address in the commit message.
Yeah, you are correct.
So my first change was correct, I just adjust the patch before sending
it out.
Let me change it back.
Thanks
- Xiubo
Thanks,
Ilya
