Re: [patch 03/29] knfsd: add userspace controls for stats tables

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Apr 27, 2009, at 7:22 PM, J. Bruce Fields wrote:

On Mon, Apr 27, 2009 at 12:06:18PM -0400, Chuck Lever wrote:
On Apr 25, 2009, at 6:03 PM, J. Bruce Fields wrote:
Pfft, did it again.

--b.

On Sat, Apr 25, 2009 at 05:57:45PM -0400, bfields wrote:
On Wed, Apr 01, 2009 at 07:28:03AM +1100, Greg Banks wrote:
Add two control files to /proc/fs/nfsd:

* "stats_enabled" can be used to disable or enable the gathering
 of per-client and per-export statistics in the server.

* "stats_prune_period" can be used to set the period at
 which the pruning timer runs, in seconds.  Unused stats
 entries will survive at most twice that time.

Signed-off-by: Greg Banks <gnb@xxxxxxx>
---

fs/nfsd/nfsctl.c | 99 +++++++++++++++++++++++++++++++++++++++++ +++
1 file changed, 99 insertions(+)

Index: bfields/fs/nfsd/nfsctl.c
= ==================================================================
--- bfields.orig/fs/nfsd/nfsctl.c
+++ bfields/fs/nfsd/nfsctl.c
@@ -64,6 +64,8 @@ enum {
	NFSD_Versions,
	NFSD_Ports,
	NFSD_MaxBlkSize,
+	NFSD_Stats_Enabled,
+	NFSD_Stats_Prune_Period,
	/*
	 * The below MUST come last.  Otherwise we leave a hole in
nfsd_files[]
	 * with !CONFIG_NFSD_V4 and simple_fill_super() goes oops
@@ -92,6 +94,8 @@ static ssize_t write_pool_threads(struct
static ssize_t write_versions(struct file *file, char *buf, size_t
size);
static ssize_t write_ports(struct file *file, char *buf, size_t
size);
static ssize_t write_maxblksize(struct file *file, char *buf,
size_t size);
+static ssize_t write_stats_enabled(struct file *file, char *buf,
size_t size);
+static ssize_t write_stats_prune_period(struct file *file, char
*buf, size_t size);
#ifdef CONFIG_NFSD_V4
static ssize_t write_leasetime(struct file *file, char *buf,
size_t size);
static ssize_t write_recoverydir(struct file *file, char *buf,
size_t size);
@@ -113,6 +117,8 @@ static ssize_t (*write_op[])(struct file
	[NFSD_Versions] = write_versions,
	[NFSD_Ports] = write_ports,
	[NFSD_MaxBlkSize] = write_maxblksize,
+	[NFSD_Stats_Enabled] = write_stats_enabled,
+	[NFSD_Stats_Prune_Period] = write_stats_prune_period,
#ifdef CONFIG_NFSD_V4
	[NFSD_Leasetime] = write_leasetime,
	[NFSD_RecoveryDir] = write_recoverydir,
@@ -1121,6 +1127,97 @@ static ssize_t write_maxblksize(struct f
	return sprintf(buf, "%d\n", nfsd_max_blksize);
}

+extern int nfsd_stats_enabled;
+
+/**
+ * write_stats_enabled - Set or report whether per-client/
+ *			 per-export stats are enabled.
+ *
+ * Input:
+ *			buf:		ignored
+ *			size:		zero
+ *
+ * OR
+ *
+ * Input:
+ * 			buf:		C string containing an unsigned
+ * 					integer value representing the new value
+ *			size:		non-zero length of C string in @buf
+ * Output:
+ *	On success:	passed-in buffer filled with '\n'-terminated C
string
+ *			containing numeric value of the current setting
+ *			return code is the size in bytes of the string
+ *	On error:	return code is zero or a negative errno value
+ */
+static ssize_t write_stats_enabled(struct file *file, char *buf,
size_t size)
+{
+	char *mesg = buf;
+	if (size > 0) {
+		int enabled;
+		int rv = get_int(&mesg, &enabled);
+		if (rv)
+			return rv;
+		/* check `enabled' against allowed range */
+		if (enabled < 0 || enabled > 1)
+			return -EINVAL;
+		/*
+		 * We can change the enabled flag at any time without
+		 * locking.  All it controls is whether stats are
+		 * gathered for new incoming NFS calls.  Old gathered
+		 * stats still sit around in the hash tables until
+		 * naturally pruned.
+		 */
+		nfsd_stats_enabled = enabled;
+	}
+	return sprintf(buf, "%d\n", nfsd_stats_enabled);
+}
+
+extern int nfsd_stats_prune_period;
+
+/**
+ * write_stats_prune_period - Set or report the period for pruning
+ *			      old per-client/per-export stats entries,
+ *			      in seconds.
+ *
+ * Input:
+ *			buf:		ignored
+ *			size:		zero
+ *
+ * OR
+ *
+ * Input:
+ * 			buf:		C string containing an unsigned
+ * 					integer value representing the new value
+ *			size:		non-zero length of C string in @buf
+ * Output:
+ *	On success:	passed-in buffer filled with '\n'-terminated C
string
+ *			containing numeric value of the current setting
+ *			return code is the size in bytes of the string
+ *	On error:	return code is zero or a negative errno value
+ */

Just an idle remark, don't worry about this for now, but: we might
want
to rein in this write_*() comment format a little some day. A lot of
the content seems duplicated.

I disagree.

How? The below seems to be an arguing against *removing* the comments, or removing information from them, neither of which I'd be in favor of.

Then I misunderstood what you meant by "rein in".

The apparent content duplication is because these functions are all slightly different. What we had before was a single description of the return values at the top of the files that more or less fit each proc file, but didn't precisely fit any but the oldest.

(Responding a bit to Greg) IMO highlighting the differences instead means a person trying to understand this interface has to read the whole damn nfsctl.c file instead of looking at the one piece s/he is interested in. This is documentation, not code, so I think a little text duplication is OK or even actually preferred.

--b.

The present nfsctl user space API is entirely ad hoc.

Although sometimes the behavior is the same, each function/file can
behave slightly differently than the others.  We have to be very
specific about this here because the comments serve as both "user"
documentation and as code/API specification.

Because this code wasn't adequately documented, features have been added over time without a close examination of the operation of other parts of
this code.

If you really want to simplify the comments, we should consider
simplifying the API first, imo.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com




--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux