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

[Sven Breuner]

Jim Meyering wrote on 02/10/2012 09:16 AM:
> Pádraig Brady wrote:
> > > Current git version patched...
> > >
> > > $ strace -c ~/tmp/ls-8.15.34-31eee_patched -l>/dev/null
> > > % time     seconds  usecs/call     calls    errors syscall
> > > ------ ----------- ----------- --------- --------- ----------------
> > >   45.50    0.313386           5     61441           lstat
> > >   43.63    0.300524           5     61441     61441 getxattr
> > >   10.60    0.072986         973        75           getdents
> >
> > So you're still getting all those errors.
> > Is that from file_has_acl() or cap_get_file()?
> > Oh it must be from file_has_acl() as you've
> > not specified --color. What is the errno in that case?
>
> So we can avoid all but one of those failing getxattr calls
> in this case.

Yes, I would say so, too. If ls gets EOPNOTSUPP from a 
getxattr(acl_stuff) call on a file, it can skip further getxattr calls 
on the corresponding file system.

> It's a little tricky because of how file_has_acl works.
> We may have to set errno=0 just prior, and then test errno
> when file_has_acl returns<=0.  Sven, can you tell us whether
> file_has_acl is returning -1 or 0 in your case?  And confirm
> that the errno value is propagated out to ls.c?
>
> So either print both "n" and "errno" right after this line in ls.c
>
>                int n = file_has_acl (absolute_name,&f->stat);
>
> or inspect those values in gdb?
> If n is always negative for you, then I think we can skip the
> kludgy errno=0 initialization.

I used ls from git with the selinux optimization patch applied and added 
the following printf:

              int n = file_has_acl (absolute_name, &f->stat);
              printf("after file_has_acl(%s): n=%d errno=%d (%s)\n",
                     absolute_name, n, errno, strerror(errno) );


Here's the result from a small test directory:
$ ~/git/coreutils/src/ls -l --color=always
after file_has_acl(file3): n=0 errno=95 (Operation not supported)
after file_has_acl(dirlink): n=0 errno=95 (Operation not supported)
after file_has_acl(file1): n=0 errno=95 (Operation not supported)
after file_has_acl(file2): n=0 errno=95 (Operation not supported)
after file_has_acl(filelink): n=0 errno=95 (Operation not supported)
after file_has_acl(dir): n=0 errno=95 (Operation not supported)
after file_has_acl(dangling_link): n=0 errno=95 (Operation not supported)
total 0
lrwxrwxrwx 1 breuner hpc 6 Feb 12 00:46 dangling_link -> nofile
drwxr-xr-x 2 breuner hpc 0 Feb 12 00:42 dir
lrwxrwxrwx 1 breuner hpc 3 Feb 12 01:32 dirlink -> dir
-rw-r--r-- 1 breuner hpc 0 Feb 12 00:44 file1
-rw-r--r-- 1 breuner hpc 0 Feb 12 00:44 file2
-rw-r--r-- 1 breuner hpc 0 Feb 12 00:44 file3
lrwxrwxrwx 1 breuner hpc 5 Feb 12 00:46 filelink -> file1

=> n is not negative
So that is probably not in sync with the comment of file_has_acl(), 
which says "Return [...] -1 (setting errno) on error."


In case it's relevant: errno after file_has_acl() is not always 
EOPNOTSUPP. Here's the result from a directory with only a single 
dangling symlink in it:

$~/git/coreutils/src/ls -l --color=always
after file_has_acl(link1): n=0 errno=61 (No data available)
total 0
lrwxrwxrwx 1 breuner hpc 5 Feb 12 00:59 link1 -> file1

(Tested with similar results on cifs, ext4, fhgfs.)

Best regards,
Sven