[Patch[ question regarding NFS_ACLS and transfer size/improvement to NFS_ACL tuning

Neil Horman <[email protected]> Tue, 07 Sep 2004 15:31:53 -0400
Newsgroups gmane.linux.file-systems.acl.devel
Message-ID <[email protected]>
Hey all-
	I'm new to the list so please forgive any ignorance I might show here 
early on.  My name is Neil Horman and I'm working on a fix for a memory 
allocation issue in the 2.4 linux kernel that I thought you all might be 
interested in.  I'm working on a bug that appears when ACL's are being 
used over NFS while a system is under memory pressure.  It seems that 
the NFS_ACL implementation currently specifies that 1024 ACE entries can 
be returned in a single NFS_ACL response.  The size of an ACE structure 
is such that allocating a response buffer to hold 1024 entries 
(regardless of how many ACE objects are actually going to be returned), 
can lead to allocation failures (1024 entries is IIRC an order 2 
allocation or 16KB of contiguous data).  When a system is being heavily 
used, this can cause long pauses/hangs in filesystem response time.  My 
idea has been to allow for a tuning of the number of ACE objects that 
the NFS_ACL client and server support via a module parameter.  This way, 
a user can adjust how many ACE objects are pre-resevered for NFS 
transactions based on the maximum number of ACL's they implement for 
their filesystem.  This way they can cut off the overhead that their 
environment doesn't need, and avoid the aforementioned allocation 
failures.  I've attached the patches that I've generated against the 
kernel that I'm currently working on (one for the nfs client, one for 
the server), but if it makes sense, I'd like to do an upstream port to 2.6.

Any thoughts anyone might have to improve upon this (or indicate why its 
a bad idea) would be appreciated.  Thanks a bunch!

Neil
-- 
/***************************************************
  *Neil Horman
  *Software Engineer
  *Red Hat, Inc.
  *[email protected]
  *gpg keyid: 1024D / 0x92A74FA1
  *http://pgp.mit.edu
  ***************************************************/

_______________________________________________
acl-devel mailing list
[email protected]
http://acl.bestbits.at/mailman/listinfo/acl-devel
linux-2.4.21-20.EL-nfs-ace.patch (text/plain, 6.4 KB)
--- linux-2.4.21-20.EL-ace/fs/nfs/inode.c.orig	2004-08-18 20:30:27.000000000 -0400
+++ linux-2.4.21-20.EL-ace/fs/nfs/inode.c	2004-09-07 17:45:18.000000000 -0400
@@ -33,6 +33,8 @@
 #include <linux/lockd/bind.h>
 #include <linux/smp_lock.h>
 #include <linux/seq_file.h>
+#include <linux/solaris_acl.h>
+#include <linux/sysctl.h>
 
 #include <asm/system.h>
 #include <asm/uaccess.h>
@@ -41,6 +43,48 @@
 #define NFSDBG_FACILITY		NFSDBG_VFS
 #define NFS_PARANOIA 1
 
+#ifdef CONFIG_NFS_ACL
+unsigned int nfs3_acl_max_entries=NFS3_ACL_MAX_ENTRIES;
+MODULE_PARM(nfs3_acl_max_entries, "i");
+MODULE_PARM_DESC(nfs3_acl_max_entries, "Max number of ACE objects to support in an NFS_ACL response");
+#endif
+
+static struct ctl_table_header *nfs_sysctl_table;
+#define CTL_UNNUMBERED          -2
+static ctl_table nfs_sysctl_files[] = {
+#ifdef CONFIG_NFS_ACL
+        {
+                .ctl_name       = CTL_UNNUMBERED,
+                .procname       = "nfs3_acl_max_entries",
+                .data           = &nfs3_acl_max_entries,
+                .maxlen         = sizeof(int),
+                .mode           = 0444,
+                .proc_handler   = &proc_dointvec,
+        },
+#endif
+        { .ctl_name = 0 }
+};
+
+static ctl_table nfs_sysctl_dir[] = {
+        {
+                .ctl_name       = CTL_UNNUMBERED,
+                .procname       = "nfs",
+                .mode           = 0555,
+                .child          =nfs_sysctl_files,
+        },
+        { .ctl_name = 0 }
+};
+
+static ctl_table nfs_sysctl_root[] = {
+        {
+                .ctl_name       = CTL_FS,
+                .procname       = "fs",
+                .mode           = 0555,
+                .child          = nfs_sysctl_dir,
+        },
+        { .ctl_name = 0 }
+};
+
 static struct inode * __nfs_fhget(struct super_block *, struct nfs_fh *, struct nfs_fattr *);
 void nfs_zap_caches(struct inode *);
 static void nfs_invalidate_inode(struct inode *);
@@ -53,6 +97,7 @@
 static void nfs_umount_begin(struct super_block *);
 static int  nfs_statfs(struct super_block *, struct statfs *);
 static int  nfs_show_options(struct seq_file *, struct vfsmount *);
+extern void nfs3_fixup_xdr_tables(unsigned int max_acl);
 
 static struct super_operations nfs_sops = { 
 	read_inode:	nfs_read_inode,
@@ -1326,6 +1371,10 @@
 {
 	int err;
 
+#ifdef CONFIG_NFS_ACL
+	nfs3_fixup_xdr_tables(nfs3_acl_max_entries);
+#endif
+
 	err = nfs_init_nfspagecache();
 	if (err)
 		return err;
@@ -1340,6 +1389,7 @@
 
 #ifdef CONFIG_PROC_FS
 	rpc_proc_register(&nfs_rpcstat);
+	nfs_sysctl_table = register_sysctl_table(nfs_sysctl_root,0);
 #endif
         return register_filesystem(&nfs_fs_type);
 }
@@ -1351,6 +1401,8 @@
 	nfs_destroy_nfspagecache();
 #ifdef CONFIG_PROC_FS
 	rpc_proc_unregister("nfs");
+	if(nfs_sysctl_table)
+		unregister_sysctl_table(nfs_sysctl_table);
 #endif
 	unregister_filesystem(&nfs_fs_type);
 }
--- linux-2.4.21-20.EL-ace/fs/nfs/nfs3proc.c.orig	2004-08-18 20:30:27.000000000 -0400
+++ linux-2.4.21-20.EL-ace/fs/nfs/nfs3proc.c	2004-09-07 13:13:20.000000000 -0400
@@ -708,7 +708,7 @@
 
 	/* We are doing this here, because XDR marshalling can only
 	   return -ENOMEM. */
-	if (acl && acl->a_count > NFS3_ACL_MAX_ENTRIES)
+	if (acl && acl->a_count > nfs3_acl_max_entries)
 		return -EINVAL;
 	args.inode = inode;
 	args.mask = NFS3_ACL|NFS3_DFACL;
--- linux-2.4.21-20.EL-ace/fs/nfs/nfs3xdr.c.orig	2004-08-18 20:30:26.000000000 -0400
+++ linux-2.4.21-20.EL-ace/fs/nfs/nfs3xdr.c	2004-09-07 13:13:20.000000000 -0400
@@ -695,10 +695,12 @@
 	p = xdr_encode_fhandle(p, NFS_FH(args->inode));
 	*p++ = htonl(args->mask);
 	acl = (args->mask & NFS3_ACL) ? args->acl_access : NULL;
-	if (!(p = nfs_acl_encode(p, end, args->inode, acl, 1, 0)))
+	if (!(p = nfs_acl_encode_limit(p, end, args->inode, acl, 
+			1, 0, nfs3_acl_max_entries)))
 		return -ENOMEM;
 	acl = (args->mask & NFS3_DFACL) ? args->acl_default : NULL;
-	if (!(p = nfs_acl_encode(p, end, args->inode, acl, 1, NFS3_ACL_DEFAULT)))
+	if (!(p = nfs_acl_encode_limit(p, end, args->inode, acl, 
+			1, NFS3_ACL_DEFAULT, nfs3_acl_max_entries)))
 		return -ENOMEM;
 	req->rq_slen = xdr_adjust_iovec(req->rq_svec, p);
 	
@@ -1082,12 +1084,12 @@
 	/* res->acl_{access,default} are released in nfs3_proc_getacl. */
 	acl = (res->mask & NFS3_ACL) ? &res->acl_access : NULL;
 	aclcnt = (res->mask & NFS3_ACLCNT) ? &res->acl_access_count : NULL;
-	if (!(p = nfs_acl_decode(p, end, aclcnt, acl)))
+	if (!(p = nfs_acl_decode_limit(p, end, aclcnt, acl, nfs3_acl_max_entries)))
 		return -EINVAL;
 
 	acl = (res->mask & NFS3_DFACL) ? &res->acl_default : NULL;
 	aclcnt = (res->mask & NFS3_DFACLCNT) ? &res->acl_default_count : NULL;
-	if (!(p = nfs_acl_decode(p, end, aclcnt, acl)))
+	if (!(p = nfs_acl_decode_limit(p, end, aclcnt, acl, nfs3_acl_max_entries)))
 		return -EINVAL;
 	return 0;
 }
@@ -1164,4 +1166,12 @@
 	sizeof(nfs3_acl_procedures)/sizeof(nfs3_acl_procedures[0]),
 	nfs3_acl_procedures
 };
+
+void nfs3_fixup_xdr_tables(unsigned int acl_max)
+{
+	nfs3_acl_procedures[1].p_bufsiz=
+		(((acl_max*3)+2)*2)+1+NFS3_post_op_attr_sz+1;
+	nfs3_acl_procedures[2].p_bufsiz=
+		(((acl_max*3)+2)*2)+1+NFS3_fh_sz;
+}
 #endif  /* CONFIG_NFS_ACL */
--- linux-2.4.21-20.EL-ace/include/linux/nfs3.h.orig	2004-08-18 20:30:26.000000000 -0400
+++ linux-2.4.21-20.EL-ace/include/linux/nfs3.h	2004-09-07 13:13:20.000000000 -0400
@@ -111,5 +111,6 @@
 /* Number of 32bit words in post_op_attr */
 #define NFS3_POST_OP_ATTR_WORDS		22
 
+extern unsigned int nfs3_acl_max_entries;
 #endif /* __KERNEL__ */
 #endif /* _LINUX_NFS3_H */
--- linux-2.4.21-20.EL-ace/include/linux/solaris_acl.h.orig	2004-08-18 20:31:33.000000000 -0400
+++ linux-2.4.21-20.EL-ace/include/linux/solaris_acl.h	2004-09-07 17:04:21.000000000 -0400
@@ -13,4 +13,21 @@
 u32 *nfs_acl_encode(u32 *, u32 *, struct inode *, struct posix_acl *, int, int);
 u32 *nfs_acl_decode(u32 *, u32 *, unsigned int *, struct posix_acl **);
 
+static __inline__ u32 *nfs_acl_encode_limit(u32 *p, u32 *end, struct inode *inode, struct posix_acl *acl,
+	int encode_entries, int typeflag, unsigned int acl_limit)
+{
+	int entries = acl ? acl->a_count : 0;
+	if(entries >= acl_limit)
+		return NULL;
+	return nfs_acl_encode(p,end,inode,acl,encode_entries,typeflag);
+}
+
+static __inline__ u32 *nfs_acl_decode_limit(u32 *p, u32 *end, unsigned int *aclcnt, struct posix_acl **pacl, unsigned int acl_limit)
+{
+	int entries = ntohl(*p);
+	
+	if(entries >= acl_limit)
+		return NULL;
+	return nfs_acl_decode(p,end,aclcnt,pacl);
+}
 #endif  /* __LINUX_SOLARIS_ACL_H */
linux-2.4.21-20.EL-nfsd-ace.patch (text/plain, 4.9 KB)
only in patch2:
unchanged:
--- linux-2.4.21-20.EL-ace/fs/nfsd/nfs3proc.c.orig	2004-08-18 20:30:26.000000000 -0400
+++ linux-2.4.21-20.EL-ace/fs/nfsd/nfs3proc.c	2004-09-07 13:13:29.000000000 -0400
@@ -798,4 +798,10 @@
   PROC(getacl,	 getacl,	getacl,		getacl,  RC_NOCACHE, ST+1+2*(1+ACL)),
   PROC(setacl,	 setacl,	setacl,		fhandle, RC_NOCACHE, ST+pAT)
 };
+
+void nfs3_fixup_proc_tables(unsigned int acl_max)
+{
+	nfsd_acl_procedures3[1].pc_xdrressize=(((acl_max*3)+2)*2)+1+ST;
+}
+
 #endif  /* CONFIG_NFSD_ACL */
only in patch2:
unchanged:
--- linux-2.4.21-20.EL-ace/fs/nfsd/nfs3xdr.c.orig	2004-08-18 20:30:27.000000000 -0400
+++ linux-2.4.21-20.EL-ace/fs/nfsd/nfs3xdr.c	2004-09-07 13:13:29.000000000 -0400
@@ -543,11 +543,11 @@
 	
 	/* argp->acl_{access,default} are released in nfsd3_proc_setacl. */
 	acl = (args->mask & NFS3_ACL) ? &args->acl_access : NULL;
-	if (!(p = nfs_acl_decode(p, end, NULL, acl)))
+	if (!(p = nfs_acl_decode_limit(p, end, NULL, acl, nfsd3_acl_max_entries)))
 		return 0;
 	
 	acl = (args->mask & NFS3_DFACL) ? &args->acl_default : NULL;
-	if (!(p = nfs_acl_decode(p, end, NULL, acl))) {
+	if (!(p = nfs_acl_decode_limit(p, end, NULL, acl, nfsd3_acl_max_entries))) {
 		posix_acl_release(args->acl_access);
 		args->acl_access = NULL;
 		return 0;
@@ -904,12 +904,12 @@
 		struct inode *inode = dentry->d_inode;
 
 		*p++ = htonl(resp->mask);
-		if (!(p = nfs_acl_encode(p, end, inode, resp->acl_access,
-				   resp->mask & NFS3_ACL, 0)))
+		if (!(p = nfs_acl_encode_limit(p, end, inode, resp->acl_access,
+				   resp->mask & NFS3_ACL, 0, nfsd3_acl_max_entries)))
 			return 0;
-		if (!(p = nfs_acl_encode(p, end, inode, resp->acl_default,
+		if (!(p = nfs_acl_encode_limit(p, end, inode, resp->acl_default,
 		    			 resp->mask & NFS3_DFACL,
-					 NFS3_ACL_DEFAULT)))
+					 NFS3_ACL_DEFAULT, nfsd3_acl_max_entries)))
 			return 0;
 	}
 
only in patch2:
unchanged:
--- linux-2.4.21-20.EL-ace/fs/nfsd/nfsctl.c.orig	2004-08-18 20:30:27.000000000 -0400
+++ linux-2.4.21-20.EL-ace/fs/nfsd/nfsctl.c	2004-09-07 17:43:19.000000000 -0400
@@ -23,9 +23,11 @@
 #include <linux/slab.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
+#include <linux/sysctl.h>
 
 #include <linux/nfs.h>
 #include <linux/sunrpc/svc.h>
+#include <linux/solaris_acl.h>
 #include <linux/nfsd/nfsd.h>
 #include <linux/nfsd/cache.h>
 #include <linux/nfsd/xdr.h>
@@ -36,6 +38,13 @@
 #include <linux/smp_lock.h>
 #include <linux/init.h>
 
+#ifdef CONFIG_NFSD_ACL
+unsigned int nfsd3_acl_max_entries=NFS3_ACL_MAX_ENTRIES;
+MODULE_PARM(nfsd3_acl_max_entries, "i");
+MODULE_PARM_DESC(nfsd3_acl_max_entries, " max ace objects to put in an \
+acl response");
+#endif
+ 
 static int	nfsctl_svc(struct nfsctl_svc *data);
 static int	nfsctl_addclient(struct nfsctl_client *data);
 static int	nfsctl_delclient(struct nfsctl_client *data);
@@ -50,6 +59,42 @@
 static int	nfsctl_ugidupdate(struct nfsctl_ugidmap *data);
 #endif
 
+static struct ctl_table_header *nfsd_sysctl_table;
+#define CTL_UNNUMBERED          -2
+static ctl_table nfsd_sysctl_files[] = {
+#ifdef CONFIG_NFSD_ACL
+	{
+		.ctl_name	= CTL_UNNUMBERED,
+		.procname	= "nfsd3_acl_max_entries",
+		.data		= &nfsd3_acl_max_entries,
+		.maxlen		= sizeof(int),
+		.mode		= 0444,
+		.proc_handler	= &proc_dointvec,
+	},
+#endif
+	{ .ctl_name = 0 }
+};
+
+static ctl_table nfsd_sysctl_dir[] = {
+	{
+		.ctl_name	= CTL_UNNUMBERED,
+		.procname	= "nfsd",
+		.mode		= 0555,
+		.child		=nfsd_sysctl_files,
+	},
+	{ .ctl_name = 0 }
+};
+
+static ctl_table nfsd_sysctl_root[] = {
+        {
+                .ctl_name       = CTL_FS,
+                .procname       = "fs",
+                .mode           = 0555,
+                .child          = nfsd_sysctl_dir,
+        },
+        { .ctl_name = 0 }
+};
+
 extern struct seq_operations nfs_exports_op;
 static int exports_open(struct inode *inode, struct file *file)
 {
@@ -349,11 +394,15 @@
 #ifdef MODULE
 	nfsd_linkage = &nfsd_linkage_s;
 #endif
+#ifdef CONFIG_NFSD_ACL
+	nfs3_fixup_proc_tables(nfsd3_acl_max_entries);
+#endif
 	nfsd_stat_init();	/* Statistics */
 	nfsd_cache_init();	/* RPC reply cache */
 	nfsd_export_init();	/* Exports table */
 	nfsd_lockd_init();	/* lockd->nfsd callbacks */
 	proc_export_init();
+	nfsd_sysctl_table = register_sysctl_table(nfsd_sysctl_root,0);
 	return 0;
 }
 
@@ -366,6 +415,8 @@
 #ifdef MODULE
 	nfsd_linkage = NULL;
 #endif
+	if(nfsd_sysctl_table)
+		unregister_sysctl_table(nfsd_sysctl_table);
 	nfsd_export_shutdown();
 	nfsd_cache_shutdown();
 	remove_proc_entry("fs/nfs/exports", NULL);
only in patch2:
unchanged:
--- linux-2.4.21-20.EL-ace/include/linux/nfsd/nfsd.h.orig	2004-08-18 20:31:33.000000000 -0400
+++ linux-2.4.21-20.EL-ace/include/linux/nfsd/nfsd.h	2004-09-07 17:04:21.000000000 -0400
@@ -74,6 +74,7 @@
 #ifdef CONFIG_NFSD_ACL
 extern struct svc_procedure	nfsd_acl_procedures3[];
 extern struct svc_program	nfsd_acl_program;
+extern unsigned int		nfsd3_acl_max_entries;
 #endif /* CONFIG_NFSD_ACL */
 
 /*