These patches are some follow-on patches for the overhaul of the nfsd
DRC. They add a bit of extra stats gathering to the DRC code, and add a
new /proc/fs/nfsd/reply_cache_stats file to present them.
I had briefly considered just adding more fields to the "nfsd" stats
file that nfsstat scrapes, but doing that would have been problematic
since older kernels presented extra info on the "rc" line.
These patches are based on top of the ones that add the checksum
tracking to the DRC code. They're fairly simple and are probably
reasonable for 3.9, unless anyone objects.
Comments and suggestions welcome.
Jeff Layton (3):
nfsd: fix comments on nfsd_cache_lookup
nfsd: break out comparator into separate function
nfsd: add new reply_cache_stats file in nfsdfs
fs/nfsd/cache.h | 1 +
fs/nfsd/nfscache.c | 62 +++++++++++++++++++++++++++++++++++++++++++-----------
fs/nfsd/nfsctl.c | 9 ++++++++
3 files changed, 60 insertions(+), 12 deletions(-)
--
1.7.11.7
On Thu, 14 Feb 2013 17:08:02 -0500
"J. Bruce Fields" <[email protected]> wrote:
> On Thu, Feb 14, 2013 at 04:45:14PM -0500, Jeff Layton wrote:
> > Break 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.
>
> Why the checksum and length, as opposed to just the checksum?
>
> Seems to me that comparing the length is no more expensive than any of
> the other comparisons (xid, proc, etc.) that we're already doing, so
> we're less likely to wonder whether it's worth it.
>
> --b.
>
I guess I was thinking that the length was sort of a property of the
NFS hunk of the packet, so it belonged with the checksum. OTOH, you
have a good point...
It's fairly trivial to change, so I'll plan to respin this once others
have had a chance to comment.
Thanks...
> >
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > fs/nfsd/nfscache.c | 37 +++++++++++++++++++++++++++----------
> > 1 file changed, 27 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
> > index 2f9c2d2..bccf74f 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,30 @@ nfsd_cache_csum(struct svc_rqst *rqstp)
> > return csum;
> > }
> >
> > +static bool
> > +nfsd_cache_match(struct svc_rqst *rqstp, __wsum csum, struct svc_cacherep *rp)
> > +{
> > + __be32 xid = rqstp->rq_xid;
> > + u32 proto = rqstp->rq_prot,
> > + vers = rqstp->rq_vers,
> > + proc = rqstp->rq_proc;
> > +
> > + /* Check RPC header info first */
> > + if (xid != rp->c_xid || proc != rp->c_proc ||
> > + proto != rp->c_prot || vers != rp->c_vers ||
> > + !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;
> > +
> > + /* check contents of the NFS data */
> > + if (rqstp->rq_arg.len != rp->c_len || 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 +308,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
> >
--
Jeff Layton <[email protected]>
For presenting statistics relating to duplicate reply cache.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/cache.h | 1 +
fs/nfsd/nfscache.c | 18 ++++++++++++++++++
fs/nfsd/nfsctl.c | 9 +++++++++
3 files changed, 28 insertions(+)
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 bccf74f..517b92d 100644
--- a/fs/nfsd/nfscache.c
+++ b/fs/nfsd/nfscache.c
@@ -540,3 +540,21 @@ nfsd_cache_append(struct svc_rqst *rqstp, struct kvec *data)
vec->iov_len += data->iov_len;
return 1;
}
+
+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, "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 29c3f0d..5dea5af 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,
@@ -194,6 +195,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
@@ -1024,6 +1032,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
Break 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 <[email protected]>
---
fs/nfsd/nfscache.c | 37 +++++++++++++++++++++++++++----------
1 file changed, 27 insertions(+), 10 deletions(-)
diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
index 2f9c2d2..bccf74f 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,30 @@ nfsd_cache_csum(struct svc_rqst *rqstp)
return csum;
}
+static bool
+nfsd_cache_match(struct svc_rqst *rqstp, __wsum csum, struct svc_cacherep *rp)
+{
+ __be32 xid = rqstp->rq_xid;
+ u32 proto = rqstp->rq_prot,
+ vers = rqstp->rq_vers,
+ proc = rqstp->rq_proc;
+
+ /* Check RPC header info first */
+ if (xid != rp->c_xid || proc != rp->c_proc ||
+ proto != rp->c_prot || vers != rp->c_vers ||
+ !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;
+
+ /* check contents of the NFS data */
+ if (rqstp->rq_arg.len != rp->c_len || 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 +308,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
On Thu, Feb 14, 2013 at 04:45:13PM -0500, Jeff Layton wrote:
> Signed-off-by: Jeff Layton <[email protected]>
Applying, thanks.--b.
> ---
> fs/nfsd/nfscache.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
> index 40db57e..2f9c2d2 100644
> --- a/fs/nfsd/nfscache.c
> +++ b/fs/nfsd/nfscache.c
> @@ -302,8 +302,10 @@ nfsd_cache_search(struct svc_rqst *rqstp, __wsum csum)
>
> /*
> * Try to find an entry matching the current call in the cache. When none
> - * is found, we grab the oldest unlocked entry off the LRU list.
> - * Note that no operation within the loop may sleep.
> + * is found, we try to grab the oldest expired entry off the LRU list. If
> + * a suitable one isn't there, then drop the cache_lock and allocate a
> + * new one, then search again in case one got inserted while this thread
> + * didn't hold the lock.
> */
> int
> nfsd_cache_lookup(struct svc_rqst *rqstp)
> @@ -344,6 +346,7 @@ nfsd_cache_lookup(struct svc_rqst *rqstp)
> }
> }
>
> + /* Drop the lock and allocate a new entry */
> spin_unlock(&cache_lock);
> rp = nfsd_reply_cache_alloc();
> if (!rp) {
> --
> 1.7.11.7
>
On Thu, Feb 14, 2013 at 04:45:14PM -0500, Jeff Layton wrote:
> Break 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.
Why the checksum and length, as opposed to just the checksum?
Seems to me that comparing the length is no more expensive than any of
the other comparisons (xid, proc, etc.) that we're already doing, so
we're less likely to wonder whether it's worth it.
--b.
>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfsd/nfscache.c | 37 +++++++++++++++++++++++++++----------
> 1 file changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
> index 2f9c2d2..bccf74f 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,30 @@ nfsd_cache_csum(struct svc_rqst *rqstp)
> return csum;
> }
>
> +static bool
> +nfsd_cache_match(struct svc_rqst *rqstp, __wsum csum, struct svc_cacherep *rp)
> +{
> + __be32 xid = rqstp->rq_xid;
> + u32 proto = rqstp->rq_prot,
> + vers = rqstp->rq_vers,
> + proc = rqstp->rq_proc;
> +
> + /* Check RPC header info first */
> + if (xid != rp->c_xid || proc != rp->c_proc ||
> + proto != rp->c_prot || vers != rp->c_vers ||
> + !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;
> +
> + /* check contents of the NFS data */
> + if (rqstp->rq_arg.len != rp->c_len || 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 +308,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
>
On Thu, Feb 14, 2013 at 04:45:15PM -0500, Jeff Layton wrote:
> For presenting statistics relating to duplicate reply cache.
Looks to me like a reasonable way to do it. Holding off on applying for
now.
--b.
>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfsd/cache.h | 1 +
> fs/nfsd/nfscache.c | 18 ++++++++++++++++++
> fs/nfsd/nfsctl.c | 9 +++++++++
> 3 files changed, 28 insertions(+)
>
> 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 bccf74f..517b92d 100644
> --- a/fs/nfsd/nfscache.c
> +++ b/fs/nfsd/nfscache.c
> @@ -540,3 +540,21 @@ nfsd_cache_append(struct svc_rqst *rqstp, struct kvec *data)
> vec->iov_len += data->iov_len;
> return 1;
> }
> +
> +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, "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 29c3f0d..5dea5af 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,
> @@ -194,6 +195,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
> @@ -1024,6 +1032,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
>
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/nfscache.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
index 40db57e..2f9c2d2 100644
--- a/fs/nfsd/nfscache.c
+++ b/fs/nfsd/nfscache.c
@@ -302,8 +302,10 @@ nfsd_cache_search(struct svc_rqst *rqstp, __wsum csum)
/*
* Try to find an entry matching the current call in the cache. When none
- * is found, we grab the oldest unlocked entry off the LRU list.
- * Note that no operation within the loop may sleep.
+ * is found, we try to grab the oldest expired entry off the LRU list. If
+ * a suitable one isn't there, then drop the cache_lock and allocate a
+ * new one, then search again in case one got inserted while this thread
+ * didn't hold the lock.
*/
int
nfsd_cache_lookup(struct svc_rqst *rqstp)
@@ -344,6 +346,7 @@ nfsd_cache_lookup(struct svc_rqst *rqstp)
}
}
+ /* Drop the lock and allocate a new entry */
spin_unlock(&cache_lock);
rp = nfsd_reply_cache_alloc();
if (!rp) {
--
1.7.11.7