diff mbox

[-,alt-2] NFSv4: Don't try to recover NFSv4 locks when they are lost.

Message ID 1378305066.3527.8.camel@leira.trondhjem.org (mailing list archive)
State New, archived
Headers show

Commit Message

Trond Myklebust Sept. 4, 2013, 2:31 p.m. UTC 
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 <neilb@suse.de>


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

Comments

NeilBrown Sept. 4, 2013, 11:48 p.m. UTC | #1 
On Wed, 4 Sep 2013 14:31:07 +0000 "Myklebust, Trond"
<Trond.Myklebust@netapp.com> wrote:

> 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 <neilb@suse.de>
> 
> 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.

Looks good - thanks.

NeilBrown
diff mbox

Patch

From 7722b2fa150b9b7b21218923ad01b757a6653b10 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@netapp.com>
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 <Trond.Myklebust@netapp.com>
---
 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