why NFS4ERR_BAD_STATEID but not NFS4ERR_EXPIRED

[J. Bruce Fields]

On Fri, Mar 27, 2009 at 05:58:35PM +0800, Bian Naimeng wrote:
> > It's almost good, but you'll have wraparound problem when the wall clock
> > time overflows. I'd suggest doing the comparisons as signed differences
> > in order to avoid that.
> > IOW:
> > 
> >   if (long)si_boot - (long)server_boot < 0
> > 	return NFS4ERR_STALE_STATEID
> >   else if (long)server_boot - (long)si_boot <= 0 &&
> >             (long)si_boot + (long)lease_time - (long)now <= 0
> > 	return NFS4ERR_EXPIRED
> >   else
> > 	return NFS4ERR_BAD_STATEID
> > 
> > Cheers
> >   Trond
> > 
>   
>   Thanks Trond. The new patch is attached.

Note units are seconds, not jiffies, so wraparound isn't a problem for
32-bit machines until 2038.  Do we really care?

> 
> Regard
>  Bian
> 
> -----
> 
>  Server should distinguish the expired, stale, and bad stateid.
> 
>  Signed-off-by: Bian Naimeng <biannm at cn.fujitsu.com>
> 
>  ---
>  fs/nfsd/nfs4state.c |   51 +++++++++++++++++++++++++++++++++++++++++----------
>  1 files changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b6f60f4..e52c212 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -216,7 +216,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_stateid *stp, struct svc_f
>  	dp->dl_recall.cbr_dp = NULL;
>  	dp->dl_recall.cbr_ident = cb->cb_ident;
>  	dp->dl_recall.cbr_trunc = 0;
> -	dp->dl_stateid.si_boot = boot_time;
> +	dp->dl_stateid.si_boot = get_seconds();
>  	dp->dl_stateid.si_stateownerid = current_delegid++;
>  	dp->dl_stateid.si_fileid = 0;
>  	dp->dl_stateid.si_generation = 0;
> @@ -1094,7 +1094,7 @@ init_stateid(struct nfs4_stateid *stp, struct nfs4_file *fp, struct nfsd4_open *
>  	stp->st_stateowner = sop;
>  	get_nfs4_file(fp);
>  	stp->st_file = fp;
> -	stp->st_stateid.si_boot = boot_time;
> +	stp->st_stateid.si_boot = get_seconds();
>  	stp->st_stateid.si_stateownerid = sop->so_id;
>  	stp->st_stateid.si_fileid = fp->fi_id;
>  	stp->st_stateid.si_generation = 0;
> @@ -1943,12 +1943,41 @@ nfs4_check_fh(struct svc_fh *fhp, struct nfs4_stateid *stp)
>  static int
>  STALE_STATEID(stateid_t *stateid)
>  {
> -	if (stateid->si_boot == boot_time)
> -		return 0;
> -	dprintk("NFSD: stale stateid (%08x/%08x/%08x/%08x)!¥n",

Something weird happened to the encoding here; I see a yen sign instead
of a backslash at the end there.

> -		stateid->si_boot, stateid->si_stateownerid, stateid->si_fileid,
> -		stateid->si_generation);
> -	return 1;
> +	if ((long)(stateid->si_boot) - (long)boot_time < 0) {
> +		dprintk("NFSD: stale stateid (%08x/%08x/%08x/%08x)!¥n",
> +			stateid->si_boot, stateid->si_stateownerid, stateid->si_fileid,
> +			stateid->si_generation);
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +static int
> +EXPIRED_STATEID(stateid_t *stateid)
> +{
> +	if((long)boot_time - (long)(stateid->si_boot) <=0 &&
> +	   ((long)(stateid->si_boot) + (long)lease_time - (long)get_seconds()) <= 0) {

Defining some equivalent to time_after/time_before would clarify the
logic.

> +		dprintk("NFSD: expired stateid(%08x/%08x/%08x/%08x)!¥n",
> +			stateid->si_boot, stateid->si_stateownerid, stateid->si_fileid,
> +			stateid->si_generation);
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +static __be32
> +stateid_error_map(stateid_t *stateid)
> +{
> +	if(STALE_STATEID(stateid))

Here and elsewhere: make that "if (", not "if(".  Run
linux/scripts/checkpatch.pl on this and fix anything it complains about
that doesn't seem unreasonable.

--b.

> +		return nfserr_stale_stateid;
> +	else if(EXPIRED_STATEID(stateid))
> +		return nfserr_expired;
> +	else {
> +		dprintk("NFSD: bad stateid (%08x/%08x/%08x/%08x)!¥n",
> +			stateid->si_boot, stateid->si_stateownerid, stateid->si_fileid,
> +			stateid->si_generation);
> +		return nfserr_bad_stateid;
> +	}
>  }
>  
>  static inline int
> @@ -2065,12 +2094,14 @@ nfs4_preprocess_stateid_op(struct svc_fh *current_fh, stateid_t *stateid, int fl
>  	if (!stateid->si_fileid) { /* delegation stateid */
>  		if(!(dp = find_delegation_stateid(ino, stateid))) {
>  			dprintk("NFSD: delegation stateid not found¥n");
> +			status = stateid_error_map(stateid);
>  			goto out;
>  		}
>  		stidp = &dp->dl_stateid;
>  	} else { /* open or lock stateid */
>  		if (!(stp = find_stateid(stateid, flags))) {
>  			dprintk("NFSD: open or lock stateid not found¥n");
> +			status = stateid_error_map(stateid);
>  			goto out;
>  		}
>  		if ((flags & CHECK_FH) && nfs4_check_fh(current_fh, stp))
> @@ -2147,7 +2178,7 @@ nfs4_preprocess_seqid_op(struct svc_fh *current_fh, u32 seqid, stateid_t *statei
>  		 */
>  		sop = search_close_lru(stateid->si_stateownerid, flags);
>  		if (sop == NULL)
> -			return nfserr_bad_stateid;
> +			return stateid_error_map(stateid);
>  		*sopp = sop;
>  		goto check_replay;
>  	}
> @@ -2621,7 +2652,7 @@ alloc_init_lock_stateid(struct nfs4_stateowner *sop, struct nfs4_file *fp, struc
>  	stp->st_stateowner = sop;
>  	get_nfs4_file(fp);
>  	stp->st_file = fp;
> -	stp->st_stateid.si_boot = boot_time;
> +	stp->st_stateid.si_boot = get_seconds();
>  	stp->st_stateid.si_stateownerid = sop->so_id;
>  	stp->st_stateid.si_fileid = fp->fi_id;
>  	stp->st_stateid.si_generation = 0;
> -- 
> 1.6.0.3
> 
> 
>