[linux-cifs-client] Re: opendir() returns valid DIR* pointer even in case when that directory no longer exists - it has (just) been removed

[Steve French]

On Thu, Jan 8, 2009 at 8:40 PM, Günter Kukkukk <linux at kukkukk.com> wrote:
> Hi Steve, Jeff,
>
> got the following failure notification on irc #samba:
>
> A user was updating from subversion 1.4 to 1.5, where the
> repository is located on a samba share (independent of
> unix extensions = Yes or No).
> svn 1.4 did work, 1.5 does not.
>
> The user did a lot of stracing of subversion - and wrote a
> testapplet to simulate the failing behaviour.
> I've converted the C++ source to C and added some error cases.
>
> When using "./testdir" on a local file system, "result2"
> is always (nil) as expected - cifs vfs behaves different here!
>
>   ./testdir /mnt/cifs/mounted/share
>
> returns a (failing) valid pointer.
>
> Some shell scripting:
> for a in a b c d e f g h i;
> do ./testdir /mnt/cifs/mounted/share; done
>
> and
> for a in a b c d e f g h i;
> do ./testdir /mnt/cifs/mounted/share; sleep 1; done
>
> show different results too ...

The cause of the problem turned out to be stranger than I expected.
The sequence of events is:
1) cifs_rmdir sets the number of links to zero (probably doesn't need
to do both drop_nlink and clear_nlink though, just the clear should be
good enough)

2) rmdir sets the inode's time to zero which would force revalidate to
go over the network if someone tried a lookup on that, but svn was
redoing the getdents before the lookup

3) the app does a getdents on the parent directory, finding an
existing pending search so it uses those cached results and recreates
the dentry/inode for "magic_dir" from the stale results
4) lookup finds the newly created magic_dir's inode and since it was
recently created (inode time is not zero) ... uses it for up to a
second


In the cifs_unlink case we already reset the time on the parent
directory which avoids this problem, but we don't for rmdir (see fix
below) - this should fix the problem

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 5ab9896..da70a54 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1285,6 +1285,11 @@ int cifs_rmdir(struct inode *inode, struct
dentry *direntry)
        cifsInode = CIFS_I(direntry->d_inode);
        cifsInode->time = 0;    /* force revalidate to go get info when
                                   needed */
+       cifsInode = CIFS_I(inode);
+       cifsInode->time = 0;    /* force revalidate to go get info
+                                  on parent directory since link count
+                                  changed and search buffer can no longer
+                                  be cached */
        direntry->d_inode->i_ctime = inode->i_ctime = inode->i_mtime =
                current_fs_time(inode->i_sb);

I wonder if this also would fix the cleanup problem we saw in some
versions of dbench

Gunter,
Thanks - good job on those traces and test case (which made it much
easier to debug).

-- 
Thanks,

Steve