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

[Jim Meyering]

Bernhard Voelker wrote:
> 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?

Is that an open-suse system?
I tried on an opensuse system with 2.6.37.6-0.11-desktop,
and since there were no getxattr calls at all, no wrapper
would run, and hence, your atexit call would never be reached.
I've added this comment at the top:

  # This test is skipped on systems that lack LD_PRELOAD support; that's fine.
+ # Similarly, on a system that lacks getxattr altogether, skipping it is fine.

and this new skip_ use below:

test -f x || skip_ "internal test failure: maybe LD_PRELOAD doesn't work?"

Which may mean this test will simply be skipped on your system.
But that is fine.

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

See above.

> Finally, I changed the comment "run the test, redirecting", as
> we're obvisouly no longer redirecting. ;-)
> What do you think?

Good idea.  I've updated that comment.

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

No, but the dev-caching logic is very simple, so probably not worth
a test for now.

I expect to push this series shortly.


>From b29db6767612cf70e9d4c7eb6ede9822f6173fca 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 |   64 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)
 create mode 100755 tests/ls/getxattr-speedup

diff --git a/tests/Makefile.am b/tests/Makefile.am
index eed6ed6..2fee97d 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -427,6 +427,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..2b7a1f3
--- /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.
+# Similarly, on a system that lacks getxattr altogether, skipping it is 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;
+
+static void __attribute__ ((destructor))
+print_call_count (void)
+{
+  FILE *fp = fopen ("x", "w"); if (!fp) return;
+  fprintf (fp, "%lu\n", n_calls); fclose (fp);
+}
+
+static ssize_t incr () { ++n_calls; errno = ENOTSUP; return -1; }
+ssize_t getxattr (const char *path, const char *name, void *value, size_t size)
+{ return incr (); }
+ssize_t lgetxattr(const char *path, const char *name, void *value, size_t size)
+{ return incr (); }
+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 20 | xargs touch || framework_failure_
+
+# Finally, run the test:
+LD_PRELOAD=./k.so ls --color=always -l . || fail=1
+
+test -f x || skip_ "internal test failure: maybe LD_PRELOAD doesn't work?"
+
+# 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