On Mon, May 29, 2023 at 02:41:31PM +0200, Jan Kara wrote:
On Fri 26-05-23 11:45:15, Christian Brauner wrote:
On Thu, May 25, 2023 at 12:16:10PM +0200, Jan Kara wrote:
Currently the locking order of inode locks for directories that are not in ancestor relationship is not defined because all operations that needed to lock two directories like this were serialized by sb->s_vfs_rename_mutex. However some filesystems need to lock two subdirectories for RENAME_EXCHANGE operations and for this we need the locking order established even for two tree-unrelated directories. Provide a helper function lock_two_inodes() that establishes lock ordering for any two inodes and use it in lock_two_directories().
CC: stable@vger.kernel.org Signed-off-by: Jan Kara jack@suse.cz
fs/inode.c | 34 ++++++++++++++++++++++++++++++++++ fs/internal.h | 2 ++ fs/namei.c | 4 ++-- 3 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/fs/inode.c b/fs/inode.c index 577799b7855f..2015fa50d34a 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -1103,6 +1103,40 @@ void discard_new_inode(struct inode *inode) } EXPORT_SYMBOL(discard_new_inode); +/**
- lock_two_inodes - lock two inodes (may be regular files but also dirs)
- Lock any non-NULL argument. The caller must make sure that if he is passing
- in two directories, one is not ancestor of the other. Zero, one or two
- objects may be locked by this function.
- @inode1: first inode to lock
- @inode2: second inode to lock
- @subclass1: inode lock subclass for the first lock obtained
- @subclass2: inode lock subclass for the second lock obtained
- */
+void lock_two_inodes(struct inode *inode1, struct inode *inode2,
unsigned subclass1, unsigned subclass2)+{
- if (!inode1 || !inode2)
goto lock;Before this change in
lock_two_nondirectories(struct inode *inode1, struct inode *inode2)
the swap() would cause the non-NULL inode to always be locked with I_MUTEX_NONDIR2. Now it can be either I_MUTEX_NORMAL or I_MUTEX_NONDIR2. Is that change intentional?
Kind of. I don't think we really care so I didn't bother to complicate the code for this. If you think keeping the lockdep class consistent is worth it, I can modify the patch...
Either a short comment or consistent lockdep class would be nice. I know it probably doesn't matter much but otherwise someone may end up wondering whether that's ok or not.