Re: [PATCH] Reinstantiating stale inodes

[Steve Dickson <SteveD@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2250 bytes --]


Trond Myklebust wrote:

>So the big question is: *why do you get all those extra writes*?
>  
>
This appears to be a red herring... After a complete build in a
new tree that only had the ctime and zapping cache patches I started
to get more reasonable results... But it does appear that the ctime
patch does (consistently) cause more getattrs and lookups...

W/OUT any patches
Client rpc stats:
calls      retrans    authrefrsh
30052      16         0
Client nfs v3:
null       getattr    setattr    lookup     access     readlink
0       0% 2288    7% 1199    3% 2561    8% 1171    3% 250     0%
read       write      create     mkdir      symlink    mknod
3955   13% 11897  39% 893     2% 175     0% 250     0% 0       0%
remove     rmdir      rename     link       readdir    readdirplus
1391    4% 175     0% 376     1% 250     0% 0       0% 1119    3%
fsstat     fsinfo     pathconf   commit
1500    4% 1       0% 0       0% 601     1%

With CTIME patch
Client rpc stats:
calls      retrans    authrefrsh
30288      7          0      
Client nfs v3:
null       getattr    setattr    lookup     access     readlink  
0       0% 2308    7% 1199    3% 2773    9% 1171    3% 250     0%
read       write      create     mkdir      symlink    mknod     
3954   13% 11899  39% 893     2% 175     0% 250     0% 0       0%
remove     rmdir      rename     link       readdir    readdirplus
1391    4% 175     0% 380     1% 250     0% 0       0% 1119    3%
fsstat     fsinfo     pathconf   commit    
1500    4% 1       0% 0       0% 600     1%

>There weren't any extra changes to nfs_refresh_inode() that might cause
>the actual page cache invalidation to depend on the inode ctime (as
>opposed to just the lookup cache)?
>  
>
No... there are some slight differences in __nfs_refresh_inode()... but
no smoking gun....

At this point I feel the numbers are close enough to declare victory.
The attached 2.4 patch *does* avoid ESTALEs when the server rsync -a the
mounted directory for a (very) small price... 1 to 2 percent increase in
traffic (in a particular kernel, running a particular test suite)
is not a bad price to pay to avoid ESTALEs.... imho...

What are the chances of this patch (or something similar) making
it into 2.4. tree?

SteveD.


[-- Attachment #2: linux-2.4-nfs-estale.patch --]
[-- Type: text/plain, Size: 3060 bytes --]

--- linux-2.4/fs/nfs/dir.c.orig	2003-11-11 16:55:40.000000000 -0500
+++ linux-2.4/fs/nfs/dir.c	2004-05-04 13:26:20.000000000 -0400
@@ -421,7 +421,7 @@ int nfs_check_verifier(struct inode *dir
 		return 1;
 	if (nfs_revalidate_inode(NFS_SERVER(dir), dir))
 		return 0;
-	return time_after(dentry->d_time, NFS_MTIME_UPDATE(dir));
+	return time_after(dentry->d_time, NFS_CTIME_UPDATE(dir));
 }
 
 /*
--- linux-2.4/fs/nfs/inode.c.orig	2004-05-04 13:26:21.000000000 -0400
+++ linux-2.4/fs/nfs/inode.c	2004-05-04 13:28:20.000000000 -0400
@@ -821,8 +821,16 @@ nfs_wait_on_inode(struct inode *inode, i
 int
 nfs_revalidate(struct dentry *dentry)
 {
+	int error;
 	struct inode *inode = dentry->d_inode;
-	return nfs_revalidate_inode(NFS_SERVER(inode), inode);
+
+	error = nfs_revalidate_inode(NFS_SERVER(inode), inode);
+	if (error == -ESTALE) {
+		struct inode *dir = dentry->d_parent->d_inode;
+		nfs_zap_caches(dir);
+	}
+
+	return error;
 }
 
 /*
@@ -1054,16 +1062,16 @@ __nfs_refresh_inode(struct inode *inode,
 	if (nfs_have_writebacks(inode) && new_isize < inode->i_size)
 		new_isize = inode->i_size;
 
-	NFS_CACHE_CTIME(inode) = fattr->ctime;
-	inode->i_ctime = nfs_time_to_secs(fattr->ctime);
+	if (NFS_CACHE_CTIME(inode) != fattr->ctime) {
+		NFS_CACHE_CTIME(inode) = fattr->ctime;
+		inode->i_ctime = nfs_time_to_secs(fattr->ctime);
+		NFS_CTIME_UPDATE(inode) = jiffies;
+	}
 
 	inode->i_atime = new_atime;
 
-	if (NFS_CACHE_MTIME(inode) != new_mtime) {
-		NFS_MTIME_UPDATE(inode) = jiffies;
-		NFS_CACHE_MTIME(inode) = new_mtime;
-		inode->i_mtime = nfs_time_to_secs(new_mtime);
-	}
+	NFS_CACHE_MTIME(inode) = new_mtime;
+	inode->i_mtime = nfs_time_to_secs(new_mtime);
 
 	NFS_CACHE_ISIZE(inode) = new_size;
 	inode->i_size = new_isize;
--- linux-2.4/include/linux/nfs_fs.h.orig	2004-05-04 13:26:23.000000000 -0400
+++ linux-2.4/include/linux/nfs_fs.h	2004-05-04 13:26:24.000000000 -0400
@@ -78,7 +78,7 @@ static inline struct nfs_inode_info *NFS
 #define NFS_CONGESTED(inode)		(RPC_CONGESTED(NFS_CLIENT(inode)))
 #define NFS_COOKIEVERF(inode)		((inode)->u.nfs_i.cookieverf)
 #define NFS_READTIME(inode)		((inode)->u.nfs_i.read_cache_jiffies)
-#define NFS_MTIME_UPDATE(inode)		((inode)->u.nfs_i.cache_mtime_jiffies)
+#define NFS_CTIME_UPDATE(inode)		((inode)->u.nfs_i.cache_ctime_jiffies)
 #define NFS_CACHE_CTIME(inode)		((inode)->u.nfs_i.read_cache_ctime)
 #define NFS_CACHE_MTIME(inode)		((inode)->u.nfs_i.read_cache_mtime)
 #define NFS_CACHE_ISIZE(inode)		((inode)->u.nfs_i.read_cache_isize)
--- linux-2.4/include/linux/nfs_fs_i.h.orig	2004-05-04 13:26:24.000000000 -0400
+++ linux-2.4/include/linux/nfs_fs_i.h	2004-05-04 13:26:24.000000000 -0400
@@ -49,10 +49,10 @@ struct nfs_inode_info {
 	unsigned long		attrtimeo_timestamp;
 
 	/*
-	 * Timestamp that dates the change made to read_cache_mtime.
+	 * Timestamp that dates the change made to read_cache_ctime.
 	 * This is of use for dentry revalidation
 	 */
-	unsigned long		cache_mtime_jiffies;
+	unsigned long		cache_ctime_jiffies;
 
 	/*
 	 * This is the cookie verifier used for NFSv3 readdir