lkml.org 
[lkml]   [2008]   [Jun]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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
\
 
 \ /
  Last update: 2008-06-24 15:41    [W:0.137 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site