From patchwork Wed Sep 4 14:31:07 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Trond Myklebust X-Patchwork-Id: 2853706 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 AFC669F485 for ; Wed, 4 Sep 2013 14:31:21 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D889B2039C for ; Wed, 4 Sep 2013 14:31:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id B23772037B for ; Wed, 4 Sep 2013 14:31:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1762689Ab3IDObK (ORCPT ); Wed, 4 Sep 2013 10:31:10 -0400 Received: from mx12.netapp.com ([216.240.18.77]:37691 "EHLO mx12.netapp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1762664Ab3IDObJ (ORCPT ); Wed, 4 Sep 2013 10:31:09 -0400 X-IronPort-AV: E=Sophos;i="4.89,1021,1367996400"; d="scan'208,223";a="86588288" Received: from vmwexceht02-prd.hq.netapp.com ([10.106.76.240]) by mx12-out.netapp.com with ESMTP; 04 Sep 2013 07:31:08 -0700 Received: from SACEXCMBX04-PRD.hq.netapp.com ([169.254.6.213]) by vmwexceht02-prd.hq.netapp.com ([10.106.76.240]) with mapi id 14.03.0123.003; Wed, 4 Sep 2013 07:31:07 -0700 From: "Myklebust, Trond" To: NeilBrown CC: NFS Subject: Re: [PATCH - alt-2] NFSv4: Don't try to recover NFSv4 locks when they are lost. Thread-Topic: [PATCH - alt-2] NFSv4: Don't try to recover NFSv4 locks when they are lost. Thread-Index: AQHOqT0cJZMdbfQqF0ytWF7FWcMQPZm2GaYA Date: Wed, 4 Sep 2013 14:31:07 +0000 Message-ID: <1378305066.3527.8.camel@leira.trondhjem.org> References: <20130904170449.7d2662cd@notabene.brown> In-Reply-To: <20130904170449.7d2662cd@notabene.brown> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: yes X-MS-TNEF-Correlator: x-originating-ip: [10.106.53.51] MIME-Version: 1.0 Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org X-Spam-Status: No, score=-9.3 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_TVD_MIME_EPI, UNPARSEABLE_RELAY autolearn=unavailable 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, 2013-09-04 at 17:04 +1000, NeilBrown wrote: > > When an NFSv4 client loses contact with the server it can lose any > locks that it holds. > > Currently when it reconnects to the server it simply tries to reclaim > those locks. This might succeed even though some other client has > held and released a lock in the mean time. So the first client might > think the file is unchanged, but it isn't. This isn't good. > > If, when recovery happens, the locks cannot be claimed because some > other client still holds the lock, then we get a message in the kernel > logs, but the client can still write. So two clients can both think > they have a lock and can both write at the same time. This is equally > not good. > > There was a patch a while ago > http://comments.gmane.org/gmane.linux.nfs/41917 > > which tried to address some of this, but it didn't seem to go > anywhere. That patch would also send a signal to the process. That > might be useful but for now this patch just causes writes to fail. > > For NFSv4 (unlike v2/v3) there is a strong link between the lock and > the write request so we can fairly easily fail any IO of the lock is > gone. While some applications might not expect this, it is still > safer than allowing the write to succeed. > > Because this is a fairly big change in behaviour a module parameter, > "recover_locks", is introduced which defaults to true (the current > behaviour) but can be set to "false" to tell the client not to try to > recover things that were lost. > > Signed-off-by: NeilBrown Thanks! > -- > This alternative uses a module parameter which defaults to the current, > incorrect, behaviour. > I suspect we don't want that one.. Agreed. We also need to document the module parameter, so I'm adding a little blurb in Documentation/kernel-parameters.txt (see attachment). I'd also like to change the parameter name to "recover_lost_locks" to make it a little more obvious. Finally, I'd like to move the parameter to fs/nfs/super.c so that we can use the same modprobe.conf 'options' lines for back ports of this patch (yes I strongly suspect we will want to back port this patch to distro kernels). See attachment. Cheers Trond -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com From 7722b2fa150b9b7b21218923ad01b757a6653b10 Mon Sep 17 00:00:00 2001 From: Trond Myklebust Date: Wed, 4 Sep 2013 10:08:54 -0400 Subject: [PATCH] NFSv4: Document the recover_lost_locks kernel parameter Rename the new 'recover_locks' kernel parameter to 'recover_lost_locks' and change the default to 'false'. Document why in Documentation/kernel-parameters.txt Move the 'recover_lost_locks' kernel parameter to fs/nfs/super.c to make it easy to backport to kernels prior to 3.6.x, which don't have a separate NFSv4 module. Signed-off-by: Trond Myklebust --- Documentation/kernel-parameters.txt | 12 ++++++++++++ fs/nfs/nfs4_fs.h | 1 + fs/nfs/nfs4proc.c | 8 +------- fs/nfs/super.c | 8 ++++++++ 4 files changed, 22 insertions(+), 7 deletions(-) diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index 15356ac..30584b1 100644 --- a/Documentation/kernel-parameters.txt +++ b/Documentation/kernel-parameters.txt @@ -1847,6 +1847,18 @@ bytes respectively. Such letter suffixes can also be entirely omitted. will be sent. The default is to send the implementation identification information. + + nfs.recover_lost_locks = + [NFSv4] Attempt to recover locks that were lost due + to a lease timeout on the server. Please note that + doing this risks data corruption, since there are + no guarantees that the file will remain unchanged + after the locks are lost. + If you want to enable the kernel legacy behaviour of + attempting to recover these locks, then set this + parameter to '1'. + The default parameter value of '0' causes the kernel + not to attempt recovery of lost locks. nfsd.nfs4_disable_idmapping= [NFSv4] When set to the default of '1', the NFSv4 diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h index 6411831..277407d 100644 --- a/fs/nfs/nfs4_fs.h +++ b/fs/nfs/nfs4_fs.h @@ -372,6 +372,7 @@ struct dentry *nfs4_try_mount(int, const char *, struct nfs_mount_info *, struct extern bool nfs4_disable_idmapping; extern unsigned short max_session_slots; extern unsigned short send_implementation_id; +extern bool recover_lost_locks; #define NFS4_CLIENT_ID_UNIQ_LEN (64) extern char nfs4_client_id_uniquifier[NFS4_CLIENT_ID_UNIQ_LEN]; diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 1eb694e..535011a 100644 --- a/fs/nfs/nfs4proc.c +++ b/fs/nfs/nfs4proc.c @@ -5523,12 +5523,6 @@ static int nfs4_lock_reclaim(struct nfs4_state *state, struct file_lock *request return err; } -bool recover_locks = true; -module_param(recover_locks, bool, 0644); -MODULE_PARM_DESC(recover_locks, - "If the server reports that a lock might be lost, " - "try to recovery it risking corruption."); - static int nfs4_lock_expired(struct nfs4_state *state, struct file_lock *request) { struct nfs_server *server = NFS_SERVER(state->inode); @@ -5540,7 +5534,7 @@ static int nfs4_lock_expired(struct nfs4_state *state, struct file_lock *request err = nfs4_set_lock_state(state, request); if (err != 0) return err; - if (!recover_locks) { + if (!recover_lost_locks) { set_bit(NFS_LOCK_LOST, &request->fl_u.nfs4_fl.owner->ls_flags); return 0; } diff --git a/fs/nfs/super.c b/fs/nfs/super.c index f2071d2..6ad9053 100644 --- a/fs/nfs/super.c +++ b/fs/nfs/super.c @@ -2758,6 +2758,7 @@ bool nfs4_disable_idmapping = true; unsigned short max_session_slots = NFS4_DEF_SLOT_TABLE_SIZE; unsigned short send_implementation_id = 1; char nfs4_client_id_uniquifier[NFS4_CLIENT_ID_UNIQ_LEN] = ""; +bool recover_lost_locks = false; EXPORT_SYMBOL_GPL(nfs_callback_set_tcpport); EXPORT_SYMBOL_GPL(nfs_callback_tcpport); @@ -2766,6 +2767,7 @@ EXPORT_SYMBOL_GPL(nfs4_disable_idmapping); EXPORT_SYMBOL_GPL(max_session_slots); EXPORT_SYMBOL_GPL(send_implementation_id); EXPORT_SYMBOL_GPL(nfs4_client_id_uniquifier); +EXPORT_SYMBOL_GPL(recover_lost_locks); #define NFS_CALLBACK_MAXPORTNR (65535U) @@ -2803,4 +2805,10 @@ MODULE_PARM_DESC(send_implementation_id, "Send implementation ID with NFSv4.1 exchange_id"); MODULE_PARM_DESC(nfs4_unique_id, "nfs_client_id4 uniquifier string"); +module_param(recover_lost_locks, bool, 0644); +MODULE_PARM_DESC(recover_lost_locks, + "If the server reports that a lock might be lost, " + "try to recover it risking data corruption."); + + #endif /* CONFIG_NFS_V4 */ -- 1.8.3.1