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

[Günter Kukkukk]

Am Dienstag, 13. Januar 2009 schrieb 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).
> 

Hi Steve,
your patch works here on kernels
  - 2.6.22.18-0.2-default
  - 2.6.28
it seems to be an outstanding one, which is fixed now. :-)

I've send your patch to the bug reporter (to build a new cifs vfs
module...) - till now no response whether subversion 1.5 is failing
or not.
I'm sure the bug is fixed...
Cheers,  Günter