* [PATCH 0/4] Take 2: TEST_STATEID fix-ups
@ 2012-06-13 18:20 Chuck Lever
2012-06-13 18:20 ` [PATCH 1/4] NFS: Clean up TEST_STATEID and FREE_STATEID proc error reporting Chuck Lever
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: Chuck Lever @ 2012-06-13 18:20 UTC (permalink / raw)
To: linux-nfs
For review.
I discovered a few problems with the new TEST_STATEID implementation
while testing nograce recovery. Review comments have been addressed,
and these have been rebased on 3.5-rc2.
---
Chuck Lever (4):
NFS: Refactor nfs41_check_expired_stateid()
NFS: State reclaim clears OPEN and LOCK state
NFS: Properly sort TEST_STATEID results
NFS: Clean up TEST_STATEID and FREE_STATEID proc error reporting
fs/nfs/nfs4proc.c | 146 ++++++++++++++++++++++++++++++++++++++++++-----------
1 files changed, 117 insertions(+), 29 deletions(-)
--
Chuck Lever
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/4] NFS: Clean up TEST_STATEID and FREE_STATEID proc error reporting
2012-06-13 18:20 [PATCH 0/4] Take 2: TEST_STATEID fix-ups Chuck Lever
@ 2012-06-13 18:20 ` Chuck Lever
2012-06-13 18:20 ` [PATCH 2/4] NFS: Properly sort TEST_STATEID results Chuck Lever
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Chuck Lever @ 2012-06-13 18:20 UTC (permalink / raw)
To: linux-nfs
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(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index d48dbef..ef69cc5 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6561,22 +6561,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;
}
@@ -6592,19 +6606,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;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/4] NFS: Properly sort TEST_STATEID results
2012-06-13 18:20 [PATCH 0/4] Take 2: TEST_STATEID fix-ups Chuck Lever
2012-06-13 18:20 ` [PATCH 1/4] NFS: Clean up TEST_STATEID and FREE_STATEID proc error reporting Chuck Lever
@ 2012-06-13 18:20 ` Chuck Lever
2012-06-13 18:20 ` [PATCH 3/4] NFS: State reclaim clears OPEN and LOCK state Chuck Lever
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Chuck Lever @ 2012-06-13 18:20 UTC (permalink / raw)
To: linux-nfs
The result of a TEST_STATEID operation can indicate a few different
things:
o If NFS_OK is returned, then the client can continue using the
state ID under test, and skip recovery.
o RFC 5661 says that if and only if the state ID was revoked, then
the client must perform an explicit FREE_STATEID before trying to
re-open.
o If the server doesn't recognize the state ID at all, then no
FREE_STATEID is needed, and the client can immediately continue
with open recovery.
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
fs/nfs/nfs4proc.c | 36 ++++++++++++++++++++++++++++++++++--
1 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ef69cc5..65bee35 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1735,6 +1735,14 @@ static int nfs4_open_expired(struct nfs4_state_owner *sp, struct nfs4_state *sta
}
#if defined(CONFIG_NFS_V4_1)
+/**
+ * nfs41_check_expired_stateid - does a state ID need recovery?
+ *
+ * @state: NFSv4 open state for an inode
+ *
+ * Returns NFS_OK if recovery for this state ID is now finished.
+ * Otherwise a negative NFS4ERR value is returned.
+ */
static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *stateid, unsigned int flags)
{
int status = NFS_OK;
@@ -1742,8 +1750,16 @@ static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *s
if (state->flags & flags) {
status = nfs41_test_stateid(server, stateid);
- if (status != NFS_OK) {
+ switch (status) {
+ case NFS_OK:
+ /* server recognizes this one, don't recover */
+ break;
+ case -NFS4ERR_ADMIN_REVOKED:
+ case -NFS4ERR_DELEG_REVOKED:
+ /* state was revoked, free then re-open */
nfs41_free_stateid(server, stateid);
+ default:
+ /* anything else: just re-open it */
state->flags &= ~flags;
}
}
@@ -4671,6 +4687,14 @@ out:
}
#if defined(CONFIG_NFS_V4_1)
+/**
+ * nfs41_check_expired_locks - clear lock state IDs
+ *
+ * @state: NFSv4 open state for a file
+ *
+ * Returns NFS_OK if recovery for this state ID is now finished.
+ * Otherwise a negative NFS4ERR value is returned.
+ */
static int nfs41_check_expired_locks(struct nfs4_state *state)
{
int status, ret = NFS_OK;
@@ -4680,8 +4704,16 @@ static int nfs41_check_expired_locks(struct nfs4_state *state)
list_for_each_entry(lsp, &state->lock_states, ls_locks) {
if (lsp->ls_flags & NFS_LOCK_INITIALIZED) {
status = nfs41_test_stateid(server, &lsp->ls_stateid);
- if (status != NFS_OK) {
+ switch (status) {
+ case NFS_OK:
+ /* server recognizes this one, don't re-lock */
+ break;
+ case -NFS4ERR_ADMIN_REVOKED:
+ case -NFS4ERR_DELEG_REVOKED:
+ /* lock was revoked, free then re-lock */
nfs41_free_stateid(server, &lsp->ls_stateid);
+ default:
+ /* anything else: just re-lock it */
lsp->ls_flags &= ~NFS_LOCK_INITIALIZED;
ret = status;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/4] NFS: State reclaim clears OPEN and LOCK state
2012-06-13 18:20 [PATCH 0/4] Take 2: TEST_STATEID fix-ups Chuck Lever
2012-06-13 18:20 ` [PATCH 1/4] NFS: Clean up TEST_STATEID and FREE_STATEID proc error reporting Chuck Lever
2012-06-13 18:20 ` [PATCH 2/4] NFS: Properly sort TEST_STATEID results Chuck Lever
@ 2012-06-13 18:20 ` Chuck Lever
2012-06-13 18:20 ` [PATCH 4/4] NFS: Refactor nfs41_check_expired_stateid() Chuck Lever
2012-06-21 21:04 ` [PATCH 0/4] Take 2: TEST_STATEID fix-ups Chuck Lever
4 siblings, 0 replies; 6+ messages in thread
From: Chuck Lever @ 2012-06-13 18:20 UTC (permalink / raw)
To: linux-nfs
Before beginning state recovery, the state manager zaps open and lock
state for each open file, thus the "state->flags & flags" test in
nfs41_{open,lock}_expired() always fails during reclaim. But open
recovery is still needed for these files.
To force a call to nfs4_open_expired(), change the default return
value for nfs41_check_expired_stateid() to force open recovery, and
the default return value for nfs41_check_locks() to force lock
recovery, if the requested flags are clear. Fix suggested by Bryan
Schumaker.
The presence of a delegation state ID must not prevent normal open
recovery. The delegation state ID must be cleared if it was revoked,
but once cleared I don't think it's presence or absence has any
bearing on whether open recovery is still needed.
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
fs/nfs/nfs4proc.c | 23 ++++++++++++-----------
1 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 65bee35..1131877 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1745,8 +1745,8 @@ static int nfs4_open_expired(struct nfs4_state_owner *sp, struct nfs4_state *sta
*/
static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *stateid, unsigned int flags)
{
- int status = NFS_OK;
struct nfs_server *server = NFS_SERVER(state->inode);
+ int status = -NFS4ERR_BAD_STATEID;
if (state->flags & flags) {
status = nfs41_test_stateid(server, stateid);
@@ -1768,16 +1768,17 @@ static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *s
static int nfs41_open_expired(struct nfs4_state_owner *sp, struct nfs4_state *state)
{
- int deleg_status, open_status;
int deleg_flags = 1 << NFS_DELEGATED_STATE;
int open_flags = (1 << NFS_O_RDONLY_STATE) | (1 << NFS_O_WRONLY_STATE) | (1 << NFS_O_RDWR_STATE);
+ int status;
- deleg_status = nfs41_check_expired_stateid(state, &state->stateid, deleg_flags);
- open_status = nfs41_check_expired_stateid(state, &state->open_stateid, open_flags);
+ nfs41_check_expired_stateid(state, &state->stateid, deleg_flags);
+ status = nfs41_check_expired_stateid(state, &state->open_stateid,
+ open_flags);
- if ((deleg_status == NFS_OK) && (open_status == NFS_OK))
- return NFS_OK;
- return nfs4_open_expired(sp, state);
+ if (status != NFS_OK)
+ status = nfs4_open_expired(sp, state);
+ return status;
}
#endif
@@ -4697,7 +4698,7 @@ out:
*/
static int nfs41_check_expired_locks(struct nfs4_state *state)
{
- int status, ret = NFS_OK;
+ int status, ret = -NFS4ERR_BAD_STATEID;
struct nfs4_lock_state *lsp;
struct nfs_server *server = NFS_SERVER(state->inode);
@@ -4729,9 +4730,9 @@ static int nfs41_lock_expired(struct nfs4_state *state, struct file_lock *reques
if (test_bit(LK_STATE_IN_USE, &state->flags))
status = nfs41_check_expired_locks(state);
- if (status == NFS_OK)
- return status;
- return nfs4_lock_expired(state, request);
+ if (status != NFS_OK)
+ status = nfs4_lock_expired(state, request);
+ return status;
}
#endif
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 4/4] NFS: Refactor nfs41_check_expired_stateid()
2012-06-13 18:20 [PATCH 0/4] Take 2: TEST_STATEID fix-ups Chuck Lever
` (2 preceding siblings ...)
2012-06-13 18:20 ` [PATCH 3/4] NFS: State reclaim clears OPEN and LOCK state Chuck Lever
@ 2012-06-13 18:20 ` Chuck Lever
2012-06-21 21:04 ` [PATCH 0/4] Take 2: TEST_STATEID fix-ups Chuck Lever
4 siblings, 0 replies; 6+ messages in thread
From: Chuck Lever @ 2012-06-13 18:20 UTC (permalink / raw)
To: linux-nfs
Clean up: The logic for dealing with a delegation state ID and an
open state ID is now sufficiently distinct that I've refactored
nfs41_check_expired_stateid() into two separate functions.
Instead of open-coded flag manipulation, use test_bit() and
clear_bit() just like all other accessors of the state->flag field.
This also eliminates several unnecessary implicit integer type
conversions.
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
fs/nfs/nfs4proc.c | 44 +++++++++++++++++++++++++++++++++++---------
1 files changed, 35 insertions(+), 9 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 1131877..13ae578 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1736,19 +1736,46 @@ static int nfs4_open_expired(struct nfs4_state_owner *sp, struct nfs4_state *sta
#if defined(CONFIG_NFS_V4_1)
/**
- * nfs41_check_expired_stateid - does a state ID need recovery?
+ * nfs41_clear_delegation_stateid - possibly clear revoked delegation state ID
+ *
+ * @state: NFSv4 open state for an inode
+ */
+static void nfs41_clear_delegation_stateid(struct nfs4_state *state)
+{
+ struct nfs_server *server = NFS_SERVER(state->inode);
+ nfs4_stateid *stateid = &state->stateid;
+
+ if (test_bit(NFS_DELEGATED_STATE, &state->flags) != 0)
+ switch (nfs41_test_stateid(server, stateid)) {
+ case NFS_OK:
+ /* delegation still OK, preserve it */
+ break;
+ case -NFS4ERR_ADMIN_REVOKED:
+ case -NFS4ERR_DELEG_REVOKED:
+ /* delegation was revoked, must free */
+ nfs41_free_stateid(server, stateid);
+ default:
+ clear_bit(NFS_DELEGATED_STATE, &state->flags);
+ }
+}
+
+/**
+ * nfs41_check_open_stateid - clear open state ID
*
* @state: NFSv4 open state for an inode
*
* Returns NFS_OK if recovery for this state ID is now finished.
* Otherwise a negative NFS4ERR value is returned.
*/
-static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *stateid, unsigned int flags)
+static int nfs41_check_open_stateid(struct nfs4_state *state)
{
struct nfs_server *server = NFS_SERVER(state->inode);
+ nfs4_stateid *stateid = &state->stateid;
int status = -NFS4ERR_BAD_STATEID;
- if (state->flags & flags) {
+ if ((test_bit(NFS_O_RDONLY_STATE, &state->flags) != 0) ||
+ (test_bit(NFS_O_WRONLY_STATE, &state->flags) != 0) ||
+ (test_bit(NFS_O_RDWR_STATE, &state->flags) != 0)) {
status = nfs41_test_stateid(server, stateid);
switch (status) {
case NFS_OK:
@@ -1760,7 +1787,9 @@ static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *s
nfs41_free_stateid(server, stateid);
default:
/* anything else: just re-open it */
- state->flags &= ~flags;
+ clear_bit(NFS_O_RDONLY_STATE, &state->flags);
+ clear_bit(NFS_O_WRONLY_STATE, &state->flags);
+ clear_bit(NFS_O_RDWR_STATE, &state->flags);
}
}
return status;
@@ -1768,13 +1797,10 @@ static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *s
static int nfs41_open_expired(struct nfs4_state_owner *sp, struct nfs4_state *state)
{
- int deleg_flags = 1 << NFS_DELEGATED_STATE;
- int open_flags = (1 << NFS_O_RDONLY_STATE) | (1 << NFS_O_WRONLY_STATE) | (1 << NFS_O_RDWR_STATE);
int status;
- nfs41_check_expired_stateid(state, &state->stateid, deleg_flags);
- status = nfs41_check_expired_stateid(state, &state->open_stateid,
- open_flags);
+ nfs41_clear_delegation_stateid(state);
+ status = nfs41_check_open_stateid(state);
if (status != NFS_OK)
status = nfs4_open_expired(sp, state);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/4] Take 2: TEST_STATEID fix-ups
2012-06-13 18:20 [PATCH 0/4] Take 2: TEST_STATEID fix-ups Chuck Lever
` (3 preceding siblings ...)
2012-06-13 18:20 ` [PATCH 4/4] NFS: Refactor nfs41_check_expired_stateid() Chuck Lever
@ 2012-06-21 21:04 ` Chuck Lever
4 siblings, 0 replies; 6+ messages in thread
From: Chuck Lever @ 2012-06-21 21:04 UTC (permalink / raw)
To: Trond Myklebust; +Cc: Linux NFS Mailing List
Hi-
Do you think these are ready for nfs-for-3.6 ?
On Jun 13, 2012, at 2:20 PM, Chuck Lever wrote:
> For review.
>
> I discovered a few problems with the new TEST_STATEID implementation
> while testing nograce recovery. Review comments have been addressed,
> and these have been rebased on 3.5-rc2.
>
> ---
>
> Chuck Lever (4):
> NFS: Refactor nfs41_check_expired_stateid()
> NFS: State reclaim clears OPEN and LOCK state
> NFS: Properly sort TEST_STATEID results
> NFS: Clean up TEST_STATEID and FREE_STATEID proc error reporting
>
>
> fs/nfs/nfs4proc.c | 146 ++++++++++++++++++++++++++++++++++++++++++-----------
> 1 files changed, 117 insertions(+), 29 deletions(-)
>
> --
> Chuck Lever
> --
> 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
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-06-21 21:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-06-13 18:20 [PATCH 0/4] Take 2: TEST_STATEID fix-ups Chuck Lever
2012-06-13 18:20 ` [PATCH 1/4] NFS: Clean up TEST_STATEID and FREE_STATEID proc error reporting Chuck Lever
2012-06-13 18:20 ` [PATCH 2/4] NFS: Properly sort TEST_STATEID results Chuck Lever
2012-06-13 18:20 ` [PATCH 3/4] NFS: State reclaim clears OPEN and LOCK state Chuck Lever
2012-06-13 18:20 ` [PATCH 4/4] NFS: Refactor nfs41_check_expired_stateid() Chuck Lever
2012-06-21 21:04 ` [PATCH 0/4] Take 2: TEST_STATEID fix-ups Chuck Lever
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.