Re: [PATCH 05/11] sysfs: sysfs_chmod_file handle multiple superblocks

[Daniel Lezcano <dlezcano@fr.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]

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

[-- Attachment #2: separate-sysfs_setattr.patch --]
[-- Type: text/plain, Size: 5869 bytes --]

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