From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tyler Hicks Subject: Re: [PATCH] vfs: Correctly set the dir i_mutex lockdep class Date: Thu, 10 Nov 2011 09:28:49 -0600 Message-ID: <20111110152848.GA5421@boyd> References: <1320907500-3767-1-git-send-email-tyhicks@canonical.com> <20111110143319.GB26644@quack.suse.cz> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="EVF5PPMfhYS0aIcm" Cc: Alexander Viro , John Johansen , Josh Boyer , Peter Zijlstra , Ingo Molnar , linux-fsdevel@vger.kernel.org, Mark Fasheh , Joel Becker , ocfs2-devel@oss.oracle.com To: Jan Kara Return-path: Received: from youngberry.canonical.com ([91.189.89.112]:57068 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933707Ab1KJP3A (ORCPT ); Thu, 10 Nov 2011 10:29:00 -0500 Content-Disposition: inline In-Reply-To: <20111110143319.GB26644@quack.suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: --EVF5PPMfhYS0aIcm Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. > >=20 > > 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. > >=20 > > 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. > >=20 > > 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. >=20 > Anyway, thanks for catching this! You can add > Reviewed-by: Jan Kara Thanks for the review! Tyler >=20 > Honza > > Signed-off-by: Tyler Hicks > > --- > > fs/inode.c | 3 +-- > > 1 files changed, 1 insertions(+), 2 deletions(-) > >=20 > > 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 =3D inode->i_sb->s_type; > > =20 > > /* 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 > > */ > > --=20 > > 1.7.5.4 > >=20 > --=20 > Jan Kara > SUSE Labs, CR --EVF5PPMfhYS0aIcm Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iQIcBAEBCgAGBQJOu+2wAAoJENaSAD2qAscKes0QAMljGIYykxThOWSnKyS/1Tyg I6+KIQ2apx7I1uzXzFX6uecppc7wgObjKLwDLo/dvc48RTcly7oqYD69Y6K7dLbf GS/H6QAl/ly3qxjrKJxVbUQ+3G1BaKp+TNeb2ZgAAwfuK11UtzrZp4whrr2I8X44 UrwQIWiqVl6C1KeisjpgY6Xnv8AH2DzqdiKKMUb97gaFfyUlttFlyKQObGhoBykH V0defPOcDUG45AT3AqMMtrXsg0gvJrKMxmpoR8l+ccNID6iISxfzFGhEWpyj81yz IeikOKyuFIXi6taj/3v9x0WELo7C/LbX2oPpFpzAnv0CD25g4fplXsFp46XUyE/I tRas2FnMNcoiuJHXwtZU6isVaAbYEzMCMjL729RkWmhAKI8sVTyYznkh9iyf3hGH hpNoxi7jSpCNYpumrVmZ8WzLEP3nSwiUnQUZSi3k1pAdt0GRcolUw89xh5oLhngN EoMvDrLdjAdOr9ERmEsU2wW8o/axASIJ5xU+K+zsnSh//dSYawcgUCIJxbcdm4x0 EOuwe5R2tzMbzwoMIsNeavUQUaI8MM7Rw5FTE3X7VOQWLik0sENIUt1ujz+uDJx5 5E7Di3Mjj9Op+yc3/22f/0GmgqwOW9iBFA5Q63WbTo4sQDyhlDhxAOs4gs4Tytjy HY36lVFzpYIoSpUOu68g =NJu0 -----END PGP SIGNATURE----- --EVF5PPMfhYS0aIcm--