From patchwork Tue Mar 5 15:33:28 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alex Elder X-Patchwork-Id: 2220231 Return-Path: X-Original-To: patchwork-ceph-devel@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 38494DF24C for ; Tue, 5 Mar 2013 15:33:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755179Ab3CEPdd (ORCPT ); Tue, 5 Mar 2013 10:33:33 -0500 Received: from mail-ia0-f172.google.com ([209.85.210.172]:56062 "EHLO mail-ia0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754960Ab3CEPdc (ORCPT ); Tue, 5 Mar 2013 10:33:32 -0500 Received: by mail-ia0-f172.google.com with SMTP id l29so6189851iag.17 for ; Tue, 05 Mar 2013 07:33:32 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:message-id:date:from:user-agent:mime-version:to:subject :content-type:content-transfer-encoding:x-gm-message-state; bh=6AFOZQkzKbfNtO4QQ8QUb/fyFhN5OMZk/3tE1G0nGug=; b=IzoOJ3PjY9sVpAB+gje1QjOlXP8CE96HycQ3JxO2WMDco6yFHPo+WrrhKRAbd2aG61 2/XQ3ORcoe9sQG6v+KhpKQi3WZfJ/yLHGkYA9UQLXAkog89xPlnd5fNqOB8PLpDR+ioJ i/Inx6EBA4XjH2EVwXwj4gCGskpr7NHYOmil7YqlGfkql5Xs9iLwlpPQk0iDf49/5qlb DH8/ghNI8V8VncXGxQEbSCOFlEGTQ23kcl3/s7rYrpJuAoN868DIOEe5AwZpkxrjYqkZ LXWl6b7yeFdX2+q6YS3FUgOLW1TJGScSjEWanQdWXzEnJOyB8aiUckHVdI4PngWrjERI 2JTg== X-Received: by 10.50.194.200 with SMTP id hy8mr6478656igc.3.1362497612173; Tue, 05 Mar 2013 07:33:32 -0800 (PST) Received: from [172.22.22.4] (c-71-195-31-37.hsd1.mn.comcast.net. [71.195.31.37]) by mx.google.com with ESMTPS id px9sm16493877igc.0.2013.03.05.07.33.29 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 05 Mar 2013 07:33:30 -0800 (PST) Message-ID: <51361048.50500@inktank.com> Date: Tue, 05 Mar 2013 09:33:28 -0600 From: Alex Elder User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130221 Thunderbird/17.0.3 MIME-Version: 1.0 To: ceph-devel@vger.kernel.org Subject: [PATCH] libceph: clean up skipped message logic X-Gm-Message-State: ALoCoQlXjBP/KhEkUKpUSMvaQVT0KqdRzvlF7NiN+5IzNDBSHQUK30/r/dudFHxIFBakpXVsYB1s Sender: ceph-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: ceph-devel@vger.kernel.org (This patch is available as the top commit in branch "review/wip-4324" in the ceph-client git repository.) In ceph_con_in_msg_alloc() it is possible for a connection's alloc_msg method to indicate an incoming message should be skipped. By default, read_partial_message() initializes the skip variable to 0 before it gets provided to ceph_con_in_msg_alloc(). The osd client, mon client, and mds client each supply an alloc_msg method. The mds client always assigns skip to be 0. The other two leave the skip value of as-is, or assigns it to zero, except: - if no (osd or mon) request having the given tid is found, in which case skip is set to 1 and NULL is returned; or - in the osd client, if the data of the reply message is not adequate to hold the message to be read, it assigns skip value 1 and returns NULL. So the returned message pointer will always be NULL if skip is ever non-zero. Clean up the logic a bit in ceph_con_in_msg_alloc() to make this state of affairs more obvious. Add a comment explaining how a null message pointer can mean either a message that should be skipped or a problem allocating a message. This resolves: http://tracker.ceph.com/issues/4324 Reported-by: Greg Farnum Signed-off-by: Alex Elder Reviewed-by: Greg Farnum --- net/ceph/messenger.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) memcpy(&con->in_msg->hdr, &con->in_hdr, sizeof(con->in_hdr)); diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c index 5bf1bb5..644cb6c 100644 --- a/net/ceph/messenger.c +++ b/net/ceph/messenger.c @@ -2860,18 +2860,21 @@ static int ceph_con_in_msg_alloc(struct ceph_connection *con, int *skip) ceph_msg_put(msg); return -EAGAIN; } - con->in_msg = msg; - if (con->in_msg) { + if (msg) { + BUG_ON(*skip); + con->in_msg = 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"; + } else { + /* + * Null message pointer means either we should skip + * this message or we couldn't allocate memory. The + * former is not an error. + */ + if (*skip) + return 0; + con->error_msg = "error allocating memory for incoming message"; + return -ENOMEM; }