On Sat, 08 May 2010 09:05:04 -0400
Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
> On 05/07/2010 06:34 PM, Neil Brown wrote:
> > On Thu, 06 May 2010 09:58:31 -0400 Trond Myklebust<trond.myklebust@xxxxxxxxxx> wrote:
> >>>
> >>>
> >>> diff --git a/fs/namei.c b/fs/namei.c
> >>> index a7dce91..256ae13 100644
> >>> --- a/fs/namei.c
> >>> +++ b/fs/namei.c
> >>> @@ -719,7 +719,11 @@ static int do_lookup(struct nameidata *nd, struct qstr *name,
> >>> done:
> >>> path->mnt = mnt;
> >>> path->dentry = dentry;
> >>> - __follow_mount(path);
> >>> + if (__follow_mount(path)&&
> >>> + (path->mnt->mnt_sb->s_type->fs_flags& FS_REVAL_DOT)) {
> >>> + if (!path->dentry->d_op->d_revalidate(path->dentry, nd))
> >>> + return -ESTALE;
> >>
> >> Won't this prevent you from ever being able to unmount the stale
> >> filesystem?
> >>
> >
> > Good point - I think you are right.
> >
> > It seems to me that ->d_revalidate is being used for two distinct, though
> > related, tasks.
> > One is to revalidate the dentry - make sure the name still refers to the same
> > inode. The other is to revalidate the inode - make sure the cached
> > attributes are still valid. In NFS (v3 and v4 at least) these are both
> > performed by one call so it makes some sense to combine them. But from the
> > VFS perspective I would have thought they were quite separate.
> > nfs_lookup_revalidate sometimes does a GETATTR, and sometimes does a LOOKUP,
> > depending to some extent on the 'intent' in the nameidata. I find this makes
> > it a bit hard to follow what is really happening, or how d_revalidate should
> > really be used.
> >
> > Maybe we should just ignore the return value above. Or maybe d_revalidate
> > should never do a GETATTR - that should be done by ->open ??
> >
> > Confused :-(
>
> The reason for this arrangement is that by the time ->open is called,
> it's too late to return ESTALE. During the path walk, there is still an
> opportunity to renew the mapping between dentry and inode, should the
> cached value of that mapping be stale.
>
Is it?
->open seems to be able to return an error code, and that error code seems to
be propagated up. What am I missing?
I appreciate that a previous lookup might have already done the equivalent of
'open' (e.g. for O_EXCL) but it seems wrong to assume that the last lookup
will always do the GETATTR required for open (because there isn't always a
'last lookup').
I would rather see the lookup (or d_revalidate) remember how much of an open
it has done, and then ->open does the rest. i.e. it does a GETATTR if that
hasn't already been done.
I don't think there is currently an easy way for 'open' to know how much work
'd_revalidate' or 'lookup' has done, though I suspect that could be fixed.
Maybe pass (as pointer to) the whole intent structure into open rather than
just the intent.open.file ??
Thanks,
NeilBrown
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
