[RESEND,BZ,#15132] avoid stat/fstat in statvfs/fstatvfs

Message ID 20140527060814.GA28198@dcvr.yhbt.net
State Committed
Headers

Commit Message

Eric Wong May 27, 2014, 6:08 a.m. UTC
  Eric Wong <normalperson@yhbt.net> wrote:
> Ondřej Bílka <neleai@seznam.cz> wrote:
> > > Sorry for the delay.  I've only tested on 3.12+ so far on x86_64.
> > > I'll try to get a VM setup on <2.6.36 soon
> > 
> > Did you test also with these kernels?
> 
> Unfortunately the machine running 2.6.32.x CentOS 6.x kernel
> already had Linux commit 365b18189789bfa1acd9939e6312b8a4b4577b28
> (add f_flags to struct statfs(64)) backported to it :<
> 
> Probably just be easier for me to test with the following kernel patch
> than to get an older kernel running:
> 
> --- a/fs/statfs.c
> +++ b/fs/statfs.c
> @@ -42,7 +42,7 @@ static int flags_by_sb(int s_flags)
>  
>  static int calculate_f_flags(struct vfsmount *mnt)
>  {
> -	return ST_VALID | flags_by_mnt(mnt->mnt_flags) |
> +	return flags_by_mnt(mnt->mnt_flags) |
>  		flags_by_sb(mnt->mnt_sb->s_flags);
>  }
>  

I tested with the above patch applied 3.14 kernel and verified it
correctly falls back to using stat/fstat if the ST_VALID flag is
missing.

On an unpatched 3.14 kernel, the same executables/binaries avoid
stat/fstat when statvfs/fstatvfs are called.

I straced the following program to verify the presence/lack-of
extraneous stat/fstat syscalls after statfs/fstatfs:

  strace /path/to/lib/ld-linux-x86-64.so.2 /tmp/statvfs

-------------------------- /tmp/statvfs.c ----------------------------
#include <sys/statvfs.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
int main(int argc, char *argv[])
{
	struct statvfs buf;
	const char *path = argc == 2 ? argv[1] : "/";
	int fd;

	statvfs(path, &buf);
	fd = open(path, O_DIRECTORY, O_RDONLY);
	fstatvfs(fd, &buf);

	return 0;
}

Original patch with updated commit message:
-------------------------------- 8< ---------------------------------
From: Eric Wong <normalperson@yhbt.net>
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.

2014-05-27  Eric Wong  <normalperson@yhbt.net>

	[BZ #15132]
	* sysdeps/unix/sysv/linux/internal_statvfs.c (__statvfs_getflags):
	Call fstat64 or stat64 internally, depending on arguments passed.
	Replace stat buffer argument with file descriptor argument.
	(INTERNAL_STATVFS): Update arguments to match __statvfs_getflags.
	* sysdeps/unix/sysv/linux/fstatvfs.c (fstatvfs):
	Pass fd to __internal_statvfs instead of calling fstat64.
	* sysdeps/unix/sysv/linux/fstatvfs64.c (__fstatvfs64):
	Pass fd to __internal_statvfs64 instead of calling fstat64.
	* sysdeps/unix/sysv/linux/statvfs.c (statvfs):
	Pass -1 to __internal_statvfs instead of calling stat64.
	* sysdeps/unix/sysv/linux/statvfs64.c (__statvfs64):
	Pass -1 to __internal_statvfs64 instead of calling stat64.
---
 sysdeps/unix/sysv/linux/fstatvfs.c         |  4 ++--
 sysdeps/unix/sysv/linux/fstatvfs64.c       | 10 +++-------
 sysdeps/unix/sysv/linux/internal_statvfs.c | 15 ++++++++-------
 sysdeps/unix/sysv/linux/statvfs.c          |  6 ++----
 sysdeps/unix/sysv/linux/statvfs64.c        | 10 +++-------
 5 files changed, 18 insertions(+), 27 deletions(-)
  

Comments

Siddhesh Poyarekar May 29, 2014, 4:17 a.m. UTC | #1
On Tue, May 27, 2014 at 06:08:14AM +0000, Eric Wong wrote:
> I tested with the above patch applied 3.14 kernel and verified it
> correctly falls back to using stat/fstat if the ST_VALID flag is
> missing.
> 
> On an unpatched 3.14 kernel, the same executables/binaries avoid
> stat/fstat when statvfs/fstatvfs are called.
> 
> I straced the following program to verify the presence/lack-of
> extraneous stat/fstat syscalls after statfs/fstatfs:
> 
>   strace /path/to/lib/ld-linux-x86-64.so.2 /tmp/statvfs
> 

I have pushed the patch into master.  Thank you for following up on
this!

Siddhesh
  

Patch

diff --git a/sysdeps/unix/sysv/linux/fstatvfs.c b/sysdeps/unix/sysv/linux/fstatvfs.c
index da1f27e..689ab7a 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 dd10c1f..006b059 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 2424c13..d71ed0c 100644
--- a/sysdeps/unix/sysv/linux/internal_statvfs.c
+++ b/sysdeps/unix/sysv/linux/internal_statvfs.c
@@ -43,9 +43,11 @@ 
 
 # 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;
+
+  if ((fd < 0 ? stat64 (name, &st) : fstat64 (fd, &st)) < 0)
     return 0;
 
   const char *fsname = NULL;
@@ -159,7 +161,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.  */
@@ -222,14 +224,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;
@@ -272,7 +273,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 8a3df97..1427961 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 e33e923..0ec8524 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;
 }