All of lore.kernel.org
 help / color / mirror / Atom feed
* grace period setting
@ 2010-03-02 23:12 J. Bruce Fields
  2010-03-02 23:12 ` [PATCH 1/7] nfsd4: simplify references to nfsd4 lease time J. Bruce Fields
  0 siblings, 1 reply; 15+ messages in thread
From: J. Bruce Fields @ 2010-03-02 23:12 UTC (permalink / raw)
  To: linux-nfs

The NFSv4 lease-time configuration interface we currently have doesn't
actually allow setting the grace period directly.

Mostly this is noticed by developers trying to set a smaller grace
period to speed testing.

However, it also makes it difficult to make reboot-recovery behavior
correct, since there's no way for userland to force the kernel to
observe the correct grace period on reboot if the previous server was
using something other than the default lease time.

So, add a grace-period setting interface as well.

Note if you actually want a shorter grace period you'll also need to
adjust the nlm_grace_period sysctl (/proc/sys/fs/nfs/nlm_grace_period),
since the server doesn't end grace until both have expired.

--b.



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/7] nfsd4: simplify references to nfsd4 lease time
  2010-03-02 23:12 grace period setting J. Bruce Fields
@ 2010-03-02 23:12 ` J. Bruce Fields
  2010-03-02 23:12   ` [PATCH 2/7] nfsd4: edit comment for concision J. Bruce Fields
  0 siblings, 1 reply; 15+ messages in thread
From: J. Bruce Fields @ 2010-03-02 23:12 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

Instead of accessing the lease time directly, some users call
nfs4_lease_time(), and some a macro, NFSD_LEASE_TIME, defined as
nfs4_lease_time().  Neither layer of indirection serves any purpose.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfsd/nfs4callback.c |    2 +-
 fs/nfsd/nfs4state.c    |   22 ++++++++--------------
 fs/nfsd/nfs4xdr.c      |    2 +-
 fs/nfsd/nfsctl.c       |    5 +----
 fs/nfsd/nfsd.h         |    5 ++---
 5 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 8fa412c..cb6b011 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -455,7 +455,7 @@ static struct rpc_program cb_program = {
 
 static int max_cb_time(void)
 {
-	return max(NFSD_LEASE_TIME/10, (time_t)1) * HZ;
+	return max(nfsd4_lease/10, (time_t)1) * HZ;
 }
 
 /* Reference counting, callback cleanup, etc., all look racy as heck.
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 3a20c09..cc9164a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -44,7 +44,7 @@
 #define NFSDDBG_FACILITY                NFSDDBG_PROC
 
 /* Globals */
-static time_t lease_time = 90;     /* default lease time */
+time_t nfsd4_lease = 90;     /* default lease time */
 static time_t user_lease_time = 90;
 static time_t boot_time;
 static u32 current_ownerid = 1;
@@ -2560,9 +2560,9 @@ nfs4_laundromat(void)
 	struct nfs4_stateowner *sop;
 	struct nfs4_delegation *dp;
 	struct list_head *pos, *next, reaplist;
-	time_t cutoff = get_seconds() - NFSD_LEASE_TIME;
-	time_t t, clientid_val = NFSD_LEASE_TIME;
-	time_t u, test_val = NFSD_LEASE_TIME;
+	time_t cutoff = get_seconds() - nfsd4_lease;
+	time_t t, clientid_val = nfsd4_lease;
+	time_t u, test_val = nfsd4_lease;
 
 	nfs4_lock_state();
 
@@ -2602,7 +2602,7 @@ nfs4_laundromat(void)
 		list_del_init(&dp->dl_recall_lru);
 		unhash_delegation(dp);
 	}
-	test_val = NFSD_LEASE_TIME;
+	test_val = nfsd4_lease;
 	list_for_each_safe(pos, next, &close_lru) {
 		sop = list_entry(pos, struct nfs4_stateowner, so_close_lru);
 		if (time_after((unsigned long)sop->so_time, (unsigned long)cutoff)) {
@@ -2672,7 +2672,7 @@ EXPIRED_STATEID(stateid_t *stateid)
 {
 	if (time_before((unsigned long)boot_time,
 			((unsigned long)stateid->si_boot)) &&
-	    time_before((unsigned long)(stateid->si_boot + lease_time), get_seconds())) {
+	    time_before((unsigned long)(stateid->si_boot + nfsd4_lease), get_seconds())) {
 		dprintk("NFSD: expired stateid " STATEID_FMT "!\n",
 			STATEID_VAL(stateid));
 		return 1;
@@ -3976,7 +3976,7 @@ nfsd4_load_reboot_recovery_data(void)
 unsigned long
 get_nfs4_grace_period(void)
 {
-	return max(user_lease_time, lease_time) * HZ;
+	return max(user_lease_time, nfsd4_lease) * HZ;
 }
 
 /*
@@ -4009,7 +4009,7 @@ __nfs4_state_start(void)
 
 	boot_time = get_seconds();
 	grace_time = get_nfs4_grace_period();
-	lease_time = user_lease_time;
+	nfsd4_lease = user_lease_time;
 	locks_start_grace(&nfsd4_manager);
 	printk(KERN_INFO "NFSD: starting %ld-second grace period\n",
 	       grace_time/HZ);
@@ -4036,12 +4036,6 @@ nfs4_state_start(void)
 	return 0;
 }
 
-time_t
-nfs4_lease_time(void)
-{
-	return lease_time;
-}
-
 static void
 __nfs4_state_shutdown(void)
 {
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index c458fb1..f61bd73 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1899,7 +1899,7 @@ nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
 	if (bmval0 & FATTR4_WORD0_LEASE_TIME) {
 		if ((buflen -= 4) < 0)
 			goto out_resource;
-		WRITE32(NFSD_LEASE_TIME);
+		WRITE32(nfsd4_lease);
 	}
 	if (bmval0 & FATTR4_WORD0_RDATTR_ERROR) {
 		if ((buflen -= 4) < 0)
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 0f0e77f..8bff674 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1203,8 +1203,6 @@ static ssize_t write_maxblksize(struct file *file, char *buf, size_t size)
 }
 
 #ifdef CONFIG_NFSD_V4
-extern time_t nfs4_leasetime(void);
-
 static ssize_t __write_leasetime(struct file *file, char *buf, size_t size)
 {
 	/* if size > 10 seconds, call
@@ -1224,8 +1222,7 @@ static ssize_t __write_leasetime(struct file *file, char *buf, size_t size)
 		nfs4_reset_lease(lease);
 	}
 
-	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%ld\n",
-							nfs4_lease_time());
+	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%ld\n", nfsd4_lease);
 }
 
 /**
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index e942a1a..b463093 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -82,7 +82,6 @@ int nfs4_state_init(void);
 void nfsd4_free_slabs(void);
 int nfs4_state_start(void);
 void nfs4_state_shutdown(void);
-time_t nfs4_lease_time(void);
 void nfs4_reset_lease(time_t leasetime);
 int nfs4_reset_recoverydir(char *recdir);
 #else
@@ -90,7 +89,6 @@ static inline int nfs4_state_init(void) { return 0; }
 static inline void nfsd4_free_slabs(void) { }
 static inline int nfs4_state_start(void) { return 0; }
 static inline void nfs4_state_shutdown(void) { }
-static inline time_t nfs4_lease_time(void) { return 0; }
 static inline void nfs4_reset_lease(time_t leasetime) { }
 static inline int nfs4_reset_recoverydir(char *recdir) { return 0; }
 #endif
@@ -229,6 +227,8 @@ extern struct timeval	nfssvc_boot;
 
 #ifdef CONFIG_NFSD_V4
 
+extern time_t nfsd4_lease;
+
 /* before processing a COMPOUND operation, we have to check that there
  * is enough space in the buffer for XDR encode to succeed.  otherwise,
  * we might process an operation with side effects, and be unable to
@@ -247,7 +247,6 @@ extern struct timeval	nfssvc_boot;
 #define	COMPOUND_SLACK_SPACE		140    /* OP_GETFH */
 #define COMPOUND_ERR_SLACK_SPACE	12     /* OP_SETATTR */
 
-#define NFSD_LEASE_TIME                 (nfs4_lease_time())
 #define NFSD_LAUNDROMAT_MINTIMEOUT      10   /* seconds */
 
 /*
-- 
1.6.3.3


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/7] nfsd4: edit comment for concision
  2010-03-02 23:12 ` [PATCH 1/7] nfsd4: simplify references to nfsd4 lease time J. Bruce Fields
@ 2010-03-02 23:12   ` J. Bruce Fields
  2010-03-02 23:12     ` [PATCH 3/7] nfsd4: simplify lease/grace interaction J. Bruce Fields
  2010-03-03 18:06     ` [PATCH 2/7] nfsd4: edit comment for concision Chuck Lever
  0 siblings, 2 replies; 15+ messages in thread
From: J. Bruce Fields @ 2010-03-02 23:12 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

Also note units are seconds.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfsd/nfsctl.c |   21 ++++++---------------
 1 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 8bff674..f1ee549 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1228,23 +1228,14 @@ static ssize_t __write_leasetime(struct file *file, char *buf, size_t size)
 /**
  * write_leasetime - Set or report the current NFSv4 lease time
  *
- * Input:
- *			buf:		ignored
- *			size:		zero
+ * If given a nonzero size, sets the NFSv4 lease time to a number of
+ * seconds (interpreting buf as a C string containing an ascii
+ * representation of the number).
  *
- * OR
+ * Returns the resulting lease time (as an ascii representation in a
+ * '\n'-terminated C string) in buf.
  *
- * Input:
- *			buf:		C string containing an unsigned
- *					integer value representing the new
- *					NFSv4 lease expiry time
- *			size:		non-zero length of C string in @buf
- * Output:
- *	On success:	passed-in buffer filled with '\n'-terminated C
- *			string containing unsigned integer value of the
- *			current lease expiry time;
- *			return code is the size in bytes of the string
- *	On error:	return code is zero or a negative errno value
+ * Given a zero size, just returns the lease time in buf.
  */
 static ssize_t write_leasetime(struct file *file, char *buf, size_t size)
 {
-- 
1.6.3.3


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/7] nfsd4: simplify lease/grace interaction
  2010-03-02 23:12   ` [PATCH 2/7] nfsd4: edit comment for concision J. Bruce Fields
@ 2010-03-02 23:12     ` J. Bruce Fields
  2010-03-02 23:12       ` [PATCH 4/7] nfsd4: remove unnecessary lease-setting function J. Bruce Fields
  2010-03-03 18:06     ` [PATCH 2/7] nfsd4: edit comment for concision Chuck Lever
  1 sibling, 1 reply; 15+ messages in thread
From: J. Bruce Fields @ 2010-03-02 23:12 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

The original code here assumed we'd allow the user to change the lease
any time, but only allow the change to take effect on restart.  Since
then we modified the code to allow setting the lease on when the server
is down.  Update the rest of the code to reflect that fact, clarify
variable names, and add document.

Also, the code insisted that the grace period always be the longer of
the old and new lease periods, but that's overly conservative--as long
as it lasts at least the old lease period, old clients should still know
to recover in time.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfsd/nfs4state.c |   32 +++++++++++---------------------
 1 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index cc9164a..eb8d124 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -45,7 +45,7 @@
 
 /* Globals */
 time_t nfsd4_lease = 90;     /* default lease time */
-static time_t user_lease_time = 90;
+static time_t nfsd4_grace = 90;
 static time_t boot_time;
 static u32 current_ownerid = 1;
 static u32 current_fileid = 1;
@@ -2551,6 +2551,12 @@ nfsd4_end_grace(void)
 	dprintk("NFSD: end of grace period\n");
 	nfsd4_recdir_purge_old();
 	locks_end_grace(&nfsd4_manager);
+	/*
+	 * Now that every NFSv4 client has had the chance to recover and
+	 * to see the (possibly new, possibly shorter) lease time, we
+	 * can safely set the next grace time to the current lease time:
+	 */
+	nfsd4_grace = nfsd4_lease;
 }
 
 static time_t
@@ -3973,12 +3979,6 @@ nfsd4_load_reboot_recovery_data(void)
 		printk("NFSD: Failure reading reboot recovery data\n");
 }
 
-unsigned long
-get_nfs4_grace_period(void)
-{
-	return max(user_lease_time, nfsd4_lease) * HZ;
-}
-
 /*
  * Since the lifetime of a delegation isn't limited to that of an open, a
  * client may quite reasonably hang on to a delegation as long as it has
@@ -4005,18 +4005,14 @@ set_max_delegations(void)
 static int
 __nfs4_state_start(void)
 {
-	unsigned long grace_time;
-
 	boot_time = get_seconds();
-	grace_time = get_nfs4_grace_period();
-	nfsd4_lease = user_lease_time;
 	locks_start_grace(&nfsd4_manager);
 	printk(KERN_INFO "NFSD: starting %ld-second grace period\n",
-	       grace_time/HZ);
+	       nfsd4_grace);
 	laundry_wq = create_singlethread_workqueue("nfsd4");
 	if (laundry_wq == NULL)
 		return -ENOMEM;
-	queue_delayed_work(laundry_wq, &laundromat_work, grace_time);
+	queue_delayed_work(laundry_wq, &laundromat_work, nfsd4_grace * HZ);
 	set_max_delegations();
 	return set_callback_cred();
 }
@@ -4123,17 +4119,11 @@ nfs4_recoverydir(void)
 /*
  * Called when leasetime is changed.
  *
- * The only way the protocol gives us to handle on-the-fly lease changes is to
- * simulate a reboot.  Instead of doing that, we just wait till the next time
- * we start to register any changes in lease time.  If the administrator
- * really wants to change the lease time *now*, they can go ahead and bring
- * nfsd down and then back up again after changing the lease time.
- *
- * user_lease_time is protected by nfsd_mutex since it's only really accessed
+ * nfsd4_lease is protected by nfsd_mutex since it's only really accessed
  * when nfsd is starting
  */
 void
 nfs4_reset_lease(time_t leasetime)
 {
-	user_lease_time = leasetime;
+	nfsd4_lease = leasetime;
 }
-- 
1.6.3.3


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/7] nfsd4: remove unnecessary lease-setting function
  2010-03-02 23:12     ` [PATCH 3/7] nfsd4: simplify lease/grace interaction J. Bruce Fields
@ 2010-03-02 23:12       ` J. Bruce Fields
  2010-03-02 23:12         ` [PATCH 5/7] nfsd4: reshuffle lease-setting code to allow reuse J. Bruce Fields
  0 siblings, 1 reply; 15+ messages in thread
From: J. Bruce Fields @ 2010-03-02 23:12 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

This is another layer of indirection that doesn't really buy us
anything.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfsd/nfs4state.c |   12 ------------
 fs/nfsd/nfsctl.c    |    2 +-
 2 files changed, 1 insertions(+), 13 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index eb8d124..4471046 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4115,15 +4115,3 @@ nfs4_recoverydir(void)
 {
 	return user_recovery_dirname;
 }
-
-/*
- * Called when leasetime is changed.
- *
- * nfsd4_lease is protected by nfsd_mutex since it's only really accessed
- * when nfsd is starting
- */
-void
-nfs4_reset_lease(time_t leasetime)
-{
-	nfsd4_lease = leasetime;
-}
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index f1ee549..7f70704 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1219,7 +1219,7 @@ static ssize_t __write_leasetime(struct file *file, char *buf, size_t size)
 			return rv;
 		if (lease < 10 || lease > 3600)
 			return -EINVAL;
-		nfs4_reset_lease(lease);
+		nfsd4_lease = lease;
 	}
 
 	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%ld\n", nfsd4_lease);
-- 
1.6.3.3


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 5/7] nfsd4: reshuffle lease-setting code to allow reuse
  2010-03-02 23:12       ` [PATCH 4/7] nfsd4: remove unnecessary lease-setting function J. Bruce Fields
@ 2010-03-02 23:12         ` J. Bruce Fields
  2010-03-02 23:12           ` [PATCH 6/7] nfsd4: allow setting grace period time J. Bruce Fields
  2010-03-03 15:47           ` [PATCH 5/7] nfsd4: reshuffle lease-setting code to allow reuse Peter Staubach
  0 siblings, 2 replies; 15+ messages in thread
From: J. Bruce Fields @ 2010-03-02 23:12 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

We'll soon allow setting the grace period, so we'll want to share this
code.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfsd/nfsctl.c |   29 +++++++++++++++++------------
 1 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 7f70704..1db8010 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1203,26 +1203,36 @@ static ssize_t write_maxblksize(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_time(struct file *file, char *buf, size_t size, time_t *time)
 {
 	/* if size > 10 seconds, call
 	 * nfs4_reset_lease() then write out the new lease (seconds) as reply
 	 */
 	char *mesg = buf;
-	int rv, lease;
+	int rv, i;
 
 	if (size > 0) {
 		if (nfsd_serv)
 			return -EBUSY;
-		rv = get_int(&mesg, &lease);
+		rv = get_int(&mesg, &i);
 		if (rv)
 			return rv;
-		if (lease < 10 || lease > 3600)
+		if (i < 10 || i > 3600)
 			return -EINVAL;
-		nfsd4_lease = lease;
+		*time = i;
 	}
 
-	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%ld\n", nfsd4_lease);
+	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%ld\n", *time);
+}
+
+static ssize_t write_time(struct file *file, char *buf, size_t size, time_t *time)
+{
+	ssize_t rv;
+
+	mutex_lock(&nfsd_mutex);
+	rv = __write_time(file, buf, size, time);
+	mutex_unlock(&nfsd_mutex);
+	return rv;
 }
 
 /**
@@ -1239,12 +1249,7 @@ static ssize_t __write_leasetime(struct file *file, char *buf, size_t size)
  */
 static ssize_t write_leasetime(struct file *file, char *buf, size_t size)
 {
-	ssize_t rv;
-
-	mutex_lock(&nfsd_mutex);
-	rv = __write_leasetime(file, buf, size);
-	mutex_unlock(&nfsd_mutex);
-	return rv;
+	return write_time(file, buf, size, &nfsd4_lease);
 }
 
 extern char *nfs4_recoverydir(void);
-- 
1.6.3.3


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 6/7] nfsd4: allow setting grace period time
  2010-03-02 23:12         ` [PATCH 5/7] nfsd4: reshuffle lease-setting code to allow reuse J. Bruce Fields
@ 2010-03-02 23:12           ` J. Bruce Fields
  2010-03-02 23:12             ` [PATCH 7/7] nfsd4: document lease/grace-period limits J. Bruce Fields
  2010-03-03 15:47           ` [PATCH 5/7] nfsd4: reshuffle lease-setting code to allow reuse Peter Staubach
  1 sibling, 1 reply; 15+ messages in thread
From: J. Bruce Fields @ 2010-03-02 23:12 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

Allow explicit configuration of the grace period time as well as the
lease period time.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfsd/nfs4state.c |    2 +-
 fs/nfsd/nfsctl.c    |   19 +++++++++++++++++++
 fs/nfsd/nfsd.h      |    1 +
 3 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 4471046..6edfe23 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -45,7 +45,7 @@
 
 /* Globals */
 time_t nfsd4_lease = 90;     /* default lease time */
-static time_t nfsd4_grace = 90;
+time_t nfsd4_grace = 90;
 static time_t boot_time;
 static u32 current_ownerid = 1;
 static u32 current_fileid = 1;
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 1db8010..7bdaf31 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -45,6 +45,7 @@ enum {
 	 */
 #ifdef CONFIG_NFSD_V4
 	NFSD_Leasetime,
+	NFSD_Gracetime,
 	NFSD_RecoveryDir,
 #endif
 };
@@ -69,6 +70,7 @@ 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);
 #ifdef CONFIG_NFSD_V4
 static ssize_t write_leasetime(struct file *file, char *buf, size_t size);
+static ssize_t write_gracetime(struct file *file, char *buf, size_t size);
 static ssize_t write_recoverydir(struct file *file, char *buf, size_t size);
 #endif
 
@@ -90,6 +92,7 @@ static ssize_t (*write_op[])(struct file *, char *, size_t) = {
 	[NFSD_MaxBlkSize] = write_maxblksize,
 #ifdef CONFIG_NFSD_V4
 	[NFSD_Leasetime] = write_leasetime,
+	[NFSD_Gracetime] = write_gracetime,
 	[NFSD_RecoveryDir] = write_recoverydir,
 #endif
 };
@@ -1252,6 +1255,21 @@ static ssize_t write_leasetime(struct file *file, char *buf, size_t size)
 	return write_time(file, buf, size, &nfsd4_lease);
 }
 
+/**
+ * write_gracetime - Set or report current NFSv4 grace period time
+ *
+ * As above, but sets the time of the NFSv4 grace period.
+ *
+ * Note this should never be set to less than the *previous*
+ * lease-period time, but we don't try to enforce this.  (In the common
+ * case (a new boot), we don't know what the previous lease time was
+ * anyway.)
+ */
+static ssize_t write_gracetime(struct file *file, char *buf, size_t size)
+{
+	return write_time(file, buf, size, &nfsd4_grace);
+}
+
 extern char *nfs4_recoverydir(void);
 
 static ssize_t __write_recoverydir(struct file *file, char *buf, size_t size)
@@ -1343,6 +1361,7 @@ static int nfsd_fill_super(struct super_block * sb, void * data, int silent)
 		[NFSD_MaxBlkSize] = {"max_block_size", &transaction_ops, S_IWUSR|S_IRUGO},
 #ifdef CONFIG_NFSD_V4
 		[NFSD_Leasetime] = {"nfsv4leasetime", &transaction_ops, S_IWUSR|S_IRUSR},
+		[NFSD_Gracetime] = {"nfsv4gracetime", &transaction_ops, S_IWUSR|S_IRUSR},
 		[NFSD_RecoveryDir] = {"nfsv4recoverydir", &transaction_ops, S_IWUSR|S_IRUSR},
 #endif
 		/* last one */ {""}
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index b463093..7237776 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -228,6 +228,7 @@ extern struct timeval	nfssvc_boot;
 #ifdef CONFIG_NFSD_V4
 
 extern time_t nfsd4_lease;
+extern time_t nfsd4_grace;
 
 /* before processing a COMPOUND operation, we have to check that there
  * is enough space in the buffer for XDR encode to succeed.  otherwise,
-- 
1.6.3.3


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 7/7] nfsd4: document lease/grace-period limits
  2010-03-02 23:12           ` [PATCH 6/7] nfsd4: allow setting grace period time J. Bruce Fields
@ 2010-03-02 23:12             ` J. Bruce Fields
  0 siblings, 0 replies; 15+ messages in thread
From: J. Bruce Fields @ 2010-03-02 23:12 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

The current documentation here is out of date, and not quite right.

(Future work: some user documentation would be useful.)

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
 fs/nfsd/nfsctl.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 7bdaf31..8dd212a 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1208,9 +1208,6 @@ static ssize_t write_maxblksize(struct file *file, char *buf, size_t size)
 #ifdef CONFIG_NFSD_V4
 static ssize_t __write_time(struct file *file, char *buf, size_t size, time_t *time)
 {
-	/* if size > 10 seconds, call
-	 * nfs4_reset_lease() then write out the new lease (seconds) as reply
-	 */
 	char *mesg = buf;
 	int rv, i;
 
@@ -1220,6 +1217,18 @@ static ssize_t __write_time(struct file *file, char *buf, size_t size, time_t *t
 		rv = get_int(&mesg, &i);
 		if (rv)
 			return rv;
+		/*
+		 * Some sanity checking.  We don't have a reason for
+		 * these particular numbers, but problems with the
+		 * extremes are:
+		 *	- Too short: the briefest network outage may
+		 *	  cause clients to lose all their locks.  Also,
+		 *	  the frequent polling may be wasteful.
+		 *	- Too long: do you really want reboot recovery
+		 *	  to take more than an hour?  Or to make other
+		 *	  clients wait an hour before being able to
+		 *	  revoke a dead client's locks?
+		 */
 		if (i < 10 || i > 3600)
 			return -EINVAL;
 		*time = i;
-- 
1.6.3.3


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/7] nfsd4: reshuffle lease-setting code to allow reuse
  2010-03-02 23:12         ` [PATCH 5/7] nfsd4: reshuffle lease-setting code to allow reuse J. Bruce Fields
  2010-03-02 23:12           ` [PATCH 6/7] nfsd4: allow setting grace period time J. Bruce Fields
@ 2010-03-03 15:47           ` Peter Staubach
  2010-03-06 18:31             ` J. Bruce Fields
  1 sibling, 1 reply; 15+ messages in thread
From: Peter Staubach @ 2010-03-03 15:47 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: linux-nfs

J. Bruce Fields wrote:
> We'll soon allow setting the grace period, so we'll want to share this
> code.
> 
> Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> ---
>  fs/nfsd/nfsctl.c |   29 +++++++++++++++++------------
>  1 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 7f70704..1db8010 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1203,26 +1203,36 @@ static ssize_t write_maxblksize(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_time(struct file *file, char *buf, size_t size, time_t *time)
>  {
>  	/* if size > 10 seconds, call
>  	 * nfs4_reset_lease() then write out the new lease (seconds) as reply
>  	 */
>  	char *mesg = buf;
> -	int rv, lease;
> +	int rv, i;
>  
>  	if (size > 0) {
>  		if (nfsd_serv)
>  			return -EBUSY;
> -		rv = get_int(&mesg, &lease);
> +		rv = get_int(&mesg, &i);
>  		if (rv)
>  			return rv;
> -		if (lease < 10 || lease > 3600)
> +		if (i < 10 || i > 3600)
>  			return -EINVAL;
> -		nfsd4_lease = lease;
> +		*time = i;
>  	}
>  
> -	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%ld\n", nfsd4_lease);
> +	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%ld\n", *time);
> +}
> +
> +static ssize_t write_time(struct file *file, char *buf, size_t size, time_t *time)
> +{
> +	ssize_t rv;
> +
> +	mutex_lock(&nfsd_mutex);
> +	rv = __write_time(file, buf, size, time);
> +	mutex_unlock(&nfsd_mutex);
> +	return rv;
>  }
>  

The name, write_time, seems a little generic, doesn't it?  It
seems to me that it might be lead to easier maintenance to give
it an NFSv4 related name at least.

	Thanx...

		ps

>  /**
> @@ -1239,12 +1249,7 @@ static ssize_t __write_leasetime(struct file *file, char *buf, size_t size)
>   */
>  static ssize_t write_leasetime(struct file *file, char *buf, size_t size)
>  {
> -	ssize_t rv;
> -
> -	mutex_lock(&nfsd_mutex);
> -	rv = __write_leasetime(file, buf, size);
> -	mutex_unlock(&nfsd_mutex);
> -	return rv;
> +	return write_time(file, buf, size, &nfsd4_lease);
>  }
>  
>  extern char *nfs4_recoverydir(void);


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/7] nfsd4: edit comment for concision
  2010-03-02 23:12   ` [PATCH 2/7] nfsd4: edit comment for concision J. Bruce Fields
  2010-03-02 23:12     ` [PATCH 3/7] nfsd4: simplify lease/grace interaction J. Bruce Fields
@ 2010-03-03 18:06     ` Chuck Lever
  2010-03-06 18:50       ` J. Bruce Fields
  1 sibling, 1 reply; 15+ messages in thread
From: Chuck Lever @ 2010-03-03 18:06 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: linux-nfs

On 03/02/2010 06:12 PM, J. Bruce Fields wrote:
> Also note units are seconds.

I'd prefer the major edits in this patch be done at the same time you 
trim the other documenting comments for the /proc/fs/nfsd API.

> Signed-off-by: J. Bruce Fields<bfields@citi.umich.edu>
> ---
>   fs/nfsd/nfsctl.c |   21 ++++++---------------
>   1 files changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 8bff674..f1ee549 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1228,23 +1228,14 @@ static ssize_t __write_leasetime(struct file *file, char *buf, size_t size)
>   /**
>    * write_leasetime - Set or report the current NFSv4 lease time
>    *
> - * Input:
> - *			buf:		ignored
> - *			size:		zero
> + * If given a nonzero size, sets the NFSv4 lease time to a number of
> + * seconds (interpreting buf as a C string containing an ascii
> + * representation of the number).
>    *
> - * OR
> + * Returns the resulting lease time (as an ascii representation in a
> + * '\n'-terminated C string) in buf.
>    *
> - * Input:
> - *			buf:		C string containing an unsigned
> - *					integer value representing the new
> - *					NFSv4 lease expiry time
> - *			size:		non-zero length of C string in @buf
> - * Output:
> - *	On success:	passed-in buffer filled with '\n'-terminated C
> - *			string containing unsigned integer value of the
> - *			current lease expiry time;
> - *			return code is the size in bytes of the string
> - *	On error:	return code is zero or a negative errno value
> + * Given a zero size, just returns the lease time in buf.
>    */
>   static ssize_t write_leasetime(struct file *file, char *buf, size_t size)
>   {


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/7] nfsd4: reshuffle lease-setting code to allow reuse
  2010-03-03 15:47           ` [PATCH 5/7] nfsd4: reshuffle lease-setting code to allow reuse Peter Staubach
@ 2010-03-06 18:31             ` J. Bruce Fields
  0 siblings, 0 replies; 15+ messages in thread
From: J. Bruce Fields @ 2010-03-06 18:31 UTC (permalink / raw)
  To: Peter Staubach; +Cc: linux-nfs

On Wed, Mar 03, 2010 at 10:47:16AM -0500, Peter Staubach wrote:
> J. Bruce Fields wrote:
> > We'll soon allow setting the grace period, so we'll want to share this
> > code.
> > 
> > Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
> > ---
> >  fs/nfsd/nfsctl.c |   29 +++++++++++++++++------------
> >  1 files changed, 17 insertions(+), 12 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index 7f70704..1db8010 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -1203,26 +1203,36 @@ static ssize_t write_maxblksize(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_time(struct file *file, char *buf, size_t size, time_t *time)
> >  {
> >  	/* if size > 10 seconds, call
> >  	 * nfs4_reset_lease() then write out the new lease (seconds) as reply
> >  	 */
> >  	char *mesg = buf;
> > -	int rv, lease;
> > +	int rv, i;
> >  
> >  	if (size > 0) {
> >  		if (nfsd_serv)
> >  			return -EBUSY;
> > -		rv = get_int(&mesg, &lease);
> > +		rv = get_int(&mesg, &i);
> >  		if (rv)
> >  			return rv;
> > -		if (lease < 10 || lease > 3600)
> > +		if (i < 10 || i > 3600)
> >  			return -EINVAL;
> > -		nfsd4_lease = lease;
> > +		*time = i;
> >  	}
> >  
> > -	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%ld\n", nfsd4_lease);
> > +	return scnprintf(buf, SIMPLE_TRANSACTION_LIMIT, "%ld\n", *time);
> > +}
> > +
> > +static ssize_t write_time(struct file *file, char *buf, size_t size, time_t *time)
> > +{
> > +	ssize_t rv;
> > +
> > +	mutex_lock(&nfsd_mutex);
> > +	rv = __write_time(file, buf, size, time);
> > +	mutex_unlock(&nfsd_mutex);
> > +	return rv;
> >  }
> >  
> 
> The name, write_time, seems a little generic, doesn't it?  It
> seems to me that it might be lead to easier maintenance to give
> it an NFSv4 related name at least.

Well, since they're static, I thought I could get away with it, but,
fair enough--prepended a "nfsd4_".

--b.

> 
> 	Thanx...
> 
> 		ps
> 
> >  /**
> > @@ -1239,12 +1249,7 @@ static ssize_t __write_leasetime(struct file *file, char *buf, size_t size)
> >   */
> >  static ssize_t write_leasetime(struct file *file, char *buf, size_t size)
> >  {
> > -	ssize_t rv;
> > -
> > -	mutex_lock(&nfsd_mutex);
> > -	rv = __write_leasetime(file, buf, size);
> > -	mutex_unlock(&nfsd_mutex);
> > -	return rv;
> > +	return write_time(file, buf, size, &nfsd4_lease);
> >  }
> >  
> >  extern char *nfs4_recoverydir(void);
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/7] nfsd4: edit comment for concision
  2010-03-03 18:06     ` [PATCH 2/7] nfsd4: edit comment for concision Chuck Lever
@ 2010-03-06 18:50       ` J. Bruce Fields
  2010-03-08 15:31         ` Chuck Lever
  0 siblings, 1 reply; 15+ messages in thread
From: J. Bruce Fields @ 2010-03-06 18:50 UTC (permalink / raw)
  To: Chuck Lever; +Cc: linux-nfs

On Wed, Mar 03, 2010 at 01:06:31PM -0500, Chuck Lever wrote:
> On 03/02/2010 06:12 PM, J. Bruce Fields wrote:
>> Also note units are seconds.
>
> I'd prefer the major edits in this patch be done at the same time you  
> trim the other documenting comments for the /proc/fs/nfsd API.

Not sure when I'll have the time to do that, but, OK: dropped for now.

Do you think there's any loss of information of readability from the
shorter format?

--b.

>
>> Signed-off-by: J. Bruce Fields<bfields@citi.umich.edu>
>> ---
>>   fs/nfsd/nfsctl.c |   21 ++++++---------------
>>   1 files changed, 6 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>> index 8bff674..f1ee549 100644
>> --- a/fs/nfsd/nfsctl.c
>> +++ b/fs/nfsd/nfsctl.c
>> @@ -1228,23 +1228,14 @@ static ssize_t __write_leasetime(struct file *file, char *buf, size_t size)
>>   /**
>>    * write_leasetime - Set or report the current NFSv4 lease time
>>    *
>> - * Input:
>> - *			buf:		ignored
>> - *			size:		zero
>> + * If given a nonzero size, sets the NFSv4 lease time to a number of
>> + * seconds (interpreting buf as a C string containing an ascii
>> + * representation of the number).
>>    *
>> - * OR
>> + * Returns the resulting lease time (as an ascii representation in a
>> + * '\n'-terminated C string) in buf.
>>    *
>> - * Input:
>> - *			buf:		C string containing an unsigned
>> - *					integer value representing the new
>> - *					NFSv4 lease expiry time
>> - *			size:		non-zero length of C string in @buf
>> - * Output:
>> - *	On success:	passed-in buffer filled with '\n'-terminated C
>> - *			string containing unsigned integer value of the
>> - *			current lease expiry time;
>> - *			return code is the size in bytes of the string
>> - *	On error:	return code is zero or a negative errno value
>> + * Given a zero size, just returns the lease time in buf.
>>    */
>>   static ssize_t write_leasetime(struct file *file, char *buf, size_t size)
>>   {
>
>
> -- 
> chuck[dot]lever[at]oracle[dot]com
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/7] nfsd4: edit comment for concision
  2010-03-06 18:50       ` J. Bruce Fields
@ 2010-03-08 15:31         ` Chuck Lever
  2010-03-08 18:10           ` J. Bruce Fields
  0 siblings, 1 reply; 15+ messages in thread
From: Chuck Lever @ 2010-03-08 15:31 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: linux-nfs

On 03/06/2010 01:50 PM, J. Bruce Fields wrote:
> On Wed, Mar 03, 2010 at 01:06:31PM -0500, Chuck Lever wrote:
>> On 03/02/2010 06:12 PM, J. Bruce Fields wrote:
>>> Also note units are seconds.
>>
>> I'd prefer the major edits in this patch be done at the same time you
>> trim the other documenting comments for the /proc/fs/nfsd API.
>
> Not sure when I'll have the time to do that, but, OK: dropped for now.
>
> Do you think there's any loss of information or readability from the
> shorter format?

The reason I spelled it all out carefully is because this code can 
sometimes change in subtle ways, and the whole API implementation is 
spread out over several source files.  IMO, we need to have a reference 
specification that describes how this API _should_ work, despite what 
the code says.

(In other words, it's one of those cases where the code documents how 
the API _does_ work, not how it _should_ work, and the latter is pretty 
important.  So comments are rather necessary I think).

You could successfully put the block comments on a diet, but going too 
far would reduce the value of having the comments in the first place.

My main objection to this patch was that we need to have a more extended 
discussion of how to reduce the size of the comments for all the proc 
files implemented here.  That seemed to me was outside the scope of this 
patch set, but still worth doing at some point.

> --b.
>
>>
>>> Signed-off-by: J. Bruce Fields<bfields@citi.umich.edu>
>>> ---
>>>    fs/nfsd/nfsctl.c |   21 ++++++---------------
>>>    1 files changed, 6 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>>> index 8bff674..f1ee549 100644
>>> --- a/fs/nfsd/nfsctl.c
>>> +++ b/fs/nfsd/nfsctl.c
>>> @@ -1228,23 +1228,14 @@ static ssize_t __write_leasetime(struct file *file, char *buf, size_t size)
>>>    /**
>>>     * write_leasetime - Set or report the current NFSv4 lease time
>>>     *
>>> - * Input:
>>> - *			buf:		ignored
>>> - *			size:		zero
>>> + * If given a nonzero size, sets the NFSv4 lease time to a number of
>>> + * seconds (interpreting buf as a C string containing an ascii
>>> + * representation of the number).
>>>     *
>>> - * OR
>>> + * Returns the resulting lease time (as an ascii representation in a
>>> + * '\n'-terminated C string) in buf.
>>>     *
>>> - * Input:
>>> - *			buf:		C string containing an unsigned
>>> - *					integer value representing the new
>>> - *					NFSv4 lease expiry time
>>> - *			size:		non-zero length of C string in @buf
>>> - * Output:
>>> - *	On success:	passed-in buffer filled with '\n'-terminated C
>>> - *			string containing unsigned integer value of the
>>> - *			current lease expiry time;
>>> - *			return code is the size in bytes of the string
>>> - *	On error:	return code is zero or a negative errno value
>>> + * Given a zero size, just returns the lease time in buf.
>>>     */
>>>    static ssize_t write_leasetime(struct file *file, char *buf, size_t size)
>>>    {
>>
>>
>> --
>> chuck[dot]lever[at]oracle[dot]com
>>


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/7] nfsd4: edit comment for concision
  2010-03-08 15:31         ` Chuck Lever
@ 2010-03-08 18:10           ` J. Bruce Fields
  2010-03-08 18:16             ` Chuck Lever
  0 siblings, 1 reply; 15+ messages in thread
From: J. Bruce Fields @ 2010-03-08 18:10 UTC (permalink / raw)
  To: Chuck Lever; +Cc: linux-nfs

On Mon, Mar 08, 2010 at 10:31:48AM -0500, Chuck Lever wrote:
> On 03/06/2010 01:50 PM, J. Bruce Fields wrote:
>> On Wed, Mar 03, 2010 at 01:06:31PM -0500, Chuck Lever wrote:
>>> On 03/02/2010 06:12 PM, J. Bruce Fields wrote:
>>>> Also note units are seconds.
>>>
>>> I'd prefer the major edits in this patch be done at the same time you
>>> trim the other documenting comments for the /proc/fs/nfsd API.
>>
>> Not sure when I'll have the time to do that, but, OK: dropped for now.
>>
>> Do you think there's any loss of information or readability from the
>> shorter format?
>
> The reason I spelled it all out carefully is because this code can  
> sometimes change in subtle ways, and the whole API implementation is  
> spread out over several source files.

Understood.  I should have been more specific: in the particular example
below, do you think their was any loss?

--b.

> IMO, we need to have a reference  
> specification that describes how this API _should_ work, despite what  
> the code says.
>
> (In other words, it's one of those cases where the code documents how  
> the API _does_ work, not how it _should_ work, and the latter is pretty  
> important.  So comments are rather necessary I think).
>
> You could successfully put the block comments on a diet, but going too  
> far would reduce the value of having the comments in the first place.
>
> My main objection to this patch was that we need to have a more extended  
> discussion of how to reduce the size of the comments for all the proc  
> files implemented here.  That seemed to me was outside the scope of this  
> patch set, but still worth doing at some point.
>
>> --b.
>>
>>>
>>>> Signed-off-by: J. Bruce Fields<bfields@citi.umich.edu>
>>>> ---
>>>>    fs/nfsd/nfsctl.c |   21 ++++++---------------
>>>>    1 files changed, 6 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>>>> index 8bff674..f1ee549 100644
>>>> --- a/fs/nfsd/nfsctl.c
>>>> +++ b/fs/nfsd/nfsctl.c
>>>> @@ -1228,23 +1228,14 @@ static ssize_t __write_leasetime(struct file *file, char *buf, size_t size)
>>>>    /**
>>>>     * write_leasetime - Set or report the current NFSv4 lease time
>>>>     *
>>>> - * Input:
>>>> - *			buf:		ignored
>>>> - *			size:		zero
>>>> + * If given a nonzero size, sets the NFSv4 lease time to a number of
>>>> + * seconds (interpreting buf as a C string containing an ascii
>>>> + * representation of the number).
>>>>     *
>>>> - * OR
>>>> + * Returns the resulting lease time (as an ascii representation in a
>>>> + * '\n'-terminated C string) in buf.
>>>>     *
>>>> - * Input:
>>>> - *			buf:		C string containing an unsigned
>>>> - *					integer value representing the new
>>>> - *					NFSv4 lease expiry time
>>>> - *			size:		non-zero length of C string in @buf
>>>> - * Output:
>>>> - *	On success:	passed-in buffer filled with '\n'-terminated C
>>>> - *			string containing unsigned integer value of the
>>>> - *			current lease expiry time;
>>>> - *			return code is the size in bytes of the string
>>>> - *	On error:	return code is zero or a negative errno value
>>>> + * Given a zero size, just returns the lease time in buf.
>>>>     */
>>>>    static ssize_t write_leasetime(struct file *file, char *buf, size_t size)
>>>>    {
>>>
>>>
>>> --
>>> chuck[dot]lever[at]oracle[dot]com
>>>
>
>
> -- 
> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/7] nfsd4: edit comment for concision
  2010-03-08 18:10           ` J. Bruce Fields
@ 2010-03-08 18:16             ` Chuck Lever
  0 siblings, 0 replies; 15+ messages in thread
From: Chuck Lever @ 2010-03-08 18:16 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: linux-nfs

On 03/08/2010 01:10 PM, J. Bruce Fields wrote:
> On Mon, Mar 08, 2010 at 10:31:48AM -0500, Chuck Lever wrote:
>> On 03/06/2010 01:50 PM, J. Bruce Fields wrote:
>>> On Wed, Mar 03, 2010 at 01:06:31PM -0500, Chuck Lever wrote:
>>>> On 03/02/2010 06:12 PM, J. Bruce Fields wrote:
>>>>> Also note units are seconds.
>>>>
>>>> I'd prefer the major edits in this patch be done at the same time you
>>>> trim the other documenting comments for the /proc/fs/nfsd API.
>>>
>>> Not sure when I'll have the time to do that, but, OK: dropped for now.
>>>
>>> Do you think there's any loss of information or readability from the
>>> shorter format?
>>
>> The reason I spelled it all out carefully is because this code can
>> sometimes change in subtle ways, and the whole API implementation is
>> spread out over several source files.
>
> Understood.  I should have been more specific: in the particular example
> below, do you think there was any loss?

Yes.

Your version of the specification allows negative timeouts to be passed 
in, and it doesn't describe the return value of the write(2) call.

Mine, on the other hand, neglects to state that the passed time-out 
values are in units of seconds.

>> IMO, we need to have a reference
>> specification that describes how this API _should_ work, despite what
>> the code says.
>>
>> (In other words, it's one of those cases where the code documents how
>> the API _does_ work, not how it _should_ work, and the latter is pretty
>> important.  So comments are rather necessary I think).
>>
>> You could successfully put the block comments on a diet, but going too
>> far would reduce the value of having the comments in the first place.
>>
>> My main objection to this patch was that we need to have a more extended
>> discussion of how to reduce the size of the comments for all the proc
>> files implemented here.  That seemed to me was outside the scope of this
>> patch set, but still worth doing at some point.
>>
>>> --b.
>>>
>>>>
>>>>> Signed-off-by: J. Bruce Fields<bfields@citi.umich.edu>
>>>>> ---
>>>>>     fs/nfsd/nfsctl.c |   21 ++++++---------------
>>>>>     1 files changed, 6 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>>>>> index 8bff674..f1ee549 100644
>>>>> --- a/fs/nfsd/nfsctl.c
>>>>> +++ b/fs/nfsd/nfsctl.c
>>>>> @@ -1228,23 +1228,14 @@ static ssize_t __write_leasetime(struct file *file, char *buf, size_t size)
>>>>>     /**
>>>>>      * write_leasetime - Set or report the current NFSv4 lease time
>>>>>      *
>>>>> - * Input:
>>>>> - *			buf:		ignored
>>>>> - *			size:		zero
>>>>> + * If given a nonzero size, sets the NFSv4 lease time to a number of
>>>>> + * seconds (interpreting buf as a C string containing an ascii
>>>>> + * representation of the number).
>>>>>      *
>>>>> - * OR
>>>>> + * Returns the resulting lease time (as an ascii representation in a
>>>>> + * '\n'-terminated C string) in buf.
>>>>>      *
>>>>> - * Input:
>>>>> - *			buf:		C string containing an unsigned
>>>>> - *					integer value representing the new
>>>>> - *					NFSv4 lease expiry time
>>>>> - *			size:		non-zero length of C string in @buf
>>>>> - * Output:
>>>>> - *	On success:	passed-in buffer filled with '\n'-terminated C
>>>>> - *			string containing unsigned integer value of the
>>>>> - *			current lease expiry time;
>>>>> - *			return code is the size in bytes of the string
>>>>> - *	On error:	return code is zero or a negative errno value
>>>>> + * Given a zero size, just returns the lease time in buf.
>>>>>      */
>>>>>     static ssize_t write_leasetime(struct file *file, char *buf, size_t size)
>>>>>     {
>>>>
>>>>
>>>> --
>>>> chuck[dot]lever[at]oracle[dot]com
>>>>
>>
>>
>> --
>> 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@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2010-03-08 18:17 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-03-02 23:12 grace period setting J. Bruce Fields
2010-03-02 23:12 ` [PATCH 1/7] nfsd4: simplify references to nfsd4 lease time J. Bruce Fields
2010-03-02 23:12   ` [PATCH 2/7] nfsd4: edit comment for concision J. Bruce Fields
2010-03-02 23:12     ` [PATCH 3/7] nfsd4: simplify lease/grace interaction J. Bruce Fields
2010-03-02 23:12       ` [PATCH 4/7] nfsd4: remove unnecessary lease-setting function J. Bruce Fields
2010-03-02 23:12         ` [PATCH 5/7] nfsd4: reshuffle lease-setting code to allow reuse J. Bruce Fields
2010-03-02 23:12           ` [PATCH 6/7] nfsd4: allow setting grace period time J. Bruce Fields
2010-03-02 23:12             ` [PATCH 7/7] nfsd4: document lease/grace-period limits J. Bruce Fields
2010-03-03 15:47           ` [PATCH 5/7] nfsd4: reshuffle lease-setting code to allow reuse Peter Staubach
2010-03-06 18:31             ` J. Bruce Fields
2010-03-03 18:06     ` [PATCH 2/7] nfsd4: edit comment for concision Chuck Lever
2010-03-06 18:50       ` J. Bruce Fields
2010-03-08 15:31         ` Chuck Lever
2010-03-08 18:10           ` J. Bruce Fields
2010-03-08 18:16             ` Chuck Lever

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.