Making sure negative lookup entries don't exist

PATCH In review Steve Dickson 3 participants 6 messages Jan 28, 2005 – Jan 29, 2005

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Steve Dickson

author

2005-01-28 18:46 UTC #0 lore raw

PATCH 1/1 Making sure negative lookup entries don't exist

Here is a 2.4.28 patch that ensures negative cached dirents do not exist by always doing an otw getattr to the server. This does cause a small increase in getattrs (~3% from what I saw in my testing) but it seems to me the price is worth it to ensure the client does not lie about the existence of a file.

comments?

steved.

fs/nfs/dir.c renamed from fs/nfs/dir.c.orig +5 -1

@@ -455,7 +455,11 @@ int nfs_lookup_verify_inode(struct inode
  */
 static inline int nfs_neg_need_reval(struct inode *dir, struct dentry *dentry)
 {
-	if (!nfs_check_verifier(dir, dentry))
+	if (IS_ROOT(dentry))
+		return 1;
+	if (__nfs_revalidate_inode(NFS_SERVER(dir), dir))
+		return 1;
+	if (time_after(NFS_MTIME_UPDATE(dir), dentry->d_time))
 		return 1;
 	return time_after(jiffies, dentry->d_time + NFS_ATTRTIMEO(dir));
 }

Mike Waychison

2005-01-29 01:08 UTC #1 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 16 lines · Steve Dickson

Here is a 2.4.28 patch that ensures negative cached dirents do not exist by always doing an otw getattr to the server. This does cause a small increase in getattrs (~3% from what I saw in my testing) but it seems to me the price is worth it to ensure the client does not lie about the existence of a file.
comments?
steved.
------------------------------------------------------------------------

fs/nfs/dir.c int nfs_lookup_verify_inode(struct inode · line 459

Quoted 6 lines

fs/nfs/dir.c renamed from fs/nfs/dir.c.orig +2 -1

@@ -455,7 +455,11 @@ int nfs_lookup_verify_inode(struct inode
  */
 static inline int nfs_neg_need_reval(struct inode *dir, struct dentry *dentry)
 {
-	if (!nfs_check_verifier(dir, dentry))
+	if (IS_ROOT(dentry))
+		return 1;

I don't think this is needed. Aren't you guaranteed that a root dentry will never have ->d_revalidate called?

fs/nfs/dir.c int nfs_lookup_verify_inode(struct inode · line 465

Quoted 6 lines

fs/nfs/dir.c renamed from fs/nfs/dir.c.orig +3 -0

+	if (__nfs_revalidate_inode(NFS_SERVER(dir), dir))
+		return 1;
+	if (time_after(NFS_MTIME_UPDATE(dir), dentry->d_time))
 		return 1;
 	return time_after(jiffies, dentry->d_time + NFS_ATTRTIMEO(dir));
 }

Am I missing something? or doesn't nfs_check_verifier do exactly these checks already?

- --
Mike Waychison
Sun Microsystems, Inc.
1 (650) 352-5299 voice
1 (416) 202-8336 voice

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: The opinions expressed in this email are held by me, and may not represent the views of Sun Microsystems, Inc.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFB+uH3dQs4kOxk3/MRArnXAJsGRs/yb8fazMcgB/T1l5K+o0O3qQCfXBEg +C712kBY6Nhp78X1qelIwe4=
=AAyH
-----END PGP SIGNATURE-----

-------------------------------------------------------
This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting Tool for open source databases. Create drag-&-drop reports. Save time by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc. Download a FREE copy at http://www.intelliview.com/go/osdn_nl
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Trond Myklebust

2005-01-29 03:05 UTC #3 lore raw

In reply to Steve Dickson

fr den 28.01.2005 Klokka 13:47 (-0500) skreiv Steve Dickson:

Quoted 5 lines

Here is a 2.4.28 patch that ensures negative cached dirents do not exist by always doing an otw getattr to the server. This does cause a small increase in getattrs (~3% from what I saw in my testing) but it seems to me the price is worth it to ensure the client does not lie about the existence of a file.

If you are going to do a GETATTR on each call, why cache negative dentries in the first place? Better then to simply remove the relevant lines in nfs_lookup() and replace all occurrences of d_delete() with d_drop().

Note, though, that both patches will seriously kill performance on static, frequently-accessed directories (think $PATH, /usr/share etc.), so we're certainly not applying that kind of patch to the mainline kernel...

Cheers,
Trond

Signature · 12 lines

Trond Myklebust <[email protected]>



-------------------------------------------------------
This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
Tool for open source databases. Create drag-&-drop reports. Save time
by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
Download a FREE copy at http://www.intelliview.com/go/osdn_nl
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Steve Dickson

author

2005-01-29 12:30 UTC #2 lore raw

In reply to Mike Waychison

Quoted 6 lines · Mike Waychison

Quoted 6 lines

fs/nfs/dir.c renamed from fs/nfs/dir.c.orig +0 -0

@@ -455,7 +455,11 @@ int nfs_lookup_verify_inode(struct inode
 */
 static inline int nfs_neg_need_reval(struct inode *dir, struct dentry *dentry)
 {

Quoted 10 lines

fs/nfs/dir.c int nfs_lookup_verify_inode(struct inode
Quoted 5 lines
fs/nfs/dir.c renamed from fs/nfs/dir.c.orig +2 -1
-	if (!nfs_check_verifier(dir, dentry))
+	if (IS_ROOT(dentry))
+		return 1;
 
 
I don't think this is needed. Aren't you guaranteed that a root dentry will never have ->d_revalidate called?

hmm... I guess I didn't realize this... but it appear your right....

Quoted 15 lines

Quoted 10 lines
fs/nfs/dir.c renamed from fs/nfs/dir.c.orig +3 -0
+	if (__nfs_revalidate_inode(NFS_SERVER(dir), dir))
+		return 1;
+	if (time_after(NFS_MTIME_UPDATE(dir), dentry->d_time))
 		return 1;
	return time_after(jiffies, dentry->d_time + NFS_ATTRTIMEO(dir));
}
Am I missing something? or doesn't nfs_check_verifier do exactly these checks already?

nfs_check_verifier() calls nfs_revalidate_inode() which checks the attribute cache before doing the getattr, __nfs_revalidate_inode() just does the getattr

steved.

Steve Dickson

author

2005-01-29 12:58 UTC #4 lore raw

In reply to Trond Myklebust

Quoted 6 lines · Trond Myklebust

If you are going to do a GETATTR on each call, why cache negative dentries in the first place? Better then to simply remove the relevant lines in nfs_lookup() and replace all occurrences of d_delete() with d_drop().

Well I believe that would cause an otw lookup which is much more expensive than a cheap getattr to ensure the correct mtime of the directory....

Quoted 4 lines

Note, though, that both patches will seriously kill performance on static, frequently-accessed directories (think $PATH, /usr/share etc.),

Well in my testing, which consisted of running the cthon04 tests a number of times and counting the opts with nfsstat, the getattrs only went up by 3% and the lookups when down by about 2%.... which was a bit surprising....

Quoted 4 lines

so we're certainly not applying that kind of patch to the mainline kernel...

hmm... I hate when you say things like that... :-) I just trying to come up with a way to more aggressively time out negative cached entires w/out effecting the entire attribute cache in hopes to make things like ClearCase run better.... Through some reverse engineering, it appears this is how other implementations seem to deal with negative cache entries....

steved.

Trond Myklebust

2005-01-29 19:33 UTC #5 lore raw

lau den 29.01.2005 Klokka 07:59 (-0500) skreiv Steve Dickson:

Quoted 10 lines

Quoted 6 lines · Trond Myklebust
If you are going to do a GETATTR on each call, why cache negative dentries in the first place? Better then to simply remove the relevant lines in nfs_lookup() and replace all occurrences of d_delete() with d_drop().
Well I believe that would cause an otw lookup which is much more expensive than a cheap getattr to ensure the correct mtime of the directory....

It is not that much more expensive (for Linux servers at least).

Quoted 9 lines

Quoted 4 lines
Note, though, that both patches will seriously kill performance on static, frequently-accessed directories (think $PATH, /usr/share etc.),
Well in my testing, which consisted of running the cthon04 tests a number of
times and counting the opts with nfsstat, the getattrs only went up by 3% and the
lookups when down by about 2%.... which was a bit surprising....

I can believe it for Connectathon, but that doesn't really test the sort of scenario I described where you are repeatedly searching through read-only directories for frequently-used files.

Try looking at an nfsroot setup with and without your patch. That should illustrate more clearly what I meant.

Quoted 6 lines

Quoted 4 lines
so we're certainly not applying that kind of patch to the mainline kernel...
hmm... I hate when you say things like that... :-) I just trying to come up with

In this case it's a question of "been there, done that, had the Torvalds chastise me for my stupidity"... 8-)

Cheers,
Trond

Signature · 12 lines

Trond Myklebust <[email protected]>



-------------------------------------------------------
This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
Tool for open source databases. Create drag-&-drop reports. Save time
by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
Download a FREE copy at http://www.intelliview.com/go/osdn_nl
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs