Skip to contentSkip to conversation

sysfs: assorted fixes

PATCH In review Tejun Heo 3 participants 8 messages May 28, 2007 – May 29, 2007 Driver core

lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

PATCH 1/1 sysfs: assorted fixes

Hello, all.

This patchset contains two cleanups and three fixes.

#01: make-sysfs_alloc_ino-static, cleanup
#02: fix-parent-refcounting, fix
#03: reorganize-sysfs_new_inode-and-sysfs_create, cleanup and prep for #04 #04: use-iget_locked-instead-of-new_inode, fix
#05: fix-root-sysfs_dirent-root-dentry-association, fix

 fs/sysfs/dir.c   |  143 +++++++++++++++++++++++--------------------------------
 fs/sysfs/inode.c |  114 ++++++++++++++++++++++++-------------------
 fs/sysfs/mount.c |   19 ++++---
 fs/sysfs/sysfs.h |    6 +-
 4 files changed, 137 insertions(+), 145 deletions(-)

Please apply. Thanks.

--
tejun

author
#1 lore raw

PATCH 1/5 sysfs: make sysfs_alloc_ino() static

sysfs_alloc_ino() isn't used out side of fs/sysfs/dir.c. Make it static.

Signed-off-by Tejun Heo <[email protected]>
---
 fs/sysfs/dir.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
fs/sysfs/dir.c +1 -1
index bc11a26..a63d12e 100644
@@ -20,7 +20,7 @@ spinlock_t kobj_sysfs_assoc_lock = SPIN_
 static spinlock_t sysfs_ino_lock = SPIN_LOCK_UNLOCKED;
 static DEFINE_IDA(sysfs_ino_ida);
 
-int sysfs_alloc_ino(ino_t *pino)
+static int sysfs_alloc_ino(ino_t *pino)
 {
 	int ino, rc;
Signature · 1 line
1.4.3.4
author
#2 lore raw

PATCH 2/5 sysfs: fix parent refcounting during rename and move

In reply to Tejun Heo

Parent reference wasn't properly transferred during rename and move. Fix it.

Signed-off-by Tejun Heo <[email protected]>
---
 fs/sysfs/dir.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
fs/sysfs/dir.c +6 -0
index a63d12e..a26e3db 100644
@@ -497,6 +497,9 @@ int sysfs_rename_dir(struct kobject * ko
 	d_move(kobj->dentry, new_dentry);
 
 	list_del_init(&sd->s_sibling);
+	sysfs_get(parent_sd);
+	sysfs_put(sd->s_parent);
+	sd->s_parent = parent_sd;
 	list_add(&sd->s_sibling, &parent_sd->s_children);
 
 	error = 0;

@@ -550,6 +553,9 @@ again:
 
 	/* Remove from old parent's list and insert into new parent's list. */
 	list_del_init(&sd->s_sibling);
+	sysfs_get(new_parent_sd);
+	sysfs_put(sd->s_parent);
+	sd->s_parent = new_parent_sd;
 	list_add(&sd->s_sibling, &new_parent_sd->s_children);
 
 out:
Signature · 1 line
1.4.3.4
author
#3 lore raw

PATCH 3/5 sysfs: reorganize sysfs_new_indoe() and sysfs_create()

In reply to Tejun Heo

Reorganize/clean up sysfs_new_inode() and sysfs_create().

* sysfs_init_inode() is separated out from sysfs_new_inode() and is responsible for basic initialization.
* sysfs_instantiate() replaces the last step of sysfs_create() and is responsible for dentry instantitaion.
* type-specific initialization is moved out to the callers.
* mode is specified only once when creating a sysfs_dirent.
* spurious list_del_init(&sd->s_sibling) dropped from create_dir()

This change is to

* prepare for inode allocation fix.
* separate alloc and init code for synchronization update.
* make dentry/inode initialization more flexible for later changes.

This patch doesn't introduce visible behavior change.

Signed-off-by Tejun Heo <[email protected]>
---
 fs/sysfs/dir.c   |  130 ++++++++++++++++++++----------------------------------
 fs/sysfs/inode.c |  108 ++++++++++++++++++++++++---------------------
 fs/sysfs/mount.c |   18 ++++---
 fs/sysfs/sysfs.h |    6 +-
 4 files changed, 118 insertions(+), 144 deletions(-)
fs/sysfs/dir.c +48 -82
index a26e3db..bbf3525 100644
@@ -191,39 +191,18 @@ int sysfs_dirent_exist(struct sysfs_dire
 	return 0;
 }
 
-static int init_dir(struct inode * inode)
-{
-	inode->i_op = &sysfs_dir_inode_operations;
-	inode->i_fop = &sysfs_dir_operations;
-
-	/* directory inodes start off with i_nlink == 2 (for "." entry) */
-	inc_nlink(inode);
-	return 0;
-}
-
-static int init_file(struct inode * inode)
-{
-	inode->i_size = PAGE_SIZE;
-	inode->i_fop = &sysfs_file_operations;
-	return 0;
-}
-
-static int init_symlink(struct inode * inode)
-{
-	inode->i_op = &sysfs_symlink_inode_operations;
-	return 0;
-}
-
 static int create_dir(struct kobject *kobj, struct dentry *parent,
 		      const char *name, struct dentry **p_dentry)
 {
 	int error;
 	umode_t mode = S_IFDIR| S_IRWXU | S_IRUGO | S_IXUGO;
 	struct dentry *dentry;
+	struct inode *inode;
 	struct sysfs_dirent *sd;
 
 	mutex_lock(&parent->d_inode->i_mutex);
 
+	/* allocate */
 	dentry = lookup_one_len(name, parent, strlen(name));
 	if (IS_ERR(dentry)) {
 		error = PTR_ERR(dentry);

@@ -231,7 +210,7 @@ static int create_dir(struct kobject *ko
 	}
 
 	error = -EEXIST;
-	if (sysfs_dirent_exist(parent->d_fsdata, name))
+	if (dentry->d_inode)
 		goto out_dput;
 
 	error = -ENOMEM;

@@ -240,19 +219,31 @@ static int create_dir(struct kobject *ko
 		goto out_drop;
 	sd->s_elem.dir.kobj = kobj;
 
-	error = sysfs_create(sd, dentry, mode, init_dir);
-	if (error)
+	inode = sysfs_new_inode(sd);
+	if (!inode)
 		goto out_sput;
 
+	inode->i_op = &sysfs_dir_inode_operations;
+	inode->i_fop = &sysfs_dir_operations;
+	/* directory inodes start off with i_nlink == 2 (for "." entry) */
+	inc_nlink(inode);
+
+	/* link in */
+	error = -EEXIST;
+	if (sysfs_dirent_exist(parent->d_fsdata, name))
+		goto out_iput;
+
+	sysfs_instantiate(dentry, inode);
 	inc_nlink(parent->d_inode);
 	sysfs_attach_dirent(sd, parent->d_fsdata, dentry);
 
 	*p_dentry = dentry;
 	error = 0;
-	goto out_dput;
+	goto out_unlock;	/* pin directory dentry in core */
 
+ out_iput:
+	iput(inode);
  out_sput:
-	list_del_init(&sd->s_sibling);
 	sysfs_put(sd);
  out_drop:
 	d_drop(dentry);

@@ -298,71 +289,46 @@ int sysfs_create_dir(struct kobject * ko
 	return error;
 }
 
-/* attaches attribute's sysfs_dirent to the dentry corresponding to the
- * attribute file
- */
-static int sysfs_attach_attr(struct sysfs_dirent * sd, struct dentry * dentry)
-{
-	struct attribute * attr = NULL;
-	struct bin_attribute * bin_attr = NULL;
-	int (* init) (struct inode *) = NULL;
-	int error = 0;
-
-        if (sd->s_type & SYSFS_KOBJ_BIN_ATTR) {
-                bin_attr = sd->s_elem.bin_attr.bin_attr;
-                attr = &bin_attr->attr;
-        } else {
-                attr = sd->s_elem.attr.attr;
-                init = init_file;
-        }
-
-	error = sysfs_create(sd, dentry,
-			     (attr->mode & S_IALLUGO) | S_IFREG, init);
-	if (error)
-		return error;
-
-        if (bin_attr) {
-		dentry->d_inode->i_size = bin_attr->size;
-		dentry->d_inode->i_fop = &bin_fops;
-	}
-
-	sysfs_attach_dentry(sd, dentry);
-
-	return 0;
-}
-
-static int sysfs_attach_link(struct sysfs_dirent * sd, struct dentry * dentry)
-{
-	int err;
-
-	err = sysfs_create(sd, dentry, S_IFLNK|S_IRWXUGO, init_symlink);
-	if (!err)
-		sysfs_attach_dentry(sd, dentry);
-
-	return err;
-}
-
 static struct dentry * sysfs_lookup(struct inode *dir, struct dentry *dentry,
 				struct nameidata *nd)
 {
 	struct sysfs_dirent * parent_sd = dentry->d_parent->d_fsdata;
 	struct sysfs_dirent * sd;
-	int err = 0;
+	struct inode *inode;
+	int found = 0;
 
 	list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
-		if (sd->s_type & SYSFS_NOT_PINNED) {
-			if (strcmp(sd->s_name, dentry->d_name.name))
-				continue;
-
-			if (sd->s_type & SYSFS_KOBJ_LINK)
-				err = sysfs_attach_link(sd, dentry);
-			else
-				err = sysfs_attach_attr(sd, dentry);
+		if ((sd->s_type & SYSFS_NOT_PINNED) &&
+		    !strcmp(sd->s_name, dentry->d_name.name)) {
+			found = 1;
 			break;
 		}
 	}
 
-	return ERR_PTR(err);
+	/* no such entry */
+	if (!found)
+		return NULL;
+
+	/* attach dentry and inode */
+	inode = sysfs_new_inode(sd);
+	if (!inode)
+		return ERR_PTR(-ENOMEM);
+
+	/* initialize inode according to type */
+	if (sd->s_type & SYSFS_KOBJ_ATTR) {
+		inode->i_size = PAGE_SIZE;
+		inode->i_fop = &sysfs_file_operations;
+	} else if (sd->s_type & SYSFS_KOBJ_BIN_ATTR) {
+		struct bin_attribute *bin_attr = sd->s_elem.bin_attr.bin_attr;
+		inode->i_size = bin_attr->size;
+		inode->i_fop = &bin_fops;
+	} else if (sd->s_type & SYSFS_KOBJ_LINK)
+		inode->i_op = &sysfs_symlink_inode_operations;
+
+	sysfs_instantiate(dentry, inode);
+	sysfs_attach_dentry(sd, dentry);
+
+	return NULL;
 }
 
 const struct inode_operations sysfs_dir_inode_operations = {
fs/sysfs/inode.c +57 -51

119 lines not on this page · raw message

fs/sysfs/mount.c +10 -8
index c5fabfe..7f2779e 100644
@@ -27,6 +27,7 @@ static struct sysfs_dirent sysfs_root =
 	.s_sibling	= LIST_HEAD_INIT(sysfs_root.s_sibling),
 	.s_children	= LIST_HEAD_INIT(sysfs_root.s_children),
 	.s_type		= SYSFS_ROOT,
+	.s_mode		= S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO,
 	.s_ino		= 1,
 	.s_iattr	= NULL,
 };

@@ -43,18 +44,19 @@ static int sysfs_fill_super(struct super
 	sb->s_time_gran = 1;
 	sysfs_sb = sb;
 
-	inode = sysfs_new_inode(S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO,
-				 &sysfs_root);
-	if (inode) {
-		inode->i_op = &sysfs_dir_inode_operations;
-		inode->i_fop = &sysfs_dir_operations;
-		/* directory inodes start off with i_nlink == 2 (for "." entry) */
-		inc_nlink(inode);
-	} else {
+	inode = new_inode(sysfs_sb);
+	if (!inode) {
 		pr_debug("sysfs: could not get root inode\n");
 		return -ENOMEM;
 	}
 
+	sysfs_init_inode(&sysfs_root, inode);
+
+	inode->i_op = &sysfs_dir_inode_operations;
+	inode->i_fop = &sysfs_dir_operations;
+	/* directory inodes start off with i_nlink == 2 (for "." entry) */
+	inc_nlink(inode);
+
 	root = d_alloc_root(inode);
 	if (!root) {
 		pr_debug("%s: could not get root dentry!\n",__FUNCTION__);
fs/sysfs/sysfs.h +3 -3
index fc6aa86..143fdbe 100644
@@ -57,9 +57,9 @@ extern struct vfsmount * sysfs_mount;
 extern struct kmem_cache *sysfs_dir_cachep;
 
 extern void sysfs_delete_inode(struct inode *inode);
-extern struct inode * sysfs_new_inode(mode_t mode, struct sysfs_dirent *);
-extern int sysfs_create(struct sysfs_dirent *sd, struct dentry *dentry,
-			int mode, int (*init)(struct inode *));
+extern void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode);
+extern struct inode * sysfs_new_inode(struct sysfs_dirent *sd);
+extern void sysfs_instantiate(struct dentry *dentry, struct inode *inode);
 
 extern void release_sysfs_dirent(struct sysfs_dirent * sd);
 extern int sysfs_dirent_exist(struct sysfs_dirent *, const unsigned char *);
Signature · 1 line
1.4.3.4
author
#4 lore raw

PATCH 4/5 sysfs: use iget_locked() instead of new_inode()

In reply to Tejun Heo

After dentry is reclaimed, sysfs always used to allocate new dentry and inode if the file is accessed again. This causes problem with operations which only pin the inode. For example, if inotify watch is added to a sysfs file and the dentry for the file is reclaimed, the next update event creates new dentry and new inode making the inotify watch miss all the events from there on.

This patch fixes it by using iget_locked() instead of new_inode(). sysfs_new_inode() is renamed to sysfs_get_inode() and inode is initialized iff the inode is newly allocated. sysfs_instantiate() is responsible for unlocking new inodes.

Signed-off-by Tejun Heo <[email protected]>
---
 fs/sysfs/dir.c   |   37 +++++++++++++++++++++----------------
 fs/sysfs/inode.c |   24 +++++++++++++++---------
 fs/sysfs/sysfs.h |    2 +-
 3 files changed, 37 insertions(+), 26 deletions(-)
fs/sysfs/dir.c +21 -16
index bbf3525..06dff2c 100644
@@ -219,14 +219,16 @@ static int create_dir(struct kobject *ko
 		goto out_drop;
 	sd->s_elem.dir.kobj = kobj;
 
-	inode = sysfs_new_inode(sd);
+	inode = sysfs_get_inode(sd);
 	if (!inode)
 		goto out_sput;
 
-	inode->i_op = &sysfs_dir_inode_operations;
-	inode->i_fop = &sysfs_dir_operations;
-	/* directory inodes start off with i_nlink == 2 (for "." entry) */
-	inc_nlink(inode);
+	if (inode->i_state & I_NEW) {
+		inode->i_op = &sysfs_dir_inode_operations;
+		inode->i_fop = &sysfs_dir_operations;
+		/* directory inodes start off with i_nlink == 2 (for ".") */
+		inc_nlink(inode);
+	}
 
 	/* link in */
 	error = -EEXIST;

@@ -310,20 +312,23 @@ static struct dentry * sysfs_lookup(stru
 		return NULL;
 
 	/* attach dentry and inode */
-	inode = sysfs_new_inode(sd);
+	inode = sysfs_get_inode(sd);
 	if (!inode)
 		return ERR_PTR(-ENOMEM);
 
-	/* initialize inode according to type */
-	if (sd->s_type & SYSFS_KOBJ_ATTR) {
-		inode->i_size = PAGE_SIZE;
-		inode->i_fop = &sysfs_file_operations;
-	} else if (sd->s_type & SYSFS_KOBJ_BIN_ATTR) {
-		struct bin_attribute *bin_attr = sd->s_elem.bin_attr.bin_attr;
-		inode->i_size = bin_attr->size;
-		inode->i_fop = &bin_fops;
-	} else if (sd->s_type & SYSFS_KOBJ_LINK)
-		inode->i_op = &sysfs_symlink_inode_operations;
+	if (inode->i_state & I_NEW) {
+		/* initialize inode according to type */
+		if (sd->s_type & SYSFS_KOBJ_ATTR) {
+			inode->i_size = PAGE_SIZE;
+			inode->i_fop = &sysfs_file_operations;
+		} else if (sd->s_type & SYSFS_KOBJ_BIN_ATTR) {
+			struct bin_attribute *bin_attr =
+				sd->s_elem.bin_attr.bin_attr;
+			inode->i_size = bin_attr->size;
+			inode->i_fop = &bin_fops;
+		} else if (sd->s_type & SYSFS_KOBJ_LINK)
+			inode->i_op = &sysfs_symlink_inode_operations;
+	}
 
 	sysfs_instantiate(dentry, inode);
 	sysfs_attach_dentry(sd, dentry);
fs/sysfs/inode.c +15 -9
index 3b2dc2a..167c7fb 100644
@@ -152,10 +152,12 @@ void sysfs_init_inode(struct sysfs_diren
 }
 
 /**
- *	sysfs_new_inode - allocate new inode for sysfs_dirent
+ *	sysfs_get_inode - get inode for sysfs_dirent
  *	@sd: sysfs_dirent to allocate inode for
  *
- *	Allocate inode for @sd and initialize basics.
+ *	Get inode for @sd.  If such inode doesn't exist, a new inode
+ *	is allocated and basics are initialized.  New inode is
+ *	returned locked.
  *
  *	LOCKING:
  *	Kernel thread context (may sleep).

@@ -163,12 +165,12 @@ void sysfs_init_inode(struct sysfs_diren
  *	RETURNS:
  *	Pointer to allocated inode on success, NULL on failure.
  */
-struct inode * sysfs_new_inode(struct sysfs_dirent *sd)
+struct inode * sysfs_get_inode(struct sysfs_dirent *sd)
 {
 	struct inode *inode;
 
-	inode = new_inode(sysfs_sb);
-	if (inode)
+	inode = iget_locked(sysfs_sb, sd->s_ino);
+	if (inode && (inode->i_state & I_NEW))
 		sysfs_init_inode(sd, inode);
 
 	return inode;

@@ -179,7 +181,7 @@ struct inode * sysfs_new_inode(struct sy
  *	@dentry: dentry to be instantiated
  *	@inode: inode associated with @sd
  *
- *	Instantiate @dentry with @inode.
+ *	Unlock @inode if locked and instantiate @dentry with @inode.
  *
  *	LOCKING:
  *	None.

@@ -188,9 +190,13 @@ void sysfs_instantiate(struct dentry *de
 {
 	BUG_ON(!dentry || dentry->d_inode);
 
-	if (dentry->d_parent && dentry->d_parent->d_inode) {
-		struct inode *p_inode = dentry->d_parent->d_inode;
-		p_inode->i_mtime = p_inode->i_ctime = CURRENT_TIME;
+	if (inode->i_state & I_NEW) {
+		unlock_new_inode(inode);
+
+		if (dentry->d_parent && dentry->d_parent->d_inode) {
+			struct inode *p_inode = dentry->d_parent->d_inode;
+			p_inode->i_mtime = p_inode->i_ctime = CURRENT_TIME;
+		}
 	}
 
 	d_instantiate(dentry, inode);
fs/sysfs/sysfs.h +1 -1
index 143fdbe..627bf39 100644
@@ -58,7 +58,7 @@ extern struct kmem_cache *sysfs_dir_cach
 
 extern void sysfs_delete_inode(struct inode *inode);
 extern void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode);
-extern struct inode * sysfs_new_inode(struct sysfs_dirent *sd);
+extern struct inode * sysfs_get_inode(struct sysfs_dirent *sd);
 extern void sysfs_instantiate(struct dentry *dentry, struct inode *inode);
 
 extern void release_sysfs_dirent(struct sysfs_dirent * sd);
Signature · 1 line
1.4.3.4
author
#5 lore raw

PATCH 5/5 sysfs: fix root sysfs_dirent -> root dentry association

In reply to Tejun Heo

The root sysfs_dirent didn't point to the root dentry fix it.

Signed-off-by Tejun Heo <[email protected]>
---
 fs/sysfs/mount.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
fs/sysfs/mount.c +1 -0
index 7f2779e..2d6294b 100644
@@ -63,6 +63,7 @@ static int sysfs_fill_super(struct super
 		iput(inode);
 		return -ENOMEM;
 	}
+	sysfs_root.s_dentry = root;
 	root->d_fsdata = &sysfs_root;
 	sb->s_root = root;
 	return 0;
Signature · 1 line
1.4.3.4
#6 lore raw
In reply to Tejun Heo
Quoted 10 lines · Tejun Heo

Hello, all.

This patchset contains two cleanups and three fixes.

#01: make-sysfs_alloc_ino-static, cleanup
#02: fix-parent-refcounting, fix
#03: reorganize-sysfs_new_inode-and-sysfs_create, cleanup and prep for #04 #04: use-iget_locked-instead-of-new_inode, fix
#05: fix-root-sysfs_dirent-root-dentry-association, fix

Quoted 7 lines
 fs/sysfs/dir.c   |  143 +++++++++++++++++++++++--------------------------------
 fs/sysfs/inode.c |  114 ++++++++++++++++++++++++-------------------
 fs/sysfs/mount.c |   19 ++++---
 fs/sysfs/sysfs.h |    6 +-
 4 files changed, 137 insertions(+), 145 deletions(-)

Please apply. Thanks.

I've tested this patchset a bit on s390 (device attach/detach and moving devices around) and didn't encounter problems. As far as I can see, this looks sane.

#7 lore raw
Quoted 19 lines · Cornelia Huck
Quoted 16 lines · Tejun Heo

Hello, all.

This patchset contains two cleanups and three fixes.

#01: make-sysfs_alloc_ino-static, cleanup
#02: fix-parent-refcounting, fix
#03: reorganize-sysfs_new_inode-and-sysfs_create, cleanup and prep for #04 #04: use-iget_locked-instead-of-new_inode, fix
#05: fix-root-sysfs_dirent-root-dentry-association, fix

 fs/sysfs/dir.c   |  143 +++++++++++++++++++++++--------------------------------
 fs/sysfs/inode.c |  114 ++++++++++++++++++++++++-------------------
 fs/sysfs/mount.c |   19 ++++---
 fs/sysfs/sysfs.h |    6 +-
 4 files changed, 137 insertions(+), 145 deletions(-)
Quoted 5 lines
Quoted 1 line

Please apply. Thanks.

I've tested this patchset a bit on s390 (device attach/detach and moving devices around) and didn't encounter problems. As far as I can see, this looks sane.

It boots on this laptop just fine too :)

Thanks for testing,

greg k-h