All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails
@ 2012-01-26 21:54 bjschuma
  2012-01-26 21:54 ` [PATCH 2/3] NFS: Keep idmapper include files in one place bjschuma
                   ` (4 more replies)
  0 siblings, 5 replies; 17+ messages in thread
From: bjschuma @ 2012-01-26 21:54 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs, Bryan Schumaker

From: Bryan Schumaker <bjschuma@netapp.com>

This patch removes the CONFIG_NFS_USE_NEW_IDMAPPER compile option.
First, the idmapper will attempt to map the id using /sbin/request-key
and nfsidmap.  If this fails (if /etc/request-key.conf is not configured
properly) then the idmapper will call the legacy code to perform the
mapping.  I left a comment stating where the legacy code begins to make
it easier for somebody to remove in the future.

Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
---
I tested this on both Archlinux with nfs-utils 1.2.5 and on Fedora with
nfs-utils 1.2.6-rc6.  It did what was expected in both cases.

 fs/nfs/Kconfig            |   11 ------
 fs/nfs/idmap.c            |   76 +++++++++++++++------------------------------
 fs/nfs/sysctl.c           |    2 -
 include/linux/nfs_idmap.h |   25 ---------------
 4 files changed, 25 insertions(+), 89 deletions(-)

diff --git a/fs/nfs/Kconfig b/fs/nfs/Kconfig
index dbcd821..021d2cf 100644
--- a/fs/nfs/Kconfig
+++ b/fs/nfs/Kconfig
@@ -132,14 +132,3 @@ config NFS_USE_KERNEL_DNS
 	select DNS_RESOLVER
 	select KEYS
 	default y
-
-config NFS_USE_NEW_IDMAPPER
-	bool "Use the new idmapper upcall routine"
-	depends on NFS_V4 && KEYS
-	help
-	  Say Y here if you want NFS to use the new idmapper upcall functions.
-	  You will need /sbin/request-key (usually provided by the keyutils
-	  package).  For details, read
-	  <file:Documentation/filesystems/nfs/idmapper.txt>.
-
-	  If you are unsure, say N.
diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
index 2c05f19..56b1104 100644
--- a/fs/nfs/idmap.c
+++ b/fs/nfs/idmap.c
@@ -142,8 +142,6 @@ static int nfs_map_numeric_to_string(__u32 id, char *buf, size_t buflen)
 	return snprintf(buf, buflen, "%u", id);
 }
 
-#ifdef CONFIG_NFS_USE_NEW_IDMAPPER
-
 #include <linux/cred.h>
 #include <linux/sunrpc/sched.h>
 #include <linux/nfs4.h>
@@ -327,43 +325,7 @@ static int nfs_idmap_lookup_id(const char *name, size_t namelen,
 	return ret;
 }
 
-int nfs_map_name_to_uid(const struct nfs_server *server, const char *name, size_t namelen, __u32 *uid)
-{
-	if (nfs_map_string_to_numeric(name, namelen, uid))
-		return 0;
-	return nfs_idmap_lookup_id(name, namelen, "uid", uid);
-}
-
-int nfs_map_group_to_gid(const struct nfs_server *server, const char *name, size_t namelen, __u32 *gid)
-{
-	if (nfs_map_string_to_numeric(name, namelen, gid))
-		return 0;
-	return nfs_idmap_lookup_id(name, namelen, "gid", gid);
-}
-
-int nfs_map_uid_to_name(const struct nfs_server *server, __u32 uid, char *buf, size_t buflen)
-{
-	int ret = -EINVAL;
-
-	if (!(server->caps & NFS_CAP_UIDGID_NOMAP))
-		ret = nfs_idmap_lookup_name(uid, "user", buf, buflen);
-	if (ret < 0)
-		ret = nfs_map_numeric_to_string(uid, buf, buflen);
-	return ret;
-}
-int nfs_map_gid_to_group(const struct nfs_server *server, __u32 gid, char *buf, size_t buflen)
-{
-	int ret = -EINVAL;
-
-	if (!(server->caps & NFS_CAP_UIDGID_NOMAP))
-		ret = nfs_idmap_lookup_name(gid, "group", buf, buflen);
-	if (ret < 0)
-		ret = nfs_map_numeric_to_string(gid, buf, buflen);
-	return ret;
-}
-
-#else  /* CONFIG_NFS_USE_NEW_IDMAPPER not defined */
-
+/* idmap classic begins here */
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/init.h>
@@ -795,19 +757,27 @@ static unsigned int fnvhash32(const void *buf, size_t buflen)
 int nfs_map_name_to_uid(const struct nfs_server *server, const char *name, size_t namelen, __u32 *uid)
 {
 	struct idmap *idmap = server->nfs_client->cl_idmap;
+	int ret = -EINVAL;
 
 	if (nfs_map_string_to_numeric(name, namelen, uid))
 		return 0;
-	return nfs_idmap_id(idmap, &idmap->idmap_user_hash, name, namelen, uid);
+	ret = nfs_idmap_lookup_id(name, namelen, "uid", uid);
+	if (ret < 0)
+		ret = nfs_idmap_id(idmap, &idmap->idmap_user_hash, name, namelen, uid);
+	return ret;
 }
 
-int nfs_map_group_to_gid(const struct nfs_server *server, const char *name, size_t namelen, __u32 *uid)
+int nfs_map_group_to_gid(const struct nfs_server *server, const char *name, size_t namelen, __u32 *gid)
 {
 	struct idmap *idmap = server->nfs_client->cl_idmap;
+	int ret = -EINVAL;
 
-	if (nfs_map_string_to_numeric(name, namelen, uid))
+	if (nfs_map_string_to_numeric(name, namelen, gid))
 		return 0;
-	return nfs_idmap_id(idmap, &idmap->idmap_group_hash, name, namelen, uid);
+	ret = nfs_idmap_lookup_id(name, namelen, "gid", gid);
+	if (ret < 0)
+		ret = nfs_idmap_id(idmap, &idmap->idmap_group_hash, name, namelen, gid);
+	return ret;
 }
 
 int nfs_map_uid_to_name(const struct nfs_server *server, __u32 uid, char *buf, size_t buflen)
@@ -815,22 +785,26 @@ int nfs_map_uid_to_name(const struct nfs_server *server, __u32 uid, char *buf, s
 	struct idmap *idmap = server->nfs_client->cl_idmap;
 	int ret = -EINVAL;
 
-	if (!(server->caps & NFS_CAP_UIDGID_NOMAP))
-		ret = nfs_idmap_name(idmap, &idmap->idmap_user_hash, uid, buf);
+	if (!(server->caps & NFS_CAP_UIDGID_NOMAP)) {
+		ret = nfs_idmap_lookup_name(uid, "user", buf, buflen);
+		if (ret < 0)
+			ret = nfs_idmap_name(idmap, &idmap->idmap_user_hash, uid, buf);
+	}
 	if (ret < 0)
 		ret = nfs_map_numeric_to_string(uid, buf, buflen);
 	return ret;
 }
-int nfs_map_gid_to_group(const struct nfs_server *server, __u32 uid, char *buf, size_t buflen)
+int nfs_map_gid_to_group(const struct nfs_server *server, __u32 gid, char *buf, size_t buflen)
 {
 	struct idmap *idmap = server->nfs_client->cl_idmap;
 	int ret = -EINVAL;
 
-	if (!(server->caps & NFS_CAP_UIDGID_NOMAP))
-		ret = nfs_idmap_name(idmap, &idmap->idmap_group_hash, uid, buf);
+	if (!(server->caps & NFS_CAP_UIDGID_NOMAP)) {
+		ret = nfs_idmap_lookup_name(gid, "group", buf, buflen);
+		if (ret < 0)
+			ret = nfs_idmap_name(idmap, &idmap->idmap_group_hash, gid, buf);
+	}
 	if (ret < 0)
-		ret = nfs_map_numeric_to_string(uid, buf, buflen);
+		ret = nfs_map_numeric_to_string(gid, buf, buflen);
 	return ret;
 }
-
-#endif /* CONFIG_NFS_USE_NEW_IDMAPPER */
diff --git a/fs/nfs/sysctl.c b/fs/nfs/sysctl.c
index 978aaeb..ad4d2e7 100644
--- a/fs/nfs/sysctl.c
+++ b/fs/nfs/sysctl.c
@@ -32,7 +32,6 @@ static ctl_table nfs_cb_sysctls[] = {
 		.extra1 = (int *)&nfs_set_port_min,
 		.extra2 = (int *)&nfs_set_port_max,
 	},
-#ifndef CONFIG_NFS_USE_NEW_IDMAPPER
 	{
 		.procname = "idmap_cache_timeout",
 		.data = &nfs_idmap_cache_timeout,
@@ -40,7 +39,6 @@ static ctl_table nfs_cb_sysctls[] = {
 		.mode = 0644,
 		.proc_handler = proc_dointvec_jiffies,
 	},
-#endif /* CONFIG_NFS_USE_NEW_IDMAPPER */
 #endif
 	{
 		.procname	= "nfs_mountpoint_timeout",
diff --git a/include/linux/nfs_idmap.h b/include/linux/nfs_idmap.h
index 308c188..717fa50 100644
--- a/include/linux/nfs_idmap.h
+++ b/include/linux/nfs_idmap.h
@@ -69,36 +69,11 @@ struct nfs_server;
 struct nfs_fattr;
 struct nfs4_string;
 
-#ifdef CONFIG_NFS_USE_NEW_IDMAPPER
-
 int nfs_idmap_init(void);
 void nfs_idmap_quit(void);
-
-static inline int nfs_idmap_new(struct nfs_client *clp)
-{
-	return 0;
-}
-
-static inline void nfs_idmap_delete(struct nfs_client *clp)
-{
-}
-
-#else /* CONFIG_NFS_USE_NEW_IDMAPPER not set */
-
-static inline int nfs_idmap_init(void)
-{
-	return 0;
-}
-
-static inline void nfs_idmap_quit(void)
-{
-}
-
 int nfs_idmap_new(struct nfs_client *);
 void nfs_idmap_delete(struct nfs_client *);
 
-#endif /* CONFIG_NFS_USE_NEW_IDMAPPER */
-
 void nfs_fattr_init_names(struct nfs_fattr *fattr,
 		struct nfs4_string *owner_name,
 		struct nfs4_string *group_name);
-- 
1.7.8.4


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

* [PATCH 2/3] NFS: Keep idmapper include files in one place
  2012-01-26 21:54 [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails bjschuma
@ 2012-01-26 21:54 ` bjschuma
  2012-02-06 23:05   ` Myklebust, Trond
  2012-01-26 21:54 ` [PATCH 3/3] NFS: Update idmapper documentation bjschuma
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: bjschuma @ 2012-01-26 21:54 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs, Bryan Schumaker

From: Bryan Schumaker <bjschuma@netapp.com>

Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
---
 fs/nfs/idmap.c |   64 ++++++++++++++++++++++++++-----------------------------
 1 files changed, 30 insertions(+), 34 deletions(-)

diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
index 56b1104..fbbc77e 100644
--- a/fs/nfs/idmap.c
+++ b/fs/nfs/idmap.c
@@ -39,6 +39,36 @@
 #include <linux/slab.h>
 #include <linux/nfs_idmap.h>
 #include <linux/nfs_fs.h>
+#include <linux/cred.h>
+#include <linux/sunrpc/sched.h>
+#include <linux/nfs4.h>
+#include <linux/nfs_fs_sb.h>
+#include <linux/keyctl.h>
+#include <linux/key-type.h>
+#include <linux/rcupdate.h>
+#include <linux/err.h>
+#include <keys/user-type.h>
+
+/* include files needed by legacy idmapper */
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/init.h>
+#include <linux/socket.h>
+#include <linux/in.h>
+#include <linux/sched.h>
+#include <linux/sunrpc/clnt.h>
+#include <linux/workqueue.h>
+#include <linux/sunrpc/rpc_pipe_fs.h>
+#include <linux/nfs_fs.h>
+#include "nfs4_fs.h"
+
+#define NFS_UINT_MAXLEN 11
+#define IDMAP_HASH_SZ          128
+
+/* Default cache timeout is 10 minutes */
+unsigned int nfs_idmap_cache_timeout = 600 * HZ;
+const struct cred *id_resolver_cache;
+
 
 /**
  * nfs_fattr_init_names - initialise the nfs_fattr owner_name/group_name fields
@@ -142,21 +172,6 @@ static int nfs_map_numeric_to_string(__u32 id, char *buf, size_t buflen)
 	return snprintf(buf, buflen, "%u", id);
 }
 
-#include <linux/cred.h>
-#include <linux/sunrpc/sched.h>
-#include <linux/nfs4.h>
-#include <linux/nfs_fs_sb.h>
-#include <linux/keyctl.h>
-#include <linux/key-type.h>
-#include <linux/rcupdate.h>
-#include <linux/err.h>
-
-#include <keys/user-type.h>
-
-#define NFS_UINT_MAXLEN 11
-
-const struct cred *id_resolver_cache;
-
 struct key_type key_type_id_resolver = {
 	.name		= "id_resolver",
 	.instantiate	= user_instantiate,
@@ -326,25 +341,6 @@ static int nfs_idmap_lookup_id(const char *name, size_t namelen,
 }
 
 /* idmap classic begins here */
-#include <linux/module.h>
-#include <linux/mutex.h>
-#include <linux/init.h>
-#include <linux/socket.h>
-#include <linux/in.h>
-#include <linux/sched.h>
-#include <linux/sunrpc/clnt.h>
-#include <linux/workqueue.h>
-#include <linux/sunrpc/rpc_pipe_fs.h>
-
-#include <linux/nfs_fs.h>
-
-#include "nfs4_fs.h"
-
-#define IDMAP_HASH_SZ          128
-
-/* Default cache timeout is 10 minutes */
-unsigned int nfs_idmap_cache_timeout = 600 * HZ;
-
 static int param_set_idmap_timeout(const char *val, struct kernel_param *kp)
 {
 	char *endp;
-- 
1.7.8.4


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

* [PATCH 3/3] NFS: Update idmapper documentation
  2012-01-26 21:54 [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails bjschuma
  2012-01-26 21:54 ` [PATCH 2/3] NFS: Keep idmapper include files in one place bjschuma
@ 2012-01-26 21:54 ` bjschuma
  2012-01-26 21:56 ` [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails J. Bruce Fields
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 17+ messages in thread
From: bjschuma @ 2012-01-26 21:54 UTC (permalink / raw)
  To: bfields; +Cc: linux-nfs, Bryan Schumaker

From: Bryan Schumaker <bjschuma@netapp.com>

Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
---
 Documentation/filesystems/nfs/idmapper.txt |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/Documentation/filesystems/nfs/idmapper.txt b/Documentation/filesystems/nfs/idmapper.txt
index 120fd3c..fe03d10 100644
--- a/Documentation/filesystems/nfs/idmapper.txt
+++ b/Documentation/filesystems/nfs/idmapper.txt
@@ -4,13 +4,21 @@ ID Mapper
 =========
 Id mapper is used by NFS to translate user and group ids into names, and to
 translate user and group names into ids.  Part of this translation involves
-performing an upcall to userspace to request the information.  Id mapper will
-user request-key to perform this upcall and cache the result.  The program
-/usr/sbin/nfs.idmap should be called by request-key, and will perform the
-translation and initialize a key with the resulting information.
+performing an upcall to userspace to request the information.  There are two
+ways NFS could obtain this information: placing a call to /sbin/request-key
+or by placing a call to the rpc.idmap daemon.
+
+NFS will attempt to call /sbin/request-key first.  If this succeeds, the
+result will be cached using the generic request-key cache.  This call should
+only fail if /etc/request-key.conf is not configured for the id_resolver key
+type, see the "Configuring" section below if you wish to use the request-key
+method.
+
+If the call to /sbin/request-key fails (if /etc/request-key.conf is not
+configured with the id_resolver key type), then the idmapper will ask the
+legacy rpc.idmap daemon for the id mapping.  This result will be stored
+in a custom NFS idmap cache.
 
- NFS_USE_NEW_IDMAPPER must be selected when configuring the kernel to use this
- feature.
 
 ===========
 Configuring
-- 
1.7.8.4


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

* Re: [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails
  2012-01-26 21:54 [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails bjschuma
  2012-01-26 21:54 ` [PATCH 2/3] NFS: Keep idmapper include files in one place bjschuma
  2012-01-26 21:54 ` [PATCH 3/3] NFS: Update idmapper documentation bjschuma
@ 2012-01-26 21:56 ` J. Bruce Fields
  2012-01-26 21:57   ` Bryan Schumaker
  2012-02-06 23:05 ` Myklebust, Trond
  2012-02-07 19:12 ` Jeff Layton
  4 siblings, 1 reply; 17+ messages in thread
From: J. Bruce Fields @ 2012-01-26 21:56 UTC (permalink / raw)
  To: bjschuma; +Cc: linux-nfs

I'm assuming you meant to send this to Trond, not me?

--b.

On Thu, Jan 26, 2012 at 04:54:23PM -0500, bjschuma@netapp.com wrote:
> From: Bryan Schumaker <bjschuma@netapp.com>
> 
> This patch removes the CONFIG_NFS_USE_NEW_IDMAPPER compile option.
> First, the idmapper will attempt to map the id using /sbin/request-key
> and nfsidmap.  If this fails (if /etc/request-key.conf is not configured
> properly) then the idmapper will call the legacy code to perform the
> mapping.  I left a comment stating where the legacy code begins to make
> it easier for somebody to remove in the future.
> 
> Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
> ---
> I tested this on both Archlinux with nfs-utils 1.2.5 and on Fedora with
> nfs-utils 1.2.6-rc6.  It did what was expected in both cases.
> 
>  fs/nfs/Kconfig            |   11 ------
>  fs/nfs/idmap.c            |   76 +++++++++++++++------------------------------
>  fs/nfs/sysctl.c           |    2 -
>  include/linux/nfs_idmap.h |   25 ---------------
>  4 files changed, 25 insertions(+), 89 deletions(-)
> 
> diff --git a/fs/nfs/Kconfig b/fs/nfs/Kconfig
> index dbcd821..021d2cf 100644
> --- a/fs/nfs/Kconfig
> +++ b/fs/nfs/Kconfig
> @@ -132,14 +132,3 @@ config NFS_USE_KERNEL_DNS
>  	select DNS_RESOLVER
>  	select KEYS
>  	default y
> -
> -config NFS_USE_NEW_IDMAPPER
> -	bool "Use the new idmapper upcall routine"
> -	depends on NFS_V4 && KEYS
> -	help
> -	  Say Y here if you want NFS to use the new idmapper upcall functions.
> -	  You will need /sbin/request-key (usually provided by the keyutils
> -	  package).  For details, read
> -	  <file:Documentation/filesystems/nfs/idmapper.txt>.
> -
> -	  If you are unsure, say N.
> diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
> index 2c05f19..56b1104 100644
> --- a/fs/nfs/idmap.c
> +++ b/fs/nfs/idmap.c
> @@ -142,8 +142,6 @@ static int nfs_map_numeric_to_string(__u32 id, char *buf, size_t buflen)
>  	return snprintf(buf, buflen, "%u", id);
>  }
>  
> -#ifdef CONFIG_NFS_USE_NEW_IDMAPPER
> -
>  #include <linux/cred.h>
>  #include <linux/sunrpc/sched.h>
>  #include <linux/nfs4.h>
> @@ -327,43 +325,7 @@ static int nfs_idmap_lookup_id(const char *name, size_t namelen,
>  	return ret;
>  }
>  
> -int nfs_map_name_to_uid(const struct nfs_server *server, const char *name, size_t namelen, __u32 *uid)
> -{
> -	if (nfs_map_string_to_numeric(name, namelen, uid))
> -		return 0;
> -	return nfs_idmap_lookup_id(name, namelen, "uid", uid);
> -}
> -
> -int nfs_map_group_to_gid(const struct nfs_server *server, const char *name, size_t namelen, __u32 *gid)
> -{
> -	if (nfs_map_string_to_numeric(name, namelen, gid))
> -		return 0;
> -	return nfs_idmap_lookup_id(name, namelen, "gid", gid);
> -}
> -
> -int nfs_map_uid_to_name(const struct nfs_server *server, __u32 uid, char *buf, size_t buflen)
> -{
> -	int ret = -EINVAL;
> -
> -	if (!(server->caps & NFS_CAP_UIDGID_NOMAP))
> -		ret = nfs_idmap_lookup_name(uid, "user", buf, buflen);
> -	if (ret < 0)
> -		ret = nfs_map_numeric_to_string(uid, buf, buflen);
> -	return ret;
> -}
> -int nfs_map_gid_to_group(const struct nfs_server *server, __u32 gid, char *buf, size_t buflen)
> -{
> -	int ret = -EINVAL;
> -
> -	if (!(server->caps & NFS_CAP_UIDGID_NOMAP))
> -		ret = nfs_idmap_lookup_name(gid, "group", buf, buflen);
> -	if (ret < 0)
> -		ret = nfs_map_numeric_to_string(gid, buf, buflen);
> -	return ret;
> -}
> -
> -#else  /* CONFIG_NFS_USE_NEW_IDMAPPER not defined */
> -
> +/* idmap classic begins here */
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/init.h>
> @@ -795,19 +757,27 @@ static unsigned int fnvhash32(const void *buf, size_t buflen)
>  int nfs_map_name_to_uid(const struct nfs_server *server, const char *name, size_t namelen, __u32 *uid)
>  {
>  	struct idmap *idmap = server->nfs_client->cl_idmap;
> +	int ret = -EINVAL;
>  
>  	if (nfs_map_string_to_numeric(name, namelen, uid))
>  		return 0;
> -	return nfs_idmap_id(idmap, &idmap->idmap_user_hash, name, namelen, uid);
> +	ret = nfs_idmap_lookup_id(name, namelen, "uid", uid);
> +	if (ret < 0)
> +		ret = nfs_idmap_id(idmap, &idmap->idmap_user_hash, name, namelen, uid);
> +	return ret;
>  }
>  
> -int nfs_map_group_to_gid(const struct nfs_server *server, const char *name, size_t namelen, __u32 *uid)
> +int nfs_map_group_to_gid(const struct nfs_server *server, const char *name, size_t namelen, __u32 *gid)
>  {
>  	struct idmap *idmap = server->nfs_client->cl_idmap;
> +	int ret = -EINVAL;
>  
> -	if (nfs_map_string_to_numeric(name, namelen, uid))
> +	if (nfs_map_string_to_numeric(name, namelen, gid))
>  		return 0;
> -	return nfs_idmap_id(idmap, &idmap->idmap_group_hash, name, namelen, uid);
> +	ret = nfs_idmap_lookup_id(name, namelen, "gid", gid);
> +	if (ret < 0)
> +		ret = nfs_idmap_id(idmap, &idmap->idmap_group_hash, name, namelen, gid);
> +	return ret;
>  }
>  
>  int nfs_map_uid_to_name(const struct nfs_server *server, __u32 uid, char *buf, size_t buflen)
> @@ -815,22 +785,26 @@ int nfs_map_uid_to_name(const struct nfs_server *server, __u32 uid, char *buf, s
>  	struct idmap *idmap = server->nfs_client->cl_idmap;
>  	int ret = -EINVAL;
>  
> -	if (!(server->caps & NFS_CAP_UIDGID_NOMAP))
> -		ret = nfs_idmap_name(idmap, &idmap->idmap_user_hash, uid, buf);
> +	if (!(server->caps & NFS_CAP_UIDGID_NOMAP)) {
> +		ret = nfs_idmap_lookup_name(uid, "user", buf, buflen);
> +		if (ret < 0)
> +			ret = nfs_idmap_name(idmap, &idmap->idmap_user_hash, uid, buf);
> +	}
>  	if (ret < 0)
>  		ret = nfs_map_numeric_to_string(uid, buf, buflen);
>  	return ret;
>  }
> -int nfs_map_gid_to_group(const struct nfs_server *server, __u32 uid, char *buf, size_t buflen)
> +int nfs_map_gid_to_group(const struct nfs_server *server, __u32 gid, char *buf, size_t buflen)
>  {
>  	struct idmap *idmap = server->nfs_client->cl_idmap;
>  	int ret = -EINVAL;
>  
> -	if (!(server->caps & NFS_CAP_UIDGID_NOMAP))
> -		ret = nfs_idmap_name(idmap, &idmap->idmap_group_hash, uid, buf);
> +	if (!(server->caps & NFS_CAP_UIDGID_NOMAP)) {
> +		ret = nfs_idmap_lookup_name(gid, "group", buf, buflen);
> +		if (ret < 0)
> +			ret = nfs_idmap_name(idmap, &idmap->idmap_group_hash, gid, buf);
> +	}
>  	if (ret < 0)
> -		ret = nfs_map_numeric_to_string(uid, buf, buflen);
> +		ret = nfs_map_numeric_to_string(gid, buf, buflen);
>  	return ret;
>  }
> -
> -#endif /* CONFIG_NFS_USE_NEW_IDMAPPER */
> diff --git a/fs/nfs/sysctl.c b/fs/nfs/sysctl.c
> index 978aaeb..ad4d2e7 100644
> --- a/fs/nfs/sysctl.c
> +++ b/fs/nfs/sysctl.c
> @@ -32,7 +32,6 @@ static ctl_table nfs_cb_sysctls[] = {
>  		.extra1 = (int *)&nfs_set_port_min,
>  		.extra2 = (int *)&nfs_set_port_max,
>  	},
> -#ifndef CONFIG_NFS_USE_NEW_IDMAPPER
>  	{
>  		.procname = "idmap_cache_timeout",
>  		.data = &nfs_idmap_cache_timeout,
> @@ -40,7 +39,6 @@ static ctl_table nfs_cb_sysctls[] = {
>  		.mode = 0644,
>  		.proc_handler = proc_dointvec_jiffies,
>  	},
> -#endif /* CONFIG_NFS_USE_NEW_IDMAPPER */
>  #endif
>  	{
>  		.procname	= "nfs_mountpoint_timeout",
> diff --git a/include/linux/nfs_idmap.h b/include/linux/nfs_idmap.h
> index 308c188..717fa50 100644
> --- a/include/linux/nfs_idmap.h
> +++ b/include/linux/nfs_idmap.h
> @@ -69,36 +69,11 @@ struct nfs_server;
>  struct nfs_fattr;
>  struct nfs4_string;
>  
> -#ifdef CONFIG_NFS_USE_NEW_IDMAPPER
> -
>  int nfs_idmap_init(void);
>  void nfs_idmap_quit(void);
> -
> -static inline int nfs_idmap_new(struct nfs_client *clp)
> -{
> -	return 0;
> -}
> -
> -static inline void nfs_idmap_delete(struct nfs_client *clp)
> -{
> -}
> -
> -#else /* CONFIG_NFS_USE_NEW_IDMAPPER not set */
> -
> -static inline int nfs_idmap_init(void)
> -{
> -	return 0;
> -}
> -
> -static inline void nfs_idmap_quit(void)
> -{
> -}
> -
>  int nfs_idmap_new(struct nfs_client *);
>  void nfs_idmap_delete(struct nfs_client *);
>  
> -#endif /* CONFIG_NFS_USE_NEW_IDMAPPER */
> -
>  void nfs_fattr_init_names(struct nfs_fattr *fattr,
>  		struct nfs4_string *owner_name,
>  		struct nfs4_string *group_name);
> -- 
> 1.7.8.4
> 

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

* Re: [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails
  2012-01-26 21:56 ` [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails J. Bruce Fields
@ 2012-01-26 21:57   ` Bryan Schumaker
  0 siblings, 0 replies; 17+ messages in thread
From: Bryan Schumaker @ 2012-01-26 21:57 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: linux-nfs

On Thu Jan 26 16:56:15 2012, J. Bruce Fields wrote:
> I'm assuming you meant to send this to Trond, not me?

Oops, that was supposed to go to Trond.  I have two scripts:  
git-submit-nfs and git-submit-nfsd.  My fingers must have been working 
faster than my brain...
>
> --b.
>
> On Thu, Jan 26, 2012 at 04:54:23PM -0500, bjschuma@netapp.com wrote:
>> From: Bryan Schumaker <bjschuma@netapp.com>
>>
>> This patch removes the CONFIG_NFS_USE_NEW_IDMAPPER compile option.
>> First, the idmapper will attempt to map the id using /sbin/request-key
>> and nfsidmap.  If this fails (if /etc/request-key.conf is not configured
>> properly) then the idmapper will call the legacy code to perform the
>> mapping.  I left a comment stating where the legacy code begins to make
>> it easier for somebody to remove in the future.
>>
>> Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
>> ---
>> I tested this on both Archlinux with nfs-utils 1.2.5 and on Fedora with
>> nfs-utils 1.2.6-rc6.  It did what was expected in both cases.
>>
>>  fs/nfs/Kconfig            |   11 ------
>>  fs/nfs/idmap.c            |   76 +++++++++++++++------------------------------
>>  fs/nfs/sysctl.c           |    2 -
>>  include/linux/nfs_idmap.h |   25 ---------------
>>  4 files changed, 25 insertions(+), 89 deletions(-)
>>
>> diff --git a/fs/nfs/Kconfig b/fs/nfs/Kconfig
>> index dbcd821..021d2cf 100644
>> --- a/fs/nfs/Kconfig
>> +++ b/fs/nfs/Kconfig
>> @@ -132,14 +132,3 @@ config NFS_USE_KERNEL_DNS
>>  	select DNS_RESOLVER
>>  	select KEYS
>>  	default y
>> -
>> -config NFS_USE_NEW_IDMAPPER
>> -	bool "Use the new idmapper upcall routine"
>> -	depends on NFS_V4 && KEYS
>> -	help
>> -	  Say Y here if you want NFS to use the new idmapper upcall functions.
>> -	  You will need /sbin/request-key (usually provided by the keyutils
>> -	  package).  For details, read
>> -	  <file:Documentation/filesystems/nfs/idmapper.txt>.
>> -
>> -	  If you are unsure, say N.
>> diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
>> index 2c05f19..56b1104 100644
>> --- a/fs/nfs/idmap.c
>> +++ b/fs/nfs/idmap.c
>> @@ -142,8 +142,6 @@ static int nfs_map_numeric_to_string(__u32 id, char *buf, size_t buflen)
>>  	return snprintf(buf, buflen, "%u", id);
>>  }
>>  
>> -#ifdef CONFIG_NFS_USE_NEW_IDMAPPER
>> -
>>  #include <linux/cred.h>
>>  #include <linux/sunrpc/sched.h>
>>  #include <linux/nfs4.h>
>> @@ -327,43 +325,7 @@ static int nfs_idmap_lookup_id(const char *name, size_t namelen,
>>  	return ret;
>>  }
>>  
>> -int nfs_map_name_to_uid(const struct nfs_server *server, const char *name, size_t namelen, __u32 *uid)
>> -{
>> -	if (nfs_map_string_to_numeric(name, namelen, uid))
>> -		return 0;
>> -	return nfs_idmap_lookup_id(name, namelen, "uid", uid);
>> -}
>> -
>> -int nfs_map_group_to_gid(const struct nfs_server *server, const char *name, size_t namelen, __u32 *gid)
>> -{
>> -	if (nfs_map_string_to_numeric(name, namelen, gid))
>> -		return 0;
>> -	return nfs_idmap_lookup_id(name, namelen, "gid", gid);
>> -}
>> -
>> -int nfs_map_uid_to_name(const struct nfs_server *server, __u32 uid, char *buf, size_t buflen)
>> -{
>> -	int ret = -EINVAL;
>> -
>> -	if (!(server->caps & NFS_CAP_UIDGID_NOMAP))
>> -		ret = nfs_idmap_lookup_name(uid, "user", buf, buflen);
>> -	if (ret < 0)
>> -		ret = nfs_map_numeric_to_string(uid, buf, buflen);
>> -	return ret;
>> -}
>> -int nfs_map_gid_to_group(const struct nfs_server *server, __u32 gid, char *buf, size_t buflen)
>> -{
>> -	int ret = -EINVAL;
>> -
>> -	if (!(server->caps & NFS_CAP_UIDGID_NOMAP))
>> -		ret = nfs_idmap_lookup_name(gid, "group", buf, buflen);
>> -	if (ret < 0)
>> -		ret = nfs_map_numeric_to_string(gid, buf, buflen);
>> -	return ret;
>> -}
>> -
>> -#else  /* CONFIG_NFS_USE_NEW_IDMAPPER not defined */
>> -
>> +/* idmap classic begins here */
>>  #include <linux/module.h>
>>  #include <linux/mutex.h>
>>  #include <linux/init.h>
>> @@ -795,19 +757,27 @@ static unsigned int fnvhash32(const void *buf, size_t buflen)
>>  int nfs_map_name_to_uid(const struct nfs_server *server, const char *name, size_t namelen, __u32 *uid)
>>  {
>>  	struct idmap *idmap = server->nfs_client->cl_idmap;
>> +	int ret = -EINVAL;
>>  
>>  	if (nfs_map_string_to_numeric(name, namelen, uid))
>>  		return 0;
>> -	return nfs_idmap_id(idmap, &idmap->idmap_user_hash, name, namelen, uid);
>> +	ret = nfs_idmap_lookup_id(name, namelen, "uid", uid);
>> +	if (ret < 0)
>> +		ret = nfs_idmap_id(idmap, &idmap->idmap_user_hash, name, namelen, uid);
>> +	return ret;
>>  }
>>  
>> -int nfs_map_group_to_gid(const struct nfs_server *server, const char *name, size_t namelen, __u32 *uid)
>> +int nfs_map_group_to_gid(const struct nfs_server *server, const char *name, size_t namelen, __u32 *gid)
>>  {
>>  	struct idmap *idmap = server->nfs_client->cl_idmap;
>> +	int ret = -EINVAL;
>>  
>> -	if (nfs_map_string_to_numeric(name, namelen, uid))
>> +	if (nfs_map_string_to_numeric(name, namelen, gid))
>>  		return 0;
>> -	return nfs_idmap_id(idmap, &idmap->idmap_group_hash, name, namelen, uid);
>> +	ret = nfs_idmap_lookup_id(name, namelen, "gid", gid);
>> +	if (ret < 0)
>> +		ret = nfs_idmap_id(idmap, &idmap->idmap_group_hash, name, namelen, gid);
>> +	return ret;
>>  }
>>  
>>  int nfs_map_uid_to_name(const struct nfs_server *server, __u32 uid, char *buf, size_t buflen)
>> @@ -815,22 +785,26 @@ int nfs_map_uid_to_name(const struct nfs_server *server, __u32 uid, char *buf, s
>>  	struct idmap *idmap = server->nfs_client->cl_idmap;
>>  	int ret = -EINVAL;
>>  
>> -	if (!(server->caps & NFS_CAP_UIDGID_NOMAP))
>> -		ret = nfs_idmap_name(idmap, &idmap->idmap_user_hash, uid, buf);
>> +	if (!(server->caps & NFS_CAP_UIDGID_NOMAP)) {
>> +		ret = nfs_idmap_lookup_name(uid, "user", buf, buflen);
>> +		if (ret < 0)
>> +			ret = nfs_idmap_name(idmap, &idmap->idmap_user_hash, uid, buf);
>> +	}
>>  	if (ret < 0)
>>  		ret = nfs_map_numeric_to_string(uid, buf, buflen);
>>  	return ret;
>>  }
>> -int nfs_map_gid_to_group(const struct nfs_server *server, __u32 uid, char *buf, size_t buflen)
>> +int nfs_map_gid_to_group(const struct nfs_server *server, __u32 gid, char *buf, size_t buflen)
>>  {
>>  	struct idmap *idmap = server->nfs_client->cl_idmap;
>>  	int ret = -EINVAL;
>>  
>> -	if (!(server->caps & NFS_CAP_UIDGID_NOMAP))
>> -		ret = nfs_idmap_name(idmap, &idmap->idmap_group_hash, uid, buf);
>> +	if (!(server->caps & NFS_CAP_UIDGID_NOMAP)) {
>> +		ret = nfs_idmap_lookup_name(gid, "group", buf, buflen);
>> +		if (ret < 0)
>> +			ret = nfs_idmap_name(idmap, &idmap->idmap_group_hash, gid, buf);
>> +	}
>>  	if (ret < 0)
>> -		ret = nfs_map_numeric_to_string(uid, buf, buflen);
>> +		ret = nfs_map_numeric_to_string(gid, buf, buflen);
>>  	return ret;
>>  }
>> -
>> -#endif /* CONFIG_NFS_USE_NEW_IDMAPPER */
>> diff --git a/fs/nfs/sysctl.c b/fs/nfs/sysctl.c
>> index 978aaeb..ad4d2e7 100644
>> --- a/fs/nfs/sysctl.c
>> +++ b/fs/nfs/sysctl.c
>> @@ -32,7 +32,6 @@ static ctl_table nfs_cb_sysctls[] = {
>>  		.extra1 = (int *)&nfs_set_port_min,
>>  		.extra2 = (int *)&nfs_set_port_max,
>>  	},
>> -#ifndef CONFIG_NFS_USE_NEW_IDMAPPER
>>  	{
>>  		.procname = "idmap_cache_timeout",
>>  		.data = &nfs_idmap_cache_timeout,
>> @@ -40,7 +39,6 @@ static ctl_table nfs_cb_sysctls[] = {
>>  		.mode = 0644,
>>  		.proc_handler = proc_dointvec_jiffies,
>>  	},
>> -#endif /* CONFIG_NFS_USE_NEW_IDMAPPER */
>>  #endif
>>  	{
>>  		.procname	= "nfs_mountpoint_timeout",
>> diff --git a/include/linux/nfs_idmap.h b/include/linux/nfs_idmap.h
>> index 308c188..717fa50 100644
>> --- a/include/linux/nfs_idmap.h
>> +++ b/include/linux/nfs_idmap.h
>> @@ -69,36 +69,11 @@ struct nfs_server;
>>  struct nfs_fattr;
>>  struct nfs4_string;
>>  
>> -#ifdef CONFIG_NFS_USE_NEW_IDMAPPER
>> -
>>  int nfs_idmap_init(void);
>>  void nfs_idmap_quit(void);
>> -
>> -static inline int nfs_idmap_new(struct nfs_client *clp)
>> -{
>> -	return 0;
>> -}
>> -
>> -static inline void nfs_idmap_delete(struct nfs_client *clp)
>> -{
>> -}
>> -
>> -#else /* CONFIG_NFS_USE_NEW_IDMAPPER not set */
>> -
>> -static inline int nfs_idmap_init(void)
>> -{
>> -	return 0;
>> -}
>> -
>> -static inline void nfs_idmap_quit(void)
>> -{
>> -}
>> -
>>  int nfs_idmap_new(struct nfs_client *);
>>  void nfs_idmap_delete(struct nfs_client *);
>>  
>> -#endif /* CONFIG_NFS_USE_NEW_IDMAPPER */
>> -
>>  void nfs_fattr_init_names(struct nfs_fattr *fattr,
>>  		struct nfs4_string *owner_name,
>>  		struct nfs4_string *group_name);
>> -- 
>> 1.7.8.4
>>



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

* Re: [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails
  2012-01-26 21:54 [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails bjschuma
                   ` (2 preceding siblings ...)
  2012-01-26 21:56 ` [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails J. Bruce Fields
@ 2012-02-06 23:05 ` Myklebust, Trond
  2012-02-07 15:54   ` Bryan Schumaker
  2012-02-07 19:12 ` Jeff Layton
  4 siblings, 1 reply; 17+ messages in thread
From: Myklebust, Trond @ 2012-02-06 23:05 UTC (permalink / raw)
  To: Schumaker, Bryan; +Cc: bfields, linux-nfs

T24gVGh1LCAyMDEyLTAxLTI2IGF0IDE2OjU0IC0wNTAwLCBianNjaHVtYUBuZXRhcHAuY29tIHdy
b3RlOg0KPiBGcm9tOiBCcnlhbiBTY2h1bWFrZXIgPGJqc2NodW1hQG5ldGFwcC5jb20+DQo+IA0K
PiBUaGlzIHBhdGNoIHJlbW92ZXMgdGhlIENPTkZJR19ORlNfVVNFX05FV19JRE1BUFBFUiBjb21w
aWxlIG9wdGlvbi4NCj4gRmlyc3QsIHRoZSBpZG1hcHBlciB3aWxsIGF0dGVtcHQgdG8gbWFwIHRo
ZSBpZCB1c2luZyAvc2Jpbi9yZXF1ZXN0LWtleQ0KPiBhbmQgbmZzaWRtYXAuICBJZiB0aGlzIGZh
aWxzIChpZiAvZXRjL3JlcXVlc3Qta2V5LmNvbmYgaXMgbm90IGNvbmZpZ3VyZWQNCj4gcHJvcGVy
bHkpIHRoZW4gdGhlIGlkbWFwcGVyIHdpbGwgY2FsbCB0aGUgbGVnYWN5IGNvZGUgdG8gcGVyZm9y
bSB0aGUNCj4gbWFwcGluZy4gIEkgbGVmdCBhIGNvbW1lbnQgc3RhdGluZyB3aGVyZSB0aGUgbGVn
YWN5IGNvZGUgYmVnaW5zIHRvIG1ha2UNCj4gaXQgZWFzaWVyIGZvciBzb21lYm9keSB0byByZW1v
dmUgaW4gdGhlIGZ1dHVyZS4NCj4gDQo+IFNpZ25lZC1vZmYtYnk6IEJyeWFuIFNjaHVtYWtlciA8
YmpzY2h1bWFAbmV0YXBwLmNvbT4NCj4gLS0tDQo+IEkgdGVzdGVkIHRoaXMgb24gYm90aCBBcmNo
bGludXggd2l0aCBuZnMtdXRpbHMgMS4yLjUgYW5kIG9uIEZlZG9yYSB3aXRoDQo+IG5mcy11dGls
cyAxLjIuNi1yYzYuICBJdCBkaWQgd2hhdCB3YXMgZXhwZWN0ZWQgaW4gYm90aCBjYXNlcy4NCj4g
DQoNCkl0IGRpZG4ndCBhcHBseSBvbiB0b3Agb2YgdGhlIG5hbWVzcGFjZSBjaGFuZ2VzLiBDYW4g
eW91IHBsZWFzZSBjaGVjayBpZg0KbXkgYXR0ZW1wdCBhdCBmaXhpbmcgaXQgdXAgaXMgY29ycmVj
dD8gWW91J2xsIGZpbmQgaXQgaW4gdGhlICdkZXZlbCcNCmJyYW5jaCBvZiBteSBnaXQgcmVwb3Np
dG9yeS4NCg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFp
bmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29t
DQoNCg==

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

* Re: [PATCH 2/3] NFS: Keep idmapper include files in one place
  2012-01-26 21:54 ` [PATCH 2/3] NFS: Keep idmapper include files in one place bjschuma
@ 2012-02-06 23:05   ` Myklebust, Trond
  0 siblings, 0 replies; 17+ messages in thread
From: Myklebust, Trond @ 2012-02-06 23:05 UTC (permalink / raw)
  To: Schumaker, Bryan; +Cc: bfields, linux-nfs

T24gVGh1LCAyMDEyLTAxLTI2IGF0IDE2OjU0IC0wNTAwLCBianNjaHVtYUBuZXRhcHAuY29tIHdy
b3RlOg0KPiBGcm9tOiBCcnlhbiBTY2h1bWFrZXIgPGJqc2NodW1hQG5ldGFwcC5jb20+DQo+IA0K
PiBTaWduZWQtb2ZmLWJ5OiBCcnlhbiBTY2h1bWFrZXIgPGJqc2NodW1hQG5ldGFwcC5jb20+DQo+
IC0tLQ0KPiAgZnMvbmZzL2lkbWFwLmMgfCAgIDY0ICsrKysrKysrKysrKysrKysrKysrKysrKysr
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0NCj4gIDEgZmlsZXMgY2hhbmdlZCwgMzAgaW5z
ZXJ0aW9ucygrKSwgMzQgZGVsZXRpb25zKC0pDQoNClNhbWUgcHJvYmxlbS4uLiBQbGVhc2UgY2hl
Y2sgdGhlIGZpeGVkIHVwIHZlcnNpb24gaW4gdGhlICdkZXZlbCcgYnJhbmNoLg0KDQotLSANClRy
b25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJv
bmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQoNCg==

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

* Re: [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails
  2012-02-06 23:05 ` Myklebust, Trond
@ 2012-02-07 15:54   ` Bryan Schumaker
  0 siblings, 0 replies; 17+ messages in thread
From: Bryan Schumaker @ 2012-02-07 15:54 UTC (permalink / raw)
  To: Myklebust, Trond; +Cc: Schumaker, Bryan, bfields, linux-nfs

On 02/06/12 18:05, Myklebust, Trond wrote:

> On Thu, 2012-01-26 at 16:54 -0500, bjschuma@netapp.com wrote:
>> From: Bryan Schumaker <bjschuma@netapp.com>
>>
>> This patch removes the CONFIG_NFS_USE_NEW_IDMAPPER compile option.
>> First, the idmapper will attempt to map the id using /sbin/request-key
>> and nfsidmap.  If this fails (if /etc/request-key.conf is not configured
>> properly) then the idmapper will call the legacy code to perform the
>> mapping.  I left a comment stating where the legacy code begins to make
>> it easier for somebody to remove in the future.
>>
>> Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
>> ---
>> I tested this on both Archlinux with nfs-utils 1.2.5 and on Fedora with
>> nfs-utils 1.2.6-rc6.  It did what was expected in both cases.
>>
> 
> It didn't apply on top of the namespace changes. Can you please check if
> my attempt at fixing it up is correct? You'll find it in the 'devel'
> branch of my git repository.


Looks like both patches still work.

- Bryan

> 
> 



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

* Re: [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails
  2012-01-26 21:54 [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails bjschuma
                   ` (3 preceding siblings ...)
  2012-02-06 23:05 ` Myklebust, Trond
@ 2012-02-07 19:12 ` Jeff Layton
  2012-02-07 19:21   ` Myklebust, Trond
  4 siblings, 1 reply; 17+ messages in thread
From: Jeff Layton @ 2012-02-07 19:12 UTC (permalink / raw)
  To: bjschuma; +Cc: bfields, linux-nfs

On Thu, 26 Jan 2012 16:54:23 -0500
bjschuma@netapp.com wrote:

> From: Bryan Schumaker <bjschuma@netapp.com>
> 
> This patch removes the CONFIG_NFS_USE_NEW_IDMAPPER compile option.
> First, the idmapper will attempt to map the id using /sbin/request-key
> and nfsidmap.  If this fails (if /etc/request-key.conf is not configured
> properly) then the idmapper will call the legacy code to perform the
> mapping.  I left a comment stating where the legacy code begins to make
> it easier for somebody to remove in the future.
> 
> Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
> ---


I think this patch is the right approach and will make the transition
easier. But there is still a bit of a problem here...

One of the main complaints about the old idmapper was that it did a
rather large allocation on every time a new nfs_client is created:

-----------------------[snip]----------------------

int
nfs_idmap_new(struct nfs_client *clp)
{
        struct idmap *idmap;
        struct rpc_pipe *pipe;
        int error;

        BUG_ON(clp->cl_idmap != NULL);

        idmap = kzalloc(sizeof(*idmap), GFP_KERNEL);
        if (idmap == NULL)
                return -ENOMEM;

-----------------------[snip]----------------------

On a 32-bit box when you try to mount and low memory is heavily
fragmented, you can get a NULL pointer back on that kzalloc with a
nice stack trace headed by a message like this:

    mount.nfs: page allocation failure. order:4, mode:0xc0d0

Here's a RHBZ against RHEL6 if you're interested in gory details:

    https://bugzilla.redhat.com/show_bug.cgi?id=730045

In any case, this problem was one of the reasons for pushing the new
idmapper. A number of people have complained about this problem in the
past and we told them "use the new idmapper". Now, with this patchset,
that won't help.

I think the right solution is to probably look at breaking up the idmap
structure in the legacy idmapper into multiple allocations. It's more
complicated to deal with and will mean restructuring the code a bit,
but it will allow for a relatively graceful transition to the new
idmapper.

Thoughts?

-- 
Jeff Layton <jlayton@redhat.com>

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

* Re: [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails
  2012-02-07 19:12 ` Jeff Layton
@ 2012-02-07 19:21   ` Myklebust, Trond
  2012-02-07 19:29     ` Bryan Schumaker
  0 siblings, 1 reply; 17+ messages in thread
From: Myklebust, Trond @ 2012-02-07 19:21 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Schumaker, Bryan, bfields, linux-nfs

T24gVHVlLCAyMDEyLTAyLTA3IGF0IDE0OjEyIC0wNTAwLCBKZWZmIExheXRvbiB3cm90ZToNCj4g
T24gYSAzMi1iaXQgYm94IHdoZW4geW91IHRyeSB0byBtb3VudCBhbmQgbG93IG1lbW9yeSBpcyBo
ZWF2aWx5DQo+IGZyYWdtZW50ZWQsIHlvdSBjYW4gZ2V0IGEgTlVMTCBwb2ludGVyIGJhY2sgb24g
dGhhdCBremFsbG9jIHdpdGggYQ0KPiBuaWNlIHN0YWNrIHRyYWNlIGhlYWRlZCBieSBhIG1lc3Nh
Z2UgbGlrZSB0aGlzOg0KPiANCj4gICAgIG1vdW50Lm5mczogcGFnZSBhbGxvY2F0aW9uIGZhaWx1
cmUuIG9yZGVyOjQsIG1vZGU6MHhjMGQwDQo+IA0KPiBIZXJlJ3MgYSBSSEJaIGFnYWluc3QgUkhF
TDYgaWYgeW91J3JlIGludGVyZXN0ZWQgaW4gZ29yeSBkZXRhaWxzOg0KPiANCj4gICAgIGh0dHBz
Oi8vYnVnemlsbGEucmVkaGF0LmNvbS9zaG93X2J1Zy5jZ2k/aWQ9NzMwMDQ1DQo+IA0KPiBJbiBh
bnkgY2FzZSwgdGhpcyBwcm9ibGVtIHdhcyBvbmUgb2YgdGhlIHJlYXNvbnMgZm9yIHB1c2hpbmcg
dGhlIG5ldw0KPiBpZG1hcHBlci4gQSBudW1iZXIgb2YgcGVvcGxlIGhhdmUgY29tcGxhaW5lZCBh
Ym91dCB0aGlzIHByb2JsZW0gaW4gdGhlDQo+IHBhc3QgYW5kIHdlIHRvbGQgdGhlbSAidXNlIHRo
ZSBuZXcgaWRtYXBwZXIiLiBOb3csIHdpdGggdGhpcyBwYXRjaHNldCwNCj4gdGhhdCB3b24ndCBo
ZWxwLg0KDQpXYWl0LiBIb3cgaXMgdGhhdCB0cnVlPyBUaGUgd2hvbGUgcG9pbnQgb2YgdGhpcyBw
YXRjaHNldCBpcyB0aGF0IGl0DQphbGxvd3MgeW91IHRvIGNvbXBpbGUgaW4gc3VwcG9ydCBmb3Ig
X2JvdGhfIGlkbWFwcGVycywgd2l0aCB0aGUgbmV3DQprZXlyaW5nLWJhc2VkIGlkbWFwcGVyIGJl
aW5nIHRyaWVkIGZpcnN0LiBUaGUgY2xpZW50IHRoZW4gZmFsbHMgYmFjayB0bw0KdXNpbmcgdGhl
IG9sZCBpZG1hcHBlciBpZiBhbmQgb25seSBpZiB0aGUgdXNlciBoYXMgZmFpbGVkIHRvIHNldCB1
cCB0aGUNCm5ldyBpZG1hcHBlciBjb3JyZWN0bHkuDQoNCj4gSSB0aGluayB0aGUgcmlnaHQgc29s
dXRpb24gaXMgdG8gcHJvYmFibHkgbG9vayBhdCBicmVha2luZyB1cCB0aGUgaWRtYXANCj4gc3Ry
dWN0dXJlIGluIHRoZSBsZWdhY3kgaWRtYXBwZXIgaW50byBtdWx0aXBsZSBhbGxvY2F0aW9ucy4g
SXQncyBtb3JlDQo+IGNvbXBsaWNhdGVkIHRvIGRlYWwgd2l0aCBhbmQgd2lsbCBtZWFuIHJlc3Ry
dWN0dXJpbmcgdGhlIGNvZGUgYSBiaXQsDQo+IGJ1dCBpdCB3aWxsIGFsbG93IGZvciBhIHJlbGF0
aXZlbHkgZ3JhY2VmdWwgdHJhbnNpdGlvbiB0byB0aGUgbmV3DQo+IGlkbWFwcGVyLg0KDQpJJ2Qg
bGlrZSB0byBzZWUgdGhlIG9sZCBpZG1hcHBlciBjb2RlIGNoYW5nZWQgdG8gdXNlIHRoZSBuZXcN
CmtleXJpbmctYmFzZWQgY2FjaGUgaW5zdGVhZC4gSSd2ZSBhc2tlZCBCcnlhbiB0byBsb29rIGlu
dG8gaG93IHdlIGNhbiBkbw0KdGhpcy4NCg0KDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51
eCBORlMgY2xpZW50IG1haW50YWluZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RAbmV0YXBw
LmNvbQ0Kd3d3Lm5ldGFwcC5jb20NCg0K

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

* Re: [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails
  2012-02-07 19:21   ` Myklebust, Trond
@ 2012-02-07 19:29     ` Bryan Schumaker
  2012-02-07 19:44       ` Myklebust, Trond
  0 siblings, 1 reply; 17+ messages in thread
From: Bryan Schumaker @ 2012-02-07 19:29 UTC (permalink / raw)
  To: Myklebust, Trond; +Cc: Jeff Layton, Schumaker, Bryan, bfields, linux-nfs

On 02/07/12 14:21, Myklebust, Trond wrote:

> On Tue, 2012-02-07 at 14:12 -0500, Jeff Layton wrote:
>> On a 32-bit box when you try to mount and low memory is heavily
>> fragmented, you can get a NULL pointer back on that kzalloc with a
>> nice stack trace headed by a message like this:
>>
>>     mount.nfs: page allocation failure. order:4, mode:0xc0d0
>>
>> Here's a RHBZ against RHEL6 if you're interested in gory details:
>>
>>     https://bugzilla.redhat.com/show_bug.cgi?id=730045
>>
>> In any case, this problem was one of the reasons for pushing the new
>> idmapper. A number of people have complained about this problem in the
>> past and we told them "use the new idmapper". Now, with this patchset,
>> that won't help.
> 
> Wait. How is that true? The whole point of this patchset is that it
> allows you to compile in support for _both_ idmappers, with the new
> keyring-based idmapper being tried first. The client then falls back to
> using the old idmapper if and only if the user has failed to set up the
> new idmapper correctly.


Because it still allocates the structures, they just go unused if the new idmapper works.  This seems kind of wasteful now that I know about it...

> 
>> I think the right solution is to probably look at breaking up the idmap
>> structure in the legacy idmapper into multiple allocations. It's more
>> complicated to deal with and will mean restructuring the code a bit,
>> but it will allow for a relatively graceful transition to the new
>> idmapper.
> 
> I'd like to see the old idmapper code changed to use the new
> keyring-based cache instead. I've asked Bryan to look into how we can do
> this.


This will probably be easier than splitting up the allocation.  It should be possible to change whatever "actor" function the request_key code takes and then try again.  What is the best way to handle the fallback?  This patch always tries the new idmapper before falling back on the old one.  I could also set it up to try the new idmapper once, change the function pointer if it fails, and never change it back (until NFS is restarted).

- Bryan

> 
> 
> 



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

* Re: [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails
  2012-02-07 19:29     ` Bryan Schumaker
@ 2012-02-07 19:44       ` Myklebust, Trond
  2012-02-07 20:05         ` Myklebust, Trond
  0 siblings, 1 reply; 17+ messages in thread
From: Myklebust, Trond @ 2012-02-07 19:44 UTC (permalink / raw)
  To: Schumaker, Bryan; +Cc: Jeff Layton, Schumaker, Bryan, bfields, linux-nfs

T24gVHVlLCAyMDEyLTAyLTA3IGF0IDE0OjI5IC0wNTAwLCBCcnlhbiBTY2h1bWFrZXIgd3JvdGU6
DQo+IE9uIDAyLzA3LzEyIDE0OjIxLCBNeWtsZWJ1c3QsIFRyb25kIHdyb3RlOg0KPiANCj4gPiBP
biBUdWUsIDIwMTItMDItMDcgYXQgMTQ6MTIgLTA1MDAsIEplZmYgTGF5dG9uIHdyb3RlOg0KPiA+
PiBPbiBhIDMyLWJpdCBib3ggd2hlbiB5b3UgdHJ5IHRvIG1vdW50IGFuZCBsb3cgbWVtb3J5IGlz
IGhlYXZpbHkNCj4gPj4gZnJhZ21lbnRlZCwgeW91IGNhbiBnZXQgYSBOVUxMIHBvaW50ZXIgYmFj
ayBvbiB0aGF0IGt6YWxsb2Mgd2l0aCBhDQo+ID4+IG5pY2Ugc3RhY2sgdHJhY2UgaGVhZGVkIGJ5
IGEgbWVzc2FnZSBsaWtlIHRoaXM6DQo+ID4+DQo+ID4+ICAgICBtb3VudC5uZnM6IHBhZ2UgYWxs
b2NhdGlvbiBmYWlsdXJlLiBvcmRlcjo0LCBtb2RlOjB4YzBkMA0KPiA+Pg0KPiA+PiBIZXJlJ3Mg
YSBSSEJaIGFnYWluc3QgUkhFTDYgaWYgeW91J3JlIGludGVyZXN0ZWQgaW4gZ29yeSBkZXRhaWxz
Og0KPiA+Pg0KPiA+PiAgICAgaHR0cHM6Ly9idWd6aWxsYS5yZWRoYXQuY29tL3Nob3dfYnVnLmNn
aT9pZD03MzAwNDUNCj4gPj4NCj4gPj4gSW4gYW55IGNhc2UsIHRoaXMgcHJvYmxlbSB3YXMgb25l
IG9mIHRoZSByZWFzb25zIGZvciBwdXNoaW5nIHRoZSBuZXcNCj4gPj4gaWRtYXBwZXIuIEEgbnVt
YmVyIG9mIHBlb3BsZSBoYXZlIGNvbXBsYWluZWQgYWJvdXQgdGhpcyBwcm9ibGVtIGluIHRoZQ0K
PiA+PiBwYXN0IGFuZCB3ZSB0b2xkIHRoZW0gInVzZSB0aGUgbmV3IGlkbWFwcGVyIi4gTm93LCB3
aXRoIHRoaXMgcGF0Y2hzZXQsDQo+ID4+IHRoYXQgd29uJ3QgaGVscC4NCj4gPiANCj4gPiBXYWl0
LiBIb3cgaXMgdGhhdCB0cnVlPyBUaGUgd2hvbGUgcG9pbnQgb2YgdGhpcyBwYXRjaHNldCBpcyB0
aGF0IGl0DQo+ID4gYWxsb3dzIHlvdSB0byBjb21waWxlIGluIHN1cHBvcnQgZm9yIF9ib3RoXyBp
ZG1hcHBlcnMsIHdpdGggdGhlIG5ldw0KPiA+IGtleXJpbmctYmFzZWQgaWRtYXBwZXIgYmVpbmcg
dHJpZWQgZmlyc3QuIFRoZSBjbGllbnQgdGhlbiBmYWxscyBiYWNrIHRvDQo+ID4gdXNpbmcgdGhl
IG9sZCBpZG1hcHBlciBpZiBhbmQgb25seSBpZiB0aGUgdXNlciBoYXMgZmFpbGVkIHRvIHNldCB1
cCB0aGUNCj4gPiBuZXcgaWRtYXBwZXIgY29ycmVjdGx5Lg0KPiANCj4gDQo+IEJlY2F1c2UgaXQg
c3RpbGwgYWxsb2NhdGVzIHRoZSBzdHJ1Y3R1cmVzLCB0aGV5IGp1c3QgZ28gdW51c2VkIGlmIHRo
ZSBuZXcgaWRtYXBwZXIgd29ya3MuICBUaGlzIHNlZW1zIGtpbmQgb2Ygd2FzdGVmdWwgbm93IHRo
YXQgSSBrbm93IGFib3V0IGl0Li4uDQoNCk9uZSB3YXkgdG8gZWFzaWx5IHNocmluayB0aGUgc2l6
ZSBvZiB0aGF0IGFsbG9jYXRpb24gaXMgdG8gY29udmVydCB0aGUNCmloX25hbWUgc3RyaW5nIGlu
dG8gYSBwb2ludGVyLCBhbmQgaGF2ZSB0aGUgZG93bmNhbGwgYWxsb2NhdGUgdGhlDQpzdG9yYWdl
IGZvciB0aGF0IHN0cmluZyBkeW5hbWljYWxseS4uLg0KDQoNCg0KLS0gDQpUcm9uZCBNeWtsZWJ1
c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVz
dEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNvbQ0KDQo=

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

* Re: [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails
  2012-02-07 19:44       ` Myklebust, Trond
@ 2012-02-07 20:05         ` Myklebust, Trond
  2012-02-08 12:04           ` Jeff Layton
  0 siblings, 1 reply; 17+ messages in thread
From: Myklebust, Trond @ 2012-02-07 20:05 UTC (permalink / raw)
  To: Schumaker, Bryan; +Cc: Jeff Layton, Schumaker, Bryan, bfields, linux-nfs

T24gVHVlLCAyMDEyLTAyLTA3IGF0IDE0OjQ0IC0wNTAwLCBUcm9uZCBNeWtsZWJ1c3Qgd3JvdGU6
DQoNCj4gT25lIHdheSB0byBlYXNpbHkgc2hyaW5rIHRoZSBzaXplIG9mIHRoYXQgYWxsb2NhdGlv
biBpcyB0byBjb252ZXJ0IHRoZQ0KPiBpaF9uYW1lIHN0cmluZyBpbnRvIGEgcG9pbnRlciwgYW5k
IGhhdmUgdGhlIGRvd25jYWxsIGFsbG9jYXRlIHRoZQ0KPiBzdG9yYWdlIGZvciB0aGF0IHN0cmlu
ZyBkeW5hbWljYWxseS4uLg0KDQpMaWtlIHNvLi4uLg0KDQo4PC0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tDQpGcm9t
IDUyMjFjODQxNTk1NzYwNDUwN2NkNzQwOGE2OTUyNDEwOTZiY2IzYWQgTW9uIFNlcCAxNyAwMDow
MDowMCAyMDAxDQpGcm9tOiBUcm9uZCBNeWtsZWJ1c3QgPFRyb25kLk15a2xlYnVzdEBuZXRhcHAu
Y29tPg0KRGF0ZTogVHVlLCA3IEZlYiAyMDEyIDE0OjU5OjA1IC0wNTAwDQpTdWJqZWN0OiBbUEFU
Q0hdIE5GU3Y0OiBSZWR1Y2UgdGhlIGZvb3RwcmludCBvZiB0aGUgaWRtYXBwZXINCg0KSW5zdGVh
ZCBvZiBwcmUtYWxsb2NhdGluZyB0aGUgc3RvcmFnZSBmb3IgYWxsIHRoZSBzdHJpbmdzLCB3ZSBj
YW4NCnNpZ25pZmljYW50bHkgcmVkdWNlIHRoZSBzaXplIG9mIHRoYXQgdGFibGUgYnkgZG9pbmcg
dGhlIGFsbG9jYXRpb24NCndoZW4gd2UgZG8gdGhlIGRvd25jYWxsLg0KDQpTaWduZWQtb2ZmLWJ5
OiBUcm9uZCBNeWtsZWJ1c3QgPFRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tPg0KLS0tDQogZnMv
bmZzL2lkbWFwLmMgfCAgIDE2ICsrKysrKysrKysrKystLS0NCiAxIGZpbGVzIGNoYW5nZWQsIDEz
IGluc2VydGlvbnMoKyksIDMgZGVsZXRpb25zKC0pDQoNCmRpZmYgLS1naXQgYS9mcy9uZnMvaWRt
YXAuYyBiL2ZzL25mcy9pZG1hcC5jDQppbmRleCA1YTU1NjZmLi5mZmY3OTQ4IDEwMDY0NA0KLS0t
IGEvZnMvbmZzL2lkbWFwLmMNCisrKyBiL2ZzL25mcy9pZG1hcC5jDQpAQCAtMzYyLDcgKzM2Miw3
IEBAIHN0cnVjdCBpZG1hcF9oYXNoZW50IHsNCiAJdW5zaWduZWQgbG9uZwkJaWhfZXhwaXJlczsN
CiAJX191MzIJCQlpaF9pZDsNCiAJc2l6ZV90CQkJaWhfbmFtZWxlbjsNCi0JY2hhcgkJCWloX25h
bWVbSURNQVBfTkFNRVNaXTsNCisJY29uc3QgY2hhcgkJKmloX25hbWU7DQogfTsNCiANCiBzdHJ1
Y3QgaWRtYXBfaGFzaHRhYmxlIHsNCkBAIC00ODIsMTIgKzQ4MiwxNyBAQCB2b2lkDQogbmZzX2lk
bWFwX2RlbGV0ZShzdHJ1Y3QgbmZzX2NsaWVudCAqY2xwKQ0KIHsNCiAJc3RydWN0IGlkbWFwICpp
ZG1hcCA9IGNscC0+Y2xfaWRtYXA7DQorCWludCBpOw0KIA0KIAlpZiAoIWlkbWFwKQ0KIAkJcmV0
dXJuOw0KIAluZnNfaWRtYXBfdW5yZWdpc3RlcihjbHAsIGlkbWFwLT5pZG1hcF9waXBlKTsNCiAJ
cnBjX2Rlc3Ryb3lfcGlwZV9kYXRhKGlkbWFwLT5pZG1hcF9waXBlKTsNCiAJY2xwLT5jbF9pZG1h
cCA9IE5VTEw7DQorCWZvciAoaSA9IDA7IGkgPCBBUlJBWV9TSVpFKGlkbWFwLT5pZG1hcF91c2Vy
X2hhc2guaF9lbnRyaWVzKTsgaSsrKQ0KKwkJa2ZyZWUoaWRtYXAtPmlkbWFwX3VzZXJfaGFzaC5o
X2VudHJpZXNbaV0uaWhfbmFtZSk7DQorCWZvciAoaSA9IDA7IGkgPCBBUlJBWV9TSVpFKGlkbWFw
LT5pZG1hcF9ncm91cF9oYXNoLmhfZW50cmllcyk7IGkrKykNCisJCWtmcmVlKGlkbWFwLT5pZG1h
cF9ncm91cF9oYXNoLmhfZW50cmllc1tpXS5paF9uYW1lKTsNCiAJa2ZyZWUoaWRtYXApOw0KIH0N
CiANCkBAIC02MzQsOSArNjM5LDE0IEBAIHN0YXRpYyB2b2lkDQogaWRtYXBfdXBkYXRlX2VudHJ5
KHN0cnVjdCBpZG1hcF9oYXNoZW50ICpoZSwgY29uc3QgY2hhciAqbmFtZSwNCiAJCXNpemVfdCBu
YW1lbGVuLCBfX3UzMiBpZCkNCiB7DQorCWNoYXIgKnN0ciA9IGttYWxsb2MobmFtZWxlbiArIDEs
IEdGUF9LRVJORUwpOw0KKwlpZiAoc3RyID09IE5VTEwpDQorCQlyZXR1cm47DQorCWtmcmVlKGhl
LT5paF9uYW1lKTsNCiAJaGUtPmloX2lkID0gaWQ7DQotCW1lbWNweShoZS0+aWhfbmFtZSwgbmFt
ZSwgbmFtZWxlbik7DQotCWhlLT5paF9uYW1lW25hbWVsZW5dID0gJ1wwJzsNCisJbWVtY3B5KHN0
ciwgbmFtZSwgbmFtZWxlbik7DQorCXN0cltuYW1lbGVuXSA9ICdcMCc7DQorCWhlLT5paF9uYW1l
ID0gc3RyOw0KIAloZS0+aWhfbmFtZWxlbiA9IG5hbWVsZW47DQogCWhlLT5paF9leHBpcmVzID0g
amlmZmllcyArIG5mc19pZG1hcF9jYWNoZV90aW1lb3V0Ow0KIH0NCi0tIA0KMS43LjcuNg0KDQoN
Cg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lcg0KDQpO
ZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNvbQ0KDQo=

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

* Re: [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails
  2012-02-07 20:05         ` Myklebust, Trond
@ 2012-02-08 12:04           ` Jeff Layton
  2012-02-08 13:50             ` Myklebust, Trond
  0 siblings, 1 reply; 17+ messages in thread
From: Jeff Layton @ 2012-02-08 12:04 UTC (permalink / raw)
  To: Myklebust, Trond; +Cc: Schumaker, Bryan, bfields, linux-nfs

On Tue, 7 Feb 2012 20:05:55 +0000
"Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:

> On Tue, 2012-02-07 at 14:44 -0500, Trond Myklebust wrote:
> 
> > One way to easily shrink the size of that allocation is to convert the
> > ih_name string into a pointer, and have the downcall allocate the
> > storage for that string dynamically...
> 
> Like so....
> 

Nice. I had started looking at this, but was still sorting my way
through how all of the hashtable cache code works.

One comment inline below:

> 8<-----------------------------------------------------------------------
> From 5221c8415957604507cd7408a695241096bcb3ad Mon Sep 17 00:00:00 2001
> From: Trond Myklebust <Trond.Myklebust@netapp.com>
> Date: Tue, 7 Feb 2012 14:59:05 -0500
> Subject: [PATCH] NFSv4: Reduce the footprint of the idmapper
> 
> Instead of pre-allocating the storage for all the strings, we can
> significantly reduce the size of that table by doing the allocation
> when we do the downcall.
> 
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
>  fs/nfs/idmap.c |   16 +++++++++++++---
>  1 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
> index 5a5566f..fff7948 100644
> --- a/fs/nfs/idmap.c
> +++ b/fs/nfs/idmap.c
> @@ -362,7 +362,7 @@ struct idmap_hashent {
>  	unsigned long		ih_expires;
>  	__u32			ih_id;
>  	size_t			ih_namelen;
> -	char			ih_name[IDMAP_NAMESZ];
> +	const char		*ih_name;
>  };
>  
>  struct idmap_hashtable {
> @@ -482,12 +482,17 @@ void
>  nfs_idmap_delete(struct nfs_client *clp)
>  {
>  	struct idmap *idmap = clp->cl_idmap;
> +	int i;
>  
>  	if (!idmap)
>  		return;
>  	nfs_idmap_unregister(clp, idmap->idmap_pipe);
>  	rpc_destroy_pipe_data(idmap->idmap_pipe);
>  	clp->cl_idmap = NULL;
> +	for (i = 0; i < ARRAY_SIZE(idmap->idmap_user_hash.h_entries); i++)
> +		kfree(idmap->idmap_user_hash.h_entries[i].ih_name);
> +	for (i = 0; i < ARRAY_SIZE(idmap->idmap_group_hash.h_entries); i++)
> +		kfree(idmap->idmap_group_hash.h_entries[i].ih_name);
>  	kfree(idmap);
>  }
>  
> @@ -634,9 +639,14 @@ static void
>  idmap_update_entry(struct idmap_hashent *he, const char *name,
>  		size_t namelen, __u32 id)
>  {
> +	char *str = kmalloc(namelen + 1, GFP_KERNEL);

Do we need to worry about recursing into direct reclaim here?

> +	if (str == NULL)
> +		return;
> +	kfree(he->ih_name);
>  	he->ih_id = id;
> -	memcpy(he->ih_name, name, namelen);
> -	he->ih_name[namelen] = '\0';
> +	memcpy(str, name, namelen);
> +	str[namelen] = '\0';
> +	he->ih_name = str;
>  	he->ih_namelen = namelen;
>  	he->ih_expires = jiffies + nfs_idmap_cache_timeout;
>  }

...and the namelen check in idmap_lookup_name should keep the memcmp
from tripping over these NULL pointers. I think this looks like a nice
interim fix for now and should cut down on the memory consumption from
this code.

On a x86_64 machine:

pre-patch:
(gdb) p sizeof(struct idmap)
$1 = 39504

post-patch:
(gdb) p sizeof(struct idmap)
$1 = 8784

That changes this from an order 4 to an order 2 allocation. Would be
even nicer to get it down to under a page, but this will help
considerably.

Reviewed-by: Jeff Layton <jlayton@redhat.com>

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

* Re: [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails
  2012-02-08 12:04           ` Jeff Layton
@ 2012-02-08 13:50             ` Myklebust, Trond
  2012-02-08 18:56               ` Myklebust, Trond
  0 siblings, 1 reply; 17+ messages in thread
From: Myklebust, Trond @ 2012-02-08 13:50 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Myklebust, Trond, Schumaker, Bryan, bfields, linux-nfs

T24gV2VkLCAyMDEyLTAyLTA4IGF0IDA3OjA0IC0wNTAwLCBKZWZmIExheXRvbiB3cm90ZToNCj4g
T24gVHVlLCA3IEZlYiAyMDEyIDIwOjA1OjU1ICswMDAwDQo+ICJNeWtsZWJ1c3QsIFRyb25kIiA8
VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20+IHdyb3RlOg0KPiANCj4gPiBPbiBUdWUsIDIwMTIt
MDItMDcgYXQgMTQ6NDQgLTA1MDAsIFRyb25kIE15a2xlYnVzdCB3cm90ZToNCj4gPiANCj4gPiA+
IE9uZSB3YXkgdG8gZWFzaWx5IHNocmluayB0aGUgc2l6ZSBvZiB0aGF0IGFsbG9jYXRpb24gaXMg
dG8gY29udmVydCB0aGUNCj4gPiA+IGloX25hbWUgc3RyaW5nIGludG8gYSBwb2ludGVyLCBhbmQg
aGF2ZSB0aGUgZG93bmNhbGwgYWxsb2NhdGUgdGhlDQo+ID4gPiBzdG9yYWdlIGZvciB0aGF0IHN0
cmluZyBkeW5hbWljYWxseS4uLg0KPiA+IA0KPiA+IExpa2Ugc28uLi4uDQo+ID4gDQo+IA0KPiBO
aWNlLiBJIGhhZCBzdGFydGVkIGxvb2tpbmcgYXQgdGhpcywgYnV0IHdhcyBzdGlsbCBzb3J0aW5n
IG15IHdheQ0KPiB0aHJvdWdoIGhvdyBhbGwgb2YgdGhlIGhhc2h0YWJsZSBjYWNoZSBjb2RlIHdv
cmtzLg0KPiANCj4gT25lIGNvbW1lbnQgaW5saW5lIGJlbG93Og0KPiANCj4gPiA4PC0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tDQo+ID4gRnJvbSA1MjIxYzg0MTU5NTc2MDQ1MDdjZDc0MDhhNjk1MjQxMDk2YmNiM2Fk
IE1vbiBTZXAgMTcgMDA6MDA6MDAgMjAwMQ0KPiA+IEZyb206IFRyb25kIE15a2xlYnVzdCA8VHJv
bmQuTXlrbGVidXN0QG5ldGFwcC5jb20+DQo+ID4gRGF0ZTogVHVlLCA3IEZlYiAyMDEyIDE0OjU5
OjA1IC0wNTAwDQo+ID4gU3ViamVjdDogW1BBVENIXSBORlN2NDogUmVkdWNlIHRoZSBmb290cHJp
bnQgb2YgdGhlIGlkbWFwcGVyDQo+ID4gDQo+ID4gSW5zdGVhZCBvZiBwcmUtYWxsb2NhdGluZyB0
aGUgc3RvcmFnZSBmb3IgYWxsIHRoZSBzdHJpbmdzLCB3ZSBjYW4NCj4gPiBzaWduaWZpY2FudGx5
IHJlZHVjZSB0aGUgc2l6ZSBvZiB0aGF0IHRhYmxlIGJ5IGRvaW5nIHRoZSBhbGxvY2F0aW9uDQo+
ID4gd2hlbiB3ZSBkbyB0aGUgZG93bmNhbGwuDQo+ID4gDQo+ID4gU2lnbmVkLW9mZi1ieTogVHJv
bmQgTXlrbGVidXN0IDxUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbT4NCj4gPiAtLS0NCj4gPiAg
ZnMvbmZzL2lkbWFwLmMgfCAgIDE2ICsrKysrKysrKysrKystLS0NCj4gPiAgMSBmaWxlcyBjaGFu
Z2VkLCAxMyBpbnNlcnRpb25zKCspLCAzIGRlbGV0aW9ucygtKQ0KPiA+IA0KPiA+IGRpZmYgLS1n
aXQgYS9mcy9uZnMvaWRtYXAuYyBiL2ZzL25mcy9pZG1hcC5jDQo+ID4gaW5kZXggNWE1NTY2Zi4u
ZmZmNzk0OCAxMDA2NDQNCj4gPiAtLS0gYS9mcy9uZnMvaWRtYXAuYw0KPiA+ICsrKyBiL2ZzL25m
cy9pZG1hcC5jDQo+ID4gQEAgLTM2Miw3ICszNjIsNyBAQCBzdHJ1Y3QgaWRtYXBfaGFzaGVudCB7
DQo+ID4gIAl1bnNpZ25lZCBsb25nCQlpaF9leHBpcmVzOw0KPiA+ICAJX191MzIJCQlpaF9pZDsN
Cj4gPiAgCXNpemVfdAkJCWloX25hbWVsZW47DQo+ID4gLQljaGFyCQkJaWhfbmFtZVtJRE1BUF9O
QU1FU1pdOw0KPiA+ICsJY29uc3QgY2hhcgkJKmloX25hbWU7DQo+ID4gIH07DQo+ID4gIA0KPiA+
ICBzdHJ1Y3QgaWRtYXBfaGFzaHRhYmxlIHsNCj4gPiBAQCAtNDgyLDEyICs0ODIsMTcgQEAgdm9p
ZA0KPiA+ICBuZnNfaWRtYXBfZGVsZXRlKHN0cnVjdCBuZnNfY2xpZW50ICpjbHApDQo+ID4gIHsN
Cj4gPiAgCXN0cnVjdCBpZG1hcCAqaWRtYXAgPSBjbHAtPmNsX2lkbWFwOw0KPiA+ICsJaW50IGk7
DQo+ID4gIA0KPiA+ICAJaWYgKCFpZG1hcCkNCj4gPiAgCQlyZXR1cm47DQo+ID4gIAluZnNfaWRt
YXBfdW5yZWdpc3RlcihjbHAsIGlkbWFwLT5pZG1hcF9waXBlKTsNCj4gPiAgCXJwY19kZXN0cm95
X3BpcGVfZGF0YShpZG1hcC0+aWRtYXBfcGlwZSk7DQo+ID4gIAljbHAtPmNsX2lkbWFwID0gTlVM
TDsNCj4gPiArCWZvciAoaSA9IDA7IGkgPCBBUlJBWV9TSVpFKGlkbWFwLT5pZG1hcF91c2VyX2hh
c2guaF9lbnRyaWVzKTsgaSsrKQ0KPiA+ICsJCWtmcmVlKGlkbWFwLT5pZG1hcF91c2VyX2hhc2gu
aF9lbnRyaWVzW2ldLmloX25hbWUpOw0KPiA+ICsJZm9yIChpID0gMDsgaSA8IEFSUkFZX1NJWkUo
aWRtYXAtPmlkbWFwX2dyb3VwX2hhc2guaF9lbnRyaWVzKTsgaSsrKQ0KPiA+ICsJCWtmcmVlKGlk
bWFwLT5pZG1hcF9ncm91cF9oYXNoLmhfZW50cmllc1tpXS5paF9uYW1lKTsNCj4gPiAgCWtmcmVl
KGlkbWFwKTsNCj4gPiAgfQ0KPiA+ICANCj4gPiBAQCAtNjM0LDkgKzYzOSwxNCBAQCBzdGF0aWMg
dm9pZA0KPiA+ICBpZG1hcF91cGRhdGVfZW50cnkoc3RydWN0IGlkbWFwX2hhc2hlbnQgKmhlLCBj
b25zdCBjaGFyICpuYW1lLA0KPiA+ICAJCXNpemVfdCBuYW1lbGVuLCBfX3UzMiBpZCkNCj4gPiAg
ew0KPiA+ICsJY2hhciAqc3RyID0ga21hbGxvYyhuYW1lbGVuICsgMSwgR0ZQX0tFUk5FTCk7DQo+
IA0KPiBEbyB3ZSBuZWVkIHRvIHdvcnJ5IGFib3V0IHJlY3Vyc2luZyBpbnRvIGRpcmVjdCByZWNs
YWltIGhlcmU/DQoNCk5vLiBUaGUgd3JpdGUgcGF0aCBkb2Vzbid0IHVzZSB0aGUgaWRtYXBwZXIg
YW55IG1vcmUuDQoNCj4gPiArCWlmIChzdHIgPT0gTlVMTCkNCj4gPiArCQlyZXR1cm47DQo+ID4g
KwlrZnJlZShoZS0+aWhfbmFtZSk7DQo+ID4gIAloZS0+aWhfaWQgPSBpZDsNCj4gPiAtCW1lbWNw
eShoZS0+aWhfbmFtZSwgbmFtZSwgbmFtZWxlbik7DQo+ID4gLQloZS0+aWhfbmFtZVtuYW1lbGVu
XSA9ICdcMCc7DQo+ID4gKwltZW1jcHkoc3RyLCBuYW1lLCBuYW1lbGVuKTsNCj4gPiArCXN0cltu
YW1lbGVuXSA9ICdcMCc7DQo+ID4gKwloZS0+aWhfbmFtZSA9IHN0cjsNCj4gPiAgCWhlLT5paF9u
YW1lbGVuID0gbmFtZWxlbjsNCj4gPiAgCWhlLT5paF9leHBpcmVzID0gamlmZmllcyArIG5mc19p
ZG1hcF9jYWNoZV90aW1lb3V0Ow0KPiA+ICB9DQo+IA0KPiAuLi5hbmQgdGhlIG5hbWVsZW4gY2hl
Y2sgaW4gaWRtYXBfbG9va3VwX25hbWUgc2hvdWxkIGtlZXAgdGhlIG1lbWNtcA0KPiBmcm9tIHRy
aXBwaW5nIG92ZXIgdGhlc2UgTlVMTCBwb2ludGVycy4gSSB0aGluayB0aGlzIGxvb2tzIGxpa2Ug
YSBuaWNlDQo+IGludGVyaW0gZml4IGZvciBub3cgYW5kIHNob3VsZCBjdXQgZG93biBvbiB0aGUg
bWVtb3J5IGNvbnN1bXB0aW9uIGZyb20NCj4gdGhpcyBjb2RlLg0KPiANCj4gT24gYSB4ODZfNjQg
bWFjaGluZToNCj4gDQo+IHByZS1wYXRjaDoNCj4gKGdkYikgcCBzaXplb2Yoc3RydWN0IGlkbWFw
KQ0KPiAkMSA9IDM5NTA0DQo+IA0KPiBwb3N0LXBhdGNoOg0KPiAoZ2RiKSBwIHNpemVvZihzdHJ1
Y3QgaWRtYXApDQo+ICQxID0gODc4NA0KPiANCj4gVGhhdCBjaGFuZ2VzIHRoaXMgZnJvbSBhbiBv
cmRlciA0IHRvIGFuIG9yZGVyIDIgYWxsb2NhdGlvbi4gV291bGQgYmUNCj4gZXZlbiBuaWNlciB0
byBnZXQgaXQgZG93biB0byB1bmRlciBhIHBhZ2UsIGJ1dCB0aGlzIHdpbGwgaGVscA0KPiBjb25z
aWRlcmFibHkuDQo+IA0KPiBSZXZpZXdlZC1ieTogSmVmZiBMYXl0b24gPGpsYXl0b25AcmVkaGF0
LmNvbT4NCg0KV2UgY291bGQgZG8gdGhhdCBieSBhbGxvY2F0aW5nIHRoZSB0d28gaWRtYXBfaGFz
aGVudCBhcnJheXMgc2VwYXJhdGVseS4NCkknbGwgc2VlIGFib3V0IHRoYXQgbGF0ZXIgdG9kYXku
DQoNCkluIHRoZSBtZWFuIHRpbWUsIEkndmUgZ290IEJyeWFuIGNvbnRpbnVpbmcgdG8gbG9vayBp
bnRvIHRoZSBwb3NzaWJpbGl0eQ0Kb2YgdXNpbmcgdGhlIGtleXJpbmdzIGZvciBib3RoIHR5cGVz
IG9mIHVwY2FsbC4NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFp
bnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBw
LmNvbQ0KDQo=

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

* Re: [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails
  2012-02-08 13:50             ` Myklebust, Trond
@ 2012-02-08 18:56               ` Myklebust, Trond
  2012-02-08 19:30                 ` Jeff Layton
  0 siblings, 1 reply; 17+ messages in thread
From: Myklebust, Trond @ 2012-02-08 18:56 UTC (permalink / raw)
  To: Myklebust, Trond; +Cc: Jeff Layton, Schumaker, Bryan, bfields, linux-nfs

T24gV2VkLCAyMDEyLTAyLTA4IGF0IDA4OjUwIC0wNTAwLCBUcm9uZCBNeWtsZWJ1c3Qgd3JvdGU6
DQo+IE9uIFdlZCwgMjAxMi0wMi0wOCBhdCAwNzowNCAtMDUwMCwgSmVmZiBMYXl0b24gd3JvdGU6
DQo+ID4gT24gYSB4ODZfNjQgbWFjaGluZToNCj4gPiANCj4gPiBwcmUtcGF0Y2g6DQo+ID4gKGdk
YikgcCBzaXplb2Yoc3RydWN0IGlkbWFwKQ0KPiA+ICQxID0gMzk1MDQNCj4gPiANCj4gPiBwb3N0
LXBhdGNoOg0KPiA+IChnZGIpIHAgc2l6ZW9mKHN0cnVjdCBpZG1hcCkNCj4gPiAkMSA9IDg3ODQN
Cj4gPiANCj4gPiBUaGF0IGNoYW5nZXMgdGhpcyBmcm9tIGFuIG9yZGVyIDQgdG8gYW4gb3JkZXIg
MiBhbGxvY2F0aW9uLiBXb3VsZCBiZQ0KPiA+IGV2ZW4gbmljZXIgdG8gZ2V0IGl0IGRvd24gdG8g
dW5kZXIgYSBwYWdlLCBidXQgdGhpcyB3aWxsIGhlbHANCj4gPiBjb25zaWRlcmFibHkuDQo+ID4g
DQo+ID4gUmV2aWV3ZWQtYnk6IEplZmYgTGF5dG9uIDxqbGF5dG9uQHJlZGhhdC5jb20+DQo+IA0K
PiBXZSBjb3VsZCBkbyB0aGF0IGJ5IGFsbG9jYXRpbmcgdGhlIHR3byBpZG1hcF9oYXNoZW50IGFy
cmF5cyBzZXBhcmF0ZWx5Lg0KPiBJJ2xsIHNlZSBhYm91dCB0aGF0IGxhdGVyIHRvZGF5Lg0KDQpI
ZXJlIHlvdSBnby4uLiBJdCBub3cgZW5zdXJlcyB0aGF0IHRoZSBhcnJheXMgZG9uJ3QgZXZlbiBn
ZXQgYWxsb2NhdGVkDQphdCBhbGwgaWYgeW91IGFyZSB1c2luZyB0aGUgbmV3IGlkbWFwcGVyLi4u
DQoNCjg8LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tDQpGcm9tIDg2NTBjOTYxMGY5YWVlYjUzNmE3MGZlYWNiNDg4
ZGZiNjU4MzA5MGUgTW9uIFNlcCAxNyAwMDowMDowMCAyMDAxDQpGcm9tOiBUcm9uZCBNeWtsZWJ1
c3QgPFRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tPg0KRGF0ZTogV2VkLCA4IEZlYiAyMDEyIDEz
OjM5OjE1IC0wNTAwDQpTdWJqZWN0OiBbUEFUQ0hdIE5GU3Y0OiBGdXJ0aGVyIHJlZHVjZSB0aGUg
Zm9vdHByaW50IG9mIHRoZSBpZG1hcHBlcg0KDQpEb24ndCBhbGxvY2F0ZSB0aGUgbGVnYWN5IGlk
bWFwcGVyIHRhYmxlcyB1bnRpbCB3ZSBhY3R1YWxseSBuZWVkDQp0aGVtLg0KDQpTaWduZWQtb2Zm
LWJ5OiBUcm9uZCBNeWtsZWJ1c3QgPFRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tPg0KLS0tDQog
ZnMvbmZzL2lkbWFwLmMgfCAgIDQyICsrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysr
Ky0tLS0tLQ0KIDEgZmlsZXMgY2hhbmdlZCwgMzYgaW5zZXJ0aW9ucygrKSwgNiBkZWxldGlvbnMo
LSkNCg0KZGlmZiAtLWdpdCBhL2ZzL25mcy9pZG1hcC5jIGIvZnMvbmZzL2lkbWFwLmMNCmluZGV4
IGZmZjc5NDguLmI1YzZkOGUgMTAwNjQ0DQotLS0gYS9mcy9uZnMvaWRtYXAuYw0KKysrIGIvZnMv
bmZzL2lkbWFwLmMNCkBAIC0zNjcsNyArMzY3LDcgQEAgc3RydWN0IGlkbWFwX2hhc2hlbnQgew0K
IA0KIHN0cnVjdCBpZG1hcF9oYXNodGFibGUgew0KIAlfX3U4CQkJaF90eXBlOw0KLQlzdHJ1Y3Qg
aWRtYXBfaGFzaGVudAloX2VudHJpZXNbSURNQVBfSEFTSF9TWl07DQorCXN0cnVjdCBpZG1hcF9o
YXNoZW50CSpoX2VudHJpZXM7DQogfTsNCiANCiBzdHJ1Y3QgaWRtYXAgew0KQEAgLTQ3OCwyMSAr
NDc4LDQwIEBAIG5mc19pZG1hcF9uZXcoc3RydWN0IG5mc19jbGllbnQgKmNscCkNCiAJcmV0dXJu
IDA7DQogfQ0KIA0KK3N0YXRpYyB2b2lkDQoraWRtYXBfYWxsb2NfaGFzaHRhYmxlKHN0cnVjdCBp
ZG1hcF9oYXNodGFibGUgKmgpDQorew0KKwlpZiAoaC0+aF9lbnRyaWVzICE9IE5VTEwpDQorCQly
ZXR1cm47DQorCWgtPmhfZW50cmllcyA9IGtjYWxsb2MoSURNQVBfSEFTSF9TWiwNCisJCQlzaXpl
b2YoKmgtPmhfZW50cmllcyksDQorCQkJR0ZQX0tFUk5FTCk7DQorfQ0KKw0KK3N0YXRpYyB2b2lk
DQoraWRtYXBfZnJlZV9oYXNodGFibGUoc3RydWN0IGlkbWFwX2hhc2h0YWJsZSAqaCkNCit7DQor
CWludCBpOw0KKw0KKwlpZiAoaC0+aF9lbnRyaWVzID09IE5VTEwpDQorCQlyZXR1cm47DQorCWZv
ciAoaSA9IDA7IGkgPCBJRE1BUF9IQVNIX1NaOyBpKyspDQorCQlrZnJlZShoLT5oX2VudHJpZXNb
aV0uaWhfbmFtZSk7DQorCWtmcmVlKGgtPmhfZW50cmllcyk7DQorfQ0KKw0KIHZvaWQNCiBuZnNf
aWRtYXBfZGVsZXRlKHN0cnVjdCBuZnNfY2xpZW50ICpjbHApDQogew0KIAlzdHJ1Y3QgaWRtYXAg
KmlkbWFwID0gY2xwLT5jbF9pZG1hcDsNCi0JaW50IGk7DQogDQogCWlmICghaWRtYXApDQogCQly
ZXR1cm47DQogCW5mc19pZG1hcF91bnJlZ2lzdGVyKGNscCwgaWRtYXAtPmlkbWFwX3BpcGUpOw0K
IAlycGNfZGVzdHJveV9waXBlX2RhdGEoaWRtYXAtPmlkbWFwX3BpcGUpOw0KIAljbHAtPmNsX2lk
bWFwID0gTlVMTDsNCi0JZm9yIChpID0gMDsgaSA8IEFSUkFZX1NJWkUoaWRtYXAtPmlkbWFwX3Vz
ZXJfaGFzaC5oX2VudHJpZXMpOyBpKyspDQotCQlrZnJlZShpZG1hcC0+aWRtYXBfdXNlcl9oYXNo
LmhfZW50cmllc1tpXS5paF9uYW1lKTsNCi0JZm9yIChpID0gMDsgaSA8IEFSUkFZX1NJWkUoaWRt
YXAtPmlkbWFwX2dyb3VwX2hhc2guaF9lbnRyaWVzKTsgaSsrKQ0KLQkJa2ZyZWUoaWRtYXAtPmlk
bWFwX2dyb3VwX2hhc2guaF9lbnRyaWVzW2ldLmloX25hbWUpOw0KKwlpZG1hcF9mcmVlX2hhc2h0
YWJsZSgmaWRtYXAtPmlkbWFwX3VzZXJfaGFzaCk7DQorCWlkbWFwX2ZyZWVfaGFzaHRhYmxlKCZp
ZG1hcC0+aWRtYXBfZ3JvdXBfaGFzaCk7DQogCWtmcmVlKGlkbWFwKTsNCiB9DQogDQpAQCAtNTg2
LDYgKzYwNSw4IEBAIHZvaWQgbmZzX2lkbWFwX3F1aXQodm9pZCkNCiBzdGF0aWMgaW5saW5lIHN0
cnVjdCBpZG1hcF9oYXNoZW50ICoNCiBpZG1hcF9uYW1lX2hhc2goc3RydWN0IGlkbWFwX2hhc2h0
YWJsZSogaCwgY29uc3QgY2hhciAqbmFtZSwgc2l6ZV90IGxlbikNCiB7DQorCWlmIChoLT5oX2Vu
dHJpZXMgPT0gTlVMTCkNCisJCXJldHVybiBOVUxMOw0KIAlyZXR1cm4gJmgtPmhfZW50cmllc1tm
bnZoYXNoMzIobmFtZSwgbGVuKSAlIElETUFQX0hBU0hfU1pdOw0KIH0NCiANCkBAIC01OTQsNiAr
NjE1LDggQEAgaWRtYXBfbG9va3VwX25hbWUoc3RydWN0IGlkbWFwX2hhc2h0YWJsZSAqaCwgY29u
c3QgY2hhciAqbmFtZSwgc2l6ZV90IGxlbikNCiB7DQogCXN0cnVjdCBpZG1hcF9oYXNoZW50ICpo
ZSA9IGlkbWFwX25hbWVfaGFzaChoLCBuYW1lLCBsZW4pOw0KIA0KKwlpZiAoaGUgPT0gTlVMTCkN
CisJCXJldHVybiBOVUxMOw0KIAlpZiAoaGUtPmloX25hbWVsZW4gIT0gbGVuIHx8IG1lbWNtcCho
ZS0+aWhfbmFtZSwgbmFtZSwgbGVuKSAhPSAwKQ0KIAkJcmV0dXJuIE5VTEw7DQogCWlmICh0aW1l
X2FmdGVyKGppZmZpZXMsIGhlLT5paF9leHBpcmVzKSkNCkBAIC02MDQsNiArNjI3LDggQEAgaWRt
YXBfbG9va3VwX25hbWUoc3RydWN0IGlkbWFwX2hhc2h0YWJsZSAqaCwgY29uc3QgY2hhciAqbmFt
ZSwgc2l6ZV90IGxlbikNCiBzdGF0aWMgaW5saW5lIHN0cnVjdCBpZG1hcF9oYXNoZW50ICoNCiBp
ZG1hcF9pZF9oYXNoKHN0cnVjdCBpZG1hcF9oYXNodGFibGUqIGgsIF9fdTMyIGlkKQ0KIHsNCisJ
aWYgKGgtPmhfZW50cmllcyA9PSBOVUxMKQ0KKwkJcmV0dXJuIE5VTEw7DQogCXJldHVybiAmaC0+
aF9lbnRyaWVzW2Zudmhhc2gzMigmaWQsIHNpemVvZihpZCkpICUgSURNQVBfSEFTSF9TWl07DQog
fQ0KIA0KQEAgLTYxMSw2ICs2MzYsOSBAQCBzdGF0aWMgc3RydWN0IGlkbWFwX2hhc2hlbnQgKg0K
IGlkbWFwX2xvb2t1cF9pZChzdHJ1Y3QgaWRtYXBfaGFzaHRhYmxlICpoLCBfX3UzMiBpZCkNCiB7
DQogCXN0cnVjdCBpZG1hcF9oYXNoZW50ICpoZSA9IGlkbWFwX2lkX2hhc2goaCwgaWQpOw0KKw0K
KwlpZiAoaGUgPT0gTlVMTCkNCisJCXJldHVybiBOVUxMOw0KIAlpZiAoaGUtPmloX2lkICE9IGlk
IHx8IGhlLT5paF9uYW1lbGVuID09IDApDQogCQlyZXR1cm4gTlVMTDsNCiAJaWYgKHRpbWVfYWZ0
ZXIoamlmZmllcywgaGUtPmloX2V4cGlyZXMpKQ0KQEAgLTYyNiwxMiArNjU0LDE0IEBAIGlkbWFw
X2xvb2t1cF9pZChzdHJ1Y3QgaWRtYXBfaGFzaHRhYmxlICpoLCBfX3UzMiBpZCkNCiBzdGF0aWMg
aW5saW5lIHN0cnVjdCBpZG1hcF9oYXNoZW50ICoNCiBpZG1hcF9hbGxvY19uYW1lKHN0cnVjdCBp
ZG1hcF9oYXNodGFibGUgKmgsIGNoYXIgKm5hbWUsIHNpemVfdCBsZW4pDQogew0KKwlpZG1hcF9h
bGxvY19oYXNodGFibGUoaCk7DQogCXJldHVybiBpZG1hcF9uYW1lX2hhc2goaCwgbmFtZSwgbGVu
KTsNCiB9DQogDQogc3RhdGljIGlubGluZSBzdHJ1Y3QgaWRtYXBfaGFzaGVudCAqDQogaWRtYXBf
YWxsb2NfaWQoc3RydWN0IGlkbWFwX2hhc2h0YWJsZSAqaCwgX191MzIgaWQpDQogew0KKwlpZG1h
cF9hbGxvY19oYXNodGFibGUoaCk7DQogCXJldHVybiBpZG1hcF9pZF9oYXNoKGgsIGlkKTsNCiB9
DQogDQotLSANCjEuNy43LjYNCg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNs
aWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3
dy5uZXRhcHAuY29tDQoNCg==

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

* Re: [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails
  2012-02-08 18:56               ` Myklebust, Trond
@ 2012-02-08 19:30                 ` Jeff Layton
  0 siblings, 0 replies; 17+ messages in thread
From: Jeff Layton @ 2012-02-08 19:30 UTC (permalink / raw)
  To: Myklebust, Trond; +Cc: Schumaker, Bryan, bfields, linux-nfs

On Wed, 8 Feb 2012 18:56:01 +0000
"Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:

> On Wed, 2012-02-08 at 08:50 -0500, Trond Myklebust wrote:
> > On Wed, 2012-02-08 at 07:04 -0500, Jeff Layton wrote:
> > > On a x86_64 machine:
> > > 
> > > pre-patch:
> > > (gdb) p sizeof(struct idmap)
> > > $1 = 39504
> > > 
> > > post-patch:
> > > (gdb) p sizeof(struct idmap)
> > > $1 = 8784
> > > 
> > > That changes this from an order 4 to an order 2 allocation. Would be
> > > even nicer to get it down to under a page, but this will help
> > > considerably.
> > > 
> > > Reviewed-by: Jeff Layton <jlayton@redhat.com>
> > 
> > We could do that by allocating the two idmap_hashent arrays separately.
> > I'll see about that later today.
> 
> Here you go... It now ensures that the arrays don't even get allocated
> at all if you are using the new idmapper...
> 
> 8<------------------------------------------------------------------------
> From 8650c9610f9aeeb536a70feacb488dfb6583090e Mon Sep 17 00:00:00 2001
> From: Trond Myklebust <Trond.Myklebust@netapp.com>
> Date: Wed, 8 Feb 2012 13:39:15 -0500
> Subject: [PATCH] NFSv4: Further reduce the footprint of the idmapper
> 
> Don't allocate the legacy idmapper tables until we actually need
> them.
> 
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
>  fs/nfs/idmap.c |   42 ++++++++++++++++++++++++++++++++++++------
>  1 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
> index fff7948..b5c6d8e 100644
> --- a/fs/nfs/idmap.c
> +++ b/fs/nfs/idmap.c
> @@ -367,7 +367,7 @@ struct idmap_hashent {
>  
>  struct idmap_hashtable {
>  	__u8			h_type;
> -	struct idmap_hashent	h_entries[IDMAP_HASH_SZ];
> +	struct idmap_hashent	*h_entries;
>  };
>  
>  struct idmap {
> @@ -478,21 +478,40 @@ nfs_idmap_new(struct nfs_client *clp)
>  	return 0;
>  }
>  
> +static void
> +idmap_alloc_hashtable(struct idmap_hashtable *h)
> +{
> +	if (h->h_entries != NULL)
> +		return;
> +	h->h_entries = kcalloc(IDMAP_HASH_SZ,
> +			sizeof(*h->h_entries),
> +			GFP_KERNEL);
> +}
> +
> +static void
> +idmap_free_hashtable(struct idmap_hashtable *h)
> +{
> +	int i;
> +
> +	if (h->h_entries == NULL)
> +		return;
> +	for (i = 0; i < IDMAP_HASH_SZ; i++)
> +		kfree(h->h_entries[i].ih_name);
> +	kfree(h->h_entries);
> +}
> +
>  void
>  nfs_idmap_delete(struct nfs_client *clp)
>  {
>  	struct idmap *idmap = clp->cl_idmap;
> -	int i;
>  
>  	if (!idmap)
>  		return;
>  	nfs_idmap_unregister(clp, idmap->idmap_pipe);
>  	rpc_destroy_pipe_data(idmap->idmap_pipe);
>  	clp->cl_idmap = NULL;
> -	for (i = 0; i < ARRAY_SIZE(idmap->idmap_user_hash.h_entries); i++)
> -		kfree(idmap->idmap_user_hash.h_entries[i].ih_name);
> -	for (i = 0; i < ARRAY_SIZE(idmap->idmap_group_hash.h_entries); i++)
> -		kfree(idmap->idmap_group_hash.h_entries[i].ih_name);
> +	idmap_free_hashtable(&idmap->idmap_user_hash);
> +	idmap_free_hashtable(&idmap->idmap_group_hash);
>  	kfree(idmap);
>  }
>  
> @@ -586,6 +605,8 @@ void nfs_idmap_quit(void)
>  static inline struct idmap_hashent *
>  idmap_name_hash(struct idmap_hashtable* h, const char *name, size_t len)
>  {
> +	if (h->h_entries == NULL)
> +		return NULL;
>  	return &h->h_entries[fnvhash32(name, len) % IDMAP_HASH_SZ];
>  }
>  
> @@ -594,6 +615,8 @@ idmap_lookup_name(struct idmap_hashtable *h, const char *name, size_t len)
>  {
>  	struct idmap_hashent *he = idmap_name_hash(h, name, len);
>  
> +	if (he == NULL)
> +		return NULL;
>  	if (he->ih_namelen != len || memcmp(he->ih_name, name, len) != 0)
>  		return NULL;
>  	if (time_after(jiffies, he->ih_expires))
> @@ -604,6 +627,8 @@ idmap_lookup_name(struct idmap_hashtable *h, const char *name, size_t len)
>  static inline struct idmap_hashent *
>  idmap_id_hash(struct idmap_hashtable* h, __u32 id)
>  {
> +	if (h->h_entries == NULL)
> +		return NULL;
>  	return &h->h_entries[fnvhash32(&id, sizeof(id)) % IDMAP_HASH_SZ];
>  }
>  
> @@ -611,6 +636,9 @@ static struct idmap_hashent *
>  idmap_lookup_id(struct idmap_hashtable *h, __u32 id)
>  {
>  	struct idmap_hashent *he = idmap_id_hash(h, id);
> +
> +	if (he == NULL)
> +		return NULL;
>  	if (he->ih_id != id || he->ih_namelen == 0)
>  		return NULL;
>  	if (time_after(jiffies, he->ih_expires))
> @@ -626,12 +654,14 @@ idmap_lookup_id(struct idmap_hashtable *h, __u32 id)
>  static inline struct idmap_hashent *
>  idmap_alloc_name(struct idmap_hashtable *h, char *name, size_t len)
>  {
> +	idmap_alloc_hashtable(h);
>  	return idmap_name_hash(h, name, len);
>  }
>  
>  static inline struct idmap_hashent *
>  idmap_alloc_id(struct idmap_hashtable *h, __u32 id)
>  {
> +	idmap_alloc_hashtable(h);
>  	return idmap_id_hash(h, id);
>  }
>  

Much, much better! Nice work.

Built on a different machine this time with a different compiler, so
sizes are a little different. Not certain why, but they're fairly close
-- maybe differences in padding or something:

Without either patch:
(gdb) p sizeof(struct idmap)
$1 = 39168

With just the first patch:
(gdb) p sizeof(struct idmap)
$1 = 8448

With both patches:
(gdb) p sizeof(struct idmap)
$1 = 272


Reviewed-by: Jeff Layton <jlayton@redhat.com>

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

end of thread, other threads:[~2012-02-08 19:31 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-26 21:54 [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails bjschuma
2012-01-26 21:54 ` [PATCH 2/3] NFS: Keep idmapper include files in one place bjschuma
2012-02-06 23:05   ` Myklebust, Trond
2012-01-26 21:54 ` [PATCH 3/3] NFS: Update idmapper documentation bjschuma
2012-01-26 21:56 ` [PATCH 1/3] NFS: Fall back on old idmapper if request_key() fails J. Bruce Fields
2012-01-26 21:57   ` Bryan Schumaker
2012-02-06 23:05 ` Myklebust, Trond
2012-02-07 15:54   ` Bryan Schumaker
2012-02-07 19:12 ` Jeff Layton
2012-02-07 19:21   ` Myklebust, Trond
2012-02-07 19:29     ` Bryan Schumaker
2012-02-07 19:44       ` Myklebust, Trond
2012-02-07 20:05         ` Myklebust, Trond
2012-02-08 12:04           ` Jeff Layton
2012-02-08 13:50             ` Myklebust, Trond
2012-02-08 18:56               ` Myklebust, Trond
2012-02-08 19:30                 ` Jeff Layton

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.