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@xxxxxxxxxx>
Idea from: Tejun Teo <htejun@xxxxxxxxx>

"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@xxxxxxxxxx>
---
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

Relevant Pages

  • Re: [PATCH 05/11] sysfs: sysfs_chmod_file handle multiple superblocks
    ... instead of failing half way through but given the interface of ... the loop and loop again to notify when exiting the first loop without ... The latter never fails and the former should always ... succeed if inode_change_oksucceeds never fails ...
    (Linux-Kernel)
  • Re: [PATCH 05/11] sysfs: sysfs_chmod_file handle multiple superblocks
    ... instead of failing half way through but given the interface of ... the loop and loop again to notify when exiting the first loop without ... The latter never fails and the former should always ... succeed if inode_change_oksucceeds never fails ...
    (Linux-Kernel)
  • Re: GRACE WAS A REALITY ONCE..DONT MISS THIS ONE..!
    ... 400 words on your hope for the Obama presidency, ... aisle who have caved and who say, "Well, I hope he succeeds. ... what is unfair about my saying I hope liberalism fails? ... Liberalism is our problem. ...
    (alt.vacation.las-vegas)
  • Re: Catalyst 6500 [FWSM] [CSM]
    ... Actually, FWSM fails over when a monitored interface "fails", not when ... context, FWSM is not going to fail over; but if one of your monitored ... monitor-interface outside ... An interface "fail" means it did not pass one of the FWSM's regular ...
    (comp.dcom.sys.cisco)
  • Simply WEIRD! Is this a directx/driver bug!
    ... Actually it does not fail until the init method of the StripedTestTuner ... As you can see in the code of releaseFilters(), ... interface fails, and with this striped down code, it is always releasing the ... second interface that fails no matter the order of the 'release blocks'. ...
    (microsoft.public.win32.programmer.directx.video)