Messages in this thread Patch in this message | | | Date | Tue, 24 Jun 2008 15:37:15 +0200 | From | Daniel Lezcano <> | Subject | Re: [PATCH 05/11] sysfs: sysfs_chmod_file handle multiple superblocks |
| |
Tejun Heo wrote: > Hello, > > Daniel Lezcano wrote: >>> 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. Any ideas? >> Is it acceptable to queue the notifications in a list until we are in >> the loop and loop again to notify when exiting the first loop without >> error ? > > 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. > > * Implement sysfs_chmod_file() in similar way rename/move are > implemented - allocate all resources and check conditions and then iff > everything looks okay commit the operation by calling sysfs_do_setattr(). > > How does that sound?
Does this patch looks like what you are describing ?
Sauf indication contraire ci-dessus: Compagnie IBM France Siège Social : Tour Descartes, 2, avenue Gambetta, La Défense 5, 92400 Courbevoie RCS Nanterre 552 118 465 Forme Sociale : S.A.S. Capital Social : 542.737.118 ? SIREN/SIRET : 552 118 465 02430 Subject: sysfs_chmod_file can do all-or-nothing From: Daniel Lezcano <dlezcano@fr.ibm.com> Idea from: Tejun Teo <htejun@gmail.com>
"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 <dlezcano@fr.ibm.com> --- 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 | |