[PATCH 000 of 8] knfsd: Assorted nfsv4 server patches.

[PATCH 000 of 8] knfsd: Assorted nfsv4 server patches.

[NeilBrown]

Following 8 patches fix minor bug and provide minor enhancements for
nfsv4 service.  They all have "no obvious style problems" and are
"ready for submission".

They are appropriate for inclusion in 2.6.23-rc1.
They are against 2.6.22-rc4-mm2.

Thanks,
NeilBrown


 [PATCH 001 of 8] knfsd: lockd: nfsd4: use same grace period for lockd and nfsd4
 [PATCH 002 of 8] knfsd: nfsd4: fix NFSv4 filehandle size units confusion
 [PATCH 003 of 8] knfsd: nfsd4: silence a compiler warning in ACL code
 [PATCH 004 of 8] knfsd: nfsd4: fix enc_stateid_sz for nfsd callbacks
 [PATCH 005 of 8] knfsd: nfsd4: fix handling of acl errrors
 [PATCH 006 of 8] knfsd: nfsd: remove unused header interface.h
 [PATCH 007 of 8] knfsd: nfsd4: vary maximum delegation limit based on RAM size
 [PATCH 008 of 8] knfsd: nfsd4: don't delegate files that have had conflicts

[PATCH 001 of 8] knfsd: lockd: nfsd4: use same grace period for lockd and nfsd4

[NeilBrown]

From: "J. Bruce Fields" <bfields@fieldses.org>

Both lockd and (in the nfsv4 case) nfsd enforce a "grace period" after
reboot, during which clients may reclaim locks from the previous server
instance, but may not acquire new locks.

Currently the lockd and nfsd enforce grace periods of different lengths.
This may cause problems when we reboot a server with both v2/v3 and v4
clients.  For example, if the lockd grace period is shorter (as is likely
the case), then a v3 client might acquire a new lock that conflicts with a
lock already held (but not yet reclaimed) by a v4 client.

This patch calculates a lease time that lockd and nfsd can both use.

Signed-off-by: Marc Eshel <eshel@almaden.ibm.com>
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/lockd/svc.c             |   27 ++++++++++++++++++++-------
 ./fs/nfsd/lockd.c            |    1 +
 ./fs/nfsd/nfs4state.c        |   16 ++++++++++++----
 ./include/linux/lockd/bind.h |    9 +++++++++
 4 files changed, 42 insertions(+), 11 deletions(-)

diff .prev/fs/lockd/svc.c ./fs/lockd/svc.c
--- .prev/fs/lockd/svc.c	2007-06-21 14:08:51.000000000 +1000
+++ ./fs/lockd/svc.c	2007-06-21 13:52:05.000000000 +1000
@@ -76,18 +76,31 @@ static const int		nlm_port_min = 0, nlm_
 
 static struct ctl_table_header * nlm_sysctl_table;
 
-static unsigned long set_grace_period(void)
+static unsigned long get_lockd_grace_period(void)
 {
-	unsigned long grace_period;
-
 	/* Note: nlm_timeout should always be nonzero */
 	if (nlm_grace_period)
-		grace_period = ((nlm_grace_period + nlm_timeout - 1)
-				/ nlm_timeout) * nlm_timeout * HZ;
+		return roundup(nlm_grace_period, nlm_timeout) * HZ;
 	else
-		grace_period = nlm_timeout * 5 * HZ;
+		return nlm_timeout * 5 * HZ;
+}
+
+unsigned long get_nfs_grace_period(void)
+{
+	unsigned long lockdgrace = get_lockd_grace_period();
+	unsigned long nfsdgrace = 0;
+
+	if (nlmsvc_ops)
+		nfsdgrace = nlmsvc_ops->get_grace_period();
+
+	return max(lockdgrace, nfsdgrace);
+}
+EXPORT_SYMBOL(get_nfs_grace_period);
+
+static unsigned long set_grace_period(void)
+{
 	nlmsvc_grace_period = 1;
-	return grace_period + jiffies;
+	return get_nfs_grace_period() + jiffies;
 }
 
 static inline void clear_grace_period(void)

diff .prev/fs/nfsd/lockd.c ./fs/nfsd/lockd.c
--- .prev/fs/nfsd/lockd.c	2007-06-21 14:08:51.000000000 +1000
+++ ./fs/nfsd/lockd.c	2007-06-21 13:46:56.000000000 +1000
@@ -65,6 +65,7 @@ nlm_fclose(struct file *filp)
 static struct nlmsvc_binding	nfsd_nlm_ops = {
 	.fopen		= nlm_fopen,		/* open file for locking */
 	.fclose		= nlm_fclose,		/* close file */
+	.get_grace_period = get_nfs4_grace_period,
 };
 
 void

diff .prev/fs/nfsd/nfs4state.c ./fs/nfsd/nfs4state.c
--- .prev/fs/nfsd/nfs4state.c	2007-06-21 14:08:51.000000000 +1000
+++ ./fs/nfsd/nfs4state.c	2007-06-21 14:09:23.000000000 +1000
@@ -51,6 +51,7 @@
 #include <linux/namei.h>
 #include <linux/mutex.h>
 #include <linux/lockd/bind.h>
+#include <linux/module.h>
 
 #define NFSDDBG_FACILITY                NFSDDBG_PROC
 
@@ -3191,20 +3192,27 @@ 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, lease_time) * HZ;
+}
+
 /* initialization to perform when the nfsd service is started: */
 
 static void
 __nfs4_state_start(void)
 {
-	time_t grace_time;
+	unsigned long grace_time;
 
 	boot_time = get_seconds();
-	grace_time = max(user_lease_time, lease_time);
+	grace_time = get_nfs_grace_period();
 	lease_time = user_lease_time;
 	in_grace = 1;
-	printk("NFSD: starting %ld-second grace period\n", grace_time);
+	printk(KERN_INFO "NFSD: starting %ld-second grace period\n",
+	       grace_time/HZ);
 	laundry_wq = create_singlethread_workqueue("nfsd4");
-	queue_delayed_work(laundry_wq, &laundromat_work, grace_time*HZ);
+	queue_delayed_work(laundry_wq, &laundromat_work, grace_time);
 }
 
 int

diff .prev/include/linux/lockd/bind.h ./include/linux/lockd/bind.h
--- .prev/include/linux/lockd/bind.h	2007-06-21 14:08:51.000000000 +1000
+++ ./include/linux/lockd/bind.h	2007-06-21 13:46:56.000000000 +1000
@@ -27,6 +27,7 @@ struct nlmsvc_binding {
 						struct nfs_fh *,
 						struct file **);
 	void			(*fclose)(struct file *);
+	unsigned long		(*get_grace_period)(void);
 };
 
 extern struct nlmsvc_binding *	nlmsvc_ops;
@@ -38,4 +39,12 @@ extern int	nlmclnt_proc(struct inode *, 
 extern int	lockd_up(int proto);
 extern void	lockd_down(void);
 
+unsigned long get_nfs_grace_period(void);
+
+#ifdef CONFIG_NFSD_V4
+unsigned long get_nfs4_grace_period(void);
+#else
+static inline unsigned long get_nfs4_grace_period(void) {return 0;}
+#endif
+
 #endif /* LINUX_LOCKD_BIND_H */

[PATCH 002 of 8] knfsd: nfsd4: fix NFSv4 filehandle size units confusion

[NeilBrown]

From: "J. Bruce Fields" <bfields@fieldses.org>


NFS4_FHSIZE is measured in bytes, not 4-byte words, so much more space
than necessary is being allocated for struct nfs4_cb_recall.

I should have wondered why this structure was so much larger than it
needed to be!

Signed-off-by: "J. Bruce Fields" <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./include/linux/nfsd/state.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff .prev/include/linux/nfsd/state.h ./include/linux/nfsd/state.h
--- .prev/include/linux/nfsd/state.h	2007-06-21 13:46:39.000000000 +1000
+++ ./include/linux/nfsd/state.h	2007-06-21 14:09:31.000000000 +1000
@@ -67,7 +67,7 @@ struct nfs4_cb_recall {
 	int			cbr_trunc;
 	stateid_t		cbr_stateid;
 	u32			cbr_fhlen;
-	u32			cbr_fhval[NFS4_FHSIZE];
+	char			cbr_fhval[NFS4_FHSIZE];
 	struct nfs4_delegation	*cbr_dp;
 };
 

[PATCH 003 of 8] knfsd: nfsd4: silence a compiler warning in ACL code

[NeilBrown]

From: "J. Bruce Fields" <bfields@fieldses.org>


Silence a compiler warning in the ACL code, and add a comment making
clear the initialization serves no other purpose.

Signed-off-by: "J. Bruce Fields" <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfs4acl.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff .prev/fs/nfsd/nfs4acl.c ./fs/nfsd/nfs4acl.c
--- .prev/fs/nfsd/nfs4acl.c	2007-06-21 13:46:39.000000000 +1000
+++ ./fs/nfsd/nfs4acl.c	2007-06-21 14:10:06.000000000 +1000
@@ -183,8 +183,13 @@ static void
 summarize_posix_acl(struct posix_acl *acl, struct posix_acl_summary *pas)
 {
 	struct posix_acl_entry *pa, *pe;
-	pas->users = 0;
-	pas->groups = 0;
+
+	/*
+	 * Only pas.users and pas.groups need initialization; previous
+	 * posix_acl_valid() calls ensure that the other fields will be
+	 * initialized in the following loop.  But, just to placate gcc:
+	 */
+	memset(pas, 0, sizeof(*pas));
 	pas->mask = 07;
 
 	pe = acl->a_entries + acl->a_count;

[PATCH 004 of 8] knfsd: nfsd4: fix enc_stateid_sz for nfsd callbacks

[NeilBrown]

From: "J. Bruce Fields" <bfields@fieldses.org>


enc_stateid_sz should be given in u32 words units, not bytes, so we were
overestimating the buffer space needed here.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
Signed-off-by: "J. Bruce Fields" <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfs4callback.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff .prev/fs/nfsd/nfs4callback.c ./fs/nfsd/nfs4callback.c
--- .prev/fs/nfsd/nfs4callback.c	2007-06-21 13:46:38.000000000 +1000
+++ ./fs/nfsd/nfs4callback.c	2007-06-21 14:10:31.000000000 +1000
@@ -75,7 +75,7 @@ enum nfs_cb_opnum4 {
 #define op_enc_sz			1
 #define op_dec_sz			2
 #define enc_nfs4_fh_sz			(1 + (NFS4_FHSIZE >> 2))
-#define enc_stateid_sz			16
+#define enc_stateid_sz			(NFS4_STATEID_SIZE >> 2)
 #define NFS4_enc_cb_recall_sz		(cb_compound_enc_hdr_sz +       \
 					1 + enc_stateid_sz +            \
 					enc_nfs4_fh_sz)

[PATCH 005 of 8] knfsd: nfsd4: fix handling of acl errrors

[NeilBrown]

From: "J. Bruce Fields" <bfields@fieldses.org>


nfs4_acl_nfsv4_to_posix() returns an error and returns any posix acls
calculated in two caller-provided pointers.  It was setting these
pointers to -errno in some error cases, resulting in
nfsd4_set_nfs4_acl() calling posix_acl_release() with a -errno as an
argument.

Fix both the caller and the callee, by modifying nfsd4_set_nfs4_acl() to
stop relying on the passed-in-pointers being left as NULL in the error
case, and by modifying nfs4_acl_nfsv4_to_posix() to stop returning
garbage in those pointers.

Thanks to Alex Soule for reporting the bug.

Signed-off-by: "J. Bruce Fields" <bfields@citi.umich.edu>
Cc: Alexander Soule <soule@umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfs4acl.c |    3 +++
 ./fs/nfsd/vfs.c     |   22 +++++++---------------
 2 files changed, 10 insertions(+), 15 deletions(-)

diff .prev/fs/nfsd/nfs4acl.c ./fs/nfsd/nfs4acl.c
--- .prev/fs/nfsd/nfs4acl.c	2007-06-21 14:10:06.000000000 +1000
+++ ./fs/nfsd/nfs4acl.c	2007-06-21 14:11:02.000000000 +1000
@@ -737,13 +737,16 @@ int nfs4_acl_nfsv4_to_posix(struct nfs4_
 	*pacl = posix_state_to_acl(&effective_acl_state, flags);
 	if (IS_ERR(*pacl)) {
 		ret = PTR_ERR(*pacl);
+		*pacl = NULL;
 		goto out_dstate;
 	}
 	*dpacl = posix_state_to_acl(&default_acl_state,
 						flags | NFS4_ACL_TYPE_DEFAULT);
 	if (IS_ERR(*dpacl)) {
 		ret = PTR_ERR(*dpacl);
+		*dpacl = NULL;
 		posix_acl_release(*pacl);
+		*pacl = NULL;
 		goto out_dstate;
 	}
 	sort_pacl(*pacl);

diff .prev/fs/nfsd/vfs.c ./fs/nfsd/vfs.c
--- .prev/fs/nfsd/vfs.c	2007-06-21 13:46:38.000000000 +1000
+++ ./fs/nfsd/vfs.c	2007-06-21 14:11:02.000000000 +1000
@@ -435,7 +435,7 @@ nfsd4_set_nfs4_acl(struct svc_rqst *rqst
 	/* Get inode */
 	error = fh_verify(rqstp, fhp, 0 /* S_IFREG */, MAY_SATTR);
 	if (error)
-		goto out;
+		return error;
 
 	dentry = fhp->fh_dentry;
 	inode = dentry->d_inode;
@@ -444,33 +444,25 @@ nfsd4_set_nfs4_acl(struct svc_rqst *rqst
 
 	host_error = nfs4_acl_nfsv4_to_posix(acl, &pacl, &dpacl, flags);
 	if (host_error == -EINVAL) {
-		error = nfserr_attrnotsupp;
-		goto out;
+		return nfserr_attrnotsupp;
 	} else if (host_error < 0)
 		goto out_nfserr;
 
 	host_error = set_nfsv4_acl_one(dentry, pacl, POSIX_ACL_XATTR_ACCESS);
 	if (host_error < 0)
-		goto out_nfserr;
+		goto out_release;
 
-	if (S_ISDIR(inode->i_mode)) {
+	if (S_ISDIR(inode->i_mode))
 		host_error = set_nfsv4_acl_one(dentry, dpacl, POSIX_ACL_XATTR_DEFAULT);
-		if (host_error < 0)
-			goto out_nfserr;
-	}
-
-	error = nfs_ok;
 
-out:
+out_release:
 	posix_acl_release(pacl);
 	posix_acl_release(dpacl);
-	return (error);
 out_nfserr:
 	if (host_error == -EOPNOTSUPP)
-		error = nfserr_attrnotsupp;
+		return nfserr_attrnotsupp;
 	else
-		error = nfserrno(host_error);
-	goto out;
+		return nfserrno(host_error);
 }
 
 static struct posix_acl *

[PATCH 006 of 8] knfsd: nfsd: remove unused header interface.h

[NeilBrown]

From: "J. Bruce Fields" <bfields@fieldses.org>


It looks like Al Viro gutted this header file five years ago and it
hasn't been touched since.

Signed-off-by: "J. Bruce Fields" <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfsctl.c               |    1 -
 ./include/linux/nfsd/interface.h |   13 -------------
 ./include/linux/nfsd/nfsd.h      |    1 -
 3 files changed, 15 deletions(-)

diff .prev/fs/nfsd/nfsctl.c ./fs/nfsd/nfsctl.c
--- .prev/fs/nfsd/nfsctl.c	2007-06-21 13:46:36.000000000 +1000
+++ ./fs/nfsd/nfsctl.c	2007-06-21 14:13:57.000000000 +1000
@@ -35,7 +35,6 @@
 #include <linux/nfsd/cache.h>
 #include <linux/nfsd/xdr.h>
 #include <linux/nfsd/syscall.h>
-#include <linux/nfsd/interface.h>
 
 #include <asm/uaccess.h>
 

diff .prev/include/linux/nfsd/interface.h ./include/linux/nfsd/interface.h
--- .prev/include/linux/nfsd/interface.h	2007-06-21 13:46:36.000000000 +1000
+++ ./include/linux/nfsd/interface.h	1970-01-01 10:00:00.000000000 +1000
@@ -1,13 +0,0 @@
-/*
- * include/linux/nfsd/interface.h
- *
- * defines interface between nfsd and other bits of
- * the kernel.  Particularly filesystems (eventually).
- *
- * Copyright (C) 2000 Neil Brown <neilb@cse.unsw.edu.au>
- */
-
-#ifndef LINUX_NFSD_INTERFACE_H
-#define LINUX_NFSD_INTERFACE_H
-
-#endif /* LINUX_NFSD_INTERFACE_H */

diff .prev/include/linux/nfsd/nfsd.h ./include/linux/nfsd/nfsd.h
--- .prev/include/linux/nfsd/nfsd.h	2007-06-21 13:46:36.000000000 +1000
+++ ./include/linux/nfsd/nfsd.h	2007-06-21 14:13:57.000000000 +1000
@@ -22,7 +22,6 @@
 #include <linux/nfsd/export.h>
 #include <linux/nfsd/auth.h>
 #include <linux/nfsd/stats.h>
-#include <linux/nfsd/interface.h>
 /*
  * nfsd version
  */

[PATCH 007 of 8] knfsd: nfsd4: vary maximum delegation limit based on RAM size

[NeilBrown]

From: "J. Bruce Fields" <bfields@fieldses.org>


Our original NFSv4 delegation policy was to give out a read delegation
on any open when it was possible to.

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
the inode cached.  This becomes an obvious problem the first time a
client's inode cache approaches the size of the server's total memory.

Our first quick solution was to add a hard-coded limit.  This patch
makes a mild incremental improvement by varying that limit according to
the server's total memory size, allowing at most 4 delegations per
megabyte of RAM.

My quick back-of-the-envelope calculation finds that in the worst case
(where every delegation is for a different inode), a delegation could
take about 1.5K, which would make the worst case usage about 6% of
memory.  The new limit works out to be about the same as the old on a
1-gig server.

Signed-off-by: "J. Bruce Fields" <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfs4state.c       |   15 ++++++++++++++-
 ./include/linux/nfsd/nfsd.h |    1 +
 2 files changed, 15 insertions(+), 1 deletion(-)

diff .prev/fs/nfsd/nfs4state.c ./fs/nfsd/nfs4state.c
--- .prev/fs/nfsd/nfs4state.c	2007-06-21 14:09:23.000000000 +1000
+++ ./fs/nfsd/nfs4state.c	2007-06-21 14:14:47.000000000 +1000
@@ -150,6 +150,7 @@ get_nfs4_file(struct nfs4_file *fi)
 }
 
 static int num_delegations;
+unsigned int max_delegations = 0;
 
 /*
  * Open owner state (share locks)
@@ -193,7 +194,7 @@ alloc_init_deleg(struct nfs4_client *clp
 	struct nfs4_callback *cb = &stp->st_stateowner->so_client->cl_callback;
 
 	dprintk("NFSD alloc_init_deleg\n");
-	if (num_delegations > STATEID_HASH_SIZE * 4)
+	if (num_delegations > max_delegations)
 		return NULL;
 	dp = kmem_cache_alloc(deleg_slab, GFP_KERNEL);
 	if (dp == NULL)
@@ -3198,6 +3199,17 @@ get_nfs4_grace_period(void)
 	return max(user_lease_time, lease_time) * HZ;
 }
 
+static void
+set_max_delegations()
+{
+	struct sysinfo sys;
+
+	si_meminfo(&sys);
+	sys.totalram *= sys.mem_unit;
+	sys.totalram >>= (18 - PAGE_SHIFT);
+	max_delegations = (unsigned int) sys.totalram;
+}
+
 /* initialization to perform when the nfsd service is started: */
 
 static void
@@ -3213,6 +3225,7 @@ __nfs4_state_start(void)
 	       grace_time/HZ);
 	laundry_wq = create_singlethread_workqueue("nfsd4");
 	queue_delayed_work(laundry_wq, &laundromat_work, grace_time);
+	set_max_delegations();
 }
 
 int

diff .prev/include/linux/nfsd/nfsd.h ./include/linux/nfsd/nfsd.h
--- .prev/include/linux/nfsd/nfsd.h	2007-06-21 14:13:57.000000000 +1000
+++ ./include/linux/nfsd/nfsd.h	2007-06-21 14:14:47.000000000 +1000
@@ -148,6 +148,7 @@ extern int nfsd_max_blksize;
  * NFSv4 State
  */
 #ifdef CONFIG_NFSD_V4
+extern unsigned int max_delegations;
 void nfs4_state_init(void);
 int nfs4_state_start(void);
 void nfs4_state_shutdown(void);

[PATCH 008 of 8] knfsd: nfsd4: don't delegate files that have had conflicts

[NeilBrown]

From: "J. Bruce Fields" <bfields@fieldses.org>


One more incremental delegation policy improvement: don't give out a
delegation on a file if conflicting access has previously required that
a delegation be revoked on that file.  (In practice we'll forget about
the conflict when the struct nfs4_file is removed on close, so this is
of limited use for now, though it should at least solve a temporary
problem with self-conflicts on write opens from the same client.)

Signed-off-by: "J. Bruce Fields" <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfs4state.c        |    4 ++++
 ./include/linux/nfsd/state.h |    1 +
 2 files changed, 5 insertions(+)

diff .prev/fs/nfsd/nfs4state.c ./fs/nfsd/nfs4state.c
--- .prev/fs/nfsd/nfs4state.c	2007-06-21 14:14:47.000000000 +1000
+++ ./fs/nfsd/nfs4state.c	2007-06-21 14:16:21.000000000 +1000
@@ -194,6 +194,8 @@ alloc_init_deleg(struct nfs4_client *clp
 	struct nfs4_callback *cb = &stp->st_stateowner->so_client->cl_callback;
 
 	dprintk("NFSD alloc_init_deleg\n");
+	if (fp->fi_had_conflict)
+		return NULL;
 	if (num_delegations > max_delegations)
 		return NULL;
 	dp = kmem_cache_alloc(deleg_slab, GFP_KERNEL);
@@ -1002,6 +1004,7 @@ alloc_init_file(struct inode *ino)
 		list_add(&fp->fi_hash, &file_hashtbl[hashval]);
 		fp->fi_inode = igrab(ino);
 		fp->fi_id = current_fileid++;
+		fp->fi_had_conflict = false;
 		return fp;
 	}
 	return NULL;
@@ -1328,6 +1331,7 @@ do_recall(void *__dp)
 {
 	struct nfs4_delegation *dp = __dp;
 
+	dp->dl_file->fi_had_conflict = true;
 	nfsd4_cb_recall(dp);
 	return 0;
 }

diff .prev/include/linux/nfsd/state.h ./include/linux/nfsd/state.h
--- .prev/include/linux/nfsd/state.h	2007-06-21 14:09:31.000000000 +1000
+++ ./include/linux/nfsd/state.h	2007-06-21 14:16:21.000000000 +1000
@@ -224,6 +224,7 @@ struct nfs4_file {
 	struct inode		*fi_inode;
 	u32                     fi_id;      /* used with stateowner->so_id 
 					     * for stateid_hashtbl hash */
+	bool			fi_had_conflict;
 };
 
 /*

Re: [PATCH 007 of 8] knfsd: nfsd4: vary maximum delegation limit based on RAM size

[J. Bruce Fields]

I missed a compile warning here, apologies.

--b.

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index c79f742..fbbbcab 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3204,7 +3204,7 @@ get_nfs4_grace_period(void)
 }
 
 static void
-set_max_delegations()
+set_max_delegations(void)
 {
 	struct sysinfo sys;

Re: [PATCH 008 of 8] knfsd: nfsd4: don't delegate files that have had conflicts

[J. Bruce Fields]

The authorship information got lost on these last two patches--both were
originally by Meelap Shah <meelap@umich.edu>.

(The problem may have been that I had a From: line for him but not a
Signed-off-by: line?)

--b.

Re: [PATCH 008 of 8] knfsd: nfsd4: don't delegate files that have had conflicts

[J. Bruce Fields]

On Thu, Jun 21, 2007 at 12:28:01PM -0400, bfields wrote:
> The authorship information got lost on these last two patches--both were
> originally by Meelap Shah <meelap@umich.edu>.
> 
> (The problem may have been that I had a From: line for him but not a
> Signed-off-by: line?)

Hm, no, actually I think the same happened to patch 1 (from Marc Eshel)
and 4 (from Benny Halevy), neither which had missing signed-off-by's, so
probably your scripts have always ignored From: lines in the body of an
email, and I only just now noticed?

--b.

Re: [PATCH 007 of 8] knfsd: nfsd4: vary maximum delegation limit based on RAM size

[Andrew Morton]

On Thu, 21 Jun 2007 14:31:12 +1000 NeilBrown <neilb@suse.de> wrote:

> 
> From: "J. Bruce Fields" <bfields@fieldses.org>
> 
> 
> Our original NFSv4 delegation policy was to give out a read delegation
> on any open when it was possible to.
> 
> 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
> the inode cached.  This becomes an obvious problem the first time a
> client's inode cache approaches the size of the server's total memory.
> 
> Our first quick solution was to add a hard-coded limit.  This patch
> makes a mild incremental improvement by varying that limit according to
> the server's total memory size, allowing at most 4 delegations per
> megabyte of RAM.
> 
> My quick back-of-the-envelope calculation finds that in the worst case
> (where every delegation is for a different inode), a delegation could
> take about 1.5K, which would make the worst case usage about 6% of
> memory.  The new limit works out to be about the same as the old on a
> 1-gig server.
> 
> ...
>  
> +static void
> +set_max_delegations()
> +{
> +	struct sysinfo sys;
> +
> +	si_meminfo(&sys);
> +	sys.totalram *= sys.mem_unit;
> +	sys.totalram >>= (18 - PAGE_SHIFT);
> +	max_delegations = (unsigned int) sys.totalram;
> +}

Please put yourself in the position of the reader who happens across this
code and wonders why it is that way.

They could of course go hunt it out of the git repo but I do think it's
quite a bit more reader-friendly to explain the thinking in code comments.

Re: [PATCH 007 of 8] knfsd: nfsd4: vary maximum delegation limit based on RAM size

[J. Bruce Fields]

This code would benefit from a little more explanation.

Also, I screwed up the previous patch and forgot to delete a line, so the
calculation here is off; fix it.

Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---

 fs/nfsd/nfs4state.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

On Mon, Jun 25, 2007 at 08:52:44PM -0700, Andrew Morton wrote:
>
> Please put yourself in the position of the reader who happens across this
> code and wonders why it is that way.
> 
> They could of course go hunt it out of the git repo but I do think it's
> quite a bit more reader-friendly to explain the thinking in code comments.
>

Yup, sorry about that.  Could you apply this?

--b.

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index fbbbcab..1b2657f 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3203,13 +3203,27 @@ get_nfs4_grace_period(void)
 	return max(user_lease_time, lease_time) * 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
+ * the inode cached.  This becomes an obvious problem the first time a
+ * client's inode cache approaches the size of the server's total memory.
+ *
+ * For now we avoid this problem by imposing a hard limit on the number
+ * of delegations, which varies according to the server's memory size.
+ */
 static void
 set_max_delegations(void)
 {
 	struct sysinfo sys;
 
 	si_meminfo(&sys);
-	sys.totalram *= sys.mem_unit;
+	/*
+	 * Allow at most 4 delegations per megabyte of RAM.  Quick
+	 * estimates suggest that in the worst case (where every delegation
+	 * is for a different inode), a delegation could take about 1.5K,
+	 * giving a worst case usage of about 6% of memory.
+	 */
 	sys.totalram >>= (18 - PAGE_SHIFT);
 	max_delegations = (unsigned int) sys.totalram;
 }
-- 
1.5.2.58.g98ee

Re: [PATCH 007 of 8] knfsd: nfsd4: vary maximum delegation limit based on RAM size

[Andrew Morton]

On Wed, 27 Jun 2007 22:15:52 -0400 "J. Bruce Fields" <bfields@fieldses.org> wrote:

> +	/*
> +	 * Allow at most 4 delegations per megabyte of RAM.  Quick
> +	 * estimates suggest that in the worst case (where every delegation
> +	 * is for a different inode), a delegation could take about 1.5K,
> +	 * giving a worst case usage of about 6% of memory.
> +	 */
>  	sys.totalram >>= (18 - PAGE_SHIFT);

6% of 32GB is 2GB is too much for a large highmem machine.  We should base
this on the amount of kernel-addressable memory (aka lowmem).

How's this?

static void
set_max_delegations(void)
{
	/*
	 * Allow at most 4 delegations per megabyte of RAM.  Quick
	 * estimates suggest that in the worst case (where every delegation
	 * is for a different inode), a delegation could take about 1.5K,
	 * giving a worst case usage of about 6% of memory.
	 */
	max_delegations = nr_free_buffer_pages() >> (20 - 2 - PAGE_SHIFT);
}


 fs/nfsd/nfs4state.c |    7 ++-----
 mm/page_alloc.c     |    1 +

diff -puN fs/nfsd/nfs4state.c~knfsd-nfsd4-vary-maximum-delegation-limit-based-on-ram-size-fix-fix-fix-fix fs/nfsd/nfs4state.c
--- a/fs/nfsd/nfs4state.c~knfsd-nfsd4-vary-maximum-delegation-limit-based-on-ram-size-fix-fix-fix-fix
+++ a/fs/nfsd/nfs4state.c
@@ -49,6 +49,7 @@
 #include <linux/nfsd/state.h>
 #include <linux/nfsd/xdr4.h>
 #include <linux/namei.h>
+#include <linux/swap.h>
 #include <linux/mutex.h>
 #include <linux/lockd/bind.h>
 #include <linux/module.h>
@@ -3210,17 +3211,13 @@ get_nfs4_grace_period(void)
 static void
 set_max_delegations(void)
 {
-	struct sysinfo sys;
-
-	si_meminfo(&sys);
 	/*
 	 * Allow at most 4 delegations per megabyte of RAM.  Quick
 	 * estimates suggest that in the worst case (where every delegation
 	 * is for a different inode), a delegation could take about 1.5K,
 	 * giving a worst case usage of about 6% of memory.
 	 */
-	sys.totalram >>= (18 - PAGE_SHIFT);
-	max_delegations = (unsigned int) sys.totalram;
+	max_delegations = nr_free_buffer_pages() >> (20 - 2 - PAGE_SHIFT);
 }
 
 /* initialization to perform when the nfsd service is started: */
diff -puN mm/page_alloc.c~knfsd-nfsd4-vary-maximum-delegation-limit-based-on-ram-size-fix-fix-fix-fix mm/page_alloc.c
--- a/mm/page_alloc.c~knfsd-nfsd4-vary-maximum-delegation-limit-based-on-ram-size-fix-fix-fix-fix
+++ a/mm/page_alloc.c
@@ -1729,6 +1729,7 @@ unsigned int nr_free_buffer_pages(void)
 {
 	return nr_free_zone_pages(gfp_zone(GFP_USER));
 }
+EXPORT_SYMBOL_GPL(nr_free_buffer_pages);
 
 /*
  * Amount of free RAM allocatable within all zones
_

Re: [PATCH 007 of 8] knfsd: nfsd4: vary maximum delegation limit based on RAM size

[J. Bruce Fields]

On Wed, Jun 27, 2007 at 07:36:13PM -0700, Andrew Morton wrote:
> How's this?

Makes sense to me, thanks.  I guess fs/nsfd/nfssvc:nfsd_create_serv()
should be similarly modified?  It calculates the size of per-nfsd-thread
buffers used to hold requests, which will eventually be allocated with
an alloc_page(GFP_KERNEL), and it bases the calculation on the same
.totalram field from struct sysinfo.

--b.

Re: [PATCH 007 of 8] knfsd: nfsd4: vary maximum delegation limit based on RAM size

[Andrew Morton]

On Wed, 27 Jun 2007 22:57:39 -0400 "J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Wed, Jun 27, 2007 at 07:36:13PM -0700, Andrew Morton wrote:
> > How's this?
> 
> Makes sense to me, thanks.  I guess fs/nsfd/nfssvc:nfsd_create_serv()
> should be similarly modified?  It calculates the size of per-nfsd-thread
> buffers used to hold requests, which will eventually be allocated with
> an alloc_page(GFP_KERNEL), and it bases the calculation on the same
> .totalram field from struct sysinfo.

Yup, it'd be better to use nr_free_buffer_pages() there too.