[NFS][PATCH] Making sure negative lookup entries don't exist

[NFS][PATCH] Making sure negative lookup entries don't exist

[Steve Dickson]

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

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.

[-- Attachment #2: linux-2.4.28-nfs-negcache.patch --]
[-- Type: text/x-patch, Size: 568 bytes --]

--- linux-2.4.28/fs/nfs/dir.c.orig	2004-08-07 19:26:06.000000000 -0400
+++ linux-2.4.28/fs/nfs/dir.c	2005-01-28 13:00:59.402239000 -0500
@@ -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));
 }

Re: [NFS][PATCH] Making sure negative lookup entries don't exist

[Mike Waychison]

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

Steve Dickson wrote:
> 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.
> 
> 
> ------------------------------------------------------------------------
> 
> --- linux-2.4.28/fs/nfs/dir.c.orig	2004-08-07 19:26:06.000000000 -0400
> +++ linux-2.4.28/fs/nfs/dir.c	2005-01-28 13:00:59.402239000 -0500
> @@ -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?

> +	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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [NFS][PATCH] Making sure negative lookup entries don't exist

[Steve Dickson]

Mike Waychison wrote:

>>--- linux-2.4.28/fs/nfs/dir.c.orig	2004-08-07 19:26:06.000000000 -0400
>>+++ linux-2.4.28/fs/nfs/dir.c	2005-01-28 13:00:59.402239000 -0500
>>@@ -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?
>  
>
hmm... I guess I didn't realize this...  but it appear your right....

>  
>
>>+	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.



-------------------------------------------------------
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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [NFS][PATCH] Making sure negative lookup entries don't exist

[Trond Myklebust]

fr den 28.01.2005 Klokka 13:47 (-0500) skreiv 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.

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
-- 
Trond Myklebust <trond.myklebust@fys.uio.no>



-------------------------------------------------------
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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [NFS][PATCH] Making sure negative lookup entries don't exist

[Steve Dickson]

Trond Myklebust wrote:

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

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

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



-------------------------------------------------------
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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [NFS][PATCH] Making sure negative lookup entries don't exist

[Trond Myklebust]

lau den 29.01.2005 Klokka 07:59 (-0500) skreiv Steve Dickson:
> Trond Myklebust wrote:
> 
> >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).

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

> >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
-- 
Trond Myklebust <trond.myklebust@fys.uio.no>



-------------------------------------------------------
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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs