From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ingo Molnar Subject: Re: [LOCKDEP] xfs: possible recursive locking detected Date: Tue, 4 Jul 2006 11:57:43 +0200 Message-ID: <20060704095743.GA21480@elte.hu> References: <20060704004116.GA7612@martell.zuzino.mipt.ru> <20060704011858.GG1605@parisc-linux.org> <20060704112503.H1495869@wobbly.melbourne.sgi.com> <20060704063225.GA2752@elte.hu> <20060704084143.GA12931@elte.hu> <20060704191100.C1497438@wobbly.melbourne.sgi.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Alexey Dobriyan , Matthew Wilcox , linux-fsdevel@vger.kernel.org, xfs@oss.sgi.com, Arjan van de Ven Return-path: Received: from mx3.mail.elte.hu ([157.181.1.138]:2962 "EHLO mx3.mail.elte.hu") by vger.kernel.org with ESMTP id S932197AbWGDKC2 (ORCPT ); Tue, 4 Jul 2006 06:02:28 -0400 To: Nathan Scott Content-Disposition: inline In-Reply-To: <20060704191100.C1497438@wobbly.melbourne.sgi.com> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org * Nathan Scott wrote: > > flag-passing into an opaque function (such as xfs_ilock), just to have > > them untangled in xfs_ilock(): > > > > if (lock_flags & XFS_IOLOCK_EXCL) { > > mrupdate(&ip->i_iolock); > > } else if (lock_flags & XFS_IOLOCK_SHARED) { > > mraccess(&ip->i_iolock); > > } > > if (lock_flags & XFS_ILOCK_EXCL) { > > mrupdate(&ip->i_lock); > > } else if (lock_flags & XFS_ILOCK_SHARED) { > > mraccess(&ip->i_lock); > > } > > > > is pretty inefficient too - there are 85 calls to xfs_ilock(), and > > 74 of them have static flags. > > Right... but that leaves plenty that don't, and they're not simple to > change. There are generic routines that need to be called from > different contexts with different locking requirements (xfs_iget). the main variation in xfs_iget() is whether we lock the inode read-write, read-only or not at all, correct? (XFS_ILOCK_EXCL, XFS_ILOCK_SHARED and 0) That could be cleaned up the following way: - rename the current xfs_iget() to __xfs_iget() and remove ilock locking from it. - add 3 new APIs: xfs_iget_read(), xfs_iget_write() and xfs_iget_nolock(): - xfs_iget_read() just calls __xfs_iget() and does a down_read() if the inode was looked up successfully. - xfs_iget_write() does the same but with down_write() - xfs_iget_nolock() is just an alias to __xfs_iget(). - update all 13 uses of xfs_iget() to one of the 3 API variants - [ there might be other details i missed, but this seems to be the rough list of things to do. ] NOTE: since the majority (9 out of 13) of xfs_iget() uses are for the 'no lock' variant, this construction of functions, besides making the code more readable, _further_ reduces overhead, because there is no ilock-flags checking overhead in __xfs_iget() anymore. Ingo