All of lore.kernel.org
 help / color / mirror / Atom feed
* setfiles and restorecon performance patch
@ 2006-01-03  8:59 Russell Coker
  2006-01-03 15:55 ` Steve G
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Russell Coker @ 2006-01-03  8:59 UTC (permalink / raw)
  To: SE-Linux, Daniel Walsh, Stephen C. Tweedie

[-- Attachment #1: Type: text/plain, Size: 4539 bytes --]

The attached patch (against policycoreutils-1.29.2-8 from rawhide) makes 
setfiles and restorecon fork off a child process to stat the files before the 
main process gets to them.

Previously the setfiles process would stat a file (often waiting for several 
disk seeks to get the data) then do some regex operations while the disk was 
idle, this was inefficient.  Now a child process will be reading all the data 
from disk (and experiencing the delays from disk seeks) while the parent will 
be doing the regex operations thus making more efficient use of the machine.

The child process will write some data to a pipe after each file has been 
stat'd, when the pipe buffer (8K) is full the child will block (this prevents 
the child from getting too far ahead of the parent - usually disk is faster 
than CPU for this).  If the child got too far ahead of the parent then the 
files it stat'd could have their Inode/dentry data discarded from the cache 
before the parent accessed them.  At the moment I have 8 bytes sent through 
the pipe per file so that the child is at most 1024 files ahead.

Thanks to Steve Tweedie for the idea.  He initially suggested this a long time 
ago (before I wrote the stem compression optimisation) and at that time disk 
IO was not a significant factor in performance (setfiles used 99% CPU time on 
most commonly available machines) so there would have been no benefit in 
implementing it then.

Here are the times for restorecon on a Compaq desktop machine with P4-1.5GHz, 
256M of 133MHz RAM, and a cheap 20G IDE disk.  Tests were performed on 
the /usr tree.

Original:
78.03user 21.77system 2:07.12elapsed 78%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+3136minor)pagefaults 0swaps

first new ver:
78.66user 15.90system 1:41.25elapsed 93%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+3129minor)pagefaults 0swaps

Reduced the elapsed time by 20.5%!  Of course if I had a server class machine 
with 15,000rpm disks then the benefit would be a lot less.  OTOH if I tested 
on a new laptop then the benefit would probably be greater.

second new ver:
77.64user 16.10system 1:39.10elapsed 94%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+3125minor)pagefaults 0swaps

The second version was after optimising the code in terms of using strdupa(), 
removing some strlen() type operations, etc.  The difference is hardly 
noticable as those optimisations weren't on the most critical paths.  Still 
worth having IMHO.


Here are the times for setfiles on the same machine:
old ver:
78.66user 14.98system 1:59.56elapsed 78%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+4340minor)pagefaults 0swaps

new ver (8):
78.50user 9.40system 1:34.16elapsed 93%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+5065minor)pagefaults 0swaps

21% reduction in elapsed time!

new ver (1):
77.63user 9.42system 1:33.53elapsed 93%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+5064minor)pagefaults 0swaps

Now I repeated my test but wrote a byte at a time instead of 8 bytes at a time 
to the pipe, this meant that up to 8192 files could be cached ahead of the 
main process.  The (8) time is for 8 bytes per operation and the (1) time is 
for one byte per operation.  The slight performance benefit reported by the 
(1) time is not noteworthy as I only did each test once, due to the margin of 
error I could get results pointing the other way when there's less than 1% 
difference.  The issue of whether we use 8 or 1 byte for pipe operations 
probably has to be a judgement call of whether 8192 or 1024 files is the best 
fit for the cache of commonly available machines (or course 2048 and 4096 are 
also options that can be considered).

Stephen, please let me know what your instinct is regarding cache sizes for 
this.

I believe that this patch is worthy of inclusion regardless of the issue of 
how many bytes we write to the pipe.  We can always fine-tune this later.

Also Dan please let me know when you have built an RPM of this and new RPMs of 
the SE Linux libraries (based on our discussion of performance patches for 
them).  I have some other ideas for improvements in this area but I want to 
get the current patches all sorted out before I start work.

-- 
http://www.coker.com.au/selinux/   My NSA Security Enhanced Linux packages
http://www.coker.com.au/bonnie++/  Bonnie++ hard drive benchmark
http://www.coker.com.au/postal/    Postal SMTP/POP benchmark
http://www.coker.com.au/~russell/  My home page

[-- Attachment #2: setf.diff --]
[-- Type: text/x-diff, Size: 10831 bytes --]

diff -rup policycoreutils-1.29.2.orig/restorecon/restorecon.c policycoreutils-1.29.2/restorecon/restorecon.c
--- policycoreutils-1.29.2.orig/restorecon/restorecon.c	2005-12-15 06:16:56.000000000 +1100
+++ policycoreutils-1.29.2/restorecon/restorecon.c	2006-01-03 18:50:29.000000000 +1100
@@ -23,7 +23,9 @@
  *
  */
 
+#define _GNU_SOURCE
 #include <stdio.h>
+#include <stdio_ext.h>
 #include <errno.h>
 #include <string.h>
 #include <stdlib.h>
@@ -45,6 +47,8 @@ static char *progname;
 static int errors=0;
 static int recurse=0;
 static int force=0;
+#define STAT_BLOCK_SIZE 8
+static int pipe_fds[2] = { -1, -1 };
 
 #define MAX_EXCLUDES 100
 static int excludeCtr=0;
@@ -115,21 +119,15 @@ void usage(const char * const name)
 	  "usage:  %s [-rRnv] [-e excludedir ] [-o filename ] [-f filename | pathname... ]\n",  name);
   exit(1);
 }
-int restore(char *filename) {
+/* filename has trailing '/' removed by nftw or other calling code */
+int restore(const char *filename) {
   int retcontext=0;
   int retval=0;
   security_context_t scontext=NULL;
   security_context_t prev_context=NULL;
-  int len=strlen(filename);
   struct stat st;
   char path[PATH_MAX+1];
   int user_only_changed=0;
-  /* 
-     Eliminate trailing /
-  */
-  if (len > 0 && filename[len-1]=='/' && (strcmp(filename,"/") != 0)) {
-    filename[len-1]=0;
-  }
 
   if (excludeCtr > 0 && exclude(filename)) {
       return 0;
@@ -143,7 +141,7 @@ int restore(char *filename) {
     if (verbose>1)
       fprintf(stderr,"Warning! %s refers to a symbolic link, not following last component.\n", filename);
     char *p = NULL, *file_sep;
-    char *tmp_path = strdup(filename);
+    char *tmp_path = strdupa(filename);
     if (!tmp_path) {
       fprintf(stderr,"strdup on %s failed:  %s\n", filename,strerror(errno));
       return 1;
@@ -155,14 +153,16 @@ int restore(char *filename) {
       file_sep++;
       p = realpath(tmp_path, path);
     }
-    if (!p || strlen(path) + strlen(file_sep) + 1 > PATH_MAX) {
+    size_t len = strlen(p);
+    if (!p || len + strlen(file_sep) + 2 > PATH_MAX) {
       fprintf(stderr,"realpath(%s) failed %s\n", filename, strerror(errno));
-      free(tmp_path);
       return 1;
     }
-    sprintf(p + strlen(p), "/%s", file_sep);
+    p += len;
+    *p = '/';
+    p++;
+    strcpy(p, file_sep);
     filename = p;
-    free(tmp_path);
   } else {
     char *p;
     p = realpath(filename, path);
@@ -182,10 +182,6 @@ int restore(char *filename) {
     fprintf(stderr,"matchpathcon(%s) failed %s\n", filename,strerror(errno));
     return 1;
   } 
-  if (strcmp(scontext,"<<none>>")==0) {
-    freecon(scontext);
-    return 0;
-  }
   retcontext=lgetfilecon(filename,&prev_context);
   
   if (retcontext >= 0 || errno == ENODATA) {
@@ -230,38 +226,80 @@ int restore(char *filename) {
   freecon(scontext);
   return errors;
 }
+
+static int pre_stat(const char *file_unused __attribute__((unused)),
+		      const struct stat *sb_unused __attribute__((unused)),
+		      int flag_unused __attribute__((unused)),
+		      struct FTW *s_unused __attribute__((unused)))
+{
+  char buf[STAT_BLOCK_SIZE];
+  if(write(pipe_fds[1], buf, STAT_BLOCK_SIZE) != STAT_BLOCK_SIZE)
+  {
+    fprintf(stderr, "Error writing to stat pipe, child exiting.\n");
+    exit(1);
+  }
+  return 0;
+}
+
 static int apply_spec(const char *file,
 		      const struct stat *sb_unused __attribute__((unused)),
 		      int flag,
 		      struct FTW *s_unused __attribute__((unused)))
 {
+	char buf[STAT_BLOCK_SIZE];
+	if(pipe_fds[0] != -1 && read(pipe_fds[0], buf, STAT_BLOCK_SIZE) != STAT_BLOCK_SIZE)
+	{
+		fprintf(stderr, "Read error on pipe.\n");
+		pipe_fds[0] = -1;
+	}
 	if (flag == FTW_DNR) {
 		fprintf(stderr, "%s:  unable to read directory %s\n",
 			progname, file);
 		return 0;
 	}
-	errors=errors+restore((char *)file);
+	errors=errors+restore(file);
 	return 0;
 }
 void process(char *buf) {
+      int rc;
       if (recurse) {
-	if (nftw
-	    (buf, apply_spec, 1024, FTW_PHYS)) {
-	  fprintf(stderr,
-		  "%s:  error while labeling files under %s\n",
+	if(pipe(pipe_fds) == -1)
+	  rc = -1;
+	else
+	  rc = fork();
+	if(rc == 0)
+	{
+	  close(pipe_fds[0]);
+	  nftw(buf, pre_stat, 1024, FTW_PHYS);
+	  exit(1);
+	}
+	if(rc > 0)
+	  close(pipe_fds[1]);
+	if(rc == -1 || rc > 0) {
+	  if (nftw(buf, apply_spec, 1024, FTW_PHYS)) {
+	    fprintf(stderr, "%s:  error while labeling files under %s\n",
 		  progname, buf);
-	  errors++;
+	    errors++;
+	  }
 	}
       }
       else
-	errors=errors+restore(buf);
+      {
+	/* Eliminate trailing / */
+	size_t len = strlen(buf);
+	if (len > 1 && buf[len - 1] == '/') {
+	  buf[len - 1] = 0;
+	}
+	errors = errors + restore(buf);
+      }
 }
 int main(int argc, char **argv) {
   int i=0;
   char *file_name=NULL;
   int file=0;
   int opt;
-  char buf[PATH_MAX];
+  char *buf = NULL;
+  size_t buf_len;
 
   memset(excludeArray,0, sizeof(excludeArray));
 
@@ -269,8 +307,6 @@ int main(int argc, char **argv) {
   if (is_selinux_enabled() <= 0 )
     exit(0);
 
-  memset(buf,0, sizeof(buf));
-
   while ((opt = getopt(argc, argv, "FrRnvf:o:e:")) > 0) {
     switch (opt) {
     case 'n':
@@ -293,6 +329,7 @@ int main(int argc, char **argv) {
 		 optarg, strerror(errno));
 	 usage(argv[0]);
       }
+      __fsetlocking(outfile, FSETLOCKING_BYCALLER);
       break;
     case 'v':
       verbose++;
@@ -307,15 +344,16 @@ int main(int argc, char **argv) {
   }
   if (file) {
     FILE *f=stdin;
+    ssize_t len;
     if (strcmp(file_name,"-")!=0) 
       f=fopen(file_name,"r");
-	
     if (f==NULL) {
       fprintf(stderr,"Unable to open %s: %s\n", file_name, strerror(errno));
       usage(argv[0]);
     }
-    while(fgets(buf,PATH_MAX,f)) {
-      buf[strlen(buf)-1]=0;
+    __fsetlocking(f, FSETLOCKING_BYCALLER);
+    while((len = getline(&buf, &buf_len, f)) != -1) {
+      buf[len - 1] = 0;
       process(buf);
     }
     if (strcmp(file_name,"-")!=0) 
diff -rup policycoreutils-1.29.2.orig/setfiles/setfiles.c policycoreutils-1.29.2/setfiles/setfiles.c
--- policycoreutils-1.29.2.orig/setfiles/setfiles.c	2005-12-15 06:16:57.000000000 +1100
+++ policycoreutils-1.29.2/setfiles/setfiles.c	2006-01-03 18:50:40.000000000 +1100
@@ -62,6 +62,7 @@
 #include <stdlib.h>
 #include <fcntl.h>
 #include <stdio.h>
+#include <stdio_ext.h>
 #include <string.h>
 #include <errno.h>
 #include <ctype.h>
@@ -78,6 +79,8 @@
 static int add_assoc = 1;
 static FILE *outfile=NULL;
 static int force=0;
+#define STAT_BLOCK_SIZE 8
+static int pipe_fds[2] = { -1, -1 };
 
 #define MAX_EXCLUDES 100
 static int excludeCtr=0;
@@ -244,6 +247,12 @@ static int apply_spec(const char *file,
 	int i, j, ret;
 	char *context, *newcon; 
 	int user_only_changed=0;
+	char buf[STAT_BLOCK_SIZE];
+	if(pipe_fds[0] != -1 && read(pipe_fds[0], buf, STAT_BLOCK_SIZE) != STAT_BLOCK_SIZE)
+	{
+		fprintf(stderr, "Read error on pipe.\n");
+		pipe_fds[0] = -1;
+	}
 
 	/* Skip the extra slash at the beginning, if present. */
 	if (file[0] == '/' && file[1] == '/')
@@ -288,17 +297,17 @@ static int apply_spec(const char *file,
 	ret = lgetfilecon(my_file, &context);
 	if (ret < 0) {
 		if (errno == ENODATA) {
-			context = malloc(10);
-			strcpy(context, "<<none>>");
+			context = NULL;
 		} else {
 			perror(my_file);
 			fprintf(stderr, "%s:  unable to obtain attribute for file %s\n", 
 				progname, my_file);
 			goto err;
 		}
+		user_only_changed = 0;
 	}
-
-	user_only_changed=only_changed_user(context, newcon);
+	else
+		user_only_changed=only_changed_user(context, newcon);
 
 	/*
 	 * Do not relabel the file if the matching specification is 
@@ -306,12 +315,12 @@ static int apply_spec(const char *file,
 	 * specification.
 	 */
 	if ((strcmp(newcon, "<<none>>") == 0) || 
-	    (strcmp(context,newcon) == 0)) {
+	    (context && (strcmp(context,newcon) == 0))) {
 		freecon(context);
 		goto out;
 	}
 
-	if (! force && 
+	if (! force && context &&
 	    ( is_context_customizable(context)>0 )) {
 		if (verbose > 1) {
 			fprintf(stderr,"%s: %s not reset customized by admin to %s\n",
@@ -325,20 +334,29 @@ static int apply_spec(const char *file,
 		 * the user has changed but the role and type are the
 		 * same.  For "-vv", emit everything. */
 		if (verbose > 1 || !user_only_changed) {
+		    if(context)
 			printf("%s:  relabeling %s from %s to %s\n", progname,
 			       my_file, context, newcon);
+		    else
+			printf("%s:  labeling %s to %s\n", progname,
+			       my_file, newcon);
 		}
 	}
 
 	if ( log && !user_only_changed ) {
-		syslog(LOG_INFO, "relabeling %s from %s to %s\n", 
-		       my_file, context, newcon);
+		if(context)
+			syslog(LOG_INFO, "relabeling %s from %s to %s\n", 
+				my_file, context, newcon);
+		else
+			syslog(LOG_INFO, "labeling %s to %s\n", 
+				my_file, newcon);
 	}
 
 	if (outfile && !user_only_changed)
 		fprintf(outfile, "%s\n", my_file);
 
-	freecon(context);
+	if(context)
+		freecon(context);
 
 	/*
 	 * Do not relabel the file if -n was used.
@@ -411,6 +429,20 @@ int canoncon(const char *path, unsigned 
 	return !valid;
 }
 
+static int pre_stat(const char *file_unused __attribute__((unused)),
+                    const struct stat *sb_unused __attribute__((unused)),
+                    int flag_unused __attribute__((unused)),
+                    struct FTW *s_unused __attribute__((unused)))
+{
+  char buf[STAT_BLOCK_SIZE];
+  if(write(pipe_fds[1], buf, STAT_BLOCK_SIZE) != STAT_BLOCK_SIZE)
+  {
+    fprintf(stderr, "Error writing to stat pipe, child exiting.\n");
+    exit(1);
+  }
+  return 0;
+}
+
 int main(int argc, char **argv)
 {
 	int opt, rc, i;
@@ -435,6 +467,7 @@ int main(int argc, char **argv)
 					policyfile, strerror(errno));
 				exit(1);
 			}
+			__fsetlocking(policystream, FSETLOCKING_BYCALLER);
 
 			if (sepol_set_policydb_from_file(policystream) < 0) {
 				fprintf(stderr, "Error reading policy %s: %s\n", policyfile,
@@ -474,6 +507,7 @@ int main(int argc, char **argv)
 
 				usage(argv[0]);
 			}
+			__fsetlocking(outfile, FSETLOCKING_BYCALLER);
 			break;
 		case 'q':
 			quiet = 1;
@@ -580,15 +614,31 @@ int main(int argc, char **argv)
 
 		qprintf("%s:  labeling files under %s\n", argv[0],
 			argv[optind]);
-		
+
+		int rc;
+		if(pipe(pipe_fds) == -1)
+			rc = -1;
+		else
+			rc = fork();
+		if(rc == 0)
+		{
+			close(pipe_fds[0]);
+			nftw(argv[optind], pre_stat, 1024, FTW_PHYS);
+			exit(1);
+		}
+		if(rc > 0)
+			close(pipe_fds[1]);
+		if(rc == -1 || rc > 0) {
+
 		/* Walk the file tree, calling apply_spec on each file. */
-		if (nftw
-		    (argv[optind], apply_spec, 1024,
-		     FTW_PHYS | FTW_MOUNT)) {
-			fprintf(stderr,
+			if (nftw
+			    (argv[optind], apply_spec, 1024,
+			     FTW_PHYS | FTW_MOUNT)) {
+				fprintf(stderr,
 				"%s:  error while labeling files under %s\n",
 				argv[0], argv[optind]);
-			exit(1);
+				exit(1);
+			}
 		}
 
 		/*

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-01-13 13:55 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-01-03  8:59 setfiles and restorecon performance patch Russell Coker
2006-01-03 15:55 ` Steve G
2006-01-03 19:08   ` Russell Coker
2006-01-03 20:22     ` Joshua Brindle
2006-01-07  1:19 ` Stephen Tweedie
2006-01-13 13:55 ` Stephen Smalley

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.