[PATCH] vfs: Correctly set the dir i_mutex lockdep class

[PATCH] vfs: Correctly set the dir i_mutex lockdep class

[Tyler Hicks]

9a7aa12f3911853a introduced additional logic around setting the i_mutex
lockdep class for directory inodes. The idea was that some filesystems
may want their own special lockdep class for different directory
inodes and calling unlock_new_inode() should not clobber one of
those special classes.

I believe that the added conditional, around the *negated* return value
of lockdep_match_class(), caused directory inodes to be placed in the
wrong lockdep class.

inode_init_always() sets the i_mutex lockdep class with i_mutex_key for
all inodes. If the filesystem did not change the class during inode
initialization, then the conditional mentioned above was false and the
directory inode was incorrectly left in the non-directory lockdep class.
If the filesystem did set a special lockdep class, then the conditional
mentioned above was true and that class was clobbered with
i_mutex_dir_key.

This patch removes the negation from the conditional so that the i_mutex
lockdep class is properly set for directory inodes. Special classes are
preserved and directory inodes with unmodified classes are set with
i_mutex_dir_key.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
 fs/inode.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index ee4e66b..9d01a0d 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -855,8 +855,7 @@ void lockdep_annotate_inode_mutex_key(struct inode *inode)
 		struct file_system_type *type = inode->i_sb->s_type;
 
 		/* Set new key only if filesystem hasn't already changed it */
-		if (!lockdep_match_class(&inode->i_mutex,
-		    &type->i_mutex_key)) {
+		if (lockdep_match_class(&inode->i_mutex, &type->i_mutex_key)) {
 			/*
 			 * ensure nobody is actually holding i_mutex
 			 */
-- 
1.7.5.4

Re: [PATCH] vfs: Correctly set the dir i_mutex lockdep class

[Jan Kara]

On Thu 10-11-11 00:45:00, Tyler Hicks wrote:
> 9a7aa12f3911853a introduced additional logic around setting the i_mutex
> lockdep class for directory inodes. The idea was that some filesystems
> may want their own special lockdep class for different directory
> inodes and calling unlock_new_inode() should not clobber one of
> those special classes.
> 
> I believe that the added conditional, around the *negated* return value
> of lockdep_match_class(), caused directory inodes to be placed in the
> wrong lockdep class.
> 
> inode_init_always() sets the i_mutex lockdep class with i_mutex_key for
> all inodes. If the filesystem did not change the class during inode
> initialization, then the conditional mentioned above was false and the
> directory inode was incorrectly left in the non-directory lockdep class.
> If the filesystem did set a special lockdep class, then the conditional
> mentioned above was true and that class was clobbered with
> i_mutex_dir_key.
> 
> This patch removes the negation from the conditional so that the i_mutex
> lockdep class is properly set for directory inodes. Special classes are
> preserved and directory inodes with unmodified classes are set with
> i_mutex_dir_key.
  Duh, you are right. I wonder how come this did not trigger any lockdep
messages during my testing with ocfs2 for which I wrote the patch...

Anyway, thanks for catching this! You can add
Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> ---
>  fs/inode.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index ee4e66b..9d01a0d 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -855,8 +855,7 @@ void lockdep_annotate_inode_mutex_key(struct inode *inode)
>  		struct file_system_type *type = inode->i_sb->s_type;
>  
>  		/* Set new key only if filesystem hasn't already changed it */
> -		if (!lockdep_match_class(&inode->i_mutex,
> -		    &type->i_mutex_key)) {
> +		if (lockdep_match_class(&inode->i_mutex, &type->i_mutex_key)) {
>  			/*
>  			 * ensure nobody is actually holding i_mutex
>  			 */
> -- 
> 1.7.5.4
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] vfs: Correctly set the dir i_mutex lockdep class

[Tyler Hicks]

[-- Attachment #1: Type: text/plain, Size: 2743 bytes --]

On 2011-11-10 15:33:19, Jan Kara wrote:
> On Thu 10-11-11 00:45:00, Tyler Hicks wrote:
> > 9a7aa12f3911853a introduced additional logic around setting the i_mutex
> > lockdep class for directory inodes. The idea was that some filesystems
> > may want their own special lockdep class for different directory
> > inodes and calling unlock_new_inode() should not clobber one of
> > those special classes.
> > 
> > I believe that the added conditional, around the *negated* return value
> > of lockdep_match_class(), caused directory inodes to be placed in the
> > wrong lockdep class.
> > 
> > inode_init_always() sets the i_mutex lockdep class with i_mutex_key for
> > all inodes. If the filesystem did not change the class during inode
> > initialization, then the conditional mentioned above was false and the
> > directory inode was incorrectly left in the non-directory lockdep class.
> > If the filesystem did set a special lockdep class, then the conditional
> > mentioned above was true and that class was clobbered with
> > i_mutex_dir_key.
> > 
> > This patch removes the negation from the conditional so that the i_mutex
> > lockdep class is properly set for directory inodes. Special classes are
> > preserved and directory inodes with unmodified classes are set with
> > i_mutex_dir_key.
>   Duh, you are right. I wonder how come this did not trigger any lockdep
> messages during my testing with ocfs2 for which I wrote the patch...

Good question. Adding the ocfs2 maintainers and devel list to cc, as
this patch is likely to change up their lockdep warnings.

I expect this will also solve many of the lockdep issues around the
order of i_mutex and mmap_sem being taken in files verse directories.
That's why I stumbled across it.

> 
> Anyway, thanks for catching this! You can add
> Reviewed-by: Jan Kara <jack@suse.cz>

Thanks for the review!

Tyler

> 
> 								Honza
> > Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> > ---
> >  fs/inode.c |    3 +--
> >  1 files changed, 1 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/inode.c b/fs/inode.c
> > index ee4e66b..9d01a0d 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -855,8 +855,7 @@ void lockdep_annotate_inode_mutex_key(struct inode *inode)
> >  		struct file_system_type *type = inode->i_sb->s_type;
> >  
> >  		/* Set new key only if filesystem hasn't already changed it */
> > -		if (!lockdep_match_class(&inode->i_mutex,
> > -		    &type->i_mutex_key)) {
> > +		if (lockdep_match_class(&inode->i_mutex, &type->i_mutex_key)) {
> >  			/*
> >  			 * ensure nobody is actually holding i_mutex
> >  			 */
> > -- 
> > 1.7.5.4
> > 
> -- 
> Jan Kara <jack@suse.cz>
> SUSE Labs, CR

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] vfs: Correctly set the dir i_mutex lockdep class

[Joel Becker]

On Thu, Nov 10, 2011 at 09:28:49AM -0600, Tyler Hicks wrote:
> On 2011-11-10 15:33:19, Jan Kara wrote:
> > On Thu 10-11-11 00:45:00, Tyler Hicks wrote:
> > > This patch removes the negation from the conditional so that the i_mutex
> > > lockdep class is properly set for directory inodes. Special classes are
> > > preserved and directory inodes with unmodified classes are set with
> > > i_mutex_dir_key.
> >   Duh, you are right. I wonder how come this did not trigger any lockdep
> > messages during my testing with ocfs2 for which I wrote the patch...
> 
> Good question. Adding the ocfs2 maintainers and devel list to cc, as
> this patch is likely to change up their lockdep warnings.

	We don't set special classes for dir->i_mutex.  We set special
classes on our DLM stuff, and we do it for our system files, but not for
regular directories.

Joel


-- 

Life's Little Instruction Book #356

	"Be there when people need you."

			http://www.jlbec.org/
			jlbec@evilplan.org

[PATCH][RESEND] vfs: Correctly set the dir i_mutex lockdep class

[Tyler Hicks]

9a7aa12f3911853a introduced additional logic around setting the i_mutex
lockdep class for directory inodes. The idea was that some filesystems
may want their own special lockdep class for different directory
inodes and calling unlock_new_inode() should not clobber one of
those special classes.

I believe that the added conditional, around the *negated* return value
of lockdep_match_class(), caused directory inodes to be placed in the
wrong lockdep class.

inode_init_always() sets the i_mutex lockdep class with i_mutex_key for
all inodes. If the filesystem did not change the class during inode
initialization, then the conditional mentioned above was false and the
directory inode was incorrectly left in the non-directory lockdep class.
If the filesystem did set a special lockdep class, then the conditional
mentioned above was true and that class was clobbered with
i_mutex_dir_key.

This patch removes the negation from the conditional so that the i_mutex
lockdep class is properly set for directory inodes. Special classes are
preserved and directory inodes with unmodified classes are set with
i_mutex_dir_key.

Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---

Hi Al - I thought I'd resend this fix after seeing Mimi's RFC for more
filesystems to use lockdep_annotate_inode_mutex_key(). Currently, that function
doesn't do what it is supposed to do.

 fs/inode.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index ee4e66b..9d01a0d 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -855,8 +855,7 @@ void lockdep_annotate_inode_mutex_key(struct inode *inode)
 		struct file_system_type *type = inode->i_sb->s_type;
 
 		/* Set new key only if filesystem hasn't already changed it */
-		if (!lockdep_match_class(&inode->i_mutex,
-		    &type->i_mutex_key)) {
+		if (lockdep_match_class(&inode->i_mutex, &type->i_mutex_key)) {
 			/*
 			 * ensure nobody is actually holding i_mutex
 			 */
-- 
1.7.5.4

Re: [PATCH][BUGFIX][RESEND] vfs: Correctly set the dir i_mutex lockdep class

[Tyler Hicks]

[-- Attachment #1: Type: text/plain, Size: 2888 bytes --]

On 2011-12-12 10:02:30, Tyler Hicks wrote:
> 9a7aa12f3911853a introduced additional logic around setting the i_mutex
> lockdep class for directory inodes. The idea was that some filesystems
> may want their own special lockdep class for different directory
> inodes and calling unlock_new_inode() should not clobber one of
> those special classes.
> 
> I believe that the added conditional, around the *negated* return value
> of lockdep_match_class(), caused directory inodes to be placed in the
> wrong lockdep class.
> 
> inode_init_always() sets the i_mutex lockdep class with i_mutex_key for
> all inodes. If the filesystem did not change the class during inode
> initialization, then the conditional mentioned above was false and the
> directory inode was incorrectly left in the non-directory lockdep class.
> If the filesystem did set a special lockdep class, then the conditional
> mentioned above was true and that class was clobbered with
> i_mutex_dir_key.
> 
> This patch removes the negation from the conditional so that the i_mutex
> lockdep class is properly set for directory inodes. Special classes are
> preserved and directory inodes with unmodified classes are set with
> i_mutex_dir_key.
> 
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
> 
> Hi Al - I thought I'd resend this fix after seeing Mimi's RFC for more
> filesystems to use lockdep_annotate_inode_mutex_key(). Currently, that function
> doesn't do what it is supposed to do.

Hi Al - Just following up again to see if I can get any comments on this
bug fix in hopes of getting it picked up soon.

Searching back through list archives, I realize there have been debates
about the locking order of i_mutex vs mmap_sem among directory and
regular inodes, which I imagine is the reason for this bit of code.  But
either way, the lock class setting of directory inodes is broken here.

Adding hch and akpm to cc for extra eyes/comments.

Tyler

> 
>  fs/inode.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index ee4e66b..9d01a0d 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -855,8 +855,7 @@ void lockdep_annotate_inode_mutex_key(struct inode *inode)
>  		struct file_system_type *type = inode->i_sb->s_type;
>  
>  		/* Set new key only if filesystem hasn't already changed it */
> -		if (!lockdep_match_class(&inode->i_mutex,
> -		    &type->i_mutex_key)) {
> +		if (lockdep_match_class(&inode->i_mutex, &type->i_mutex_key)) {
>  			/*
>  			 * ensure nobody is actually holding i_mutex
>  			 */
> -- 
> 1.7.5.4
> 
> --
> 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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]