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

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@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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [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@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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html