Re: [PATCH] Should we expect close-to-open consistency on directories?

[Neil Brown <neilb@suse.de>]

On Sat, 08 May 2010 09:05:04 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> On 05/07/2010 06:34 PM, Neil Brown wrote:
> > On Thu, 06 May 2010 09:58:31 -0400 Trond Myklebust<trond.myklebust@fys.uio.no>  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