From patchwork Wed Oct 30 14:35:52 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "J. Bruce Fields" X-Patchwork-Id: 3115011 Return-Path: X-Original-To: patchwork-linux-nfs@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 240D69F2B7 for ; Wed, 30 Oct 2013 14:36:06 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id EEA44203ED for ; Wed, 30 Oct 2013 14:36:04 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 8D9BA203EB for ; Wed, 30 Oct 2013 14:36:03 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754630Ab3J3Ofz (ORCPT ); Wed, 30 Oct 2013 10:35:55 -0400 Received: from fieldses.org ([174.143.236.118]:44239 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754473Ab3J3Ofy (ORCPT ); Wed, 30 Oct 2013 10:35:54 -0400 Received: from bfields by fieldses.org with local (Exim 4.76) (envelope-from ) id 1VbWsC-0004bN-Rp; Wed, 30 Oct 2013 10:35:52 -0400 Date: Wed, 30 Oct 2013 10:35:52 -0400 From: "J. Bruce Fields" To: Benny Halevy Cc: bfields@redhat.com, linux-nfs@vger.kernel.org Subject: Re: [PATCH 1/7] nfsd4: fix recall_lock use in unhash_delegation Message-ID: <20131030143552.GE3227@fieldses.org> References: <526F81DE.6060704@primarydata.com> <1383039544-27157-1-git-send-email-bhalevy@primarydata.com> <20131029144658.GR31322@fieldses.org> <52711187.10202@primarydata.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <52711187.10202@primarydata.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org X-Spam-Status: No, score=-7.4 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Oct 30, 2013 at 04:02:47PM +0200, Benny Halevy wrote: > On 2013-10-29 16:46, J. Bruce Fields wrote: > > On Tue, Oct 29, 2013 at 11:39:04AM +0200, Benny Halevy wrote: > >> Access to dp->dl_perclnt must be synchronized by the recall_lock > > > > Are you sure? recall_lock is for stuff that's needed in the delegation > > break callback (nfsd_break_deleg_cb() and any of the subsequent callback > > handling). I don't think that includes dl_perclnt. > > I was mislead by the fact that destroy_client and nfs4_state_shutdown_net > care to acquire the recall_lock for traversing the clp->cl_delegations > and nn->del_recall_lru lists, respectively. > > Now nfs4_state_shutdown_net, although its comment says it should be > called with the state lock held (SHOULD or should, or should it be MUST? :) > It doesn't seem like nfsd_shutdown_net acquires the state lock. Good point. > Therefore, we either need to grab the state lock in nfs4_state_shutdown_net > which adds yet another problem to fix, That sounds simplest as the short-term fix.... What problem do you see? --b. commit 2c9407ae694dad8a1ad6394d9bf6077024b9fae7 Author: J. Bruce Fields Date: Wed Oct 30 10:33:09 2013 -0400 nfsd4: nfsd_shutdown_net needs state lock A comment claims the caller should take it, but that's not being done. Note we don't want it around the cancel_delayed_work_sync since that may wait on work which holds the client lock. Signed-off-by: J. Bruce Fields --- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 21eb678..e03e8ef 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -5124,7 +5124,6 @@ out_recovery: return ret; } -/* should be called with the state lock held */ void nfs4_state_shutdown_net(struct net *net) { @@ -5135,6 +5134,7 @@ nfs4_state_shutdown_net(struct net *net) cancel_delayed_work_sync(&nn->laundromat_work); locks_end_grace(&nn->nfsd4_manager); + nfs4_lock_state(); INIT_LIST_HEAD(&reaplist); spin_lock(&recall_lock); list_for_each_safe(pos, next, &nn->del_recall_lru) { @@ -5149,6 +5149,7 @@ nfs4_state_shutdown_net(struct net *net) nfsd4_client_tracking_exit(net); nfs4_state_destroy_net(net); + nfs4_unlock_state(); } void