| Message ID | 524f69650901131033p1735d14ej8bed9730004327e9@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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);