On Jul 4, 2019, at 04:44, Pádraig Brady <p...@draigbrady.com> 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
