From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751933AbdLMWIG (ORCPT ); Wed, 13 Dec 2017 17:08:06 -0500 Received: from mx2.suse.de ([195.135.220.15]:34767 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750992AbdLMWIE (ORCPT ); Wed, 13 Dec 2017 17:08:04 -0500 From: NeilBrown To: Jeff Layton , linux-fsdevel@vger.kernel.org Date: Thu, 14 Dec 2017 09:07:53 +1100 Cc: linux-kernel@vger.kernel.org, hch@lst.de, neilb@suse.de, bfields@fieldses.org, amir73il@gmail.com, jack@suse.de, viro@zeniv.linux.org.uk Subject: Re: [PATCH 02/19] fs: don't take the i_lock in inode_inc_iversion In-Reply-To: <1513201930.3498.37.camel@kernel.org> References: <20171213142017.23653-1-jlayton@kernel.org> <20171213142017.23653-3-jlayton@kernel.org> <1513201930.3498.37.camel@kernel.org> Message-ID: <87fu8enux2.fsf@notabene.neil.brown.name> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable On Wed, Dec 13 2017, Jeff Layton wrote: > On Wed, 2017-12-13 at 09:20 -0500, Jeff Layton wrote: >> From: Jeff Layton >>=20 >> The rationale for taking the i_lock when incrementing this value is >> lost in antiquity. The readers of the field don't take it (at least >> not universally), so my assumption is that it was only done here to >> serialize incrementors. >>=20 >> If that is indeed the case, then we can drop the i_lock from this >> codepath and treat it as a atomic64_t for the purposes of >> incrementing it. This allows us to use inode_inc_iversion without >> any danger of lock inversion. >>=20 >> Note that the read side is not fetched atomically with this change. >> The assumption here is that that is not a critical issue since the >> i_version is not fully synchronized with anything else anyway. >>=20 >> Signed-off-by: Jeff Layton >> --- >> include/linux/fs.h | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >>=20 >> diff --git a/include/linux/fs.h b/include/linux/fs.h >> index 5001e77342fd..c234fac4bb77 100644 >> --- a/include/linux/fs.h >> +++ b/include/linux/fs.h >> @@ -2136,9 +2136,9 @@ inode_set_iversion_queried(struct inode *inode, co= nst u64 new) >> static inline bool >> inode_maybe_inc_iversion(struct inode *inode, bool force) >> { >> - spin_lock(&inode->i_lock); >> - inode->i_version++; >> - spin_unlock(&inode->i_lock); >> + atomic64_t *ivp =3D (atomic64_t *)&inode->i_version; >> + >> + atomic64_inc(ivp); >> return true; >> } >>=20=20 > > FWIW, I'm not sure this patch is strictly necessary as an interim step. > > Adding the i_lock into the all of the places where we currently just do > inode->i_version++ without properly auditing all of them gave me pause > though. > > In any case, the last patch in the series cleans this nastiness up. Yes, I thought "nastiness" too, and was happy to see it cleaned up. I would have guessed that the purpose of the spinlock was to avoid the risk for torn-reads/writes on 32bit platforms that cannot access a 64bit value atomically. In either case, using atomic64_t is the right thing to do. Thanks, NeilBrown --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEG8Yp69OQ2HB7X0l6Oeye3VZigbkFAloxpLoACgkQOeye3VZi gbnj+xAAkJnJhDT7TwHRYIkr1WasnOmnb6681NAKMjnBa/qjCjpi1GK3LHOUHvSz e/+5Aan3FMUGF4g5egPd5ksQDFEcUlvjaqyPE0ITgjqFF0BuTy+kkdNw0HimRZRG c6rRY+SLZ4jgiF8ak08AAxN2Bc8tiQAEYie9LI4Vmgre6nhOT/WN8hH84KLakyDz KplkkmdbTNPBeZeuv5JPStsBbOrsjqcCElncmcmDFa4EIpV+26sbK/xJTD1cdl6d K0YTrv1n3h2fNoaYbuR/GgFefYdnE2OXs+q/q3beRpJKbrXYsEsQgsmgKBaLOOx0 fIeRguBtgrzmrhGYfBZQY6h/S1SVI+mPqFzv6wkWBYEuMTGSay34Jiholn+AczVc 61nP5cshmqRHMDuvuoYS2J/DboStzVprSGlkVumz3tqgGl+T/DENOd8iTUup871S 0KHDN1uh4/RFTSqowSNgN4OBWxj6GyHT5c8/Q0ZdnEDdRDS5jlsPupt7dxoxnvAm cPOoDStaWK4OZB4IxUYC+rlVQvELM+l8SDJ5TwaQ5Ny/zyn93WJZkxDSQJH/+Uex 8KF9mLOhFg73SXhi5Q+7glMmB4s1Z4Jf74Z/zVMBhmHdT3Lxu9QYdtWAYaWPgM/G 2XkNOZQ6sC7rHNz/D7eF4U5zRItWrR/5CSwaYE8YW2yikoQyyN4= =qBa6 -----END PGP SIGNATURE----- --=-=-=--