[PATCH v2 1/2] NFS: Test and Free stateid should take a stateid argument

[PATCH v2 1/2] NFS: Test and Free stateid should take a stateid argument

[bjschuma]

From: Bryan Schumaker <bjschuma@netapp.com>

Passing the nfs4_state doesn't guarantee that the correct stateid will
be used.  The correct stateid can be determined during lock_expired() or
open_expired() and then passed to these functions.

Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
---
 fs/nfs/nfs4proc.c |   31 +++++++++++++++++--------------
 1 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 93ef776..f37218b 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -81,8 +81,8 @@ static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
 			    struct nfs_fattr *fattr, struct iattr *sattr,
 			    struct nfs4_state *state);
 #ifdef CONFIG_NFS_V4_1
-static int nfs41_test_stateid(struct nfs_server *, struct nfs4_state *);
-static int nfs41_free_stateid(struct nfs_server *, struct nfs4_state *);
+static int nfs41_test_stateid(struct nfs_server *, nfs4_stateid *);
+static int nfs41_free_stateid(struct nfs_server *, nfs4_stateid *);
 #endif
 /* Prevent leaks of NFSv4 errors into userland */
 static int nfs4_map_errors(int err)
@@ -1696,10 +1696,10 @@ static int nfs41_open_expired(struct nfs4_state_owner *sp, struct nfs4_state *st
 	int status;
 	struct nfs_server *server = NFS_SERVER(state->inode);
 
-	status = nfs41_test_stateid(server, state);
+	status = nfs41_test_stateid(server, &state->open_stateid);
 	if (status == NFS_OK)
 		return 0;
-	nfs41_free_stateid(server, state);
+	nfs41_free_stateid(server, &state->open_stateid);
 	return nfs4_open_expired(sp, state);
 }
 #endif
@@ -4466,11 +4466,14 @@ static int nfs41_lock_expired(struct nfs4_state *state, struct file_lock *reques
 {
 	int status;
 	struct nfs_server *server = NFS_SERVER(state->inode);
+	struct nfs4_lock_state *lock_state = request->fl_u.nfs4_fl.owner;
+	nfs4_stateid *stateid = &lock_state->ls_stateid;
 
-	status = nfs41_test_stateid(server, state);
+	status = nfs41_test_stateid(server, stateid);
 	if (status == NFS_OK)
 		return 0;
-	nfs41_free_stateid(server, state);
+	nfs41_free_stateid(server, stateid);
+	lock_state->ls_seqid.flags &= !NFS_SEQID_CONFIRMED;
 	return nfs4_lock_expired(state, request);
 }
 #endif
@@ -6046,11 +6049,11 @@ out_freepage:
 out:
 	return err;
 }
-static int _nfs41_test_stateid(struct nfs_server *server, struct nfs4_state *state)
+static int _nfs41_test_stateid(struct nfs_server *server, nfs4_stateid *stateid)
 {
 	int status;
 	struct nfs41_test_stateid_args args = {
-		.stateid = &state->stateid,
+		.stateid = stateid,
 	};
 	struct nfs41_test_stateid_res res;
 	struct rpc_message msg = {
@@ -6063,23 +6066,23 @@ static int _nfs41_test_stateid(struct nfs_server *server, struct nfs4_state *sta
 	return status;
 }
 
-static int nfs41_test_stateid(struct nfs_server *server, struct nfs4_state *state)
+static int nfs41_test_stateid(struct nfs_server *server, nfs4_stateid *stateid)
 {
 	struct nfs4_exception exception = { };
 	int err;
 	do {
 		err = nfs4_handle_exception(server,
-				_nfs41_test_stateid(server, state),
+				_nfs41_test_stateid(server, stateid),
 				&exception);
 	} while (exception.retry);
 	return err;
 }
 
-static int _nfs4_free_stateid(struct nfs_server *server, struct nfs4_state *state)
+static int _nfs4_free_stateid(struct nfs_server *server, nfs4_stateid *stateid)
 {
 	int status;
 	struct nfs41_free_stateid_args args = {
-		.stateid = &state->stateid,
+		.stateid = stateid,
 	};
 	struct nfs41_free_stateid_res res;
 	struct rpc_message msg = {
@@ -6093,13 +6096,13 @@ static int _nfs4_free_stateid(struct nfs_server *server, struct nfs4_state *stat
 	return status;
 }
 
-static int nfs41_free_stateid(struct nfs_server *server, struct nfs4_state *state)
+static int nfs41_free_stateid(struct nfs_server *server, nfs4_stateid *stateid)
 {
 	struct nfs4_exception exception = { };
 	int err;
 	do {
 		err = nfs4_handle_exception(server,
-				_nfs4_free_stateid(server, state),
+				_nfs4_free_stateid(server, stateid),
 				&exception);
 	} while (exception.retry);
 	return err;
-- 
1.7.6

[PATCH v2 2/2] NFS: Send SIGIO on lost locks

[bjschuma]

From: Bryan Schumaker <bjschuma@netapp.com>

If we discover that the server has lost a client's lock, we send SIGIO to the
client application and let the application decide the best method of handling
the error.

This could also be done by sending the SIGLOST signal to the userland
application.  Most architectures have a definition of SIGIO commented out in
their signal.h.  In most cases, SIGLOST would be defined to the same value as
SIGIO.  A new signal could be created, but there are no available slots before
SIGRTMIN.

Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
CC: Trond Myklebust <Trond.Myklebust@netapp.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redahat.com>
CC: David Howels <dhowells@redhat.com>
CC: Steve French <sfrench@samba.org>
CC: Miklos Szeredi <miklos@szeredi.hu>
CC: linux-fsdevel@vger.kernel.org
---
 Documentation/kernel-parameters.txt |    7 +++++++
 fs/nfs/nfs4_fs.h                    |    2 ++
 fs/nfs/nfs4proc.c                   |   21 +++++++++++++++++++++
 fs/nfs/nfs4state.c                  |   14 ++++++++++++++
 fs/nfs/read.c                       |    9 +++++++++
 fs/nfs/write.c                      |    7 +++++++
 6 files changed, 60 insertions(+), 0 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index fd248a31..988ebdc 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1593,6 +1593,13 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			of returning the full 64-bit number.
 			The default is to return 64-bit inode numbers.
 
+	nfs.enable_sigio=
+			[NFSv4.1] enable sending SIGIO to applications.
+			If true, the NFS client will send SIGIO to applications
+			when a lost file lock is detected.  If false, the client
+			will attempt to recover the lock.
+			The default is to send SIGIO.
+
 	nfs.nfs4_disable_idmapping=
 			[NFSv4] When set, this option disables the NFSv4
 			idmapper on the client, but only if the mount
diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index c30aed2..8fa02af 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -163,6 +163,7 @@ struct nfs4_lock_state {
 	struct list_head	ls_locks;	/* Other lock stateids */
 	struct nfs4_state *	ls_state;	/* Pointer to open state */
 #define NFS_LOCK_INITIALIZED 1
+#define NFS_LOCK_INVALID     2
 	int			ls_flags;
 	struct nfs_seqid_counter	ls_seqid;
 	struct rpc_sequence	ls_sequence;
@@ -346,6 +347,7 @@ extern struct nfs4_state * nfs4_get_open_state(struct inode *, struct nfs4_state
 extern void nfs4_put_open_state(struct nfs4_state *);
 extern void nfs4_close_state(struct path *, struct nfs4_state *, fmode_t);
 extern void nfs4_close_sync(struct path *, struct nfs4_state *, fmode_t);
+extern int nfs4_validate_lock_stateid(struct nfs4_state *, struct nfs_lock_context *);
 extern void nfs4_state_set_mode_locked(struct nfs4_state *, fmode_t);
 extern void nfs4_schedule_lease_recovery(struct nfs_client *);
 extern void nfs4_schedule_state_manager(struct nfs_client *);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index f37218b..bb29384 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4462,6 +4462,8 @@ out:
 }
 
 #if defined(CONFIG_NFS_V4_1)
+bool enable_sigio = true;
+
 static int nfs41_lock_expired(struct nfs4_state *state, struct file_lock *request)
 {
 	int status;
@@ -4472,10 +4474,29 @@ static int nfs41_lock_expired(struct nfs4_state *state, struct file_lock *reques
 	status = nfs41_test_stateid(server, stateid);
 	if (status == NFS_OK)
 		return 0;
+
 	nfs41_free_stateid(server, stateid);
 	lock_state->ls_seqid.flags &= !NFS_SEQID_CONFIRMED;
+
+	if (enable_sigio) {
+		u32 secid;
+		struct siginfo info = {
+			.si_signo = SIGIO,
+			.si_code  = __SI_SIGLOST,
+			.si_errno = -ENOLCK,
+		};
+		lock_state->ls_flags |= NFS_LOCK_INVALID;
+		security_task_getsecid(current, &secid);
+		kill_pid_info_as_uid(SIGIO, &info, request->fl_nspid,
+			current_cred()->uid, current_cred()->euid, secid);
+		return 0;
+	}
 	return nfs4_lock_expired(state, request);
 }
+
+module_param(enable_sigio, bool, 0644);
+MODULE_PARM_DESC(enable_sigio, "Send SIGIO to an application when a lost lock"
+			"is detected instead of attempting recovery");
 #endif
 
 static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock *request)
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 5d744a5..101d9b4 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -725,6 +725,20 @@ __nfs4_find_lock_state(struct nfs4_state *state, fl_owner_t fl_owner, pid_t fl_p
 	return NULL;
 }
 
+int nfs4_validate_lock_stateid(struct nfs4_state *state, struct nfs_lock_context *lock_ctx)
+{
+	int valid = 1;
+	struct nfs4_lock_state *lock_state;
+
+	spin_lock(&state->state_lock);
+	lock_state = __nfs4_find_lock_state(state, lock_ctx->lockowner, lock_ctx->pid, NFS4_ANY_LOCK_TYPE);
+	if (lock_state != NULL)
+		valid = (lock_state->ls_flags & NFS_LOCK_INVALID) == 0;
+	spin_unlock(&state->state_lock);
+	nfs4_put_lock_state(lock_state);
+	return valid;
+}
+
 /*
  * Return a compatible lock_state. If no initialized lock_state structure
  * exists, return an uninitialized one.
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index c394662..c597f9e 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -468,6 +468,15 @@ static void nfs_readpage_release_partial(void *calldata)
 void nfs_read_prepare(struct rpc_task *task, void *calldata)
 {
 	struct nfs_read_data *data = calldata;
+	struct nfs4_state *state = data->args.context->state;
+	struct nfs_lock_context *lock_context = data->args.lock_context;
+
+	if (!nfs4_validate_lock_stateid(state, lock_context)) {
+		task->tk_status = -EIO;
+		task->tk_action = NULL;
+		return;
+	}
+
 
 	if (nfs4_setup_sequence(NFS_SERVER(data->inode),
 				&data->args.seq_args, &data->res.seq_res,
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 7f8732e..49ca975 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1114,7 +1114,14 @@ out:
 void nfs_write_prepare(struct rpc_task *task, void *calldata)
 {
 	struct nfs_write_data *data = calldata;
+	struct nfs4_state *state = data->args.context->state;
+	struct nfs_lock_context *lock_context = data->args.lock_context;
 
+	if (!nfs4_validate_lock_stateid(state, lock_context)) {
+		task->tk_status = -EIO;
+		task->tk_action = NULL;
+		return;
+	}
 	if (nfs4_setup_sequence(NFS_SERVER(data->inode),
 				&data->args.seq_args,
 				&data->res.seq_res, 1, task))
-- 
1.7.6

Re: [PATCH v2 2/2] NFS: Send SIGIO on lost locks

[Bryan Schumaker]

I fixed Ingo's email address in the patch, it should be "@redhat.com" and not "@redahat".  Sorry about that.

- Bryan

On 07/18/2011 01:53 PM, bjschuma@netapp.com wrote:
> From: Bryan Schumaker <bjschuma@netapp.com>
> 
> If we discover that the server has lost a client's lock, we send SIGIO to the
> client application and let the application decide the best method of handling
> the error.
> 
> This could also be done by sending the SIGLOST signal to the userland
> application.  Most architectures have a definition of SIGIO commented out in
> their signal.h.  In most cases, SIGLOST would be defined to the same value as
> SIGIO.  A new signal could be created, but there are no available slots before
> SIGRTMIN.
> 
> Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
> CC: Trond Myklebust <Trond.Myklebust@netapp.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: David Howels <dhowells@redhat.com>
> CC: Steve French <sfrench@samba.org>
> CC: Miklos Szeredi <miklos@szeredi.hu>
> CC: linux-fsdevel@vger.kernel.org
> ---
>  Documentation/kernel-parameters.txt |    7 +++++++
>  fs/nfs/nfs4_fs.h                    |    2 ++
>  fs/nfs/nfs4proc.c                   |   21 +++++++++++++++++++++
>  fs/nfs/nfs4state.c                  |   14 ++++++++++++++
>  fs/nfs/read.c                       |    9 +++++++++
>  fs/nfs/write.c                      |    7 +++++++
>  6 files changed, 60 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index fd248a31..988ebdc 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1593,6 +1593,13 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			of returning the full 64-bit number.
>  			The default is to return 64-bit inode numbers.
>  
> +	nfs.enable_sigio=
> +			[NFSv4.1] enable sending SIGIO to applications.
> +			If true, the NFS client will send SIGIO to applications
> +			when a lost file lock is detected.  If false, the client
> +			will attempt to recover the lock.
> +			The default is to send SIGIO.
> +
>  	nfs.nfs4_disable_idmapping=
>  			[NFSv4] When set, this option disables the NFSv4
>  			idmapper on the client, but only if the mount
> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> index c30aed2..8fa02af 100644
> --- a/fs/nfs/nfs4_fs.h
> +++ b/fs/nfs/nfs4_fs.h
> @@ -163,6 +163,7 @@ struct nfs4_lock_state {
>  	struct list_head	ls_locks;	/* Other lock stateids */
>  	struct nfs4_state *	ls_state;	/* Pointer to open state */
>  #define NFS_LOCK_INITIALIZED 1
> +#define NFS_LOCK_INVALID     2
>  	int			ls_flags;
>  	struct nfs_seqid_counter	ls_seqid;
>  	struct rpc_sequence	ls_sequence;
> @@ -346,6 +347,7 @@ extern struct nfs4_state * nfs4_get_open_state(struct inode *, struct nfs4_state
>  extern void nfs4_put_open_state(struct nfs4_state *);
>  extern void nfs4_close_state(struct path *, struct nfs4_state *, fmode_t);
>  extern void nfs4_close_sync(struct path *, struct nfs4_state *, fmode_t);
> +extern int nfs4_validate_lock_stateid(struct nfs4_state *, struct nfs_lock_context *);
>  extern void nfs4_state_set_mode_locked(struct nfs4_state *, fmode_t);
>  extern void nfs4_schedule_lease_recovery(struct nfs_client *);
>  extern void nfs4_schedule_state_manager(struct nfs_client *);
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index f37218b..bb29384 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -4462,6 +4462,8 @@ out:
>  }
>  
>  #if defined(CONFIG_NFS_V4_1)
> +bool enable_sigio = true;
> +
>  static int nfs41_lock_expired(struct nfs4_state *state, struct file_lock *request)
>  {
>  	int status;
> @@ -4472,10 +4474,29 @@ static int nfs41_lock_expired(struct nfs4_state *state, struct file_lock *reques
>  	status = nfs41_test_stateid(server, stateid);
>  	if (status == NFS_OK)
>  		return 0;
> +
>  	nfs41_free_stateid(server, stateid);
>  	lock_state->ls_seqid.flags &= !NFS_SEQID_CONFIRMED;
> +
> +	if (enable_sigio) {
> +		u32 secid;
> +		struct siginfo info = {
> +			.si_signo = SIGIO,
> +			.si_code  = __SI_SIGLOST,
> +			.si_errno = -ENOLCK,
> +		};
> +		lock_state->ls_flags |= NFS_LOCK_INVALID;
> +		security_task_getsecid(current, &secid);
> +		kill_pid_info_as_uid(SIGIO, &info, request->fl_nspid,
> +			current_cred()->uid, current_cred()->euid, secid);
> +		return 0;
> +	}
>  	return nfs4_lock_expired(state, request);
>  }
> +
> +module_param(enable_sigio, bool, 0644);
> +MODULE_PARM_DESC(enable_sigio, "Send SIGIO to an application when a lost lock"
> +			"is detected instead of attempting recovery");
>  #endif
>  
>  static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock *request)
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index 5d744a5..101d9b4 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -725,6 +725,20 @@ __nfs4_find_lock_state(struct nfs4_state *state, fl_owner_t fl_owner, pid_t fl_p
>  	return NULL;
>  }
>  
> +int nfs4_validate_lock_stateid(struct nfs4_state *state, struct nfs_lock_context *lock_ctx)
> +{
> +	int valid = 1;
> +	struct nfs4_lock_state *lock_state;
> +
> +	spin_lock(&state->state_lock);
> +	lock_state = __nfs4_find_lock_state(state, lock_ctx->lockowner, lock_ctx->pid, NFS4_ANY_LOCK_TYPE);
> +	if (lock_state != NULL)
> +		valid = (lock_state->ls_flags & NFS_LOCK_INVALID) == 0;
> +	spin_unlock(&state->state_lock);
> +	nfs4_put_lock_state(lock_state);
> +	return valid;
> +}
> +
>  /*
>   * Return a compatible lock_state. If no initialized lock_state structure
>   * exists, return an uninitialized one.
> diff --git a/fs/nfs/read.c b/fs/nfs/read.c
> index c394662..c597f9e 100644
> --- a/fs/nfs/read.c
> +++ b/fs/nfs/read.c
> @@ -468,6 +468,15 @@ static void nfs_readpage_release_partial(void *calldata)
>  void nfs_read_prepare(struct rpc_task *task, void *calldata)
>  {
>  	struct nfs_read_data *data = calldata;
> +	struct nfs4_state *state = data->args.context->state;
> +	struct nfs_lock_context *lock_context = data->args.lock_context;
> +
> +	if (!nfs4_validate_lock_stateid(state, lock_context)) {
> +		task->tk_status = -EIO;
> +		task->tk_action = NULL;
> +		return;
> +	}
> +
>  
>  	if (nfs4_setup_sequence(NFS_SERVER(data->inode),
>  				&data->args.seq_args, &data->res.seq_res,
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index 7f8732e..49ca975 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -1114,7 +1114,14 @@ out:
>  void nfs_write_prepare(struct rpc_task *task, void *calldata)
>  {
>  	struct nfs_write_data *data = calldata;
> +	struct nfs4_state *state = data->args.context->state;
> +	struct nfs_lock_context *lock_context = data->args.lock_context;
>  
> +	if (!nfs4_validate_lock_stateid(state, lock_context)) {
> +		task->tk_status = -EIO;
> +		task->tk_action = NULL;
> +		return;
> +	}
>  	if (nfs4_setup_sequence(NFS_SERVER(data->inode),
>  				&data->args.seq_args,
>  				&data->res.seq_res, 1, task))