cleaning up negative dentries when remote directory is changed

RFC PATCH In review Steve French 2 participants 5 messages Nov 25, 2005 VFS

Share & tools

Steve French

author

2005-11-25 01:32 UTC #0 lore raw

RFC PATCH 1/1 cleaning up negative dentries when remote directory is changed

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?

fs/cifs/inode.c +12 -8

index 05b5258..45b67e9 100644
@@ -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); */

Trond Myklebust

2005-11-25 01:55 UTC #1 lore raw

Quoted 9 lines · Steve French

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.

Quoted 4 lines

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

Steve French

author

2005-11-25 02:06 UTC #2 lore raw

Quoted 13 lines · Trond Myklebust

Quoted 3 lines · Steve French
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.

Quoted 9 lines

Quoted 2 lines
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?

Quoted 8 lines

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

Trond Myklebust

2005-11-25 02:18 UTC #3 lore raw

Quoted 10 lines · Steve French

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

Steve French

author

2005-11-25 02:28 UTC #4 lore raw

Quoted 5 lines · Trond Myklebust

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.

Quoted 9 lines

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.