From: "J. Bruce Fields" Subject: Re: [patch 03/29] knfsd: add userspace controls for stats tables Date: Mon, 27 Apr 2009 19:22:25 -0400 Message-ID: <20090427232225.GB5108@fieldses.org> References: <20090331202800.739621000@sgi.com> <20090331202938.939647000@sgi.com> <20090425215745.GA5088@fieldses.org> <20090425220329.GB5088@fieldses.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Greg Banks , Linux NFS ML To: Chuck Lever Return-path: Received: from mail.fieldses.org ([141.211.133.115]:50028 "EHLO pickle.fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751389AbZD0XW3 (ORCPT ); Mon, 27 Apr 2009 19:22:29 -0400 In-Reply-To: Sender: linux-nfs-owner@vger.kernel.org List-ID: 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 >>>> --- >>>> >>>> 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.