64 bit ino support for NFS server

Hi.

Attached is a patch to modify the NFS server code to support 64 bit ino's, as appropriate for the system and the NFS protocol version.

The gist of the changes is to query the underlying file system for attributes and not just to use the cached attributes in the inode. For this specific purpose, the inode only contains an ino field which unsigned long, which is large enough on 64 bit platforms, but is not large enough on 32 bit platforms.

fs/nfsd/nfs3xdr.c renamed from fs/nfsd/nfs3xdr.c.org +0 -11

 	/* Attributes to follow */
 	*p++ = xdr_one;
 
-	*p++ = htonl(nfs3_ftypes[(fhp->fh_post_mode & S_IFMT) >> 12]);
-	*p++ = htonl((u32) fhp->fh_post_mode);
-	*p++ = htonl((u32) fhp->fh_post_nlink);
-	*p++ = htonl((u32) nfsd_ruid(rqstp, fhp->fh_post_uid));
-	*p++ = htonl((u32) nfsd_rgid(rqstp, fhp->fh_post_gid));
-	if (S_ISLNK(fhp->fh_post_mode) && fhp->fh_post_size > NFS3_MAXPATHLEN) {
-		p = xdr_encode_hyper(p, (u64) NFS3_MAXPATHLEN);
-	} else {
-		p = xdr_encode_hyper(p, (u64) fhp->fh_post_size);
-	}
-	p = xdr_encode_hyper(p, ((u64)fhp->fh_post_blocks) << 9);

Quoted 6 lines · Peter Staubach

Hi.

Attached is a patch to modify the NFS server code to support 64 bit ino's, as appropriate for the system and the NFS protocol version.

Quoted 6 lines

The gist of the changes is to query the underlying file system for attributes and not just to use the cached attributes in the inode. For this specific purpose, the inode only contains an ino field which unsigned long, which is large enough on 64 bit platforms, but is not large enough on 32 bit platforms.

Thanks!

fs/nfsd/nfs3xdr.c encode_fattr3(struct svc_rqst *rqstp, __ · line 210

Quoted 10 lines

fs/nfsd/nfs3xdr.c renamed from fs/nfsd/nfs3xdr.c.org +4 -1

@@ -203,31 +203,15 @@ encode_fattr3(struct svc_rqst *rqstp, __
 static __be32 *
 encode_saved_post_attr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp)
 {
-	struct inode	*inode = fhp->fh_dentry->d_inode;
+	if (!fhp->fh_post_saved) {
+		*p++ = xdr_zero;
+		return p;
+	}
 

The caller, encode_wcc_data(), already did this check.

Quoted 21 lines

fs/nfsd/nfs3xdr.c renamed from fs/nfsd/nfs3xdr.c.org +0 -18

 	/* Attributes to follow */
 	*p++ = xdr_one;
 
-	*p++ = htonl(nfs3_ftypes[(fhp->fh_post_mode & S_IFMT) >> 12]);
-	*p++ = htonl((u32) fhp->fh_post_mode);
-	*p++ = htonl((u32) fhp->fh_post_nlink);
-	*p++ = htonl((u32) nfsd_ruid(rqstp, fhp->fh_post_uid));
-	*p++ = htonl((u32) nfsd_rgid(rqstp, fhp->fh_post_gid));
-	if (S_ISLNK(fhp->fh_post_mode) && fhp->fh_post_size > NFS3_MAXPATHLEN) {
-		p = xdr_encode_hyper(p, (u64) NFS3_MAXPATHLEN);
-	} else {
-		p = xdr_encode_hyper(p, (u64) fhp->fh_post_size);
-	}
-	p = xdr_encode_hyper(p, ((u64)fhp->fh_post_blocks) << 9);
-	*p++ = fhp->fh_post_rdev[0];
-	*p++ = fhp->fh_post_rdev[1];
-	p = encode_fsid(p, fhp);
-	p = xdr_encode_hyper(p, (u64) inode->i_ino);
-	p = encode_time3(p, &fhp->fh_post_atime);
-	p = encode_time3(p, &fhp->fh_post_mtime);
-	p = encode_time3(p, &fhp->fh_post_ctime);

fs/nfsd/nfs3xdr.c encode_fattr3(struct svc_rqst *rqstp, __ · line 216

Quoted 4 lines

fs/nfsd/nfs3xdr.c renamed from fs/nfsd/nfs3xdr.c.org +1 -2

-
-	return p;
+	return encode_fattr3(rqstp, p, fhp, &fhp->fh_post_attr);
 

Is there a problem with the lease_get_mtime() call in encode_fattr3()? It looks like that could return the current time rather than the time that was supposedly atomic with respect to the operation.

Dumb question: I assume it's always legal to call ->getattr while holding the i_mutex?

Quoted 14 lines · J. Bruce Fields

fs/nfsd/nfs3xdr.c encode_fattr3(struct svc_rqst *rqstp, __ · line 211

Quoted 11 lines · Peter Staubach

fs/nfsd/nfs3xdr.c renamed from fs/nfsd/nfs3xdr.c.org +4 -1

@@ -203,31 +203,15 @@ encode_fattr3(struct svc_rqst *rqstp, __
 static __be32 *
 encode_saved_post_attr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh 
 *fhp)
 {
-	struct inode	*inode = fhp->fh_dentry->d_inode;
+	if (!fhp->fh_post_saved) {
+		*p++ = xdr_zero;
+		return p;
+	}
 

The caller, encode_wcc_data(), already did this check.

True. What a twisty maze of small passages, all alike.

The current algorithms don't look quite right to me. I don't think that it is valid to return a post_op_attr, which is not atomic w.r.t. the operation, even when not returning a pre_op_attr. Perhaps a little more cleanup might be good. I will look into this.

Quoted 3 lines

Dumb question: I assume it's always legal to call ->getattr while holding the i_mutex?

Not so dumb, and I couldn't find an answer to that, either way. I couldn't find any reason why it would be illegal, but neither did I find explicit reasons for why it would be legal.

Does anyone else know? This gets into the lack of complete definition for the virtual file system layer...

Quoted 6 lines · Peter Staubach

Hi.

Attached is a patch to modify the NFS server code to support 64 bit ino's, as appropriate for the system and the NFS protocol version.

Quoted 6 lines

The gist of the changes is to query the underlying file system for attributes and not just to use the cached attributes in the inode. For this specific purpose, the inode only contains an ino field which unsigned long, which is large enough on 64 bit platforms, but is not large enough on 32 bit platforms.

Thanks!

fs/nfsd/nfs3xdr.c encode_fattr3(struct svc_rqst *rqstp, __ · line 210

Quoted 10 lines

fs/nfsd/nfs3xdr.c renamed from fs/nfsd/nfs3xdr.c.org +4 -1

@@ -203,31 +203,15 @@ encode_fattr3(struct svc_rqst *rqstp, __
 static __be32 *
 encode_saved_post_attr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp)
 {
-	struct inode	*inode = fhp->fh_dentry->d_inode;
+	if (!fhp->fh_post_saved) {
+		*p++ = xdr_zero;
+		return p;
+	}
 

The caller, encode_wcc_data(), already did this check.

Quoted 21 lines

fs/nfsd/nfs3xdr.c renamed from fs/nfsd/nfs3xdr.c.org +0 -18

 	/* Attributes to follow */
 	*p++ = xdr_one;
 
-	*p++ = htonl(nfs3_ftypes[(fhp->fh_post_mode & S_IFMT) >> 12]);
-	*p++ = htonl((u32) fhp->fh_post_mode);
-	*p++ = htonl((u32) fhp->fh_post_nlink);
-	*p++ = htonl((u32) nfsd_ruid(rqstp, fhp->fh_post_uid));
-	*p++ = htonl((u32) nfsd_rgid(rqstp, fhp->fh_post_gid));
-	if (S_ISLNK(fhp->fh_post_mode) && fhp->fh_post_size > NFS3_MAXPATHLEN) {
-		p = xdr_encode_hyper(p, (u64) NFS3_MAXPATHLEN);
-	} else {
-		p = xdr_encode_hyper(p, (u64) fhp->fh_post_size);
-	}
-	p = xdr_encode_hyper(p, ((u64)fhp->fh_post_blocks) << 9);
-	*p++ = fhp->fh_post_rdev[0];
-	*p++ = fhp->fh_post_rdev[1];
-	p = encode_fsid(p, fhp);
-	p = xdr_encode_hyper(p, (u64) inode->i_ino);
-	p = encode_time3(p, &fhp->fh_post_atime);
-	p = encode_time3(p, &fhp->fh_post_mtime);
-	p = encode_time3(p, &fhp->fh_post_ctime);

fs/nfsd/nfs3xdr.c encode_fattr3(struct svc_rqst *rqstp, __ · line 216

Quoted 4 lines

fs/nfsd/nfs3xdr.c renamed from fs/nfsd/nfs3xdr.c.org +1 -2

-
-	return p;
+	return encode_fattr3(rqstp, p, fhp, &fhp->fh_post_attr);
 

Is there a problem with the lease_get_mtime() call in encode_fattr3()? It looks like that could return the current time rather than the time that was supposedly atomic with respect to the operation.

Dumb question: I assume it's always legal to call ->getattr while holding the i_mutex?

--b.

I'm testing a new version of the patch, which addresses this issue, but then I got to wondering, what is lease_get_mtime() really for and is it solving a real problem, and if so, is it solving it in a reasonable fashion?

It appears to me that it is used to detect when an application on the server has acquired an exclusive lease for a file and may be modifying it, but without informing the kernel. If it was informing the kernel, then the mtime would be updated.

NFS doesn't support leases like this, so it would need to be an application running on the server, either as a standalone application or as a server for some other service.

And, why does it matter whether the attributes are being returned via GETATTR, via a post_op_attr, or via a post_op_attr as part of a wcc_data? The first two cases invoke lease_get_mtime(), while the third does not.

Could this not lead to time jumping around on a particular file, forwards and backwards?

Quoted 5 lines · Peter Staubach

I'm testing a new version of the patch, which addresses this issue, but then I got to wondering, what is lease_get_mtime() really for and is it solving a real problem, and if so, is it solving it in a reasonable fashion?

Quoted 9 lines

It appears to me that it is used to detect when an application on the server has acquired an exclusive lease for a file and may be modifying it, but without informing the kernel. If it was informing the kernel, then the mtime would be updated.

NFS doesn't support leases like this, so it would need to be an application running on the server, either as a standalone application or as a server for some other service.

Let's say it's Samba, since that's probably what is is.

So Samba is holding a write lease on the file on behalf of one of its clients. We never see an open from our NFSv2/v3 client--if it doesn't have data cached, we may never even see a read, just access and getattr--so we don't know to break that lease.

(Actually, if a write lease, like an NFSv4 write delegation, is meant to cover file attributes as well as data, then perhaps stat should break write leases. That sounds painful.)

Anyway, the delay between the time the Samba client makes a change and the time the NFS client sees it could be unbounded. Whereas with lease_get_mtime() the client will invalidate its cache and perform a read, which *does* break the lease (in nfsd_open).

But I agree that this is a strange compromise, and I'd like to understand better what the semantics for sharing with a Samba client are supposed to be.

Quoted 5 lines

And, why does it matter whether the attributes are being returned via GETATTR, via a post_op_attr, or via a post_op_attr as part of a wcc_data? The first two cases invoke lease_get_mtime(), while the third does not.

What's the code path in the third case?

Quoted 3 lines

Could this not lead to time jumping around on a particular file, forwards and backwards?

Yeah, sounds bad.

--b.

Quoted 14 lines · J. Bruce Fields

So Samba is holding a write lease on the file on behalf of one of its clients. We never see an open from our NFSv2/v3 client--if it doesn't have data cached, we may never even see a read, just access and getattr--so we don't know to break that lease.

(Actually, if a write lease, like an NFSv4 write delegation, is meant to cover file attributes as well as data, then perhaps stat should break write leases. That sounds painful.)

Anyway, the delay between the time the Samba client makes a change and the time the NFS client sees it could be unbounded. Whereas with lease_get_mtime() the client will invalidate its cache and perform a read, which *does* break the lease (in nfsd_open).

Quoted 3 lines

But I agree that this is a strange compromise, and I'd like to understand better what the semantics for sharing with a Samba client are supposed to be.

Yes.

Quoted 6 lines · Peter Staubach

Hi.

Attached is a patch to modify the NFS server code to support 64 bit ino's, as appropriate for the system and the NFS protocol version.

Quoted 6 lines

The gist of the changes is to query the underlying file system for attributes and not just to use the cached attributes in the inode. For this specific purpose, the inode only contains an ino field which unsigned long, which is large enough on 64 bit platforms, but is not large enough on 32 bit platforms.

Thanks!

Review encode_fattr3(struct svc_rqst *rqstp, __ · line 210

Quoted 10 lines

(quoted diff) +4 -1

@@ -203,31 +203,15 @@ encode_fattr3(struct svc_rqst *rqstp, __
 static __be32 *
 encode_saved_post_attr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp)
 {
-	struct inode	*inode = fhp->fh_dentry->d_inode;
+	if (!fhp->fh_post_saved) {
+		*p++ = xdr_zero;
+		return p;
+	}
 

The caller, encode_wcc_data(), already did this check.

Quoted 15 lines

(quoted diff) +0 -12

 	/* Attributes to follow */
 	*p++ = xdr_one;
 
-	*p++ = htonl(nfs3_ftypes[(fhp->fh_post_mode & S_IFMT) >> 12]);
-	*p++ = htonl((u32) fhp->fh_post_mode);
-	*p++ = htonl((u32) fhp->fh_post_nlink);
-	*p++ = htonl((u32) nfsd_ruid(rqstp, fhp->fh_post_uid));
-	*p++ = htonl((u32) nfsd_rgid(rqstp, fhp->fh_post_gid));
-	if (S_ISLNK(fhp->fh_post_mode) && fhp->fh_post_size > NFS3_MAXPATHLEN) {
-		p = xdr_encode_hyper(p, (u64) NFS3_MAXPATHLEN);
-	} else {
-		p = xdr_encode_hyper(p, (u64) fhp->fh_post_size);
-	}
-	p = xdr_encode_hyper(p, ((u64)fhp->fh_post_blocks) << 9);
-	*p++ = fhp->fh_post_rdev[0];

Review encode_fattr3(struct svc_rqst *rqstp, __ · line 215

Quoted 10 lines

(quoted diff) +1 -8

-	*p++ = fhp->fh_post_rdev[1];
-	p = encode_fsid(p, fhp);
-	p = xdr_encode_hyper(p, (u64) inode->i_ino);
-	p = encode_time3(p, &fhp->fh_post_atime);
-	p = encode_time3(p, &fhp->fh_post_mtime);
-	p = encode_time3(p, &fhp->fh_post_ctime);
-
-	return p;
+	return encode_fattr3(rqstp, p, fhp, &fhp->fh_post_attr);
 

Is there a problem with the lease_get_mtime() call in encode_fattr3()? It looks like that could return the current time rather than the time that was supposedly atomic with respect to the operation.

Dumb question: I assume it's always legal to call ->getattr while holding the i_mutex?

Attached is a new patch which should address the issues raised by Bruce.

I also haven't come to any conclusions regarding the value of lease_get_mtime() and whether it should or should not be invoked by fill_post_wcc() too. I chose not to change this because I thought that it was safer to leave well enough alone. If we decide to make a change, it can be done separately.

Quoted 6 lines · J. Bruce Fields

- This would be better as two, maybe three separate patches; e.g. moving the lease_get_mtime out of encode_fattr3 could be done separately first. Ideally we'd do some trivial transformations like that, followed by one change that actually changes the inode behavior. That makes the whole thing trival to review.

My test system isn't at a place where I could factor the patch and do testing. For the time being, I'd prefer to stick with the single patch, since it is tested.

Quoted 3 lines · Peter Staubach

Attached is a new patch which should address the issues raised by Bruce.

Thanks!

Quoted 6 lines

I also haven't come to any conclusions regarding the value of lease_get_mtime() and whether it should or should not be invoked by fill_post_wcc() too. I chose not to change this because I thought that it was safer to leave well enough alone. If we decide to make a change, it can be done separately.

OK.

Only superficial complaints:
- There were some minor whitespace oddities; running the patch through scripts/checkpatch.pl may be the quickest way to catch those.

- This would be better as two, maybe three separate patches; e.g. moving the lease_get_mtime out of encode_fattr3 could be done separately first. Ideally we'd do some trivial transformations like that, followed by one change that actually changes the inode behavior. That makes the whole thing trival to review.

I fixed up the first and added the result to

git://linux-nfs.org/~bfields/linux.git for-mm

so it should show up in the next -mm. I'd happily replace it by a more finely split up version if that was something you could whip up in a few minutes.