Re: modify chmod

[jeff.liu]

Hi Jim,

Thanks for your so detailed instruction! And sorry for my later response, I 
just back from travel.

Could you please check my comments inline?

Jim Meyering wrote:
 jeff.liu wrote:
 jeff.liu 写道:
 Jim Meyering 写道:
 Finally, I see the one true way ;-)
 Do this for each name:

 - open each file/dir. with fd = openat (fts_cwd_fd, name, ...
 - if that succeeds, then call fstatfs (fd, ...

 Using the combination of openat and fstatfs is required in order to
 avoid using the full relative name of each object that chmod processes.

 The above works for all readable objects.
 For unreadable ones, revert to using the statfs with the full relative 
 name.

 Thanks for the hints.

 But according to my tryout, it looks we can not gain performance benefits 
 in this way. :(
 calling openat() could cause a bits overhead as well.

 I also tried to pass over the openat(), instead just comparing the 
 fts_cwd_fd againt AT_FDCWD,
 
 Hi Jeff,
 
 Skipping the openat call and thus operating on the full relative
 file name is not an option, since that will always fail when its length
 is longer than PATH_MAX.
Thanks for the point out, this is indeed the safe way to avoid ENAMETOOLONG.

 Hence, you must use openat.  However, if[*] we agree not to worry about
 bind-mounted non-directories, you should still be able to use openat
 without prohibitive overhead.
 
 You would maintain at least a single static pair {FD,val}
 indicating whether FD may_have_nfsacl (yes, no, do-not-know).
 Then you'd incur the cost of an openat call only for command-line
 arguments (where fd is AT_FDCWD), and when a traversal transitions
 from one directory to another.
So my understanding is call openat() only if the cwd_fd is AT_FDCWD, then check 
the returned FD by
fstatfs(). if openat() failed due to the dir mount pont changed, we have to 
call statfs() againt the
full relative path.

Otherwise, we could determine the FS type by call fstatfs() against the 
dirfd(cwd_fd) on which the
files resides, it is safe since chmod do not do operation on the symlink files.
of coz, we have to call statfs() again if fstatfs() failed with the same issue 
as I mentioned above.

Am I right?
 
 The approach (mentioned initially) taken by fts.c is to record
 not just the single {FD,val} pair, but a set of {stat.st_dev,val}
 pairs, one for each distinct device encountered.  This requires
 a small amount of extra overhead (time and memory), but reduces
 the number of *statfs calls to the bare minimum.
 
 [*] This change is intended to be an optimization.
 I am leery of letting an optimization change the semantics of a
 program like chmod (which is already very efficient), even if only
 for the unusual case of a bind-mounted ACL-possessing non-directory
 that resides on an NFS-mounted file system.
 Other opinions?
 


Thanks,
-Jeff

Re: modify chmod

[Jim Meyering]

jeff.liu wrote:
 Hi Jim,

 Thanks for your so detailed instruction! And sorry for my later response, I 
 just back from travel.

 Could you please check my comments inline?
...

This is the most important point:

 [*] This change is intended to be an optimization.
 I am leery of letting an optimization change the semantics of a
 program like chmod (which is already very efficient), even if only
 for the unusual case of a bind-mounted ACL-possessing non-directory
 that resides on an NFS-mounted file system.
 Other opinions?

Considering that this optimization is possible only
if we agree to degrade (even by only a small amount)
chmod's correctness, I am inclined to drop it.

No one disagreed.

Re: modify chmod

[Jim Meyering]

jeff.liu wrote:
 Jim Meyering wrote:
 jeff.liu wrote:
 Hi Jim,

 Thanks for your so detailed instruction! And sorry for my later response, I 
 just back from travel.

 Could you please check my comments inline?
 ...

 This is the most important point:

 [*] This change is intended to be an optimization.
 I am leery of letting an optimization change the semantics of a
 program like chmod (which is already very efficient), even if only
 for the unusual case of a bind-mounted ACL-possessing non-directory
 that resides on an NFS-mounted file system.
 Other opinions?

 Considering that this optimization is possible only
 if we agree to degrade (even by only a small amount)
 chmod's correctness, I am inclined to drop it.

 so you means this optimization is acceptable only if we can ignore the 
 correctness for the
 bind-mounted files?
 but you think it is no properly since it will affect the semantics of chmod?

Hi Jeff,

That's right.
If someone measures the performance difference and
we find it compelling enough that we're willing to overlook
the incorrectness with (admittedly rare) bind-mounted ACL-possessing
non-directories, this might be worthwhile -- via a new option.
It must not be the default, since it can constitute a real regression.

Chmod is typically quite fast, even for large hierarchies.
Since the value of the performance improvement is less when
enabled only via an option, it becomes harder to justify
the added complexity.

Re: modify chmod

[jeff.liu]

Jim Meyering wrote:
 jeff.liu wrote:
 Jim Meyering wrote:
 jeff.liu wrote:
 Hi Jim,

 Thanks for your so detailed instruction! And sorry for my later response, 
 I just back from travel.

 Could you please check my comments inline?
 ...

 This is the most important point:

 [*] This change is intended to be an optimization.
 I am leery of letting an optimization change the semantics of a
 program like chmod (which is already very efficient), even if only
 for the unusual case of a bind-mounted ACL-possessing non-directory
 that resides on an NFS-mounted file system.
 Other opinions?
 Considering that this optimization is possible only
 if we agree to degrade (even by only a small amount)
 chmod's correctness, I am inclined to drop it.

 so you means this optimization is acceptable only if we can ignore the 
 correctness for the
 bind-mounted files?
 but you think it is no properly since it will affect the semantics of chmod?
 
 Hi Jeff,
 
 That's right.
 If someone measures the performance difference and
 we find it compelling enough that we're willing to overlook
 the incorrectness with (admittedly rare) bind-mounted ACL-possessing
 non-directories, this might be worthwhile -- via a new option.
 It must not be the default, since it can constitute a real regression.

 
 Chmod is typically quite fast, even for large hierarchies.
 Since the value of the performance improvement is less when
 enabled only via an option, it becomes harder to justify
 the added complexity.

Hi Jim,

Thanks for your clarification.
I have learn a lot from your guys even if the patch does not acceptable after 
all.


Regards,
-Jeff

Re: modify chmod

[Jim Meyering]

jeff.liu wrote:
 jeff.liu 写道:
 Jim Meyering 写道:
 Finally, I see the one true way ;-)
 Do this for each name:

 - open each file/dir. with fd = openat (fts_cwd_fd, name, ...
 - if that succeeds, then call fstatfs (fd, ...

 Using the combination of openat and fstatfs is required in order to
 avoid using the full relative name of each object that chmod processes.

 The above works for all readable objects.
 For unreadable ones, revert to using the statfs with the full relative name.


 Thanks for the hints.

 But according to my tryout, it looks we can not gain performance benefits in 
 this way. :(
 calling openat() could cause a bits overhead as well.


 I also tried to pass over the openat(), instead just comparing the fts_cwd_fd 
 againt AT_FDCWD,

Hi Jeff,

Skipping the openat call and thus operating on the full relative
file name is not an option, since that will always fail when its length
is longer than PATH_MAX.
Hence, you must use openat.  However, if[*] we agree not to worry about
bind-mounted non-directories, you should still be able to use openat
without prohibitive overhead.

You would maintain at least a single static pair {FD,val}
indicating whether FD may_have_nfsacl (yes, no, do-not-know).
Then you'd incur the cost of an openat call only for command-line
arguments (where fd is AT_FDCWD), and when a traversal transitions
from one directory to another.

The approach (mentioned initially) taken by fts.c is to record
not just the single {FD,val} pair, but a set of {stat.st_dev,val}
pairs, one for each distinct device encountered.  This requires
a small amount of extra overhead (time and memory), but reduces
the number of *statfs calls to the bare minimum.

[*] This change is intended to be an optimization.
I am leery of letting an optimization change the semantics of a
program like chmod (which is already very efficient), even if only
for the unusual case of a bind-mounted ACL-possessing non-directory
that resides on an NFS-mounted file system.
Other opinions?

Re: modify chmod

[jeff.liu]

Jim Meyering 写道:
 Finally, I see the one true way ;-)
 Do this for each name:
 
 - open each file/dir. with fd = openat (fts_cwd_fd, name, ...
 - if that succeeds, then call fstatfs (fd, ...
 
 Using the combination of openat and fstatfs is required in order to
 avoid using the full relative name of each object that chmod processes.
 
 The above works for all readable objects.
 For unreadable ones, revert to using the statfs with the full relative name.


Thanks for the hints.

But according to my tryout, it looks we can not gain performance benefits in 
this way. :(
calling openat() could cause a bits overhead as well.

I have done a simple test against a directory mounted as ext3 fs, set its 
default permission bits to
644.

r...@jeff-laptop:/ocfs2# du -sh .
2.0G.

r...@jeff-laptop:/ocfs2# ls -ld .
drw-r--r-- 8 root root 4096 Feb  8 16:11 .

j...@jeff-laptop:~/opensource_dev/coreutils$ time yes | sudo ./src/chmod -R 644 
/ocfs2/

real0m16.421s
user0m0.400s
sys 0m15.605s

j...@jeff-laptop:~/opensource_dev/coreutils$ time yes | sudo ./src/chmod -R 755 
/ocfs2/

real0m17.129s
user0m0.308s
sys 0m16.233s

j...@jeff-laptop:~/opensource_dev/coreutils$ time yes | sudo ./src/chmod -R 755 
/ocfs2/

real0m17.029s
user0m0.400s
sys 0m16.157s


Could you check the following patch, hope I didn't misunderstood your idea.

From ec84e84d79a69d1422e0d02ab99a65c30c75b27f Mon Sep 17 00:00:00 2001
From: Jie Liu jeff@oracle.com
Date: Mon, 8 Feb 2010 16:51:37 +0800
Subject: [PATCH] chmod modify: do not touch inode for same permission bits v2

This patch change the chmod default behavior to skip chmodat(2)
the inode if its new permission bits is same as it was before.

Signed-off-by: Jie Liu jeff@oracle.com
---
 src/chmod.c |   87 ++-
 1 files changed, 80 insertions(+), 7 deletions(-)

diff --git a/src/chmod.c b/src/chmod.c
index 3dab92e..b49adb8 100644
--- a/src/chmod.c
+++ b/src/chmod.c
@@ -20,6 +20,7 @@
 #include stdio.h
 #include getopt.h
 #include sys/types.h
+#include unistd.h

 #include system.h
 #include dev-ino.h
@@ -32,6 +33,16 @@
 #include root-dev-ino.h
 #include xfts.h

+/* Some systems only have sys/vfs.h, other systems also
+   have sys/statfs.h, where the former includes the latter.
+   So it seems including the former is the best choice.  */
+#include fs.h
+#if HAVE_SYS_VFS_H
+# include sys/vfs.h
+#else
+# include sys/statfs.h
+#endif
+
 /* The official name of this program (e.g., no `g' prefix).  */
 #define PROGRAM_NAME chmod

@@ -83,6 +94,9 @@ static enum Verbosity verbosity = V_off;
Otherwise NULL.  */
 static struct dev_ino *root_dev_ino;

+/* The effective user ID of the caller process.  */
+static uid_t euid;
+
 /* For long options that have no equivalent short option, use a
non-character as a pseudo short option, starting with CHAR_MAX + 1.  */
 enum
@@ -170,6 +184,42 @@ describe_change (const char *file, mode_t mode,
   (unsigned long int) (mode  CHMOD_MODE_BITS), perms[1]);
 }

+/* Return true if the file resides on NFS filesystem.
+   Limit this optimization to systems that provide fstatfs.  */
+
+#if HAVE_FSTATFS  HAVE_SYS_STATFS_H  HAVE_STRUCT_STATFS_F_TYPE
+static bool
+may_have_nfsacl (FTSENT const *ent)
+{
+  int fd;
+  int cwd_fd = ent-fts_fts-fts_cwd_fd;
+  char const *file = ent-fts_accpath;
+  struct statfs buf;
+
+  fd = openat (cwd_fd, file, O_RDONLY);
+  if (0 = fd)
+{
+  /* If fstatfs fails, assume we can't use the optimization.  */
+  if (fstatfs (fd, buf)  0)
+{
+  close (fd);
+  return true;
+   }
+
+  close (fd);
+  return buf.f_type == S_MAGIC_NFS;
+}
+
+  char const *file_full_name = ent-fts_path;
+  if (statfs (file_full_name, buf)  0)
+return true;
+
+  return buf.f_type == S_MAGIC_NFS;
+}
+#else
+# define may_have_nfsacl(ignored) (true)
+#endif
+
 /* Change the mode of FILE.
Return true if successful.  This function is called
once for every file system object that fts encounters.  */
@@ -258,16 +308,38 @@ process_file (FTS *fts, FTSENT *ent)
   new_mode = mode_adjust (old_mode, S_ISDIR (old_mode) != 0, umask_value,
   change, NULL);

-  if (! S_ISLNK (old_mode))
+  /* Do not touch the inode if the new file mode is the same as it was 
before;
+ This is an optimization for some cases. However, the new behavior 
must be
+ disabled for filesystems that support NFSv4 ACLs.
+ This idea suggest by Paul Eggert refer to FreeBSD chmod(1).
+ It uses pathconf(2)/lpathconf(2) to determine whether this is the 
case.
+ But we lack of them on linux, Instead, calling fstatfs(2) and compare 
the
+ f_type we can still do optimization at least its not NFS.  */
+
+  if ((old_mode  CHMOD_MODE_BITS) == new_mode)
 {
-  if (chmodat (fts-fts_cwd_fd, file, new_mode) == 0)
-chmod_succeeded = true;
-  

Re: modify chmod

[jeff.liu]

jeff.liu 写道:
 Jim Meyering 写道:
 Finally, I see the one true way ;-)
 Do this for each name:

 - open each file/dir. with fd = openat (fts_cwd_fd, name, ...
 - if that succeeds, then call fstatfs (fd, ...

 Using the combination of openat and fstatfs is required in order to
 avoid using the full relative name of each object that chmod processes.

 The above works for all readable objects.
 For unreadable ones, revert to using the statfs with the full relative name.

 
 Thanks for the hints.
 
 But according to my tryout, it looks we can not gain performance benefits in 
 this way. :(
 calling openat() could cause a bits overhead as well.
 

I also tried to pass over the openat(), instead just comparing the fts_cwd_fd 
againt AT_FDCWD,
if the pathname is interpreted relative to the current working directory of the 
calling process,
call statfs().
or call fstafs(fts_cwd_fd...).

But consider the bind-mounted directory, it is not a proper way.
Anyway, I add a timer instruments to help figure out the performance difference 
between
may_have_acl() and chmodat(),
If does not have to call openat(), the performance looks fine.

Please check the following test results:
j...@jeff-laptop:~/opensource_dev/coreutils$ time yes | sudo ./src/chmod -R 755 
/ocfs2/
Time spent: 1.419816

real0m10.584s
user0m0.296s
sys 0m10.045s
j...@jeff-laptop:~/opensource_dev/coreutils$ time yes | sudo ./src/chmod -R 755 
/ocfs2/
Time spent: 1.256965

real0m9.176s
user0m0.220s
sys 0m8.841s
j...@jeff-laptop:~/opensource_dev/coreutils$ time yes | sudo ./src/chmod -R 644 
/ocfs2/
Time spent: 9.245956

real0m17.541s
user0m0.344s
sys 0m17.053s
j...@jeff-laptop:~/opensource_dev/coreutils$ time yes | sudo ./src/chmod -R 644 
/ocfs2/
Time spent: 1.494338

real0m10.972s
user0m0.360s
sys 0m10.293s


code diff based on the last replay:

diff --git a/src/chmod.c b/src/chmod.c
index b49adb8..430ee6f 100644
--- a/src/chmod.c
+++ b/src/chmod.c
@@ -21,6 +21,7 @@
 #include getopt.h
 #include sys/types.h
 #include unistd.h
+#include sys/time.h

 #include system.h
 #include dev-ino.h
@@ -194,8 +195,10 @@ may_have_nfsacl (FTSENT const *ent)
   int fd;
   int cwd_fd = ent-fts_fts-fts_cwd_fd;
   char const *file = ent-fts_accpath;
+  char const *file_full_name = ent-fts_path;
   struct statfs buf;

+#if 0
   fd = openat (cwd_fd, file, O_RDONLY);
   if (0 = fd)
 {
@@ -209,10 +212,18 @@ may_have_nfsacl (FTSENT const *ent)
   close (fd);
   return buf.f_type == S_MAGIC_NFS;
 }
+#endif

-  char const *file_full_name = ent-fts_path;
-  if (statfs (file_full_name, buf)  0)
-return true;
+  if (cwd_fd != AT_FDCWD)
+{
+  if (fstatfs (cwd_fd, buf)  0)
+return true;
+}
+  else
+{
+  if (statfs (file_full_name, buf)  0)
+return true;
+}

   return buf.f_type == S_MAGIC_NFS;
 }
@@ -220,6 +231,8 @@ may_have_nfsacl (FTSENT const *ent)
 # define may_have_nfsacl(ignored) (true)
 #endif

+double tot_timer_spent;
+
 /* Change the mode of FILE.
Return true if successful.  This function is called
once for every file system object that fts encounters.  */
@@ -234,6 +247,9 @@ process_file (FTS *fts, FTSENT *ent)
   mode_t new_mode IF_LINT (= 0);
   bool ok = true;
   bool chmod_succeeded = false;
+  struct timeval timer_start;
+  struct timeval timer_end;
+  double timer_spent;

   switch (ent-fts_info)
 {
@@ -318,6 +334,8 @@ process_file (FTS *fts, FTSENT *ent)

   if ((old_mode  CHMOD_MODE_BITS) == new_mode)
 {
+  gettimeofday(timer_start, NULL);
+
   if (!may_have_nfsacl (ent))
 {
   if (file_stats-st_uid != euid  euid != 0)
@@ -326,11 +344,18 @@ process_file (FTS *fts, FTSENT *ent)
   else
 chmod_succeeded = true;
 }
+
+  gettimeofday(timer_end, NULL);
+  timer_spent = ((timer_end.tv_sec - timer_start.tv_sec) +
+ (timer_end.tv_usec - timer_start.tv_usec) / 
100.0);
+  tot_timer_spent += timer_spent;
}
   else
 {
   if (! S_ISLNK (old_mode))
 {
+  gettimeofday(timer_start, NULL);
+
   if (chmodat (fts-fts_cwd_fd, file, new_mode) == 0)
 chmod_succeeded = true;
   else
@@ -340,6 +365,11 @@ process_file (FTS *fts, FTSENT *ent)
quote (file_full_name));
   ok = false;
 }
+
+ gettimeofday(timer_end, NULL);
+  timer_spent = ((timer_end.tv_sec - timer_start.tv_sec) +
+ (timer_end.tv_usec - timer_start.tv_usec) / 
100.0);
+  tot_timer_spent += timer_spent;
 }
 }
 }
@@ -416,6 +446,8 @@ process_files (char **files, int bit_flags)
   ok = process_file (fts, ent);
 }

+  fprintf(stderr, Time spent: %.6f\n, tot_timer_spent);
+
   if (fts_close (fts) != 0)
 {
   error (0, 

Re: modify chmod

[jeff.liu]

Jim Meyering 写道:

jeff.liu wrote:
  

A tiny patch, make chmod do not touch the inode if the new file
permission mode is same as it was before.


...
  

+static bool
+may_have_nfsacl(const char *file)
+{
+# if HAVE_SYS_VFS_H  HAVE_SYS_STATFS_H  HAVE_STRUCT_STATFS_F_TYPE
+ struct statfs buf;
+
+ /* If statfs fails, assume we can't use the optimization. */
+ if (statfs (file, buf)  0)



This function should accept a file descriptor, not a file name,
and should call fstatfs, not statfs.

  

Thanks for the correction.
IMHO, to get a fd, then call fstatfs ranther than statfs here, is to 
ensure the same file object proceeding to operated on?

For your next version of this patch, please find a way to send
it that does not split lines or remove leading spaces.
  

I will take care for this kind of formating issues.

Re: modify chmod

[Jim Meyering]

jeff.liu wrote:

 Jim Meyering 写道:
 jeff.liu wrote:

 A tiny patch, make chmod do not touch the inode if the new file
 permission mode is same as it was before.

 ...

 +static bool
 +may_have_nfsacl(const char *file)
 +{
 +# if HAVE_SYS_VFS_H  HAVE_SYS_STATFS_H  HAVE_STRUCT_STATFS_F_TYPE
 + struct statfs buf;
 +
 + /* If statfs fails, assume we can't use the optimization. */
 + if (statfs (file, buf)  0)


 This function should accept a file descriptor, not a file name,
 and should call fstatfs, not statfs.


 Thanks for the correction.
 IMHO, to get a fd, then call fstatfs ranther than statfs here, is to
 ensure the same file object proceeding to operated on?

fts provides a file descriptor for every directory it processes,
so you can use an FTS_ENT const *ent parameter instead.
When it refers to a directory, use ent-fts_fts-fts_cwd_fd as the
file descriptor.  Otherwise, you will have to call statfs after all.
At first, I was thinking we could avoid statfs most of the time (i.e.,
change device only at a directory), but with bind-mounted regular files,
the device can change at any time during a traversal.

Re: modify chmod

[jeff.liu]

Jim Meyering 写道:

jeff.liu wrote:

  

Jim Meyering 写道:


jeff.liu wrote:

  

A tiny patch, make chmod do not touch the inode if the new file
permission mode is same as it was before.



...

  

+static bool
+may_have_nfsacl(const char *file)
+{
+# if HAVE_SYS_VFS_H  HAVE_SYS_STATFS_H  HAVE_STRUCT_STATFS_F_TYPE
+ struct statfs buf;
+
+ /* If statfs fails, assume we can't use the optimization. */
+ if (statfs (file, buf)  0)



This function should accept a file descriptor, not a file name,
and should call fstatfs, not statfs.


  

Thanks for the correction.
IMHO, to get a fd, then call fstatfs ranther than statfs here, is to
ensure the same file object proceeding to operated on?



fts provides a file descriptor for every directory it processes,
so you can use an FTS_ENT const *ent parameter instead.
When it refers to a directory, use ent-fts_fts-fts_cwd_fd as the
file descriptor.  Otherwise, you will have to call statfs after all.
  

thanks for the point out.
Actually, by consulting your primary comments for using fstatfs(2),
I was also thought the only way is pass the fts_cwd_fd to it,
then I started to study the fts_() series functions and added debugging
instruments to check the fts_cwd_fd value againt a five-depth directory.
but at that time, I found many -100(AT_FDCWD) printed out somehow.
so I took it for granted to call statfs since it can save the time if do not
need to check the fts_cwd_fd against AT_FDCWD frequently.

At first, I was thinking we could avoid statfs most of the time (i.e.,
change device only at a directory), but with bind-mounted regular files,
the device can change at any time during a traversal.


  

Re: modify chmod

[Jim Meyering]

Finally, I see the one true way ;-)
Do this for each name:

- open each file/dir. with fd = openat (fts_cwd_fd, name, ...
- if that succeeds, then call fstatfs (fd, ...

Using the combination of openat and fstatfs is required in order to
avoid using the full relative name of each object that chmod processes.

The above works for all readable objects.
For unreadable ones, revert to using the statfs with the full relative name.

Re: modify chmod

[jeff.liu]

Eric Blake 写道:
 According to jeff.liu on 2/5/2010 7:44 AM:
   
 +++ b/gnulib
 @@ -1 +1 @@
 -Subproject commit 2eb5a8a0ff8348149a9ca985e2ccbfb03bc8de53
 +Subproject commit 4b93a2579fb567b9fbbeb24439770d814dac95cd
 

 Why are you modifying the gnulib submodule?

   
Hmm, I also wonder what's the reason, maybe caused by my another code
tryout.
 +/* Some systems only have sys/vfs.h, other systems also
 + * have sys/statfs.h, where the former includes the latter.
 + * So it seems including the former is the best choice. */
 +#include fs.h
 +#if HAVE_SYS_VFS_H
 +# include sys/vfs.h
 +#else
 +# include sys/statfs.h
 +#endif
 

 Hmm.  POSIX standardized sys/statvfs.h, not sys/vfs.h.  Maybe a better
 approach would be to propose a patch to gnulib that guarantees that
 sys/statvfs.h provides everything we need, so that we don't have to ever
 worry about sys/vfs.h in coreutils.

   
Let me do more investigation for gnulib.
 +/* Return true if the file resides on NFS filesystem.
 + * limit this optimization to systems that provide statfs. */
 

 This comment formatting is not consistent with other comments in the file.


   
 +
 +static bool
 +may_have_nfsacl(const char *file)
 

 Missing a space between the function name and the (.  And coreutils
 prefers 'char const *' over the synonymous 'const char *'.

   
I still need to get familiar with the GNU code style.
 +{
 +# if HAVE_SYS_VFS_H  HAVE_SYS_STATFS_H  HAVE_STRUCT_STATFS_F_TYPE
 + struct statfs buf;
 +
 + /* If statfs fails, assume we can't use the optimization. */
 + if (statfs (file, buf)  0)
 + return true;
 +
 + return buf.f_type == S_MAGIC_NFS;
 +#endif
 +
 + return true;
 

 Rather than have #ifdefs inside the function body, this seems like it
 would be better to do:

 #ifdef ...
 function
 #else
 # define may_have_nfsacl(ignored) true
 #else

   
Really a good idea.
 +}
 +
 /* Change the mode of FILE.
 Return true if successful. This function is called
 once for every file system object that fts encounters. */
 @@ -257,18 +290,38 @@ process_file (FTS *fts, FTSENT *ent)
 old_mode = file_stats-st_mode;
 new_mode = mode_adjust (old_mode, S_ISDIR (old_mode) != 0, umask_value,
 change, NULL);
 -
 - if (! S_ISLNK (old_mode))
 +
 + /* Do not touch the inode if the new file mode is the same as it was
 before;
 + * This is an optimization for some cases. However, the new behavior
 must be
 + * disabled for filesystems that support NFSv4 ACLs.
 + * The idea suggested by Paul Eggert refer to FreeBSD chmod(1).
 + * it uses pathconf(2)/lpathconf(2) to determine whether this is the case.
 + * but on linux, we lack of them. Instead, calling statfs(2) and
 compare the
 + * f_type we can still do optimization at least its not NFS. */
 

 Again, inconsistent comment formatting.

   
 + if (file_stats-st_uid != euid  euid != 0)
 + error (0, 0, _(changing permissions of %s: Operation not permitted),
 + quote (file_full_name));
 

 Indentation in this patch looks wrong; did it get munged?  Also, typically
 the Operation not permitted is provided for free by passing the
 appropriate errno to error(), rather than open-coding it into the error
 string.

   
Thanks for the point out, learn from your comments again!

Re: modify chmod

[Jim Meyering]

jeff.liu wrote:
 A tiny patch, make chmod do not touch the inode if the new file
 permission mode is same as it was before.
...
 +static bool
 +may_have_nfsacl(const char *file)
 +{
 +# if HAVE_SYS_VFS_H  HAVE_SYS_STATFS_H  HAVE_STRUCT_STATFS_F_TYPE
 + struct statfs buf;
 +
 + /* If statfs fails, assume we can't use the optimization. */
 + if (statfs (file, buf)  0)

This function should accept a file descriptor, not a file name,
and should call fstatfs, not statfs.

For your next version of this patch, please find a way to send
it that does not split lines or remove leading spaces.

Re: modify chmod

[jeff.liu]

From c7bc43a05e4f021db8047ecd390b4fb75368f558 Mon Sep 17 00:00:00 2001
From: Jeff Liu jeff@oracle.com
Date: Fri, 5 Feb 2010 20:45:41 +0800
Subject: [PATCH 1/1] Modify chmod v1
 do not touch the inode if the new file permission bits is identical to it was before.

Signed-off-by: Jie Liu jeff@oracle.com
---
 gnulib  |2 +-
 src/chmod.c |   75 +++
 2 files changed, 66 insertions(+), 11 deletions(-)

diff --git a/gnulib b/gnulib
index 2eb5a8a..4b93a25 16
--- a/gnulib
+++ b/gnulib
@@ -1 +1 @@
-Subproject commit 2eb5a8a0ff8348149a9ca985e2ccbfb03bc8de53
+Subproject commit 4b93a2579fb567b9fbbeb24439770d814dac95cd
diff --git a/src/chmod.c b/src/chmod.c
index 3dab92e..30802d7 100644
--- a/src/chmod.c
+++ b/src/chmod.c
@@ -20,6 +20,7 @@
 #include stdio.h
 #include getopt.h
 #include sys/types.h
+#include unistd.h
 
 #include system.h
 #include dev-ino.h
@@ -32,6 +33,16 @@
 #include root-dev-ino.h
 #include xfts.h
 
+/* Some systems only have sys/vfs.h, other systems also
+ * have sys/statfs.h, where the former includes the latter.
+ * So it seems including the former is the best choice.  */
+#include fs.h
+#if HAVE_SYS_VFS_H
+#  include sys/vfs.h
+#else
+#  include sys/statfs.h
+#endif
+
 /* The official name of this program (e.g., no `g' prefix).  */
 #define PROGRAM_NAME chmod
 
@@ -83,6 +94,9 @@ static enum Verbosity verbosity = V_off;
Otherwise NULL.  */
 static struct dev_ino *root_dev_ino;
 
+/* The effective user ID of the caller process.  */
+static uid_t euid;
+
 /* For long options that have no equivalent short option, use a
non-character as a pseudo short option, starting with CHAR_MAX + 1.  */
 enum
@@ -170,6 +184,25 @@ describe_change (const char *file, mode_t mode,
   (unsigned long int) (mode  CHMOD_MODE_BITS), perms[1]);
 }
 
+/* Return true if the file resides on NFS filesystem.
+ * limit this optimization to systems that provide statfs.  */
+
+static bool
+may_have_nfsacl(const char *file)
+{
+# if HAVE_SYS_VFS_H  HAVE_SYS_STATFS_H  HAVE_STRUCT_STATFS_F_TYPE
+  struct statfs buf;
+
+  /* If statfs fails, assume we can't use the optimization.  */
+  if (statfs (file, buf)  0)
+  return true;
+
+  return buf.f_type == S_MAGIC_NFS;
+#endif
+
+  return true;
+}
+
 /* Change the mode of FILE.
Return true if successful.  This function is called
once for every file system object that fts encounters.  */
@@ -257,18 +290,38 @@ process_file (FTS *fts, FTSENT *ent)
   old_mode = file_stats-st_mode;
   new_mode = mode_adjust (old_mode, S_ISDIR (old_mode) != 0, umask_value,
   change, NULL);
-
-  if (! S_ISLNK (old_mode))
+
+ /* Do not touch the inode if the new file mode is the same as it was before;
+  * This is an optimization for some cases. However, the new behavior must be
+  * disabled for filesystems that support NFSv4 ACLs.
+  * The idea suggested by Paul Eggert refer to FreeBSD chmod(1).
+  * it uses pathconf(2)/lpathconf(2) to determine whether this is the case.
+  * but on linux, we lack of them. Instead, calling statfs(2) and compare the
+  * f_type we can still do optimization at least its not NFS.   */
+
+  if ((old_mode  CHMOD_MODE_BITS) == new_mode 
+  !may_have_nfsacl (file_full_name))
 {
-  if (chmodat (fts-fts_cwd_fd, file, new_mode) == 0)
-chmod_succeeded = true;
+  if (file_stats-st_uid != euid  euid != 0)
+error (0, 0, _(changing permissions of %s: Operation not permitted),
+   quote (file_full_name));
   else
-{
-  if (! force_silent)
-error (0, errno, _(changing permissions of %s),
-   quote (file_full_name));
-  ok = false;
-}
+chmod_succeeded = true;
+}
+  else
+{
+  if (! S_ISLNK (old_mode))
+  {
+if (chmodat (fts-fts_cwd_fd, file, new_mode) == 0)
+  chmod_succeeded = true;
+else
+  {
+if (! force_silent)
+  error (0, errno, _(changing permissions of %s),
+ quote (file_full_name));
+ok = false;
+  }
+	  }
 }
 }
 
@@ -545,6 +598,8 @@ main (int argc, char **argv)
   root_dev_ino = NULL;
 }
 
+  euid = geteuid ();
+
   ok = process_files (argv + optind,
   FTS_COMFOLLOW | FTS_PHYSICAL | FTS_DEFER_STAT);
 
-- 
1.5.4.3

Re: modify chmod

[jeff.liu]

Sorry for the inconvenient, looks my email body was filtered somehow, :-(

Re: modify chmod

[jeff.liu]

Re: modify chmod

[jeff.liu]

A tiny patch, make chmod do not touch the inode if the new file
permission mode is same as it was before.

From c7bc43a05e4f021db8047ecd390b4fb75368f558 Mon Sep 17 00:00:00 2001
From: Jeff Liu jeff@oracle.com
Date: Fri, 5 Feb 2010 20:45:41 +0800
Subject: [PATCH 1/1] Modify chmod v1
do not touch the inode if the new file permission bits is identical to
it was before.

Signed-off-by: Jie Liu jeff@oracle.com
---
gnulib | 2 +-
src/chmod.c | 75 +++
2 files changed, 66 insertions(+), 11 deletions(-)

diff --git a/gnulib b/gnulib
index 2eb5a8a..4b93a25 16
--- a/gnulib
+++ b/gnulib
@@ -1 +1 @@
-Subproject commit 2eb5a8a0ff8348149a9ca985e2ccbfb03bc8de53
+Subproject commit 4b93a2579fb567b9fbbeb24439770d814dac95cd
diff --git a/src/chmod.c b/src/chmod.c
index 3dab92e..30802d7 100644
--- a/src/chmod.c
+++ b/src/chmod.c
@@ -20,6 +20,7 @@
#include stdio.h
#include getopt.h
#include sys/types.h
+#include unistd.h

#include system.h
#include dev-ino.h
@@ -32,6 +33,16 @@
#include root-dev-ino.h
#include xfts.h

+/* Some systems only have sys/vfs.h, other systems also
+ * have sys/statfs.h, where the former includes the latter.
+ * So it seems including the former is the best choice. */
+#include fs.h
+#if HAVE_SYS_VFS_H
+# include sys/vfs.h
+#else
+# include sys/statfs.h
+#endif
+
/* The official name of this program (e.g., no `g' prefix). */
#define PROGRAM_NAME chmod

@@ -83,6 +94,9 @@ static enum Verbosity verbosity = V_off;
Otherwise NULL. */
static struct dev_ino *root_dev_ino;

+/* The effective user ID of the caller process. */
+static uid_t euid;
+
/* For long options that have no equivalent short option, use a
non-character as a pseudo short option, starting with CHAR_MAX + 1. */
enum
@@ -170,6 +184,25 @@ describe_change (const char *file, mode_t mode,
(unsigned long int) (mode  CHMOD_MODE_BITS), perms[1]);
}

+/* Return true if the file resides on NFS filesystem.
+ * limit this optimization to systems that provide statfs. */
+
+static bool
+may_have_nfsacl(const char *file)
+{
+# if HAVE_SYS_VFS_H  HAVE_SYS_STATFS_H  HAVE_STRUCT_STATFS_F_TYPE
+ struct statfs buf;
+
+ /* If statfs fails, assume we can't use the optimization. */
+ if (statfs (file, buf)  0)
+ return true;
+
+ return buf.f_type == S_MAGIC_NFS;
+#endif
+
+ return true;
+}
+
/* Change the mode of FILE.
Return true if successful. This function is called
once for every file system object that fts encounters. */
@@ -257,18 +290,38 @@ process_file (FTS *fts, FTSENT *ent)
old_mode = file_stats-st_mode;
new_mode = mode_adjust (old_mode, S_ISDIR (old_mode) != 0, umask_value,
change, NULL);
-
- if (! S_ISLNK (old_mode))
+
+ /* Do not touch the inode if the new file mode is the same as it was
before;
+ * This is an optimization for some cases. However, the new behavior
must be
+ * disabled for filesystems that support NFSv4 ACLs.
+ * The idea suggested by Paul Eggert refer to FreeBSD chmod(1).
+ * it uses pathconf(2)/lpathconf(2) to determine whether this is the case.
+ * but on linux, we lack of them. Instead, calling statfs(2) and
compare the
+ * f_type we can still do optimization at least its not NFS. */
+
+ if ((old_mode  CHMOD_MODE_BITS) == new_mode 
+ !may_have_nfsacl (file_full_name))
{
- if (chmodat (fts-fts_cwd_fd, file, new_mode) == 0)
- chmod_succeeded = true;
+ if (file_stats-st_uid != euid  euid != 0)
+ error (0, 0, _(changing permissions of %s: Operation not permitted),
+ quote (file_full_name));
else
- {
- if (! force_silent)
- error (0, errno, _(changing permissions of %s),
- quote (file_full_name));
- ok = false;
- }
+ chmod_succeeded = true;
+ }
+ else
+ {
+ if (! S_ISLNK (old_mode))
+ {
+ if (chmodat (fts-fts_cwd_fd, file, new_mode) == 0)
+ chmod_succeeded = true;
+ else
+ {
+ if (! force_silent)
+ error (0, errno, _(changing permissions of %s),
+ quote (file_full_name));
+ ok = false;
+ }
+ }
}
}

@@ -545,6 +598,8 @@ main (int argc, char **argv)
root_dev_ino = NULL;
}

+ euid = geteuid ();
+
ok = process_files (argv + optind,
FTS_COMFOLLOW | FTS_PHYSICAL | FTS_DEFER_STAT);

-- 
1.5.4.3

Re: modify chmod

[Eric Blake]

According to jeff.liu on 2/5/2010 7:44 AM:
 +++ b/gnulib
 @@ -1 +1 @@
 -Subproject commit 2eb5a8a0ff8348149a9ca985e2ccbfb03bc8de53
 +Subproject commit 4b93a2579fb567b9fbbeb24439770d814dac95cd

Why are you modifying the gnulib submodule?

 
 +/* Some systems only have sys/vfs.h, other systems also
 + * have sys/statfs.h, where the former includes the latter.
 + * So it seems including the former is the best choice. */
 +#include fs.h
 +#if HAVE_SYS_VFS_H
 +# include sys/vfs.h
 +#else
 +# include sys/statfs.h
 +#endif

Hmm.  POSIX standardized sys/statvfs.h, not sys/vfs.h.  Maybe a better
approach would be to propose a patch to gnulib that guarantees that
sys/statvfs.h provides everything we need, so that we don't have to ever
worry about sys/vfs.h in coreutils.

 
 +/* Return true if the file resides on NFS filesystem.
 + * limit this optimization to systems that provide statfs. */

This comment formatting is not consistent with other comments in the file.


 +
 +static bool
 +may_have_nfsacl(const char *file)

Missing a space between the function name and the (.  And coreutils
prefers 'char const *' over the synonymous 'const char *'.

 +{
 +# if HAVE_SYS_VFS_H  HAVE_SYS_STATFS_H  HAVE_STRUCT_STATFS_F_TYPE
 + struct statfs buf;
 +
 + /* If statfs fails, assume we can't use the optimization. */
 + if (statfs (file, buf)  0)
 + return true;
 +
 + return buf.f_type == S_MAGIC_NFS;
 +#endif
 +
 + return true;

Rather than have #ifdefs inside the function body, this seems like it
would be better to do:

#ifdef ...
function
#else
# define may_have_nfsacl(ignored) true
#else

 +}
 +
 /* Change the mode of FILE.
 Return true if successful. This function is called
 once for every file system object that fts encounters. */
 @@ -257,18 +290,38 @@ process_file (FTS *fts, FTSENT *ent)
 old_mode = file_stats-st_mode;
 new_mode = mode_adjust (old_mode, S_ISDIR (old_mode) != 0, umask_value,
 change, NULL);
 -
 - if (! S_ISLNK (old_mode))
 +
 + /* Do not touch the inode if the new file mode is the same as it was
 before;
 + * This is an optimization for some cases. However, the new behavior
 must be
 + * disabled for filesystems that support NFSv4 ACLs.
 + * The idea suggested by Paul Eggert refer to FreeBSD chmod(1).
 + * it uses pathconf(2)/lpathconf(2) to determine whether this is the case.
 + * but on linux, we lack of them. Instead, calling statfs(2) and
 compare the
 + * f_type we can still do optimization at least its not NFS. */

Again, inconsistent comment formatting.

 + if (file_stats-st_uid != euid  euid != 0)
 + error (0, 0, _(changing permissions of %s: Operation not permitted),
 + quote (file_full_name));

Indentation in this patch looks wrong; did it get munged?  Also, typically
the Operation not permitted is provided for free by passing the
appropriate errno to error(), rather than open-coding it into the error
string.

-- 
Don't work too hard, make some time for fun as well!

Eric Blake e...@byu.net



signature.asc
Description: OpenPGP digital signature

Re: modify chmod

[jeff.liu]

Re: modify chmod

[jeff.liu]

Re: modify chmod

[Jim Meyering]

jeff.liu wrote:
...
   http://git.sv.gnu.org/cgit/coreutils.git/plain/HACKING

 Sure,  that's sounds interesting.

 For the copyright assignment, Oracle did, it as a company, and the assignment
 covers all the employees. :-)

Confirmed.  That was easy ;-)

 Just one thing need to statement here, does it has a deadline?

No.  But if someday you realize you're not going to do it
after all, please give us a heads-up.

 I can not promise it could be done in a precise time,
 I am coding in my spare time for fun.

You'll still need to file an individual copyright assignment.

Re: modify chmod

[jeff.liu]

Re: modify chmod

[jeff.liu]

Re: modify chmod

[jeff.liu]

Re: modify chmod

[Jim Meyering]

Paul Eggert wrote:
 Jim Meyering j...@meyering.net writes:
 This new behavior could be useful...
 maybe noticeably more efficient in some cases, too.
 Perhaps worth a new option, if not the default.

 In rereading the old bug report, I noticed that FreeBSD is listed as the
 only OS that does things the more-efficient way.  I looked at the
 FreeBSD 8.0 chmod.c source, and what it actually does is apply the
 optimization only if it knows that the file system does not have ACLs
 (it calls something named may_have_nfs4acl, because apparently NFSv4
 ACLs might change even if you apply a mode that's the same as before).
 I suggest that we do something similar.

Good idea.

I took a look.  It uses pathconf/lpathconf with _PC_ACL_NFS4.
Since in general, we have neither the lpathconf function,
nor its _PC_ACL_NFS4, we'll have to roll our own.  But that
is not too hard:  it can do the same sort of thing fts.c does,
in calling fstatfs and comparing statfs.f_type to at least
S_MAGIC_NFS (from fs.h).

IMHO, it's fine to limit this optimization to systems that
provide fstatfs.

Jeff, are you still interested in pursuing this?
If so, please start the copyright assignment process with
your employer.  See the Copyright assignment section
in the HACKING file:
  http://git.sv.gnu.org/cgit/coreutils.git/plain/HACKING

Re: modify chmod

[Jim Meyering]

jeff.liu wrote:
 It looks the Modify chmod task has been in TODO list for a long time.

 I have looked through the discussion thread referred to
 http://bugs.debian.org/497514.

 Does it make sense to make chmod do not change the inode's ctime
 if the new permission bits is identical to the old as the default behavior?

 I'd like to handle it if it is still a valid task and nobody is working
 on for now.

Hi Jeff,

Thanks for bringing that up.
I refreshed my memory, wrote a tiny patch
and tested the result.  Running as non-root,
must chmod u+x / fail?  With coreutils-8.4, it does:

$ chmod u+x /
chmod: changing permissions of `/': Operation not permitted

With the patch below, it skips the chmodat call altogether, because
chmod notices that u+x would not modify the existing permissions:

$ ./chmod u+x /
$

If we were to insist that the modified chmod
produce the same diagnostic, we *could* try.
But we'd have to compare effective uid to stat.st_uid, ...
and what about the possibility of ACLs?  So I think that is
not an option.

This new behavior could be useful...
maybe noticeably more efficient in some cases, too.
Perhaps worth a new option, if not the default.

Opinions?

diff --git a/src/chmod.c b/src/chmod.c
index 3dab92e..838ba2f 100644
--- a/src/chmod.c
+++ b/src/chmod.c
@@ -258,16 +258,24 @@ process_file (FTS *fts, FTSENT *ent)
   new_mode = mode_adjust (old_mode, S_ISDIR (old_mode) != 0, umask_value,
   change, NULL);

-  if (! S_ISLNK (old_mode))
+  if ((old_mode  CHMOD_MODE_BITS) == new_mode)
 {
-  if (chmodat (fts-fts_cwd_fd, file, new_mode) == 0)
-chmod_succeeded = true;
-  else
+  /* Perm+special bits are the same, so skip the chmod altogether.  */
+  chmod_succeeded = true;
+}
+  else
+{
+  if (! S_ISLNK (old_mode))
 {
-  if (! force_silent)
-error (0, errno, _(changing permissions of %s),
-   quote (file_full_name));
-  ok = false;
+  if (chmodat (fts-fts_cwd_fd, file, new_mode) == 0)
+chmod_succeeded = true;
+  else
+{
+  if (! force_silent)
+error (0, errno, _(changing permissions of %s),
+   quote (file_full_name));
+  ok = false;
+}
 }
 }
 }

Re: modify chmod

[Eric Blake]

According to Jim Meyering on 1/30/2010 8:50 AM:
 Does it make sense to make chmod do not change the inode's ctime
 if the new permission bits is identical to the old as the default behavior?

I still think POSIX allows it.  Rather than calling out chmod(2) behavior,
POSIX states that for chmod(1):

Upon successfully changing the file mode bits of a file, the chmod
utility shall mark for update the last file status change timestamp of the
file.

 With the patch below, it skips the chmodat call altogether, because
 chmod notices that u+x would not modify the existing permissions:
 
 $ ./chmod u+x /
 $
 
 If we were to insist that the modified chmod
 produce the same diagnostic, we *could* try.
 But we'd have to compare effective uid to stat.st_uid, ...
 and what about the possibility of ACLs?  So I think that is
 not an option.
 
 This new behavior could be useful...
 maybe noticeably more efficient in some cases, too.
 Perhaps worth a new option, if not the default.

I think that optimizing no-op chmod(1) by default is reasonable, but it
would still be nice to have an option to guarantee a chmod(2) call (for
its ACL side-effects, even if the permission bits appear to be a no-op).

 -  if (! S_ISLNK (old_mode))
 +  if ((old_mode  CHMOD_MODE_BITS) == new_mode)

One other thing - I'm still hoping to find some time to work on:

providing a gnulib replacement to work around Solaris 9's bug ('chmod
file/' mistakenly succeeds)

teaching coreutils chmod to use lchmod (and/or
fchmodat(,AT_SYMLINK_NOFOLLOW)) on systems that support it (BSD systems at
the moment), such that we can copy BSD's 'chmod -h' and 'chmod -PR'.

Such a patch would impact the same areas of code as the patch under
discussion in this thread.

-- 
Don't work too hard, make some time for fun as well!

Eric Blake e...@byu.net



signature.asc
Description: OpenPGP digital signature

Re: modify chmod

[Paul Eggert]

Jim Meyering j...@meyering.net writes:

 This new behavior could be useful...
 maybe noticeably more efficient in some cases, too.
 Perhaps worth a new option, if not the default.

In rereading the old bug report, I noticed that FreeBSD is listed as the
only OS that does things the more-efficient way.  I looked at the
FreeBSD 8.0 chmod.c source, and what it actually does is apply the
optimization only if it knows that the file system does not have ACLs
(it calls something named may_have_nfs4acl, because apparently NFSv4
ACLs might change even if you apply a mode that's the same as before).
I suggest that we do something similar.

modify chmod

[jeff.liu]

Hi Jim,

It looks the Modify chmod task has been in TODO list for a long time.

I have looked through the discussion thread referred to
http://bugs.debian.org/497514.

Does it make sense to make chmod do not change the inode's ctime
if the new permission bits is identical to the old as the default behavior?

I'd like to handle it if it is still a valid task and nobody is working
on for now.


Regards,
-Jeff