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

[Bernhard Voelker]

On 02/17/2012 05:20 PM, Jim Meyering wrote:
> Good idea.
> And with that, we can also drop the eval "...".

Great.

> I have a slight preference for writing the file only from the destructor.
> WDYT?

You convinced me ... but unfortunately not my PC.

See the attached getxattr-speedup.log.gz - I added a "cat k.c" so that
you can check that I got your patch right.
It seems that GCC doesn't like our "__attribute__((destructor)) void p()".
It basically works in a standalone program, but not with LD_PRELOAD.
I didn't find much about this. Can this be a 64-bit issue?

I worked on another alternative: using atexit(). I had to add -lc
to pull atexit from glibc, of course. And I made p() and count_1()
static to avoid potential name clashes with original ls symbols.
Finally, I changed the comment "run the test, redirecting", as
we're obvisouly no longer redirecting. ;-)
What do you think?

Ah yes, and another aspect came to my mind: the test does not
cover the other side of the changes in ls.c: it maybe should
prove that ls tries to get the attributes again when the cache
functions start having success for another file, i.e. that we
didn't break the normal XATTR, CAP and ACL queries. Is this
covered by other tests?

Have a nice weekend,
Berny


>From 31c4976c5787222c48b7a27e7c7dfe983a1a40ad Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sat, 18 Feb 2012 01:30:26 +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 |   64 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 0 deletions(-)
 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..8166af1
--- /dev/null
+++ b/tests/ls/getxattr-speedup
@@ -0,0 +1,64 @@
+#!/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"
+# after ls has terminated via a function registered with atexit.
+cat > k.c <<'EOF' || framework_failure_
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+static unsigned long int n_calls;
+static void p()
+{
+  FILE *fp = fopen ("x", "w"); if (!fp) return;
+  fprintf (fp, "%lu\n", n_calls);
+  fclose (fp);
+}
+static ssize_t count_1 (void)
+{
+  if (!n_calls)
+    atexit (p);
+  ++n_calls; errno = ENOTSUP; return -1;
+}
+ssize_t getxattr (const char *path, const char *name, void *value, size_t size)
+{ return count_1 (); }
+ssize_t lgetxattr(const char *path, const char *name, void *value, size_t size)
+{ return count_1 (); }
+EOF
+
+# Then compile/link it:
+$CC -fPIC -O2 -c k.c || framework_failure_ 'failed to compile with -fPIC'
+ld -G k.o -lc -o k.so || framework_failure_ 'failed to invoke ld -G ...'
+
+# create a few files
+seq 100 | xargs touch || framework_failure_
+
+# Finally, run the test, with the stubs and counting from above.
+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.7

getxattr-speedup.log.gz
Description: GNU Zip compressed data