[PATCH, RFC] nfsd: fix nfsd4_cb_recall_done error handling

[Christoph Hellwig]

The error handling for CB_RECALL seems fairly broken to me.

What looks good:

- for EBADHANDLE and NFS4ERR_BAD_STATEID retry until dl_retries
hits zero, then mark the connection down and set cb_done

What looks wrong:

- for everything else we first mark the connection down, then
retry until dl_retries hits zero, then mark the connection down
again and set cb_done.
- keep the behavior for EBADHANDLE and NFS4ERR_BAD_STATEID,
otherwise jump straight to making the connection down
and setting cb_done

But maybe I'm missing something?


diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 17d5441..ed25c58 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -971,24 +971,21 @@ static void nfsd4_cb_recall_done(struct rpc_task *task, void *calldata)
return;
switch (task->tk_status) {
case 0:
- cb->cb_done = true;
- return;
+ break;
case -EBADHANDLE:
case -NFS4ERR_BAD_STATEID:
/* Race: client probably got cb_recall
* before open reply granting delegation */
- break;
+ if (dp->dl_retries--) {
+ rpc_delay(task, 2*HZ);
+ task->tk_status = 0;
+ rpc_restart_call_prepare(task);
+ return;
+ }
default:
/* Network partition? */
nfsd4_mark_cb_down(clp, task->tk_status);
}
- if (dp->dl_retries--) {
- rpc_delay(task, 2*HZ);
- task->tk_status = 0;
- rpc_restart_call_prepare(task);
- return;
- }
- nfsd4_mark_cb_down(clp, task->tk_status);
cb->cb_done = true;
}

[Trond Myklebust]

Post by Christoph Hellwig
The error handling for CB_RECALL seems fairly broken to me.
- for EBADHANDLE and NFS4ERR_BAD_STATEID retry until dl_retries
hits zero, then mark the connection down and set cb_done
- for everything else we first mark the connection down, then
retry until dl_retries hits zero, then mark the connection down
again and set cb_done.
- keep the behavior for EBADHANDLE and NFS4ERR_BAD_STATEID,
otherwise jump straight to making the connection down
and setting cb_done
But maybe I'm missing something?
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 17d5441..ed25c58 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -971,24 +971,21 @@ static void nfsd4_cb_recall_done(struct rpc_task *task, void *calldata)
return;
switch (task->tk_status) {
- cb->cb_done = true;
- return;
+ break;
/* Race: client probably got cb_recall
* before open reply granting delegation */
- break;
+ if (dp->dl_retries--) {
+ rpc_delay(task, 2*HZ);
+ task->tk_status = 0;
+ rpc_restart_call_prepare(task);
+ return;
+ }
/* Network partition? */
nfsd4_mark_cb_down(clp, task->tk_status);
}
- if (dp->dl_retries--) {
- rpc_delay(task, 2*HZ);
- task->tk_status = 0;
- rpc_restart_call_prepare(task);
- return;
- }
- nfsd4_mark_cb_down(clp, task->tk_status);
cb->cb_done = true;
}

We're also missing a handler for NFS4ERR_DELAY, which is listed as a
legal response to CB_RECALL in both RFC5661 and RFC3530bis. As far as
I can tell from the above, knfsd will currently take that to be a sign
it should mark the callback path as being down...
--
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust-7I+n7zu2hftEKMMhf/***@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Christoph Hellwig]

Post by Trond Myklebust
We're also missing a handler for NFS4ERR_DELAY, which is listed as a
legal response to CB_RECALL in both RFC5661 and RFC3530bis. As far as
I can tell from the above, knfsd will currently take that to be a sign
it should mark the callback path as being down...

Yes. I've got a fix of that further down in my queue with the pnfs
patches, just wanted to set this bit out first.

I plan to handle NFS4ERR_DELAY in the generic callback layer instead of
burderning it onto the individual callback implementations.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[J. Bruce Fields]

Post by Christoph Hellwig
The error handling for CB_RECALL seems fairly broken to me.
- for EBADHANDLE and NFS4ERR_BAD_STATEID retry until dl_retries
hits zero, then mark the connection down and set cb_done
- for everything else we first mark the connection down, then
retry until dl_retries hits zero, then mark the connection down
again and set cb_done.
- keep the behavior for EBADHANDLE and NFS4ERR_BAD_STATEID,
otherwise jump straight to making the connection down
and setting cb_done
But maybe I'm missing something?

I can't think of anything; let me know when you want something applied.

--b.

Post by Christoph Hellwig
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 17d5441..ed25c58 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -971,24 +971,21 @@ static void nfsd4_cb_recall_done(struct rpc_task *task, void *calldata)
return;
switch (task->tk_status) {
- cb->cb_done = true;
- return;
+ break;
/* Race: client probably got cb_recall
* before open reply granting delegation */
- break;
+ if (dp->dl_retries--) {
+ rpc_delay(task, 2*HZ);
+ task->tk_status = 0;
+ rpc_restart_call_prepare(task);
+ return;
+ }
/* Network partition? */
nfsd4_mark_cb_down(clp, task->tk_status);
}
- if (dp->dl_retries--) {
- rpc_delay(task, 2*HZ);
- task->tk_status = 0;
- rpc_restart_call_prepare(task);
- return;
- }
- nfsd4_mark_cb_down(clp, task->tk_status);
cb->cb_done = true;
}
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html