[RFC PATCH] cleaning up negative dentries when remote directory is changed

[RFC PATCH] cleaning up negative dentries when remote directory is changed

[Steve French]

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

Due to the dcache caching negative dentries, files which were deleted on 
the client (if it resulted in a negative denty) and soon after readded 
on the server would still appear deleted to stat (lookup) at least for 
the case of cifs .   Although a readdir would readd the positive dentry, 
it would be nice to invalidate negative dentries if the client sees that 
the parent directory changed (on the server) so that the negative 
dentries would get tossed.   Looking around some other examples, it 
looks like at least two (coda, nfs) handle this by calling
    shrink_dcache_parent(dentry)
which is the obvious way to do this (as in the attached patch which 
seems to work for cifs and address the reported bug).   Is there a 
better way to remove the negative dentries (which may no longer be 
negative) under a changed directory?



[-- Attachment #2: dir-revalidate.patch --]
[-- Type: text/x-patch, Size: 1206 bytes --]

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 05b5258..45b67e9 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1039,14 +1042,18 @@ int cifs_revalidate(struct dentry *diren
 		filemap_fdatawrite(direntry->d_inode->i_mapping);
 	}
 	if (invalidate_inode) {
-		if (direntry->d_inode->i_mapping)
-			filemap_fdatawait(direntry->d_inode->i_mapping);
-		/* may eventually have to do this for open files too */
-		if (list_empty(&(cifsInode->openFileList))) {
-			/* Has changed on server - flush read ahead pages */
-			cFYI(1, ("Invalidating read ahead data on "
-				 "closed file"));
-			invalidate_remote_inode(direntry->d_inode);
+		if(S_ISDIR(direntry->d_inode->i_mode)) {
+			shrink_dcache_parent(direntry);
+		} else if (S_ISREG(direntry->d_inode->i_mode)) {
+			if (direntry->d_inode->i_mapping)
+				filemap_fdatawait(direntry->d_inode->i_mapping);
+			/* may eventually have to do this for open files too */
+			if (list_empty(&(cifsInode->openFileList))) {
+				/* changed on server - flush read ahead pages */
+				cFYI(1, ("Invalidating read ahead data on "
+					 "closed file"));
+				invalidate_remote_inode(direntry->d_inode);
+			}
 		}
 	}
 /*	up(&direntry->d_inode->i_sem); */

Re: [RFC PATCH] cleaning up negative dentries when remote directory is changed

[Trond Myklebust]

On Thu, 2005-11-24 at 19:33 -0600, Steve French wrote:
> Due to the dcache caching negative dentries, files which were deleted on 
> the client (if it resulted in a negative denty) and soon after readded 
> on the server would still appear deleted to stat (lookup) at least for 
> the case of cifs .   Although a readdir would readd the positive dentry, 
> it would be nice to invalidate negative dentries if the client sees that 
> the parent directory changed (on the server) so that the negative 
> dentries would get tossed.   Looking around some other examples, it 
> looks like at least two (coda, nfs) handle this by calling
>     shrink_dcache_parent(dentry)

No they don't. shrink_dcache_parent() gets rid of _all_ unused child
dentries, which is worse than not caching the negative dentry.

d_drop() is the right thing to do when you are talking about a single
dentry.

> which is the obvious way to do this (as in the attached patch which 
> seems to work for cifs and address the reported bug).   Is there a 
> better way to remove the negative dentries (which may no longer be 
> negative) under a changed directory?

The way we do it in NFS is that we check the mtime of the parent
directory. If the latter has changed since the last time we revalidated
the negative dentry we call d_drop() in order to force a new lookup (the
timestamp is stored in dentry->d_time).

In order to deal with the (rare) case where the server does not support
mtime, we also have a hard limit on the lifetime of the negative dentry.

Cheers,
  Trond

Re: [RFC PATCH] cleaning up negative dentries when remote directory is changed

[Steve French]

Trond Myklebust wrote:
> On Thu, 2005-11-24 at 19:33 -0600, Steve French wrote:
>   
>> Due to the dcache caching negative dentries, files which were deleted on 
>> the client (if it resulted in a negative denty) and soon after readded 
>> on the server would still appear deleted to stat (lookup) at least for 
>> the case of cifs .   Although a readdir would readd the positive dentry, 
>> it would be nice to invalidate negative dentries if the client sees that 
>> the parent directory changed (on the server) so that the negative 
>> dentries would get tossed.   Looking around some other examples, it 
>> looks like at least two (coda, nfs) handle this by calling
>>     shrink_dcache_parent(dentry)
>>     
>
> No they don't. shrink_dcache_parent() gets rid of _all_ unused child
> dentries, which is worse than not caching the negative dentry.
>
> d_drop() is the right thing to do when you are talking about a single
> dentry.
>
>   
>> which is the obvious way to do this (as in the attached patch which 
>> seems to work for cifs and address the reported bug).   Is there a 
>> better way to remove the negative dentries (which may no longer be 
>> negative) under a changed directory?
>>     
>
> The way we do it in NFS is that we check the mtime of the parent
> directory. If the latter has changed since the last time we revalidated
> the negative dentry we call d_drop() in order to force a new lookup (the
> timestamp is stored in dentry->d_time).
>
>   
I don't even see any code in my vfs being called on the negative dentry 
other than
the lookup on the parent e.g:
    ls /mnt/modified_dir/deleted_file

I see that the lookup on modified_dir hits cifs's dentry_revalidate 
routine but there is
no call to dentry_revalidate on "deleted_file" ... does nfs export some 
magic dentry
routines other than revalidate?

> In order to deal with the (rare) case where the server does not support
> mtime, we also have a hard limit on the lifetime of the negative dentry.
>
> Cheers,
>   Trond
>
>
>   

Re: [RFC PATCH] cleaning up negative dentries when remote directory is changed

[Trond Myklebust]

On Thu, 2005-11-24 at 20:06 -0600, Steve French wrote:

> I don't even see any code in my vfs being called on the negative dentry 
> other than
> the lookup on the parent e.g:
>     ls /mnt/modified_dir/deleted_file
> 
> I see that the lookup on modified_dir hits cifs's dentry_revalidate 
> routine but there is
> no call to dentry_revalidate on "deleted_file" ... does nfs export some 
> magic dentry
> routines other than revalidate?

Nope. The d_revalidate() dentry operation is the right place to do this.
There is nothing in the VFS that special-cases revalidation of negative
dentries, so d_revalidate() will indeed be called whenever a cached
entry is looked up.

AFAICS, the reason why CIFS fails to do this is that you have not
initialised dentry->d_op on the negative dentry. IOW: CIFS negative
dentries currently don't have a d_revalidate()...

Cheers,
  Trond

Re: [RFC PATCH] cleaning up negative dentries when remote directory is changed

[Steve French]

Trond Myklebust wrote:
> Nope. The d_revalidate() dentry operation is the right place to do this.
> There is nothing in the VFS that special-cases revalidation of negative
> dentries, so d_revalidate() will indeed be called whenever a cached
> entry is looked up.
>
> AFAICS, the reason why CIFS fails to do this is that you have not
> initialised dentry->d_op on the negative dentry. IOW: CIFS negative
> dentries currently don't have a d_revalidate()...
>
> Cheers,
>   Trond
>
>
>   

Argh ... It looks you are probably right, I am trying adding the dentry 
ops to negative dentries.  I like it when there are obvious reasons for 
strange behavior.

Thanks.