Re: [PATCH] CIFS: fix automount for DFS shares

[Ian Kent]

On Sun, 2011-10-23 at 07:46 -0400, Jeff Layton wrote:
> On Sun, 23 Oct 2011 19:14:34 +0800
> Ian Kent <raven@xxxxxxxxxx> wrote:
> 
> > On Fri, 2011-10-07 at 13:09 +0200, Gerlando Falauto wrote:
> > > Automounting directories are now invalidated by .d_revalidate()
> > > so to be d_instantiate()d again with the right DCACHE_NEED_AUTOMOUNT
> > > flag
> > 
> > But why doesn't CIFS know this is a DFS inode the first time around, it
> > appears to do a truck load of work looking that stuff up?
> > 
> 
> This area needs some work...
> 
> The readdir codepath in cifs uses the FIND_FIRST/NEXT calls on the
> wire. Those return both filenames and attributes for the particular SMB
> call infolevel. That in turn is used to instantiate dentries and inodes
> for those entries.
> 
> Unfortunately though, we have not found a way to determine whether a
> particular inode is a DFS referral from within this codepath. The only
> way to know for sure (AFAIK) is to try an operation on a specific
> pathname and then look for a NT_STATUS_PATH_NOT_COVERED return code.
> 
> > > 
> > > Signed-off-by: Gerlando Falauto <gerlando.falauto@xxxxxxxxxxx>
> > > ---
> > >  fs/cifs/dir.c |    7 ++++++-
> > >  1 files changed, 6 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> > > index 9ea65cf..67f54d3 100644
> > > --- a/fs/cifs/dir.c
> > > +++ b/fs/cifs/dir.c
> > > @@ -637,8 +637,13 @@ cifs_d_revalidate(struct dentry *direntry, struct nameidata *nd)
> > >  	if (direntry->d_inode) {
> > >  		if (cifs_revalidate_dentry(direntry))
> > >  			return 0;
> > > -		else
> > > +		else {
> > > +			/* We want automonting inodes to be
> > > +			 * considered invalid or so */
> > > +			if (IS_AUTOMOUNT(direntry->d_inode))
> > > +				return 0;
> > 
> > I'd be inclined to set DCACHE_NEED_AUTOMOUNT here but are we certain
> > that cifs_revalidate_dentry() will always return 0 for a DFS inode or at
> > least ones that don't yet have DCACHE_NEED_AUTOMOUNT set and why?
> > 
> 
> You mean you'd set that in the "return 1" case here? That doesn't sound
> quite right if so. This inode could be entirely unrelated to DFS.

Actually, the return code might not matter. The current code returns 1
if cifs_revalidate_dentry() returns 0, so yes, that's what I'm saying.
Setting DCACHE_NEED_AUTOMOUNT in d_flags if S_AUTOMOUNT() is set is
enough to trigger the mount and S_AUTOMOUNT() will be set if it's been
seen as a DFS point.

If cifs_revalidate_dentry() returns 1 it returns false and the lookup
gets re-done so the other case looks like it handles the case where we
now know the thing is a DFS inode but haven't set the dcache flag since
the dentry wasn't actually instantiated.

> 
> Currently, cifs_revalidate_dentry checks to see if the attributes on
> the inode are "too old" (past the actimeo setting). If they are then it
> will issue a QUERY_PATH_INFO call on the wire to try and update those
> attributes. If it's a DFS inode then you'll get an error back and that
> function should return 0.
> 
> It's possible however that the inode has already had its attributes
> updated recently, and was found to be an automount point. That is, it
> got S_AUTOMOUNT set but no referral chasing happened. At that point I'm
> a little fuzzy as to what should happen...

Right, from the POV of CIFS .... the automounting bit is ....

The automounting happens during the follow after the lookup/revalidate
of the path component if the appropriate flag(s) is set. The follow will
be done for each mountpoint dentry in the stack, calling ->d_manage() if
needed, for each and once the last one is reached ->d_automount() is
called if it is needed. That's a bit simplified but is basically the way
it's done.

What I'm suggesting is that there's no need to allocate a new dentry
when the attributes have now been filled in and the dentry is seen as a
DFS point.

Not sure how that will go with multiple concurrent walks though.

> 
> The safest thing would seem to be to return 0 in that case, to force an
> invalidation and lookup on this dentry again, but maybe there's a better
> way to handle that?

Who else can we consult on this?

> 
> > >  			return 1;
> > > +		}
> > >  	}
> > >  
> > >  	/*
> > 
> > 
> 
> 


--
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/