Re: [PATCH 0/3] VFS: name lookup improvements.

From: NeilBrown
Date: Thu Nov 09 2017 - 19:21:48 EST

On Fri, Nov 10 2017, NeilBrown wrote:
>>
>> Your patch does make me wonder if we should consider merging
>> d_weak_revalidate and d_revalidate back together, and simply require all
>> the d_revalidate ops vet the flags more thoroughly.
>
> This boils down to adding
>
> if (flags & LOOKUP_JUMPED)
> return 1;
>
> to the front of almost every d_revalidate function.
> I'm probably in favour of that, but it isn't an obvious win.
> I can certainly offer it as a follow-on patch so we can see
> exactly the impact.


See below.
NeilBrown

From: NeilBrown <neilb@xxxxxxxx>
Date: Wed, 16 Aug 2017 09:03:36 +1000
Subject: [PATCH] VFS: remove d_weak_revalidate inode operation

d_weak_revalidate is now very similar to d_revalidate.
The difference is that the former always has LOOKUP_JUMPED
set and the later doesn't. When LOOKUP_JUMPED is set
the name does not need to be revalidated, but the inode might.

This distinction can be managed entirely in the filesystem
without needing two similar entry points.

So discard d_weak_revalidate and call d_revalidate instead.
Make sure all d_revalidate functions avoid checking the validity
of the name if LOOKUP_JUMPED is set.

Signed-off-by: NeilBrown <neilb@xxxxxxxx>
---
Documentation/filesystems/Locking | 2 --
Documentation/filesystems/porting | 6 ++++++
Documentation/filesystems/vfs.txt | 20 +++++---------------
drivers/staging/lustre/lustre/llite/dcache.c | 2 +-
fs/9p/vfs_dentry.c | 3 +++
fs/afs/dir.c | 3 +++
fs/ceph/dir.c | 3 +++
fs/cifs/dir.c | 3 +++
fs/coda/dir.c | 3 +++
fs/crypto/crypto.c | 3 +++
fs/dcache.c | 3 ---
fs/ecryptfs/dentry.c | 3 +++
fs/fat/namei_vfat.c | 4 ++++
fs/fuse/dir.c | 3 +++
fs/gfs2/dentry.c | 3 +++
fs/kernfs/dir.c | 3 +++
fs/namei.c | 4 ++--
fs/ncpfs/dir.c | 3 +++
fs/nfs/dir.c | 2 --
fs/ocfs2/dcache.c | 3 +++
fs/orangefs/dcache.c | 3 +++
fs/overlayfs/super.c | 19 -------------------
fs/overlayfs/util.c | 3 +--
fs/proc/base.c | 3 +++
fs/proc/fd.c | 3 +++
include/linux/dcache.h | 3 ---
26 files changed, 64 insertions(+), 49 deletions(-)

diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
index 75d2d57e2c44..a824a305cf5d 100644
--- a/Documentation/filesystems/Locking
+++ b/Documentation/filesystems/Locking
@@ -10,7 +10,6 @@ be able to use diff(1).
--------------------------- dentry_operations --------------------------
prototypes:
int (*d_revalidate)(struct dentry *, unsigned int);
- int (*d_weak_revalidate)(struct dentry *, unsigned int);
int (*d_hash)(const struct dentry *, struct qstr *);
int (*d_compare)(const struct dentry *,
unsigned int, const char *, const struct qstr *);
@@ -27,7 +26,6 @@ prototypes:
locking rules:
rename_lock ->d_lock may block rcu-walk
d_revalidate: no no yes (ref-walk) maybe
-d_weak_revalidate:no no yes no
d_hash no no no maybe
d_compare: yes no no maybe
d_delete: no yes no no
diff --git a/Documentation/filesystems/porting b/Documentation/filesystems/porting
index f455757ff1c6..56809d6938b5 100644
--- a/Documentation/filesystems/porting
+++ b/Documentation/filesystems/porting
@@ -611,3 +611,9 @@ in your dentry operations instead.
->d_weak_revalidate should perform the same handling of LOOKUP_OPEN as
->d_revalidate. If LOOKUP_OPEN is not set, d_weak_revalidate need not
do anything.
+--
+[mandatory]
+ ->d_weak_revalidate() is gone. ->d_revalidate must check for
+ LOOKUP_JUMPED and skip revalidate of the name in that case.
+ If LOOKUP_JUMPED is set and LOOKUP_OPEN is not set, ->d_revalidate
+ need not do anything.
diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index c2025e226b29..d4bd4f016115 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -976,7 +976,6 @@ defined:

struct dentry_operations {
int (*d_revalidate)(struct dentry *, unsigned int);
- int (*d_weak_revalidate)(struct dentry *, unsigned int);
int (*d_hash)(const struct dentry *, struct qstr *);
int (*d_compare)(const struct dentry *,
unsigned int, const char *, const struct qstr *);
@@ -1010,20 +1009,11 @@ struct dentry_operations {
If a situation is encountered that rcu-walk cannot handle, return
-ECHILD and it will be called again in ref-walk mode.

- d_weak_revalidate: called when the VFS needs to revalidate a "jumped" dentry.
- This is called when a path-walk ends at dentry that was not acquired by
- doing a lookup in the parent directory. This includes "/", "." and "..",
- as well as procfs-style symlinks and mountpoint traversal.
-
- Filesystems only need this if they handle LOOKUP_OPEN in
- d_revalidate, in which case the same handling should be applied
- in d_weak_revalidate. When LOOKUP_OPEN is not set,
- d_weak_revalidate can safely be a no-op.
-
- This function has the same return code semantics as d_revalidate.
-
- d_weak_revalidate is only called after leaving rcu-walk mode,
- so LOOKUP_RCU is never set.
+ If LOOKUP_JUMPED is set in flags, the name does not need to be
+ revalidated and in many cases d_revalidate can safely do
+ nothing. If the filesystem handles LOOKUP_OPEN in
+ d_revalidate, that same handling should still be performed
+ when LOOKUP_JUMPED is set.

d_hash: called when the VFS adds a dentry to the hash table. The first
dentry passed to d_hash is the parent directory that the name is
diff --git a/drivers/staging/lustre/lustre/llite/dcache.c b/drivers/staging/lustre/lustre/llite/dcache.c
index 3670fcaf373f..b6c54edc45cd 100644
--- a/drivers/staging/lustre/lustre/llite/dcache.c
+++ b/drivers/staging/lustre/lustre/llite/dcache.c
@@ -240,7 +240,7 @@ static int ll_revalidate_dentry(struct dentry *dentry,
* to this dentry, then its lock has not been revoked and the
* path component is valid.
*/
- if (lookup_flags & LOOKUP_PARENT)
+ if (lookup_flags & (LOOKUP_PARENT | LOOKUP_JUMPED))
return 1;

/* Symlink - always valid as long as the dentry was found */
diff --git a/fs/9p/vfs_dentry.c b/fs/9p/vfs_dentry.c
index 99eaf3c6d44c..0995aaa4e48e 100644
--- a/fs/9p/vfs_dentry.c
+++ b/fs/9p/vfs_dentry.c
@@ -83,6 +83,9 @@ static int v9fs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
if (flags & LOOKUP_RCU)
return -ECHILD;

+ if (flags & LOOKUP_JUMPED)
+ return 1;
+
inode = d_inode(dentry);
if (!inode)
goto out_valid;
diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index 613a77058263..71b46202c15a 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -588,6 +588,9 @@ static int afs_d_revalidate(struct dentry *dentry, unsigned int flags)
if (flags & LOOKUP_RCU)
return -ECHILD;

+ if (flags & LOOKUP_JUMPED)
+ return 1;
+
vnode = AFS_FS_I(d_inode(dentry));

if (d_really_is_positive(dentry))
diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
index 8a5266699b67..718547b8c348 100644
--- a/fs/ceph/dir.c
+++ b/fs/ceph/dir.c
@@ -1217,6 +1217,9 @@ static int ceph_d_revalidate(struct dentry *dentry, unsigned int flags)
struct dentry *parent;
struct inode *dir;

+ if (flags & LOOKUP_JUMPED)
+ return 1;
+
if (flags & LOOKUP_RCU) {
parent = READ_ONCE(dentry->d_parent);
dir = d_inode_rcu(parent);
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 81ba6e0d88d8..5714a036c0a6 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -843,6 +843,9 @@ cifs_d_revalidate(struct dentry *direntry, unsigned int flags)
if (flags & LOOKUP_RCU)
return -ECHILD;

+ if (flags & LOOKUP_JUMPED)
+ return 1;
+
if (d_really_is_positive(direntry)) {
if (cifs_revalidate_dentry(direntry))
return 0;
diff --git a/fs/coda/dir.c b/fs/coda/dir.c
index 00876ddadb43..41a44465de2d 100644
--- a/fs/coda/dir.c
+++ b/fs/coda/dir.c
@@ -461,6 +461,9 @@ static int coda_dentry_revalidate(struct dentry *de, unsigned int flags)
if (flags & LOOKUP_RCU)
return -ECHILD;

+ if (flags & LOOKUP_JUMPED)
+ return 1;
+
inode = d_inode(de);
if (!inode || is_root_inode(inode))
goto out;
diff --git a/fs/crypto/crypto.c b/fs/crypto/crypto.c
index c7835df7e7b8..fd14eb2cd6cc 100644
--- a/fs/crypto/crypto.c
+++ b/fs/crypto/crypto.c
@@ -339,6 +339,9 @@ static int fscrypt_d_revalidate(struct dentry *dentry, unsigned int flags)
if (flags & LOOKUP_RCU)
return -ECHILD;

+ if (flags & LOOKUP_JUMPED)
+ return 1;
+
dir = dget_parent(dentry);
if (!d_inode(dir)->i_sb->s_cop->is_encrypted(d_inode(dir))) {
dput(dir);
diff --git a/fs/dcache.c b/fs/dcache.c
index 3130d62f29c9..0dd4e53fbc43 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1755,7 +1755,6 @@ void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op)
WARN_ON_ONCE(dentry->d_flags & (DCACHE_OP_HASH |
DCACHE_OP_COMPARE |
DCACHE_OP_REVALIDATE |
- DCACHE_OP_WEAK_REVALIDATE |
DCACHE_OP_DELETE |
DCACHE_OP_REAL));
dentry->d_op = op;
@@ -1767,8 +1766,6 @@ void d_set_d_op(struct dentry *dentry, const struct dentry_operations *op)
dentry->d_flags |= DCACHE_OP_COMPARE;
if (op->d_revalidate)
dentry->d_flags |= DCACHE_OP_REVALIDATE;
- if (op->d_weak_revalidate)
- dentry->d_flags |= DCACHE_OP_WEAK_REVALIDATE;
if (op->d_delete)
dentry->d_flags |= DCACHE_OP_DELETE;
if (op->d_prune)
diff --git a/fs/ecryptfs/dentry.c b/fs/ecryptfs/dentry.c
index 63cd2c147221..bf908d964aab 100644
--- a/fs/ecryptfs/dentry.c
+++ b/fs/ecryptfs/dentry.c
@@ -50,6 +50,9 @@ static int ecryptfs_d_revalidate(struct dentry *dentry, unsigned int flags)
if (flags & LOOKUP_RCU)
return -ECHILD;

+ if (flags & LOOKUP_JUMPED)
+ return 1;
+
if (lower_dentry->d_flags & DCACHE_OP_REVALIDATE)
rc = lower_dentry->d_op->d_revalidate(lower_dentry, flags);

diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index 02c066663a3a..a5bfb7fde9c5 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -45,6 +45,7 @@ static inline void vfat_d_version_set(struct dentry *dentry,
static int vfat_revalidate_shortname(struct dentry *dentry)
{
int ret = 1;
+
spin_lock(&dentry->d_lock);
if (vfat_d_version(dentry) != d_inode(dentry->d_parent)->i_version)
ret = 0;
@@ -68,6 +69,9 @@ static int vfat_revalidate_ci(struct dentry *dentry, unsigned int flags)
if (flags & LOOKUP_RCU)
return -ECHILD;

+ if (flags & LOOKUP_JUMPED)
+ return 1;
+
/*
* This is not negative dentry. Always valid.
*
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 24967382a7b1..883215127bcd 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -186,6 +186,9 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
struct fuse_inode *fi;
int ret;

+ if (flags & LOOKUP_JUMPED)
+ return 1;
+
inode = d_inode_rcu(entry);
if (inode && is_bad_inode(inode))
goto invalid;
diff --git a/fs/gfs2/dentry.c b/fs/gfs2/dentry.c
index 5173b98ca036..8cd906abf424 100644
--- a/fs/gfs2/dentry.c
+++ b/fs/gfs2/dentry.c
@@ -47,6 +47,9 @@ static int gfs2_drevalidate(struct dentry *dentry, unsigned int flags)
if (flags & LOOKUP_RCU)
return -ECHILD;

+ if (flags & LOOKUP_JUMPED)
+ return 1;
+
parent = dget_parent(dentry);
sdp = GFS2_SB(d_inode(parent));
dip = GFS2_I(d_inode(parent));
diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 89d1dc19340b..8f0aad50c3b3 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -562,6 +562,9 @@ static int kernfs_dop_revalidate(struct dentry *dentry, unsigned int flags)
if (flags & LOOKUP_RCU)
return -ECHILD;

+ if (flags & LOOKUP_JUMPED)
+ return 1;
+
/* Always perform fresh lookup for negatives */
if (d_really_is_negative(dentry))
goto out_bad_unlocked;
diff --git a/fs/namei.c b/fs/namei.c
index e90680a3f6f1..cc1d472749b1 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -809,10 +809,10 @@ static int complete_walk(struct nameidata *nd)
if (likely(!(nd->flags & LOOKUP_JUMPED)))
return 0;

- if (likely(!(dentry->d_flags & DCACHE_OP_WEAK_REVALIDATE)))
+ if (likely(!(dentry->d_flags & DCACHE_OP_REVALIDATE)))
return 0;

- status = dentry->d_op->d_weak_revalidate(dentry, nd->flags);
+ status = dentry->d_op->d_revalidate(dentry, nd->flags);
if (status > 0)
return 0;

diff --git a/fs/ncpfs/dir.c b/fs/ncpfs/dir.c
index b5ec1d980dc9..fb78808b45e2 100644
--- a/fs/ncpfs/dir.c
+++ b/fs/ncpfs/dir.c
@@ -323,6 +323,9 @@ ncp_lookup_validate(struct dentry *dentry, unsigned int flags)
if (flags & LOOKUP_RCU)
return -ECHILD;

+ if (flags & LOOKUP_JUMPED)
+ return 1;
+
parent = dget_parent(dentry);
dir = d_inode(parent);

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index fc349577526f..ffe76746cce8 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1273,7 +1273,6 @@ static void nfs_d_release(struct dentry *dentry)

const struct dentry_operations nfs_dentry_operations = {
.d_revalidate = nfs_lookup_revalidate,
- .d_weak_revalidate = nfs_lookup_revalidate,
.d_delete = nfs_dentry_delete,
.d_iput = nfs_dentry_iput,
.d_automount = nfs_d_automount,
@@ -1352,7 +1351,6 @@ static int nfs4_lookup_revalidate(struct dentry *, unsigned int);

const struct dentry_operations nfs4_dentry_operations = {
.d_revalidate = nfs4_lookup_revalidate,
- .d_weak_revalidate = nfs4_lookup_revalidate,
.d_delete = nfs_dentry_delete,
.d_iput = nfs_dentry_iput,
.d_automount = nfs_d_automount,
diff --git a/fs/ocfs2/dcache.c b/fs/ocfs2/dcache.c
index 290373024d9d..2ca92974f0b3 100644
--- a/fs/ocfs2/dcache.c
+++ b/fs/ocfs2/dcache.c
@@ -57,6 +57,9 @@ static int ocfs2_dentry_revalidate(struct dentry *dentry, unsigned int flags)
if (flags & LOOKUP_RCU)
return -ECHILD;

+ if (flags & LOOKUP_JUMPED)
+ return 1;
+
inode = d_inode(dentry);
osb = OCFS2_SB(dentry->d_sb);

diff --git a/fs/orangefs/dcache.c b/fs/orangefs/dcache.c
index ae782df5c063..64d5f462ffc2 100644
--- a/fs/orangefs/dcache.c
+++ b/fs/orangefs/dcache.c
@@ -103,6 +103,9 @@ static int orangefs_d_revalidate(struct dentry *dentry, unsigned int flags)
if (flags & LOOKUP_RCU)
return -ECHILD;

+ if (flags & LOOKUP_JUMPED)
+ return 1;
+
gossip_debug(GOSSIP_DCACHE_DEBUG, "%s: called on dentry %p.\n",
__func__, dentry);

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index f5738e96a052..6824ac56927f 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -138,24 +138,6 @@ static int ovl_dentry_revalidate(struct dentry *dentry, unsigned int flags)
return 1;
}

-static int ovl_dentry_weak_revalidate(struct dentry *dentry, unsigned int flags)
-{
- struct ovl_entry *oe = dentry->d_fsdata;
- unsigned int i;
- int ret = 1;
-
- for (i = 0; i < oe->numlower; i++) {
- struct dentry *d = oe->lowerstack[i].dentry;
-
- if (d->d_flags & DCACHE_OP_WEAK_REVALIDATE) {
- ret = d->d_op->d_weak_revalidate(d, flags);
- if (ret <= 0)
- break;
- }
- }
- return ret;
-}
-
static const struct dentry_operations ovl_dentry_operations = {
.d_release = ovl_dentry_release,
.d_real = ovl_d_real,
@@ -165,7 +147,6 @@ static const struct dentry_operations ovl_reval_dentry_operations = {
.d_release = ovl_dentry_release,
.d_real = ovl_d_real,
.d_revalidate = ovl_dentry_revalidate,
- .d_weak_revalidate = ovl_dentry_weak_revalidate,
};

static struct kmem_cache *ovl_inode_cachep;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index b9b239fa5cfd..131501b5a35b 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -78,8 +78,7 @@ struct ovl_entry *ovl_alloc_entry(unsigned int numlower)
bool ovl_dentry_remote(struct dentry *dentry)
{
return dentry->d_flags &
- (DCACHE_OP_REVALIDATE | DCACHE_OP_WEAK_REVALIDATE |
- DCACHE_OP_REAL);
+ (DCACHE_OP_REVALIDATE | DCACHE_OP_REAL);
}

bool ovl_dentry_weird(struct dentry *dentry)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 9d357b2ea6cb..10709224efca 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1926,6 +1926,9 @@ static int map_files_d_revalidate(struct dentry *dentry, unsigned int flags)
if (flags & LOOKUP_RCU)
return -ECHILD;

+ if (flags & LOOKUP_JUMPED)
+ return 1;
+
inode = d_inode(dentry);
task = get_proc_task(inode);
if (!task)
diff --git a/fs/proc/fd.c b/fs/proc/fd.c
index 96fc70225e54..68ec359c8161 100644
--- a/fs/proc/fd.c
+++ b/fs/proc/fd.c
@@ -91,6 +91,9 @@ static int tid_fd_revalidate(struct dentry *dentry, unsigned int flags)
if (flags & LOOKUP_RCU)
return -ECHILD;

+ if (flags & LOOKUP_JUMPED)
+ return 1;
+
inode = d_inode(dentry);
task = get_proc_task(inode);
fd = proc_fd(inode);
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index f05a659cdf34..5da4c2ed5f3b 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -135,7 +135,6 @@ enum dentry_d_lock_class

struct dentry_operations {
int (*d_revalidate)(struct dentry *, unsigned int);
- int (*d_weak_revalidate)(struct dentry *, unsigned int);
int (*d_hash)(const struct dentry *, struct qstr *);
int (*d_compare)(const struct dentry *,
unsigned int, const char *, const struct qstr *);
@@ -184,8 +183,6 @@ struct dentry_operations {
#define DCACHE_GENOCIDE 0x00000200
#define DCACHE_SHRINK_LIST 0x00000400

-#define DCACHE_OP_WEAK_REVALIDATE 0x00000800
-
#define DCACHE_NFSFS_RENAMED 0x00001000
/* this dentry has been "silly renamed" and has to be deleted on the last
* dput() */
--
2.14.0.rc0.dirty

Attachment: signature.asc
Description: PGP signature