All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC 0/7] nfsd: limit the use of md5 hashes in nfsdv4 code
@ 2012-11-09 20:06 Jeff Layton
  2012-11-09 20:06 ` [PATCH RFC 1/7] nfsd: remove unused argument to nfs4_has_reclaimed_state Jeff Layton
                   ` (7 more replies)
  0 siblings, 8 replies; 9+ messages in thread
From: Jeff Layton @ 2012-11-09 20:06 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs

knfsd has historically used md5 hashes as a way to turn nfs_client_id4
blobs into printable strings. It mainly does this to make directories in
the v4recoverydir, but it also uses those strings as part of a scheme to
pick a hash bucket for tracking it.

The choice of md5 here turns out to be problematic. We have customers
that would like to be able to use knfsd while booted in FIPS mode, but
serving v4 currently falls down because those algorithms are proscribed.

This patchset is a first pass at making md5 hashing less necessary.
Clearly, we *do* need md5 hashes for the legacy client tracking code.
But, if someone is using nfsdcltrack, for instance it's not strictly
necessary.

With this patchset it also ought to be possible to support v4 serving
with the legacy tracker, even if md5 is unavailable. Reclaiming won't
work correctly of course, but you'll be able to serve NFSv4 without
that.

This code builds and I've done some rudimentary testing on it that
indicates that it works. It needs more testing, so consider this an RFC.
This patchset is based on the 4 patches that add the nfsdcltrack
recovery tracker.

The code is also available here in my nfsd-3.8 branch:

    http://git.samba.org/?p=jlayton/linux.git;a=shortlog;h=refs/heads/nfsd-3.8

Jeff Layton (7):
  nfsd: remove unused argument to nfs4_has_reclaimed_state
  nfsd: have nfsd4_find_reclaim_client take a char * argument
  nfsd: break out reclaim record removal into separate function
  nfsd: make nfs4_client_to_reclaim return a pointer to the reclaim
    record
  nfsd: don't search for client by hash on legacy reboot recovery
    gracedone
  nfsd: move the confirmed and unconfirmed hlists to a rbtree
  nfsd: get rid of cl_recdir field

 fs/nfsd/nfs4recover.c |  75 +++++++++++++-----
 fs/nfsd/nfs4state.c   | 207 +++++++++++++++++++++++++++++---------------------
 fs/nfsd/state.h       |  14 ++--
 3 files changed, 183 insertions(+), 113 deletions(-)

-- 
1.7.11.7


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

* [PATCH RFC 1/7] nfsd: remove unused argument to nfs4_has_reclaimed_state
  2012-11-09 20:06 [PATCH RFC 0/7] nfsd: limit the use of md5 hashes in nfsdv4 code Jeff Layton
@ 2012-11-09 20:06 ` Jeff Layton
  2012-11-09 20:06 ` [PATCH RFC 2/7] nfsd: have nfsd4_find_reclaim_client take a char * argument Jeff Layton
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Jeff Layton @ 2012-11-09 20:06 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4recover.c | 2 +-
 fs/nfsd/nfs4state.c   | 2 +-
 fs/nfsd/state.h       | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 0a9ac9d..10b7f47 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -319,7 +319,7 @@ purge_old(struct dentry *parent, struct dentry *child)
 {
 	int status;
 
-	if (nfs4_has_reclaimed_state(child->d_name.name, false))
+	if (nfs4_has_reclaimed_state(child->d_name.name))
 		return 0;
 
 	status = vfs_rmdir(parent->d_inode, child);
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index fba2996..f15a2a0 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4465,7 +4465,7 @@ alloc_reclaim(void)
 }
 
 int
-nfs4_has_reclaimed_state(const char *name, bool use_exchange_id)
+nfs4_has_reclaimed_state(const char *name)
 {
 	unsigned int strhashval = clientstr_hashval(name);
 	struct nfs4_client *clp;
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index e036894..bd734f0 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -470,7 +470,7 @@ extern void nfsd4_shutdown_callback(struct nfs4_client *);
 extern void nfs4_put_delegation(struct nfs4_delegation *dp);
 extern __be32 nfs4_make_rec_clidname(char *clidname, struct xdr_netobj *clname);
 extern int nfs4_client_to_reclaim(const char *name);
-extern int nfs4_has_reclaimed_state(const char *name, bool use_exchange_id);
+extern int nfs4_has_reclaimed_state(const char *name);
 extern void release_session_client(struct nfsd4_session *);
 extern void nfsd4_purge_closed_stateid(struct nfs4_stateowner *);
 
-- 
1.7.11.7


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

* [PATCH RFC 2/7] nfsd: have nfsd4_find_reclaim_client take a char * argument
  2012-11-09 20:06 [PATCH RFC 0/7] nfsd: limit the use of md5 hashes in nfsdv4 code Jeff Layton
  2012-11-09 20:06 ` [PATCH RFC 1/7] nfsd: remove unused argument to nfs4_has_reclaimed_state Jeff Layton
@ 2012-11-09 20:06 ` Jeff Layton
  2012-11-09 20:06 ` [PATCH RFC 3/7] nfsd: break out reclaim record removal into separate function Jeff Layton
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Jeff Layton @ 2012-11-09 20:06 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs

Currently, it takes a client pointer, but later we're going to need to
search for these records without knowing whether a matching client even
exists.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4recover.c |  2 +-
 fs/nfsd/nfs4state.c   | 11 ++++-------
 fs/nfsd/state.h       |  2 +-
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 10b7f47..df582fa 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -485,7 +485,7 @@ nfsd4_check_legacy_client(struct nfs4_client *clp)
 		return 0;
 
 	/* look for it in the reclaim hashtable otherwise */
-	if (nfsd4_find_reclaim_client(clp)) {
+	if (nfsd4_find_reclaim_client(clp->cl_recdir)) {
 		set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
 		return 0;
 	}
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f15a2a0..d12f10e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4518,19 +4518,16 @@ nfs4_release_reclaim(void)
 /*
  * called from OPEN, CLAIM_PREVIOUS with a new clientid. */
 struct nfs4_client_reclaim *
-nfsd4_find_reclaim_client(struct nfs4_client *clp)
+nfsd4_find_reclaim_client(const char *recdir)
 {
 	unsigned int strhashval;
 	struct nfs4_client_reclaim *crp = NULL;
 
-	dprintk("NFSD: nfs4_find_reclaim_client for %.*s with recdir %s\n",
-		            clp->cl_name.len, clp->cl_name.data,
-			    clp->cl_recdir);
+	dprintk("NFSD: nfs4_find_reclaim_client for recdir %s\n", recdir);
 
-	/* find clp->cl_name in reclaim_str_hashtbl */
-	strhashval = clientstr_hashval(clp->cl_recdir);
+	strhashval = clientstr_hashval(recdir);
 	list_for_each_entry(crp, &reclaim_str_hashtbl[strhashval], cr_strhash) {
-		if (same_name(crp->cr_recdir, clp->cl_recdir)) {
+		if (same_name(crp->cr_recdir, recdir)) {
 			return crp;
 		}
 	}
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index bd734f0..ec01d45 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -454,7 +454,7 @@ extern void nfs4_lock_state(void);
 extern void nfs4_unlock_state(void);
 extern int nfs4_in_grace(void);
 extern void nfs4_release_reclaim(void);
-extern struct nfs4_client_reclaim *nfsd4_find_reclaim_client(struct nfs4_client *crp);
+extern struct nfs4_client_reclaim *nfsd4_find_reclaim_client(const char *recdir);
 extern __be32 nfs4_check_open_reclaim(clientid_t *clid, bool sessions);
 extern void nfs4_free_openowner(struct nfs4_openowner *);
 extern void nfs4_free_lockowner(struct nfs4_lockowner *);
-- 
1.7.11.7


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

* [PATCH RFC 3/7] nfsd: break out reclaim record removal into separate function
  2012-11-09 20:06 [PATCH RFC 0/7] nfsd: limit the use of md5 hashes in nfsdv4 code Jeff Layton
  2012-11-09 20:06 ` [PATCH RFC 1/7] nfsd: remove unused argument to nfs4_has_reclaimed_state Jeff Layton
  2012-11-09 20:06 ` [PATCH RFC 2/7] nfsd: have nfsd4_find_reclaim_client take a char * argument Jeff Layton
@ 2012-11-09 20:06 ` Jeff Layton
  2012-11-09 20:06 ` [PATCH RFC 4/7] nfsd: make nfs4_client_to_reclaim return a pointer to the reclaim record Jeff Layton
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Jeff Layton @ 2012-11-09 20:06 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs

We'll need to be able to call this from nfs4recover.c eventually.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4state.c | 12 +++++++++---
 fs/nfsd/state.h     |  1 +
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d12f10e..28664de 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4498,6 +4498,14 @@ nfs4_client_to_reclaim(const char *name)
 }
 
 void
+nfs4_remove_reclaim_record(struct nfs4_client_reclaim *crp)
+{
+	list_del(&crp->cr_strhash);
+	kfree(crp);
+	reclaim_str_hashtbl_size--;
+}
+
+void
 nfs4_release_reclaim(void)
 {
 	struct nfs4_client_reclaim *crp = NULL;
@@ -4507,9 +4515,7 @@ nfs4_release_reclaim(void)
 		while (!list_empty(&reclaim_str_hashtbl[i])) {
 			crp = list_entry(reclaim_str_hashtbl[i].next,
 			                struct nfs4_client_reclaim, cr_strhash);
-			list_del(&crp->cr_strhash);
-			kfree(crp);
-			reclaim_str_hashtbl_size--;
+			nfs4_remove_reclaim_record(crp);
 		}
 	}
 	BUG_ON(reclaim_str_hashtbl_size);
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index ec01d45..8983169 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -453,6 +453,7 @@ extern __be32 nfs4_preprocess_stateid_op(struct net *net,
 extern void nfs4_lock_state(void);
 extern void nfs4_unlock_state(void);
 extern int nfs4_in_grace(void);
+void nfs4_remove_reclaim_record(struct nfs4_client_reclaim *);
 extern void nfs4_release_reclaim(void);
 extern struct nfs4_client_reclaim *nfsd4_find_reclaim_client(const char *recdir);
 extern __be32 nfs4_check_open_reclaim(clientid_t *clid, bool sessions);
-- 
1.7.11.7


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

* [PATCH RFC 4/7] nfsd: make nfs4_client_to_reclaim return a pointer to the reclaim record
  2012-11-09 20:06 [PATCH RFC 0/7] nfsd: limit the use of md5 hashes in nfsdv4 code Jeff Layton
                   ` (2 preceding siblings ...)
  2012-11-09 20:06 ` [PATCH RFC 3/7] nfsd: break out reclaim record removal into separate function Jeff Layton
@ 2012-11-09 20:06 ` Jeff Layton
  2012-11-09 20:06 ` [PATCH RFC 5/7] nfsd: don't search for client by hash on legacy reboot recovery gracedone Jeff Layton
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Jeff Layton @ 2012-11-09 20:06 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs

Later callers will need to make changes to the record.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4state.c | 8 ++++----
 fs/nfsd/state.h     | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 28664de..23947b3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4479,22 +4479,22 @@ nfs4_has_reclaimed_state(const char *name)
 /*
  * failure => all reset bets are off, nfserr_no_grace...
  */
-int
+struct nfs4_client_reclaim *
 nfs4_client_to_reclaim(const char *name)
 {
 	unsigned int strhashval;
-	struct nfs4_client_reclaim *crp = NULL;
+	struct nfs4_client_reclaim *crp;
 
 	dprintk("NFSD nfs4_client_to_reclaim NAME: %.*s\n", HEXDIR_LEN, name);
 	crp = alloc_reclaim();
 	if (!crp)
-		return 0;
+		return crp;
 	strhashval = clientstr_hashval(name);
 	INIT_LIST_HEAD(&crp->cr_strhash);
 	list_add(&crp->cr_strhash, &reclaim_str_hashtbl[strhashval]);
 	memcpy(crp->cr_recdir, name, HEXDIR_LEN);
 	reclaim_str_hashtbl_size++;
-	return 1;
+	return crp;
 }
 
 void
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 8983169..7b9e656 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -470,7 +470,7 @@ extern void nfsd4_destroy_callback_queue(void);
 extern void nfsd4_shutdown_callback(struct nfs4_client *);
 extern void nfs4_put_delegation(struct nfs4_delegation *dp);
 extern __be32 nfs4_make_rec_clidname(char *clidname, struct xdr_netobj *clname);
-extern int nfs4_client_to_reclaim(const char *name);
+extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name);
 extern int nfs4_has_reclaimed_state(const char *name);
 extern void release_session_client(struct nfsd4_session *);
 extern void nfsd4_purge_closed_stateid(struct nfs4_stateowner *);
-- 
1.7.11.7


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

* [PATCH RFC 5/7] nfsd: don't search for client by hash on legacy reboot recovery gracedone
  2012-11-09 20:06 [PATCH RFC 0/7] nfsd: limit the use of md5 hashes in nfsdv4 code Jeff Layton
                   ` (3 preceding siblings ...)
  2012-11-09 20:06 ` [PATCH RFC 4/7] nfsd: make nfs4_client_to_reclaim return a pointer to the reclaim record Jeff Layton
@ 2012-11-09 20:06 ` Jeff Layton
  2012-11-09 20:06 ` [PATCH RFC 6/7] nfsd: move the confirmed and unconfirmed hlists to a rbtree Jeff Layton
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Jeff Layton @ 2012-11-09 20:06 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs

When nfsd starts, the legacy reboot recovery code creates a tracking
struct for each directory in the v4recoverydir. When the grace period
ends, it basically does a "readdir" on the directory again, and matches
each dentry in there to an existing client id to see if it should be
removed or not. If the matching client doesn't exist, or hasn't
reclaimed its state then it will remove that dentry.

This is pretty inefficient since it involves doing a lot of hash-bucket
searching. It also means that we have to keep relying on being able to
search for a nfs4_client by md5 hashed cl_recdir name.

Instead, add a pointer to the nfs4_client that indicates the association
between the nfs4_client_reclaim and nfs4_client. When a reclaim operation
comes in, we set the pointer to make that association. On gracedone, the
legacy client tracker will keep the recdir around iff:

1/ there is a reclaim record for the directory

...and...

2/ there's an association between the reclaim record and a client record
-- that is, a create or check operation was performed on the client that
matches that directory.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4recover.c | 31 +++++++++++++++++++++++++++----
 fs/nfsd/nfs4state.c   | 12 +++++-------
 fs/nfsd/state.h       |  4 ++--
 3 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index df582fa..e010152 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -65,6 +65,7 @@ struct nfsd4_client_tracking_ops {
 static struct file *rec_file;
 static char user_recovery_dirname[PATH_MAX] = "/var/lib/nfs/v4recovery";
 static struct nfsd4_client_tracking_ops *client_tracking_ops;
+static bool in_grace;
 
 static int
 nfs4_save_creds(const struct cred **original_creds)
@@ -142,6 +143,7 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
 	const struct cred *original_cred;
 	char *dname = clp->cl_recdir;
 	struct dentry *dir, *dentry;
+	struct nfs4_client_reclaim *crp;
 	int status;
 
 	dprintk("NFSD: nfsd4_create_clid_dir for \"%s\"\n", dname);
@@ -182,13 +184,19 @@ out_put:
 	dput(dentry);
 out_unlock:
 	mutex_unlock(&dir->d_inode->i_mutex);
-	if (status == 0)
+	if (status == 0) {
+		if (in_grace) {
+			crp = nfs4_client_to_reclaim(clp->cl_recdir);
+			if (crp)
+				crp->cr_clp = clp;
+		}
 		vfs_fsync(rec_file, 0);
-	else
+	} else {
 		printk(KERN_ERR "NFSD: failed to write recovery record"
 				" (err %d); please check that %s exists"
 				" and is writeable", status,
 				user_recovery_dirname);
+	}
 	mnt_drop_write_file(rec_file);
 	nfs4_reset_creds(original_cred);
 }
@@ -289,6 +297,7 @@ static void
 nfsd4_remove_clid_dir(struct nfs4_client *clp)
 {
 	const struct cred *original_cred;
+	struct nfs4_client_reclaim *crp;
 	int status;
 
 	if (!rec_file || !test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
@@ -305,8 +314,15 @@ nfsd4_remove_clid_dir(struct nfs4_client *clp)
 
 	status = nfsd4_unlink_clid_dir(clp->cl_recdir, HEXDIR_LEN-1);
 	nfs4_reset_creds(original_cred);
-	if (status == 0)
+	if (status == 0) {
 		vfs_fsync(rec_file, 0);
+		if (in_grace) {
+			/* remove reclaim record */
+			crp = nfsd4_find_reclaim_client(clp->cl_recdir);
+			if (crp)
+				nfs4_remove_reclaim_record(crp);
+		}
+	}
 	mnt_drop_write_file(rec_file);
 out:
 	if (status)
@@ -409,6 +425,8 @@ nfsd4_init_recdir(void)
 	}
 
 	nfs4_reset_creds(original_cred);
+	if (!status)
+		in_grace = true;
 	return status;
 }
 
@@ -446,6 +464,7 @@ nfsd4_shutdown_recdir(void)
 static void
 nfsd4_legacy_tracking_exit(struct net *net)
 {
+	in_grace = false;
 	nfs4_release_reclaim();
 	nfsd4_shutdown_recdir();
 }
@@ -480,13 +499,17 @@ nfs4_recoverydir(void)
 static int
 nfsd4_check_legacy_client(struct nfs4_client *clp)
 {
+	struct nfs4_client_reclaim *crp;
+
 	/* did we already find that this client is stable? */
 	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
 		return 0;
 
 	/* look for it in the reclaim hashtable otherwise */
-	if (nfsd4_find_reclaim_client(clp->cl_recdir)) {
+	crp = nfsd4_find_reclaim_client(clp->cl_recdir);
+	if (crp) {
 		set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
+		crp->cr_clp = clp;
 		return 0;
 	}
 
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 23947b3..f26a7e6 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4464,16 +4464,13 @@ alloc_reclaim(void)
 	return kmalloc(sizeof(struct nfs4_client_reclaim), GFP_KERNEL);
 }
 
-int
+bool
 nfs4_has_reclaimed_state(const char *name)
 {
-	unsigned int strhashval = clientstr_hashval(name);
-	struct nfs4_client *clp;
+	struct nfs4_client_reclaim *crp;
 
-	clp = find_confirmed_client_by_str(name, strhashval);
-	if (!clp)
-		return 0;
-	return test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
+	crp = nfsd4_find_reclaim_client(name);
+	return (crp && crp->cr_clp);
 }
 
 /*
@@ -4494,6 +4491,7 @@ nfs4_client_to_reclaim(const char *name)
 	list_add(&crp->cr_strhash, &reclaim_str_hashtbl[strhashval]);
 	memcpy(crp->cr_recdir, name, HEXDIR_LEN);
 	reclaim_str_hashtbl_size++;
+	crp->cr_clp = NULL;
 	return crp;
 }
 
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 7b9e656..e1b39b9 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -292,6 +292,7 @@ is_client_expired(struct nfs4_client *clp)
  */
 struct nfs4_client_reclaim {
 	struct list_head	cr_strhash;	/* hash by cr_name */
+	struct nfs4_client	*cr_clp;	/* pointer to associated clp */
 	char			cr_recdir[HEXDIR_LEN]; /* recover dir */
 };
 
@@ -452,7 +453,6 @@ extern __be32 nfs4_preprocess_stateid_op(struct net *net,
 		stateid_t *stateid, int flags, struct file **filp);
 extern void nfs4_lock_state(void);
 extern void nfs4_unlock_state(void);
-extern int nfs4_in_grace(void);
 void nfs4_remove_reclaim_record(struct nfs4_client_reclaim *);
 extern void nfs4_release_reclaim(void);
 extern struct nfs4_client_reclaim *nfsd4_find_reclaim_client(const char *recdir);
@@ -471,7 +471,7 @@ extern void nfsd4_shutdown_callback(struct nfs4_client *);
 extern void nfs4_put_delegation(struct nfs4_delegation *dp);
 extern __be32 nfs4_make_rec_clidname(char *clidname, struct xdr_netobj *clname);
 extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name);
-extern int nfs4_has_reclaimed_state(const char *name);
+extern bool nfs4_has_reclaimed_state(const char *name);
 extern void release_session_client(struct nfsd4_session *);
 extern void nfsd4_purge_closed_stateid(struct nfs4_stateowner *);
 
-- 
1.7.11.7


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

* [PATCH RFC 6/7] nfsd: move the confirmed and unconfirmed hlists to a rbtree
  2012-11-09 20:06 [PATCH RFC 0/7] nfsd: limit the use of md5 hashes in nfsdv4 code Jeff Layton
                   ` (4 preceding siblings ...)
  2012-11-09 20:06 ` [PATCH RFC 5/7] nfsd: don't search for client by hash on legacy reboot recovery gracedone Jeff Layton
@ 2012-11-09 20:06 ` Jeff Layton
  2012-11-09 20:06 ` [PATCH RFC 7/7] nfsd: get rid of cl_recdir field Jeff Layton
  2012-11-11 12:46 ` [PATCH RFC 0/7] nfsd: limit the use of md5 hashes in nfsdv4 code J. Bruce Fields
  7 siblings, 0 replies; 9+ messages in thread
From: Jeff Layton @ 2012-11-09 20:06 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs

The current code requires that we md5 hash the name in order to store
the client in the confirmed and unconfirmed trees. Change it instead
to store the clients in a pair of rbtrees, and simply compare the
cl_names directly instead of hashing them. This also necessitates that
we add a new flag to the clp->cl_flags field to indicate which tree
the client is currently in.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4state.c | 144 +++++++++++++++++++++++++++++++++-------------------
 fs/nfsd/state.h     |   3 +-
 2 files changed, 95 insertions(+), 52 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f26a7e6..3894376 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -412,10 +412,10 @@ static unsigned int clientstr_hashval(const char *name)
  * reclaim_str_hashtbl[] holds known client info from previous reset/reboot
  * used in reboot/reset lease grace period processing
  *
- * conf_id_hashtbl[], and conf_str_hashtbl[] hold confirmed
+ * conf_id_hashtbl[], and conf_name_tree hold confirmed
  * setclientid_confirmed info. 
  *
- * unconf_str_hastbl[] and unconf_id_hashtbl[] hold unconfirmed 
+ * unconf_id_hashtbl[] and unconf_name_tree hold unconfirmed
  * setclientid info.
  *
  * client_lru holds client queue ordered by nfs4_client.cl_time
@@ -423,13 +423,15 @@ static unsigned int clientstr_hashval(const char *name)
  *
  * close_lru holds (open) stateowner queue ordered by nfs4_stateowner.so_time
  * for last close replay.
+ *
+ * All of the above fields are protected by the client_mutex.
  */
 static struct list_head	reclaim_str_hashtbl[CLIENT_HASH_SIZE];
 static int reclaim_str_hashtbl_size = 0;
 static struct list_head	conf_id_hashtbl[CLIENT_HASH_SIZE];
-static struct list_head	conf_str_hashtbl[CLIENT_HASH_SIZE];
-static struct list_head	unconf_str_hashtbl[CLIENT_HASH_SIZE];
 static struct list_head	unconf_id_hashtbl[CLIENT_HASH_SIZE];
+static struct rb_root conf_name_tree;
+static struct rb_root unconf_name_tree;
 static struct list_head client_lru;
 static struct list_head close_lru;
 
@@ -1144,7 +1146,10 @@ destroy_client(struct nfs4_client *clp)
 	if (clp->cl_cb_conn.cb_xprt)
 		svc_xprt_put(clp->cl_cb_conn.cb_xprt);
 	list_del(&clp->cl_idhash);
-	list_del(&clp->cl_strhash);
+	if (test_bit(NFSD4_CLIENT_CONFIRMED, &clp->cl_flags))
+		rb_erase(&clp->cl_namenode, &conf_name_tree);
+	else
+		rb_erase(&clp->cl_namenode, &unconf_name_tree);
 	spin_lock(&client_lock);
 	unhash_client_locked(clp);
 	if (atomic_read(&clp->cl_refcount) == 0)
@@ -1187,6 +1192,17 @@ static int copy_cred(struct svc_cred *target, struct svc_cred *source)
 	return 0;
 }
 
+static long
+compare_blob(const struct xdr_netobj *o1, const struct xdr_netobj *o2)
+{
+	long res;
+
+	res = o1->len - o2->len;
+	if (res)
+		return (int)res;
+	return (long)memcmp(o1->data, o2->data, o1->len);
+}
+
 static int same_name(const char *n1, const char *n2)
 {
 	return 0 == memcmp(n1, n2, HEXDIR_LEN);
@@ -1307,7 +1323,6 @@ static struct nfs4_client *create_client(struct xdr_netobj name, char *recdir,
 	atomic_set(&clp->cl_refcount, 0);
 	clp->cl_cb_state = NFSD4_CB_UNKNOWN;
 	INIT_LIST_HEAD(&clp->cl_idhash);
-	INIT_LIST_HEAD(&clp->cl_strhash);
 	INIT_LIST_HEAD(&clp->cl_openowners);
 	INIT_LIST_HEAD(&clp->cl_delegations);
 	INIT_LIST_HEAD(&clp->cl_lru);
@@ -1325,11 +1340,52 @@ static struct nfs4_client *create_client(struct xdr_netobj name, char *recdir,
 }
 
 static void
-add_to_unconfirmed(struct nfs4_client *clp, unsigned int strhashval)
+add_clp_to_name_tree(struct nfs4_client *new_clp, struct rb_root *root)
+{
+	struct rb_node **new = &(root->rb_node), *parent = NULL;
+	struct nfs4_client *clp;
+
+	while (*new) {
+		clp = rb_entry(*new, struct nfs4_client, cl_namenode);
+		parent = *new;
+
+		if (compare_blob(&clp->cl_name, &new_clp->cl_name) > 0)
+			new = &((*new)->rb_left);
+		else
+			new = &((*new)->rb_right);
+	}
+
+	rb_link_node(&new_clp->cl_namenode, parent, new);
+	rb_insert_color(&new_clp->cl_namenode, root);
+}
+
+static struct nfs4_client *
+find_clp_in_name_tree(struct xdr_netobj *name, struct rb_root *root)
+{
+	long cmp;
+	struct rb_node *node = root->rb_node;
+	struct nfs4_client *clp;
+
+	while (node) {
+		clp = rb_entry(node, struct nfs4_client, cl_namenode);
+		cmp = compare_blob(&clp->cl_name, name);
+		if (cmp > 0)
+			node = node->rb_left;
+		else if (cmp < 0)
+			node = node->rb_right;
+		else
+			return clp;
+	}
+	return NULL;
+}
+
+static void
+add_to_unconfirmed(struct nfs4_client *clp)
 {
 	unsigned int idhashval;
 
-	list_add(&clp->cl_strhash, &unconf_str_hashtbl[strhashval]);
+	clear_bit(NFSD4_CLIENT_CONFIRMED, &clp->cl_flags);
+	add_clp_to_name_tree(clp, &unconf_name_tree);
 	idhashval = clientid_hashval(clp->cl_clientid.cl_id);
 	list_add(&clp->cl_idhash, &unconf_id_hashtbl[idhashval]);
 	renew_client(clp);
@@ -1339,12 +1395,12 @@ static void
 move_to_confirmed(struct nfs4_client *clp)
 {
 	unsigned int idhashval = clientid_hashval(clp->cl_clientid.cl_id);
-	unsigned int strhashval;
 
 	dprintk("NFSD: move_to_confirm nfs4_client %p\n", clp);
 	list_move(&clp->cl_idhash, &conf_id_hashtbl[idhashval]);
-	strhashval = clientstr_hashval(clp->cl_recdir);
-	list_move(&clp->cl_strhash, &conf_str_hashtbl[strhashval]);
+	rb_erase(&clp->cl_namenode, &unconf_name_tree);
+	add_clp_to_name_tree(clp, &conf_name_tree);
+	set_bit(NFSD4_CLIENT_CONFIRMED, &clp->cl_flags);
 	renew_client(clp);
 }
 
@@ -1387,27 +1443,15 @@ static bool clp_used_exchangeid(struct nfs4_client *clp)
 } 
 
 static struct nfs4_client *
-find_confirmed_client_by_str(const char *dname, unsigned int hashval)
+find_confirmed_client_by_name(struct xdr_netobj *name)
 {
-	struct nfs4_client *clp;
-
-	list_for_each_entry(clp, &conf_str_hashtbl[hashval], cl_strhash) {
-		if (same_name(clp->cl_recdir, dname))
-			return clp;
-	}
-	return NULL;
+	return find_clp_in_name_tree(name, &conf_name_tree);
 }
 
 static struct nfs4_client *
-find_unconfirmed_client_by_str(const char *dname, unsigned int hashval)
+find_unconfirmed_client_by_name(struct xdr_netobj *name)
 {
-	struct nfs4_client *clp;
-
-	list_for_each_entry(clp, &unconf_str_hashtbl[hashval], cl_strhash) {
-		if (same_name(clp->cl_recdir, dname))
-			return clp;
-	}
-	return NULL;
+	return find_clp_in_name_tree(name, &unconf_name_tree);
 }
 
 static void
@@ -1572,7 +1616,6 @@ nfsd4_exchange_id(struct svc_rqst *rqstp,
 {
 	struct nfs4_client *unconf, *conf, *new;
 	__be32 status;
-	unsigned int		strhashval;
 	char			dname[HEXDIR_LEN];
 	char			addr_str[INET6_ADDRSTRLEN];
 	nfs4_verifier		verf = exid->verifier;
@@ -1605,11 +1648,9 @@ nfsd4_exchange_id(struct svc_rqst *rqstp,
 	if (status)
 		return status;
 
-	strhashval = clientstr_hashval(dname);
-
 	/* Cases below refer to rfc 5661 section 18.35.4: */
 	nfs4_lock_state();
-	conf = find_confirmed_client_by_str(dname, strhashval);
+	conf = find_confirmed_client_by_name(&exid->clname);
 	if (conf) {
 		bool creds_match = same_creds(&conf->cl_cred, &rqstp->rq_cred);
 		bool verfs_match = same_verf(&verf, &conf->cl_verifier);
@@ -1654,7 +1695,7 @@ nfsd4_exchange_id(struct svc_rqst *rqstp,
 		goto out;
 	}
 
-	unconf  = find_unconfirmed_client_by_str(dname, strhashval);
+	unconf  = find_unconfirmed_client_by_name(&exid->clname);
 	if (unconf) /* case 4, possible retry or client restart */
 		expire_client(unconf);
 
@@ -1668,7 +1709,7 @@ out_new:
 	new->cl_minorversion = 1;
 
 	gen_clid(new);
-	add_to_unconfirmed(new, strhashval);
+	add_to_unconfirmed(new);
 out_copy:
 	exid->clientid.cl_boot = new->cl_clientid.cl_boot;
 	exid->clientid.cl_id = new->cl_clientid.cl_id;
@@ -1789,7 +1830,6 @@ nfsd4_create_session(struct svc_rqst *rqstp,
 			goto out_free_conn;
 		}
 	} else if (unconf) {
-		unsigned int hash;
 		struct nfs4_client *old;
 		if (!same_creds(&unconf->cl_cred, &rqstp->rq_cred) ||
 		    !rpc_cmp_addr(sa, (struct sockaddr *) &unconf->cl_addr)) {
@@ -1803,8 +1843,7 @@ nfsd4_create_session(struct svc_rqst *rqstp,
 			status = nfserr_seq_misordered;
 			goto out_free_conn;
 		}
-		hash = clientstr_hashval(unconf->cl_recdir);
-		old = find_confirmed_client_by_str(unconf->cl_recdir, hash);
+		old = find_confirmed_client_by_name(&unconf->cl_name);
 		if (old)
 			expire_client(old);
 		move_to_confirmed(unconf);
@@ -2181,7 +2220,6 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 {
 	struct xdr_netobj 	clname = setclid->se_name;
 	nfs4_verifier		clverifier = setclid->se_verf;
-	unsigned int 		strhashval;
 	struct nfs4_client	*conf, *unconf, *new;
 	__be32 			status;
 	char                    dname[HEXDIR_LEN];
@@ -2190,11 +2228,9 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (status)
 		return status;
 
-	strhashval = clientstr_hashval(dname);
-
 	/* Cases below refer to rfc 3530 section 14.2.33: */
 	nfs4_lock_state();
-	conf = find_confirmed_client_by_str(dname, strhashval);
+	conf = find_confirmed_client_by_name(&clname);
 	if (conf) {
 		/* case 0: */
 		status = nfserr_clid_inuse;
@@ -2209,7 +2245,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 			goto out;
 		}
 	}
-	unconf = find_unconfirmed_client_by_str(dname, strhashval);
+	unconf = find_unconfirmed_client_by_name(&clname);
 	if (unconf)
 		expire_client(unconf);
 	status = nfserr_jukebox;
@@ -2223,7 +2259,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		gen_clid(new);
 	new->cl_minorversion = 0;
 	gen_callback(new, setclid, rqstp);
-	add_to_unconfirmed(new, strhashval);
+	add_to_unconfirmed(new);
 	setclid->se_clientid.cl_boot = new->cl_clientid.cl_boot;
 	setclid->se_clientid.cl_id = new->cl_clientid.cl_id;
 	memcpy(setclid->se_confirm.data, new->cl_confirm.data, sizeof(setclid->se_confirm.data));
@@ -2276,9 +2312,7 @@ nfsd4_setclientid_confirm(struct svc_rqst *rqstp,
 		nfsd4_probe_callback(conf);
 		expire_client(unconf);
 	} else { /* case 3: normal case; new or rebooted client */
-		unsigned int hash = clientstr_hashval(unconf->cl_recdir);
-
-		conf = find_confirmed_client_by_str(unconf->cl_recdir, hash);
+		conf = find_confirmed_client_by_name(&unconf->cl_name);
 		if (conf)
 			expire_client(conf);
 		move_to_confirmed(unconf);
@@ -4687,11 +4721,11 @@ nfs4_state_init(void)
 
 	for (i = 0; i < CLIENT_HASH_SIZE; i++) {
 		INIT_LIST_HEAD(&conf_id_hashtbl[i]);
-		INIT_LIST_HEAD(&conf_str_hashtbl[i]);
-		INIT_LIST_HEAD(&unconf_str_hashtbl[i]);
 		INIT_LIST_HEAD(&unconf_id_hashtbl[i]);
 		INIT_LIST_HEAD(&reclaim_str_hashtbl[i]);
 	}
+	conf_name_tree = RB_ROOT;
+	unconf_name_tree = RB_ROOT;
 	for (i = 0; i < SESSION_HASH_SIZE; i++)
 		INIT_LIST_HEAD(&sessionid_hashtbl[i]);
 	for (i = 0; i < FILE_HASH_SIZE; i++) {
@@ -4776,6 +4810,7 @@ out_recovery:
 	return ret;
 }
 
+/* should be called with the state lock held */
 static void
 __nfs4_state_shutdown(void)
 {
@@ -4783,17 +4818,24 @@ __nfs4_state_shutdown(void)
 	struct nfs4_client *clp = NULL;
 	struct nfs4_delegation *dp = NULL;
 	struct list_head *pos, *next, reaplist;
+	struct rb_node *node, *tmp;
 
 	for (i = 0; i < CLIENT_HASH_SIZE; i++) {
 		while (!list_empty(&conf_id_hashtbl[i])) {
 			clp = list_entry(conf_id_hashtbl[i].next, struct nfs4_client, cl_idhash);
 			destroy_client(clp);
 		}
-		while (!list_empty(&unconf_str_hashtbl[i])) {
-			clp = list_entry(unconf_str_hashtbl[i].next, struct nfs4_client, cl_strhash);
-			destroy_client(clp);
-		}
 	}
+
+	node = rb_first(&unconf_name_tree);
+	while (node != NULL) {
+		tmp = node;
+		node = rb_next(tmp);
+		clp = rb_entry(tmp, struct nfs4_client, cl_namenode);
+		rb_erase(tmp, &unconf_name_tree);
+		destroy_client(clp);
+	}
+
 	INIT_LIST_HEAD(&reaplist);
 	spin_lock(&recall_lock);
 	list_for_each_safe(pos, next, &del_recall_lru) {
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index e1b39b9..5ee88bf 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -221,7 +221,7 @@ struct nfsd4_sessionid {
  */
 struct nfs4_client {
 	struct list_head	cl_idhash; 	/* hash by cl_clientid.id */
-	struct list_head	cl_strhash; 	/* hash by cl_name */
+	struct rb_node		cl_namenode;	/* link into by-name trees */
 	struct list_head	cl_openowners;
 	struct idr		cl_stateids;	/* stateid lookup */
 	struct list_head	cl_delegations;
@@ -242,6 +242,7 @@ struct nfs4_client {
 #define NFSD4_CLIENT_CB_KILL		(1)
 #define NFSD4_CLIENT_STABLE		(2)	/* client on stable storage */
 #define NFSD4_CLIENT_RECLAIM_COMPLETE	(3)	/* reclaim_complete done */
+#define NFSD4_CLIENT_CONFIRMED		(4)	/* client is confirmed */
 #define NFSD4_CLIENT_CB_FLAG_MASK	(1 << NFSD4_CLIENT_CB_UPDATE | \
 					 1 << NFSD4_CLIENT_CB_KILL)
 	unsigned long		cl_flags;
-- 
1.7.11.7


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

* [PATCH RFC 7/7] nfsd: get rid of cl_recdir field
  2012-11-09 20:06 [PATCH RFC 0/7] nfsd: limit the use of md5 hashes in nfsdv4 code Jeff Layton
                   ` (5 preceding siblings ...)
  2012-11-09 20:06 ` [PATCH RFC 6/7] nfsd: move the confirmed and unconfirmed hlists to a rbtree Jeff Layton
@ 2012-11-09 20:06 ` Jeff Layton
  2012-11-11 12:46 ` [PATCH RFC 0/7] nfsd: limit the use of md5 hashes in nfsdv4 code J. Bruce Fields
  7 siblings, 0 replies; 9+ messages in thread
From: Jeff Layton @ 2012-11-09 20:06 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs

Remove the cl_recdir field from the nfs4_client struct. Instead, just
compute it on the fly when and if it's needed, which is now only when
the legacy client tracking code is in effect.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfsd/nfs4recover.c | 48 ++++++++++++++++++++++++++++++++----------------
 fs/nfsd/nfs4state.c   | 18 +++---------------
 fs/nfsd/state.h       |  2 --
 3 files changed, 35 insertions(+), 33 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index e010152..d8d3553 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -103,13 +103,13 @@ md5_to_hex(char *out, char *md5)
 	*out = '\0';
 }
 
-__be32
-nfs4_make_rec_clidname(char *dname, struct xdr_netobj *clname)
+static int
+nfs4_make_rec_clidname(char *dname, const struct xdr_netobj *clname)
 {
 	struct xdr_netobj cksum;
 	struct hash_desc desc;
 	struct scatterlist sg;
-	__be32 status = nfserr_jukebox;
+	int status = -EAGAIN;
 
 	dprintk("NFSD: nfs4_make_rec_clidname for %.*s\n",
 			clname->len, clname->data);
@@ -129,7 +129,7 @@ nfs4_make_rec_clidname(char *dname, struct xdr_netobj *clname)
 
 	md5_to_hex(dname, cksum.data);
 
-	status = nfs_ok;
+	status = 0;
 out:
 	kfree(cksum.data);
 	crypto_free_hash(desc.tfm);
@@ -141,7 +141,7 @@ static void
 nfsd4_create_clid_dir(struct nfs4_client *clp)
 {
 	const struct cred *original_cred;
-	char *dname = clp->cl_recdir;
+	char dname[HEXDIR_LEN];
 	struct dentry *dir, *dentry;
 	struct nfs4_client_reclaim *crp;
 	int status;
@@ -164,6 +164,7 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
 	/* lock the parent */
 	mutex_lock(&dir->d_inode->i_mutex);
 
+	status = nfs4_make_rec_clidname(dname, &clp->cl_name);
 	dentry = lookup_one_len(dname, dir, HEXDIR_LEN-1);
 	if (IS_ERR(dentry)) {
 		status = PTR_ERR(dentry);
@@ -186,7 +187,7 @@ out_unlock:
 	mutex_unlock(&dir->d_inode->i_mutex);
 	if (status == 0) {
 		if (in_grace) {
-			crp = nfs4_client_to_reclaim(clp->cl_recdir);
+			crp = nfs4_client_to_reclaim(dname);
 			if (crp)
 				crp->cr_clp = clp;
 		}
@@ -298,6 +299,7 @@ nfsd4_remove_clid_dir(struct nfs4_client *clp)
 {
 	const struct cred *original_cred;
 	struct nfs4_client_reclaim *crp;
+	char dname[HEXDIR_LEN];
 	int status;
 
 	if (!rec_file || !test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
@@ -312,13 +314,15 @@ nfsd4_remove_clid_dir(struct nfs4_client *clp)
 	if (status < 0)
 		goto out;
 
-	status = nfsd4_unlink_clid_dir(clp->cl_recdir, HEXDIR_LEN-1);
+	status = nfs4_make_rec_clidname(dname, &clp->cl_name);
+	if (!status)
+		status = nfsd4_unlink_clid_dir(dname, HEXDIR_LEN-1);
 	nfs4_reset_creds(original_cred);
 	if (status == 0) {
 		vfs_fsync(rec_file, 0);
 		if (in_grace) {
 			/* remove reclaim record */
-			crp = nfsd4_find_reclaim_client(clp->cl_recdir);
+			crp = nfsd4_find_reclaim_client(dname);
 			if (crp)
 				nfs4_remove_reclaim_record(crp);
 		}
@@ -327,7 +331,7 @@ nfsd4_remove_clid_dir(struct nfs4_client *clp)
 out:
 	if (status)
 		printk("NFSD: Failed to remove expired client state directory"
-				" %.*s\n", HEXDIR_LEN, clp->cl_recdir);
+				" %.*s\n", HEXDIR_LEN, dname);
 }
 
 static int
@@ -499,14 +503,20 @@ nfs4_recoverydir(void)
 static int
 nfsd4_check_legacy_client(struct nfs4_client *clp)
 {
+	int status;
+	char dname[HEXDIR_LEN];
 	struct nfs4_client_reclaim *crp;
 
 	/* did we already find that this client is stable? */
 	if (test_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags))
 		return 0;
 
+	status = nfs4_make_rec_clidname(dname, &clp->cl_name);
+	if (status)
+		return status;
+
 	/* look for it in the reclaim hashtable otherwise */
-	crp = nfsd4_find_reclaim_client(clp->cl_recdir);
+	crp = nfsd4_find_reclaim_client(dname);
 	if (crp) {
 		set_bit(NFSD4_CLIENT_STABLE, &clp->cl_flags);
 		crp->cr_clp = clp;
@@ -992,7 +1002,7 @@ nfsd4_cltrack_legacy_topdir(void)
 }
 
 static char *
-nfsd4_cltrack_legacy_recdir(const char *recdir)
+nfsd4_cltrack_legacy_recdir(const struct xdr_netobj *name)
 {
 	int copied;
 	size_t len;
@@ -1009,10 +1019,16 @@ nfsd4_cltrack_legacy_recdir(const char *recdir)
 	if (!result)
 		return result;
 
-	copied = snprintf(result, len, LEGACY_RECDIR_ENV_PREFIX "%s/%s",
-				nfs4_recoverydir(), recdir);
-	if (copied >= len) {
-		/* just return nothing if output was truncated */
+	copied = snprintf(result, len, LEGACY_RECDIR_ENV_PREFIX "%s/",
+				nfs4_recoverydir());
+	if (copied >= (len - HEXDIR_LEN)) {
+		/* just return nothing if output will be truncated */
+		kfree(result);
+		return NULL;
+	}
+
+	copied = nfs4_make_rec_clidname(result + copied, name);
+	if (copied) {
 		kfree(result);
 		return NULL;
 	}
@@ -1125,7 +1141,7 @@ nfsd4_umh_cltrack_check(struct nfs4_client *clp)
 		dprintk("%s: can't allocate memory for upcall!\n", __func__);
 		return -ENOMEM;
 	}
-	legacy = nfsd4_cltrack_legacy_recdir(clp->cl_recdir);
+	legacy = nfsd4_cltrack_legacy_recdir(&clp->cl_name);
 	ret = nfsd4_umh_cltrack_upcall("check", hexid, legacy);
 	kfree(legacy);
 	kfree(hexid);
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 3894376..dad8e4e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1299,7 +1299,7 @@ static struct nfs4_stid *find_stateid_by_type(struct nfs4_client *cl, stateid_t
 	return NULL;
 }
 
-static struct nfs4_client *create_client(struct xdr_netobj name, char *recdir,
+static struct nfs4_client *create_client(struct xdr_netobj name,
 		struct svc_rqst *rqstp, nfs4_verifier *verf)
 {
 	struct nfs4_client *clp;
@@ -1319,7 +1319,6 @@ static struct nfs4_client *create_client(struct xdr_netobj name, char *recdir,
 		return NULL;
 	}
 	idr_init(&clp->cl_stateids);
-	memcpy(clp->cl_recdir, recdir, HEXDIR_LEN);
 	atomic_set(&clp->cl_refcount, 0);
 	clp->cl_cb_state = NFSD4_CB_UNKNOWN;
 	INIT_LIST_HEAD(&clp->cl_idhash);
@@ -1616,7 +1615,6 @@ nfsd4_exchange_id(struct svc_rqst *rqstp,
 {
 	struct nfs4_client *unconf, *conf, *new;
 	__be32 status;
-	char			dname[HEXDIR_LEN];
 	char			addr_str[INET6_ADDRSTRLEN];
 	nfs4_verifier		verf = exid->verifier;
 	struct sockaddr		*sa = svc_addr(rqstp);
@@ -1643,11 +1641,6 @@ nfsd4_exchange_id(struct svc_rqst *rqstp,
 		return nfserr_serverfault;	/* no excuse :-/ */
 	}
 
-	status = nfs4_make_rec_clidname(dname, &exid->clname);
-
-	if (status)
-		return status;
-
 	/* Cases below refer to rfc 5661 section 18.35.4: */
 	nfs4_lock_state();
 	conf = find_confirmed_client_by_name(&exid->clname);
@@ -1701,7 +1694,7 @@ nfsd4_exchange_id(struct svc_rqst *rqstp,
 
 	/* case 1 (normal case) */
 out_new:
-	new = create_client(exid->clname, dname, rqstp, &verf);
+	new = create_client(exid->clname, rqstp, &verf);
 	if (new == NULL) {
 		status = nfserr_jukebox;
 		goto out;
@@ -2222,12 +2215,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	nfs4_verifier		clverifier = setclid->se_verf;
 	struct nfs4_client	*conf, *unconf, *new;
 	__be32 			status;
-	char                    dname[HEXDIR_LEN];
 	
-	status = nfs4_make_rec_clidname(dname, &clname);
-	if (status)
-		return status;
-
 	/* Cases below refer to rfc 3530 section 14.2.33: */
 	nfs4_lock_state();
 	conf = find_confirmed_client_by_name(&clname);
@@ -2249,7 +2237,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (unconf)
 		expire_client(unconf);
 	status = nfserr_jukebox;
-	new = create_client(clname, dname, rqstp, &clverifier);
+	new = create_client(clname, rqstp, &clverifier);
 	if (new == NULL)
 		goto out;
 	if (conf && same_verf(&conf->cl_verifier, &clverifier))
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 5ee88bf..c049039 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -227,7 +227,6 @@ struct nfs4_client {
 	struct list_head	cl_delegations;
 	struct list_head        cl_lru;         /* tail queue */
 	struct xdr_netobj	cl_name; 	/* id generated by client */
-	char                    cl_recdir[HEXDIR_LEN]; /* recovery dir */
 	nfs4_verifier		cl_verifier; 	/* generated by client */
 	time_t                  cl_time;        /* time of last lease renewal */
 	struct sockaddr_storage	cl_addr; 	/* client ipaddress */
@@ -470,7 +469,6 @@ extern int nfsd4_create_callback_queue(void);
 extern void nfsd4_destroy_callback_queue(void);
 extern void nfsd4_shutdown_callback(struct nfs4_client *);
 extern void nfs4_put_delegation(struct nfs4_delegation *dp);
-extern __be32 nfs4_make_rec_clidname(char *clidname, struct xdr_netobj *clname);
 extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name);
 extern bool nfs4_has_reclaimed_state(const char *name);
 extern void release_session_client(struct nfsd4_session *);
-- 
1.7.11.7


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

* Re: [PATCH RFC 0/7] nfsd: limit the use of md5 hashes in nfsdv4 code
  2012-11-09 20:06 [PATCH RFC 0/7] nfsd: limit the use of md5 hashes in nfsdv4 code Jeff Layton
                   ` (6 preceding siblings ...)
  2012-11-09 20:06 ` [PATCH RFC 7/7] nfsd: get rid of cl_recdir field Jeff Layton
@ 2012-11-11 12:46 ` J. Bruce Fields
  7 siblings, 0 replies; 9+ messages in thread
From: J. Bruce Fields @ 2012-11-11 12:46 UTC (permalink / raw)
  To: Jeff Layton; +Cc: linux-nfs

On Fri, Nov 09, 2012 at 03:06:37PM -0500, Jeff Layton wrote:
> knfsd has historically used md5 hashes as a way to turn nfs_client_id4
> blobs into printable strings. It mainly does this to make directories in
> the v4recoverydir, but it also uses those strings as part of a scheme to
> pick a hash bucket for tracking it.
> 
> The choice of md5 here turns out to be problematic. We have customers
> that would like to be able to use knfsd while booted in FIPS mode, but
> serving v4 currently falls down because those algorithms are proscribed.
> 
> This patchset is a first pass at making md5 hashing less necessary.
> Clearly, we *do* need md5 hashes for the legacy client tracking code.
> But, if someone is using nfsdcltrack, for instance it's not strictly
> necessary.
> 
> With this patchset it also ought to be possible to support v4 serving
> with the legacy tracker, even if md5 is unavailable. Reclaiming won't
> work correctly of course, but you'll be able to serve NFSv4 without
> that.
> 
> This code builds and I've done some rudimentary testing on it that
> indicates that it works. It needs more testing, so consider this an RFC.
> This patchset is based on the 4 patches that add the nfsdcltrack
> recovery tracker.

Looks reasonable to me, and for what it's worth I've checked that your
nfsd-3.8 branch does pass my usual regression tests (which include some
minimal reboot testing as part of pynfs).

I've applied the first patch.  When you resubmit these if you didn't
mind resending the nfsdcltrack patches, I'll get those applied as
well....

--b.

> 
> The code is also available here in my nfsd-3.8 branch:
> 
>     http://git.samba.org/?p=jlayton/linux.git;a=shortlog;h=refs/heads/nfsd-3.8
> 
> Jeff Layton (7):
>   nfsd: remove unused argument to nfs4_has_reclaimed_state
>   nfsd: have nfsd4_find_reclaim_client take a char * argument
>   nfsd: break out reclaim record removal into separate function
>   nfsd: make nfs4_client_to_reclaim return a pointer to the reclaim
>     record
>   nfsd: don't search for client by hash on legacy reboot recovery
>     gracedone
>   nfsd: move the confirmed and unconfirmed hlists to a rbtree
>   nfsd: get rid of cl_recdir field
> 
>  fs/nfsd/nfs4recover.c |  75 +++++++++++++-----
>  fs/nfsd/nfs4state.c   | 207 +++++++++++++++++++++++++++++---------------------
>  fs/nfsd/state.h       |  14 ++--
>  3 files changed, 183 insertions(+), 113 deletions(-)
> 
> -- 
> 1.7.11.7
> 

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

end of thread, other threads:[~2012-11-11 12:46 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-09 20:06 [PATCH RFC 0/7] nfsd: limit the use of md5 hashes in nfsdv4 code Jeff Layton
2012-11-09 20:06 ` [PATCH RFC 1/7] nfsd: remove unused argument to nfs4_has_reclaimed_state Jeff Layton
2012-11-09 20:06 ` [PATCH RFC 2/7] nfsd: have nfsd4_find_reclaim_client take a char * argument Jeff Layton
2012-11-09 20:06 ` [PATCH RFC 3/7] nfsd: break out reclaim record removal into separate function Jeff Layton
2012-11-09 20:06 ` [PATCH RFC 4/7] nfsd: make nfs4_client_to_reclaim return a pointer to the reclaim record Jeff Layton
2012-11-09 20:06 ` [PATCH RFC 5/7] nfsd: don't search for client by hash on legacy reboot recovery gracedone Jeff Layton
2012-11-09 20:06 ` [PATCH RFC 6/7] nfsd: move the confirmed and unconfirmed hlists to a rbtree Jeff Layton
2012-11-09 20:06 ` [PATCH RFC 7/7] nfsd: get rid of cl_recdir field Jeff Layton
2012-11-11 12:46 ` [PATCH RFC 0/7] nfsd: limit the use of md5 hashes in nfsdv4 code J. Bruce Fields

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.