[PATCH v2 0/5] nfsd: add a reply_cache_stats file to nfsd for tracking DRC performance

[PATCH v2 0/5] nfsd: add a reply_cache_stats file to nfsd for tracking DRC performance

[Jeff Layton]

This patchset adds some stats tracking to the DRC to hopefully allow
us to make better-informed decisions in the future on performance
improvements.

Some of these I've already posted, but reposting them now as a set
to allow Bruce to more easily merge them. I think these would be good
for 3.9, but they're obviously not critical.

Jeff Layton (5):
  nfsd: break out comparator into separate function
  nfsd: track memory utilization in the DRC
  nfsd: add new reply_cache_stats file in nfsdfs
  nfsd: report length of the longest hash chain in reply cache stats
  nfsd: keep track of the max and average time to search the cache

 fs/nfsd/cache.h    |   1 +
 fs/nfsd/nfscache.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++-------
 fs/nfsd/nfsctl.c   |   9 ++++
 3 files changed, 117 insertions(+), 16 deletions(-)

-- 
1.7.11.7

[PATCH v2 1/5] nfsd: break out comparator into separate function

[Jeff Layton]

Break out the function that compares the rqstp and checksum against a
reply cache entry. While we're at it, track the efficacy of the checksum
over the NFS data by tracking the cases where we would have incorrectly
matched a DRC entry if we had not tracked it or the length.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfscache.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
index ca43664..8986568 100644
--- a/fs/nfsd/nfscache.c
+++ b/fs/nfsd/nfscache.c
@@ -25,6 +25,7 @@ static struct list_head 	lru_head;
 static struct kmem_cache	*drc_slab;
 static unsigned int		num_drc_entries;
 static unsigned int		max_drc_entries;
+static unsigned int		csum_misses;
 
 /*
  * Calculate the hash index from an XID.
@@ -272,6 +273,26 @@ nfsd_cache_csum(struct svc_rqst *rqstp)
 	return csum;
 }
 
+static bool
+nfsd_cache_match(struct svc_rqst *rqstp, __wsum csum, struct svc_cacherep *rp)
+{
+	/* Check RPC header info first */
+	if (rqstp->rq_xid != rp->c_xid || rqstp->rq_proc != rp->c_proc ||
+	    rqstp->rq_prot != rp->c_prot || rqstp->rq_vers != rp->c_vers ||
+	    rqstp->rq_arg.len != rp->c_len ||
+	    !rpc_cmp_addr(svc_addr(rqstp), (struct sockaddr *)&rp->c_addr) ||
+	    rpc_get_port(svc_addr(rqstp)) != rpc_get_port((struct sockaddr *)&rp->c_addr))
+		return false;
+
+	/* compare checksum of NFS data */
+	if (csum != rp->c_csum) {
+		++csum_misses;
+		return false;
+	}
+
+	return true;
+}
+
 /*
  * Search the request hash for an entry that matches the given rqstp.
  * Must be called with cache_lock held. Returns the found entry or
@@ -283,18 +304,10 @@ nfsd_cache_search(struct svc_rqst *rqstp, __wsum csum)
 	struct svc_cacherep	*rp;
 	struct hlist_node	*hn;
 	struct hlist_head 	*rh;
-	__be32			xid = rqstp->rq_xid;
-	u32			proto =  rqstp->rq_prot,
-				vers = rqstp->rq_vers,
-				proc = rqstp->rq_proc;
 
-	rh = &cache_hash[request_hash(xid)];
+	rh = &cache_hash[request_hash(rqstp->rq_xid)];
 	hlist_for_each_entry(rp, hn, rh, c_hash) {
-		if (xid == rp->c_xid && proc == rp->c_proc &&
-		    proto == rp->c_prot && vers == rp->c_vers &&
-		    rqstp->rq_arg.len == rp->c_len && csum == rp->c_csum &&
-		    rpc_cmp_addr(svc_addr(rqstp), (struct sockaddr *)&rp->c_addr) &&
-		    rpc_get_port(svc_addr(rqstp)) == rpc_get_port((struct sockaddr *)&rp->c_addr))
+		if (nfsd_cache_match(rqstp, csum, rp))
 			return rp;
 	}
 	return NULL;
-- 
1.7.11.7

[PATCH v2 2/5] nfsd: track memory utilization in the DRC

[Jeff Layton]

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfscache.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
index 8986568..efed50e 100644
--- a/fs/nfsd/nfscache.c
+++ b/fs/nfsd/nfscache.c
@@ -25,6 +25,7 @@ static struct list_head 	lru_head;
 static struct kmem_cache	*drc_slab;
 static unsigned int		num_drc_entries;
 static unsigned int		max_drc_entries;
+static unsigned int		drc_mem_usage;
 static unsigned int		csum_misses;
 
 /*
@@ -101,11 +102,14 @@ nfsd_reply_cache_alloc(void)
 static void
 nfsd_reply_cache_free_locked(struct svc_cacherep *rp)
 {
-	if (rp->c_type == RC_REPLBUFF)
+	if (rp->c_type == RC_REPLBUFF && rp->c_replvec.iov_base) {
+		drc_mem_usage -= ksize(rp->c_replvec.iov_base);
 		kfree(rp->c_replvec.iov_base);
+	}
 	hlist_del(&rp->c_hash);
 	list_del(&rp->c_lru);
 	--num_drc_entries;
+	drc_mem_usage -= ksize(rp);
 	kmem_cache_free(drc_slab, rp);
 }
 
@@ -367,6 +371,7 @@ nfsd_cache_lookup(struct svc_rqst *rqstp)
 		return RC_DOIT;
 	}
 	spin_lock(&cache_lock);
+	drc_mem_usage += ksize(rp);
 	++num_drc_entries;
 
 	/*
@@ -407,6 +412,7 @@ setup_entry:
 
 	/* release any buffer */
 	if (rp->c_type == RC_REPLBUFF) {
+		drc_mem_usage -= ksize(rp->c_replvec.iov_base);
 		kfree(rp->c_replvec.iov_base);
 		rp->c_replvec.iov_base = NULL;
 	}
@@ -475,6 +481,7 @@ nfsd_cache_update(struct svc_rqst *rqstp, int cachetype, __be32 *statp)
 	struct svc_cacherep *rp = rqstp->rq_cacherep;
 	struct kvec	*resv = &rqstp->rq_res.head[0], *cachv;
 	int		len;
+	size_t		replbufsize = 0;
 
 	if (!rp)
 		return;
@@ -501,6 +508,7 @@ nfsd_cache_update(struct svc_rqst *rqstp, int cachetype, __be32 *statp)
 			nfsd_reply_cache_free(rp);
 			return;
 		}
+		replbufsize = ksize(cachv->iov_base);
 		cachv->iov_len = len << 2;
 		memcpy(cachv->iov_base, statp, len << 2);
 		break;
@@ -509,6 +517,7 @@ nfsd_cache_update(struct svc_rqst *rqstp, int cachetype, __be32 *statp)
 		return;
 	}
 	spin_lock(&cache_lock);
+	drc_mem_usage += replbufsize;
 	lru_put_end(rp);
 	rp->c_secure = rqstp->rq_secure;
 	rp->c_type = cachetype;
-- 
1.7.11.7

[PATCH v2 3/5] nfsd: add new reply_cache_stats file in nfsdfs

[Jeff Layton]

For presenting statistics relating to duplicate reply cache.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/cache.h    |  1 +
 fs/nfsd/nfscache.c | 37 +++++++++++++++++++++++++++++++++----
 fs/nfsd/nfsctl.c   |  9 +++++++++
 3 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/cache.h b/fs/nfsd/cache.h
index 87fd141..d5c5b3e 100644
--- a/fs/nfsd/cache.h
+++ b/fs/nfsd/cache.h
@@ -82,6 +82,7 @@ int	nfsd_reply_cache_init(void);
 void	nfsd_reply_cache_shutdown(void);
 int	nfsd_cache_lookup(struct svc_rqst *);
 void	nfsd_cache_update(struct svc_rqst *, int, __be32 *);
+int	nfsd_reply_cache_stats_open(struct inode *, struct file *);
 
 #ifdef CONFIG_NFSD_V4
 void	nfsd4_set_statp(struct svc_rqst *rqstp, __be32 *statp);
diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
index efed50e..a5ac9ab 100644
--- a/fs/nfsd/nfscache.c
+++ b/fs/nfsd/nfscache.c
@@ -23,12 +23,17 @@
 static struct hlist_head *	cache_hash;
 static struct list_head 	lru_head;
 static struct kmem_cache	*drc_slab;
-static unsigned int		num_drc_entries;
-static unsigned int		max_drc_entries;
-static unsigned int		drc_mem_usage;
-static unsigned int		csum_misses;
 
 /*
+ * Stats on the duplicate reply cache code. All of these and the "rc" fields
+ * in nfsdstats are protected by the cache_lock
+ */
+static unsigned int		num_drc_entries; /* number of entries */
+static unsigned int		max_drc_entries; /* max number of entries */
+static unsigned int		drc_mem_usage;   /* memory used by cache */
+static unsigned int		csum_misses;     /* cache misses due only to
+						    csum comparison failures */
+/*
  * Calculate the hash index from an XID.
  */
 static inline u32 request_hash(u32 xid)
@@ -545,3 +550,27 @@ nfsd_cache_append(struct svc_rqst *rqstp, struct kvec *data)
 	vec->iov_len += data->iov_len;
 	return 1;
 }
+
+/*
+ * Note that fields may be added, removed or reordered in the future. Programs
+ * scraping this file for info should test the labels to ensure they're
+ * getting the correct field.
+ */
+static int nfsd_reply_cache_stats_show(struct seq_file *m, void *v)
+{
+	spin_lock(&cache_lock);
+	seq_printf(m, "max entries:     %u\n", max_drc_entries);
+	seq_printf(m, "num entries:     %u\n", num_drc_entries);
+	seq_printf(m, "mem usage:       %u\n", drc_mem_usage);
+	seq_printf(m, "cache hits:      %u\n", nfsdstats.rchits);
+	seq_printf(m, "cache misses:    %u\n", nfsdstats.rcmisses);
+	seq_printf(m, "not cached:      %u\n", nfsdstats.rcnocache);
+	seq_printf(m, "checksum misses: %u\n", csum_misses);
+	spin_unlock(&cache_lock);
+	return 0;
+}
+
+int nfsd_reply_cache_stats_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, nfsd_reply_cache_stats_show, NULL);
+}
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 8ead2c2..c7238a5 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -35,6 +35,7 @@ enum {
 	NFSD_Threads,
 	NFSD_Pool_Threads,
 	NFSD_Pool_Stats,
+	NFSD_Reply_Cache_Stats,
 	NFSD_Versions,
 	NFSD_Ports,
 	NFSD_MaxBlkSize,
@@ -212,6 +213,13 @@ static const struct file_operations pool_stats_operations = {
 	.owner		= THIS_MODULE,
 };
 
+static struct file_operations reply_cache_stats_operations = {
+	.open		= nfsd_reply_cache_stats_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
 /*----------------------------------------------------------------------------*/
 /*
  * payload - write methods
@@ -1047,6 +1055,7 @@ static int nfsd_fill_super(struct super_block * sb, void * data, int silent)
 		[NFSD_Threads] = {"threads", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_Pool_Threads] = {"pool_threads", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_Pool_Stats] = {"pool_stats", &pool_stats_operations, S_IRUGO},
+		[NFSD_Reply_Cache_Stats] = {"reply_cache_stats", &reply_cache_stats_operations, S_IRUGO},
 		[NFSD_Versions] = {"versions", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_Ports] = {"portlist", &transaction_ops, S_IWUSR|S_IRUGO},
 		[NFSD_MaxBlkSize] = {"max_block_size", &transaction_ops, S_IWUSR|S_IRUGO},
-- 
1.7.11.7

[PATCH v2 4/5] nfsd: report length of the longest hash chain in reply cache stats

[Jeff Layton]

So we can get a feel for how effective the hashing function is.

As Chuck Lever pointed out to me, it's generally acceptable to do
"expensive" stuff when reading the stats since that's a relatively
rare activity. When we go to read the file, walk all of the hash
chains and count the number of entries in each. Report the length
of the longest one.

Cc: Chuck Lever <chuck.lever@oracle.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfscache.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
index a5ac9ab..52493cb 100644
--- a/fs/nfsd/nfscache.c
+++ b/fs/nfsd/nfscache.c
@@ -552,6 +552,28 @@ nfsd_cache_append(struct svc_rqst *rqstp, struct kvec *data)
 }
 
 /*
+ * Walk each hash chain and count the number of entries. Return the length of
+ * the longest one. Must be called with the cache_lock held.
+ */
+static unsigned int
+nfsd_repcache_max_chain_len(void)
+{
+	int i;
+	struct hlist_node *pos;
+	unsigned int max = 0;
+
+	for (i = 0; i < HASHSIZE; ++i) {
+		unsigned int cur = 0;
+
+		hlist_for_each(pos, &cache_hash[i])
+			++cur;
+		max = max(cur, max);
+	}
+
+	return max;
+}
+
+/*
  * Note that fields may be added, removed or reordered in the future. Programs
  * scraping this file for info should test the labels to ensure they're
  * getting the correct field.
@@ -566,6 +588,7 @@ static int nfsd_reply_cache_stats_show(struct seq_file *m, void *v)
 	seq_printf(m, "cache misses:    %u\n", nfsdstats.rcmisses);
 	seq_printf(m, "not cached:      %u\n", nfsdstats.rcnocache);
 	seq_printf(m, "checksum misses: %u\n", csum_misses);
+	seq_printf(m, "max chain len:   %u\n", nfsd_repcache_max_chain_len());
 	spin_unlock(&cache_lock);
 	return 0;
 }
-- 
1.7.11.7

[PATCH v2 5/5] nfsd: keep track of the max and average time to search the cache

[Jeff Layton]

Since we're moderately concerned about the potential increase in hash
chain lengths, keep a running average of the time and the max time to
search.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfscache.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
index 52493cb..ae6575d 100644
--- a/fs/nfsd/nfscache.c
+++ b/fs/nfsd/nfscache.c
@@ -33,6 +33,9 @@ static unsigned int		max_drc_entries; /* max number of entries */
 static unsigned int		drc_mem_usage;   /* memory used by cache */
 static unsigned int		csum_misses;     /* cache misses due only to
 						    csum comparison failures */
+static unsigned int		max_search_time; /* max time to search */
+static unsigned int		avg_search_time; /* avg time to search */
+
 /*
  * Calculate the hash index from an XID.
  */
@@ -310,16 +313,28 @@ nfsd_cache_match(struct svc_rqst *rqstp, __wsum csum, struct svc_cacherep *rp)
 static struct svc_cacherep *
 nfsd_cache_search(struct svc_rqst *rqstp, __wsum csum)
 {
-	struct svc_cacherep	*rp;
+	struct svc_cacherep	*rp, *ret = NULL;
 	struct hlist_node	*hn;
 	struct hlist_head 	*rh;
+	ktime_t			start;
+	unsigned int		delta;
+	static unsigned int	nsearches = 0;
+	unsigned int		osearches;
 
+	start = ktime_get();
 	rh = &cache_hash[request_hash(rqstp->rq_xid)];
 	hlist_for_each_entry(rp, hn, rh, c_hash) {
-		if (nfsd_cache_match(rqstp, csum, rp))
-			return rp;
+		if (nfsd_cache_match(rqstp, csum, rp)) {
+			ret = rp;
+			break;
+		}
 	}
-	return NULL;
+	delta = (unsigned int)ktime_to_ns(ktime_sub(ktime_get(), start));
+	osearches = nsearches++;
+	avg_search_time = (avg_search_time * osearches + delta) / nsearches;
+	max_search_time = max(max_search_time, delta);
+
+	return ret;
 }
 
 /*
@@ -589,6 +604,8 @@ static int nfsd_reply_cache_stats_show(struct seq_file *m, void *v)
 	seq_printf(m, "not cached:      %u\n", nfsdstats.rcnocache);
 	seq_printf(m, "checksum misses: %u\n", csum_misses);
 	seq_printf(m, "max chain len:   %u\n", nfsd_repcache_max_chain_len());
+	seq_printf(m, "avg search time: %uns\n", avg_search_time);
+	seq_printf(m, "max search time: %uns\n", max_search_time);
 	spin_unlock(&cache_lock);
 	return 0;
 }
-- 
1.7.11.7

[PATCH v2 0/5] nfsd: add a reply_cache_stats file to nfsd for tracking DRC performance

[Jeff Layton]

This patchset adds some stats tracking to the DRC to hopefully allow
us to make better-informed decisions in the future on performance
improvements.

Some of these I've already posted, but reposting them now as a set
to allow Bruce to more easily merge them. I think these would be good
for 3.9, but they're obviously not critical.

Jeff Layton (5):
  nfsd: break out comparator into separate function
  nfsd: track memory utilization in the DRC
  nfsd: add new reply_cache_stats file in nfsdfs
  nfsd: keep track of the max and average time to search the cache
  nfsd: keep stats on worst hash balancing seen so far

 fs/nfsd/cache.h    |   1 +
 fs/nfsd/nfscache.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++-------
 fs/nfsd/nfsctl.c   |   9 ++++
 3 files changed, 137 insertions(+), 17 deletions(-)

-- 
1.7.11.7

Re: [PATCH v2 0/5] nfsd: add a reply_cache_stats file to nfsd for tracking DRC performance

[J. Bruce Fields]

On Mon, Mar 04, 2013 at 01:56:35PM -0500, Jeff Layton wrote:
> This patchset adds some stats tracking to the DRC to hopefully allow
> us to make better-informed decisions in the future on performance
> improvements.
> 
> Some of these I've already posted, but reposting them now as a set
> to allow Bruce to more easily merge them. I think these would be good
> for 3.9, but they're obviously not critical.

OK, thanks.  I'll queue them up for 3.10.

--b.

> 
> Jeff Layton (5):
>   nfsd: break out comparator into separate function
>   nfsd: track memory utilization in the DRC
>   nfsd: add new reply_cache_stats file in nfsdfs
>   nfsd: keep track of the max and average time to search the cache
>   nfsd: keep stats on worst hash balancing seen so far
> 
>  fs/nfsd/cache.h    |   1 +
>  fs/nfsd/nfscache.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++-------
>  fs/nfsd/nfsctl.c   |   9 ++++
>  3 files changed, 137 insertions(+), 17 deletions(-)
> 
> -- 
> 1.7.11.7
>