[PATCH 000 of 8] knfsd: Introduction

[PATCH 000 of 8] knfsd: Introduction

[NeilBrown]

Follow are 8 knfsd related patches made against 2.6.18-mm2
and suitable for 2.6.19.

1 adds support to tmpfs to make it exportable
2 and 3 are straight forward bugfixes.
4 is a bugfix in dcache.c code, so some external review wouldn't
go astray.
5-8 are more nfsv4 updates.

Thanks,
NeilBrown

 [PATCH 001 of 8] knfsd: Add nfs-export support to tmpfs
 [PATCH 002 of 8] knfsd: lockd: fix refount on nsm.
 [PATCH 003 of 8] knfsd: Fix auto-sizing of nfsd request/reply buffers
 [PATCH 004 of 8] knfsd: Close a race-opportunity in d_splice_alias
 [PATCH 005 of 8] knfsd: nfsd: store export path in export
 [PATCH 006 of 8] knfsd: nfsd4: fslocations data structures
 [PATCH 007 of 8] knfsd: nfsd4: xdr encoding for fs_locations
 [PATCH 008 of 8] knfsd: nfsd4: actually use all the pieces to implement referrals

[PATCH 001 of 8] knfsd: Add nfs-export support to tmpfs

[NeilBrown]

We need to encode a decode the 'file' part of a handle.
We simply use the inode number and generation number to
construct the filehandle.
The generation number is the time when the file was created.
As inode numbers cycle through the full 32 bits before being
reused, there is no real chance of the same inum being allocated
to different files in the same second so this is suitably unique.
Using time-of-day rather than e.g. jiffies makes it less likely that
the same filehandle can be created after a reboot.

In order to be able to decode a filehandle we need to be able to
lookup by inum, which means that the inode needs to be added to the
inode hash table (tmpfs doesn't currently hash inodes as there is never
a need to lookup by inum).  To avoid overhead when not exporting,
we only hash an inode when it is first exported.  This requires a lock
to ensure it isn't hashed twice.

This code is separate from the patch posted in June06 from Atal
Shargorodsky which provided the same functionality, but does borrow
slightly from it.


From: "David M. Grimes" <dgrimes@navisite.com>
Cc: Atal Shargorodsky <atal@codefidence.com>
Cc: Gilad Ben-Yossef <gilad@codefidence.com>


Signed-off-by: David M. Grimes <dgrimes@navisite.com>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./mm/shmem.c |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff .prev/mm/shmem.c ./mm/shmem.c
--- .prev/mm/shmem.c	2006-09-29 11:52:40.000000000 +1000
+++ ./mm/shmem.c	2006-09-29 11:52:44.000000000 +1000
@@ -1362,6 +1362,7 @@ shmem_get_inode(struct super_block *sb, 
 		inode->i_mapping->a_ops = &shmem_aops;
 		inode->i_mapping->backing_dev_info = &shmem_backing_dev_info;
 		inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
+		inode->i_generation = get_seconds();
 		info = SHMEM_I(inode);
 		memset(info, 0, (char *)inode - (char *)info);
 		spin_lock_init(&info->lock);
@@ -1956,6 +1957,71 @@ static struct xattr_handler *shmem_xattr
 };
 #endif
 
+static struct dentry *shmem_get_parent(struct dentry *child)
+{
+	return ERR_PTR(-ESTALE);
+}
+
+static struct dentry *shmem_get_dentry(struct super_block *sb, void *vfh)
+{
+	struct dentry *de = NULL;
+	struct inode *inode;
+	__u32 *fh = vfh;
+
+	inode = ilookup(sb, (unsigned long)fh[0]);
+	if (inode) {
+		if (inode->i_generation == fh[1])
+			de = d_find_alias(inode);
+		iput(inode);
+	}
+
+	return de? de: ERR_PTR(-ESTALE);
+}
+
+static struct dentry *shmem_decode_fh(struct super_block *sb, __u32 *fh, int len, int type,
+				      int (*acceptable)(void *context, struct dentry *de),
+				      void *context)
+{
+	if (len < 2)
+		return ERR_PTR(-ESTALE);
+
+	return sb->s_export_op->find_exported_dentry(sb, fh, NULL, acceptable, context);
+}
+
+static int shmem_encode_fh(struct dentry *dentry, __u32 *fh, int *len, int connectable)
+{
+	struct inode *inode = dentry->d_inode;
+
+	if (*len < 2)
+		return 255;
+
+	if (hlist_unhashed(&inode->i_hash)) {
+		/* Unfortunately insert_inode_hash is not idempotent,
+		 * so as we hash inodes here rather than at creation
+		 * time, we need a lock to ensure we only try
+		 * to do it once
+		 */
+		static DEFINE_SPINLOCK(lock);
+		spin_lock(&lock);
+		if (hlist_unhashed(&inode->i_hash))
+			insert_inode_hash(inode);
+		spin_unlock(&lock);
+	}
+
+	fh[0] = inode->i_ino;
+	fh[1] = inode->i_generation;
+
+	*len = 2;
+	return 1;
+}
+
+static struct export_operations shmem_export_ops = {
+	.get_parent     = shmem_get_parent,
+	.get_dentry     = shmem_get_dentry,
+	.encode_fh      = shmem_encode_fh,
+	.decode_fh      = shmem_decode_fh,
+};
+
 static int shmem_parse_options(char *options, int *mode, uid_t *uid,
 	gid_t *gid, unsigned long *blocks, unsigned long *inodes,
 	int *policy, nodemask_t *policy_nodes)
@@ -2128,6 +2194,7 @@ static int shmem_fill_super(struct super
 					&inodes, &policy, &policy_nodes))
 			return -EINVAL;
 	}
+	sb->s_export_op = &shmem_export_ops;
 #else
 	sb->s_flags |= MS_NOUSER;
 #endif

[PATCH 002 of 8] knfsd: lockd: fix refount on nsm.

[NeilBrown]

If nlm_lookup_host finds what it is looking for
it exits with an extra reference on the matching
'nsm' structure.
So don't actually count the reference until we are
(fairly) sure it is going to be used.

Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/lockd/host.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff .prev/fs/lockd/host.c ./fs/lockd/host.c
--- .prev/fs/lockd/host.c	2006-09-29 11:44:21.000000000 +1000
+++ ./fs/lockd/host.c	2006-09-29 11:55:15.000000000 +1000
@@ -103,8 +103,8 @@ nlm_lookup_host(int server, const struct
 			continue;
 
 		/* See if we have an NSM handle for this client */
-		if (!nsm && (nsm = host->h_nsmhandle) != 0)
-			atomic_inc(&nsm->sm_count);
+		if (!nsm)
+			nsm = host->h_nsmhandle;
 
 		if (host->h_proto != proto)
 			continue;
@@ -120,6 +120,8 @@ nlm_lookup_host(int server, const struct
 		nlm_get_host(host);
 		goto out;
 	}
+	if (nsm)
+		atomic_inc(&nsm->sm_count);
 
 	host = NULL;
 

[PATCH 003 of 8] knfsd: Fix auto-sizing of nfsd request/reply buffers

[NeilBrown]

totalram is measured in pages, not bytes, so PAGE_SHIFT must be used
when trying to find 1/4096 of RAM.

Cc:  "J. Bruce Fields" <bfields@fieldses.org>
Signed-off-by: Neil Brown <neilb@suse.de>

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

diff .prev/fs/nfsd/nfssvc.c ./fs/nfsd/nfssvc.c
--- .prev/fs/nfsd/nfssvc.c	2006-09-29 11:44:32.000000000 +1000
+++ ./fs/nfsd/nfssvc.c	2006-09-29 11:57:27.000000000 +1000
@@ -209,7 +209,7 @@ int nfsd_create_serv(void)
 		 * Of course, this is only a default.
 		 */
 		nfsd_max_blksize = NFSSVC_MAXBLKSIZE;
-		i.totalram >>= 12;
+		i.totalram <<= PAGE_SHIFT - 12;
 		while (nfsd_max_blksize > i.totalram &&
 		       nfsd_max_blksize >= 8*1024*2)
 			nfsd_max_blksize /= 2;

[PATCH 004 of 8] knfsd: Close a race-opportunity in d_splice_alias

[NeilBrown]

There is a possible race in d_splice_alias.
Though __d_find_alias(inode, 1) will only return a dentry
with DCACHE_DISCONNECTED set, it is possible for it to get cleared
before the BUG_ON, and it is is not possible to lock against that.

There are a couple of problems here.
Firstly, the code doesn't match the comment.  The comment describes
a 'disconnected' dentry as being IS_ROOT as well as DCACHE_DISCONNECTED,
however there is not testing of IS_ROOT anythere.

A dentry is marked DCACHE_DISCONNECTED when allocated with
d_alloc_anon, and remains DCACHE_DISCONNECTED while a path is built up
towards the root.  So a dentry can have a valid name and a valid
parent and even grandparent, but will still be DCACHE_DISCONNECTED
until a path to the root is created.  Once the path to the root is
complete, everything in the path gets DCACHE_DISCONNECTED cleared.  So
the fact that DCACHE_DISCONNECTED isn't enough to say that a dentry is
free to be spliced in with a given name.  This can only be allowed if
the dentry does not yet have a name, so the IS_ROOT test is needed too.

However even adding that test to __d_find_alias isn't enough.  As
d_splice_alias drops dcache_lock before calling d_move to perform the
splice, it could race with another thread calling d_splice_alias to
splice the inode in with a different name in a different part of the
tree (in the case where a file has hard links).  So that splicing code
is only really safe for directories (as we know that directories only
have one link).  For directories, the caller of d_splice_alias will be
holding i_mutex on the (unique) parent so there is no room for a race.

A consequence of this is that a non-directory will never benefit from
being spliced into a pre-exisiting dentry, but that isn't a problem.
It is perfectly OK for a non-directory to have multiple dentries, some
anonymous, some not.  And the comment for d_splice_alias says that it
only happens for directories anyway.

Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/dcache.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff .prev/fs/dcache.c ./fs/dcache.c
--- .prev/fs/dcache.c	2006-09-29 12:57:26.000000000 +1000
+++ ./fs/dcache.c	2006-09-29 12:57:34.000000000 +1000
@@ -291,9 +291,9 @@ struct dentry * dget_locked(struct dentr
  * it can be unhashed only if it has no children, or if it is the root
  * of a filesystem.
  *
- * If the inode has a DCACHE_DISCONNECTED alias, then prefer
+ * If the inode has an IS_ROOT, DCACHE_DISCONNECTED alias, then prefer
  * any other hashed alias over that one unless @want_discon is set,
- * in which case only return a DCACHE_DISCONNECTED alias.
+ * in which case only return an IS_ROOT, DCACHE_DISCONNECTED alias.
  */
 
 static struct dentry * __d_find_alias(struct inode *inode, int want_discon)
@@ -309,7 +309,8 @@ static struct dentry * __d_find_alias(st
 		prefetch(next);
 		alias = list_entry(tmp, struct dentry, d_alias);
  		if (S_ISDIR(inode->i_mode) || !d_unhashed(alias)) {
-			if (alias->d_flags & DCACHE_DISCONNECTED)
+			if (IS_ROOT(alias) &&
+			    (alias->d_flags & DCACHE_DISCONNECTED))
 				discon_alias = alias;
 			else if (!want_discon) {
 				__dget_locked(alias);
@@ -1134,7 +1135,7 @@ struct dentry *d_splice_alias(struct ino
 {
 	struct dentry *new = NULL;
 
-	if (inode) {
+	if (inode && S_ISDIR(inode->i_mode)) {
 		spin_lock(&dcache_lock);
 		new = __d_find_alias(inode, 1);
 		if (new) {

[PATCH 005 of 8] knfsd: nfsd: store export path in export

[NeilBrown]

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

Store the export path in the svc_export structure instead of storing only
the dentry.  This will prevent the need for additional d_path calls to
provide NFSv4 fs_locations support.

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

### Diffstat output
 ./fs/nfsd/export.c            |   10 ++++++++++
 ./include/linux/nfsd/export.h |    1 +
 2 files changed, 11 insertions(+)

diff .prev/fs/nfsd/export.c ./fs/nfsd/export.c
--- .prev/fs/nfsd/export.c	2006-09-29 12:57:30.000000000 +1000
+++ ./fs/nfsd/export.c	2006-09-29 12:57:47.000000000 +1000
@@ -325,6 +325,7 @@ static void svc_export_put(struct kref *
 	dput(exp->ex_dentry);
 	mntput(exp->ex_mnt);
 	auth_domain_put(exp->ex_client);
+	kfree(exp->ex_path);
 	kfree(exp);
 }
 
@@ -398,6 +399,7 @@ static int svc_export_parse(struct cache
 	int an_int;
 
 	nd.dentry = NULL;
+	exp.ex_path = NULL;
 
 	if (mesg[mlen-1] != '\n')
 		return -EINVAL;
@@ -428,6 +430,10 @@ static int svc_export_parse(struct cache
 	exp.ex_client = dom;
 	exp.ex_mnt = nd.mnt;
 	exp.ex_dentry = nd.dentry;
+	exp.ex_path = kstrdup(buf, GFP_KERNEL);
+	err = -ENOMEM;
+	if (!exp.ex_path)
+		goto out;
 
 	/* expiry */
 	err = -EINVAL;
@@ -473,6 +479,7 @@ static int svc_export_parse(struct cache
 	else
 		exp_put(expp);
  out:
+ 	kfree(exp.ex_path);
 	if (nd.dentry)
 		path_release(&nd);
  out_no_path:
@@ -524,6 +531,7 @@ static void svc_export_init(struct cache
 	new->ex_client = item->ex_client;
 	new->ex_dentry = dget(item->ex_dentry);
 	new->ex_mnt = mntget(item->ex_mnt);
+	new->ex_path = NULL;
 }
 
 static void export_update(struct cache_head *cnew, struct cache_head *citem)
@@ -535,6 +543,8 @@ static void export_update(struct cache_h
 	new->ex_anon_uid = item->ex_anon_uid;
 	new->ex_anon_gid = item->ex_anon_gid;
 	new->ex_fsid = item->ex_fsid;
+	new->ex_path = item->ex_path;
+	item->ex_path = NULL;
 }
 
 static struct cache_head *svc_export_alloc(void)

diff .prev/include/linux/nfsd/export.h ./include/linux/nfsd/export.h
--- .prev/include/linux/nfsd/export.h	2006-09-29 12:57:30.000000000 +1000
+++ ./include/linux/nfsd/export.h	2006-09-29 12:57:47.000000000 +1000
@@ -51,6 +51,7 @@ struct svc_export {
 	int			ex_flags;
 	struct vfsmount *	ex_mnt;
 	struct dentry *		ex_dentry;
+	char *			ex_path;
 	uid_t			ex_anon_uid;
 	gid_t			ex_anon_gid;
 	int			ex_fsid;

[PATCH 006 of 8] knfsd: nfsd4: fslocations data structures

[NeilBrown]

From: Manoj Naik <manoj@almaden.ibm.com>

Define FS locations structures, some functions to manipulate them, and add
code to parse FS locations in downcall and add to the exports structure.

Signed-off-by: Manoj Naik <manoj@almaden.ibm.com>
Signed-off-by: Fred Isaman <iisaman@citi.umich.edu>
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/export.c            |  118 ++++++++++++++++++++++++++++++++++++++++--
 ./include/linux/nfsd/export.h |   17 ++++++
 2 files changed, 131 insertions(+), 4 deletions(-)

diff .prev/fs/nfsd/export.c ./fs/nfsd/export.c
--- .prev/fs/nfsd/export.c	2006-09-29 12:57:47.000000000 +1000
+++ ./fs/nfsd/export.c	2006-09-29 13:02:13.000000000 +1000
@@ -319,6 +319,17 @@ svc_expkey_update(struct svc_expkey *new
 
 static struct cache_head *export_table[EXPORT_HASHMAX];
 
+static void nfsd4_fslocs_free(struct nfsd4_fs_locations *fsloc)
+{
+	int i;
+
+	for (i = 0; i < fsloc->locations_count; i++) {
+		kfree(fsloc->locations[i].path);
+		kfree(fsloc->locations[i].hosts);
+	}
+	kfree(fsloc->locations);
+}
+
 static void svc_export_put(struct kref *ref)
 {
 	struct svc_export *exp = container_of(ref, struct svc_export, h.ref);
@@ -326,6 +337,7 @@ static void svc_export_put(struct kref *
 	mntput(exp->ex_mnt);
 	auth_domain_put(exp->ex_client);
 	kfree(exp->ex_path);
+	nfsd4_fslocs_free(&exp->ex_fslocs);
 	kfree(exp);
 }
 
@@ -387,6 +399,69 @@ static int check_export(struct inode *in
 
 }
 
+#ifdef CONFIG_NFSD_V4
+
+static int
+fsloc_parse(char **mesg, char *buf, struct nfsd4_fs_locations *fsloc)
+{
+	int len;
+	int migrated, i, err;
+
+	len = qword_get(mesg, buf, PAGE_SIZE);
+	if (len != 5 || memcmp(buf, "fsloc", 5))
+		return 0;
+
+	/* listsize */
+	err = get_int(mesg, &fsloc->locations_count);
+	if (err)
+		return err;
+	if (fsloc->locations_count < 0)
+		return -EINVAL;
+	if (fsloc->locations_count == 0)
+		return 0;
+
+	fsloc->locations = kzalloc(fsloc->locations_count
+			* sizeof(struct nfsd4_fs_location), GFP_KERNEL);
+	if (!fsloc->locations)
+		return -ENOMEM;
+	for (i=0; i < fsloc->locations_count; i++) {
+		/* colon separated host list */
+		err = -EINVAL;
+		len = qword_get(mesg, buf, PAGE_SIZE);
+		if (len <= 0)
+			goto out_free_all;
+		err = -ENOMEM;
+		fsloc->locations[i].hosts = kstrdup(buf, GFP_KERNEL);
+		if (!fsloc->locations[i].hosts)
+			goto out_free_all;
+		err = -EINVAL;
+		/* slash separated path component list */
+		len = qword_get(mesg, buf, PAGE_SIZE);
+		if (len <= 0)
+			goto out_free_all;
+		err = -ENOMEM;
+		fsloc->locations[i].path = kstrdup(buf, GFP_KERNEL);
+		if (!fsloc->locations[i].path)
+			goto out_free_all;
+	}
+	/* migrated */
+	err = get_int(mesg, &migrated);
+	if (err)
+		goto out_free_all;
+	err = -EINVAL;
+	if (migrated < 0 || migrated > 1)
+		goto out_free_all;
+	fsloc->migrated = migrated;
+	return 0;
+out_free_all:
+	nfsd4_fslocs_free(fsloc);
+	return err;
+}
+
+#else /* CONFIG_NFSD_V4 */
+static int fsloc_parse(char **, char *, struct svc_export *) { return 0; }
+#endif
+
 static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
 {
 	/* client path expiry [flags anonuid anongid fsid] */
@@ -441,6 +516,11 @@ static int svc_export_parse(struct cache
 	if (exp.h.expiry_time == 0)
 		goto out;
 
+	/* fs locations */
+	exp.ex_fslocs.locations = NULL;
+	exp.ex_fslocs.locations_count = 0;
+	exp.ex_fslocs.migrated = 0;
+
 	/* flags */
 	err = get_int(&mesg, &an_int);
 	if (err == -ENOENT)
@@ -466,6 +546,10 @@ static int svc_export_parse(struct cache
 
 		err = check_export(nd.dentry->d_inode, exp.ex_flags);
 		if (err) goto out;
+
+		err = fsloc_parse(&mesg, buf, &exp.ex_fslocs);
+		if (err)
+			goto out;
 	}
 
 	expp = svc_export_lookup(&exp);
@@ -489,7 +573,8 @@ static int svc_export_parse(struct cache
 	return err;
 }
 
-static void exp_flags(struct seq_file *m, int flag, int fsid, uid_t anonu, uid_t anong);
+static void exp_flags(struct seq_file *m, int flag, int fsid,
+		uid_t anonu, uid_t anong, struct nfsd4_fs_locations *fslocs);
 
 static int svc_export_show(struct seq_file *m,
 			   struct cache_detail *cd,
@@ -508,8 +593,8 @@ static int svc_export_show(struct seq_fi
 	seq_putc(m, '(');
 	if (test_bit(CACHE_VALID, &h->flags) && 
 	    !test_bit(CACHE_NEGATIVE, &h->flags))
-		exp_flags(m, exp->ex_flags, exp->ex_fsid, 
-			  exp->ex_anon_uid, exp->ex_anon_gid);
+		exp_flags(m, exp->ex_flags, exp->ex_fsid,
+			  exp->ex_anon_uid, exp->ex_anon_gid, &exp->ex_fslocs);
 	seq_puts(m, ")\n");
 	return 0;
 }
@@ -532,6 +617,9 @@ static void svc_export_init(struct cache
 	new->ex_dentry = dget(item->ex_dentry);
 	new->ex_mnt = mntget(item->ex_mnt);
 	new->ex_path = NULL;
+	new->ex_fslocs.locations = NULL;
+	new->ex_fslocs.locations_count = 0;
+	new->ex_fslocs.migrated = 0;
 }
 
 static void export_update(struct cache_head *cnew, struct cache_head *citem)
@@ -545,6 +633,12 @@ static void export_update(struct cache_h
 	new->ex_fsid = item->ex_fsid;
 	new->ex_path = item->ex_path;
 	item->ex_path = NULL;
+	new->ex_fslocs.locations = item->ex_fslocs.locations;
+	item->ex_fslocs.locations = NULL;
+	new->ex_fslocs.locations_count = item->ex_fslocs.locations_count;
+	item->ex_fslocs.locations_count = 0;
+	new->ex_fslocs.migrated = item->ex_fslocs.migrated;
+	item->ex_fslocs.migrated = 0;
 }
 
 static struct cache_head *svc_export_alloc(void)
@@ -1159,7 +1253,8 @@ static struct flags {
 	{ 0, {"", ""}}
 };
 
-static void exp_flags(struct seq_file *m, int flag, int fsid, uid_t anonu, uid_t anong)
+static void exp_flags(struct seq_file *m, int flag, int fsid,
+		uid_t anonu, uid_t anong, struct nfsd4_fs_locations *fsloc)
 {
 	int first = 0;
 	struct flags *flg;
@@ -1175,6 +1270,21 @@ static void exp_flags(struct seq_file *m
 		seq_printf(m, "%sanonuid=%d", first++?",":"", anonu);
 	if (anong != (gid_t)-2 && anong != (0x10000-2))
 		seq_printf(m, "%sanongid=%d", first++?",":"", anong);
+	if (fsloc && fsloc->locations_count > 0) {
+		char *loctype = (fsloc->migrated) ? "refer" : "replicas";
+		int i;
+
+		seq_printf(m, "%s%s=", first++?",":"", loctype);
+		seq_escape(m, fsloc->locations[0].path, ",;@ \t\n\\");
+		seq_putc(m, '@');
+		seq_escape(m, fsloc->locations[0].hosts, ",;@ \t\n\\");
+		for (i = 1; i < fsloc->locations_count; i++) {
+			seq_putc(m, ';');
+			seq_escape(m, fsloc->locations[i].path, ",;@ \t\n\\");
+			seq_putc(m, '@');
+			seq_escape(m, fsloc->locations[i].hosts, ",;@ \t\n\\");
+		}
+	}
 }
 
 static int e_show(struct seq_file *m, void *p)

diff .prev/include/linux/nfsd/export.h ./include/linux/nfsd/export.h
--- .prev/include/linux/nfsd/export.h	2006-09-29 12:57:47.000000000 +1000
+++ ./include/linux/nfsd/export.h	2006-09-29 12:59:01.000000000 +1000
@@ -45,6 +45,22 @@
 
 #ifdef __KERNEL__
 
+/*
+ * FS Locations
+ */
+struct nfsd4_fs_location {
+	char *hosts; /* colon separated list of hosts */
+	char *path;  /* slash separated list of path components */
+};
+
+struct nfsd4_fs_locations {
+	uint32_t locations_count;
+	struct nfsd4_fs_location *locations;
+/* If we're not actually serving this data ourselves (only providing a
+ * list of replicas that do serve it) then we set "migrated": */
+	int migrated;
+};
+
 struct svc_export {
 	struct cache_head	h;
 	struct auth_domain *	ex_client;
@@ -55,6 +71,7 @@ struct svc_export {
 	uid_t			ex_anon_uid;
 	gid_t			ex_anon_gid;
 	int			ex_fsid;
+	struct nfsd4_fs_locations ex_fslocs;
 };
 
 /* an "export key" (expkey) maps a filehandlefragement to an

[PATCH 007 of 8] knfsd: nfsd4: xdr encoding for fs_locations

[NeilBrown]

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

Encode fs_locations attribute.

Signed-off-by: Manoj Naik <manoj@almaden.ibm.com>
Signed-off-by: Fred Isaman <iisaman@citi.umich.edu>
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfs4xdr.c         |  125 ++++++++++++++++++++++++++++++++++++++++++++
 ./include/linux/nfsd/nfsd.h |    3 -
 2 files changed, 126 insertions(+), 2 deletions(-)

diff .prev/fs/nfsd/nfs4xdr.c ./fs/nfsd/nfs4xdr.c
--- .prev/fs/nfsd/nfs4xdr.c	2006-09-29 12:57:28.000000000 +1000
+++ ./fs/nfsd/nfs4xdr.c	2006-09-29 13:04:07.000000000 +1000
@@ -1224,6 +1224,119 @@ nfsd4_decode_compound(struct nfsd4_compo
  			stateowner->so_replay.rp_buflen); 	\
 	} } while (0);
 
+/* Encode as an array of strings the string given with components
+ * seperated @sep.
+ */
+static int nfsd4_encode_components(char sep, char *components,
+				   u32 **pp, int *buflen)
+{
+	u32 *p = *pp;
+	u32 *countp = p;
+	int strlen, count=0;
+	char *str, *end;
+
+	dprintk("nfsd4_encode_components(%s)\n", components);
+	if ((*buflen -= 4) < 0)
+		return nfserr_resource;
+	WRITE32(0); /* We will fill this in with @count later */
+	end = str = components;
+	while (*end) {
+		for (; *end && (*end != sep); end++)
+			; /* Point to end of component */
+		strlen = end - str;
+		if (strlen) {
+			if ((*buflen -= ((XDR_QUADLEN(strlen) << 2) + 4)) < 0)
+				return nfserr_resource;
+			WRITE32(strlen);
+			WRITEMEM(str, strlen);
+			count++;
+		}
+		else
+			end++;
+		str = end;
+	}
+	*pp = p;
+	p = countp;
+	WRITE32(count);
+	return 0;
+}
+
+/*
+ * encode a location element of a fs_locations structure
+ */
+static int nfsd4_encode_fs_location4(struct nfsd4_fs_location *location,
+				    u32 **pp, int *buflen)
+{
+	int status;
+	u32 *p = *pp;
+
+	status = nfsd4_encode_components(':', location->hosts, &p, buflen);
+	if (status)
+		return status;
+	status = nfsd4_encode_components('/', location->path, &p, buflen);
+	if (status)
+		return status;
+	*pp = p;
+	return 0;
+}
+
+/*
+ * Return the path to an export point in the pseudo filesystem namespace
+ * Returned string is safe to use as long as the caller holds a reference
+ * to @exp.
+ */
+static char *nfsd4_path(struct svc_rqst *rqstp, struct svc_export *exp)
+{
+	struct svc_fh tmp_fh;
+	char *path, *rootpath;
+	int stat;
+
+	fh_init(&tmp_fh, NFS4_FHSIZE);
+	stat = exp_pseudoroot(rqstp->rq_client, &tmp_fh, &rqstp->rq_chandle);
+	if (stat)
+		return ERR_PTR(stat);
+	rootpath = tmp_fh.fh_export->ex_path;
+
+	path = exp->ex_path;
+
+	if (strncmp(path, rootpath, strlen(rootpath))) {
+		printk("nfsd: fs_locations failed;"
+			"%s is not contained in %s\n", path, rootpath);
+		return ERR_PTR(-EOPNOTSUPP);
+	}
+
+	return path + strlen(rootpath);
+}
+
+/*
+ *  encode a fs_locations structure
+ */
+static int nfsd4_encode_fs_locations(struct svc_rqst *rqstp,
+				     struct svc_export *exp,
+				     u32 **pp, int *buflen)
+{
+	int status, i;
+	u32 *p = *pp;
+	struct nfsd4_fs_locations *fslocs = &exp->ex_fslocs;
+	char *root = nfsd4_path(rqstp, exp);
+
+	if (IS_ERR(root))
+		return PTR_ERR(root);
+	status = nfsd4_encode_components('/', root, &p, buflen);
+	if (status)
+		return status;
+	if ((*buflen -= 4) < 0)
+		return nfserr_resource;
+	WRITE32(fslocs->locations_count);
+	for (i=0; i<fslocs->locations_count; i++) {
+		status = nfsd4_encode_fs_location4(&fslocs->locations[i],
+						   &p, buflen);
+		if (status)
+			return status;
+	}
+	*pp = p;
+	return 0;
+}
 
 static u32 nfs4_ftypes[16] = {
         NF4BAD,  NF4FIFO, NF4CHR, NF4BAD,
@@ -1335,6 +1448,11 @@ nfsd4_encode_fattr(struct svc_fh *fhp, s
 				goto out_nfserr;
 		}
 	}
+	if (bmval0 & FATTR4_WORD0_FS_LOCATIONS) {
+		if (exp->ex_fslocs.locations == NULL) {
+			bmval0 &= ~FATTR4_WORD0_FS_LOCATIONS;
+		}
+	}
 	if ((buflen -= 16) < 0)
 		goto out_resource;
 
@@ -1514,6 +1632,13 @@ out_acl:
 			goto out_resource;
 		WRITE64((u64) statfs.f_files);
 	}
+	if (bmval0 & FATTR4_WORD0_FS_LOCATIONS) {
+		status = nfsd4_encode_fs_locations(rqstp, exp, &p, &buflen);
+		if (status == nfserr_resource)
+			goto out_resource;
+		if (status)
+			goto out;
+	}
 	if (bmval0 & FATTR4_WORD0_HOMOGENEOUS) {
 		if ((buflen -= 4) < 0)
 			goto out_resource;

diff .prev/include/linux/nfsd/nfsd.h ./include/linux/nfsd/nfsd.h
--- .prev/include/linux/nfsd/nfsd.h	2006-09-29 12:57:28.000000000 +1000
+++ ./include/linux/nfsd/nfsd.h	2006-09-29 13:04:07.000000000 +1000
@@ -292,7 +292,6 @@ static inline int is_fsid(struct svc_fh 
 /*
  * The following attributes are currently not supported by the NFSv4 server:
  *    ARCHIVE       (deprecated anyway)
- *    FS_LOCATIONS  (will be supported eventually)
  *    HIDDEN        (unlikely to be supported any time soon)
  *    MIMETYPE      (unlikely to be supported any time soon)
  *    QUOTA_*       (will be supported in a forthcoming patch)
@@ -308,7 +307,7 @@ static inline int is_fsid(struct svc_fh 
  | FATTR4_WORD0_ACLSUPPORT      | FATTR4_WORD0_CANSETTIME   | FATTR4_WORD0_CASE_INSENSITIVE \
  | FATTR4_WORD0_CASE_PRESERVING | FATTR4_WORD0_CHOWN_RESTRICTED                             \
  | FATTR4_WORD0_FILEHANDLE      | FATTR4_WORD0_FILEID       | FATTR4_WORD0_FILES_AVAIL      \
- | FATTR4_WORD0_FILES_FREE      | FATTR4_WORD0_FILES_TOTAL  | FATTR4_WORD0_HOMOGENEOUS      \
+ | FATTR4_WORD0_FILES_FREE      | FATTR4_WORD0_FILES_TOTAL  | FATTR4_WORD0_FS_LOCATIONS | FATTR4_WORD0_HOMOGENEOUS      \
  | FATTR4_WORD0_MAXFILESIZE     | FATTR4_WORD0_MAXLINK      | FATTR4_WORD0_MAXNAME          \
  | FATTR4_WORD0_MAXREAD         | FATTR4_WORD0_MAXWRITE     | FATTR4_WORD0_ACL)
 

[PATCH 008 of 8] knfsd: nfsd4: actually use all the pieces to implement referrals

[NeilBrown]

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

Use all the pieces set up so far to implement referral support, allowing
return of NFS4ERR_MOVED and fs_locations attribute.

Signed-off-by: Manoj Naik <manoj@almaden.ibm.com>
Signed-off-by: Fred Isaman <iisaman@citi.umich.edu>
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./fs/nfsd/nfs4proc.c        |   30 +++++++++++++++++++------
 ./fs/nfsd/nfs4xdr.c         |   51 ++++++++++++++++++++++++++++++++++++++------
 ./include/linux/nfsd/nfsd.h |    1 
 3 files changed, 69 insertions(+), 13 deletions(-)

diff .prev/fs/nfsd/nfs4proc.c ./fs/nfsd/nfs4proc.c
--- .prev/fs/nfsd/nfs4proc.c	2006-09-29 12:57:26.000000000 +1000
+++ ./fs/nfsd/nfs4proc.c	2006-09-29 13:05:03.000000000 +1000
@@ -802,13 +802,29 @@ nfsd4_proc_compound(struct svc_rqst *rqs
 		* SETCLIENTID_CONFIRM, PUTFH and PUTROOTFH
 		* require a valid current filehandle
 		*/
-		if ((!current_fh->fh_dentry) &&
-		   !((op->opnum == OP_PUTFH) || (op->opnum == OP_PUTROOTFH) ||
-		   (op->opnum == OP_SETCLIENTID) ||
-		   (op->opnum == OP_SETCLIENTID_CONFIRM) ||
-		   (op->opnum == OP_RENEW) || (op->opnum == OP_RESTOREFH) ||
-		   (op->opnum == OP_RELEASE_LOCKOWNER))) {
-			op->status = nfserr_nofilehandle;
+		if (!current_fh->fh_dentry) {
+			if (!((op->opnum == OP_PUTFH) ||
+			      (op->opnum == OP_PUTROOTFH) ||
+			      (op->opnum == OP_SETCLIENTID) ||
+			      (op->opnum == OP_SETCLIENTID_CONFIRM) ||
+			      (op->opnum == OP_RENEW) ||
+			      (op->opnum == OP_RESTOREFH) ||
+			      (op->opnum == OP_RELEASE_LOCKOWNER))) {
+				op->status = nfserr_nofilehandle;
+				goto encode_op;
+			}
+		}
+		/* Check must be done at start of each operation, except
+		 * for GETATTR and ops not listed as returning NFS4ERR_MOVED
+		 */
+		else if (current_fh->fh_export->ex_fslocs.migrated &&
+			 !((op->opnum == OP_GETATTR) ||
+			   (op->opnum == OP_PUTROOTFH) ||
+			   (op->opnum == OP_PUTPUBFH) ||
+			   (op->opnum == OP_RENEW) ||
+			   (op->opnum == OP_SETCLIENTID) ||
+			   (op->opnum == OP_RELEASE_LOCKOWNER))) {
+			op->status = nfserr_moved;
 			goto encode_op;
 		}
 		switch (op->opnum) {

diff .prev/fs/nfsd/nfs4xdr.c ./fs/nfsd/nfs4xdr.c
--- .prev/fs/nfsd/nfs4xdr.c	2006-09-29 13:04:07.000000000 +1000
+++ ./fs/nfsd/nfs4xdr.c	2006-09-29 13:05:03.000000000 +1000
@@ -60,6 +60,14 @@
 
 #define NFSDDBG_FACILITY		NFSDDBG_XDR
 
+/*
+ * As per referral draft, the fsid for a referral MUST be different from the fsid of the containing
+ * directory in order to indicate to the client that a filesystem boundary is present
+ * We use a fixed fsid for a referral
+ */
+#define NFS4_REFERRAL_FSID_MAJOR	0x8000000ULL
+#define NFS4_REFERRAL_FSID_MINOR	0x8000000ULL
+
 static int
 check_filename(char *str, int len, int err)
 {
@@ -1386,6 +1394,25 @@ nfsd4_encode_aclname(struct svc_rqst *rq
 	return nfsd4_encode_name(rqstp, whotype, id, group, p, buflen);
 }
 
+#define WORD0_ABSENT_FS_ATTRS (FATTR4_WORD0_FS_LOCATIONS | FATTR4_WORD0_FSID | \
+			      FATTR4_WORD0_RDATTR_ERROR)
+#define WORD1_ABSENT_FS_ATTRS FATTR4_WORD1_MOUNTED_ON_FILEID
+
+static int fattr_handle_absent_fs(u32 *bmval0, u32 *bmval1, u32 *rdattr_err)
+{
+	/* As per referral draft:  */
+	if (*bmval0 & ~WORD0_ABSENT_FS_ATTRS ||
+	    *bmval1 & ~WORD1_ABSENT_FS_ATTRS) {
+		if (*bmval0 & FATTR4_WORD0_RDATTR_ERROR ||
+	            *bmval0 & FATTR4_WORD0_FS_LOCATIONS)
+			*rdattr_err = NFSERR_MOVED;
+		else
+			return nfserr_moved;
+	}
+	*bmval0 &= WORD0_ABSENT_FS_ATTRS;
+	*bmval1 &= WORD1_ABSENT_FS_ATTRS;
+	return 0;
+}
 
 /*
  * Note: @fhp can be NULL; in this case, we might have to compose the filehandle
@@ -1408,6 +1435,7 @@ nfsd4_encode_fattr(struct svc_fh *fhp, s
 	u32 *attrlenp;
 	u32 dummy;
 	u64 dummy64;
+	u32 rdattr_err = 0;
 	u32 *p = buffer;
 	int status;
 	int aclsupport = 0;
@@ -1417,6 +1445,12 @@ nfsd4_encode_fattr(struct svc_fh *fhp, s
 	BUG_ON(bmval0 & ~NFSD_SUPPORTED_ATTRS_WORD0);
 	BUG_ON(bmval1 & ~NFSD_SUPPORTED_ATTRS_WORD1);
 
+	if (exp->ex_fslocs.migrated) {
+		status = fattr_handle_absent_fs(&bmval0, &bmval1, &rdattr_err);
+		if (status)
+			goto out;
+	}
+
 	status = vfs_getattr(exp->ex_mnt, dentry, &stat);
 	if (status)
 		goto out_nfserr;
@@ -1462,12 +1496,15 @@ nfsd4_encode_fattr(struct svc_fh *fhp, s
 	attrlenp = p++;                /* to be backfilled later */
 
 	if (bmval0 & FATTR4_WORD0_SUPPORTED_ATTRS) {
+		u32 word0 = NFSD_SUPPORTED_ATTRS_WORD0;
 		if ((buflen -= 12) < 0)
 			goto out_resource;
+		if (!aclsupport)
+			word0 &= ~FATTR4_WORD0_ACL;
+		if (!exp->ex_fslocs.locations)
+			word0 &= ~FATTR4_WORD0_FS_LOCATIONS;
 		WRITE32(2);
-		WRITE32(aclsupport ?
-			NFSD_SUPPORTED_ATTRS_WORD0 :
-			NFSD_SUPPORTED_ATTRS_WORD0 & ~FATTR4_WORD0_ACL);
+		WRITE32(word0);
 		WRITE32(NFSD_SUPPORTED_ATTRS_WORD1);
 	}
 	if (bmval0 & FATTR4_WORD0_TYPE) {
@@ -1521,7 +1558,10 @@ nfsd4_encode_fattr(struct svc_fh *fhp, s
 	if (bmval0 & FATTR4_WORD0_FSID) {
 		if ((buflen -= 16) < 0)
 			goto out_resource;
-		if (is_fsid(fhp, rqstp->rq_reffh)) {
+		if (exp->ex_fslocs.migrated) {
+			WRITE64(NFS4_REFERRAL_FSID_MAJOR);
+			WRITE64(NFS4_REFERRAL_FSID_MINOR);
+		} else if (is_fsid(fhp, rqstp->rq_reffh)) {
 			WRITE64((u64)exp->ex_fsid);
 			WRITE64((u64)0);
 		} else {
@@ -1544,7 +1584,7 @@ nfsd4_encode_fattr(struct svc_fh *fhp, s
 	if (bmval0 & FATTR4_WORD0_RDATTR_ERROR) {
 		if ((buflen -= 4) < 0)
 			goto out_resource;
-		WRITE32(0);
+		WRITE32(rdattr_err);
 	}
 	if (bmval0 & FATTR4_WORD0_ACL) {
 		struct nfs4_ace *ace;
@@ -1971,7 +2011,6 @@ nfsd4_encode_getattr(struct nfsd4_compou
 	nfserr = nfsd4_encode_fattr(fhp, fhp->fh_export, fhp->fh_dentry,
 				    resp->p, &buflen, getattr->ga_bmval,
 				    resp->rqstp);
-
 	if (!nfserr)
 		resp->p += buflen;
 	return nfserr;

diff .prev/include/linux/nfsd/nfsd.h ./include/linux/nfsd/nfsd.h
--- .prev/include/linux/nfsd/nfsd.h	2006-09-29 13:04:07.000000000 +1000
+++ ./include/linux/nfsd/nfsd.h	2006-09-29 13:05:03.000000000 +1000
@@ -216,6 +216,7 @@ void		nfsd_lockd_shutdown(void);
 #define	nfserr_clid_inuse	__constant_htonl(NFSERR_CLID_INUSE)
 #define	nfserr_stale_clientid	__constant_htonl(NFSERR_STALE_CLIENTID)
 #define	nfserr_resource		__constant_htonl(NFSERR_RESOURCE)
+#define	nfserr_moved		__constant_htonl(NFSERR_MOVED)
 #define	nfserr_nofilehandle	__constant_htonl(NFSERR_NOFILEHANDLE)
 #define	nfserr_minor_vers_mismatch	__constant_htonl(NFSERR_MINOR_VERS_MISMATCH)
 #define nfserr_share_denied	__constant_htonl(NFSERR_SHARE_DENIED)

Re: [NFS] [PATCH 002 of 8] knfsd: lockd: fix refount on nsm.

[Olaf Kirch]

On Fri, Sep 29, 2006 at 01:08:45PM +1000, NeilBrown wrote:
> If nlm_lookup_host finds what it is looking for
> it exits with an extra reference on the matching
> 'nsm' structure.
> So don't actually count the reference until we are
> (fairly) sure it is going to be used.
> 
> Signed-off-by: Neil Brown <neilb@suse.de>

Correct, even though I would have done it slightly differently. The if()
is not needed anymore, and there's another if (nsm) two lines down.

Olaf

 fs/lockd/host.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

Index: build/fs/lockd/host.c
===================================================================
--- build.orig/fs/lockd/host.c
+++ build/fs/lockd/host.c
@@ -109,9 +109,9 @@ nlm_lookup_host(int server, const struct
 		if (!nlm_cmp_addr(&host->h_addr, sin))
 			continue;
 
-		/* See if we have an NSM handle for this client */
-		if (!nsm && (nsm = host->h_nsmhandle) != 0)
-			atomic_inc(&nsm->sm_count);
+		/* Stash away the NSM handle in case we need
+		 * to create a new host entry. */
+		nsm = host->h_nsmhandle;
 
 		if (host->h_proto != proto)
 			continue;
@@ -133,7 +133,9 @@ nlm_lookup_host(int server, const struct
 	/* Sadly, the host isn't in our hash table yet. See if
 	 * we have an NSM handle for it. If not, create one.
 	 */
-	if (!nsm && !(nsm = nsm_find(sin, hostname, hostname_len)))
+	if (nsm != NULL)
+		atomic_inc(&nsm->sm_count);
+	else if (!(nsm = nsm_find(sin, hostname, hostname_len)))
 		goto out;
 
 	if (!(host = (struct nlm_host *) kmalloc(sizeof(*host), GFP_KERNEL))) {

-- 
Olaf Kirch   |  --- o --- Nous sommes du soleil we love when we play
okir@suse.de |    / | \   sol.dhoop.naytheet.ah kin.ir.samse.qurax

Re: [PATCH 001 of 8] knfsd: Add nfs-export support to tmpfs

[Andrew Morton]

On Fri, 29 Sep 2006 13:08:39 +1000
NeilBrown <neilb@suse.de> wrote:

> +static int shmem_encode_fh(struct dentry *dentry, __u32 *fh, int *len, int connectable)
> +{
> +	struct inode *inode = dentry->d_inode;
> +
> +	if (*len < 2)
> +		return 255;
> +
> +	if (hlist_unhashed(&inode->i_hash)) {
> +		/* Unfortunately insert_inode_hash is not idempotent,
> +		 * so as we hash inodes here rather than at creation
> +		 * time, we need a lock to ensure we only try
> +		 * to do it once
> +		 */
> +		static DEFINE_SPINLOCK(lock);
> +		spin_lock(&lock);
> +		if (hlist_unhashed(&inode->i_hash))
> +			insert_inode_hash(inode);
> +		spin_unlock(&lock);
> +	}

This looks fishy.

How do we get two callers in here at the same time for the same inode?

Why don't other filesystems have the same problem?

Re: [PATCH 006 of 8] knfsd: nfsd4: fslocations data structures

[Andrew Morton]

On Fri, 29 Sep 2006 13:09:13 +1000
NeilBrown <neilb@suse.de> wrote:

> 
> From: Manoj Naik <manoj@almaden.ibm.com>
> 
> Define FS locations structures, some functions to manipulate them, and add
> code to parse FS locations in downcall and add to the exports structure.
> 
> ...
>  
> +#ifdef CONFIG_NFSD_V4
> +
> +static int
> +fsloc_parse(char **mesg, char *buf, struct nfsd4_fs_locations *fsloc)
> +{
> +	int len;
> +	int migrated, i, err;
> +
> +	len = qword_get(mesg, buf, PAGE_SIZE);
> +	if (len != 5 || memcmp(buf, "fsloc", 5))
> +		return 0;
> +
> +	/* listsize */
> +	err = get_int(mesg, &fsloc->locations_count);
> +	if (err)
> +		return err;
> +	if (fsloc->locations_count < 0)

this is unsigned.

> +		return -EINVAL;
> +	if (fsloc->locations_count == 0)
> +		return 0;
> +
> +	fsloc->locations = kzalloc(fsloc->locations_count
> +			* sizeof(struct nfsd4_fs_location), GFP_KERNEL);

This is subject to multiplication overflow.  If it's a privileged operation
and isn't dependent on stuff coming in over the wire then ok..


> +	if (!fsloc->locations)
> +		return -ENOMEM;
> +	for (i=0; i < fsloc->locations_count; i++) {
> +		/* colon separated host list */
> +		err = -EINVAL;
> +		len = qword_get(mesg, buf, PAGE_SIZE);
> +		if (len <= 0)
> +			goto out_free_all;
> +		err = -ENOMEM;
> +		fsloc->locations[i].hosts = kstrdup(buf, GFP_KERNEL);
> +		if (!fsloc->locations[i].hosts)
> +			goto out_free_all;
> +		err = -EINVAL;
> +		/* slash separated path component list */
> +		len = qword_get(mesg, buf, PAGE_SIZE);
> +		if (len <= 0)
> +			goto out_free_all;
> +		err = -ENOMEM;
> +		fsloc->locations[i].path = kstrdup(buf, GFP_KERNEL);
> +		if (!fsloc->locations[i].path)
> +			goto out_free_all;
> +	}
> +	/* migrated */
> +	err = get_int(mesg, &migrated);
> +	if (err)
> +		goto out_free_all;
> +	err = -EINVAL;
> +	if (migrated < 0 || migrated > 1)
> +		goto out_free_all;
> +	fsloc->migrated = migrated;
> +	return 0;
> +out_free_all:
> +	nfsd4_fslocs_free(fsloc);

This call to nfsd4_fslocs_free() can end up kfreeing members of
fsloc->locations[] which haven't been initialised here.  Are we sure the
caller set them all to zero?

> +	return err;
> +}
> +
> +#else /* CONFIG_NFSD_V4 */
> +static int fsloc_parse(char **, char *, struct svc_export *) { return 0; }

static inline

This has a different prototype from the other version of fsloc_parse()

This ain't C++ - function arguments need identifiers as well as types.

Someone needs to read Documentation/SubmitChecklist..

Re: [NFS] [PATCH 001 of 8] knfsd: Add nfs-export support to tmpfs

[Neil Brown]

On Thursday September 28, akpm@osdl.org wrote:
> On Fri, 29 Sep 2006 13:08:39 +1000
> NeilBrown <neilb@suse.de> wrote:
> 
> > +static int shmem_encode_fh(struct dentry *dentry, __u32 *fh, int *len, int connectable)
> > +{
> > +	struct inode *inode = dentry->d_inode;
> > +
> > +	if (*len < 2)
> > +		return 255;
> > +
> > +	if (hlist_unhashed(&inode->i_hash)) {
> > +		/* Unfortunately insert_inode_hash is not idempotent,
> > +		 * so as we hash inodes here rather than at creation
> > +		 * time, we need a lock to ensure we only try
> > +		 * to do it once
> > +		 */
> > +		static DEFINE_SPINLOCK(lock);
> > +		spin_lock(&lock);
> > +		if (hlist_unhashed(&inode->i_hash))
> > +			insert_inode_hash(inode);
> > +		spin_unlock(&lock);
> > +	}
> 
> This looks fishy.
> 
> How do we get two callers in here at the same time for the same inode?

Probably not very easily.
But imagine a file has two hard links in different directories.
And two clients issue LOOKUP requests, one for each link.
They could conceivably be processed at exactly the same time and so
shmem_encode_fh could be running on two different CPU's at the same
time for the same inode.

> 
> Why don't other filesystems have the same problem?
> 

Because most filesystems that hash their inodes do so at the point
where the 'struct inode' is initialised, and that has suitable locking
(I_NEW).  Here in shmem, we are hashing the inode later, the first
time we need an NFS file handle for it.  We no longer have I_NEW to
ensure only one thread tries to add it to the hash table.

The comment tries to explain this, but obviously isn't completely
successful.

NeilBrown

Re: [NFS] [PATCH 001 of 8] knfsd: Add nfs-export support to tmpfs

[Hugh Dickins]

On Fri, 29 Sep 2006, Neil Brown wrote:
> On Thursday September 28, akpm@osdl.org wrote:
> > 
> > Why don't other filesystems have the same problem?
> 
> Because most filesystems that hash their inodes do so at the point
> where the 'struct inode' is initialised, and that has suitable locking
> (I_NEW).  Here in shmem, we are hashing the inode later, the first
> time we need an NFS file handle for it.  We no longer have I_NEW to
> ensure only one thread tries to add it to the hash table.
> 
> The comment tries to explain this, but obviously isn't completely
> successful.

That makes sense.

This patch looks mostly good to me, thanks to David and you for it;
and my apologies to Atal and Gilad for never quite getting around
to looking at theirs properly (but the hashing in this new one does
look better than their linear search).  Though I know tmpfs, I
don't know NFS, so I'm glad this is coming from your direction.

But one anxiety, regarding i_ino.  That's an unsigned long originating
from last_inode in fs/inode.c, isn't it?  Here getting cast to a __u32
to make up one part of the file handle, which will then be cast back
to an unsigned long for ilookup later.

So on 64-bit arches, it'll only take one dedicated creator and
deleter of files to advance the last_inode count to the point where
the high int is non-zero, and shmem_get_dentry won't ever recognize
any inodes created thereafter, always reporting -ESTALE?  I'm on
unfamiliar territory, but it looks to me like we need another
__u32 in the file handle to cope with that?

Then there's the lesser but more tiresome problem, of i_ino collisions
on 32-bit arches.  tmpfs hasn't worried about that to date, just taken
whatever new_inode() has given it from last_inode: but perhaps these
file handles now make that more of a concern?

Hugh

Re: [NFS] [PATCH 006 of 8] knfsd: nfsd4: fslocations data structures

[J. Bruce Fields]

On Thu, Sep 28, 2006 at 11:45:40PM -0700, Andrew Morton wrote:
> On Fri, 29 Sep 2006 13:09:13 +1000
> NeilBrown <neilb@suse.de> wrote:
> 
> > 
> > From: Manoj Naik <manoj@almaden.ibm.com>
> > 
> > Define FS locations structures, some functions to manipulate them, and add
> > code to parse FS locations in downcall and add to the exports structure.
...
> > +	if (fsloc->locations_count < 0)
> 
> this is unsigned.

Yes, thanks.

> > +	fsloc->locations = kzalloc(fsloc->locations_count
> > +			* sizeof(struct nfsd4_fs_location), GFP_KERNEL);
> 
> This is subject to multiplication overflow.  If it's a privileged operation
> and isn't dependent on stuff coming in over the wire then ok..

This is data provided locally by root.  But there's no reason to allow
this to be arbitrarily large, and I'd still prefer to have some
sanity-checking, so I'll replace the bogus "<0" check above by a
check for something "too large".

> > +out_free_all:
> > +	nfsd4_fslocs_free(fsloc);
> 
> This call to nfsd4_fslocs_free() can end up kfreeing members of
> fsloc->locations[] which haven't been initialised here.  Are we sure the
> caller set them all to zero?

Yes.  There will only ever be one caller, and it has to initialize
these to zero.  I agree that this could be more obvious....

> > +	return err;
> > +}
> > +
> > +#else /* CONFIG_NFSD_V4 */
> > +static int fsloc_parse(char **, char *, struct svc_export *) { return 0; }
> 
> static inline
> 
> This has a different prototype from the other version of fsloc_parse()
> 
> This ain't C++ - function arguments need identifiers as well as types.
> 
> Someone needs to read Documentation/SubmitChecklist..

That would be me, sorry, yes.

Bryce Harrington is also setting up automatic compile-testing for us
with the obvious config options turned on and off, so hopefully that
will help save me from myself....

Patches addressing the above follow.

--b.

[PATCH 1 of 3] nfsd4: fix fs locations bounds-checking

[J. Bruce Fields]

The comparison here is obviously useless as locations_count is unsigned.

Though fsloc_parse can only be handed data by root, still I'd rather have
some sanity-checking; so set a (generous) maximum number of fslocations to
keep the following kzalloc to a reasonable size.

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

diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index 7e429ca..71f3655 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -418,7 +418,7 @@ fsloc_parse(char **mesg, char *buf, stru
 	err = get_int(mesg, &fsloc->locations_count);
 	if (err)
 		return err;
-	if (fsloc->locations_count < 0)
+	if (fsloc->locations_count > MAX_FS_LOCATIONS)
 		return -EINVAL;
 	if (fsloc->locations_count == 0)
 		return 0;
diff --git a/include/linux/nfsd/export.h b/include/linux/nfsd/export.h
index 101fb4c..6e78ea9 100644
--- a/include/linux/nfsd/export.h
+++ b/include/linux/nfsd/export.h
@@ -48,6 +48,9 @@ #ifdef __KERNEL__
 /*
  * FS Locations
  */
+
+#define MAX_FS_LOCATIONS	128
+
 struct nfsd4_fs_location {
 	char *hosts; /* colon separated list of hosts */
 	char *path;  /* slash separated list of path components */
-- 
1.4.2.g55c3

[PATCH 2 of 3] nfsd4: fslocs: fix compile in non-CONFIG_NFSD_V4 case

[J. Bruce Fields]

Fix fsloc_parse in the non-CONFIG_NFSD_V4 case, to make it inline, to
make it consistent with the definition in the CONFIG_NFSD_V4 case, and
to include parameter names.

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

diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index 71f3655..ee7b03a 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -463,7 +463,7 @@ out_free_all:
 }
 
 #else /* CONFIG_NFSD_V4 */
-static int fsloc_parse(char **, char *, struct svc_export *) { return 0; }
+static inline int fsloc_parse(char **mesg, char *buf, struct nfsd4_fs_locations *fsloc) { return 0; }
 #endif
 
 static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
-- 
1.4.2.g55c3

[PATCH 3 of 3] nfsd4: fslocs: remove spurious NULL check

[J. Bruce Fields]

It's obvious that the two callers of this function never call it with NULL.

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

diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index ee7b03a..4a11092 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -323,9 +323,6 @@ static void nfsd4_fslocs_free(struct nfs
 {
 	int i;
 
-	if (fsloc == NULL)
-		return;
-
 	for (i = 0; i < fsloc->locations_count; i++) {
 		kfree(fsloc->locations[i].path);
 		kfree(fsloc->locations[i].hosts);
-- 
1.4.2.g55c3

Re: [NFS] [PATCH 001 of 8] knfsd: Add nfs-export support to tmpfs

[Neil Brown]

On Friday September 29, hugh@veritas.com wrote:
> 
> But one anxiety, regarding i_ino.  That's an unsigned long originating
> from last_inode in fs/inode.c, isn't it?  Here getting cast to a __u32
> to make up one part of the file handle, which will then be cast back
> to an unsigned long for ilookup later.

Good point.  Another __u32 in the filehandle is called for.  It will
always be 0 on 32bit arches, but that's not big deal (it will almost
always be 0 on 64bit to...)

> Then there's the lesser but more tiresome problem, of i_ino collisions
> on 32-bit arches.  tmpfs hasn't worried about that to date, just taken
> whatever new_inode() has given it from last_inode: but perhaps these
> file handles now make that more of a concern?

Yes.....
The i_generation will still be different so we wont get to files with
the same filehandle.  But once an inum is reused, the previous one
won't be visible in the hash table any more.  This is easily fixed by
using ilookup5 and testing the i_generation.

There is still the fact that the duplicate inode numbers will be
visible on the client.  I don't think this will upset the NFS client
code as it uses the full filehandle to differentiate files.  I could
confuse usespace, but no worse than tmpfs already could confuse
userspace.  So I think this can be considered to be a non-problem.

So: this patch extends the filehandle to store 64bits of inode number
(high word, then low word, each in host-endian) and uses ilookup5 to
ensure that inum reuse doesn't confuse the server.

It even compiles and appear to work!

Thanks,
NeilBrown

----------------

Fix issues with nfs exporting of tmpfs

Two particular problem:
 i_ino can be 64bit, so use 64 bits in filehandle to store it.

 i_ino can be reused, so use ilookup5 and i_generation to avoid any
  possible confusion.

Cc: Hugh Dickins <hugh@veritas.com>
Cc: "David M. Grimes" <dgrimes@navisite.com>
Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./mm/shmem.c |   29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff .prev/mm/shmem.c ./mm/shmem.c
--- .prev/mm/shmem.c	2006-09-29 11:52:44.000000000 +1000
+++ ./mm/shmem.c	2006-10-03 09:54:11.000000000 +1000
@@ -1962,16 +1962,25 @@ static struct dentry *shmem_get_parent(s
 	return ERR_PTR(-ESTALE);
 }
 
+static int shmem_match(struct inode *ino, void *vfh)
+{
+	__u32 *fh = vfh;
+	__u64 inum = fh[2];
+	inum = (inum << 32) | fh[1];
+	return ino->i_ino == inum && fh[0] == ino->i_generation;
+}
+
 static struct dentry *shmem_get_dentry(struct super_block *sb, void *vfh)
 {
 	struct dentry *de = NULL;
 	struct inode *inode;
 	__u32 *fh = vfh;
+	__u64 inum = fh[2];
+	inum = (inum << 32) | fh[1];
 
-	inode = ilookup(sb, (unsigned long)fh[0]);
+	inode = ilookup5(sb, (unsigned long)(inum+fh[0]), shmem_match, vfh);
 	if (inode) {
-		if (inode->i_generation == fh[1])
-			de = d_find_alias(inode);
+		de = d_find_alias(inode);
 		iput(inode);
 	}
 
@@ -1982,7 +1991,7 @@ static struct dentry *shmem_decode_fh(st
 				      int (*acceptable)(void *context, struct dentry *de),
 				      void *context)
 {
-	if (len < 2)
+	if (len < 3)
 		return ERR_PTR(-ESTALE);
 
 	return sb->s_export_op->find_exported_dentry(sb, fh, NULL, acceptable, context);
@@ -1992,7 +2001,7 @@ static int shmem_encode_fh(struct dentry
 {
 	struct inode *inode = dentry->d_inode;
 
-	if (*len < 2)
+	if (*len < 3)
 		return 255;
 
 	if (hlist_unhashed(&inode->i_hash)) {
@@ -2004,14 +2013,16 @@ static int shmem_encode_fh(struct dentry
 		static DEFINE_SPINLOCK(lock);
 		spin_lock(&lock);
 		if (hlist_unhashed(&inode->i_hash))
-			insert_inode_hash(inode);
+			__insert_inode_hash(inode,
+					    inode->i_ino + inode->i_generation);
 		spin_unlock(&lock);
 	}
 
-	fh[0] = inode->i_ino;
-	fh[1] = inode->i_generation;
+	fh[0] = inode->i_generation;
+	fh[1] = inode->i_ino;
+	fh[2] = ((__u64)inode->i_ino) >> 32;
 
-	*len = 2;
+	*len = 3;
 	return 1;
 }