Re: NFS: NFS3 lookup fails if creating a file with O_EXCL & multiple mountpoints in pathname

[Linda Dunaphant <linda.dunaphant@ccur.com>]

Hi Trond,

Thank you so much for looking into this further!

I duplicated the original problem using the unpatched 2.6.12-rc6. I then
applied your patch to 2.6.12-rc6 and retested. The original problem was
fixed by your patch. 

I now need to backfit your patch to the older kernel that I am working
with.

Thanks again very much for your help!

Cheers!
Linda

On Tue, 2005-06-07 at 09:41, Trond Myklebust wrote:
> må den 06.06.2005 Klokka 22:29 (-0400) skreiv Linda Dunaphant:
> > I changed the nfs_is_exclusive_create() to use the LOOKUP_PARENT
> > flag instead of the LOOKUP_CONTINUE flag. I found that LOOKUP_PARENT
> > stays set until you get to the very last pathname level, which is
> > the correct point for it to check whether it is an exclusive create.
> 
> The intent information as it stands today should really only be applied
> if we're looking up the very last path component of an open()/create()
> or access() call. When we're walking the path doing lookups, that rule
> basically boils down to: never apply the intent if either
> LOOKUP_CONTINUE or LOOKUP_PARENT are set.
> 
> It therefore turns out that your patch is in fact a valid fix in the
> case of nfs_is_exclusive_create() since the VFS is guaranteed to always
> call LOOKUP_PARENT first (my apologies).
> However, when hunting through the NFS lookup code, I found several other
> cases where we check for an intent and where we do need the more general
> test. To avoid confusion, I'd therefore prefer to introduce a helper
> that _always_ returns the correct intent information for the component
> that is being looked up.
> 
> Could you therefore check if this patch works for you?
> 
> Cheers,
>   Trond
> 
> 
> ______________________________________________________________________
> [NFS] Fix lookup intent handling
> 
>  We should never apply a lookup intent to the path component of a
>  LOOKUP_PARENT call.
>  Introduce nd_lookup_intent() which always returns zero if LOOKUP_CONTINUE
>  or LOOKUP_PARENT is set, and otherwise returns the intent flags.
> 
>  Problem noticed by Linda Dunaphant <linda.dunaphant@ccur.com>
>  Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
>  dir.c |   49 +++++++++++++++++++++++++++++++++++--------------
>  1 files changed, 35 insertions(+), 14 deletions(-)
> 
> Index: linux-2.6.12-rc6/fs/nfs/dir.c
> ===================================================================
> --- linux-2.6.12-rc6.orig/fs/nfs/dir.c
> +++ linux-2.6.12-rc6/fs/nfs/dir.c
> @@ -528,19 +528,39 @@ static inline void nfs_renew_times(struc
>  	dentry->d_time = jiffies;
>  }
>  
> +/*
> + * Return the intent data that applies to this particular path component
> + *
> + * Note that the current set of intents only apply to the very last
> + * component of the path.
> + * We check for this using LOOKUP_CONTINUE and LOOKUP_PARENT.
> + */
> +static inline unsigned int lookup_check_intent(struct nameidata *nd, unsigned int mask)
> +{
> +	if (nd->flags & (LOOKUP_CONTINUE|LOOKUP_PARENT))
> +		return 0;
> +	return nd->flags & mask;
> +}
> +
> +/*
> + * Inode and filehandle revalidation for lookups.
> + *
> + * We force revalidation in the cases where the VFS sets LOOKUP_REVAL,
> + * or if the intent information indicates that we're about to open this
> + * particular file and the "nocto" mount flag is not set.
> + *
> + */
>  static inline
>  int nfs_lookup_verify_inode(struct inode *inode, struct nameidata *nd)
>  {
>  	struct nfs_server *server = NFS_SERVER(inode);
>  
>  	if (nd != NULL) {
> -		int ndflags = nd->flags;
>  		/* VFS wants an on-the-wire revalidation */
> -		if (ndflags & LOOKUP_REVAL)
> +		if (nd->flags & LOOKUP_REVAL)
>  			goto out_force;
>  		/* This is an open(2) */
> -		if ((ndflags & LOOKUP_OPEN) &&
> -				!(ndflags & LOOKUP_CONTINUE) &&
> +		if (lookup_check_intent(nd, LOOKUP_OPEN) != 0 &&
>  				!(server->flags & NFS_MOUNT_NOCTO))
>  			goto out_force;
>  	}
> @@ -560,12 +580,8 @@ static inline
>  int nfs_neg_need_reval(struct inode *dir, struct dentry *dentry,
>  		       struct nameidata *nd)
>  {
> -	int ndflags = 0;
> -
> -	if (nd)
> -		ndflags = nd->flags;
>  	/* Don't revalidate a negative dentry if we're creating a new file */
> -	if ((ndflags & LOOKUP_CREATE) && !(ndflags & LOOKUP_CONTINUE))
> +	if (nd != NULL && lookup_check_intent(nd, LOOKUP_CREATE) != 0)
>  		return 0;
>  	return !nfs_check_verifier(dir, dentry);
>  }
> @@ -700,12 +716,16 @@ struct dentry_operations nfs_dentry_oper
>  	.d_iput		= nfs_dentry_iput,
>  };
>  
> +/*
> + * Use intent information to check whether or not we're going to do
> + * an O_EXCL create using this path component.
> + */
>  static inline
>  int nfs_is_exclusive_create(struct inode *dir, struct nameidata *nd)
>  {
>  	if (NFS_PROTO(dir)->version == 2)
>  		return 0;
> -	if (!nd || (nd->flags & LOOKUP_CONTINUE) || !(nd->flags & LOOKUP_CREATE))
> +	if (nd == NULL || lookup_check_intent(nd, LOOKUP_CREATE) == 0)
>  		return 0;
>  	return (nd->intent.open.flags & O_EXCL) != 0;
>  }
> @@ -772,12 +792,13 @@ struct dentry_operations nfs4_dentry_ope
>  	.d_iput		= nfs_dentry_iput,
>  };
>  
> +/*
> + * Use intent information to determine whether we need to substitute
> + * the NFSv4-style stateful OPEN for the LOOKUP call
> + */
>  static int is_atomic_open(struct inode *dir, struct nameidata *nd)
>  {
> -	if (!nd)
> -		return 0;
> -	/* Check that we are indeed trying to open this file */
> -	if ((nd->flags & LOOKUP_CONTINUE) || !(nd->flags & LOOKUP_OPEN))
> +	if (nd == NULL || lookup_check_intent(nd, LOOKUP_OPEN) == 0)
>  		return 0;
>  	/* NFS does not (yet) have a stateful open for directories */
>  	if (nd->flags & LOOKUP_DIRECTORY)