From patchwork Wed Jul 26 08:04:09 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Dryomov X-Patchwork-Id: 9864353 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id C81CC6038F for ; Wed, 26 Jul 2017 08:04:16 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B5BC5286E6 for ; Wed, 26 Jul 2017 08:04:16 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id A8D402872B; Wed, 26 Jul 2017 08:04:16 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.3 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM, T_DKIM_INVALID, T_TVD_MIME_EPI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 26979286E6 for ; Wed, 26 Jul 2017 08:04:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751479AbdGZIEM (ORCPT ); Wed, 26 Jul 2017 04:04:12 -0400 Received: from mail-vk0-f66.google.com ([209.85.213.66]:33710 "EHLO mail-vk0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751013AbdGZIEL (ORCPT ); Wed, 26 Jul 2017 04:04:11 -0400 Received: by mail-vk0-f66.google.com with SMTP id o19so2088452vkd.0 for ; Wed, 26 Jul 2017 01:04:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=9adaUTTt9uvavrVPv/bCuVCAGnblaTbtXVTqWo2QCxg=; b=d+U3SjgU853BKqZCo5zgocft0RPnIaUeMm5V0ywtDLhWrNB2dvwB5qqPAA392btDjC JXYbdHjx+rYlYtpN3bVT3eNdQHyEqyDbXhBT0UHv3eYyd3fUZNSYPkMPN26Fx1PCkx2j Ti9hrpELPYTN6c8aNfcmb3d8on5SwwS5Bwhg0oDYarxVEZ/mcvfjqwIycqE56w+t75F0 /cXbBY0Vn1Y1rj5YAdgMnwr8qfqSGm9axy7GoFHEVr0cNu2NEXVw8P1zs9Tq80tpBR35 OJsuLfNBye3Kfpmc0auqdqgdsqgrCpuy4alFaZSZ2KPXtJ8ebfCQEV3JMdx7HrqOxJvy rLzw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=9adaUTTt9uvavrVPv/bCuVCAGnblaTbtXVTqWo2QCxg=; b=iqzAE3FH/a15iM7RLTNUlQayfWokIDb0PDdU0yH6PCGXltOYqmnQKMNXVgcZv4TnFq wqUsVAKW6FLjB01uaWCx22zpFfvyx7md3nE60T3GsrK2InBY67hAb87zktAGbaUie+pz Q6cvLR6Hd5SNjbtEGNy1sXPoSAF627WraL0AAePjzLu8VqruztTeqOQAMz+I/dDcIzq8 e+KNjEmyDLspODWbR4waHmC1SIM0SHLHXynyQQL81ZSPccQgADuliGO6L+7k3niJn7JU 36m3YlrVJMrBVl91wRFGHN0Jixt3EO2duEFHNl/koFeq9J0iqLoVjdH6ArgIW+kFY4wk qJaQ== X-Gm-Message-State: AIVw111JiR3FEhDhBk58Fe0rrfLakEdS6zVv7DYFuArC3ajlW5HcZvg3 aiSmQyWE8Qrctd8E2dMhUEtRW1i30g== X-Received: by 10.31.85.197 with SMTP id j188mr61081vkb.135.1501056250264; Wed, 26 Jul 2017 01:04:10 -0700 (PDT) MIME-Version: 1.0 Received: by 10.176.1.175 with HTTP; Wed, 26 Jul 2017 01:04:09 -0700 (PDT) In-Reply-To: References: <20170724132802.7916-1-zyan@redhat.com> From: Ilya Dryomov Date: Wed, 26 Jul 2017 10:04:09 +0200 Message-ID: Subject: Re: [PATCH] libceph: fix osd request encoding regression To: "Yan, Zheng" Cc: Ceph Development Sender: ceph-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: ceph-devel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, Jul 25, 2017 at 3:33 PM, Ilya Dryomov wrote: > On Tue, Jul 25, 2017 at 3:14 PM, Yan, Zheng wrote: >> >>> On 25 Jul 2017, at 21:01, Ilya Dryomov wrote: >>> >>> On Mon, Jul 24, 2017 at 3:28 PM, Yan, Zheng 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" >>>> --- >>>> 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. Thanks, Ilya From 8524b57a5881b548ecb17b150aaf599e647b8982 Mon Sep 17 00:00:00 2001 From: Ilya Dryomov 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 --- 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