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

[Tyler Hicks <tyhicks@canonical.com>]

[-- 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 --]