[PATCH] avoid stat/fstat in statvfs/fstatvfs

[Eric Wong <normalperson at yhbt dot net>]

It turns out this problem was not specific to eglibc or Debian.
glibc has the same issue with stat() being called in statvfs() on
Linux 2.6.36+

Eric Wong <normalperson@yhbt.net> wrote:
> Paul Eggert <eggert@cs.ucla.edu> wrote:
> > On 01/29/2013 06:44 PM, Eric Wong wrote:
> > > eglibc on Debian is only configured to only use features available in
> > > Linux 2.6.26 at build time.  This means statvfs() uses stat() internally
> > > regardless of the run-time kernel version[1].
> > 
> > That sounds like a bug in eglibc.  Perhaps you can fix this problem on the
> > eglibc side.  That is, eglibc can use only 2.6.26 features, but make an
> > exception for this feature, since it's obviously needed.
> 
> Actually, reading the eglibc and kernel source more closely, my initial
> analysis seems wrong, and I'm even more at a loss as to why stat()
> gets called when I call statvfs().

I completely missed the stat() calls inside the __internal_statvfs()
argument list :x

> The stat() I see after statfs() is /not/ happening from a check of
> where the 2.6.36+ kernel sets ST_VALID:
> 
> At first, I thought the following code was entering __statvfs_getflags()
> in sysdeps/unix/sysv/linux/internal_statvfs.c:
> 
> #ifndef __ASSUME_STATFS_F_FLAGS
>   if ((fsbuf->f_flags & ST_VALID) == 0)
>     /* Determining the flags is tricky.  We have to read /proc/mounts or
>        the /etc/mtab file and search for the entry which matches the given
>        file.  The way we can test for matching filesystem is using the
>        device number.  */
>     buf->f_flag = __statvfs_getflags (name, fsbuf->f_type, st);
>   else
> #endif
>     buf->f_flag = fsbuf->f_flags ^ ST_VALID;
> -----------------------------------------------------------------
> But now I see it's not, since __statvfs_getflags() would also
> open /proc/mounts (which I don't see happening).
> 
> Anyways I've filed a bug report with Debian:
> http://bugs.debian.org/699321
> 
> Thanks for looking at this, hopefully somebody from Debian can help.

>From 2374682f5dc38d7cc752cdbdecd55bd870b38a90 Mon Sep 17 00:00:00 2001
From: Eric Wong <normalperson@yhbt.net>
Date: Fri, 1 Feb 2013 01:55:27 +0000
Subject: [PATCH] avoid stat/fstat in statvfs/fstatvfs

Delay the use of stat/fstat until stat data is required.
When the kernel returns ST_VALID, stat data is not used
by __internal_statvfs.
---
 sysdeps/unix/sysv/linux/fstatvfs.c         |  4 ++--
 sysdeps/unix/sysv/linux/fstatvfs64.c       | 10 +++-------
 sysdeps/unix/sysv/linux/internal_statvfs.c | 16 +++++++++-------
 sysdeps/unix/sysv/linux/statvfs.c          |  6 ++----
 sysdeps/unix/sysv/linux/statvfs64.c        | 10 +++-------
 5 files changed, 19 insertions(+), 27 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/fstatvfs.c b/sysdeps/unix/sysv/linux/fstatvfs.c
index 8f08d14..6a03a87 100644
--- a/sysdeps/unix/sysv/linux/fstatvfs.c
+++ b/sysdeps/unix/sysv/linux/fstatvfs.c
@@ -22,7 +22,7 @@
 #include <sys/statvfs.h>
 
 extern void __internal_statvfs (const char *name, struct statvfs *buf,
-				struct statfs *fsbuf, struct stat64 *st);
+				struct statfs *fsbuf, int fd);
 
 
 int
@@ -36,7 +36,7 @@ fstatvfs (int fd, struct statvfs *buf)
     return -1;
 
   /* Convert the result.  */
-  __internal_statvfs (NULL, buf, &fsbuf, fstat64 (fd, &st) == -1 ? NULL : &st);
+  __internal_statvfs (NULL, buf, &fsbuf, fd);
 
   /* We signal success if the statfs call succeeded.  */
   return 0;
diff --git a/sysdeps/unix/sysv/linux/fstatvfs64.c b/sysdeps/unix/sysv/linux/fstatvfs64.c
index 2dcef94..3b49ab2 100644
--- a/sysdeps/unix/sysv/linux/fstatvfs64.c
+++ b/sysdeps/unix/sysv/linux/fstatvfs64.c
@@ -25,7 +25,7 @@
 
 
 extern void __internal_statvfs64 (const char *name, struct statvfs64 *buf,
-				  struct statfs64 *fsbuf, struct stat64 *st);
+				  struct statfs64 *fsbuf, int fd);
 
 
 /* Return information about the filesystem on which FD resides.  */
@@ -60,12 +60,8 @@ __fstatvfs64 (int fd, struct statvfs64 *buf)
 #endif
 
   if (res == 0)
-    {
-      /* Convert the result.  */
-      struct stat64 st;
-      __internal_statvfs64 (NULL, buf, &fsbuf,
-			    fstat64 (fd, &st) == -1 ? NULL : &st);
-    }
+    /* Convert the result.  */
+    __internal_statvfs64 (NULL, buf, &fsbuf, fd);
 
   return res;
 }
diff --git a/sysdeps/unix/sysv/linux/internal_statvfs.c b/sysdeps/unix/sysv/linux/internal_statvfs.c
index 4cd4f04..cb9400e 100644
--- a/sysdeps/unix/sysv/linux/internal_statvfs.c
+++ b/sysdeps/unix/sysv/linux/internal_statvfs.c
@@ -43,9 +43,12 @@
 
 # ifndef __ASSUME_STATFS_F_FLAGS
 int
-__statvfs_getflags (const char *name, int fstype, struct stat64 *st)
+__statvfs_getflags (const char *name, int fstype, int fd)
 {
-  if (st == NULL)
+  struct stat64 st;
+  int r = fd >= 0 ? fstat64(fd, &st) : stat64(name, &st);
+
+  if (r == -1)
     return 0;
 
   const char *fsname = NULL;
@@ -153,7 +156,7 @@ __statvfs_getflags (const char *name, int fstype, struct stat64 *st)
 	  /* Find out about the device the current entry is for.  */
 	  struct stat64 fsst;
 	  if (stat64 (mntbuf.mnt_dir, &fsst) >= 0
-	      && st->st_dev == fsst.st_dev)
+	      && st.st_dev == fsst.st_dev)
 	    {
 	      /* Bingo, we found the entry for the device FD is on.
 		 Now interpret the option string.  */
@@ -216,14 +219,13 @@ __statvfs_getflags (const char *name, int fstype, struct stat64 *st)
 }
 # endif
 #else
-extern int __statvfs_getflags (const char *name, int fstype,
-			       struct stat64 *st);
+extern int __statvfs_getflags (const char *name, int fstype, int fd);
 #endif
 
 
 void
 INTERNAL_STATVFS (const char *name, struct STATVFS *buf,
-		  struct STATFS *fsbuf, struct stat64 *st)
+		  struct STATFS *fsbuf, int fd)
 {
   /* Now fill in the fields we have information for.  */
   buf->f_bsize = fsbuf->f_bsize;
@@ -266,7 +268,7 @@ INTERNAL_STATVFS (const char *name, struct STATVFS *buf,
        the /etc/mtab file and search for the entry which matches the given
        file.  The way we can test for matching filesystem is using the
        device number.  */
-    buf->f_flag = __statvfs_getflags (name, fsbuf->f_type, st);
+    buf->f_flag = __statvfs_getflags (name, fsbuf->f_type, fd);
   else
 #endif
     buf->f_flag = fsbuf->f_flags ^ ST_VALID;
diff --git a/sysdeps/unix/sysv/linux/statvfs.c b/sysdeps/unix/sysv/linux/statvfs.c
index 5d91d85..09b53f7 100644
--- a/sysdeps/unix/sysv/linux/statvfs.c
+++ b/sysdeps/unix/sysv/linux/statvfs.c
@@ -22,22 +22,20 @@
 #include <sys/statvfs.h>
 
 extern void __internal_statvfs (const char *name, struct statvfs *buf,
-				struct statfs *fsbuf, struct stat64 *st);
+				struct statfs *fsbuf, int fd);
 
 
 int
 statvfs (const char *file, struct statvfs *buf)
 {
   struct statfs fsbuf;
-  struct stat64 st;
 
   /* Get as much information as possible from the system.  */
   if (__statfs (file, &fsbuf) < 0)
     return -1;
 
   /* Convert the result.  */
-  __internal_statvfs (file, buf, &fsbuf,
-		      stat64 (file, &st) == -1 ? NULL : &st);
+  __internal_statvfs (file, buf, &fsbuf, -1);
 
   /* We signal success if the statfs call succeeded.  */
   return 0;
diff --git a/sysdeps/unix/sysv/linux/statvfs64.c b/sysdeps/unix/sysv/linux/statvfs64.c
index 42c1089..cd9a7ca 100644
--- a/sysdeps/unix/sysv/linux/statvfs64.c
+++ b/sysdeps/unix/sysv/linux/statvfs64.c
@@ -26,7 +26,7 @@
 
 
 extern void __internal_statvfs64 (const char *name, struct statvfs64 *buf,
-				  struct statfs64 *fsbuf, struct stat64 *st);
+				  struct statfs64 *fsbuf, int fd);
 
 
 /* Return information about the filesystem on which FILE resides.  */
@@ -61,12 +61,8 @@ __statvfs64 (const char *file, struct statvfs64 *buf)
 #endif
 
   if (res == 0)
-    {
-      /* Convert the result.  */
-      struct stat64 st;
-      __internal_statvfs64 (file, buf, &fsbuf,
-			    stat64 (file, &st) == -1 ? NULL : &st);
-    }
+    /* Convert the result.  */
+    __internal_statvfs64 (file, buf, &fsbuf, -1);
 
   return res;
 }
-- 
Eric Wong