Re: [announce] vfs-scale git tree update

[Ian Kent <raven@themaw.net>]

On Wed, 2011-01-12 at 10:19 +0530, Aneesh Kumar K. V wrote:
> On Wed, 12 Jan 2011 11:59:36 +0800, Ian Kent <raven@themaw.net> 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 <aelder@sgi.com> 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().  I 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 same
> > > > dentry.
> > > > 
> > > > Possibly the thing could be relaxed to allow setting the _same_ d_op
> > > > pointer, ie do something like
> > > > 
> > > >    if (dentry->d_op == op)
> > > >       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  dentry lookup hit before it's actually
> > > > fully done.
> > > 
> > > Agreed.
> > 
> > Isn't the parent i_mutex held during mkdir()?
> > 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.  I.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.
> > 
> 
> for 9p I did the other way round. lookup now set the dentry op for both
> negative and positive dentries and rest of the place we dropped
> d_set_d_op (b8b80cf37c7f0e32729262f805bc0fa81c3e9d12 have the details)

Sadly I can't do that.

For now I need to avoid calls to ->d_revalidate() as much as possible to
reduce a potential deadlock and have access to the flags of the negative
dentry (during create) for the purpose of blocking concurrent walks.

But, if all goes well, later changes will see the autofs
->d_revalidate() gone and consequently the potential deadlock gone as
well.

Ian