diff mbox

[01/14] NFS: Clean up TEST_STATEID and FREE_STATEID proc error reporting

Message ID 20120709154349.1604.58109.stgit@degas.1015granger.net (mailing list archive)
State New, archived
Headers show

Commit Message

Chuck Lever July 9, 2012, 3:43 p.m. UTC 
The TEST_STATEID and FREE_STATEID operations can return
NFS4ERR_BAD_STATEID, NFS4ERR_OLD_STATEID, or NFS4ERR_DEADSESSION.
These error values should not be passed to nfs4_handle_exception(),
since that will recursively kick off state recovery, resulting in a
deadlock.  The reason to continue using nfs4_handle_exception() is
to deal with NFS4ERR_DELAY.

Moreover, specifically when the TEST_STATEID operation returns
NFS4_OK, res.status can contain one of these errors.
_nfs41_test_stateid() replaces NFS4_OK with the value in res.status,
which is then returned to callers.

But res.status is not passed through nfs4_stat_to_errno(), and thus is
a positive NFS4ERR value.  Currently callers are only interested in
!NFS4_OK, and nfs4_handle_exception() ignores positive values, so this
oversight hasn't bitten us so far.

Bryan agrees the original intent was to return res.status as a
negative NFS4ERR value to callers of nfs41_test_stateid(), as long as
no state ID recovery is attempted.

As a finishing touch, add appropriate documenting comments and some
debugging printk's.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/nfs/nfs4proc.c |   51 ++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 40 insertions(+), 11 deletions(-)


--
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

Comments

Trond Myklebust July 9, 2012, 7:36 p.m. UTC | #1 
On Mon, 2012-07-09 at 11:43 -0400, Chuck Lever wrote:
> The TEST_STATEID and FREE_STATEID operations can return

> NFS4ERR_BAD_STATEID, NFS4ERR_OLD_STATEID, or NFS4ERR_DEADSESSION.

> These error values should not be passed to nfs4_handle_exception(),

> since that will recursively kick off state recovery, resulting in a

> deadlock.  The reason to continue using nfs4_handle_exception() is

> to deal with NFS4ERR_DELAY.

> 

> Moreover, specifically when the TEST_STATEID operation returns

> NFS4_OK, res.status can contain one of these errors.

> _nfs41_test_stateid() replaces NFS4_OK with the value in res.status,

> which is then returned to callers.

> 

> But res.status is not passed through nfs4_stat_to_errno(), and thus is

> a positive NFS4ERR value.  Currently callers are only interested in

> !NFS4_OK, and nfs4_handle_exception() ignores positive values, so this

> oversight hasn't bitten us so far.

> 

> Bryan agrees the original intent was to return res.status as a

> negative NFS4ERR value to callers of nfs41_test_stateid(), as long as

> no state ID recovery is attempted.


None of this describes a cleanup... AFAICS it is all a bugfix

> As a finishing touch, add appropriate documenting comments and some

> debugging printk's.


This should be a separate patch so that we can send the bugfix part to
stable@vger if needed...

> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

> ---



-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com
Chuck Lever July 9, 2012, 7:51 p.m. UTC | #2 
On Jul 9, 2012, at 3:36 PM, Myklebust, Trond wrote:

> On Mon, 2012-07-09 at 11:43 -0400, Chuck Lever wrote:
>> The TEST_STATEID and FREE_STATEID operations can return
>> NFS4ERR_BAD_STATEID, NFS4ERR_OLD_STATEID, or NFS4ERR_DEADSESSION.
>> These error values should not be passed to nfs4_handle_exception(),
>> since that will recursively kick off state recovery, resulting in a
>> deadlock.  The reason to continue using nfs4_handle_exception() is
>> to deal with NFS4ERR_DELAY.
>> 
>> Moreover, specifically when the TEST_STATEID operation returns
>> NFS4_OK, res.status can contain one of these errors.
>> _nfs41_test_stateid() replaces NFS4_OK with the value in res.status,
>> which is then returned to callers.
>> 
>> But res.status is not passed through nfs4_stat_to_errno(), and thus is
>> a positive NFS4ERR value.  Currently callers are only interested in
>> !NFS4_OK, and nfs4_handle_exception() ignores positive values, so this
>> oversight hasn't bitten us so far.
>> 
>> Bryan agrees the original intent was to return res.status as a
>> negative NFS4ERR value to callers of nfs41_test_stateid(), as long as
>> no state ID recovery is attempted.
> 
> None of this describes a cleanup... AFAICS it is all a bugfix

IIRC behavior isn't supposed to change, so I don't think this should go to -stable.  The subsequent patches rely on the error path being corrected, though.

>> As a finishing touch, add appropriate documenting comments and some
>> debugging printk's.
> 
> This should be a separate patch so that we can send the bugfix part to
> stable@vger if needed...

I can redrive as two patches, but we should be careful about back porting, IMO.
diff mbox

Patch

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 15fc7e4..971ec8c 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6587,22 +6587,36 @@  static int _nfs41_test_stateid(struct nfs_server *server, nfs4_stateid *stateid)
 		.rpc_resp = &res,
 	};
 
+	dprintk("NFS call  test_stateid %p\n", stateid);
 	nfs41_init_sequence(&args.seq_args, &res.seq_res, 0);
 	status = nfs4_call_sync_sequence(server->client, server, &msg, &args.seq_args, &res.seq_res, 1);
-
-	if (status == NFS_OK)
-		return res.status;
-	return status;
+	if (status != NFS_OK) {
+		dprintk("NFS reply test_stateid: failed, %d\n", status);
+		return status;
+	}
+	dprintk("NFS reply test_stateid: succeeded, %d\n", -res.status);
+	return -res.status;
 }
 
+/**
+ * nfs41_test_stateid - perform a TEST_STATEID operation
+ *
+ * @server: NFS server that may know about "stateid"
+ * @stateid: state ID to test
+ *
+ * Returns NFS_OK if the server recognizes the state ID as valid.
+ * Otherwise a negative NFS4ERR value is returned if the operation
+ * failed or the state ID is not currently valid.
+ */
 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, stateid),
-				&exception);
+		err = _nfs41_test_stateid(server, stateid);
+		if (err != -NFS4ERR_DELAY)
+			break;
+		nfs4_handle_exception(server, err, &exception);
 	} while (exception.retry);
 	return err;
 }
@@ -6618,19 +6632,34 @@  static int _nfs4_free_stateid(struct nfs_server *server, nfs4_stateid *stateid)
 		.rpc_argp = &args,
 		.rpc_resp = &res,
 	};
+	int status;
 
+	dprintk("NFS call  free_stateid %p\n", stateid);
 	nfs41_init_sequence(&args.seq_args, &res.seq_res, 0);
-	return nfs4_call_sync_sequence(server->client, server, &msg, &args.seq_args, &res.seq_res, 1);
+	status = nfs4_call_sync_sequence(server->client, server, &msg,
+					 &args.seq_args, &res.seq_res, 1);
+	dprintk("NFS reply free_stateid: %d\n", status);
+	return status;
 }
 
+/**
+ * nfs41_free_stateid - perform a FREE_STATEID operation
+ *
+ * @server: NFS server that may know about "stateid"
+ * @stateid: state ID to release
+ *
+ * Returns NFS_OK if the server freed the state ID.  Otherwise a
+ * negative NFS4ERR value is returned if the operation failed.
+ */
 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, stateid),
-				&exception);
+		err = _nfs4_free_stateid(server, stateid);
+		if (err != -NFS4ERR_DELAY)
+			break;
+		nfs4_handle_exception(server, err, &exception);
 	} while (exception.retry);
 	return err;
 }