enhanced ESTALE error handling

PATCH In review Peter Staubach 2 participants 5 messages Jan 18, 2008 – Mar 10, 2008

Subsystems (3)

VFS NFS client NFS server

lore

Share & tools

lore

Peter Staubach

author

2008-01-18 15:36 UTC #0 lore raw

PATCH 3/3 enhanced ESTALE error handling

Hi.

The patch enhanced the ESTALE error handling for NFS mounted file systems. It expands the number of places that the NFS client checks for ESTALE returns from the server.

It also enhances the ESTALE handling for directories by occasionally retrying revalidation to check to see whether the directory becomes valid again. This sounds odd, but can occur when a systems administrator, accidently or unknowingly, unexports a file system which is in use. All active non-directory files become permanently inaccessible, but directories can be become accessible again after the administrator re-exports the file system. This is a situation that users have been complaining about for years and this support can help to alleviate their situations.

Thanx...

Signed-off-by Peter Staubach <[email protected]>

fs/nfs/inode.c renamed from fs/nfs/inode.c.org +37 ~~-25~~

@@ -192,7 +192,7 @@ void nfs_invalidate_atime(struct inode *
  */
 static void nfs_invalidate_inode(struct inode *inode)
 {
-	set_bit(NFS_INO_STALE, &NFS_FLAGS(inode));
+	nfs_handle_estale(-ESTALE, inode);
 	nfs_zap_caches_locked(inode);
 }

@@ -385,6 +385,8 @@ nfs_setattr(struct dentry *dentry, struc
 	error = NFS_PROTO(inode)->setattr(dentry, &fattr, attr);
 	if (error == 0)
 		nfs_refresh_inode(inode, &fattr);
+	else
+		nfs_handle_estale(error, inode);
 	unlock_kernel();
 	return error;
 }

@@ -629,7 +631,7 @@ int nfs_release(struct inode *inode, str
 int
 __nfs_revalidate_inode(struct nfs_server *server, struct inode *inode)
 {
-	int		 status = -ESTALE;
+	int status = -ESTALE;
 	struct nfs_fattr fattr;
 	struct nfs_inode *nfsi = NFS_I(inode);

@@ -640,15 +642,25 @@ __nfs_revalidate_inode(struct nfs_server
 	lock_kernel();
 	if (is_bad_inode(inode))
  		goto out_nowait;
-	if (NFS_STALE(inode))
+	if (NFS_STALE(inode) && !S_ISDIR(inode->i_mode))
  		goto out_nowait;
 
 	status = nfs_wait_on_inode(inode);
 	if (status < 0)
 		goto out;
 
+	/*
+	 * Do we believe that the file handle is still stale?
+	 * For non-directories, once stale, always stale.
+	 * For directories, believe the stale status for the
+	 * attribute cache timeout period, and then try again.
+	 * This will help to address the problem of the server
+	 * admin "accidently" unexporting a file system without
+	 * stopping the NFS server first.
+	 */
 	status = -ESTALE;
-	if (NFS_STALE(inode))
+	if (NFS_STALE(inode) &&
+	    (!S_ISDIR(inode->i_mode) || !nfs_attribute_timeout(inode)))
 		goto out;
 
 	status = NFS_PROTO(inode)->getattr(server, NFS_FH(inode), &fattr);

@@ -656,11 +668,9 @@ __nfs_revalidate_inode(struct nfs_server
 		dfprintk(PAGECACHE, "nfs_revalidate_inode: (%s/%Ld) getattr failed, error=%d\n",
 			 inode->i_sb->s_id,
 			 (long long)NFS_FILEID(inode), status);
-		if (status == -ESTALE) {
+		nfs_handle_estale(status, inode);
+		if (status == -ESTALE)
 			nfs_zap_caches(inode);
-			if (!S_ISDIR(inode->i_mode))
-				set_bit(NFS_INO_STALE, &NFS_FLAGS(inode));
-		}
 		goto out;
 	}

@@ -986,14 +996,28 @@ static int nfs_update_inode(struct inode
 			__FUNCTION__, inode->i_sb->s_id, inode->i_ino,
 			atomic_read(&inode->i_count), fattr->valid);
 
-	if (nfsi->fileid != fattr->fileid)
-		goto out_fileid;
+	if (nfsi->fileid != fattr->fileid) {
+		printk(KERN_ERR "NFS: server %s error: fileid changed\n"
+			"fsid %s: expected fileid 0x%Lx, got 0x%Lx\n",
+			NFS_SERVER(inode)->nfs_client->cl_hostname,
+			inode->i_sb->s_id,
+			(long long)nfsi->fileid, (long long)fattr->fileid);
+		goto out_err;
+	}
 
 	/*
 	 * Make sure the inode's type hasn't changed.
 	 */
-	if ((inode->i_mode & S_IFMT) != (fattr->mode & S_IFMT))
-		goto out_changed;
+	if ((inode->i_mode & S_IFMT) != (fattr->mode & S_IFMT)) {
+		/*
+		 * Big trouble! The inode has become a different object.
+		 */
+		printk(KERN_DEBUG "%s: inode %ld mode changed, %07o to %07o\n",
+			__FUNCTION__, inode->i_ino, inode->i_mode, fattr->mode);
+		goto out_err;
+	}
+
+	nfs_clear_estale(inode);
 
 	server = NFS_SERVER(inode);
 	/* Update the fsid? */

@@ -1099,12 +1123,7 @@ static int nfs_update_inode(struct inode
 	nfsi->cache_validity &= ~NFS_INO_REVAL_FORCED;
 
 	return 0;
- out_changed:
-	/*
-	 * Big trouble! The inode has become a different object.
-	 */
-	printk(KERN_DEBUG "%s: inode %ld mode changed, %07o to %07o\n",
-			__FUNCTION__, inode->i_ino, inode->i_mode, fattr->mode);
+
  out_err:
 	/*
 	 * No need to worry about unhashing the dentry, as the

@@ -1113,13 +1132,6 @@ static int nfs_update_inode(struct inode
 	 */
 	nfs_invalidate_inode(inode);
 	return -ESTALE;
-
- out_fileid:
-	printk(KERN_ERR "NFS: server %s error: fileid changed\n"
-		"fsid %s: expected fileid 0x%Lx, got 0x%Lx\n",
-		NFS_SERVER(inode)->nfs_client->cl_hostname, inode->i_sb->s_id,
-		(long long)nfsi->fileid, (long long)fattr->fileid);
-	goto out_err;
 }

fs/nfs/dir.c renamed from fs/nfs/dir.c.org +36 ~~-13~~

159 lines not on this page · raw message

fs/nfs/nfs4proc.c renamed from fs/nfs/nfs4proc.c.org +8 -2

@@ -3627,6 +3627,7 @@ int nfs4_setxattr(struct dentry *dentry,
 		size_t buflen, int flags)
 {
 	struct inode *inode = dentry->d_inode;
+	int error;
 
 	if (strcmp(key, XATTR_NAME_NFSV4_ACL) != 0)
 		return -EOPNOTSUPP;

@@ -3635,7 +3636,9 @@ int nfs4_setxattr(struct dentry *dentry,
 	    (!S_ISDIR(inode->i_mode) || inode->i_mode & S_ISVTX))
 		return -EPERM;
 
-	return nfs4_proc_set_acl(inode, buf, buflen);
+	error = nfs4_proc_set_acl(inode, buf, buflen);
+	nfs_handle_estale(error, inode);
+	return error;
 }
 
 /* The getxattr man page suggests returning -ENODATA for unknown attributes,

@@ -3646,11 +3649,14 @@ ssize_t nfs4_getxattr(struct dentry *den
 		size_t buflen)
 {
 	struct inode *inode = dentry->d_inode;
+	int error;
 
 	if (strcmp(key, XATTR_NAME_NFSV4_ACL) != 0)
 		return -EOPNOTSUPP;
 
-	return nfs4_proc_get_acl(inode, buf, buflen);
+	error = nfs4_proc_get_acl(inode, buf, buflen);
+	nfs_handle_estale(error, inode);
+	return error;
 }
 
 ssize_t nfs4_listxattr(struct dentry *dentry, char *buf, size_t buflen)

fs/nfs/namespace.c renamed from fs/nfs/namespace.c.org +1 -0

@@ -115,6 +115,7 @@ static void * nfs_follow_mountpoint(stru
 	err = server->nfs_client->rpc_ops->lookup(parent->d_inode,
 						  &nd->dentry->d_name,
 						  &fh, &fattr);
+	nfs_handle_estale(err, parent->d_inode);
 	dput(parent);
 	if (err != 0)
 		goto out_err;

fs/nfs/symlink.c renamed from fs/nfs/symlink.c.org +1 -0

@@ -40,6 +40,7 @@ static int nfs_symlink_filler(struct ino
 	return 0;
 
 error:
+	nfs_handle_estale(error, inode);
 	SetPageError(page);
 	unlock_page(page);
 	return -EIO;

fs/nfs/nfs3acl.c renamed from fs/nfs/nfs3acl.c.org +22 -6

@@ -22,8 +22,10 @@ ssize_t nfs3_listxattr(struct dentry *de
 		} while(0)
 
 	acl = nfs3_proc_getacl(inode, ACL_TYPE_ACCESS);
-	if (IS_ERR(acl))
+	if (IS_ERR(acl)) {
+		nfs_handle_estale(PTR_ERR(acl), inode);
 		return PTR_ERR(acl);
+	}
 	if (acl) {
 		output("system.posix_acl_access");
 		posix_acl_release(acl);

@@ -31,8 +33,10 @@ ssize_t nfs3_listxattr(struct dentry *de
 
 	if (S_ISDIR(inode->i_mode)) {
 		acl = nfs3_proc_getacl(inode, ACL_TYPE_DEFAULT);
-		if (IS_ERR(acl))
+		if (IS_ERR(acl)) {
+			nfs_handle_estale(PTR_ERR(acl), inode);
 			return PTR_ERR(acl);
+		}
 		if (acl) {
 			output("system.posix_acl_default");
 			posix_acl_release(acl);

@@ -61,9 +65,11 @@ ssize_t nfs3_getxattr(struct dentry *den
 		return -EOPNOTSUPP;
 
 	acl = nfs3_proc_getacl(inode, type);
-	if (IS_ERR(acl))
+	if (IS_ERR(acl)) {
+		nfs_handle_estale(PTR_ERR(acl), inode);
 		return PTR_ERR(acl);
-	else if (acl) {
+	}
+	if (acl) {
 		if (type == ACL_TYPE_ACCESS && acl->a_count == 0)
 			error = -ENODATA;
 		else

@@ -92,6 +98,7 @@ int nfs3_setxattr(struct dentry *dentry,
 	acl = posix_acl_from_xattr(value, size);
 	if (IS_ERR(acl))
 		return PTR_ERR(acl);
+
 	error = nfs3_proc_setacl(inode, type, acl);
 	posix_acl_release(acl);

@@ -355,16 +362,22 @@ int nfs3_proc_setacl(struct inode *inode
 			case ACL_TYPE_ACCESS:
 				alloc = dfacl = nfs3_proc_getacl(inode,
 						ACL_TYPE_DEFAULT);
-				if (IS_ERR(alloc))
+				if (IS_ERR(alloc)) {
+					nfs_handle_estale(PTR_ERR(alloc),
+							inode);
 					goto fail;
+				}
 				break;
 
 			case ACL_TYPE_DEFAULT:
 				dfacl = acl;
 				alloc = acl = nfs3_proc_getacl(inode,
 						ACL_TYPE_ACCESS);
-				if (IS_ERR(alloc))
+				if (IS_ERR(alloc)) {
+					nfs_handle_estale(PTR_ERR(alloc),
+							inode);
 					goto fail;
+				}
 				break;
 
 			default:

@@ -380,6 +393,7 @@ int nfs3_proc_setacl(struct inode *inode
 	}
 	status = nfs3_proc_setacls(inode, acl, dfacl);
 	posix_acl_release(alloc);
+	nfs_handle_estale(status, inode);
 	return status;
 
 fail:

@@ -395,6 +409,7 @@ int nfs3_proc_set_default_acl(struct ino
 	dfacl = nfs3_proc_getacl(dir, ACL_TYPE_DEFAULT);
 	if (IS_ERR(dfacl)) {
 		error = PTR_ERR(dfacl);
+		nfs_handle_estale(error, dir);
 		return (error == -EOPNOTSUPP) ? 0 : error;
 	}
 	if (!dfacl)

@@ -408,6 +423,7 @@ int nfs3_proc_set_default_acl(struct ino
 		goto out_release_acl;
 	error = nfs3_proc_setacls(inode, acl, S_ISDIR(inode->i_mode) ?
 						      dfacl : NULL);
+	nfs_handle_estale(error, inode);
 out_release_acl:
 	posix_acl_release(acl);
 out_release_dfacl:

include/linux/nfs_fs.h renamed from include/linux/nfs_fs.h.org +25 -0

@@ -219,6 +219,31 @@ static inline struct nfs_inode *NFS_I(st
 
 #define NFS_FILEID(inode)		(NFS_I(inode)->fileid)
 
+static inline void nfs_handle_estale(int error, struct inode *inode)
+{
+	struct nfs_inode *nfsi = NFS_I(inode);
+
+	if (error == -ESTALE) {
+		if (!NFS_STALE(inode))
+			set_bit(NFS_INO_STALE, &NFS_FLAGS(inode));
+		/*
+		 * If a directory is being marked as stale, then
+		 * reset the attribute cache timeout.  This controls
+		 * the duration that the stale condition is believed
+		 * before being verified again with another call to
+		 * the server.
+		 */
+		if (S_ISDIR(inode->i_mode))
+			nfsi->read_cache_jiffies = jiffies;
+	}
+}
+
+static inline void nfs_clear_estale(struct inode *inode)
+{
+	if (NFS_STALE(inode))
+		clear_bit(NFS_INO_STALE, &NFS_FLAGS(inode));
+}
+
 static inline void nfs_mark_for_revalidate(struct inode *inode)
 {
 	struct nfs_inode *nfsi = NFS_I(inode);

Peter Staubach

author

2008-02-01 20:58 UTC #1 lore raw

PATCH 3/3 enhanced NFS ESTALE error handling (v2)

Hi.

The patch enhanced the ESTALE error handling for NFS mounted file systems. It expands the number of places that the NFS client checks for ESTALE returns from the server.

Thanx...

Signed-off-by Peter Staubach <[email protected]>

fs/nfs/inode.c renamed from fs/nfs/inode.c.org +37 ~~-25~~

@@ -192,7 +192,7 @@ void nfs_invalidate_atime(struct inode *
  */
 static void nfs_invalidate_inode(struct inode *inode)
 {
-	set_bit(NFS_INO_STALE, &NFS_FLAGS(inode));
+	nfs_handle_estale(-ESTALE, inode);
 	nfs_zap_caches_locked(inode);
 }

@@ -385,6 +385,8 @@ nfs_setattr(struct dentry *dentry, struc
 	error = NFS_PROTO(inode)->setattr(dentry, &fattr, attr);
 	if (error == 0)
 		nfs_refresh_inode(inode, &fattr);
+	else
+		nfs_handle_estale(error, inode);
 	unlock_kernel();
 	return error;
 }

@@ -629,7 +631,7 @@ int nfs_release(struct inode *inode, str
 int
 __nfs_revalidate_inode(struct nfs_server *server, struct inode *inode)
 {
-	int		 status = -ESTALE;
+	int status = -ESTALE;
 	struct nfs_fattr fattr;
 	struct nfs_inode *nfsi = NFS_I(inode);

@@ -640,15 +642,25 @@ __nfs_revalidate_inode(struct nfs_server
 	lock_kernel();
 	if (is_bad_inode(inode))
  		goto out_nowait;
-	if (NFS_STALE(inode))
+	if (NFS_STALE(inode) && !S_ISDIR(inode->i_mode))
  		goto out_nowait;
 
 	status = nfs_wait_on_inode(inode);
 	if (status < 0)
 		goto out;
 
+	/*
+	 * Do we believe that the file handle is still stale?
+	 * For non-directories, once stale, always stale.
+	 * For directories, believe the stale status for the
+	 * attribute cache timeout period, and then try again.
+	 * This will help to address the problem of the server
+	 * admin "accidently" unexporting a file system without
+	 * stopping the NFS server first.
+	 */
 	status = -ESTALE;
-	if (NFS_STALE(inode))
+	if (NFS_STALE(inode) &&
+	    (!S_ISDIR(inode->i_mode) || !nfs_attribute_timeout(inode)))
 		goto out;
 
 	status = NFS_PROTO(inode)->getattr(server, NFS_FH(inode), &fattr);

@@ -656,11 +668,9 @@ __nfs_revalidate_inode(struct nfs_server
 		dfprintk(PAGECACHE, "nfs_revalidate_inode: (%s/%Ld) getattr failed, error=%d\n",
 			 inode->i_sb->s_id,
 			 (long long)NFS_FILEID(inode), status);
-		if (status == -ESTALE) {
+		nfs_handle_estale(status, inode);
+		if (status == -ESTALE)
 			nfs_zap_caches(inode);
-			if (!S_ISDIR(inode->i_mode))
-				set_bit(NFS_INO_STALE, &NFS_FLAGS(inode));
-		}
 		goto out;
 	}

@@ -986,14 +996,28 @@ static int nfs_update_inode(struct inode
 			__FUNCTION__, inode->i_sb->s_id, inode->i_ino,
 			atomic_read(&inode->i_count), fattr->valid);
 
-	if (nfsi->fileid != fattr->fileid)
-		goto out_fileid;
+	if (nfsi->fileid != fattr->fileid) {
+		printk(KERN_ERR "NFS: server %s error: fileid changed\n"
+			"fsid %s: expected fileid 0x%Lx, got 0x%Lx\n",
+			NFS_SERVER(inode)->nfs_client->cl_hostname,
+			inode->i_sb->s_id,
+			(long long)nfsi->fileid, (long long)fattr->fileid);
+		goto out_err;
+	}
 
 	/*
 	 * Make sure the inode's type hasn't changed.
 	 */
-	if ((inode->i_mode & S_IFMT) != (fattr->mode & S_IFMT))
-		goto out_changed;
+	if ((inode->i_mode & S_IFMT) != (fattr->mode & S_IFMT)) {
+		/*
+		 * Big trouble! The inode has become a different object.
+		 */
+		printk(KERN_DEBUG "%s: inode %ld mode changed, %07o to %07o\n",
+			__FUNCTION__, inode->i_ino, inode->i_mode, fattr->mode);
+		goto out_err;
+	}
+
+	nfs_clear_estale(inode);
 
 	server = NFS_SERVER(inode);
 	/* Update the fsid? */

@@ -1099,12 +1123,7 @@ static int nfs_update_inode(struct inode
 	nfsi->cache_validity &= ~NFS_INO_REVAL_FORCED;
 
 	return 0;
- out_changed:
-	/*
-	 * Big trouble! The inode has become a different object.
-	 */
-	printk(KERN_DEBUG "%s: inode %ld mode changed, %07o to %07o\n",
-			__FUNCTION__, inode->i_ino, inode->i_mode, fattr->mode);
+
  out_err:
 	/*
 	 * No need to worry about unhashing the dentry, as the

@@ -1113,13 +1132,6 @@ static int nfs_update_inode(struct inode
 	 */
 	nfs_invalidate_inode(inode);
 	return -ESTALE;
-
- out_fileid:
-	printk(KERN_ERR "NFS: server %s error: fileid changed\n"
-		"fsid %s: expected fileid 0x%Lx, got 0x%Lx\n",
-		NFS_SERVER(inode)->nfs_client->cl_hostname, inode->i_sb->s_id,
-		(long long)nfsi->fileid, (long long)fattr->fileid);
-	goto out_err;
 }

fs/nfs/dir.c renamed from fs/nfs/dir.c.org +36 ~~-13~~

159 lines not on this page · raw message

fs/nfs/nfs4proc.c renamed from fs/nfs/nfs4proc.c.org +8 -2

@@ -3627,6 +3627,7 @@ int nfs4_setxattr(struct dentry *dentry,
 		size_t buflen, int flags)
 {
 	struct inode *inode = dentry->d_inode;
+	int error;
 
 	if (strcmp(key, XATTR_NAME_NFSV4_ACL) != 0)
 		return -EOPNOTSUPP;

@@ -3635,7 +3636,9 @@ int nfs4_setxattr(struct dentry *dentry,
 	    (!S_ISDIR(inode->i_mode) || inode->i_mode & S_ISVTX))
 		return -EPERM;
 
-	return nfs4_proc_set_acl(inode, buf, buflen);
+	error = nfs4_proc_set_acl(inode, buf, buflen);
+	nfs_handle_estale(error, inode);
+	return error;
 }
 
 /* The getxattr man page suggests returning -ENODATA for unknown attributes,

@@ -3646,11 +3649,14 @@ ssize_t nfs4_getxattr(struct dentry *den
 		size_t buflen)
 {
 	struct inode *inode = dentry->d_inode;
+	int error;
 
 	if (strcmp(key, XATTR_NAME_NFSV4_ACL) != 0)
 		return -EOPNOTSUPP;
 
-	return nfs4_proc_get_acl(inode, buf, buflen);
+	error = nfs4_proc_get_acl(inode, buf, buflen);
+	nfs_handle_estale(error, inode);
+	return error;
 }
 
 ssize_t nfs4_listxattr(struct dentry *dentry, char *buf, size_t buflen)

fs/nfs/namespace.c renamed from fs/nfs/namespace.c.org +1 -0

@@ -115,6 +115,7 @@ static void * nfs_follow_mountpoint(stru
 	err = server->nfs_client->rpc_ops->lookup(parent->d_inode,
 						  &nd->dentry->d_name,
 						  &fh, &fattr);
+	nfs_handle_estale(err, parent->d_inode);
 	dput(parent);
 	if (err != 0)
 		goto out_err;

fs/nfs/symlink.c renamed from fs/nfs/symlink.c.org +1 -0

@@ -40,6 +40,7 @@ static int nfs_symlink_filler(struct ino
 	return 0;
 
 error:
+	nfs_handle_estale(error, inode);
 	SetPageError(page);
 	unlock_page(page);
 	return -EIO;

fs/nfs/nfs3acl.c renamed from fs/nfs/nfs3acl.c.org +22 -6

@@ -22,8 +22,10 @@ ssize_t nfs3_listxattr(struct dentry *de
 		} while(0)
 
 	acl = nfs3_proc_getacl(inode, ACL_TYPE_ACCESS);
-	if (IS_ERR(acl))
+	if (IS_ERR(acl)) {
+		nfs_handle_estale(PTR_ERR(acl), inode);
 		return PTR_ERR(acl);
+	}
 	if (acl) {
 		output("system.posix_acl_access");
 		posix_acl_release(acl);

@@ -31,8 +33,10 @@ ssize_t nfs3_listxattr(struct dentry *de
 
 	if (S_ISDIR(inode->i_mode)) {
 		acl = nfs3_proc_getacl(inode, ACL_TYPE_DEFAULT);
-		if (IS_ERR(acl))
+		if (IS_ERR(acl)) {
+			nfs_handle_estale(PTR_ERR(acl), inode);
 			return PTR_ERR(acl);
+		}
 		if (acl) {
 			output("system.posix_acl_default");
 			posix_acl_release(acl);

@@ -61,9 +65,11 @@ ssize_t nfs3_getxattr(struct dentry *den
 		return -EOPNOTSUPP;
 
 	acl = nfs3_proc_getacl(inode, type);
-	if (IS_ERR(acl))
+	if (IS_ERR(acl)) {
+		nfs_handle_estale(PTR_ERR(acl), inode);
 		return PTR_ERR(acl);
-	else if (acl) {
+	}
+	if (acl) {
 		if (type == ACL_TYPE_ACCESS && acl->a_count == 0)
 			error = -ENODATA;
 		else

@@ -92,6 +98,7 @@ int nfs3_setxattr(struct dentry *dentry,
 	acl = posix_acl_from_xattr(value, size);
 	if (IS_ERR(acl))
 		return PTR_ERR(acl);
+
 	error = nfs3_proc_setacl(inode, type, acl);
 	posix_acl_release(acl);

@@ -355,16 +362,22 @@ int nfs3_proc_setacl(struct inode *inode
 			case ACL_TYPE_ACCESS:
 				alloc = dfacl = nfs3_proc_getacl(inode,
 						ACL_TYPE_DEFAULT);
-				if (IS_ERR(alloc))
+				if (IS_ERR(alloc)) {
+					nfs_handle_estale(PTR_ERR(alloc),
+							inode);
 					goto fail;
+				}
 				break;
 
 			case ACL_TYPE_DEFAULT:
 				dfacl = acl;
 				alloc = acl = nfs3_proc_getacl(inode,
 						ACL_TYPE_ACCESS);
-				if (IS_ERR(alloc))
+				if (IS_ERR(alloc)) {
+					nfs_handle_estale(PTR_ERR(alloc),
+							inode);
 					goto fail;
+				}
 				break;
 
 			default:

@@ -380,6 +393,7 @@ int nfs3_proc_setacl(struct inode *inode
 	}
 	status = nfs3_proc_setacls(inode, acl, dfacl);
 	posix_acl_release(alloc);
+	nfs_handle_estale(status, inode);
 	return status;
 
 fail:

@@ -395,6 +409,7 @@ int nfs3_proc_set_default_acl(struct ino
 	dfacl = nfs3_proc_getacl(dir, ACL_TYPE_DEFAULT);
 	if (IS_ERR(dfacl)) {
 		error = PTR_ERR(dfacl);
+		nfs_handle_estale(error, dir);
 		return (error == -EOPNOTSUPP) ? 0 : error;
 	}
 	if (!dfacl)

@@ -408,6 +423,7 @@ int nfs3_proc_set_default_acl(struct ino
 		goto out_release_acl;
 	error = nfs3_proc_setacls(inode, acl, S_ISDIR(inode->i_mode) ?
 						      dfacl : NULL);
+	nfs_handle_estale(error, inode);
 out_release_acl:
 	posix_acl_release(acl);
 out_release_dfacl:

include/linux/nfs_fs.h renamed from include/linux/nfs_fs.h.org +25 -0

@@ -219,6 +219,31 @@ static inline struct nfs_inode *NFS_I(st
 
 #define NFS_FILEID(inode)		(NFS_I(inode)->fileid)
 
+static inline void nfs_handle_estale(int error, struct inode *inode)
+{
+	struct nfs_inode *nfsi = NFS_I(inode);
+
+	if (error == -ESTALE) {
+		if (!NFS_STALE(inode))
+			set_bit(NFS_INO_STALE, &NFS_FLAGS(inode));
+		/*
+		 * If a directory is being marked as stale, then
+		 * reset the attribute cache timeout.  This controls
+		 * the duration that the stale condition is believed
+		 * before being verified again with another call to
+		 * the server.
+		 */
+		if (S_ISDIR(inode->i_mode))
+			nfsi->read_cache_jiffies = jiffies;
+	}
+}
+
+static inline void nfs_clear_estale(struct inode *inode)
+{
+	if (NFS_STALE(inode))
+		clear_bit(NFS_INO_STALE, &NFS_FLAGS(inode));
+}
+
 static inline void nfs_mark_for_revalidate(struct inode *inode)
 {
 	struct nfs_inode *nfsi = NFS_I(inode);

Trond Myklebust

2008-02-01 21:06 UTC #2 lore raw

Re: [PATCH 3/3] enhanced NFS ESTALE error handling (v2)

Quoted 6 lines · Peter Staubach

Hi.
The patch enhanced the ESTALE error handling for NFS mounted file systems. It expands the number of places that the NFS client checks for ESTALE returns from the server.

Quoted 10 lines

It also enhances the ESTALE handling for directories by occasionally retrying revalidation to check to see whether the directory becomes valid again. This sounds odd, but can occur when a systems administrator, accidently or unknowingly, unexports a file system which is in use. All active non-directory files become permanently inaccessible, but directories can be become accessible again after the administrator re-exports the file system. This is a situation that users have been complaining about for years and this support can help to alleviate their situations.

As far as I can see, this patch can be applied separately from the VFS fixes. If so, would it make sense for me to take charge of this patch in the NFS tree, while Andrew queues up the other two VFS changes in -mm?

Cheers
Trond

Peter Staubach

author

2008-02-01 21:29 UTC #3 lore raw

Re: [PATCH 3/3] enhanced NFS ESTALE error handling (v2)

Quoted 20 lines · Trond Myklebust

Quoted 6 lines · Peter Staubach
Hi.
The patch enhanced the ESTALE error handling for NFS mounted file systems. It expands the number of places that the NFS client checks for ESTALE returns from the server.
Quoted 11 lines
It also enhances the ESTALE handling for directories by occasionally retrying revalidation to check to see whether the directory becomes valid again. This sounds odd, but can occur when a systems administrator, accidently or unknowingly, unexports a file system which is in use. All active non-directory files become permanently inaccessible, but directories can be become accessible again after the administrator re-exports the file system. This is a situation that users have been complaining about for years and this support can help to alleviate their situations.

Quoted 3 lines

As far as I can see, this patch can be applied separately from the VFS fixes. If so, would it make sense for me to take charge of this patch in the NFS tree, while Andrew queues up the other two VFS changes in -mm?

Yes, I think that this would make good sense.

Thanx...

-
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Peter Staubach

author

2008-03-10 20:23 UTC #4 lore raw

PATCH 3/3 enhanced NFS ESTALE error handling (v3)

In reply to Peter Staubach

Hi.

The patch enhanced the ESTALE error handling for NFS mounted file systems. It expands the number of places that the NFS client checks for ESTALE returns from the server.

Thanx...

Signed-off-by Peter Staubach <[email protected]>

fs/nfs/inode.c renamed from fs/nfs/inode.c.org +36 ~~-23~~

@@ -192,7 +192,7 @@ void nfs_invalidate_atime(struct inode *
  */
 static void nfs_invalidate_inode(struct inode *inode)
 {
-	set_bit(NFS_INO_STALE, &NFS_I(inode)->flags);
+	nfs_handle_estale(-ESTALE, inode);
 	nfs_zap_caches_locked(inode);
 }

@@ -386,6 +386,8 @@ nfs_setattr(struct dentry *dentry, struc
 	error = NFS_PROTO(inode)->setattr(dentry, &fattr, attr);
 	if (error == 0)
 		nfs_refresh_inode(inode, &fattr);
+	else
+		nfs_handle_estale(error, inode);
 	unlock_kernel();
 	return error;
 }

@@ -635,7 +637,7 @@ int nfs_release(struct inode *inode, str
 int
 __nfs_revalidate_inode(struct nfs_server *server, struct inode *inode)
 {
-	int		 status = -ESTALE;
+	int status = -ESTALE;
 	struct nfs_fattr fattr;
 	struct nfs_inode *nfsi = NFS_I(inode);

@@ -646,15 +648,25 @@ __nfs_revalidate_inode(struct nfs_server
 	lock_kernel();
 	if (is_bad_inode(inode))
  		goto out_nowait;
-	if (NFS_STALE(inode))
+	if (NFS_STALE(inode) && !S_ISDIR(inode->i_mode))
  		goto out_nowait;
 
 	status = nfs_wait_on_inode(inode);
 	if (status < 0)
 		goto out;
 
+	/*
+	 * Do we believe that the file handle is still stale?
+	 * For non-directories, once stale, always stale.
+	 * For directories, believe the stale status for the
+	 * attribute cache timeout period, and then try again.
+	 * This will help to address the problem of the server
+	 * admin "accidently" unexporting a file system without
+	 * stopping the NFS server first.
+	 */
 	status = -ESTALE;
-	if (NFS_STALE(inode))
+	if (NFS_STALE(inode) &&
+	    (!S_ISDIR(inode->i_mode) || !nfs_attribute_timeout(inode)))
 		goto out;
 
 	status = NFS_PROTO(inode)->getattr(server, NFS_FH(inode), &fattr);

@@ -663,9 +675,8 @@ __nfs_revalidate_inode(struct nfs_server
 			 inode->i_sb->s_id,
 			 (long long)NFS_FILEID(inode), status);
 		if (status == -ESTALE) {
+			nfs_handle_estale(status, inode);
 			nfs_zap_caches(inode);
-			if (!S_ISDIR(inode->i_mode))
-				set_bit(NFS_INO_STALE, &NFS_I(inode)->flags);
 		}
 		goto out;
 	}

@@ -993,14 +1004,28 @@ static int nfs_update_inode(struct inode
 			__FUNCTION__, inode->i_sb->s_id, inode->i_ino,
 			atomic_read(&inode->i_count), fattr->valid);
 
-	if (nfsi->fileid != fattr->fileid)
-		goto out_fileid;
+	if (nfsi->fileid != fattr->fileid) {
+		printk(KERN_ERR "NFS: server %s error: fileid changed\n"
+			"fsid %s: expected fileid 0x%Lx, got 0x%Lx\n",
+			NFS_SERVER(inode)->nfs_client->cl_hostname,
+			inode->i_sb->s_id,
+			(long long)nfsi->fileid, (long long)fattr->fileid);
+		goto out_err;
+	}
 
 	/*
 	 * Make sure the inode's type hasn't changed.
 	 */
-	if ((inode->i_mode & S_IFMT) != (fattr->mode & S_IFMT))
-		goto out_changed;
+	if ((inode->i_mode & S_IFMT) != (fattr->mode & S_IFMT)) {
+		/*
+		 * Big trouble! The inode has become a different object.
+		 */
+		printk(KERN_DEBUG "%s: inode %ld mode changed, %07o to %07o\n",
+			__FUNCTION__, inode->i_ino, inode->i_mode, fattr->mode);
+		goto out_err;
+	}
+
+	nfs_clear_estale(inode);
 
 	server = NFS_SERVER(inode);
 	/* Update the fsid? */

@@ -1109,12 +1134,7 @@ static int nfs_update_inode(struct inode
 	nfsi->cache_validity &= ~NFS_INO_REVAL_FORCED;
 
 	return 0;
- out_changed:
-	/*
-	 * Big trouble! The inode has become a different object.
-	 */
-	printk(KERN_DEBUG "%s: inode %ld mode changed, %07o to %07o\n",
-			__FUNCTION__, inode->i_ino, inode->i_mode, fattr->mode);
+
  out_err:
 	/*
 	 * No need to worry about unhashing the dentry, as the

@@ -1123,13 +1143,6 @@ static int nfs_update_inode(struct inode
 	 */
 	nfs_invalidate_inode(inode);
 	return -ESTALE;
-
- out_fileid:
-	printk(KERN_ERR "NFS: server %s error: fileid changed\n"
-		"fsid %s: expected fileid 0x%Lx, got 0x%Lx\n",
-		NFS_SERVER(inode)->nfs_client->cl_hostname, inode->i_sb->s_id,
-		(long long)nfsi->fileid, (long long)fattr->fileid);
-	goto out_err;
 }

fs/nfs/dir.c renamed from fs/nfs/dir.c.org +38 ~~-13~~

161 lines not on this page · raw message

fs/nfs/nfs4proc.c renamed from fs/nfs/nfs4proc.c.org +8 -2

@@ -3604,11 +3604,14 @@ int nfs4_setxattr(struct dentry *dentry,
 		size_t buflen, int flags)
 {
 	struct inode *inode = dentry->d_inode;
+	int error;
 
 	if (strcmp(key, XATTR_NAME_NFSV4_ACL) != 0)
 		return -EOPNOTSUPP;
 
-	return nfs4_proc_set_acl(inode, buf, buflen);
+	error = nfs4_proc_set_acl(inode, buf, buflen);
+	nfs_handle_estale(error, inode);
+	return error;
 }
 
 /* The getxattr man page suggests returning -ENODATA for unknown attributes,

@@ -3619,11 +3622,14 @@ ssize_t nfs4_getxattr(struct dentry *den
 		size_t buflen)
 {
 	struct inode *inode = dentry->d_inode;
+	int error;
 
 	if (strcmp(key, XATTR_NAME_NFSV4_ACL) != 0)
 		return -EOPNOTSUPP;
 
-	return nfs4_proc_get_acl(inode, buf, buflen);
+	error = nfs4_proc_get_acl(inode, buf, buflen);
+	nfs_handle_estale(error, inode);
+	return error;
 }
 
 ssize_t nfs4_listxattr(struct dentry *dentry, char *buf, size_t buflen)

fs/nfs/namespace.c renamed from fs/nfs/namespace.c.org +1 -0

@@ -115,6 +115,7 @@ static void * nfs_follow_mountpoint(stru
 	err = server->nfs_client->rpc_ops->lookup(parent->d_inode,
 						  &nd->path.dentry->d_name,
 						  &fh, &fattr);
+	nfs_handle_estale(err, parent->d_inode);
 	dput(parent);
 	if (err != 0)
 		goto out_err;

fs/nfs/symlink.c renamed from fs/nfs/symlink.c.org +1 -0

@@ -40,6 +40,7 @@ static int nfs_symlink_filler(struct ino
 	return 0;
 
 error:
+	nfs_handle_estale(error, inode);
 	SetPageError(page);
 	unlock_page(page);
 	return -EIO;

fs/nfs/nfs3acl.c renamed from fs/nfs/nfs3acl.c.org +22 -6

@@ -22,8 +22,10 @@ ssize_t nfs3_listxattr(struct dentry *de
 		} while(0)
 
 	acl = nfs3_proc_getacl(inode, ACL_TYPE_ACCESS);
-	if (IS_ERR(acl))
+	if (IS_ERR(acl)) {
+		nfs_handle_estale(PTR_ERR(acl), inode);
 		return PTR_ERR(acl);
+	}
 	if (acl) {
 		output("system.posix_acl_access");
 		posix_acl_release(acl);

@@ -31,8 +33,10 @@ ssize_t nfs3_listxattr(struct dentry *de
 
 	if (S_ISDIR(inode->i_mode)) {
 		acl = nfs3_proc_getacl(inode, ACL_TYPE_DEFAULT);
-		if (IS_ERR(acl))
+		if (IS_ERR(acl)) {
+			nfs_handle_estale(PTR_ERR(acl), inode);
 			return PTR_ERR(acl);
+		}
 		if (acl) {
 			output("system.posix_acl_default");
 			posix_acl_release(acl);

@@ -61,9 +65,11 @@ ssize_t nfs3_getxattr(struct dentry *den
 		return -EOPNOTSUPP;
 
 	acl = nfs3_proc_getacl(inode, type);
-	if (IS_ERR(acl))
+	if (IS_ERR(acl)) {
+		nfs_handle_estale(PTR_ERR(acl), inode);
 		return PTR_ERR(acl);
-	else if (acl) {
+	}
+	if (acl) {
 		if (type == ACL_TYPE_ACCESS && acl->a_count == 0)
 			error = -ENODATA;
 		else

@@ -92,6 +98,7 @@ int nfs3_setxattr(struct dentry *dentry,
 	acl = posix_acl_from_xattr(value, size);
 	if (IS_ERR(acl))
 		return PTR_ERR(acl);
+
 	error = nfs3_proc_setacl(inode, type, acl);
 	posix_acl_release(acl);

@@ -355,16 +362,22 @@ int nfs3_proc_setacl(struct inode *inode
 			case ACL_TYPE_ACCESS:
 				alloc = dfacl = nfs3_proc_getacl(inode,
 						ACL_TYPE_DEFAULT);
-				if (IS_ERR(alloc))
+				if (IS_ERR(alloc)) {
+					nfs_handle_estale(PTR_ERR(alloc),
+							inode);
 					goto fail;
+				}
 				break;
 
 			case ACL_TYPE_DEFAULT:
 				dfacl = acl;
 				alloc = acl = nfs3_proc_getacl(inode,
 						ACL_TYPE_ACCESS);
-				if (IS_ERR(alloc))
+				if (IS_ERR(alloc)) {
+					nfs_handle_estale(PTR_ERR(alloc),
+							inode);
 					goto fail;
+				}
 				break;
 
 			default:

@@ -380,6 +393,7 @@ int nfs3_proc_setacl(struct inode *inode
 	}
 	status = nfs3_proc_setacls(inode, acl, dfacl);
 	posix_acl_release(alloc);
+	nfs_handle_estale(status, inode);
 	return status;
 
 fail:

@@ -395,6 +409,7 @@ int nfs3_proc_set_default_acl(struct ino
 	dfacl = nfs3_proc_getacl(dir, ACL_TYPE_DEFAULT);
 	if (IS_ERR(dfacl)) {
 		error = PTR_ERR(dfacl);
+		nfs_handle_estale(error, dir);
 		return (error == -EOPNOTSUPP) ? 0 : error;
 	}
 	if (!dfacl)

@@ -408,6 +423,7 @@ int nfs3_proc_set_default_acl(struct ino
 		goto out_release_acl;
 	error = nfs3_proc_setacls(inode, acl, S_ISDIR(inode->i_mode) ?
 						      dfacl : NULL);
+	nfs_handle_estale(error, inode);
 out_release_acl:
 	posix_acl_release(acl);
 out_release_dfacl:

include/linux/nfs_fs.h renamed from include/linux/nfs_fs.h.org +25 -0

@@ -259,6 +259,31 @@ static inline void set_nfs_fileid(struct
 	NFS_I(inode)->fileid = fileid;
 }
 
+static inline void nfs_handle_estale(int error, struct inode *inode)
+{
+	struct nfs_inode *nfsi = NFS_I(inode);
+
+	if (error == -ESTALE) {
+		if (!NFS_STALE(inode))
+			set_bit(NFS_INO_STALE, &NFS_I(inode)->flags);
+		/*
+		 * If a directory is being marked as stale, then
+		 * reset the attribute cache timeout.  This controls
+		 * the duration that the stale condition is believed
+		 * before being verified again with another call to
+		 * the server.
+		 */
+		if (S_ISDIR(inode->i_mode))
+			nfsi->read_cache_jiffies = jiffies;
+	}
+}
+
+static inline void nfs_clear_estale(struct inode *inode)
+{
+	if (NFS_STALE(inode))
+		clear_bit(NFS_INO_STALE, &NFS_I(inode)->flags);
+}
+
 static inline void nfs_mark_for_revalidate(struct inode *inode)
 {
 	struct nfs_inode *nfsi = NFS_I(inode);