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

[Pádraig Brady]

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?

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?

>> $ 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().

>> getxattr("file9", "system.posix_acl_access", 0x0, 0) = -1 EOPNOTSUPP
>> (Operation not supported)

cheers,
Pádraig.