[PATCH] nfsd4: fix seqid_mutating_error

[PATCH] nfsd4: fix seqid_mutating_error

[J. Bruce Fields]

Yipes--noticed while doing some unrelated v4 state work.  I'll probably
send this along soonish.

--b.

commit 8eea0f78ed3f3238e238c8ca76e5c7a5c1e7b5f5
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Wed Aug 10 19:16:22 2011 -0400

    nfsd4: fix seqid_mutating_error
    
    The set of errors here does *not* agree with the set of errors specified
    in the rfc!
    
    While we're there, turn this macros into a function, for the usual
    reasons, and move it to the one place where it's actually used.
    
    Cc: stable@kernel.org
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index c8bf405..f810996 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1623,6 +1623,18 @@ static void write_cinfo(__be32 **p, struct nfsd4_change_info *c)
 								\
 	save = resp->p;
 
+static bool seqid_mutating_err(__be32 err)
+{
+	/* rfc 3530 section 8.1.5: */
+	return	err != nfserr_stale_clientid &&
+		err != nfserr_stale_stateid &&
+		err != nfserr_bad_stateid &&
+		err != nfserr_bad_seqid &&
+		err != nfserr_bad_xdr &&
+		err != nfserr_resource &&
+		err != nfserr_nofilehandle;
+}
+
 /*
  * Routine for encoding the result of a "seqid-mutating" NFSv4 operation.  This
  * is where sequence id's are incremented, and the replay cache is filled.
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 4eefaf1..5cfebe5 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -447,12 +447,6 @@ struct nfs4_stateid {
 #define WR_STATE	        0x00000020
 #define CLOSE_STATE             0x00000040
 
-#define seqid_mutating_err(err)                       \
-	(((err) != nfserr_stale_clientid) &&    \
-	((err) != nfserr_bad_seqid) &&          \
-	((err) != nfserr_stale_stateid) &&      \
-	((err) != nfserr_bad_stateid))
-
 struct nfsd4_compound_state;
 
 extern __be32 nfs4_preprocess_stateid_op(struct nfsd4_compound_state *cstate,

Re: [PATCH] nfsd4: fix seqid_mutating_error

[Trond Myklebust]

On Fri, 2011-08-19 at 13:24 -0400, J. Bruce Fields wrote: 
> Yipes--noticed while doing some unrelated v4 state work.  I'll probably
> send this along soonish.
> 
> --b.
> 
> commit 8eea0f78ed3f3238e238c8ca76e5c7a5c1e7b5f5
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Wed Aug 10 19:16:22 2011 -0400
> 
>     nfsd4: fix seqid_mutating_error
>     
>     The set of errors here does *not* agree with the set of errors specified
>     in the rfc!
>     
>     While we're there, turn this macros into a function, for the usual
>     reasons, and move it to the one place where it's actually used.
>     
>     Cc: stable@kernel.org
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index c8bf405..f810996 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1623,6 +1623,18 @@ static void write_cinfo(__be32 **p, struct nfsd4_change_info *c)
>  								\
>  	save = resp->p;
>  
> +static bool seqid_mutating_err(__be32 err)
> +{
> +	/* rfc 3530 section 8.1.5: */
> +	return	err != nfserr_stale_clientid &&
> +		err != nfserr_stale_stateid &&
> +		err != nfserr_bad_stateid &&
> +		err != nfserr_bad_seqid &&
> +		err != nfserr_bad_xdr &&
> +		err != nfserr_resource &&
> +		err != nfserr_nofilehandle;

Any reason not to convert the above into a switch() statement while you
are at it?

Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] nfsd4: fix seqid_mutating_error

[J. Bruce Fields]

On Fri, Aug 19, 2011 at 05:12:53PM -0400, Trond Myklebust wrote:
> On Fri, 2011-08-19 at 13:24 -0400, J. Bruce Fields wrote: 
> > +static bool seqid_mutating_err(__be32 err)
> > +{
> > +	/* rfc 3530 section 8.1.5: */
> > +	return	err != nfserr_stale_clientid &&
> > +		err != nfserr_stale_stateid &&
> > +		err != nfserr_bad_stateid &&
> > +		err != nfserr_bad_seqid &&
> > +		err != nfserr_bad_xdr &&
> > +		err != nfserr_resource &&
> > +		err != nfserr_nofilehandle;
> 
> Any reason not to convert the above into a switch() statement while you
> are at it?

It would be more straightforward without the negatives.

Also, we've got two copies of this list.  How about something like this?

--b.

commit 9ee2cabf7a814d48798380bc2cb8516645d1d0dc
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Tue Aug 23 15:43:04 2011 -0400

    nfsd4: cleanup and consolidate seqid_mutating_err
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index 1ec1a85..1a652a0 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -13,30 +13,6 @@
 
 struct idmap;
 
-/*
- * In a seqid-mutating op, this macro controls which error return
- * values trigger incrementation of the seqid.
- *
- * from rfc 3010:
- * The client MUST monotonically increment the sequence number for the
- * CLOSE, LOCK, LOCKU, OPEN, OPEN_CONFIRM, and OPEN_DOWNGRADE
- * operations.  This is true even in the event that the previous
- * operation that used the sequence number received an error.  The only
- * exception to this rule is if the previous operation received one of
- * the following errors: NFSERR_STALE_CLIENTID, NFSERR_STALE_STATEID,
- * NFSERR_BAD_STATEID, NFSERR_BAD_SEQID, NFSERR_BADXDR,
- * NFSERR_RESOURCE, NFSERR_NOFILEHANDLE.
- *
- */
-#define seqid_mutating_err(err)       \
-(((err) != NFSERR_STALE_CLIENTID) &&  \
- ((err) != NFSERR_STALE_STATEID)  &&  \
- ((err) != NFSERR_BAD_STATEID)    &&  \
- ((err) != NFSERR_BAD_SEQID)      &&  \
- ((err) != NFSERR_BAD_XDR)        &&  \
- ((err) != NFSERR_RESOURCE)       &&  \
- ((err) != NFSERR_NOFILEHANDLE))
-
 enum nfs4_client_state {
 	NFS4CLNT_MANAGER_RUNNING  = 0,
 	NFS4CLNT_CHECK_LEASE,
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 78c792f..04ad9a2 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1623,18 +1623,6 @@ static void write_cinfo(__be32 **p, struct nfsd4_change_info *c)
 								\
 	save = resp->p;
 
-static bool seqid_mutating_err(__be32 err)
-{
-	/* rfc 3530 section 8.1.5: */
-	return	err != nfserr_stale_clientid &&
-		err != nfserr_stale_stateid &&
-		err != nfserr_bad_stateid &&
-		err != nfserr_bad_seqid &&
-		err != nfserr_bad_xdr &&
-		err != nfserr_resource &&
-		err != nfserr_nofilehandle;
-}
-
 /*
  * Routine for encoding the result of a "seqid-mutating" NFSv4 operation.  This
  * is where sequence id's are incremented, and the replay cache is filled.
@@ -1643,7 +1631,7 @@ static bool seqid_mutating_err(__be32 err)
  */
 
 #define ENCODE_SEQID_OP_TAIL(stateowner) do {			\
-	if (seqid_mutating_err(nfserr) && stateowner) { 	\
+	if (seqid_mutating_err(ntohl(nfserr)) && stateowner) { 	\
 		stateowner->so_seqid++;				\
 		stateowner->so_replay.rp_status = nfserr;   	\
 		stateowner->so_replay.rp_buflen = 		\
diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
index 76f99e8..b875b03 100644
--- a/include/linux/nfs4.h
+++ b/include/linux/nfs4.h
@@ -373,6 +373,22 @@ enum nfsstat4 {
 	NFS4ERR_DELEG_REVOKED	= 10087,	/* deleg./layout revoked */
 };
 
+static inline bool seqid_mutating_err(u32 err)
+{
+	/* rfc 3530 section 8.1.5: */
+	switch (err) {
+	case NFS4ERR_STALE_CLIENTID:
+	case NFS4ERR_STALE_STATEID:
+	case NFS4ERR_BAD_STATEID:
+	case NFS4ERR_BAD_SEQID:
+	case NFS4ERR_BADXDR:
+	case NFS4ERR_RESOURCE:
+	case NFS4ERR_NOFILEHANDLE:
+		return false;
+	};
+	return true;
+}
+
 /*
  * Note: NF4BAD is not actually part of the protocol; it is just used
  * internally by nfsd.

RE: [PATCH] nfsd4: fix seqid_mutating_error

[Myklebust, Trond]

> -----Original Message-----
> From: J. Bruce Fields [mailto:bfields@fieldses.org]
> Sent: Tuesday, August 23, 2011 5:42 PM
> To: Myklebust, Trond
> Cc: linux-nfs@vger.kernel.org
> Subject: Re: [PATCH] nfsd4: fix seqid_mutating_error
> 
> On Fri, Aug 19, 2011 at 05:12:53PM -0400, Trond Myklebust wrote:
> > On Fri, 2011-08-19 at 13:24 -0400, J. Bruce Fields wrote:
> > > +static bool seqid_mutating_err(__be32 err)
> > > +{
> > > +	/* rfc 3530 section 8.1.5: */
> > > +	return	err != nfserr_stale_clientid &&
> > > +		err != nfserr_stale_stateid &&
> > > +		err != nfserr_bad_stateid &&
> > > +		err != nfserr_bad_seqid &&
> > > +		err != nfserr_bad_xdr &&
> > > +		err != nfserr_resource &&
> > > +		err != nfserr_nofilehandle;
> >
> > Any reason not to convert the above into a switch() statement while
> you
> > are at it?
> 
> It would be more straightforward without the negatives.
> 
> Also, we've got two copies of this list.  How about something like
> this?
> 
> --b.
> 
> commit 9ee2cabf7a814d48798380bc2cb8516645d1d0dc
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Tue Aug 23 15:43:04 2011 -0400
> 
>     nfsd4: cleanup and consolidate seqid_mutating_err
> 
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
> index 1ec1a85..1a652a0 100644
> --- a/fs/nfs/nfs4_fs.h
> +++ b/fs/nfs/nfs4_fs.h
> @@ -13,30 +13,6 @@
> 
>  struct idmap;
> 
> -/*
> - * In a seqid-mutating op, this macro controls which error return
> - * values trigger incrementation of the seqid.
> - *
> - * from rfc 3010:
> - * The client MUST monotonically increment the sequence number for
the
> - * CLOSE, LOCK, LOCKU, OPEN, OPEN_CONFIRM, and OPEN_DOWNGRADE
> - * operations.  This is true even in the event that the previous
> - * operation that used the sequence number received an error.  The
> only
> - * exception to this rule is if the previous operation received one
of
> - * the following errors: NFSERR_STALE_CLIENTID, NFSERR_STALE_STATEID,
> - * NFSERR_BAD_STATEID, NFSERR_BAD_SEQID, NFSERR_BADXDR,
> - * NFSERR_RESOURCE, NFSERR_NOFILEHANDLE.
> - *
> - */
> -#define seqid_mutating_err(err)       \
> -(((err) != NFSERR_STALE_CLIENTID) &&  \
> - ((err) != NFSERR_STALE_STATEID)  &&  \
> - ((err) != NFSERR_BAD_STATEID)    &&  \
> - ((err) != NFSERR_BAD_SEQID)      &&  \
> - ((err) != NFSERR_BAD_XDR)        &&  \
> - ((err) != NFSERR_RESOURCE)       &&  \
> - ((err) != NFSERR_NOFILEHANDLE))
> -
>  enum nfs4_client_state {
>  	NFS4CLNT_MANAGER_RUNNING  = 0,
>  	NFS4CLNT_CHECK_LEASE,
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 78c792f..04ad9a2 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1623,18 +1623,6 @@ static void write_cinfo(__be32 **p, struct
> nfsd4_change_info *c)
>  								\
>  	save = resp->p;
> 
> -static bool seqid_mutating_err(__be32 err)
> -{
> -	/* rfc 3530 section 8.1.5: */
> -	return	err != nfserr_stale_clientid &&
> -		err != nfserr_stale_stateid &&
> -		err != nfserr_bad_stateid &&
> -		err != nfserr_bad_seqid &&
> -		err != nfserr_bad_xdr &&
> -		err != nfserr_resource &&
> -		err != nfserr_nofilehandle;
> -}
> -
>  /*
>   * Routine for encoding the result of a "seqid-mutating" NFSv4
> operation.  This
>   * is where sequence id's are incremented, and the replay cache is
> filled.
> @@ -1643,7 +1631,7 @@ static bool seqid_mutating_err(__be32 err)
>   */
> 
>  #define ENCODE_SEQID_OP_TAIL(stateowner) do {
\
> -	if (seqid_mutating_err(nfserr) && stateowner) { 	\
> +	if (seqid_mutating_err(ntohl(nfserr)) && stateowner) { 	\
>  		stateowner->so_seqid++;				\
>  		stateowner->so_replay.rp_status = nfserr;   	\
>  		stateowner->so_replay.rp_buflen = 		\
> diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
> index 76f99e8..b875b03 100644
> --- a/include/linux/nfs4.h
> +++ b/include/linux/nfs4.h
> @@ -373,6 +373,22 @@ enum nfsstat4 {
>  	NFS4ERR_DELEG_REVOKED	= 10087,	/* deleg./layout revoked
*/
>  };
> 
> +static inline bool seqid_mutating_err(u32 err)
> +{
> +	/* rfc 3530 section 8.1.5: */
> +	switch (err) {
> +	case NFS4ERR_STALE_CLIENTID:
> +	case NFS4ERR_STALE_STATEID:
> +	case NFS4ERR_BAD_STATEID:
> +	case NFS4ERR_BAD_SEQID:
> +	case NFS4ERR_BADXDR:
> +	case NFS4ERR_RESOURCE:
> +	case NFS4ERR_NOFILEHANDLE:
> +		return false;
> +	};
> +	return true;
> +}
> +
>  /*
>   * Note: NF4BAD is not actually part of the protocol; it is just used
>   * internally by nfsd.

Should we make it take a 's32' rather than a 'u32' argument? We do
usually expect those errors to be signed.

Cheers
  Trond

Re: [PATCH] nfsd4: fix seqid_mutating_error

[J. Bruce Fields]

On Tue, Aug 23, 2011 at 03:58:05PM -0700, Myklebust, Trond wrote:
> > -----Original Message-----
> > From: J. Bruce Fields [mailto:bfields@fieldses.org]
> > diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
> > index 76f99e8..b875b03 100644
> > --- a/include/linux/nfs4.h
> > +++ b/include/linux/nfs4.h
> > @@ -373,6 +373,22 @@ enum nfsstat4 {
> >  	NFS4ERR_DELEG_REVOKED	= 10087,	/* deleg./layout revoked
> */
> >  };
> > 
> > +static inline bool seqid_mutating_err(u32 err)
> > +{
> > +	/* rfc 3530 section 8.1.5: */
> > +	switch (err) {
> > +	case NFS4ERR_STALE_CLIENTID:
> > +	case NFS4ERR_STALE_STATEID:
> > +	case NFS4ERR_BAD_STATEID:
> > +	case NFS4ERR_BAD_SEQID:
> > +	case NFS4ERR_BADXDR:
> > +	case NFS4ERR_RESOURCE:
> > +	case NFS4ERR_NOFILEHANDLE:
> > +		return false;
> > +	};
> > +	return true;
> > +}
> > +
> >  /*
> >   * Note: NF4BAD is not actually part of the protocol; it is just used
> >   * internally by nfsd.
> 
> Should we make it take a 's32' rather than a 'u32' argument? We do
> usually expect those errors to be signed.

On the server side it's unsigned--bigendian in fact.  The caller is:

> > +	if (seqid_mutating_err(ntohl(nfserr)) && stateowner) { 	\

I put this in nfs4.h which is mostly protocol constants, and I figure
u32 is the type closest to the relevant xdr, and we should convert as
necessary in callers.  I dunno.

--b.

Re: [PATCH] nfsd4: fix seqid_mutating_error

[J. Bruce Fields]

On Tue, Aug 23, 2011 at 09:22:48PM -0400, J. Bruce Fields wrote:
> On Tue, Aug 23, 2011 at 03:58:05PM -0700, Myklebust, Trond wrote:
> > > -----Original Message-----
> > > From: J. Bruce Fields [mailto:bfields@fieldses.org]
> > > diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h
> > > index 76f99e8..b875b03 100644
> > > --- a/include/linux/nfs4.h
> > > +++ b/include/linux/nfs4.h
> > > @@ -373,6 +373,22 @@ enum nfsstat4 {
> > >  	NFS4ERR_DELEG_REVOKED	= 10087,	/* deleg./layout revoked
> > */
> > >  };
> > > 
> > > +static inline bool seqid_mutating_err(u32 err)
> > > +{
> > > +	/* rfc 3530 section 8.1.5: */
> > > +	switch (err) {
> > > +	case NFS4ERR_STALE_CLIENTID:
> > > +	case NFS4ERR_STALE_STATEID:
> > > +	case NFS4ERR_BAD_STATEID:
> > > +	case NFS4ERR_BAD_SEQID:
> > > +	case NFS4ERR_BADXDR:
> > > +	case NFS4ERR_RESOURCE:
> > > +	case NFS4ERR_NOFILEHANDLE:
> > > +		return false;
> > > +	};
> > > +	return true;
> > > +}
> > > +
> > >  /*
> > >   * Note: NF4BAD is not actually part of the protocol; it is just used
> > >   * internally by nfsd.
> > 
> > Should we make it take a 's32' rather than a 'u32' argument? We do
> > usually expect those errors to be signed.
> 
> On the server side it's unsigned--bigendian in fact.  The caller is:
> 
> > > +	if (seqid_mutating_err(ntohl(nfserr)) && stateowner) { 	\
> 
> I put this in nfs4.h which is mostly protocol constants, and I figure
> u32 is the type closest to the relevant xdr, and we should convert as
> necessary in callers.  I dunno.

(Translation: "I dunno" == "I have no strong opinion, but I'm sticking
with this patch unless you want to write another".)

--b.