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 <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.
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.
--
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
