From patchwork Wed Jul 6 23:45:32 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Trond Myklebust X-Patchwork-Id: 951642 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by demeter2.kernel.org (8.14.4/8.14.4) with ESMTP id p66NjZ2P007513 for ; Wed, 6 Jul 2011 23:45:36 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753618Ab1GFXpe (ORCPT ); Wed, 6 Jul 2011 19:45:34 -0400 Received: from mx2.netapp.com ([216.240.18.37]:44102 "EHLO mx2.netapp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753792Ab1GFXpd convert rfc822-to-8bit (ORCPT ); Wed, 6 Jul 2011 19:45:33 -0400 X-IronPort-AV: E=Sophos;i="4.65,489,1304319600"; d="scan'208";a="560862970" Received: from smtp1.corp.netapp.com ([10.57.156.124]) by mx2-out.netapp.com with ESMTP; 06 Jul 2011 16:45:33 -0700 Received: from sacrsexc2-prd.hq.netapp.com (sacrsexc2-prd.hq.netapp.com [10.99.115.28]) by smtp1.corp.netapp.com (8.13.1/8.13.1/NTAP-1.6) with ESMTP id p66NjXET008486; Wed, 6 Jul 2011 16:45:33 -0700 (PDT) Received: from SACMVEXC2-PRD.hq.netapp.com ([10.99.115.18]) by sacrsexc2-prd.hq.netapp.com with Microsoft SMTPSVC(6.0.3790.3959); Wed, 6 Jul 2011 16:45:33 -0700 Received: from 10.55.68.7 ([10.55.68.7]) by SACMVEXC2-PRD.hq.netapp.com ([10.99.115.16]) with Microsoft Exchange Server HTTP-DAV ; Wed, 6 Jul 2011 23:45:32 +0000 Received: from lade.trondhjem.org by SACMVEXC2-PRD.hq.netapp.com; 06 Jul 2011 19:45:32 -0400 Subject: Re: [RFC] sunrpc: Fix race between work-queue and rpc_killall_tasks. From: Trond Myklebust To: greearb@candelatech.com Cc: linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org Date: Wed, 06 Jul 2011 19:45:32 -0400 In-Reply-To: <1309992581-25199-1-git-send-email-greearb@candelatech.com> References: <1309992581-25199-1-git-send-email-greearb@candelatech.com> Organization: NetApp Inc X-Mailer: Evolution 3.0.2 (3.0.2-3.fc15) Message-ID: <1309995932.5447.6.camel@lade.trondhjem.org> Mime-Version: 1.0 X-OriginalArrivalTime: 06 Jul 2011 23:45:33.0402 (UTC) FILETIME=[CBEBA3A0:01CC3C36] Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter2.kernel.org [140.211.167.43]); Wed, 06 Jul 2011 23:45:36 +0000 (UTC) On Wed, 2011-07-06 at 15:49 -0700, greearb@candelatech.com wrote: > From: Ben Greear > > The rpc_killall_tasks logic is not locked against > the work-queue thread, but it still directly modifies > function pointers and data in the task objects. > > This patch changes the killall-tasks logic to set a flag > that tells the work-queue thread to terminate the task > instead of directly calling the terminate logic. > > Signed-off-by: Ben Greear > --- > > NOTE: This needs review, as I am still struggling to understand > the rpc code, and it's quite possible this patch either doesn't > fully fix the problem or actually causes other issues. That said, > my nfs stress test seems to run a bit more stable with this patch applied. Yes, but I don't see why you are adding a new flag, nor do I see why we want to keep checking for that flag in the rpc_execute() loop. rpc_killall_tasks() is not a frequent operation that we want to optimise for. How about the following instead? 8<---------------------------------------------------------------------------------- From ecb7244b661c3f9d2008ef6048733e5cea2f98ab Mon Sep 17 00:00:00 2001 From: Trond Myklebust Date: Wed, 6 Jul 2011 19:44:52 -0400 Subject: [PATCH] SUNRPC: Fix a race between work-queue and rpc_killall_tasks Since rpc_killall_tasks may modify the rpc_task's tk_action field without any locking, we need to be careful when dereferencing it. Reported-by: Ben Greear Signed-off-by: Trond Myklebust --- net/sunrpc/sched.c | 27 +++++++++++---------------- 1 files changed, 11 insertions(+), 16 deletions(-) diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c index a27406b..4814e24 100644 --- a/net/sunrpc/sched.c +++ b/net/sunrpc/sched.c @@ -616,30 +616,25 @@ static void __rpc_execute(struct rpc_task *task) BUG_ON(RPC_IS_QUEUED(task)); for (;;) { + void (*do_action)(struct rpc_task *); /* - * Execute any pending callback. + * Execute any pending callback first. */ - if (task->tk_callback) { - void (*save_callback)(struct rpc_task *); - - /* - * We set tk_callback to NULL before calling it, - * in case it sets the tk_callback field itself: - */ - save_callback = task->tk_callback; - task->tk_callback = NULL; - save_callback(task); - } else { + do_action = task->tk_callback; + task->tk_callback = NULL; + if (do_action == NULL) { /* * Perform the next FSM step. - * tk_action may be NULL when the task has been killed - * by someone else. + * tk_action may be NULL if the task has been killed. + * In particular, note that rpc_killall_tasks may + * do this at any time, so beware when dereferencing. */ - if (task->tk_action == NULL) + do_action = task->tk_action; + if (do_action == NULL) break; - task->tk_action(task); } + do_action(task); /* * Lockless check for whether task is sleeping or not.