Re: [PATCH v2 3/9] nfs: remove d_drop()/d_alloc_parallel() from nfs_atomic_open()

[John Stoffel]

>>>>> "NeilBrown" == NeilBrown  <neilb@xxxxxxxxxxx> writes:

> From: NeilBrown <neil@xxxxxxxxxx>
> It is important that two non-create NFS "open"s of a negative dentry
> don't race.  They both have only a shared lock on i_rwsem and so could
> run concurrently, but they might both try to call d_splice_alias() at
> the same time which is confusing at best.

> nfs_atomic_open() currently avoids this by discarding the negative
> dentry and creating a new one using d_alloc_parallel().  Only one thread
> can successfully get the d_in_lookup() dentry, the other will wait for
> the first to finish, and can use the result of that first lookup.

This paragraph is confusing, and I'm not sure which thread can use the
result of the first lookup from this description.  

> A proposed locking change inverts the order between i_rwsem and
> d_alloc_parallel() so it will not be safe to call d_alloc_parallel()
> while holding i_rwsem - even shared.

> We can achieve the same effect by causing ->d_revalidate to invalidate a
> negative dentry when LOOKUP_OPEN is set.  Doing this is consistent with
> the "close to open" caching semantics of NFS which requires the server
> to be queried whenever opening a file - cached information must not be
> trusted.

> With this change to ->d_revaliate (implemented in nfs_neg_need_reval())
> we can be sure that we have exclusive access to any dentry that reaches
> nfs_atomic_open().  Either O_CREAT was requested and so the parent is
> locked exclusively, or the dentry will have DCACHE_PAR_LOOKUP set.
> [Note that when nfs_neg_need_reval() returns 1, ->d_revalidate returns 0].

> This means that the d_drop() and d_alloc_parallel() calls in
> nfs_atomic_lookup() are no longer needed to provide exclusion

> There is still a d_drop() which allowed d_splice_alias() to succeed.
> This will be removed in a future patch.

> Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
> ---
>  fs/nfs/dir.c | 30 +++++++-----------------------
>  1 file changed, 7 insertions(+), 23 deletions(-)

> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 4f1bf45b0c70..f219cc1a5fc5 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -1656,6 +1656,13 @@ int nfs_neg_need_reval(struct inode *dir, struct dentry *dentry,
>  {
>  	if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET))
>  		return 0;
> +	if (flags & LOOKUP_OPEN)
> +		/* close-to-open semantics require we go to the server
> +		 * on each open.  By invalidating the dentry we
> +		 * also ensure nfs_atomic_open() always has exclusive
> +		 * access to the dentry.
> +		 */
> +		return 1;
>  	if (NFS_SERVER(dir)->flags & NFS_MOUNT_LOOKUP_CACHE_NONEG)
>  		return 1;
>  	/* Case insensitive server? Revalidate negative dentries */
> @@ -2113,7 +2120,6 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
>  	struct inode *inode;
>  	unsigned int lookup_flags = 0;
>  	unsigned long dir_verifier;
> -	bool switched = false;
>  	int created = 0;
>  	int err;
 
> @@ -2158,17 +2164,6 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
>  		attr.ia_size = 0;
>  	}
 
> -	if (!(open_flags & O_CREAT) && !d_in_lookup(dentry)) {
> -		d_drop(dentry);
> -		switched = true;
> -		dentry = d_alloc_parallel(dentry->d_parent,
> -					  &dentry->d_name);
> -		if (IS_ERR(dentry))
> -			return PTR_ERR(dentry);
> -		if (unlikely(!d_in_lookup(dentry)))
> -			return finish_no_open(file, dentry);
> -	}
> -
>  	ctx = create_nfs_open_context(dentry, open_flags, file);
>  	err = PTR_ERR(ctx);
>  	if (IS_ERR(ctx))
> @@ -2219,10 +2214,6 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
>  	trace_nfs_atomic_open_exit(dir, ctx, open_flags, err);
>  	put_nfs_open_context(ctx);
>  out:
> -	if (unlikely(switched)) {
> -		d_lookup_done(dentry);
> -		dput(dentry);
> -	}
>  	return err;
 
>  no_open:
> @@ -2245,13 +2236,6 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
>  			res = ERR_PTR(-EOPENSTALE);
>  		}
>  	}
> -	if (switched) {
> -		d_lookup_done(dentry);
> -		if (!res)
> -			res = dentry;
> -		else
> -			dput(dentry);
> -	}
>  	return finish_no_open(file, res);
>  }
>  EXPORT_SYMBOL_GPL(nfs_atomic_open);
> -- 
> 2.50.0.107.gf914562f5916.dirty