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

[Pádraig Brady]

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:

  $ 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

diff --git a/lib/areadlink-with-size.c b/lib/areadlink-with-size.c
index eacad3f..2fbe51c 100644
--- a/lib/areadlink-with-size.c
+++ b/lib/areadlink-with-size.c
@@ -36,14 +36,15 @@
    check, so it's OK to guess too small on hosts where there is no
    arbitrary limit to symbolic link length.  */
 #ifndef SYMLINK_MAX
-# define SYMLINK_MAX 1024
+# define SYMLINK_MAX 1023
 #endif
 
 #define MAXSIZE (SIZE_MAX < SSIZE_MAX ? SIZE_MAX : SSIZE_MAX)
 
 /* Call readlink to get the symbolic link value of FILE.
    SIZE is a hint as to how long the link is expected to be;
-   typically it is taken from st_size.  It need not be correct.
+   typically it is taken from st_size.  It need not be correct,
+   and a value of 0 (or more than 8Ki) will select an appropriate lower bound.
    Return a pointer to that NUL-terminated string in malloc'd storage.
    If readlink fails, malloc fails, or if the link value is longer
    than SSIZE_MAX, return NULL (caller may use errno to diagnose).  */
@@ -61,7 +62,7 @@ areadlink_with_size (char const *file, size_t size)
                           : INITIAL_LIMIT_BOUND);
 
   /* The initial buffer size for the link value.  */
-  size_t buf_size = size < initial_limit ? size + 1 : initial_limit;
+  size_t buf_size = size && size < initial_limit ? size + 1 : initial_limit;
 
   while (1)
     {