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 <[email protected]>
---
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
+ */
+static ssize_t write_stats_prune_period(struct file *file, char *buf, size_t size)
+{
+ char *mesg = buf;
+ if (size > 0) {
+ int period;
+ int rv = get_int(&mesg, &period);
+ if (rv)
+ return rv;
+ /* check `period' against allowed range */
+ if (period < 10 || period > 14*86400)
+ return -EINVAL;
+ /*
+ * We can change the period at any time without
+ * locking. All it controls is the timeout on the
+ * next run of the prune timer. This might cause
+ * some unexpected behaviour if the period is
+ * changed from really high to really low.
+ */
+ nfsd_stats_prune_period = period;
+ }
+ return sprintf(buf, "%d\n", nfsd_stats_prune_period);
+}
+
#ifdef CONFIG_NFSD_V4
extern time_t nfs4_leasetime(void);
@@ -1263,6 +1360,8 @@ static int nfsd_fill_super(struct super_
[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},
+ [NFSD_Stats_Enabled] = {"stats_enabled", &transaction_ops, S_IWUSR|S_IRUGO},
+ [NFSD_Stats_Prune_Period] = {"stats_prune_period", &transaction_ops, S_IWUSR|S_IRUGO},
#ifdef CONFIG_NFSD_V4
[NFSD_Leasetime] = {"nfsv4leasetime", &transaction_ops, S_IWUSR|S_IRUSR},
[NFSD_RecoveryDir] = {"nfsv4recoverydir", &transaction_ops, S_IWUSR|S_IRUSR},
--
Greg
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 <[email protected]>
>>>> ---
>>>>
>>>> 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.
--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.
G'day,
Whoops, this bounced from the list because I accidentally had HTML
formatting on. Resending.
On Tue, Apr 28, 2009 at 11:27 AM, Greg Banks <[email protected]> wrote:
>
>
> On Tue, Apr 28, 2009 at 2:06 AM, Chuck Lever <[email protected]> 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:
>>>>>
>>>>> +
>>>>> +/**
>>>>> + * 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.
>>
>> The present nfsctl user space API is entirely ad hoc.
>
> Agreed.
>
>>
>>
>> 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.
>
> Sure, but the comments aren't organised in such a way that the subtle
> differences between each pseudofile are obvious. Instead there are large
> swathes of text above each write_*() function which describe behaviours
> common to all the write_*() functions, and you have to hunt carefully for
> the differences.
>
> For example, we don't need 10 copies of this text:
>
> * 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
>
> Bruce rightly points out that my patch was continuing this poor trend.
>
>>
>> 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.
>
> Meh. Most of the stuff in the nfsd filesystem could be *simplified* by
> using the actual /proc filesystem mechanisms instead.
>
>
>
> --
> Greg.
>
--
Greg.
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 <[email protected]>
>>>>> ---
>>>>>
>>>>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Tue, Apr 28, 2009 at 11:37:09AM -0400, Chuck Lever wrote:
>
> 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 <[email protected]>
>>>>>> ---
>>>>>>
>>>>>> 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.
Agreed, and I agree that nobody should have to read the whole file. But
appropriate cross-references ("foo() behaves like bar() except...")
could prevent that. As long as we don't require following too many such
references, I think the burden of tracking following references would be
outweighed by the benefits of more concise descriptions.
--b.
On Apr 28, 2009, at 11:57 AM, J. Bruce Fields wrote:
> On Tue, Apr 28, 2009 at 11:37:09AM -0400, Chuck Lever wrote:
>>
>> 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 <[email protected]>
>>>>>>> ---
>>>>>>>
>>>>>>> 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.
>
> Agreed, and I agree that nobody should have to read the whole file.
> But
> appropriate cross-references ("foo() behaves like bar() except...")
> could prevent that. As long as we don't require following too many
> such
> references, I think the burden of tracking following references
> would be
> outweighed by the benefits of more concise descriptions.
I'm OK with more brevity in these comments as long as we recognize
that the devil is in the details. We should preserve the specificity
of the API descriptions.
I think it was you who was complaining recently about knowing whether
a \n or a \0 is expected at the end of some of these strings. ;-)
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Tue, Apr 28, 2009 at 12:03:49PM -0400, Chuck Lever wrote:
> I'm OK with more brevity in these comments as long as we recognize that
> the devil is in the details. We should preserve the specificity of the
> API descriptions.
>
> I think it was you who was complaining recently about knowing whether a
> \n or a \0 is expected at the end of some of these strings. ;-)
Oh, absolutely, I do appreciate the fact that we've got some
documentation there.
--b.
On Wed, Apr 29, 2009 at 1:57 AM, J. Bruce Fields <[email protected]> wrote:
> On Tue, Apr 28, 2009 at 11:37:09AM -0400, Chuck Lever wrote:
>>
>> (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.
>
> Agreed, and I agree that nobody should have to read the whole file. But
> appropriate cross-references ("foo() behaves like bar() except...")
> could prevent that.
I think that would be a definite improvement. I'll see what I can do.
--
Greg.
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 <[email protected]>
> ---
>
> 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.
--b.
> +static ssize_t write_stats_prune_period(struct file *file, char *buf, size_t size)
> +{
> + char *mesg = buf;
> + if (size > 0) {
> + int period;
> + int rv = get_int(&mesg, &period);
> + if (rv)
> + return rv;
> + /* check `period' against allowed range */
> + if (period < 10 || period > 14*86400)
> + return -EINVAL;
> + /*
> + * We can change the period at any time without
> + * locking. All it controls is the timeout on the
> + * next run of the prune timer. This might cause
> + * some unexpected behaviour if the period is
> + * changed from really high to really low.
> + */
> + nfsd_stats_prune_period = period;
> + }
> + return sprintf(buf, "%d\n", nfsd_stats_prune_period);
> +}
> +
> #ifdef CONFIG_NFSD_V4
> extern time_t nfs4_leasetime(void);
>
> @@ -1263,6 +1360,8 @@ static int nfsd_fill_super(struct super_
> [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},
> + [NFSD_Stats_Enabled] = {"stats_enabled", &transaction_ops, S_IWUSR|S_IRUGO},
> + [NFSD_Stats_Prune_Period] = {"stats_prune_period", &transaction_ops, S_IWUSR|S_IRUGO},
> #ifdef CONFIG_NFSD_V4
> [NFSD_Leasetime] = {"nfsv4leasetime", &transaction_ops, S_IWUSR|S_IRUSR},
> [NFSD_RecoveryDir] = {"nfsv4recoverydir", &transaction_ops, S_IWUSR|S_IRUSR},
>
> --
> Greg
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 <[email protected]>
> > ---
> >
> > 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.
>
> --b.
>
> > +static ssize_t write_stats_prune_period(struct file *file, char *buf, size_t size)
> > +{
> > + char *mesg = buf;
> > + if (size > 0) {
> > + int period;
> > + int rv = get_int(&mesg, &period);
> > + if (rv)
> > + return rv;
> > + /* check `period' against allowed range */
> > + if (period < 10 || period > 14*86400)
> > + return -EINVAL;
> > + /*
> > + * We can change the period at any time without
> > + * locking. All it controls is the timeout on the
> > + * next run of the prune timer. This might cause
> > + * some unexpected behaviour if the period is
> > + * changed from really high to really low.
> > + */
> > + nfsd_stats_prune_period = period;
> > + }
> > + return sprintf(buf, "%d\n", nfsd_stats_prune_period);
> > +}
> > +
> > #ifdef CONFIG_NFSD_V4
> > extern time_t nfs4_leasetime(void);
> >
> > @@ -1263,6 +1360,8 @@ static int nfsd_fill_super(struct super_
> > [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},
> > + [NFSD_Stats_Enabled] = {"stats_enabled", &transaction_ops, S_IWUSR|S_IRUGO},
> > + [NFSD_Stats_Prune_Period] = {"stats_prune_period", &transaction_ops, S_IWUSR|S_IRUGO},
> > #ifdef CONFIG_NFSD_V4
> > [NFSD_Leasetime] = {"nfsv4leasetime", &transaction_ops, S_IWUSR|S_IRUSR},
> > [NFSD_RecoveryDir] = {"nfsv4recoverydir", &transaction_ops, S_IWUSR|S_IRUSR},
> >
> > --
> > Greg
On Sun, Apr 26, 2009 at 7:57 AM, J. Bruce Fields <[email protected]> wrote:
> On Wed, Apr 01, 2009 at 07:28:03AM +1100, Greg Banks wrote:
>> +
>> +/**
>> + * 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.
>
Fair comment. Noted for later.
--
Greg.
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 <[email protected]>
>>> ---
>>>
>>> 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.
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.
>> --b.
>>
>>> +static ssize_t write_stats_prune_period(struct file *file, char
>>> *buf, size_t size)
>>> +{
>>> + char *mesg = buf;
>>> + if (size > 0) {
>>> + int period;
>>> + int rv = get_int(&mesg, &period);
>>> + if (rv)
>>> + return rv;
>>> + /* check `period' against allowed range */
>>> + if (period < 10 || period > 14*86400)
>>> + return -EINVAL;
>>> + /*
>>> + * We can change the period at any time without
>>> + * locking. All it controls is the timeout on the
>>> + * next run of the prune timer. This might cause
>>> + * some unexpected behaviour if the period is
>>> + * changed from really high to really low.
>>> + */
>>> + nfsd_stats_prune_period = period;
>>> + }
>>> + return sprintf(buf, "%d\n", nfsd_stats_prune_period);
>>> +}
>>> +
>>> #ifdef CONFIG_NFSD_V4
>>> extern time_t nfs4_leasetime(void);
>>>
>>> @@ -1263,6 +1360,8 @@ static int nfsd_fill_super(struct super_
>>> [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},
>>> + [NFSD_Stats_Enabled] = {"stats_enabled", &transaction_ops,
>>> S_IWUSR|S_IRUGO},
>>> + [NFSD_Stats_Prune_Period] = {"stats_prune_period",
>>> &transaction_ops, S_IWUSR|S_IRUGO},
>>> #ifdef CONFIG_NFSD_V4
>>> [NFSD_Leasetime] = {"nfsv4leasetime", &transaction_ops, S_IWUSR|
>>> S_IRUSR},
>>> [NFSD_RecoveryDir] = {"nfsv4recoverydir", &transaction_ops,
>>> S_IWUSR|S_IRUSR},
>>>
>>> --
>>> Greg
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs"
> in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com