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

[Jim Meyering]

Pádraig Brady wrote:
> On 02/19/2012 08:57 AM, Jim Meyering wrote:
>> Sven Breuner wrote:
>>
>>> Jim Meyering wrote on 02/09/2012 03:17 PM:
>>>> Here's that simpler patch:
>>>> [...]
>>>
>>> Jim Meyering wrote on 02/17/2012 01:11 PM:
>>>> Here's the updates series, along with this addition to NEWS:
>>>> [...]
>>>
>>> Thanks for all this optimization work so far!
>>>
>>> My "ls -l" test case with >60K files in a single directory is now down
>>> from about 50s to about 20s already. (When the problem below is
>>> solved, it should be down to about 10s).
>>>
>>> Unfortunately, some getxattr selinux calls seem to be back with the
>>> current coreutils git version.
>>> Those calls were gone after Jim's initial patch from 02/09/2012.
>>>
>>> With only the patch from 02/09/2012 applied, strace showed this output
>>> for the first three files:
>>
>> The initial patch interpreted ENODATA as "not supported",
>> but that shouldn't matter, since we cached the failure only
>> when getfilecon returned -1.  Here, it's always returning 10
>> (sizeof "unlabeled") and hence is not a failure.
>>
>> I think it is legitimate to cache that case (success with "unlabeled")
>> as well.  Presuming I can confirm that, I'll make that change, too.
>
> Hmm I'm not sure.
> Couldn't you get that for some files and not others on some systems?

Technically, you can get a security context of "unlabeled" on one file,
and a regular context for the next file in the same directory, but in
practice it seems to be all or nothing, at a per-device level.
Here's where it'd be nice to have a user-specified format string,
so that those for whom it is inordinately expensive could easily
omit the "alternate access method flag" (the ".", "+", "" or "?" after
the 10-byte type+permission indicator), for which we incur most of this
overhead.

I'm afraid that we can't unilaterally interpret <10,"unlabeled">
as a per-device condition.

> Also I was a bit surprised to see EBUSY and ENOENT in errno_unsupported().
> Can one assume there are no other attributes on a device if you get those?

I added those to align with ACL_NOT_WELL_SUPPORTED from
lib/acl-internal.h, but now that you mention it, I suppose we'll need
different errno-checking functions for ACL-related than for getfilecon
failures.  I suppose ENOENT can arise when the file is removed between
when we stat'd it and when we perform one of these three file-name-using
tests.

>>> $ strace ~/tmp/ls_20120209 -l --color=always
>>> [...]
>>> lstat("file9", {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
>>> capget(0x20080522, 0, NULL)             = -1 EFAULT (Bad address)
>>> getxattr("file9", "security.capability", 0x7fffffffda30, 20) = -1
>>> EOPNOTSUPP (Operation not supported)
>>> lgetxattr("file9", "security.selinux", "unlabeled", 255) = 10
>
>>From Sven's strace it seems that lgetfilecon() is interpreting
> the "unlabeled" return from lgetxattr() and converting that
> into a -1 return with ENODATA?  That's the only logical explanation
> I can see for how ENODATA is significant. As mentioned before
> I'm not sure we could add ENODATA to errno_unsupported().

Right.  We definitely don't want to add ENODATA.
I removed it for this sort of reason.

The gnulib getfilecon wrapper maps 10,"unlabeled" to -1,ENODATA.

Back when I wrote that wrapper, it sounded like 10,"unlabeled"
would happen only with kernels deemed crufty back then (in 2009).

  http://thread.gmane.org/gmane.comp.gnu.coreutils.bugs/18378/focus=18384

That we're still seeing such syscall traces in 2012...