Reinstantiating stale inodes

Here is a 2.4 patch that will reinstantiate an inode when a ESTALE error is returned on a getattr. When the error occurs, a lookup is immediately issued to get a new fh.

The fixes the problem of a server rsync -a directory that a client has mounted. The key being the -a flag since it causes the server not to update the mtime on the directory.

My initial efforts was to make nfs_lookup_revalidate() a bit smarter with the use of ctimes but turns out that when nfs_lookup_revalidate() does no caching (i.e. an otw lookup is issued on every call) the ESTALEs still occurred.

Then I turned my attention to __nfs_refresh_inode() and had it used ctime in its calculations of what is
and is not valid... This did work, but it cause a significant amount of extra otw traffic (using the cthon test suite) for the non error cases. The one thing good about this patch (imho) is the extra lookups only occur after an error that generally does not happen...

Comments would be appreciated, especially about how I'm reinstantiating the dentry....

SteveD.

@@ -953,13 +953,57 @@ nfs_wait_on_inode(struct inode *inode, i
 }
 
 /*
+ * Reinstantiate an inode that has gone stale
+ */
+static int
+nfs_reinstantiate(
+	struct inode *dir, 
+	struct dentry *dentry, 
+	struct nfs_fattr *fattr)
+{
+	int error;
+	struct nfs_fh fhandle;
+	struct inode *inode;
+
+	error = NFS_PROTO(dir)->lookup(dir, &dentry->d_name, &fhandle, &fattr);
+	if (!error) {
+		error = -ENOMEM;
+		inode = nfs_fhget(dentry, &fhandle, &fattr);
+		if (inode) {
+			d_drop(dentry);
+			dput(dentry);
+			d_instantiate(dentry, inode);
+			dentry->d_time = jiffies;
+			error = 0;
+		}
+	}
+	return error;
+}
+
+/*
  * Externally visible revalidation function
  */
 int
 nfs_revalidate(struct dentry *dentry)
 {
 	struct inode *inode = dentry->d_inode;
-	return nfs_revalidate_inode(NFS_SERVER(inode), inode);
+	struct inode *pinode;
+	struct nfs_fattr fattr;
+	int error;
+
+	error = nfs_revalidate_inode(NFS_SERVER(inode), inode);
+	if (!error || error != -ESTALE)
+		return error;
+	/*
+	 * We have a stale fh so ask the server for another one
+	 */
+	pinode = dentry->d_parent->d_inode;	
+	if (nfs_reinstantiate(pinode, dentry, &fattr) == 0) {
+		inode = dentry->d_inode;
+		if (nfs_refresh_inode(inode, &fattr) == 0)
+			error = 0;
+	}
+	return error;
 }
 
 /*

Brrr. Are you sure this is such a good idea? It will have all sorts of bad side effects. For instance, you may be writing a file named "foo". Someone else replaces foo with their copy (mv foo-new foo) and your file handle becomes stale.

If you issue a lookup immediately, you will continue writing, but now your writes go to the new file and produce garbage.

At a minimum, the lookup should occur at file open, and only if there are no other users of the inode.

Olaf

Olaf Kirch     |  The Hardware Gods hate me.
[email protected]   |
---------------+ 


-------------------------------------------------------
This SF.net email is sponsored by: The Robotic Monkeys at ThinkGeek
For a limited time only, get FREE Ground shipping on all orders of $35
or more. Hurry up and shop folks, this offer expires April 30th!
http://www.thinkgeek.com/freeshipping/?cpg=12297
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Hi Olaf,

This is why I'm asking.... In the beginning I also thought this ideas was a bit brain dead ... but after a bunch of trials and tribulations and not being able to do it any other way that didn't effect normal traffic... it kinda grew on me... :)

True... but the write or commit will still fail with ESTALE. This patch only effects getattrs (or stat()s). I guess my subject line was a bit miss leading...

Here are the tests I ran:

client: writes 100m file called foo
server: rm foo
client: the write or commit failed with ESTALE

client: writes 100m file called foo
server: renames foo to foo.old and immediately creates foo
client: failed with ESTALE

client: writes 100m file called foo
Same client: rm foo
client: finishes writing then the file is removed

client: writes 100m file called foo
Same client: renames foo to foo.old and immediately creates foo
client: both write to foo and foo.old complete and sum -r shows
they are identical

Again only getattrs will be followed up with lookups..

Unfortunately, in this particular case, there were no opens...

SteveD.

-------------------------------------------------------
This SF.net email is sponsored by: The Robotic Monkeys at ThinkGeek For a limited time only, get FREE Ground shipping on all orders of $35 or more. Hurry up and shop folks, this offer expires April 30th!
http://www.thinkgeek.com/freeshipping/?cpg=12297
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

This shouldn't fail, because the file still exists, just under a different name.

What will get you into trouble is if your client writes to "foo" and you do a "mv bar foo".

Yes, but it's just a matter of probabilities if the first call that returns ESTALE is a write or a getattr (called from revalidate_inode).

So what exactly went wrong? A stat() on the files that just got replaced?

Olaf

Olaf Kirch     |  The Hardware Gods hate me.
[email protected]   |
---------------+ 


-------------------------------------------------------
This SF.net email is sponsored by: The Robotic Monkeys at ThinkGeek
For a limited time only, get FREE Ground shipping on all orders of $35
or more. Hurry up and shop folks, this offer expires April 30th!
http://www.thinkgeek.com/freeshipping/?cpg=12297
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

This must have been cut and paste error... Because I ran the test again and didn't see a ESTALE error...

This also seem to work... I figured since foo was open it got renamed to .nfs file until the first write was finished...

Yes... problem is the files get changed but the mtime on the directory does not...

SteveD.

-------------------------------------------------------
This SF.net email is sponsored by: The Robotic Monkeys at ThinkGeek For a limited time only, get FREE Ground shipping on all orders of $35 or more. Hurry up and shop folks, this offer expires April 30th!
http://www.thinkgeek.com/freeshipping/?cpg=12297
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

It will, as long as the mv is executed on the NFS client where your writing process runs. But in the particular case that prompted you do create that patch, rsync ran on the NFS server, didn't it?

Olaf

Olaf Kirch     |  The Hardware Gods hate me.
[email protected]   |
---------------+ 


-------------------------------------------------------
This SF.net email is sponsored by: The Robotic Monkeys at ThinkGeek
For a limited time only, get FREE Ground shipping on all orders of $35
or more. Hurry up and shop folks, this offer expires April 30th!
http://www.thinkgeek.com/freeshipping/?cpg=12297
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Yes...

SteveD.

-------------------------------------------------------
This SF.net email is sponsored by: The Robotic Monkeys at ThinkGeek For a limited time only, get FREE Ground shipping on all orders of $35 or more. Hurry up and shop folks, this offer expires April 30th!
http://www.thinkgeek.com/freeshipping/?cpg=12297
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

There are several problems here, but the main one is that you have no guarantees that you are the exclusive user of that dentry.

This again means that people who think they have open files on the "old" inode may suddenly find their program Oopsing or corrupting the new file. Ditto for all those shrink_dcache_*() which are likely to Oops.

Cheers,
Trond

-------------------------------------------------------
This SF.net email is sponsored by: The Robotic Monkeys at ThinkGeek For a limited time only, get FREE Ground shipping on all orders of $35 or more. Hurry up and shop folks, this offer expires April 30th!
http://www.thinkgeek.com/freeshipping/?cpg=12297
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

True, there are no guarantees the user has exclusive access but what does it matter? Its a stale inode if they try to used its going to fail...

Again, this new fh is only gotten on getattrs any other ops will still fail with ESTALE...

SteveD

-------------------------------------------------------
This SF.net email is sponsored by: The Robotic Monkeys at ThinkGeek For a limited time only, get FREE Ground shipping on all orders of $35 or more. Hurry up and shop folks, this offer expires April 30th!
http://www.thinkgeek.com/freeshipping/?cpg=12297
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

No! That dentry is a shared object, and you may be changing it while other ops are referencing/using it. There are no locks or any other mechanisms here that ensure this is not the case.

Even if you do solve the above race, you still have another bug: you are changing the object that the "struct file" refers to, and hence at best you are going to cause all the existing open files to start reading from/writing to the new file (which is a bug!) instead of returning ESTALE.

Cheers,
Trond

-------------------------------------------------------
This SF.net email is sponsored by: The Robotic Monkeys at ThinkGeek For a limited time only, get FREE Ground shipping on all orders of $35 or more. Hurry up and shop folks, this offer expires April 30th!
http://www.thinkgeek.com/freeshipping/?cpg=12297
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Ok... this is the silver bullet that got me... :(

Question: if (atomic_read(&dentry->d_count) < 2) can I assume that the current process is the only one attached to the dentry and it would be safe reinstantiate the dentry?

SteveD.

-------------------------------------------------------
This SF.net email is sponsored by: The Robotic Monkeys at ThinkGeek For a limited time only, get FREE Ground shipping on all orders of $35 or more. Hurry up and shop folks, this offer expires April 30th!
http://www.thinkgeek.com/freeshipping/?cpg=12297
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

You can if and only if

- The dentry is unhashed, and you are holding the semaphore dir->i_sem
on the parent directory.
OR
- You are holding the spin lock dcache_lock.

Both of these two methods will prevent the VFS from looking the dentry up.

Note: on 2.6.x, the second condition needs to be modified a bit: you probably need to hold dentry->d_lock too since the 2.6.x version of d_lookup() is by and large lockless...

Cheers,
Trond

-------------------------------------------------------
This SF.net email is sponsored by: The Robotic Monkeys at ThinkGeek For a limited time only, get FREE Ground shipping on all orders of $35 or more. Hurry up and shop folks, this offer expires April 30th!
http://www.thinkgeek.com/freeshipping/?cpg=12297
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

I decide to hold the dir->i_sem semaphore since d_drop() needs the dcache_lock to do the unhashing...

I'll take a look asaic...

SteveD.

@@ -953,13 +953,74 @@ nfs_wait_on_inode(struct inode *inode, i
 }
 
 /*
+ * Reinstantiate an inode that has gone stale
+ */
+static int
+nfs_reinstantiate(
+	struct inode *dir, 
+	struct dentry *dentry, 
+	struct nfs_fattr *fattr)
+{
+	int error;
+	struct nfs_fh fhandle;
+	struct inode *inode;
+
+	/*
+	 * We can only reinstantiate the inode if we are the
+	 * only ones accessing it.
+	 */
+	if (atomic_read(&dentry->d_count) > 1)
+		return EACCES;
+
+	error = NFS_PROTO(dir)->lookup(dir, &dentry->d_name, &fhandle, &fattr);
+	if (!error) {
+		error = -ENOMEM;
+		inode = nfs_fhget(dentry, &fhandle, &fattr);
+		if (inode) {
+			/*
+			 * Check again this time holding the i_sem semaphore
+			 * to make sure we are the only ones accessing this
+			 * dentry
+			 */
+			down(&dir->i_sem);
+			if (atomic_read(&dentry->d_count) < 2) {
+				d_drop(dentry);
+				dput(dentry);
+				d_instantiate(dentry, inode);
+				dentry->d_time = jiffies;
+				error = 0;
+			} else
+				iput(inode);
+			up(&dir->i_sem);
+		}
+	}
+	return error;
+}
+
+/*
  * Externally visible revalidation function
  */
 int
 nfs_revalidate(struct dentry *dentry)
 {
 	struct inode *inode = dentry->d_inode;
-	return nfs_revalidate_inode(NFS_SERVER(inode), inode);
+	struct inode *pinode;
+	struct nfs_fattr fattr;
+	int error;
+
+	error = nfs_revalidate_inode(NFS_SERVER(inode), inode);
+	if (!error || error != -ESTALE)
+		return error;
+	/*
+	 * We have a stale fh so ask the server for another one
+	 */
+	pinode = dentry->d_parent->d_inode;	
+	if (nfs_reinstantiate(pinode, dentry, &fattr) == 0) {
+		inode = dentry->d_inode;
+		if (nfs_refresh_inode(inode, &fattr) == 0)
+			error = 0;
+	}
+	return error;
 }
 
 /*

It really matter since the original ESTALE error will be returned if any part of the lookup fails...

SteveD.

-------------------------------------------------------
This SF.net email is sponsored by: The Robotic Monkeys at ThinkGeek For a limited time only, get FREE Ground shipping on all orders of $35 or more. Hurry up and shop folks, this offer expires April 30th!
http://www.thinkgeek.com/freeshipping/?cpg=12297
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

It turns out there is a much easier and simpler (safer) way to reinstantiate inodes than my original patch.

Realizing only parts of an inode needed to be reinstantiated not the entire thing, this second patch only reinstantiates two parts of the inode (i.e. fhandle and fattrs) instead of the entire inode like my original patch did.

I had to export the new nfs_reinstantiate() function because the ACL code needs to use it, so things like ls -l will work....

Comments? Is this something Marcelo might be interested in?

SteveD.

@@ -953,13 +953,48 @@ nfs_wait_on_inode(struct inode *inode, i
 }
 
 /*
+ * Reinstantiate an inode that has gone stale
+ */
+int
+nfs_reinstantiate(struct dentry *dentry)
+{
+	int error;
+	struct nfs_fh fhandle;
+	struct inode *dir = dentry->d_parent->d_inode;
+	struct inode *inode = dentry->d_inode;
+	struct nfs_fattr fattr;
+
+	nfs_zap_caches(dir);
+	nfs_zap_caches(inode);
+
+	error = NFS_PROTO(dir)->lookup(dir, &dentry->d_name, &fhandle, &fattr);
+	if (!error) {
+		memcpy(&inode->u.nfs_i.fh, &fhandle, sizeof(struct nfs_fh));
+		nfs_refresh_inode(inode, &fattr);
+	}
+
+	return error;
+}
+
+/*
  * Externally visible revalidation function
  */
 int
 nfs_revalidate(struct dentry *dentry)
 {
 	struct inode *inode = dentry->d_inode;
-	return nfs_revalidate_inode(NFS_SERVER(inode), inode);
+	int error;
+
+	error = nfs_revalidate_inode(NFS_SERVER(inode), inode);
+	if (!error || error != -ESTALE)
+		return error;
+	/*
+	 * We have a stale fh so ask the server for another one
+	 */
+	if (nfs_reinstantiate(dentry) == 0)
+		error = 0;
+
+	return error;
 }
 
 /*

@@ -159,6 +159,7 @@ extern struct inode *nfs_fhget(struct de
 				struct nfs_fattr *);
 extern int __nfs_refresh_inode(struct inode *, struct nfs_fattr *);
 extern int nfs_revalidate(struct dentry *);
+extern int nfs_reinstantiate(struct dentry *);
 extern int nfs_permission(struct inode *, int);
 extern int nfs_open(struct inode *, struct file *);
 extern int nfs_release(struct inode *, struct file *);

Vetoed!

You are not listening to what I am saying: you are NOT allowed to change inodes in this manner. The filehandle defines which file you are referencing. By changing the filehandle, you are changing files from beneath other processes and your own process.

Imagine if I do

rm blah; ln /etc/passwd blah

Your patch means that anyone that was writing to file "blah" before you deleted it, will suddenly find themselves overwriting /etc/passwd. That is NOT POSIX-compatible behaviour!

The ONLY way to overcome a stale inode is to d_drop() the dentry, unhash the inode, and then force the VFS to look up a new inode. That way only new calls to open() end up overwriting /etc/passwd in the above case.

Cheers,
Trond

-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Believe me... I hear you.... and do understand what I'm trying to do.... And I also apologize for being so persistent.... but just bothers the hell out of me that other clients can recover from ESTALEs and we don't...

I see this point in theory, but in my testing, bad things don't seem to happen. The writes always fails with ESTALE... but I can't deny there is not a window just because I can't reproduce it...

I guess I would be interested on how other implementation handle this problem, since they seem to recover... Maybe their VFS layers is more friendly to these types of things....

SteveD.

-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

So please tell me why the following patch (which addresses the particular problem that you raised of someone resetting mtime on the parent directory) does not suffice?

Cheers,
Trond

diff -u --recursive --new-file --show-c-function linux-2.4.26/fs/nfs/dir.c gnurr/fs/nfs/dir.c
@@ -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));
 }
 
 /*

diff -u --recursive --new-file --show-c-function linux-2.4.26/fs/nfs/inode.c gnurr/fs/nfs/inode.c
@@ -1054,16 +1054,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;

diff -u --recursive --new-file --show-c-function linux-2.4.26/include/linux/nfs_fs.h gnurr/include/linux/nfs_fs.h
@@ -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)

diff -u --recursive --new-file --show-c-function linux-2.4.26/include/linux/nfs_fs_i.h gnurr/include/linux/nfs_fs_i.h
@@ -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

Again this is were I started... and this patch does take care of the ESTALEs but it also increases normal traffic by 2% to 3% (mostly getattrs and lookups) when I ran the connectathon04 tests... Granted 2% to 3% is not that much of an increase and my testing is not that exact... but.... any increase for an error that generally does not happen, I didn't think would be acceptable...

But if this the patch thats going to make it into the 2.4 tree... so be it.... since it does avoid the ESTALE issues and maybe things will be a bit more coherent since we do send a few more getattrs and lookups...

Thanks for your guidance.... it is definitely appreciated!

SteveD.

-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

So this is what I don't understand.

Which are the operations that are supposed to "ctime" without changing "mtime"? AFAICS the *only* such operation is utime(), which was the one that was causing you trouble in the first place.

I certainly would not expect any difference between the two when looking at the standard connectathon suite.

Unless....

Is your kernel perhaps missing the appended patch, which is already in 2.4.26?

Cheers,
Trond

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/02/09 11:09:22-06:00 [email protected]
# JFS: rename should update mtime on source and target directories
#

# fs/jfs/namei.c
#   2004/02/09 11:07:24-06:00 [email protected] +2 -2
#   rename should update i_mtime on affected directories
# 
diff -Nru a/fs/jfs/namei.c b/fs/jfs/namei.c
--- a/fs/jfs/namei.c	Sat May  1 23:28:13 2004
+++ b/fs/jfs/namei.c	Sat May  1 23:28:13 2004
@@ -1223,7 +1223,7 @@
 	old_ip->i_ctime = CURRENT_TIME;
 	mark_inode_dirty(old_ip);

- new_dir->i_ctime = CURRENT_TIME;
+ new_dir->i_ctime = new_dir->i_mtime = CURRENT_TIME;
mark_inode_dirty(new_dir);

/* Build list of inodes modified by this transaction */
@@ -1235,7 +1235,7 @@

if (old_dir != new_dir) {
iplist[ipcount++] = new_dir;
- old_dir->i_ctime = CURRENT_TIME;
+ old_dir->i_ctime = old_dir->i_mtime = CURRENT_TIME;
mark_inode_dirty(old_dir);
}

Nor did I... I kinda thought it would reduce traffic.... but over the last few days, I've tried a number of combinations of ctime and mtime to 1) fix the estale problem and
2) not added more traffic... unfortunately with no success....

So I would like to propose a less intrusive patch what will simply zap the directories caches when nfs_revalidate() sees an ESTALE.

This patch does not effect traffic and only the first dir entry is failed with ESTALE and finally subsequent ls will not fail with ESTALEs....

Comments?

SteveD.

@@ -958,8 +958,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;
 }
 
 /*

That one looks fine (although it will not apply to 2.6.x).

I'm still curious to find out which operations are changing ctime on the parent directory without changing mtime, though.

I've identified only two more (beside utime()), so far: link()/unlink() change the ctime on the file but both do set mtime and ctime on the parent directory.

Cheers,
Trond

-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Its a 2.4 patch...

Here are two if the runs I've seen... The first one is without the patch, the second one is with the patch...

client rpc stats:

calls      retrans    authrefrsh
30073      7          0

Client nfs v3:
null getattr setattr lookup access readlink
0 0% 2280 7% 1199 3% 2595 8% 1173 3% 250 0%
read write create mkdir symlink mknod
3954 13% 11892 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% 600 1%

Client rpc stats:

calls      retrans    authrefrsh
34134      22         0
Client nfs v3:
null       getattr    setattr    lookup     access     readlink
0       0% 2124    6% 1199    3% 2979    8% 1174    3% 250     0%
read       write      create     mkdir      symlink    mknod     3954   11% 15752  46% 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% 568 1%

The lookups and writes seem to sky rocketing.... and I've seen other runs (with using both mtime and ctimes) where the lookups, getattrs and reads sky rocket

SteveD.

-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Interesting... You've got a few more renames too.

I assume those are just extra sillyrenames() if this was a standard Connectathon test workload. OK... So the extra sillyrename() might explain why you get more lookups() (assuming at least one of the sillyrenames is on one of those large directories that Cthon creates). The extra writes probably account for the sillyrenames() because they change the timing of close()...

So the big question is: *why do you get all those extra writes*?

Hmm... Just wanting to check: the above results were indeed made with only the patch I sent you applied?
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)?

Cheers,
Trond

-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g. Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3149&alloc_id=8166&op=click
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

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%

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.

@@ -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));
 }
 
 /*

@@ -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;

@@ -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)

@@ -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

Here is a proper way of reinstantiating stale inodes....

SteveD.

@@ -958,8 +958,17 @@ 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);	
+		d_drop(dentry);
+	}
+
+	return error;
 }
 
 /*