Skip to contentSkip to conversation

fix reiserfs lock inversion of bkl vs inode semaphore

PATCH In review Chris Mason 3 participants 4 messages Aug 4, 2006 – Aug 5, 2006 VFS

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 fix reiserfs lock inversion of bkl vs inode semaphore

From: Chris Mason <[email protected]>
Subject: fix reiserfs lock inversion of bkl vs inode semaphore

The correct lock ordering is inode lock -> BKL

fs/reiserfs/file.c +1 -1
@@ -48,8 +48,8 @@ static int reiserfs_file_release(struct 
 		return 0;
 	}
 
+	mutex_lock(&inode->i_mutex);
 	reiserfs_write_lock(inode->i_sb);
-	mutex_lock(&inode->i_mutex);
 	/* freeing preallocation only involves relogging blocks that
 	 * are already in the current transaction.  preallocation gets
 	 * freed at the end of each transaction, so it is impossible for
fs/reiserfs/inode.c +2 -2
@@ -35,11 +35,11 @@ void reiserfs_delete_inode(struct inode 
 
 	truncate_inode_pages(&inode->i_data, 0);
 
-	reiserfs_write_lock(inode->i_sb);
 
 	/* The = 0 happens when we abort creating a new inode for some reason like lack of space.. */
 	if (!(inode->i_state & I_NEW) && INODE_PKEY(inode)->k_objectid != 0) {	/* also handles bad_inode case */
 		mutex_lock(&inode->i_mutex);
+		reiserfs_write_lock(inode->i_sb);
 
 		reiserfs_delete_xattrs(inode);
 
@@ -75,7 +75,7 @@ void reiserfs_delete_inode(struct inode 
 								 * about an error here */
 	} else {
 		/* no object items are in the tree */
-		;
+		reiserfs_write_lock(inode->i_sb);
 	}
       out:
 	clear_inode(inode);	/* note this must go after the journal_end to prevent deadlock */
fs/reiserfs/ioctl.c +1 -1
@@ -116,12 +116,12 @@ static int reiserfs_unpack(struct inode 
 	if (REISERFS_I(inode)->i_flags & i_nopack_mask) {
 		return 0;
 	}
-	reiserfs_write_lock(inode->i_sb);
 
 	/* we need to make sure nobody is changing the file size beneath
 	 ** us
 	 */
 	mutex_lock(&inode->i_mutex);
+	reiserfs_write_lock(inode->i_sb);
 
 	write_from = inode->i_size & (blocksize - 1);
 	/* if we are on a block boundary, we are already unpacked.  */
#1 lore raw
Quoted 1 line · On Fri, 4 Aug 2006 10:00:15 -0400 Chris Mason <[email protected]> wrote:

The correct lock ordering is inode lock -> BKL

hm. With CONFIG_PREEMPT_BKL=y we should be getting lockdep warnings and deadlocks, shouldn't we? I recall seeing reports of neither.

#2 lore raw
Quoted 7 lines · Andrew Morton
Quoted 1 line · On Fri, 4 Aug 2006 10:00:15 -0400 Chris Mason <[email protected]> wrote:

The correct lock ordering is inode lock -> BKL

hm. With CONFIG_PREEMPT_BKL=y we should be getting lockdep warnings and deadlocks, shouldn't we? I recall seeing reports of neither.

hmmmm my mind is half blank at this (no coffee yet) but something tells me the BKL is special cased in lockdep because of it's magic and very undefined semantics......

Signature · 1 line
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
#3 lore raw
In reply to Chris Mason

fs/reiserfs/inode.c void reiserfs_delete_inode(struct inode · line 42

Quoted 12 lines · On Fri, 4 Aug 2006 10:00:15 -0400 Chris Mason <[email protected]> wrote:
fs/reiserfs/inode.c +1 -1
@@ -35,11 +35,11 @@ void reiserfs_delete_inode(struct inode 
 
 	truncate_inode_pages(&inode->i_data, 0);
 
-	reiserfs_write_lock(inode->i_sb);
 
 	/* The = 0 happens when we abort creating a new inode for some reason like lack of space.. */
 	if (!(inode->i_state & I_NEW) && INODE_PKEY(inode)->k_objectid != 0) {	/* also handles bad_inode case */
 		mutex_lock(&inode->i_mutex);
+		reiserfs_write_lock(inode->i_sb);

zam has removed the mutex_lock() from reiserfs_delete_inode() so I dropped the changes to this function altogether.