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

[Jim Meyering]

Pádraig Brady wrote:

> On 02/09/2012 09:17 PM, Sven Breuner wrote:
>> Jim Meyering wrote on 02/09/2012 03:17 PM:
>>> Pádraig Brady wrote:
>>>> On 02/09/2012 11:32 AM, Jim Meyering wrote:
>>>>> Technically, we could probably exempt all files on that device, but
>>>>> ...
>>>>
>>>> So you avoid symlinks as they can point outside the device.
>>>> Unfortunately so can bind mounts, so you probably have to key on device?
>>>
>>> Thanks.  That confirms my impression that my patch was not worthwhile:
>>> It was buggy, too.
>>>
>>> I suppose we could use a little hash table, whose entries are
>>> <dev_t device_number, bool getfilecon_required>  pairs.
>>>
>>> Or even a single static dev_t selinux_challenged_device,
>>> that if equal to f->stat.st_dev, we can skip the *getfilecon call.
>>> That should be good enough for most uses.
>>>
>>> Here's that simpler patch:
>>>
>>> diff --git a/src/ls.c b/src/ls.c
>>> index f5cd37a..cb9f834 100644
>>> [...]
>>
>>
>> Just tried it out and the difference is very significant, as expected.
>> (Test case with 61,441 files in a directory.)
>>
>> Current git version without the patch...
>>
>> $ time ~/tmp/ls-8.15.34-31eee -l >/dev/null
>>
>> real    0m31.135s
>> user    0m0.859s
>> sys     0m4.276s
>>
>> $ strace -c ~/tmp/ls-8.15.34-31eee -l >/dev/null
>> % time     seconds  usecs/call     calls    errors syscall
>> ------ ----------- ----------- --------- --------- ----------------
>>  31.13    0.303648           5     61441           lstat
>>  30.81    0.300585           5     61441     61441 getxattr
>>  30.65    0.298937           5     61441           lgetxattr
>>   7.17    0.069987         933        75           getdents
>> [...]
>>
>> Current git version patched...
>>
>> $ time ~/tmp/ls-8.15.34-31eee_patched -l >/dev/null
>>
>> real    0m21.254s
>> user    0m0.565s
>> sys     0m2.496s
>>
>> $ 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.  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.