NFSD: Fix a bug in the NFSv4 'supported attrs' mandatory attribute

NFSD: Fix a bug in the NFSv4 'supported attrs' mandatory attribute

[Trond Myklebust]

From: Trond Myklebust <Trond.Myklebust@netapp.com>

The fact that the filesystem is present does _not_ imply that the
fs_locations attribute should be marked as "unsupported".

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 fs/nfsd/nfs4xdr.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)


diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 2dcc7fe..1b07183 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1825,8 +1825,6 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
 			goto out_resource;
 		if (!aclsupport)
 			word0 &= ~FATTR4_WORD0_ACL;
-		if (!exp->ex_fslocs.locations)
-			word0 &= ~FATTR4_WORD0_FS_LOCATIONS;
 		if (!word2) {
 			WRITE32(2);
 			WRITE32(word0);

-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: NFSD: Fix a bug in the NFSv4 'supported attrs' mandatory attribute

[J. Bruce Fields]

On Mon, Aug 31, 2009 at 03:16:11PM -0400, Trond Myklebust wrote:
> From: Trond Myklebust <Trond.Myklebust@netapp.com>
> 
> The fact that the filesystem is present does _not_ imply that the
> fs_locations attribute should be marked as "unsupported".

Yes, but the fact that the filesystem is present also does not imply
that exp->ex_fslocs.locations is NULL.  So I'm confused by your
characterization of the existing code.

If you want to do this, you probably also want to turn off the previous
exfslocs.locations check in the same function.  Otherwise we claim
support for the attribute but then never actually return it.

--b.

> 
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
> 
>  fs/nfsd/nfs4xdr.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 2dcc7fe..1b07183 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1825,8 +1825,6 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
>  			goto out_resource;
>  		if (!aclsupport)
>  			word0 &= ~FATTR4_WORD0_ACL;
> -		if (!exp->ex_fslocs.locations)
> -			word0 &= ~FATTR4_WORD0_FS_LOCATIONS;
>  		if (!word2) {
>  			WRITE32(2);
>  			WRITE32(word0);
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer
> 
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com

Re: NFSD: Fix a bug in the NFSv4 'supported attrs' mandatory attribute

[Trond Myklebust]

On Mon, 2009-08-31 at 18:03 -0400, J. Bruce Fields wrote:
> On Mon, Aug 31, 2009 at 03:16:11PM -0400, Trond Myklebust wrote:
> > From: Trond Myklebust <Trond.Myklebust@netapp.com>
> > 
> > The fact that the filesystem is present does _not_ imply that the
> > fs_locations attribute should be marked as "unsupported".
> 
> Yes, but the fact that the filesystem is present also does not imply
> that exp->ex_fslocs.locations is NULL.  So I'm confused by your
> characterization of the existing code.
> 
> If you want to do this, you probably also want to turn off the previous
> exfslocs.locations check in the same function.  Otherwise we claim
> support for the attribute but then never actually return it.

...why is that a problem? Clients that conform to RFC3530 are perfectly
capable of dealing with that case. The point is that if you say "I don't
support this attribute", then the client will _never_ be allowed to
check it.

IOW: if you later decide to add replica information to the /etc/exports
file, then none of the existing clients will be able to check that
information.

Furthermore, you are basically promising the clients that you will never
return NFS4ERR_MOVED on that volume, since you're telling them that you
don't support fs_locations on it.
So you can't ever migrate it...

-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: NFSD: Fix a bug in the NFSv4 'supported attrs' mandatory attribute

[Trond Myklebust]

On Mon, 2009-08-31 at 18:21 -0400, Trond Myklebust wrote:
> On Mon, 2009-08-31 at 18:03 -0400, J. Bruce Fields wrote:
> > If you want to do this, you probably also want to turn off the previous
> > exfslocs.locations check in the same function.  Otherwise we claim
> > support for the attribute but then never actually return it.
> 
> ...why is that a problem? Clients that conform to RFC3530 are perfectly
> capable of dealing with that case. The point is that if you say "I don't
> support this attribute", then the client will _never_ be allowed to
> check it.

Put differently: not setting the fs_locations bit in the supp_attr
information is equivalent to telling the client that it shouldn't ask
for it, and there is no time limit on that prohibition...

-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: NFSD: Fix a bug in the NFSv4 'supported attrs' mandatory attribute

[J. Bruce Fields]

On Mon, Aug 31, 2009 at 06:29:50PM -0400, Trond Myklebust wrote:
> On Mon, 2009-08-31 at 18:21 -0400, Trond Myklebust wrote:
> > On Mon, 2009-08-31 at 18:03 -0400, J. Bruce Fields wrote:
> > > If you want to do this, you probably also want to turn off the previous
> > > exfslocs.locations check in the same function.  Otherwise we claim
> > > support for the attribute but then never actually return it.
> > 
> > ...why is that a problem? Clients that conform to RFC3530 are perfectly
> > capable of dealing with that case. The point is that if you say "I don't
> > support this attribute", then the client will _never_ be allowed to
> > check it.
> 
> Put differently: not setting the fs_locations bit in the supp_attr
> information is equivalent to telling the client that it shouldn't ask
> for it, and there is no time limit on that prohibition...

OK, makes sense.  There's no explicit time limit on the fs_locations
data either, but there's at least some expectation that it could change.

To me it would seem permissible, however, for a client to interpret a
GETATTR reply which turns off a requested FS_LOCATIONS bit as stating a
permanent lack of support for the attribute, rather than necessarily as
stating that the list of fs_locations is currently zero-length.  So why
not make it unambiguous and return a zero-length list?

With that and a corrected comment, we'd have the appended.

--b.

commit dfe14eaccdf05090b802be52ef93764387001331
Author: Trond Myklebust <Trond.Myklebust@netapp.com>
Date:   Mon Aug 31 15:16:11 2009 -0400

    NFSD: Fix a bug in the NFSv4 'supported attrs' mandatory attribute
    
    The fact that the filesystem doesn't currently list any alternate
    locations does _not_ imply that the fs_locations attribute should be
    marked as "unsupported".
    
    Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
    Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index fdf632b..20c5e3d 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1793,11 +1793,6 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
 				goto out_nfserr;
 		}
 	}
-	if (bmval0 & FATTR4_WORD0_FS_LOCATIONS) {
-		if (exp->ex_fslocs.locations == NULL) {
-			bmval0 &= ~FATTR4_WORD0_FS_LOCATIONS;
-		}
-	}
 	if ((buflen -= 16) < 0)
 		goto out_resource;
 
@@ -1825,8 +1820,6 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
 			goto out_resource;
 		if (!aclsupport)
 			word0 &= ~FATTR4_WORD0_ACL;
-		if (!exp->ex_fslocs.locations)
-			word0 &= ~FATTR4_WORD0_FS_LOCATIONS;
 		if (!word2) {
 			WRITE32(2);
 			WRITE32(word0);

Re: NFSD: Fix a bug in the NFSv4 'supported attrs' mandatory attribute

[Trond Myklebust]

On Mon, 2009-08-31 at 18:49 -0400, J. Bruce Fields wrote:
> On Mon, Aug 31, 2009 at 06:29:50PM -0400, Trond Myklebust wrote:
> > On Mon, 2009-08-31 at 18:21 -0400, Trond Myklebust wrote:
> > > On Mon, 2009-08-31 at 18:03 -0400, J. Bruce Fields wrote:
> > > > If you want to do this, you probably also want to turn off the previous
> > > > exfslocs.locations check in the same function.  Otherwise we claim
> > > > support for the attribute but then never actually return it.
> > > 
> > > ...why is that a problem? Clients that conform to RFC3530 are perfectly
> > > capable of dealing with that case. The point is that if you say "I don't
> > > support this attribute", then the client will _never_ be allowed to
> > > check it.
> > 
> > Put differently: not setting the fs_locations bit in the supp_attr
> > information is equivalent to telling the client that it shouldn't ask
> > for it, and there is no time limit on that prohibition...
> 
> OK, makes sense.  There's no explicit time limit on the fs_locations
> data either, but there's at least some expectation that it could change.

Actually, if you read Dave and Carl's 2005 internet draft, you'll see
that they recommend you bump the change attribute whenever fs_locations
changes.
IOW: clients would only be allowed to cache the fs_locations information
while the change attribute remains constant.

> To me it would seem permissible, however, for a client to interpret a
> GETATTR reply which turns off a requested FS_LOCATIONS bit as stating a
> permanent lack of support for the attribute, rather than necessarily as
> stating that the list of fs_locations is currently zero-length.  So why
> not make it unambiguous and return a zero-length list?

That would work.

> With that and a corrected comment, we'd have the appended.
> 
> --b.
> 
> commit dfe14eaccdf05090b802be52ef93764387001331
> Author: Trond Myklebust <Trond.Myklebust@netapp.com>
> Date:   Mon Aug 31 15:16:11 2009 -0400
> 
>     NFSD: Fix a bug in the NFSv4 'supported attrs' mandatory attribute
>     
>     The fact that the filesystem doesn't currently list any alternate
>     locations does _not_ imply that the fs_locations attribute should be
>     marked as "unsupported".
>     
>     Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
>     Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index fdf632b..20c5e3d 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1793,11 +1793,6 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
>  				goto out_nfserr;
>  		}
>  	}
> -	if (bmval0 & FATTR4_WORD0_FS_LOCATIONS) {
> -		if (exp->ex_fslocs.locations == NULL) {
> -			bmval0 &= ~FATTR4_WORD0_FS_LOCATIONS;
> -		}
> -	}
>  	if ((buflen -= 16) < 0)
>  		goto out_resource;
>  
> @@ -1825,8 +1820,6 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
>  			goto out_resource;
>  		if (!aclsupport)
>  			word0 &= ~FATTR4_WORD0_ACL;
> -		if (!exp->ex_fslocs.locations)
> -			word0 &= ~FATTR4_WORD0_FS_LOCATIONS;
>  		if (!word2) {
>  			WRITE32(2);
>  			WRITE32(word0);

-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: NFSD: Fix a bug in the NFSv4 'supported attrs' mandatory attribute

[J. Bruce Fields]

On Mon, Aug 31, 2009 at 07:26:05PM -0400, Trond Myklebust wrote:
> Actually, if you read Dave and Carl's 2005 internet draft, you'll see
> that they recommend you bump the change attribute whenever fs_locations
> changes.
> IOW: clients would only be allowed to cache the fs_locations information
> while the change attribute remains constant.

Which works for pure referrals since the object itself doesn't really
have any other use for the change attribute.  OK, makes sense.

> > To me it would seem permissible, however, for a client to interpret a
> > GETATTR reply which turns off a requested FS_LOCATIONS bit as stating a
> > permanent lack of support for the attribute, rather than necessarily as
> > stating that the list of fs_locations is currently zero-length.  So why
> > not make it unambiguous and return a zero-length list?
> 
> That would work.

Eh, just one more check's slightly incorrect (well, anyway pynfs
complains we're returning the wrong error), so the following is what I'm
commiting--thanks.

--b.

commit a06b1261bdb580b35967d0e055d1ab131b332254
Author: Trond Myklebust <Trond.Myklebust@netapp.com>
Date:   Mon Aug 31 15:16:11 2009 -0400

    NFSD: Fix a bug in the NFSv4 'supported attrs' mandatory attribute
    
    The fact that the filesystem doesn't currently list any alternate
    locations does _not_ imply that the fs_locations attribute should be
    marked as "unsupported".
    
    Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
    Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 6fde431..bebc0c2 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -68,7 +68,6 @@ check_attr_support(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		   u32 *bmval, u32 *writable)
 {
 	struct dentry *dentry = cstate->current_fh.fh_dentry;
-	struct svc_export *exp = cstate->current_fh.fh_export;
 
 	/*
 	 * Check about attributes are supported by the NFSv4 server or not.
@@ -80,17 +79,13 @@ check_attr_support(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		return nfserr_attrnotsupp;
 
 	/*
-	 * Check FATTR4_WORD0_ACL & FATTR4_WORD0_FS_LOCATIONS can be supported
+	 * Check FATTR4_WORD0_ACL can be supported
 	 * in current environment or not.
 	 */
 	if (bmval[0] & FATTR4_WORD0_ACL) {
 		if (!IS_POSIXACL(dentry->d_inode))
 			return nfserr_attrnotsupp;
 	}
-	if (bmval[0] & FATTR4_WORD0_FS_LOCATIONS) {
-		if (exp->ex_fslocs.locations == NULL)
-			return nfserr_attrnotsupp;
-	}
 
 	/*
 	 * According to spec, read-only attributes return ERR_INVAL.
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index fdf632b..20c5e3d 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1793,11 +1793,6 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
 				goto out_nfserr;
 		}
 	}
-	if (bmval0 & FATTR4_WORD0_FS_LOCATIONS) {
-		if (exp->ex_fslocs.locations == NULL) {
-			bmval0 &= ~FATTR4_WORD0_FS_LOCATIONS;
-		}
-	}
 	if ((buflen -= 16) < 0)
 		goto out_resource;
 
@@ -1825,8 +1820,6 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
 			goto out_resource;
 		if (!aclsupport)
 			word0 &= ~FATTR4_WORD0_ACL;
-		if (!exp->ex_fslocs.locations)
-			word0 &= ~FATTR4_WORD0_FS_LOCATIONS;
 		if (!word2) {
 			WRITE32(2);
 			WRITE32(word0);