Re: A missing i_mutex in rename? (Linux kernel 2.6.latest)

[Anton Altaparmakov <aia21@cam.ac.uk>]

On Thu, 20 Apr 2006, Al Viro wrote:
> On Wed, Apr 19, 2006 at 01:51:21PM +0100, Anton Altaparmakov wrote:
> > >  - I don't immediately see a race that taking the lock on the victim of
> > >    sys_unlink() solves; however, for symmetry with sys_rmdir(), it seems
> > >    desirable.
> > 
> > I guess the symmetry thing is fair enough.
> 
> Not only; it does, among other things, guarantee that fs can assume that
> ->link() won't race with unlink() (and that link count is protected by
> ->i_mutex, while we are at it).

Ok.

> > ntfs_rename() at the moment looks roughly like this:
> > 
> > if (target_inode) {
> > 	if (S_ISDIR(target_inode->i_mode)
> > 		ntfs_rmdir(target_dir_inode, target_dentry);
> > 	else
> > 		ntfs_unlink(target_dir_inode, target_dentry);
> > }
> > mutex_lock(&old_inode->i_mutex);
> > ntfs_link(old_dentry, target_dir_inode, target_dentry);
> > ntfs_unlink(old_dir_inode, old_dentry);
> > mutex_unlock(&old_inode->i_mutex);
> 
> Have fun dealing with error handling in the above...  Note that failing
> rename() should _NOT_ lead to target disappearing.

Indeed.  But if I do not define a rename operation at all then the mv 
command looses the target if the move fails, too.  So I don't see how the 
native rename needs to be any better other than for standards compliance.

Error recovery is actually pretty easy as if the second ntfs_unlink() 
fails I simply ntfs_unlink() the link I just created with ntfs_link().  
That already works fine.  But yes at the moment I do lose the target if 
either the ntfs_link() or the second ntfs_unlink() fails.

I am planning to fix this though.  It is not too hard because I can more 
or less just ntfs_link() it back in (with a few special casings).

Even if I implement are more "native" rename not using 
ntfs_unlink()/ntfs_link() I still have to remove the target first 
otherwise I cannot insert the new target because you cannot have two 
entries in the B+tree that have identical index keys (the index key is the 
filename itself so the two would be equal).  And I cannot simply replace 
the B+tree index entry because NTFS is not case sensitive so the two 
names (old and new) may not be in the same place in the B+tree.  The only 
thing I can do is to remove the old B+tree index entry and add the new one 
after that fact.  So no matter what I do, I have to be able to recreate 
the old index entry and that is just what ntfs_link() does which is why 
I chose my "simplified rename" approach in the first place.

Thanks a lot for you comments about the locking!

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer, http://www.linux-ntfs.org/