On Thu, Jan 8, 2009 at 8:40 PM, Günter Kukkukk <linux@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
