Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756453AbYHZCqM (ORCPT ); Mon, 25 Aug 2008 22:46:12 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753638AbYHZCp4 (ORCPT ); Mon, 25 Aug 2008 22:45:56 -0400 Received: from ipmail01.adl6.internode.on.net ([203.16.214.146]:9453 "EHLO ipmail01.adl6.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753721AbYHZCpx (ORCPT ); Mon, 25 Aug 2008 22:45:53 -0400 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: AtACAAwLs0h5LD0wiGdsb2JhbACSLAEBAQ8gpCWBaw X-IronPort-AV: E=Sophos;i="4.32,267,1217773800"; d="scan'208";a="180334682" Date: Tue, 26 Aug 2008 12:45:47 +1000 From: Dave Chinner To: Christoph Hellwig Cc: Peter Zijlstra , Lachlan McIlroy , Daniel J Blueman , Linux Kernel , xfs@oss.sgi.com Subject: Re: [2.6.27-rc4] XFS i_lock vs i_iolock... Message-ID: <20080826024547.GX5706@disturbed> Mail-Followup-To: Christoph Hellwig , Peter Zijlstra , Lachlan McIlroy , Daniel J Blueman , Linux Kernel , xfs@oss.sgi.com References: <6278d2220808221412x28f4ac5dl508884c8030b364a@mail.gmail.com> <20080825010213.GO5706@disturbed> <48B21507.9050708@sgi.com> <20080825035542.GR5706@disturbed> <1219647573.20732.28.camel@twins> <20080825215532.GB28188@lst.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20080825215532.GB28188@lst.de> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3126 Lines: 95 On Mon, Aug 25, 2008 at 11:55:32PM +0200, Christoph Hellwig wrote: > On Mon, Aug 25, 2008 at 08:59:33AM +0200, Peter Zijlstra wrote: > > How can you take two locks in one go? It seems to me you always need to > > take them one after another, and as soon as you do that, you have > > ordering constraints. > > Yes, you would. Except that in all other places we only have a single > iolock involved, so the ordering of the second iolock and second ilock > don't matter. > > Because of that I think declaring that xfs_lock_two_inodes can just > lock on lock type at a time might be the better solution. Agreed. Patch below. Cheers, Dave. -- Dave Chinner david@fromorbit.com XFS: prevent lockdep false positives when locking two inodes If we call xfs_lock_two_inodes() to grab both the iolock and the ilock, then drop the ilocks on both inodes, then grab them again (as xfs_swap_extents() does) then lockdep will report a locking order problem. This is a false positive. To avoid this, disallow xfs_lock_two_inodes() fom locking both inode locks at once - force calers to make two separate calls. This means that nested dropping and regaining of the ilocks will retain the same lockdep subclass and so lockdep will not see anything wrong with this code. Signed-off-by: Dave Chinner --- fs/xfs/xfs_dfrag.c | 9 ++++++++- fs/xfs/xfs_vnodeops.c | 10 ++++++++++ 2 files changed, 18 insertions(+), 1 deletions(-) diff --git a/fs/xfs/xfs_dfrag.c b/fs/xfs/xfs_dfrag.c index 760f4c5..75b0cd4 100644 --- a/fs/xfs/xfs_dfrag.c +++ b/fs/xfs/xfs_dfrag.c @@ -149,7 +149,14 @@ xfs_swap_extents( sbp = &sxp->sx_stat; - xfs_lock_two_inodes(ip, tip, lock_flags); + /* + * we have to do two separate lock calls here to keep lockdep + * happy. If we try to get all the locks in one call, lock will + * report false positives when we drop the ILOCK and regain them + * below. + */ + xfs_lock_two_inodes(ip, tip, XFS_IOLOCK_EXCL); + xfs_lock_two_inodes(ip, tip, XFS_ILOCK_EXCL); locked = 1; /* Verify that both files have the same format */ diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c index f108102..cb1b5fd 100644 --- a/fs/xfs/xfs_vnodeops.c +++ b/fs/xfs/xfs_vnodeops.c @@ -1836,6 +1836,12 @@ again: #endif } +/* + * xfs_lock_two_inodes() can only be used to lock one type of lock + * at a time - the iolock or the ilock, but not both at once. If + * we lock both at once, lockdep will report false positives saying + * we have violated locking orders. + */ void xfs_lock_two_inodes( xfs_inode_t *ip0, @@ -1846,7 +1852,11 @@ xfs_lock_two_inodes( int attempts = 0; xfs_log_item_t *lp; +#ifdef DEBUG + if (lock_mode & (XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL)) + ASSERT((lock_mode & (XFS_ILOCK_SHARED|XFS_ILOCK_EXCL)) == 0); ASSERT(ip0->i_ino != ip1->i_ino); +#endif if (ip0->i_ino > ip1->i_ino) { temp = ip0; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/