[PATCH 0/13] sysfs lazification.

Quoting Eric W. Biederman (ebiederm@xmission.com):
> From: Eric W. Biederman <ebiederm RemoveThis @xmission.com>
>
> Now that sysfs_getattr and sysfs_permission refresh the vfs
> inode there is no need to immediatly push the mode change
> into the vfs cache. Reducing the amount of work needed and
> simplifying the locking.
>
> Acked-by: Tejun Heo <tj RemoveThis @kernel.org>
> Signed-off-by: Eric W. Biederman <ebiederm RemoveThis @aristanetworks.com>

Nice.

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

> ---
> fs/sysfs/file.c | 31 ++++++++-----------------------
> 1 files changed, 8 insertions(+), 23 deletions(-)
>
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index faa1a80..dc30d9e 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -579,38 +579,23 @@ EXPORT_SYMBOL_GPL(sysfs_add_file_to_group);
> */
> int sysfs_chmod_file(struct kobject *kobj, struct attribute *attr, mode_t mode)
> {
> - struct sysfs_dirent *victim_sd = NULL;
> - struct dentry *victim = NULL;
> - struct inode * inode;
> + struct sysfs_dirent *sd;
> struct iattr newattrs;
> int rc;
>
> - rc = -ENOENT;
> - victim_sd = sysfs_get_dirent(kobj->sd, attr->name);
> - if (!victim_sd)
> - goto out;
> + mutex_lock(&sysfs_mutex);
>
> - mutex_lock(&sysfs_rename_mutex);
> - victim = sysfs_get_dentry(victim_sd);
> - mutex_unlock(&sysfs_rename_mutex);
> - if (IS_ERR(victim)) {
> - rc = PTR_ERR(victim);
> - victim = NULL;
> + rc = -ENOENT;
> + sd = sysfs_find_dirent(kobj->sd, attr->name);
> + if (!sd)
> goto out;
> - }
> -
> - inode = victim->d_inode;
>
> - mutex_lock(&inode->i_mutex);
> -
> - newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
> + newattrs.ia_mode = (mode & S_IALLUGO) | (sd->s_mode & ~S_IALLUGO);
> newattrs.ia_valid = ATTR_MODE;
> - rc = sysfs_setattr(victim, &newattrs);
> + rc = sysfs_sd_setattr(sd, &newattrs);
>
> - mutex_unlock(&inode->i_mutex);
> out:
> - dput(victim);
> - sysfs_put(victim_sd);
> + mutex_unlock(&sysfs_mutex);
> return rc;
> }
> EXPORT_SYMBOL_GPL(sysfs_chmod_file);
> --
> 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 RemoveThis @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 RemoveThis @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):
> From: Eric W. Biederman <ebiederm RemoveThis @xmission.com>
>
> With the implementation of sysfs_getattr and sysfs_permission
> sysfs becomes able to lazily propogate inode attribute changes
> from the sysfs_dirents to the vfs inodes. This paves the way
> for deleting significant chunks of now unnecessary code.
>
> Acked-by: Tejun Heo <tj RemoveThis @kernel.org>
> Signed-off-by: Eric W. Biederman <ebiederm RemoveThis @aristanetworks.com>
> ---
> fs/sysfs/dir.c | 2 +
> fs/sysfs/inode.c | 64 ++++++++++++++++++++++++++++++++++++++-------------
> fs/sysfs/symlink.c | 3 ++
> fs/sysfs/sysfs.h | 2 +
> 4 files changed, 54 insertions(+), 17 deletions(-)
>
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index fa37126..25d052a 100644
> --- a/fs/sysfs/dir.c
> +++ b/fs/sysfs/dir.c
> @@ -800,7 +800,9 @@ static struct dentry * sysfs_lookup(struct inode *dir, struct dentry *dentry,
>
> const struct inode_operations sysfs_dir_inode_operations = {
> .lookup = sysfs_lookup,
> + .permission = sysfs_permission,
> .setattr = sysfs_setattr,
> + .getattr = sysfs_getattr,
> .setxattr = sysfs_setxattr,
> };
>
> diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
> index fccfb55..2dcafe8 100644
> --- a/fs/sysfs/inode.c
> +++ b/fs/sysfs/inode.c
> @@ -37,7 +37,9 @@ static struct backing_dev_info sysfs_backing_dev_info = {
> };
>
> static const struct inode_operations sysfs_inode_operations ={
> + .permission = sysfs_permission,
> .setattr = sysfs_setattr,
> + .getattr = sysfs_getattr,
> .setxattr = sysfs_setxattr,
> };
>
> @@ -196,7 +198,6 @@ static inline void set_default_inode_attr(struct inode * inode, mode_t mode)
>
> static inline void set_inode_attr(struct inode * inode, struct iattr * iattr)
> {
> - inode->i_mode = iattr->ia_mode;
> inode->i_uid = iattr->ia_uid;
> inode->i_gid = iattr->ia_gid;
> inode->i_atime = iattr->ia_atime;
> @@ -227,38 +228,56 @@ static int sysfs_count_nlink(struct sysfs_dirent *sd)
> return nr + 2;
> }
>
> +static void sysfs_refresh_inode(struct sysfs_dirent *sd, struct inode *inode)
> +{
> + struct sysfs_inode_attrs *iattrs = sd->s_iattr;
> +
> + inode->i_mode = sd->s_mode;
> + if (iattrs) {
> + /* sysfs_dirent has non-default attributes
> + * get them from persistent copy in sysfs_dirent
> + */
> + set_inode_attr(inode, &iattrs->ia_iattr);
> + security_inode_notifysecctx(inode,
> + iattrs->ia_secdata,
> + iattrs->ia_secdata_len);
> + }
> +
> + if (sysfs_type(sd) == SYSFS_DIR)
> + inode->i_nlink = sysfs_count_nlink(sd);
> +}
> +
> +int sysfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat)
> +{
> + struct sysfs_dirent *sd = dentry->d_fsdata;
> + struct inode *inode = dentry->d_inode;
> +
> + mutex_lock(&sysfs_mutex);
> + sysfs_refresh_inode(sd, inode);
> + mutex_unlock(&sysfs_mutex);

So the inode->i_mutex is not needed?

> +
> + generic_fillattr(inode, stat);
> + return 0;
> +}
> +
> static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode)
> {
> struct bin_attribute *bin_attr;
> - struct sysfs_inode_attrs *iattrs;
>
> inode->i_private = sysfs_get(sd);
> inode->i_mapping->a_ops = &sysfs_aops;
> inode->i_mapping->backing_dev_info = &sysfs_backing_dev_info;
> inode->i_op = &sysfs_inode_operations;
> - inode->i_ino = sd->s_ino;
> lockdep_set_class(&inode->i_mutex, &sysfs_inode_imutex_key);
>
> - iattrs = sd->s_iattr;
> - if (iattrs) {
> - /* sysfs_dirent has non-default attributes
> - * get them for the new inode from persistent copy
> - * in sysfs_dirent
> - */
> - set_inode_attr(inode, &iattrs->ia_iattr);
> - if (iattrs->ia_secdata)
> - security_inode_notifysecctx(inode,
> - iattrs->ia_secdata,
> - iattrs->ia_secdata_len);
> - } else
> - set_default_inode_attr(inode, sd->s_mode);
> + set_default_inode_attr(inode, sd->s_mode);
> + sysfs_refresh_inode(sd, inode);
>
> /* initialize inode according to type */
> switch (sysfs_type(sd)) {
> case SYSFS_DIR:
> inode->i_op = &sysfs_dir_inode_operations;
> inode->i_fop = &sysfs_dir_operations;
> - inode->i_nlink = sysfs_count_nlink(sd);
> break;
> case SYSFS_KOBJ_ATTR:
> inode->i_size = PAGE_SIZE;
> @@ -341,3 +360,14 @@ int sysfs_hash_and_remove(struct sysfs_dirent *dir_sd, const char *name)
> else
> return -ENOENT;
> }
> +
> +int sysfs_permission(struct inode *inode, int mask)
> +{
> + struct sysfs_dirent *sd = inode->i_private;
> +
> + mutex_lock(&sysfs_mutex);
> + sysfs_refresh_inode(sd, inode);
> + mutex_unlock(&sysfs_mutex);
> +
> + return generic_permission(inode, mask, NULL);
> +}
> diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
> index 1137418..c5eff49 100644
> --- a/fs/sysfs/symlink.c
> +++ b/fs/sysfs/symlink.c
> @@ -214,6 +214,9 @@ const struct inode_operations sysfs_symlink_inode_operations = {
> .readlink = generic_readlink,
> .follow_link = sysfs_follow_link,
> .put_link = sysfs_put_link,
> + .setattr = sysfs_setattr,
> + .getattr = sysfs_getattr,
> + .permission = sysfs_permission,
> };
>
>
> diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
> index a96d967..12ccc07 100644
> --- a/fs/sysfs/sysfs.h
> +++ b/fs/sysfs/sysfs.h
> @@ -156,7 +156,9 @@ static inline void __sysfs_put(struct sysfs_dirent *sd)
> struct inode *sysfs_get_inode(struct sysfs_dirent *sd);
> void sysfs_delete_inode(struct inode *inode);
> int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr *iattr);
> +int sysfs_permission(struct inode *inode, int mask);
> int sysfs_setattr(struct dentry *dentry, struct iattr *iattr);
> +int sysfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat);
> int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
> size_t size, int flags);
> int sysfs_hash_and_remove(struct sysfs_dirent *dir_sd, const char *name);
> --
> 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 RemoveThis @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 RemoveThis @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):
> From: Eric W. Biederman <ebiederm DeleteThis @xmission.com>
>
> Lining up the functions in sysfs_symlink_inode_operations
> follows the pattern in the rest of sysfs and makes things
> slightly more readable.
>
> Acked-by: Tejun Heo <tj DeleteThis @kernel.org>
> Signed-off-by: Eric W. Biederman <ebiederm DeleteThis @aristanetworks.com>

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

> ---
> fs/sysfs/symlink.c | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
> index c5081ad..1137418 100644
> --- a/fs/sysfs/symlink.c
> +++ b/fs/sysfs/symlink.c
> @@ -210,10 +210,10 @@ static void sysfs_put_link(struct dentry *dentry, struct nameidata *nd, void *co
> }
>
> const struct inode_operations sysfs_symlink_inode_operations = {
> - .setxattr = sysfs_setxattr,
> - .readlink = generic_readlink,
> - .follow_link = sysfs_follow_link,
> - .put_link = sysfs_put_link,
> + .setxattr = sysfs_setxattr,
> + .readlink = generic_readlink,
> + .follow_link = sysfs_follow_link,
> + .put_link = sysfs_put_link,
> };
>
>
> --
> 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 DeleteThis @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 DeleteThis @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):
> From: Eric W. Biederman <ebiederm DeleteThis @xmission.com>
>
> Using dentry instead of d in the function name is what
> several other filesystems are doing and it seems to be
> a more readable convention.
>
> Acked-by: Tejun Heo <tj DeleteThis @kernel.org>
> Signed-off-by: Eric W. Biederman <ebiederm DeleteThis @aristanetworks.com>

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

> ---
> fs/sysfs/dir.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index e020183..130dfc3 100644
> --- a/fs/sysfs/dir.c
> +++ b/fs/sysfs/dir.c
> @@ -298,7 +298,7 @@ void release_sysfs_dirent(struct sysfs_dirent * sd)
> goto repeat;
> }
>
> -static void sysfs_d_iput(struct dentry * dentry, struct inode * inode)
> +static void sysfs_dentry_iput(struct dentry * dentry, struct inode * inode)
> {
> struct sysfs_dirent * sd = dentry->d_fsdata;
>
> @@ -307,7 +307,7 @@ static void sysfs_d_iput(struct dentry * dentry, struct inode * inode)
> }
>
> static const struct dentry_operations sysfs_dentry_ops = {
> - .d_iput = sysfs_d_iput,
> + .d_iput = sysfs_dentry_iput,
> };
>
> struct sysfs_dirent *sysfs_new_dirent(const char *name, umode_t mode, int type)
> --
> 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 DeleteThis @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 DeleteThis @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):
> From: Eric W. Biederman <ebiederm.TakeThisOut@aristanetworks.com>
>
> With lazy inode updates and dentry operations bringing everything
> into sync on demand there is no longer any need to immediately
> update the vfs or grab i_mutex to protect those updates as we
> make changes to sysfs.
>
> Signed-off-by: Eric W. Biederman <ebiederm.TakeThisOut@aristanetworks.com>

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

> ---
> fs/sysfs/dir.c | 91 ++---------------------------------------------------
> fs/sysfs/sysfs.h | 2 -
> 2 files changed, 4 insertions(+), 89 deletions(-)
>
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index 25d052a..a05b027 100644
> --- a/fs/sysfs/dir.c
> +++ b/fs/sysfs/dir.c
> @@ -386,12 +386,6 @@ struct sysfs_dirent *sysfs_new_dirent(const char *name, umode_t mode, int type)
> return NULL;
> }
>
> -static int sysfs_ilookup_test(struct inode *inode, void *arg)
> -{
> - struct sysfs_dirent *sd = arg;
> - return inode->i_ino == sd->s_ino;
> -}
> -
> /**
> * sysfs_addrm_start - prepare for sysfs_dirent add/remove
> * @acxt: pointer to sysfs_addrm_cxt to be used
> @@ -399,47 +393,20 @@ static int sysfs_ilookup_test(struct inode *inode, void *arg)
> *
> * This function is called when the caller is about to add or
> * remove sysfs_dirent under @parent_sd. This function acquires
> - * sysfs_mutex, grabs inode for @parent_sd if available and lock
> - * i_mutex of it. @acxt is used to keep and pass context to
> + * sysfs_mutex. @acxt is used to keep and pass context to
> * other addrm functions.
> *
> * LOCKING:
> * Kernel thread context (may sleep). sysfs_mutex is locked on
> - * return. i_mutex of parent inode is locked on return if
> - * available.
> + * return.
> */
> void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,
> struct sysfs_dirent *parent_sd)
> {
> - struct inode *inode;
> -
> memset(acxt, 0, sizeof(*acxt));
> acxt->parent_sd = parent_sd;
>
> - /* Lookup parent inode. inode initialization is protected by
> - * sysfs_mutex, so inode existence can be determined by
> - * looking up inode while holding sysfs_mutex.
> - */
> mutex_lock(&sysfs_mutex);
> -
> - inode = ilookup5(sysfs_sb, parent_sd->s_ino, sysfs_ilookup_test,
> - parent_sd);
> - if (inode) {
> - WARN_ON(inode->i_state & I_NEW);
> -
> - /* parent inode available */
> - acxt->parent_inode = inode;
> -
> - /* sysfs_mutex is below i_mutex in lock hierarchy.
> - * First, trylock i_mutex. If fails, unlock
> - * sysfs_mutex and lock them in order.
> - */
> - if (!mutex_trylock(&inode->i_mutex)) {
> - mutex_unlock(&sysfs_mutex);
> - mutex_lock(&inode->i_mutex);
> - mutex_lock(&sysfs_mutex);
> - }
> - }
> }
>
> /**
> @@ -471,11 +438,6 @@ int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
>
> sd->s_parent = sysfs_get(acxt->parent_sd);
>
> - if (sysfs_type(sd) == SYSFS_DIR && acxt->parent_inode)
> - inc_nlink(acxt->parent_inode);
> -
> - acxt->cnt++;
> -
> sysfs_link_sibling(sd);
>
> /* Update timestamps on the parent */
> @@ -579,40 +541,6 @@ void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
> sd->s_flags |= SYSFS_FLAG_REMOVED;
> sd->s_sibling = acxt->removed;
> acxt->removed = sd;
> -
> - if (sysfs_type(sd) == SYSFS_DIR && acxt->parent_inode)
> - drop_nlink(acxt->parent_inode);
> -
> - acxt->cnt++;
> -}
> -
> -/**
> - * sysfs_dec_nlink - Decrement link count for the specified sysfs_dirent
> - * @sd: target sysfs_dirent
> - *
> - * Decrement nlink for @sd. @sd must have been unlinked from its
> - * parent on entry to this function such that it can't be looked
> - * up anymore.
> - */
> -static void sysfs_dec_nlink(struct sysfs_dirent *sd)
> -{
> - struct inode *inode;
> -
> - inode = ilookup(sysfs_sb, sd->s_ino);
> - if (!inode)
> - return;
> -
> - /* adjust nlink and update timestamp */
> - mutex_lock(&inode->i_mutex);
> -
> - inode->i_ctime = CURRENT_TIME;
> - drop_nlink(inode);
> - if (sysfs_type(sd) == SYSFS_DIR)
> - drop_nlink(inode);
> -
> - mutex_unlock(&inode->i_mutex);
> -
> - iput(inode);
> }
>
> /**
> @@ -621,25 +549,15 @@ static void sysfs_dec_nlink(struct sysfs_dirent *sd)
> *
> * Finish up sysfs_dirent add/remove. Resources acquired by
> * sysfs_addrm_start() are released and removed sysfs_dirents are
> - * cleaned up. Timestamps on the parent inode are updated.
> + * cleaned up.
> *
> * LOCKING:
> - * All mutexes acquired by sysfs_addrm_start() are released.
> + * sysfs_mutex is released.
> */
> void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt)
> {
> /* release resources acquired by sysfs_addrm_start() */
> mutex_unlock(&sysfs_mutex);
> - if (acxt->parent_inode) {
> - struct inode *inode = acxt->parent_inode;
> -
> - /* if added/removed, update timestamps on the parent */
> - if (acxt->cnt)
> - inode->i_ctime = inode->i_mtime = CURRENT_TIME;
> -
> - mutex_unlock(&inode->i_mutex);
> - iput(inode);
> - }
>
> /* kill removed sysfs_dirents */
> while (acxt->removed) {
> @@ -648,7 +566,6 @@ void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt)
> acxt->removed = sd->s_sibling;
> sd->s_sibling = NULL;
>
> - sysfs_dec_nlink(sd);
> sysfs_deactivate(sd);
> unmap_bin_file(sd);
> sysfs_put(sd);
> diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
> index 12ccc07..90b3501 100644
> --- a/fs/sysfs/sysfs.h
> +++ b/fs/sysfs/sysfs.h
> @@ -89,9 +89,7 @@ static inline unsigned int sysfs_type(struct sysfs_dirent *sd)
> */
> struct sysfs_addrm_cxt {
> struct sysfs_dirent *parent_sd;
> - struct inode *parent_inode;
> struct sysfs_dirent *removed;
> - int cnt;
> };
>
> /*
> --
> 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.TakeThisOut@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.TakeThisOut@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.DeleteThis@us.ibm.com> writes:
>
> > Quoting Eric W. Biederman (ebiederm@xmission.com):
> >> From: Eric W. Biederman <ebiederm.DeleteThis@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.DeleteThis@us.ibm.com>

-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@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.DeleteThis@us.ibm.com> writes:
>
> > Quoting Eric W. Biederman (ebiederm@xmission.com):
> >> From: Eric W. Biederman <ebiederm.DeleteThis@xmission.com>
> >>
> >> Currently sysfs updates the timestamps on the vfs directory
> >> inode when we create or remove a directory entry but doesn't
> >> update the cached copy on the sysfs_dirent, fix that oversight.
> >
> > confused... why not do this in sysfs_addrm_finish()?
> >
> > I guess you'd have to do at it at top before dropping sysfs_mutex
> > so it wouldn't be as pretty as I was thinking, but at least you
> > could just do it once.
>
> Well sysfs_addrm_finish doesn't really know if you did anything.

Oh right - well it used to through cnt right? but not after your
last patch.

> Beyond that my ultimate goal is to kill sysfs_addrm_start and
> sysfs_addrm_finish. Of course that requires fixing all of the
> sysfs users that depend on the impossible to get right recursive
> directory removal in sysfs, so it is not the subject of this patchset.

I didn't see the patch nixing inode->i_mtime (and cnt) changing from
sysfs_addrm_finish() until after responding. Got it now.

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

-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@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 RemoveThis @us.ibm.com> writes:

> So the inode->i_mutex is not needed?

Good question. Nothing in sysfs needs it. The VFS does not grab the
inode mutex on this path, but the vfs does grab the inode mutex when
writing to the inode.

Since the VFs isn't grabbing the inode_mutex there is probably a race in
here somewhere if someone looks at things just right.

I am too tired tonight to be that person.

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

In general everything that writes to vfs inodes holds the
inode mutex, so hold the inode mutex over sysfs_refresh_inode.
The sysfs data structures don't need this but it looks like the
vfs might.

Signed-off-by: Eric W. Biederman <ebiederm RemoveThis @aristanetworks.com>
---
fs/sysfs/inode.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index e2595a7..ad549f5 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -240,9 +240,11 @@ int sysfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *sta
struct sysfs_dirent *sd = dentry->d_fsdata;
struct inode *inode = dentry->d_inode;

+ mutex_lock(&inode->i_mutex);
mutex_lock(&sysfs_mutex);
sysfs_refresh_inode(sd, inode);
mutex_unlock(&sysfs_mutex);
+ mutex_unlock(&inode->i_mutex);

generic_fillattr(inode, stat);
return 0;
@@ -353,9 +355,11 @@ int sysfs_permission(struct inode *inode, int mask)
{
struct sysfs_dirent *sd = inode->i_private;

+ mutex_lock(&inode->i_mutex);
mutex_lock(&sysfs_mutex);
sysfs_refresh_inode(sd, inode);
mutex_unlock(&sysfs_mutex);
+ mutex_unlock(&inode->i_mutex);

return generic_permission(inode, mask, NULL);
}
--
1.6.5.2.143.g8cc62

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

inode_change_ok already clears the SGID bit when necessary so there is
no reason for sysfs_setattr to carry code to do the same, and it is
good to kill the extra copy because when I moved the code, I goofed
and in certain corner cases the code will look at the wrong gid.

Signed-off-by: Eric W. Biederman <ebiederm DeleteThis @aristanetworks.com>
---
fs/sysfs/inode.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index 72e2e99..e2595a7 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -120,10 +120,6 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
return error;

iattr->ia_valid &= ~ATTR_SIZE; /* ignore size changes */
- if (iattr->ia_valid & ATTR_MODE) {
- if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
- iattr->ia_mode &= ~S_ISGID;
- }

error = inode_setattr(inode, iattr);
if (error)
--
1.6.5.2.143.g8cc62

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

ebiederm.TakeThisOut@xmission.com (Eric W. Biederman) writes:

> "Serge E. Hallyn" <serue.TakeThisOut@us.ibm.com> writes:
>
>> Quoting Eric W. Biederman (ebiederm@xmission.com):
>>> From: Eric W. Biederman <ebiederm.TakeThisOut@xmission.com>
>>>
>>> Cleanly separate the work that is specific to setting the
>>> attributes of a sysfs_dirent from what is needed to update
>>> the attributes of a vfs inode.
>>>
>>> Additionally grab the sysfs_mutex to keep any nasties from
>>> surprising us when updating the sysfs_dirent.
>>>
>>> Acked-by: Tejun Heo <tj.TakeThisOut@kernel.org>
>>> Signed-off-by: Eric W. Biederman <ebiederm.TakeThisOut@aristanetworks.com>
>>> ---
>>> fs/sysfs/inode.c | 52 ++++++++++++++++++++++++++++++++--------------------
>>> fs/sysfs/sysfs.h | 1 +
>>> 2 files changed, 33 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
>>> index fed7a74..fccfb55 100644
>>> --- a/fs/sysfs/inode.c
>>> +++ b/fs/sysfs/inode.c
>>> @@ -64,30 +64,15 @@ struct sysfs_inode_attrs *sysfs_init_inode_attrs(struct sysfs_dirent *sd)
>>>
>>> return attrs;
>>> }
>>> -int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
>>> +
>>> +int sysfs_sd_setattr(struct sysfs_dirent *sd, struct iattr * iattr)
>>> {
>>> - struct inode * inode = dentry->d_inode;
>>> - struct sysfs_dirent * sd = dentry->d_fsdata;
>>> struct sysfs_inode_attrs *sd_attrs;
>>> struct iattr *iattrs;
>>> unsigned int ia_valid = iattr->ia_valid;
>>> - int error;
>>> -
>>> - if (!sd)
>>> - return -EINVAL;
>>>
>>> sd_attrs = sd->s_iattr;
>>>
>>> - error = inode_change_ok(inode, iattr);
>>> - if (error)
>>> - return error;
>>> -
>>> - iattr->ia_valid &= ~ATTR_SIZE; /* ignore size changes */
>>> -
>>> - error = inode_setattr(inode, iattr);
>>> - if (error)
>>> - return error;
>>> -
>>> if (!sd_attrs) {
>>> /* setting attributes for the first time, allocate now */
>>> sd_attrs = sysfs_init_inode_attrs(sd);
>>> @@ -110,12 +95,39 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
>>> iattrs->ia_ctime = iattr->ia_ctime;
>>> if (ia_valid & ATTR_MODE) {
>>> umode_t mode = iattr->ia_mode;
>>> -
>>> - if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
>>> - mode &= ~S_ISGID;
>>> iattrs->ia_mode = sd->s_mode = mode;
>>> }
>>> }
>>> + return 0;
>>> +}
>>> +
>>> +int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
>>> +{
>>> + struct inode * inode = dentry->d_inode;
>>> + struct sysfs_dirent * sd = dentry->d_fsdata;
>>> + int error;
>>> +
>>> + if (!sd)
>>> + return -EINVAL;
>>> +
>>> + error = inode_change_ok(inode, iattr);
>>> + if (error)
>>> + return error;
>>> +
>>> + iattr->ia_valid &= ~ATTR_SIZE; /* ignore size changes */
>>> + if (iattr->ia_valid & ATTR_MODE) {
>>> + if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
>>> + iattr->ia_mode &= ~S_ISGID;
>>> + }
>>
>> Was it a bug that before this patch this wasn't cleared before the
>> actual inode_setattr()?
>
> Not strictly as inode_setattr performs the exact same check.
>
>> Since the S_ISGID will be set for the *new* gid, that is,
>> iattr->ia_gid, shouldn't the user be required to be
>> in_group_p(iattr->i_gid)? Note you haven't done the
>> inode_setattr() yet.
>
> Interesting point, yes I am potentially testing the wrong gid
> there. The dangers of moving code around.
>
> Thank you for catching that. !#@#!#

Turns out it wasn't as bad as it looks because inode_change_ok already
does what I am trying to do properly.

Although come to think of it, SGID on a sysfs file is completely bogus.
We don't exec files and we don't allow users to create anything.

I have already sent the follow up patch to remove those few lines of code.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@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.TakeThisOut@us.ibm.com> writes:
>
> > So the inode->i_mutex is not needed?
>
> Good question. Nothing in sysfs needs it. The VFS does not grab the
> inode mutex on this path, but the vfs does grab the inode mutex when
> writing to the inode.

All callers of fs/attr.c:notify_change() do seem to take the i_mutex,
though. And Documentation/filesystem/Locking claims that ->setattr()
does need i_mutex. So I assume that setting of inode->i_ctime etc,
which is what you're doing here, needs to be protected by the i_mutex.

> Since the VFs isn't grabbing the inode_mutex there is probably a race in
> here somewhere if someone looks at things just right.
>
> I am too tired tonight to be that person.

The readers take no lock of any sort (i.e. generic_fillattr and its
callers) so IIUC they could get inconsistent data...

-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@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):
>
> In general everything that writes to vfs inodes holds the
> inode mutex, so hold the inode mutex over sysfs_refresh_inode.
> The sysfs data structures don't need this but it looks like the
> vfs might.
>
> Signed-off-by: Eric W. Biederman <ebiederm RemoveThis @aristanetworks.com>

Oh right so pls disregard my last reply to patch 9


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

> ---
> fs/sysfs/inode.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
> index e2595a7..ad549f5 100644
> --- a/fs/sysfs/inode.c
> +++ b/fs/sysfs/inode.c
> @@ -240,9 +240,11 @@ int sysfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *sta
> struct sysfs_dirent *sd = dentry->d_fsdata;
> struct inode *inode = dentry->d_inode;
>
> + mutex_lock(&inode->i_mutex);
> mutex_lock(&sysfs_mutex);
> sysfs_refresh_inode(sd, inode);
> mutex_unlock(&sysfs_mutex);
> + mutex_unlock(&inode->i_mutex);
>
> generic_fillattr(inode, stat);
> return 0;
> @@ -353,9 +355,11 @@ int sysfs_permission(struct inode *inode, int mask)
> {
> struct sysfs_dirent *sd = inode->i_private;
>
> + mutex_lock(&inode->i_mutex);
> mutex_lock(&sysfs_mutex);
> sysfs_refresh_inode(sd, inode);
> mutex_unlock(&sysfs_mutex);
> + mutex_unlock(&inode->i_mutex);
>
> return generic_permission(inode, mask, NULL);
> }
> --
> 1.6.5.2.143.g8cc62
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/