Re: [PATCH] stat: don't explicitly request file size for filenames

[Andreas Dilger]

On Jul 4, 2019, at 04:44, Pádraig Brady <address@hidden> wrote:
> 
> On 03/07/19 21:24, Andreas Dilger wrote:
>> When calling 'stat -c %N' to print the filename, don't explicitly
>> request the size of the file via statx(), as it may add overhead on
>> some filesystems.  The size is only needed to optimize an allocation
>> for the relatively rare case of reading a symlink name, and the worst
>> effect is a somewhat-too-large temporary buffer may be allocated for
>> areadlink_with_size(), or internal retries if buffer is too small.
>> 
>> The file size will be returned by statx() on most filesystems, even
>> if not requested, unless the filesystem considers this to be too
>> expensive for that file, in which case the tradeoff is worthwhile.
>> 
>> * src/stat.c: Don't explicitly request STATX_SIZE for filenames.
>> Start with a 1KB buffer for areadlink_with_size() if st_size unset.
>> ---
>> src/stat.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/src/stat.c b/src/stat.c
>> index ec0bb7d..c887013 100644
>> --- a/src/stat.c
>> +++ b/src/stat.c
>> @@ -1282,7 +1282,7 @@ fmt_to_mask (char fmt)
>>   switch (fmt)
>>     {
>>     case 'N':
>> -      return STATX_MODE|STATX_SIZE;
>> +      return STATX_MODE;
>>     case 'd':
>>     case 'D':
>>       return STATX_MODE;
>> @@ -1491,7 +1491,9 @@ print_stat (char *pformat, size_t prefix_len, unsigned 
>> int m,
>>       out_string (pformat, prefix_len, quoteN (filename));
>>       if (S_ISLNK (statbuf->st_mode))
>>         {
>> -          char *linkname = areadlink_with_size (filename, statbuf->st_size);
>> +          /* if statx() didn't set size, most symlinks are under 1KB */
>> +          char *linkname = areadlink_with_size (filename, statbuf->st_size 
>> ?:
>> +                                                1023);
> 
> It would be nice to have areadlink_with_size treat 0 as auto select some 
> lower bound.
> There is already logic there, and it would be generally helpful,
> as st_size can often be 0, as shown with:

I had originally added code to handle this case by allocating a large buffer, 
but I
removed it to keep the patch minimal.  The benefit of passing the 1024-byte 
size from
the caller is that we know this is a temporary buffer and over-allocating 1KB 
for the
symlink is not kept in memory a long time.

That said, it doesn't make any sense to be allocating a 1-byte buffer.  I don't 
know
the glibc malloc routines, but in the kernel, allocating anything less than 32 
bytes
is meaningless, since the minimum slab size is 32 bytes.

>  $ strace -e readlink stat -c %N /proc/$$/cwd
>  readlink("/proc/9036/cwd", "/", 1)      = 1
>  readlink("/proc/9036/cwd", "/h", 2)     = 2
>  readlink("/proc/9036/cwd", "/hom", 4)   = 4
>  readlink("/proc/9036/cwd", "/home/pa", 8) = 8
>  readlink("/proc/9036/cwd", "/home/padraig", 16) = 13
> 
> With the attached gnulib diff, we get:
> 
>  $ strace -e readlink git/coreutils/src/stat -c %N /proc/$$/cwd
>  readlink("/proc/12512/cwd", "/home/padraig", 1024) = 13
> 
> I'll push both later.
> 
> thanks!
> Pádraig
> <areadlink-zero-size.diff>

Cheers, Andreas
--
Andreas Dilger
Principal Lustre Architect
Whamcloud