Re: "ls -l": Avoid unnecessary getxattr() overhead

[Jim Meyering]

Bernhard Voelker wrote:

> On 02/17/2012 04:15 PM, Jim Meyering wrote:
>> And here's the patch I've just squashed into the test c-set.
>>
>> diff --git a/tests/ls/getxattr-speedup b/tests/ls/getxattr-speedup
>> index eaa3342..a3d47c1 100755
>> --- a/tests/ls/getxattr-speedup
>> +++ b/tests/ls/getxattr-speedup
>> @@ -28,16 +28,18 @@ fd=33
>>  # determine the total number of calls.  FIXME: there must be a better way.
>>  cat > k.c <<'EOF' || framework_failure_
>>  #include <errno.h>
>> -#include <unistd.h>
>> -static void
>> -write_1 (void)
>> -{
>> -  write (FD, "x\n", 2);
>> -}
>> +#include <stdio.h>
>> +static unsigned long int n_calls;
>>  ssize_t getxattr (const char *path, const char *name, void *value, size_t 
>> size)
>> -{ write_1 (); errno = ENOTSUP; return -1; }
>> +{ ++n_calls; errno = ENOTSUP; return -1; }
>>  ssize_t lgetxattr(const char *path, const char *name, void *value, size_t 
>> size)
>> -{ write_1 (); errno = ENOTSUP; return -1; }
>> +{ ++n_calls; errno = ENOTSUP; return -1; }
>> +__attribute__((destructor)) void p()
>> +{
>> +  FILE *fp = fdopen (FD, "a"); if (!fp) return;
>> +  fprintf (fp, "%lu\n", n_calls);
>> +  fclose (fp);
>> +}
>>  EOF
>>
>>  # Then compile/link it:
>> @@ -52,7 +54,7 @@ seq 100 | xargs touch || framework_failure_
>>  eval "LD_PRELOAD=./k.so ls --color=always -l . $fd>x" || fail=1
>>
>>  # Ensure that there were no more than 3 *getxattr calls.
>> -n_calls=$(wc -l < x)
>> +n_calls=$(cat x)
>>  test "$n_calls" -le 3 || fail=1
>>
>>  Exit $fail
>
> Hmm, this doesn't work here - "x" is not created :-(
>
> ++ cat x
> + n_calls=
> + test '' -le 3
> ./ls/getxattr-speedup: line 58: test: : integer expression expected
>
> gcc (SUSE Linux) 4.6.2
> openSUSE 12.1 (x86_64)
>
> Another suggestion: writing into "x" in write_1(). By this, you can
> also get rid of fd=33 ... admittedly to the cost of maybe several
> fopen calls. On my system, n_calls doesn't get more than 1.
> We could yet s/seq 100/seq 20/.
>
>
> diff --git a/tests/ls/getxattr-speedup b/tests/ls/getxattr-speedup
> index ecdd126..60a72ed 100755
> --- a/tests/ls/getxattr-speedup
> +++ b/tests/ls/getxattr-speedup
> @@ -21,18 +21,18 @@
>  . "${srcdir=.}/init.sh"; path_prepend_ ../src
>  print_ver_ ls
>
> -fd=33
> -
>  # Replace each getxattr and lgetxattr call with a call to these stubs.
> -# Each writes a single line to the specified FD so that we can later
> -# determine the total number of calls.  FIXME: there must be a better way.
> +# Each writes the number of calls into "x".
>  cat > k.c <<'EOF' || framework_failure_
>  #include <errno.h>
> -#include <unistd.h>
> +#include <stdio.h>
>  static void
>  write_1 (void)
>  {
> -  write (FD, "x\n", 2);
> +  static unsigned long int n_calls;
> +  ++n_calls;
> +  FILE *fp = fopen ("x", "w"); if (!fp) return;
> +  fprintf (fp, "%lu\n", n_calls);
>  }
>  ssize_t getxattr (const char *path, const char *name, void *value, size_t 
> size)
>  { write_1 (); errno = ENOTSUP; return -1; }
> @@ -41,7 +41,7 @@ ssize_t lgetxattr(const char *path, const char *name, void 
> *value, size_t size)
>  EOF
>
>  # Then compile/link it:
> -$CC -DFD=$fd -fPIC -O2 -c k.c \
> +$CC -fPIC -O2 -c k.c \
>    || framework_failure_ 'failed to compile with -fPIC'
>  ld -G k.o -o k.so || framework_failure_ 'failed to invoke ld -G ...'
>
> @@ -49,10 +49,10 @@ ld -G k.o -o k.so || framework_failure_ 'failed to invoke 
> ld -G ...'
>  seq 100 | xargs touch || framework_failure_
>
>  # Finally, run the test, redirecting
> -eval "LD_PRELOAD=$PWD/k.so ls --color=always -l . $fd>x" || fail=1
> +eval "LD_PRELOAD=./k.so ls --color=always -l ." || fail=1

Good idea.
And with that, we can also drop the eval "...".
I have a slight preference for writing the file only from the destructor.
WDYT?

>From db3e5c56bb191c5991d2852d4396517e844bb8f3 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 13 Feb 2012 12:52:19 +0100
Subject: [PATCH] tests: test for ls speed-up

* tests/ls/getxattr-speedup: New test.
* tests/Makefile.am (TESTS): Add it.
Improved-by: Bernhard Voelker.
---
 tests/Makefile.am         |    1 +
 tests/ls/getxattr-speedup |   57 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)
 create mode 100755 tests/ls/getxattr-speedup

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 7b53681..625b636 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -426,6 +426,7 @@ TESTS =                                             \
   ls/dired                                     \
   ls/file-type                                 \
   ls/follow-slink                              \
+  ls/getxattr-speedup                          \
   ls/infloop                                   \
   ls/inode                                     \
   ls/m-option                                  \
diff --git a/tests/ls/getxattr-speedup b/tests/ls/getxattr-speedup
new file mode 100755
index 0000000..927cbe8
--- /dev/null
+++ b/tests/ls/getxattr-speedup
@@ -0,0 +1,57 @@
+#!/bin/sh
+# Show that we've eliminated most of ls' failing getxattr syscalls,
+# regardless of how many files are in a directory we list.
+# This test is skipped on systems that lack LD_PRELOAD support; that's fine.
+
+# Copyright (C) 2012 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/init.sh"; path_prepend_ ../src
+print_ver_ ls
+
+# Replace each getxattr and lgetxattr call with a call to these stubs.
+# Count those and write the total number of calls to the file "x"
+# via a global destructor.
+cat > k.c <<'EOF' || framework_failure_
+#include <errno.h>
+#include <stdio.h>
+static unsigned long int n_calls;
+ssize_t getxattr (const char *path, const char *name, void *value, size_t size)
+{ ++n_calls; errno = ENOTSUP; return -1; }
+ssize_t lgetxattr(const char *path, const char *name, void *value, size_t size)
+{ ++n_calls; errno = ENOTSUP; return -1; }
+__attribute__((destructor)) void p()
+{
+  FILE *fp = fopen ("x", "w"); if (!fp) return;
+  fprintf (fp, "%lu\n", n_calls);
+  fclose (fp);
+}
+EOF
+
+# Then compile/link it:
+$CC -fPIC -O2 -c k.c || framework_failure_ 'failed to compile with -fPIC'
+ld -G k.o -o k.so || framework_failure_ 'failed to invoke ld -G ...'
+
+# create a few files
+seq 100 | xargs touch || framework_failure_
+
+# Finally, run the test, redirecting
+LD_PRELOAD=./k.so ls --color=always -l . || fail=1
+
+# Ensure that there were no more than 3 *getxattr calls.
+n_calls=$(cat x)
+test "$n_calls" -le 3 || fail=1
+
+Exit $fail
--
1.7.9.1.245.gbbe07