sysfs: sysfs_chmod_file handle multiple superblocks
Teach sysfs_chmod_file how to handle multiple sysfs
superblocks.
Signed-off-by: Eric W. Biederman <[email protected]>
Signed-off-by: Benjamin Thery <[email protected]>
---
fs/sysfs/file.c | 54 ++++++++++++++++++++++++++++++------------------------
1 file changed, 30 insertions(+), 24 deletions(-)
Index: linux-mm/fs/sysfs/file.c
===================================================================
--- linux-mm.orig/fs/sysfs/file.c
+++ linux-mm/fs/sysfs/file.c
@@ -573,7 +573,8 @@ EXPORT_SYMBOL_GPL(sysfs_add_file_to_grou
int sysfs_chmod_file(struct kobject *kobj, struct attribute *attr, mode_t mode)
{
struct sysfs_dirent *victim_sd = NULL;
- struct dentry *victim = NULL;
+ struct super_block *sb;
+ struct dentry *victim;
struct inode * inode;
struct iattr newattrs;
int rc;
@@ -584,31 +585,36 @@ int sysfs_chmod_file(struct kobject *kob
goto out;
mutex_lock(&sysfs_rename_mutex);
- victim = sysfs_get_dentry(sysfs_sb, victim_sd);
- mutex_unlock(&sysfs_rename_mutex);
- if (IS_ERR(victim)) {
- rc = PTR_ERR(victim);
- victim = NULL;
- goto out;
- }
-
- inode = victim->d_inode;
+ sysfs_grab_supers();
+ list_for_each_entry(sb, &sysfs_fs_type.fs_supers, s_instances) {
+ victim = sysfs_get_dentry(sb, victim_sd);
+ if (victim == ERR_PTR(-EXDEV))
+ continue;
+ if (IS_ERR(victim)) {
+ rc = PTR_ERR(victim);
+ victim = NULL;
+ goto out_unlock;
+ }
+
+ inode = victim->d_inode;
+ mutex_lock(&inode->i_mutex);
+ newattrs.ia_mode = (mode & S_IALLUGO) |
+ (inode->i_mode & ~S_IALLUGO);
+ newattrs.ia_valid = ATTR_MODE | ATTR_CTIME;
+ rc = notify_change(victim, &newattrs);
+ if (rc == 0) {
+ mutex_lock(&sysfs_mutex);
+ victim_sd->s_mode = newattrs.ia_mode;
+ mutex_unlock(&sysfs_mutex);
+ }
+ mutex_unlock(&inode->i_mutex);
- mutex_lock(&inode->i_mutex);
-
- newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
- newattrs.ia_valid = ATTR_MODE | ATTR_CTIME;
- rc = notify_change(victim, &newattrs);
-
- if (rc == 0) {
- mutex_lock(&sysfs_mutex);
- victim_sd->s_mode = newattrs.ia_mode;
- mutex_unlock(&sysfs_mutex);
+ dput(victim);
}
-
- mutex_unlock(&inode->i_mutex);
- out:
- dput(victim);
+out_unlock:
+ sysfs_release_supers();
+ mutex_unlock(&sysfs_rename_mutex);
+out:
sysfs_put(victim_sd);
return rc;
}
--
Hello, guys. Sorry about the long silence. Recent releases of
popular distros overwhelmed me with ata bugs. They now seem to be
under control (and hopefully stay that way for some time to come).
On the previous iteration, I was hoping I could sort out sysfs
interface problem before this patchset but given that this is a long
overdue feature, I think we should get this thing working first.
The first four patches looked good to me, so feel free to add
Acked-by: Tejun Heo <[email protected]>
Benjamin Thery Wrote:
> sysfs: sysfs_chmod_file handle multiple superblocks
>
> Teach sysfs_chmod_file how to handle multiple sysfs
> superblocks.
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?
Thanks.
--
tejun
Tejun Heo wrote:
> Hello, guys. Sorry about the long silence. Recent releases of
> popular distros overwhelmed me with ata bugs. They now seem to be
> under control (and hopefully stay that way for some time to come).
>
> On the previous iteration, I was hoping I could sort out sysfs
> interface problem before this patchset but given that this is a long
> overdue feature, I think we should get this thing working first.
>
> The first four patches looked good to me, so feel free to add
> Acked-by: Tejun Heo <[email protected]>
>
> Benjamin Thery Wrote:
>> sysfs: sysfs_chmod_file handle multiple superblocks
>>
>> Teach sysfs_chmod_file how to handle multiple sysfs
>> superblocks.
>
> 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 ?
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?
Thanks.
--
tejun
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?
Much better than my first proposition :)
I will do a separate patchset.
Thanks.
Subject: sysfs_chmod_file can do all-or-nothing
From: Daniel Lezcano <[email protected]>
Idea from: Tejun Teo <[email protected]>
"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 <[email protected]>
---
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
Daniel Lezcano wrote:
> 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 ?
Yeah, something like that. With looping for all the inodes added, it
looks like it will work fine.
Thanks.
--
tejun