Re: d_splice_alias() problem.

[Neil Brown]

[I wrote this a few days ago but it looks like I forgot to post it...]

On Monday May 10, gnb@xxxxxxxxxxxxxxxxx wrote:
> Neil Brown wrote:
> > 
> > > > *   Logic bug in d_splice_alias() forgets to clear the DCACHE_DISCONNECTED
> > > >     flag when a lookup connects a disconnected dentry.  Fix is (relative
> > > >     to Neil's patch):
> > >
> > Ok, I've reviewed the code and thought about it some more.
> > 
> > This was intentional and the patch to clear DCACHE_DISCONNECTED is not
> > needed and possibly wrong.
> > 
> > DCACHE_DISCONNECTED *doesn't* mean that the entries isn't connected,
> > but only that it might not be.
> 
> Agreed.  After reading your comments below the semantics of the flag
> make sense: was once disconnected and may or may not still be depending
> on lazy clearing by the expfs.c code).  My dentry state checking code
> was wrong.
> 
> Of course I now have an issue with the misleading name ;-)

Maybe:   DCACHE_NOT_KNOWN_TO_BE_CONNECTED.
Unfortunately the absence of the flags is stronger information that
it's presence and that makes it hard to name...

> 
> What I'm wondering is, do we still need DCACHE_DISCONNECTED at all?
> Perhaps the uses of it could be replaced with combinations of checks
> of IS_ROOT() and (d == d->d_sb->s_root) ?

It is still needed.
Suppose one thread creates a disconnected dentry, and then starts building
the path from the bottom up.
When it is half way up another request comes in for the same
filehandle.  The same dentry is found.  It is now not IS_ROOT, but
still DCACHE_DISCONNECTED.  Until it is fully connected the second
request shouldn't proceed, and without the DCACHE_DISCONNECTED flag it
is expensive to test for connected-ness.  !IS_ROOT certainly isn't
enough.

> 
> The changes look good but I think the invariants you describe above
> should go in a comment.

Good point.  Which comment I wonder...

> 
> I will try to do some testing in the next couple of days.
> 
> > -void d_move(struct dentry * dentry, struct dentry * target)
> > +static void d_move_locked(struct dentry * dentry, struct dentry * target)
> >  {
> >  	if (!dentry->d_inode)
> >  		printk(KERN_WARNING "VFS: moving negative dcache entry\n");
> >[...]
> > +
> > +void d_move(struct dentry * dentry, struct dentry * target)
> > +{
> > +       if (!dentry->d_inode)
> > +               printk(KERN_WARNING "VFS: moving negative dcache entry\n");
> > +
> 
> Do you need two copies of this check in the same path?
> 

No.  cut-and-paste error I think.

NeilBrown
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/