Re: [PATCH] NFSD: TEST_STATEID should not return NFS4ERR_STALE_STATEID

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, May 24, 2012 at 03:29:51PM -0400, Chuck Lever wrote:
> The error values that TEST_STATEID is allowed to return does not
> include NFS4ERR_STALE_STATEID.  In addition, RFC 5661 says:
> 
> 15.1.16.5.  NFS4ERR_STALE_STATEID (Error Code 10023)
> 
>    A stateid generated by an earlier server instance was used.  This
>    error is moot in NFSv4.1 because all operations that take a stateid
>    MUST be preceded by the SEQUENCE operation, and the earlier server
>    instance is detected by the session infrastructure that supports
>    SEQUENCE.
> 
> I triggered the NFS4ERR_STALE_STATEID during nograce recovery testing.
> My client had updated its boot verifier, so the server instance hadn't
> changed, but the client instance had.  Thus the server allowed the
> SEQUENCE operation, but returned NFS4ERR_STALE_STATEID on the
> TEST_STATEID operation.
> 
> After a client's lease expires, TEST_STATEID should report
> NFS4ERR_EXPIRED for state IDs that the client tries to recover.  I
> don't see a way to make that happen, though.

After the client's lease expires, the SEQUENCE operation will fail.

(Which I believe to be a valid, if unforgiving, server implementation.
If we were to implement "courtesy locks" in this case, I believe we'd
remember the clientid for longer, permit the SEQUENCE, and fail
individual stateid's with EXPIRED as appropriate?)

> Finally, RFC 5661, section 18.48.3 has this:
> 
>  o  Special stateids are always considered invalid (they result in the
>     error code NFS4ERR_BAD_STATEID).

Thanks for the explanation!

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
> Bruce, would you consider taking something like this?

Sure; nits:

>  fs/nfsd/nfs4state.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 9235cfa..ae1fab3 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3365,12 +3365,13 @@ __be32 nfs4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
>  	struct nfs4_ol_stateid *ols;
>  	__be32 status;
>  
> +	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid))
> +		return nfserr_bad_stateid;

Or inval?  This is just a buggy client.

>  	if (STALE_STATEID(stateid))
> -		return nfserr_stale_stateid;
> -
> +		return nfserr_bad_stateid;

Again, this is just a buggy client, since we shouldn't have gotten past
the SEQUENCE in this case unless the client's sending a stateid that's
actually someone else's.

If you think it's worth checking for those buggy client cases, we could
instaed check that stateid->si_opaque.so_clid and cl->clientid agree.
That'd cover the special-stateid checks too.

>  	s = find_stateid(cl, stateid);
>  	if (!s)
> -		 return nfserr_stale_stateid;
> +		 return nfserr_bad_stateid;

So this must be the case you actually hit.  Agreed with this change.


--b.

>  	status = check_stateid_generation(stateid, &s->sc_stateid, 1);
>  	if (status)
>  		return status;
>