Subject: sysfs_chmod_file can do all-or-nothing From: Daniel Lezcano Idea from: Tejun Teo "I think it would be great if sysfs_chmod_file can do all-or-nothing instead of failing half way through but given the interface of notify_change(), it could be difficult to implement. Can you please take a look at the following patch? http://article.gmane.org/gmane.linux.file-systems/24484 Which replaces notify_change() call to two calls to sysfs_setattr() and fsnotify_change(). The latter never fails and the former should always succeed if inode_change_ok() succeeds (inode_setattr() never fails unless the size is changing), so I think the correct thing to do is... * Separate out sysfs_do_setattr() which doesn't do inode_change_ok() and just sets the attributes. Making it a void function which triggers WARN_ON() when inode_setattr() fails would be a good idea." Signed-off-by: Daniel Lezcano --- fs/sysfs/file.c | 23 ++++++++++++----- fs/sysfs/inode.c | 73 +++++++++++++++++++++++++++++++++---------------------- fs/sysfs/sysfs.h | 3 ++ 3 files changed, 63 insertions(+), 36 deletions(-) Index: 2.6.26-rc5-mm3/fs/sysfs/inode.c =================================================================== --- 2.6.26-rc5-mm3.orig/fs/sysfs/inode.c +++ 2.6.26-rc5-mm3/fs/sysfs/inode.c @@ -42,41 +42,29 @@ int __init sysfs_inode_init(void) return bdi_init(&sysfs_backing_dev_info); } -int sysfs_setattr(struct dentry * dentry, struct iattr * iattr) +struct iattr *sysfs_alloc_iattr(struct sysfs_dirent *sd) { - struct inode * inode = dentry->d_inode; - struct sysfs_dirent * sd = dentry->d_fsdata; struct iattr * sd_iattr; - unsigned int ia_valid = iattr->ia_valid; - int error; - if (!sd) - return -EINVAL; - - sd_iattr = sd->s_iattr; + sd_iattr = kzalloc(sizeof(struct iattr), GFP_KERNEL); + if (sd_iattr) { + sd_iattr->ia_mode = sd->s_mode; + sd_iattr->ia_uid = sd_iattr->ia_gid = 0; + sd_iattr->ia_atime = sd_iattr->ia_mtime = \ + sd_iattr->ia_ctime = CURRENT_TIME; + } + return sd_iattr; +} - error = inode_change_ok(inode, iattr); - if (error) - return error; +void sysfs_do_setattr(struct sysfs_dirent * sd, struct inode * inode, + struct iattr * iattr) +{ + unsigned int ia_valid = iattr->ia_valid; + struct iattr * sd_iattr = sd->s_iattr; iattr->ia_valid &= ~ATTR_SIZE; /* ignore size changes */ - error = inode_setattr(inode, iattr); - if (error) - return error; - - if (!sd_iattr) { - /* setting attributes for the first time, allocate now */ - sd_iattr = kzalloc(sizeof(struct iattr), GFP_KERNEL); - if (!sd_iattr) - return -ENOMEM; - /* assign default attributes */ - sd_iattr->ia_mode = sd->s_mode; - sd_iattr->ia_uid = 0; - sd_iattr->ia_gid = 0; - sd_iattr->ia_atime = sd_iattr->ia_mtime = sd_iattr->ia_ctime = CURRENT_TIME; - sd->s_iattr = sd_iattr; - } + WARN_ON(inode_setattr(inode, iattr)); /* attributes were changed atleast once in past */ @@ -100,8 +88,35 @@ int sysfs_setattr(struct dentry * dentry mode &= ~S_ISGID; sd_iattr->ia_mode = sd->s_mode = mode; } +} + +int sysfs_setattr(struct dentry * dentry, struct iattr * iattr) +{ + struct inode * inode = dentry->d_inode; + struct sysfs_dirent * sd = dentry->d_fsdata; + struct iattr * sd_iattr; + int error; + + if (!sd) + return -EINVAL; + + error = inode_change_ok(inode, iattr); + if (error) + return error; + + sd_iattr = sd->s_iattr; + + if (!sd_iattr) { + /* setting attributes for the first time, allocate now */ + sd_iattr = sysfs_alloc_iattr(sd); + if (!sd_iattr) + return -ENOMEM; + sd->s_iattr = sd_iattr; + } + + sysfs_do_setattr(sd, inode, iattr); - return error; + return 0; } static inline void set_default_inode_attr(struct inode * inode, mode_t mode) Index: 2.6.26-rc5-mm3/fs/sysfs/file.c =================================================================== --- 2.6.26-rc5-mm3.orig/fs/sysfs/file.c +++ 2.6.26-rc5-mm3/fs/sysfs/file.c @@ -577,6 +577,7 @@ int sysfs_chmod_file(struct kobject *kob struct dentry *victim = NULL; struct inode * inode; struct iattr newattrs; + struct iattr * sd_iattr; int rc; rc = -ENOENT; @@ -593,6 +594,14 @@ int sysfs_chmod_file(struct kobject *kob goto out; } + sd_iattr = victim_sd->s_iattr; + if (!sd_iattr) { + sd_iattr = sysfs_alloc_iattr(victim_sd); + if (!sd_iattr) + return -ENOMEM; + victim_sd->s_iattr = sd_iattr; + } + inode = victim->d_inode; mutex_lock(&inode->i_mutex); @@ -600,14 +609,14 @@ int sysfs_chmod_file(struct kobject *kob 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); - 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); - } + /* These two functions do not fail */ + sysfs_do_setattr(victim_sd, inode, &newattrs); + 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: Index: 2.6.26-rc5-mm3/fs/sysfs/sysfs.h =================================================================== --- 2.6.26-rc5-mm3.orig/fs/sysfs/sysfs.h +++ 2.6.26-rc5-mm3/fs/sysfs/sysfs.h @@ -143,9 +143,12 @@ static inline void sysfs_put(struct sysf * inode.c */ struct inode *sysfs_get_inode(struct sysfs_dirent *sd); +struct iattr *sysfs_alloc_iattr(struct sysfs_dirent *sd); int sysfs_setattr(struct dentry *dentry, struct iattr *iattr); int sysfs_hash_and_remove(struct sysfs_dirent *dir_sd, const char *name); int sysfs_inode_init(void); +void sysfs_do_setattr(struct sysfs_dirent * sd, struct inode * inode, + struct iattr * iattr); /* * file.c