From: NeilBrown <neil@xxxxxxxxxx>
It is important that two non-create NFS "open"s of a negative dentry don't race.
They both have a shared lock on i_rwsem and so could run concurrently, but
they might both try to call d_exact_alias() or 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 with 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.
Dropping the dentry like this will defeat a proposed new locking scheme
which locks the dentry and requires it to remain hashed.
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 require 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 if the dentry that reaches nfs_atomic_open() is
not d_in_lookup and is negative, then some other lookup or atomic_open
must have happened since d_revalidate was called, so we have recent
confirmation from the server that the name doesn't exist, and we can
safely fail the open request (which finish_no_open() will effectively do
when passed a negative dentry).
Note that none of the above applies to O_CREAT opens. These are fully
serialised with an exclusive lock on i_rwsem and there is no attempt to
d_drop() the dentry in that case. d_revalidate() does not need to
request invalidation of a negative dentry as a create will proceed in
that case anyway.
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 d81217923936..bc417566508e 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1615,6 +1615,11 @@ 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 server
+ * on each open.
+ */
+ return 1;
if (NFS_SERVER(dir)->flags & NFS_MOUNT_LOOKUP_CACHE_NONEG)
return 1;
/* Case insensitive server? Revalidate negative dentries */
@@ -2060,14 +2065,12 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
struct file *file, unsigned open_flags,
umode_t mode)
{
- DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
struct nfs_open_context *ctx;
struct dentry *res;
struct iattr attr = { .ia_valid = ATTR_OPEN };
struct inode *inode;
unsigned int lookup_flags = 0;
unsigned long dir_verifier;
- bool switched = false;
int created = 0;
int err;
@@ -2112,16 +2115,8 @@ 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, &wq);
- if (IS_ERR(dentry))
- return PTR_ERR(dentry);
- if (unlikely(!d_in_lookup(dentry)))
- return finish_no_open(file, dentry);
- }
+ if (!(open_flags & O_CREAT) && !d_in_lookup(dentry))
+ return finish_no_open(file, dentry);
ctx = create_nfs_open_context(dentry, open_flags, file);
err = PTR_ERR(ctx);
@@ -2165,10 +2160,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:
@@ -2191,13 +2182,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);
- }
if (IS_ERR(res))
return PTR_ERR(res);
return finish_no_open(file, res);
--
2.50.0.107.gf914562f5916.dirty
