From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nick Piggin Subject: Re: [announce] vfs-scale git tree update Date: Wed, 12 Jan 2011 15:06:01 +1100 Message-ID: References: <20110107075831.GA14915@amd> <1294763679.2435.72.camel@doink> <1294768668.2435.177.camel@doink> <1294804776.2821.4.camel@perseus> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: aelder@sgi.com, Linus Torvalds , Nick Piggin , Al Viro , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org To: Ian Kent Return-path: In-Reply-To: <1294804776.2821.4.camel@perseus> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Wed, Jan 12, 2011 at 2:59 PM, Ian Kent wrote: > On Tue, 2011-01-11 at 11:57 -0600, Alex Elder wrote: >> On Tue, 2011-01-11 at 08:51 -0800, Linus Torvalds wrote: >> > On Tue, Jan 11, 2011 at 8:34 AM, Alex Elder wrote= : >> > > >> > > FYI, when using this code, as merged by Linus, I hit the >> > > BUG_ON() at the beginning of d_set_d_op() when it's called >> > > by autofs4_dir_mkdir(). =A0I managed to work around it by >> > > just commenting out those BUG_ON() calls but it's something >> > > that ought to get addressed properly. >> > >> > Yeah, removing the BUG_ON() isn't the right thing to do - it means >> > that autofs4 is obviously setting the dentry ops twice for the sam= e >> > dentry. >> > >> > Possibly the thing could be relaxed to allow setting the _same_ d_= op >> > pointer, ie do something like >> > >> > =A0 =A0if (dentry->d_op =3D=3D op) >> > =A0 =A0 =A0 return; >> > >> > at the top of that function. But looking at it, I don't think that >> > fixes the autofs4 issue. >> >> That's easy enough, but it seems everybody else ensures >> this gets done just once per dentry, and it would be nice >> to preserve that "tightness" if possible. >> >> > The fact that autofs4 does "d_add()" before it sets the d_ops (or >> > other dentry state, for that matter) looks a bit scary. To me that >> > smells like it might get a =A0dentry lookup hit before it's actual= ly >> > fully done. >> >> Agreed. > > Isn't the parent i_mutex held during mkdir()? Yes but there are concurrency cases allowed without i_mutex. Lookup, for example, which ends up touching d_revalidate and when dropping the dentry, possibly d_delete. There seems no benefit to allowing switch of d_ops on a live dentry, and many downsides. So the rule should just be that it is not allowed. > Still the order can be changed, of course. > >> >> > Does it make any difference if you move the various d_add() calls = down >> > to the end of the functions to when the "dentry" has really been >> > instantiated? >> >> Looking at it quickly, I don't think that would matter for >> the case at hand. =A0I.e., that might be safer but it doesn't >> address the fact that these fields are getting initialized >> multiple times. > > Yeah, a hangover from changes done over time. > Not setting the dentry op in ->lookup() should fix this. How about negative dentries? They should be set up with d_ops upon allocation, preferably if your operations can handle negative dentries. Thanks, Nick