On Fri, Apr 27, 2007 at 06:50:45PM +1000, Lachlan McIlroy wrote:
I don't think this is entirely correct, and it misses some of the
most interesting cases.
I've Cc'ed -fsdevel and Al to get some comments on the more tricky
issues in the rename section at the end of the mail.
The XFS_ILOCK_PARENT uses in xfs_create, xfs_mkdir and xfs_symlink look good.
xfs_lock_dir_and_entry should go away and just become and opencoded
xfs_ilock(dp, XFS_ILOCK_EXCL | XFS_ILOCK_PARENT);
xfs_ilock(ip, XFS_ILOCK_EXCL);
in the two callers, once we made sure to have a sufficient locking
protocol where we always lock the parent before the child.
xfs_lock_dir_and_entry can be totally removed and replaced with just
the two ilock calls if we sort out the locking as proposed in this
mail.
This looks a bit odd to me - the rt inodes are not connected to the
filesystem namespace so the root inode can't really be it's parent.
Why are we locking the root inode so early. Is there a good reason we
don't delay the locking until we're done with the rt inodes?
If not the parent annotation is probably safe beause we never lock
the rt inode at the same time as any other inode, but it at least needs
a big comment describing what's going on.
Now what seems to be completely lacking is any kind of annotation in
xfs_rename.c, which is the most difficult thing to get right for
inode locking because we may have to lock up to four inodes. I suggest
to implement the same locking protocol the the VFS uses for locking
i_mutex, as document in Documentation/filesystems/directory-locking:
Also xfs_lock_inodes lacks any kind of annotation.
Let's start with the xfs_lock_inodes that don't fall into rename or
xfs_lock_dir_and_entry handled above:
- xfs_swap_extents locks two inodes of the same type, but these
could be directories, so there is a chance we can get into
conflicts with the parent->child type locking
- xfs_link locks the source inode and the target directory
inode. vfs locking rule is lock parent, lock source and
we should follow this as it's in line with the directory
before child rule except that the source doesn't always
have to be a child, in which case we don't have a problem
anyway
And now rename gets ugly, we should follow the VFS rules with
the following required adjustments:
- XFS needs both source and target inode (if existing) locked.
Because both must be non-directories sorting by inode number
should be okay
- Doing a lock_rename equivalent for locking the parent directories
requires dentries, but only inodes are passed down from the VFS.
On the other hand they are obviously guranteed to be directories
so i_dentry has exactly one dentry on which we can do the upwards
walk.
s_vfs_rename_mutex is already held by the vfs so we don't need
to do that again.
I'd suggest having a copy of the directory-locking file with the
XFS adjustments somewhere so all this is actually well documented.
- case for source directory == parent directory is trivial.
lock parent
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
