Hi-
I'm trying to understand the error recovery logic in the new TEST_STATEID and FREE_STATEID procedures.
For TEST_STATEID, we have this:
6395 static int nfs41_test_stateid(struct nfs_server *server, nfs4_stateid *stateid)
6396 {
6397 struct nfs4_exception exception = { };
6398 int err;
6399 do {
6400 err = nfs4_handle_exception(server,
6401 _nfs41_test_stateid(server, stateid),
6402 &exception);
6403 } while (exception.retry);
6404 return err;
6405 }
According to RFC 5661, the TEST_STATEID and FREE_STATEID procedures can return NFS4ERR_BAD_STATEID, NFS4ERR_OLD_STATEID, and NFS4ERR_DEADSESSION, among other things. Do you really want to enter the exception handler here? Seems to me that nfs41_{test,free}_stateid() are invoked mainly (only?) by the state manager, and thus you don't want to kick the state manager here and wait, as that would deadlock. About the only error code you might want to pass into nfs4_handle_exception() is NFS4ERR_DELAY. Everything else probably ought to be returned outright to the caller to let her figure out how to recover.
Also, _nfs41_test_stateid() does this:
6390 if (status == NFS_OK)
6391 return res.status;
6392 return status;
status will contain NFS4_OK or a negative NFS4ERR value. But the "if / return" will return res.status, which could be NFS4_OK, but it could also be (according to RFC 5661 section 18.48.3) NFS4ERR_BAD_STATEID, NFS4ERR_OLD_STATEID, NFS4ERR_EXPIRED, NFS4ERR_ADMIN_REVOKED, or NFS4ERR_DELEG_REVOKED. Note that these are positive values, and the above logic returns them directly to the caller.
The caller then passes the positive result to nfs4_handle_exception(). Now I think nfs4_handle_exception() will ignore positive values. And, I don't see any caller who is not doing "if (status != NFS4_OK)" so maybe this doesn't matter. But it sure is confusing.
Do you remember what was intended? Was it positive NFS4ERR values for res.status and negative NFS4ERR values if the operation failed? If that's the case, then that intention should be carefully documented. Otherwise, maybe that should read "return -res.status;" (as long as we aren't passing that to the exception handler!).
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
