From mboxrd@z Thu Jan 1 00:00:00 1970 From: Al Viro Subject: Re: ->d_lock FUBAR (was Re: Linux 3.0-rc6) Date: Wed, 13 Jul 2011 03:59:18 +0100 Message-ID: <20110713025918.GM11013@ZenIV.linux.org.uk> References: <20110711060315.GI11013@ZenIV.linux.org.uk> <20110712234806.GJ11013@ZenIV.linux.org.uk> <20110713005634.GK11013@ZenIV.linux.org.uk> <20110713013936.GL11013@ZenIV.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-fsdevel@vger.kernel.org, npiggin@kernel.dk To: Linus Torvalds Return-path: Received: from zeniv.linux.org.uk ([195.92.253.2]:51814 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932295Ab1GMC7T (ORCPT ); Tue, 12 Jul 2011 22:59:19 -0400 Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, Jul 12, 2011 at 07:40:01PM -0700, Linus Torvalds wrote: > It's dentry_lock_for_move() that makes me really nervous. Not only > does it lock up to four dentries, but it mixes the whole parenthood vs > pointer ordering. Or course, it does have those BUG_ON() checks, so > it should never cause any circular dependencies, but still.. Me too, obviously. > The actual main protection to get lookups correct in the presence of > concurrent moves largely depends on the sequence numbers (ie > d_lookup() retrying if it hits a rename), which is why I also find it > unlikely that we really should need to hold all those d_lock cases all > at the same time. > > So does d_move() really need to get all the locks at the same time and > then do all the operations inside that "super-locked" region? Or could > we get the locks in sequence and do individual parts of the rename > operations under individual locks? > > Are there any other d_lock cases that depend on the pointer ordering? > Most everything else seems to be about direct parenthood, no? It's not that easy. We want ->d_lock on parents - not only because there's code iterating through the list of children, but because ordering on direct parenthood bloody depends on children not moving out while we hold ->d_lock on their parent. Otherwise we are looking for nightmares in shrink_dcache_parent() et.al. I'm not sure how much do we care about stability of x->d_parent when x->d_lock is held. ->d_compare() is the most obvious potential area of trouble in that respect, but there might be more. I'm still not finished reviewing ->d_lock uses; about a couple of hundreds is left to wade through. I would really, *REALLY* appreciate explicitly defined locking rules from Nick (it's his changes, mostly). As in "this, this and that field is protected by ->d_lock on..." Note that ->d_parent is stable when i_mutex is held on parent, which makes most of the users of ->d_parent safe and fine (->lookup(), etc. are all called with directory locked). I've not finished reviewing ->d_parent users either, but IMO ->d_lock review is more important, so it got bumped in front of queue... Back to GrepTFS and stripping the paint off the walls...