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

Anton Altaparmakov
2006-04-19T12:51:31Z

Hi Matthew,

Thanks for the quick reply.

On Wed, 19 Apr 2006, Matthew Wilcox wrote:
> On Wed, Apr 19, 2006 at 11:50:00AM +0100, Anton Altaparmakov wrote:
> > Both sys_unlink()/sys_rmdir() and sys_link() all end up taking the i_mutex
> > on all parent directories and source/destination inodes before calling
> > into the file system inode operations.
> >
> > sys_rename() OTOH, does not take i_mutex on the old inode. It only takes
> > i_mutex on the two parent directories and on the target inode if it
> > exists.
> >
> > Why is this? To me it seems that either sys_rename() must take i_mutex on
> > the old inode or sys_unlink()/sys_rmdir(), sys_link(), etc do not need to
> > hold the i_mutex.
> >
> > What am I missing?
>
> I believe the current locking scheme to be correct. Reading
> Documentation/filesystems/directory-locking and pondering for a few
> minutes leads me to the following conclusions:
>
> - sys_rmdir() must take the lock on the parent directory and on the
> victim. If a different process is trying to create a file in the
> victim, sys_rmdir() mustn't race with it.

Agreed.

> - 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.

> - sys_link() needs to lock the target to be sure it isn't removed and
> replaced with a directory in the meantime.

Agreed.

> - sys_rename() does not need to lock the old inode. Since the parent
> is already locked, the old inode can't be removed/renamed by a racing
> process. It doesn't matter if something's created or deleted from
> within the old inode (if it's a directory), unlike rmdir(). It
> doesn't need to be protected from a sys_link() race.

Agreed.

> If you need to lock the old inode inside ntfs for your own consistency
> purposes, that looks like it should be fine, but the VFS doesn't need to
> lock it for you.

Great, thanks. That was my own conclusion also but it never hurts to be
sure. (-:

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);

Which is incredibly inefficient but very simple and works (with minimal
special casing in ntfs_link() and ntfs_unlink() mostly so if old_inode is
a directory we never get a link count greater one on the VFS inode) and I
doubt sys_rename() is a very often invoked system call. Normally, a
sys_link() and sys_unlink() would take i_mutex on old_inode as shown in
above code which is why I was wondering whether I should take it as shown
above or whether I can just not worry and not take yet another lock in a
code path where tons of locks are already being taken and released.

My conclusion is that the above code is safe even if I remove the
mutex_lock()/mutex_unlock() around the ntfs_link()/ntfs_unlink() given
that sys_unlink() only takes the lock for symmetry reasons and
sys_link()'s need for locking is taken care of by the fact that
sys_rename() has the lock on both parent directory inodes.

Would you agree?

Thanks a lot in advance!