Skip to contentSkip to conversation

NFS: Fall back on old idmapper if request_key() fails

PATCH In review [email protected] 4 participants 17 messages Jan 26, 2012 – Feb 8, 2012

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/3 NFS: Fall back on old idmapper if request_key() fails

From: Bryan Schumaker <[email protected]>

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.

---
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(-)
fs/nfs/Kconfig +0 -11
index dbcd821..021d2cf 100644
@@ -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.
fs/nfs/idmap.c +25 -51

117 lines not on this page · raw message

fs/nfs/sysctl.c +0 -2
index 978aaeb..ad4d2e7 100644
@@ -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",
include/linux/nfs_idmap.h +0 -25
index 308c188..717fa50 100644
@@ -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);
Signature · 1 line
1.7.8.4
#1 lore raw

PATCH 2/3 NFS: Keep idmapper include files in one place

From: Bryan Schumaker <[email protected]>

---
 fs/nfs/idmap.c |   64 ++++++++++++++++++++++++++-----------------------------
 1 files changed, 30 insertions(+), 34 deletions(-)
fs/nfs/idmap.c +30 -34
index 56b1104..fbbc77e 100644
@@ -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;
Signature · 1 line
1.7.8.4
#2 lore raw

Re: [PATCH 2/3] NFS: Keep idmapper include files in one place

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

#3 lore raw

PATCH 3/3 NFS: Update idmapper documentation

In reply to [email protected]

From: Bryan Schumaker <[email protected]>

---
 Documentation/filesystems/nfs/idmapper.txt |   20 ++++++++++++++------
 1 files changed, 14 insertions(+), 6 deletions(-)
Documentation/filesystems/nfs/idmapper.txt +14 -6
index 120fd3c..fe03d10 100644
@@ -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
Signature · 1 line
1.7.8.4
#4 lore raw
In reply to [email protected]

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

--b.

Quoted 228 lines · On Thu, Jan 26, 2012 at 04:54:23PM -0500, [email protected] wrote:

From: Bryan Schumaker <[email protected]>

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.

---
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(-)
fs/nfs/Kconfig +0 -11
index dbcd821..021d2cf 100644
@@ -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.
fs/nfs/idmap.c +25 -51
index 2c05f19..56b1104 100644
@@ -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 */
fs/nfs/sysctl.c +0 -2
index 978aaeb..ad4d2e7 100644
@@ -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",
include/linux/nfs_idmap.h +0 -25
index 308c188..717fa50 100644
@@ -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);
Signature · 1 line
1.7.8.4
#5 lore raw
Quoted 1 line · J. Bruce Fields

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...

Quoted 232 lines

--b.

Quoted 228 lines · On Thu, Jan 26, 2012 at 04:54:23PM -0500, [email protected] wrote:

From: Bryan Schumaker <[email protected]>

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.

---
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(-)
fs/nfs/Kconfig +0 -11
index dbcd821..021d2cf 100644
@@ -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.
fs/nfs/idmap.c +25 -51
index 2c05f19..56b1104 100644
@@ -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 */
fs/nfs/sysctl.c +0 -2
index 978aaeb..ad4d2e7 100644
@@ -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",
include/linux/nfs_idmap.h +0 -25
index 308c188..717fa50 100644
@@ -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);
Signature · 1 line
1.7.8.4
#6 lore raw
In reply to [email protected]

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

#7 lore raw
Quoted 16 lines · Trond
Quoted 9 lines · On Thu, 2012-01-26 at 16:54 -0500, [email protected] wrote:

From: Bryan Schumaker <[email protected]>

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.

Quoted 5 lines

---
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.

Quoted 3 lines

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

Quoted 2 lines
#8 lore raw
In reply to [email protected]
Quoted 11 lines · On Thu, 26 Jan 2012 16:54:23 -0500 [email protected] wrote:

From: Bryan Schumaker <[email protected]>

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.

---

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?

Signature · 1 line
Jeff Layton <[email protected]>
#9 lore raw

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

#10 lore raw
Quoted 16 lines · Trond
Quoted 6 lines · Jeff Layton

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
Quoted 8 lines

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.

Quoted 5 lines

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...

Quoted 10 lines
Quoted 6 lines

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

Quoted 3 lines
#11 lore raw

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=

#12 lore raw

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=

Reviewed-by
#13 lore raw
Quoted 8 lines · Trond
Quoted 3 lines · Trond Myklebust

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:

Quoted 47 lines

8<----------------------------------------------------------------------- From 5221c8415957604507cd7408a695241096bcb3ad Mon Sep 17 00:00:00 2001
From: Trond Myklebust <[email protected]>
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.

---
 fs/nfs/idmap.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)
fs/nfs/idmap.c +6 -1
index 5a5566f..fff7948 100644
@@ -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);
 }
 

fs/nfs/idmap.c static void · line 642

Quoted 4 lines
fs/nfs/idmap.c +1 -0
@@ -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?

fs/nfs/idmap.c static void · line 652

Quoted 12 lines
fs/nfs/idmap.c +6 -2
+	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.

#14 lore raw

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=

#15 lore raw

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==

Reviewed-by
#16 lore raw
Quoted 139 lines · Trond
Quoted 11 lines · Trond Myklebust
Quoted 10 lines · Jeff Layton

On a x86_64 machine:

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

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

Quoted 8 lines
Quoted 5 lines

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.

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 <[email protected]>
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.

---
 fs/nfs/idmap.c |   42 ++++++++++++++++++++++++++++++++++++------
 1 files changed, 36 insertions(+), 6 deletions(-)
fs/nfs/idmap.c +35 -6
index fff7948..b5c6d8e 100644
@@ -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);

fs/nfs/idmap.c idmap_lookup_id(struct idmap_hashtable *h, __u32 id) · line 667

Quoted 10 lines
fs/nfs/idmap.c +1 -0
 	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