Patchworkβ [04/13] sysfs: Simplify sysfs_chmod_file semantics

From: Eric W. Biederman <ebiederm@xmission.com>

Currently every caller of sysfs_chmod_file happens at either
file creation time to set a non-default mode or in response
to a specific user requested space change in policy.  Making
timestamps of when the chmod happens and notification of
a file changing mode uninteresting.

Remove the unnecessary time stamp and filesystem change
notification, and removes the last of the explicit inotify
and donitfy support from sysfs.

Acked-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 fs/sysfs/file.c |   10 +---------
 1 files changed, 1 insertions(+), 9 deletions(-)

Quoting Eric W. Biederman (ebiederm@xmission.com):
> From: Eric W. Biederman <ebiederm@xmission.com>
> 
> Currently every caller of sysfs_chmod_file happens at either
> file creation time to set a non-default mode or in response
> to a specific user requested space change in policy.  Making
> timestamps of when the chmod happens and notification of
> a file changing mode uninteresting.

But these changes can occur by togging values in sysfs files
(i.e. f71805f.c), right?  Is this (specifically not doing inotify)
definately uncontroversial?

I can't exactly picture an admin sitting there watching
nautilus for a sysfs file to become writeable, but could
imagine some site's automation getting hung...  Or am I way
off base?

> Remove the unnecessary time stamp and filesystem change
> notification, and removes the last of the explicit inotify
> and donitfy support from sysfs.
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
> ---
>  fs/sysfs/file.c |   10 +---------
>  1 files changed, 1 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index f5ea468..faa1a80 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -604,17 +604,9 @@ int sysfs_chmod_file(struct kobject *kobj, struct attribute *attr, mode_t mode)
>  	mutex_lock(&inode->i_mutex);
> 
>  	newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
> -	newattrs.ia_valid = ATTR_MODE | ATTR_CTIME;
> -	newattrs.ia_ctime = current_fs_time(inode->i_sb);
> +	newattrs.ia_valid = ATTR_MODE;
>  	rc = sysfs_setattr(victim, &newattrs);
> 
> -	if (rc == 0) {
> -		fsnotify_change(victim, newattrs.ia_valid);
> -		mutex_lock(&sysfs_mutex);
> -		victim_sd->s_mode = newattrs.ia_mode;
> -		mutex_unlock(&sysfs_mutex);
> -	}
> -
>  	mutex_unlock(&inode->i_mutex);
>   out:
>  	dput(victim);
> -- 
> 1.6.5.2.143.g8cc62
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

"Serge E. Hallyn" <serue@us.ibm.com> writes:

> Quoting Eric W. Biederman (ebiederm@xmission.com):
>> From: Eric W. Biederman <ebiederm@xmission.com>
>> 
>> Currently every caller of sysfs_chmod_file happens at either
>> file creation time to set a non-default mode or in response
>> to a specific user requested space change in policy.  Making
>> timestamps of when the chmod happens and notification of
>> a file changing mode uninteresting.
>
> But these changes can occur by togging values in sysfs files
> (i.e. f71805f.c), right?  Is this (specifically not doing inotify)
> definately uncontroversial?

The fs_notify_change was not introduced to deliberately support
a feature but as a side effect of other cleanups.  So there
is no indication that anyone cares about inotify support.

> I can't exactly picture an admin sitting there watching
> nautilus for a sysfs file to become writeable, but could
> imagine some site's automation getting hung...  Or am I way
> off base?

I would be stunned if the shell script in the automation that writes
to a sysfs file to make things writeable doesn't on it's next line
kick off whatever needs it to be writable.

With no benefit to using inotify and with only a handful of sysfs
files affected I don't expect this change to break anything in
userspace and I have been happily running with it for a year or so on
all of our machines at work with no one problems.

The reason I am making the change is that the goal of this patchset is
to get sysfs to act like any other distributed filesystem in linux,
and to use the same interfaces in roughly the same ways as other
distributed filesystems.  Unfortunately there is not a good interface
for distributed filesystems to support inotify or I would use it.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoting Eric W. Biederman (ebiederm@xmission.com):
> "Serge E. Hallyn" <serue@us.ibm.com> writes:
> 
> > Quoting Eric W. Biederman (ebiederm@xmission.com):
> >> From: Eric W. Biederman <ebiederm@xmission.com>
> >> 
> >> Currently every caller of sysfs_chmod_file happens at either
> >> file creation time to set a non-default mode or in response
> >> to a specific user requested space change in policy.  Making
> >> timestamps of when the chmod happens and notification of
> >> a file changing mode uninteresting.
> >
> > But these changes can occur by togging values in sysfs files
> > (i.e. f71805f.c), right?  Is this (specifically not doing inotify)
> > definately uncontroversial?
> 
> The fs_notify_change was not introduced to deliberately support
> a feature but as a side effect of other cleanups.  So there
> is no indication that anyone cares about inotify support.
> 
> > I can't exactly picture an admin sitting there watching
> > nautilus for a sysfs file to become writeable, but could
> > imagine some site's automation getting hung...  Or am I way
> > off base?
> 
> I would be stunned if the shell script in the automation that writes
> to a sysfs file to make things writeable doesn't on it's next line
> kick off whatever needs it to be writable.
> 
> With no benefit to using inotify and with only a handful of sysfs
> files affected I don't expect this change to break anything in
> userspace and I have been happily running with it for a year or so on
> all of our machines at work with no one problems.
> 
> The reason I am making the change is that the goal of this patchset is
> to get sysfs to act like any other distributed filesystem in linux,
> and to use the same interfaces in roughly the same ways as other
> distributed filesystems.  Unfortunately there is not a good interface
> for distributed filesystems to support inotify or I would use it.
> 
> Eric

Ok - I personally agree, but I know there are admins out there with
very different mindsets from mine

Acked-by: Serge Hallyn <serue@us.ibm.com>

-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/