svn commit: r226967 - head/sys/ufs/ufs

[John Baldwin]

On Monday, March 05, 2012 4:43:54 pm Peter Holm wrote:
> On Mon, Mar 05, 2012 at 12:38:57PM -0500, John Baldwin wrote:
> > On Saturday, March 03, 2012 11:38:43 am Peter Holm wrote:
> > > On Sat, Mar 03, 2012 at 01:01:32AM -0500, Rick Macklem wrote:
> > > > John Baldwin wrote:
> > > > > On Friday, March 02, 2012 8:29:21 am Peter Holm wrote:
> > > > > > On Thu, Mar 01, 2012 at 04:47:41PM -0500, John Baldwin wrote:
> > > > > > > On Monday, October 31, 2011 11:01:47 am Peter Holm wrote:
> > > > > > > > Author: pho
> > > > > > > > Date: Mon Oct 31 15:01:47 2011
> > > > > > > > New Revision: 226967
> > > > > > > > URL: http://svn.freebsd.org/changeset/base/226967
> > > > > > > >
> > > > > > > > Log:
> > > > > > > >   The kern_renameat() looks up the fvp using the DELETE flag,
> > > > > > > >   which causes
> > > > > > > >   the removal of the name cache entry for fvp.
> > > > > > > >
> > > > > > > >   Reported by: Anton Yuzhaninov <citrin citrin ru>
> > > > > > > >   In collaboration with: kib
> > > > > > > >   MFC after: 1 week
> > > > > > > >
> > > > > > > > Modified:
> > > > > > > >   head/sys/ufs/ufs/ufs_vnops.c
> > > > > > >
> > > > > > > So I ran into this at work recently, and even this fix applied I
> > > > > > > was still
> > > > > > > seeing rename()'s that were seemingly not taking effect. After
> > > > > > > getting some
> > > > > > > extra KTR traces, I figured out that the same purge needs to be
> > > > > > > applied to the
> > > > > > > destination vnode. Specifically, the issue I ran into was that was
> > > > > > > renaming
> > > > > > > 'foo' to 'bar', but lookups for 'bar' were still returning the old
> > > > > > > file. The
> > > > > > > reason was that a lookup after the namei(RENAME) of the
> > > > > > > destination while
> > > > > > > ufs_rename() had its locks dropped was readding the name cache
> > > > > > > entry for
> > > > > > > 'bar', and then a cache_lookup() of 'bar' would return the old
> > > > > > > vnode as long
> > > > > > > as that vnode was valid (e.g. if it had a link in another
> > > > > > > location, or other
> > > > > > > processes had an open file descriptor for it). I'm currently
> > > > > > > testing the
> > > > > > > patch below:
> > > > > > >
> > > > > >
> > > > > > I now have a scenario that fails, but not quite the same way you
> > > > > > describe.
> > > > > >
> > > > > > It looks like this:
> > > > > >
> > > > > > touch file1
> > > > > > echo xxx > file2
> > > > > > rename(file1, file2)
> > > > > >
> > > > > > A different process performs stat() on both files in a tight loop.
> > > > > >
> > > > > > Once in a while I observe that a stat() of file2, after the rename,
> > > > > > returns a link count of zero. Size is zero as expected, but the
> > > > > > inode
> > > > > > number of file2 is unchanged.
> > > > > 
> > > > Peter, were you doing a stat() using the file name, or an fstat()?
> > > > (Using stat() with afile name might explain it, maybe??)
> > > > 
> > > 
> > > Yes. Switching to open()/fstat() of the "from" file in the loop, makes
> > > the cache problem go away.
> > 
> > Using fstat avoids the changes of getting a stale name cache, so it just 
> > avoids the race altogether.  However, there is no reason I can think of why 
> > stat() should ever give you can inconsistent view of attributes.  You should 
> > always get a consistent snapshot of attributes.
> > 
> 
> Maybe my test scenario is broken, but it sure looks like the link
> count is zero, after the rename.

Hmmm, I think it is more the size of zero I'm surprised at.  Presumably a vnode
whose only references are open file descriptors would have a link count of zero,
and so if this race were to occur, I would expect to see that.  However, I would
expect the file to have the 'xxx' contents, so a non-zero file.

Hmm, I wonder what happens if the only link to a file are hold counts (e.g. name
cache entries), but the usecount drops to zero.  Will UFS recycle the i-node in
that case?  If so, perhaps that could explain it?  Or even better, perhaps because
the usecount is zero, when the rename happens, UFS goes ahead and zeros out the
file.  That probably explains it then.  (Clearly we need to add a 4th reference
count to vnodes as it is far too simple as-is..)

> 
> $ ./r9.sh
> FAIL: old and new "To" inode number is identical
> stat() before the rename():
> fromFile.log    : ino =    4, nlink = 1, size = 0
> toFile.log.00065: ino =   70, nlink = 1, size = 8
> 
> stat() after the rename():
> toFile.log.00065: ino =   70, nlink = 0, size = 0
> $ 
> 
> This on r232558 with r232401 reverted. No problem on a pristine
> HEAD of cause.
> 
> http://people.freebsd.org/~pho/r9.sh
> 
> - Peter
> 

-- 
John Baldwin