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

[Jim Meyering]

Sven Breuner wrote:
> Pádraig Brady wrote on 02/06/2012 09:06 PM:
>> This seems to have some related info.
>> https://bugzilla.redhat.com/show_bug.cgi?id=662011
>> We might be able to cache something.
>> We'll investigate.
>
> Thanks, Pádraig.
>
> I assume that bugzilla report is only related on a very abstract level:
> The reporter of that ticket is suggesting special behavior after a
> getxattr(acl_stuff) call returned ENODATA, in which case following
> getxattr(acl_stuff) calls might of course have other results.
>
> I'm referring to getxattr(acl_stuff) calls that return EOPNOTSUPP,
> because the file system does not support ACLs / extended attributes,
> in which case all following getxattr(acl_stuff) calls will definitely
> have the same result (plus the other lgetxattr(selinux_stuff) calls,
> which simply cannot provide any interesting information when selinux
> is completely disabled on a machine).

Here's a proof-of-concept patch:
When, for a given directory (being read via readdir),
lgetfilecon fails with ENOTSUP or EOPNOTSUPP the first
time we call it on a file, we skip that function for
all remaining entries in the same directory (but only at the same level).
Technically, we could probably exempt all files on that device, but
until/unless we convert ls.c to use fts, it's probably not worth
trying to do that.

Even this tiny patch...
I'm not sure it's worth it.  What ls.c really needs is a revamp,
to make it use fts like all other dir-tree traversal programs in
coreutils.  However, that is sure to be a big task.

It avoids 33% of the *getxattr calls here, and results
in that saving only when not following symlinks.

diff --git a/src/ls.c b/src/ls.c
index f5cd37a..4ee8cef 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -244,7 +244,8 @@ static size_t quote_name (FILE *out, const char *name,
 static char *make_link_name (char const *name, char const *linkname);
 static int decode_switches (int argc, char **argv);
 static bool file_ignored (char const *name);
-static uintmax_t gobble_file (char const *name, enum filetype type,
+static uintmax_t gobble_file (bool first_entry, char const *name,
+                              enum filetype type,
                               ino_t inode, bool command_line_arg,
                               char const *dirname);
 static bool print_color_indicator (const struct fileinfo *f,
@@ -1388,13 +1389,13 @@ main (int argc, char **argv)
   if (n_files <= 0)
     {
       if (immediate_dirs)
-        gobble_file (".", directory, NOT_AN_INODE_NUMBER, true, "");
+        gobble_file (true, ".", directory, NOT_AN_INODE_NUMBER, true, "");
       else
         queue_directory (".", NULL, true);
     }
   else
     do
-      gobble_file (argv[i++], unknown, NOT_AN_INODE_NUMBER, true, "");
+      gobble_file (true, argv[i++], unknown, NOT_AN_INODE_NUMBER, true, "");
     while (i < argc);

   if (cwd_n_used)
@@ -2563,6 +2564,7 @@ print_dir (char const *name, char const *realname, bool 
command_line_arg)

   clear_files ();

+  bool first_entry = true;
   while (1)
     {
       /* Set errno to zero so we can distinguish between a readdir failure
@@ -2590,9 +2592,10 @@ print_dir (char const *name, char const *realname, bool 
command_line_arg)
 # endif
                 }
 #endif
-              total_blocks += gobble_file (next->d_name, type,
+              total_blocks += gobble_file (first_entry, next->d_name, type,
                                            RELIABLE_D_INO (next),
                                            false, name);
+              first_entry = false;

               /* In this narrow case, print out each name right away, so
                  ls uses constant memory while processing the entries of
@@ -2777,13 +2780,23 @@ clear_files (void)
   file_size_width = 0;
 }

+static bool
+errno_unsupported (int err)
+{
+  return err == ENOTSUP || err == EOPNOTSUPP;
+}
+
 /* Add a file to the current table of files.
    Verify that the file exists, and print an error message if it does not.
-   Return the number of blocks that the file occupies.  */
+   Return the number of blocks that the file occupies.
+   Set FIRST_ENTRY to false for second and subsequent entries when
+   calling from a readdir loop.  Otherwise, set it to true.  This permits
+   an optimization (when false) to avoid repeatedly calling a function that
+   fails with an indication of no support.  */

 static uintmax_t
-gobble_file (char const *name, enum filetype type, ino_t inode,
-             bool command_line_arg, char const *dirname)
+gobble_file (bool first_entry, char const *name, enum filetype type,
+             ino_t inode, bool command_line_arg, char const *dirname)
 {
   uintmax_t blocks = 0;
   struct fileinfo *f;
@@ -2798,6 +2811,10 @@ gobble_file (char const *name, enum filetype type, ino_t 
inode,
       cwd_n_alloc *= 2;
     }

+  static bool first_lgetfilecon = true;
+  if (first_entry)
+    first_lgetfilecon = true;
+
   f = &cwd_file[cwd_n_used];
   memset (f, '\0', sizeof *f);
   f->stat.st_ino = inode;
@@ -2918,10 +2935,17 @@ gobble_file (char const *name, enum filetype type, 
ino_t inode,
         {
           bool have_selinux = false;
           bool have_acl = false;
-          int attr_len = (do_deref
-                          ?  getfilecon (absolute_name, &f->scontext)
-                          : lgetfilecon (absolute_name, &f->scontext));
+          static int prev_errno;
+          int attr_len =
+            (do_deref
+             ? getfilecon (absolute_name, &f->scontext)
+             : (( ! command_line_arg && ! first_lgetfilecon
+                  && errno_unsupported (prev_errno))
+                ? (errno = ENOTSUP), -1
+                : lgetfilecon (absolute_name, &f->scontext)));
+          first_lgetfilecon = false;
           err = (attr_len < 0);
+          prev_errno = err ? errno : 0;

           if (err == 0)
             have_selinux = ! STREQ ("unlabeled", f->scontext);