[RFC/RFT PATCH 0/3] d_time removal

[RFC/RFT PATCH 0/3] d_time removal

[Miklos Szeredi]

Only two filesystems remaining: nfs and ncpfs.  Both use d_fsdata as well
as d_time which means we have to allocate a separate structure (RCU freed
in case of NFS).

I still haven't tested these; hoping someone will do it for me.

Thanks,
Miklos

---
Miklos Szeredi (3):
  nfs: don't use ->d_time
  ncpfs: don't use ->d_time
  vfs: remove ->d_time

 fs/ncpfs/dir.c           | 21 ++++++++++++++++++---
 fs/ncpfs/ncplib_kernel.h | 16 +++++++++++++---
 fs/nfs/dir.c             | 18 ++++++++++++++----
 fs/nfs/getroot.c         |  4 ++--
 fs/nfs/namespace.c       |  2 +-
 fs/nfs/unlink.c          | 16 ++++++++--------
 include/linux/dcache.h   |  7 +++----
 include/linux/nfs_fs.h   | 19 ++++++++++++++++++-
 8 files changed, 77 insertions(+), 26 deletions(-)

-- 
2.5.5

[PATCH 1/3] nfs: don't use ->d_time

[Miklos Szeredi]

NFS is the most complicated of the lot.  It uses ->d_fsdata to

 1) store devname in the root dentry, and
 2) store nfs_unlinkdata for sillyrenames.

In addition it stores a verifier in ->d_time.

Introduce nfs_dentry structure that to store all of the above and make
d_fsdata point to it.  Need to use rcu freeing since ->verf is accessed
from dentry revalidation.  And not only read, but changed as well, so
rcu_head must not overlay verf.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Cc: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/dir.c           | 18 ++++++++++++++----
 fs/nfs/getroot.c       |  4 ++--
 fs/nfs/namespace.c     |  2 +-
 fs/nfs/unlink.c        | 16 ++++++++--------
 include/linux/nfs_fs.h | 19 ++++++++++++++++++-
 5 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 5f1af4cd1a33..5a3f02c4c28a 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1041,7 +1041,7 @@ static int nfs_check_verifier(struct inode *dir, struct dentry *dentry,
 		return 1;
 	if (NFS_SERVER(dir)->flags & NFS_MOUNT_LOOKUP_CACHE_NONE)
 		return 0;
-	if (!nfs_verify_change_attribute(dir, dentry->d_time))
+	if (!nfs_verify_change_attribute(dir, NFS_D(dentry)->verf))
 		return 0;
 	/* Revalidate nfsi->cache_change_attribute before we declare a match */
 	if (rcu_walk)
@@ -1050,7 +1050,7 @@ static int nfs_check_verifier(struct inode *dir, struct dentry *dentry,
 		ret = nfs_revalidate_inode(NFS_SERVER(dir), dir);
 	if (ret < 0)
 		return 0;
-	if (!nfs_verify_change_attribute(dir, dentry->d_time))
+	if (!nfs_verify_change_attribute(dir, NFS_D(dentry)->verf))
 		return 0;
 	return 1;
 }
@@ -1358,15 +1358,23 @@ static void nfs_dentry_iput(struct dentry *dentry, struct inode *inode)
 	iput(inode);
 }
 
+static int nfs_d_init(struct dentry *dentry)
+{
+	dentry->d_fsdata = kzalloc(sizeof(struct nfs_dentry), GFP_KERNEL);
+
+	return dentry->d_fsdata ? 0 : -ENOMEM;
+}
+
 static void nfs_d_release(struct dentry *dentry)
 {
 	/* free cached devname value, if it survived that far */
-	if (unlikely(dentry->d_fsdata)) {
+	if (unlikely(NFS_D(dentry)->devname)) {
 		if (dentry->d_flags & DCACHE_NFSFS_RENAMED)
 			WARN_ON(1);
 		else
-			kfree(dentry->d_fsdata);
+			kfree(NFS_D(dentry)->devname);
 	}
+	kfree_rcu(NFS_D(dentry), rcu);
 }
 
 const struct dentry_operations nfs_dentry_operations = {
@@ -1375,6 +1383,7 @@ const struct dentry_operations nfs_dentry_operations = {
 	.d_delete	= nfs_dentry_delete,
 	.d_iput		= nfs_dentry_iput,
 	.d_automount	= nfs_d_automount,
+	.d_init		= nfs_d_init,
 	.d_release	= nfs_d_release,
 };
 EXPORT_SYMBOL_GPL(nfs_dentry_operations);
@@ -1453,6 +1462,7 @@ const struct dentry_operations nfs4_dentry_operations = {
 	.d_delete	= nfs_dentry_delete,
 	.d_iput		= nfs_dentry_iput,
 	.d_automount	= nfs_d_automount,
+	.d_init		= nfs_d_init,
 	.d_release	= nfs_d_release,
 };
 EXPORT_SYMBOL_GPL(nfs4_dentry_operations);
diff --git a/fs/nfs/getroot.c b/fs/nfs/getroot.c
index a608ffd28acc..84482c656e05 100644
--- a/fs/nfs/getroot.c
+++ b/fs/nfs/getroot.c
@@ -120,9 +120,9 @@ struct dentry *nfs_get_root(struct super_block *sb, struct nfs_fh *mntfh,
 
 	security_d_instantiate(ret, inode);
 	spin_lock(&ret->d_lock);
-	if (IS_ROOT(ret) && !ret->d_fsdata &&
+	if (IS_ROOT(ret) && !NFS_D(ret)->devname &&
 	    !(ret->d_flags & DCACHE_NFSFS_RENAMED)) {
-		ret->d_fsdata = name;
+		NFS_D(ret)->devname = name;
 		name = NULL;
 	}
 	spin_unlock(&ret->d_lock);
diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
index c8162c660c44..0060806d047b 100644
--- a/fs/nfs/namespace.c
+++ b/fs/nfs/namespace.c
@@ -90,7 +90,7 @@ char *nfs_path(char **p, struct dentry *dentry, char *buffer, ssize_t buflen,
 		*--end = '/';
 	}
 	*p = end;
-	base = dentry->d_fsdata;
+	base = NFS_D(dentry)->devname;
 	if (!base) {
 		spin_unlock(&dentry->d_lock);
 		rcu_read_unlock();
diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
index 191aa577dd1f..74e708fbd945 100644
--- a/fs/nfs/unlink.c
+++ b/fs/nfs/unlink.c
@@ -134,8 +134,8 @@ static int nfs_call_unlink(struct dentry *dentry, struct nfs_unlinkdata *data)
 		spin_lock(&alias->d_lock);
 		if (d_really_is_positive(alias) &&
 		    !(alias->d_flags & DCACHE_NFSFS_RENAMED)) {
-			devname_garbage = alias->d_fsdata;
-			alias->d_fsdata = data;
+			devname_garbage = NFS_D(alias)->devname;
+			NFS_D(alias)->data = data;
 			alias->d_flags |= DCACHE_NFSFS_RENAMED;
 			ret = 1;
 		} else
@@ -189,8 +189,8 @@ nfs_async_unlink(struct dentry *dentry, const struct qstr *name)
 	if (dentry->d_flags & DCACHE_NFSFS_RENAMED)
 		goto out_unlock;
 	dentry->d_flags |= DCACHE_NFSFS_RENAMED;
-	devname_garbage = dentry->d_fsdata;
-	dentry->d_fsdata = data;
+	devname_garbage = NFS_D(dentry)->devname;
+	NFS_D(dentry)->data = data;
 	spin_unlock(&dentry->d_lock);
 	/*
 	 * If we'd displaced old cached devname, free it.  At that
@@ -226,8 +226,8 @@ nfs_complete_unlink(struct dentry *dentry, struct inode *inode)
 
 	spin_lock(&dentry->d_lock);
 	dentry->d_flags &= ~DCACHE_NFSFS_RENAMED;
-	data = dentry->d_fsdata;
-	dentry->d_fsdata = NULL;
+	data = NFS_D(dentry)->data;
+	NFS_D(dentry)->data = NULL;
 	spin_unlock(&dentry->d_lock);
 
 	if (NFS_STALE(inode) || !nfs_call_unlink(dentry, data))
@@ -240,10 +240,10 @@ nfs_cancel_async_unlink(struct dentry *dentry)
 {
 	spin_lock(&dentry->d_lock);
 	if (dentry->d_flags & DCACHE_NFSFS_RENAMED) {
-		struct nfs_unlinkdata *data = dentry->d_fsdata;
+		struct nfs_unlinkdata *data = NFS_D(dentry)->data;
 
 		dentry->d_flags &= ~DCACHE_NFSFS_RENAMED;
-		dentry->d_fsdata = NULL;
+		NFS_D(dentry)->data = NULL;
 		spin_unlock(&dentry->d_lock);
 		nfs_free_unlinkdata(data);
 		return;
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 810124b33327..20a07688a392 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -187,6 +187,18 @@ struct nfs_inode {
 };
 
 /*
+ * NFS specific dentry data
+ */
+struct nfs_dentry {
+	union {
+		char *devname;
+		struct nfs_unlinkdata *data;
+		struct rcu_head rcu;
+	};
+	unsigned long verf;
+};
+
+/*
  * Cache validity bit flags
  */
 #define NFS_INO_INVALID_ATTR	0x0001		/* cached attrs are invalid */
@@ -217,6 +229,11 @@ static inline struct nfs_inode *NFS_I(const struct inode *inode)
 	return container_of(inode, struct nfs_inode, vfs_inode);
 }
 
+static inline struct nfs_dentry *NFS_D(struct dentry *dentry)
+{
+	return (struct nfs_dentry *) dentry->d_fsdata;
+}
+
 static inline struct nfs_server *NFS_SB(const struct super_block *s)
 {
 	return (struct nfs_server *)(s->s_fs_info);
@@ -299,7 +316,7 @@ static inline int nfs_server_capable(struct inode *inode, int cap)
 
 static inline void nfs_set_verifier(struct dentry * dentry, unsigned long verf)
 {
-	dentry->d_time = verf;
+	NFS_D(dentry)->verf = verf;
 }
 
 /**
-- 
2.5.5

[PATCH 2/3] ncpfs: don't use ->d_time

[Miklos Szeredi]

Ncpfs stores a boolean value in ->d_fsdata as well as using d_time.
Introcude ncp_dentry to store both of these and make d_fsdata point to it.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/ncpfs/dir.c           | 21 ++++++++++++++++++---
 fs/ncpfs/ncplib_kernel.h | 16 +++++++++++++---
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/fs/ncpfs/dir.c b/fs/ncpfs/dir.c
index 6df2a3827574..ce42dbdde777 100644
--- a/fs/ncpfs/dir.c
+++ b/fs/ncpfs/dir.c
@@ -72,6 +72,8 @@ const struct inode_operations ncp_dir_inode_operations =
 /*
  * Dentry operations routines
  */
+static int ncp_d_init(struct dentry *);
+static void ncp_d_release(struct dentry *);
 static int ncp_lookup_validate(struct dentry *, unsigned int);
 static int ncp_hash_dentry(const struct dentry *, struct qstr *);
 static int ncp_compare_dentry(const struct dentry *,
@@ -81,6 +83,8 @@ static void ncp_d_prune(struct dentry *dentry);
 
 const struct dentry_operations ncp_dentry_operations =
 {
+	.d_init		= ncp_d_init,
+	.d_release	= ncp_d_release,
 	.d_revalidate	= ncp_lookup_validate,
 	.d_hash		= ncp_hash_dentry,
 	.d_compare	= ncp_compare_dentry,
@@ -305,6 +309,17 @@ leave_me:;
 }
 #endif	/* CONFIG_NCPFS_STRONG */
 
+static int ncp_d_init(struct dentry *dentry)
+{
+	dentry->d_fsdata = kzalloc(sizeof(struct ncp_dentry), GFP_KERNEL);
+
+	return dentry->d_fsdata ? 0 : -ENOMEM;
+}
+
+static void ncp_d_release(struct dentry *dentry)
+{
+	kfree(dentry->d_fsdata);
+}
 
 static int
 ncp_lookup_validate(struct dentry *dentry, unsigned int flags)
@@ -408,7 +423,7 @@ ncp_invalidate_dircache_entries(struct dentry *parent)
 
 	spin_lock(&parent->d_lock);
 	list_for_each_entry(dentry, &parent->d_subdirs, d_child) {
-		dentry->d_fsdata = NULL;
+		NCP_DENTRY(dentry)->cached = false;
 		ncp_age_dentry(server, dentry);
 	}
 	spin_unlock(&parent->d_lock);
@@ -568,7 +583,7 @@ static int ncp_readdir(struct file *file, struct dir_context *ctx)
 
 static void ncp_d_prune(struct dentry *dentry)
 {
-	if (!dentry->d_fsdata)	/* not referenced from page cache */
+	if (!NCP_DENTRY(dentry)->cached) /* not referenced from page cache */
 		return;
 	NCP_FINFO(d_inode(dentry->d_parent))->flags &= ~NCPI_DIR_CACHE;
 }
@@ -659,7 +674,7 @@ ncp_fill_cache(struct file *file, struct dir_context *ctx,
 	}
 	if (ctl.cache) {
 		if (d_really_is_positive(newdent)) {
-			newdent->d_fsdata = newdent;
+			NCP_DENTRY(newdent)->cached = true;
 			ctl.cache->dentry[ctl.idx] = newdent;
 			ino = d_inode(newdent)->i_ino;
 			ncp_new_dentry(newdent);
diff --git a/fs/ncpfs/ncplib_kernel.h b/fs/ncpfs/ncplib_kernel.h
index 17cfb743b5bf..76459e4f6b64 100644
--- a/fs/ncpfs/ncplib_kernel.h
+++ b/fs/ncpfs/ncplib_kernel.h
@@ -168,20 +168,30 @@ static inline int ncp_strnicmp(const struct nls_table *t,
 
 #endif /* CONFIG_NCPFS_NLS */
 
-#define NCP_GET_AGE(dentry)	(jiffies - (dentry)->d_time)
+struct ncp_dentry {
+	unsigned long time;
+	bool cached;
+};
+
+static inline struct ncp_dentry *NCP_DENTRY(struct dentry *dentry)
+{
+	return (struct ncp_dentry *) dentry->d_fsdata;
+}
+
+#define NCP_GET_AGE(dentry)	(jiffies - NCP_DENTRY(dentry)->time)
 #define NCP_MAX_AGE(server)	atomic_read(&(server)->dentry_ttl)
 #define NCP_TEST_AGE(server,dentry)	(NCP_GET_AGE(dentry) < NCP_MAX_AGE(server))
 
 static inline void
 ncp_age_dentry(struct ncp_server* server, struct dentry* dentry)
 {
-	dentry->d_time = jiffies - NCP_MAX_AGE(server);
+	NCP_DENTRY(dentry)->time = jiffies - NCP_MAX_AGE(server);
 }
 
 static inline void
 ncp_new_dentry(struct dentry* dentry)
 {
-	dentry->d_time = jiffies;
+	NCP_DENTRY(dentry)->time = jiffies;
 }
 
 struct ncp_cache_head {
-- 
2.5.5

[PATCH 3/3] vfs: remove ->d_time

[Miklos Szeredi]

Remove dentry->d_time field, which is now unused.  This makes space for
longer inline names.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 include/linux/dcache.h | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 5beed7b30561..0061089099a8 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -69,12 +69,12 @@ extern struct dentry_stat_t dentry_stat;
  * large memory footprint increase).
  */
 #ifdef CONFIG_64BIT
-# define DNAME_INLINE_LEN 32 /* 192 bytes */
+# define DNAME_INLINE_LEN 40 /* 192 bytes */
 #else
 # ifdef CONFIG_SMP
-#  define DNAME_INLINE_LEN 36 /* 128 bytes */
-# else
 #  define DNAME_INLINE_LEN 40 /* 128 bytes */
+# else
+#  define DNAME_INLINE_LEN 44 /* 128 bytes */
 # endif
 #endif
 
@@ -95,7 +95,6 @@ struct dentry {
 	struct lockref d_lockref;	/* per-dentry lock and refcount */
 	const struct dentry_operations *d_op;
 	struct super_block *d_sb;	/* The root of the dentry tree */
-	unsigned long d_time;		/* used by d_revalidate */
 	void *d_fsdata;			/* fs-specific data */
 
 	union {
-- 
2.5.5

Re: [RFC/RFT PATCH 0/3] d_time removal

[Al Viro]

On Mon, Nov 07, 2016 at 11:51:57AM +0100, Miklos Szeredi wrote:
> Only two filesystems remaining: nfs and ncpfs.  Both use d_fsdata as well
> as d_time which means we have to allocate a separate structure (RCU freed
> in case of NFS).
> 
> I still haven't tested these; hoping someone will do it for me.

I would suggest profiling the NFS part - you are introducing a separate
allocation for every dentry there, which could get unpleasant for something
like dcache seeding in readdir.  Another interesting part is the extra
cachelines accessed in ->d_revalidate().

In principle, getting rid of ->d_time is nice, but the benefits are not
all that impressive - slightly longer embedded names, but how many files
have names between 32 and 40 characters long?
<checks on a fairly large local box>
33 0.201368%
34 0.183738%
35 0.154355%
36 0.133560%
37 0.117422%
38 0.963897%
39 0.083879%
40 0.156796%

IOW, here it's just a bit under 2% - not a lot.  Getting rid of fs-specific
fields in struct dentry per se...  Fine, but that'd better not come at the
cost of appreciable NFS overhead.