| Message ID | 20110823214141.GC25350@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.