[PATCH] fix reiserfs lock inversion of bkl vs inode semaphore

[PATCH] fix reiserfs lock inversion of bkl vs inode semaphore

[Chris Mason]

From: Chris Mason <mason@suse.com>
Subject: fix reiserfs lock inversion of bkl vs inode semaphore

The correct lock ordering is inode lock -> BKL

Signed-off-by: Chris Mason <mason@suse.com>

--- a/fs/reiserfs/file.c	Thu Aug 03 16:18:18 2006 -0400
+++ b/fs/reiserfs/file.c	Thu Aug 03 16:18:18 2006 -0400
@@ -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
--- a/fs/reiserfs/inode.c	Thu Aug 03 16:18:18 2006 -0400
+++ b/fs/reiserfs/inode.c	Thu Aug 03 16:18:28 2006 -0400
@@ -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 */
--- a/fs/reiserfs/ioctl.c	Thu Aug 03 16:18:18 2006 -0400
+++ b/fs/reiserfs/ioctl.c	Thu Aug 03 16:18:18 2006 -0400
@@ -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.  */

-- 

Re: [PATCH] fix reiserfs lock inversion of bkl vs inode semaphore

[Andrew Morton]

On Fri, 4 Aug 2006 10:00:15 -0400
Chris Mason <mason@suse.com> 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.

Re: [PATCH] fix reiserfs lock inversion of bkl vs inode semaphore

[Andrew Morton]

On Fri, 4 Aug 2006 10:00:15 -0400
Chris Mason <mason@suse.com> wrote:

> --- a/fs/reiserfs/inode.c	Thu Aug 03 16:18:18 2006 -0400
> +++ b/fs/reiserfs/inode.c	Thu Aug 03 16:18:28 2006 -0400
> @@ -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.

Re: [PATCH] fix reiserfs lock inversion of bkl vs inode semaphore

[Arjan van de Ven]

On Fri, 2006-08-04 at 15:51 -0700, Andrew Morton wrote:
> On Fri, 4 Aug 2006 10:00:15 -0400
> Chris Mason <mason@suse.com> 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......

-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com