sysfs: locking fix and cleanups

PATCH In review Tejun Heo 5 participants 20 messages Jul 31, 2007 – Aug 2, 2007 Driver core

lore

Share & tools

lore

Tejun Heo

author

2007-07-31 10:15 UTC #0 lore raw

PATCH 1/1 sysfs: locking fix and cleanups

Hello, all.

This patchset contains a locking fix and cleanup patches for sysfs.

#01 locking fix
#02 shadow support removal from Eric adapted to apply after #01
#03-07 clean up patches

#01 should go mainline as it fixes a locking regression introduced by -rc1 merge. The rest should stay in -gregkh till 24 merge window opens up.

Clean up patches don't introduce any noticeable behavior change. They just refactor and simplify codes.

Thanks.

--
tejun

Tejun Heo

author

2007-07-31 10:16 UTC #1 lore raw

PATCH 1/7 sysfs: fix locking in sysfs_lookup() and sysfs_rename_dir()

sd children list walking in sysfs_lookup() and sd renaming in sysfs_rename_dir() were left out during i_mutex -> sysfs_mutex conversion. Fix them.

Signed-off-by Tejun Heo <[email protected]>

---
 fs/sysfs/dir.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

fs/sysfs/dir.c +12 -9

index 048e605..83e76b3 100644
@@ -762,12 +762,15 @@ static int sysfs_count_nlink(struct sysfs_dirent *sd)
 static struct dentry * sysfs_lookup(struct inode *dir, struct dentry *dentry,
 				struct nameidata *nd)
 {
+	struct dentry *ret = NULL;
 	struct sysfs_dirent * parent_sd = dentry->d_parent->d_fsdata;
 	struct sysfs_dirent * sd;
 	struct bin_attribute *bin_attr;
 	struct inode *inode;
 	int found = 0;
 
+	mutex_lock(&sysfs_mutex);
+
 	for (sd = parent_sd->s_children; sd; sd = sd->s_sibling) {
 		if (sysfs_type(sd) &&
 		    !strcmp(sd->s_name, dentry->d_name.name)) {

@@ -778,14 +781,14 @@ static struct dentry * sysfs_lookup(struct inode *dir, struct dentry *dentry,
 
 	/* no such entry */
 	if (!found)
-		return NULL;
+		goto out_unlock;
 
 	/* attach dentry and inode */
 	inode = sysfs_get_inode(sd);
-	if (!inode)
-		return ERR_PTR(-ENOMEM);
-
-	mutex_lock(&sysfs_mutex);
+	if (!inode) {
+		ret = ERR_PTR(-ENOMEM);
+		goto out_unlock;
+	}
 
 	if (inode->i_state & I_NEW) {
 		/* initialize inode according to type */

@@ -815,9 +818,9 @@ static struct dentry * sysfs_lookup(struct inode *dir, struct dentry *dentry,
 	sysfs_instantiate(dentry, inode);
 	sysfs_attach_dentry(sd, dentry);
 
+ out_unlock:
 	mutex_unlock(&sysfs_mutex);
-
-	return NULL;
+	return ret;
 }
 
 const struct inode_operations sysfs_dir_inode_operations = {

@@ -942,6 +945,8 @@ int sysfs_rename_dir(struct kobject *kobj, struct sysfs_dirent *new_parent_sd,
 	if (error)
 		goto out_drop;
 
+	mutex_lock(&sysfs_mutex);
+
 	dup_name = sd->s_name;
 	sd->s_name = new_name;

@@ -949,8 +954,6 @@ int sysfs_rename_dir(struct kobject *kobj, struct sysfs_dirent *new_parent_sd,
 	d_add(new_dentry, NULL);
 	d_move(sd->s_dentry, new_dentry);
 
-	mutex_lock(&sysfs_mutex);
-
 	sysfs_unlink_sibling(sd);
 	sysfs_get(new_parent_sd);
 	sysfs_put(sd->s_parent);

Signature · 1 line

1.5.0.3

Cornelia Huck

Acked-by on v1 1/7

2007-07-31 12:56 UTC #2 lore raw

Re: [PATCH 1/7] sysfs: fix locking in sysfs_lookup() and sysfs_rename_dir()

Quoted 8 lines · Tejun Heo

sd children list walking in sysfs_lookup() and sd renaming in sysfs_rename_dir() were left out during i_mutex -> sysfs_mutex conversion. Fix them.
Signed-off-by Tejun Heo <[email protected]>
---
 fs/sysfs/dir.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

Looks sane.

Acked-by Cornelia Huck <[email protected]>

Greg KH

2007-08-02 00:26 UTC #3 lore raw

Re: [PATCH 1/7] sysfs: fix locking in sysfs_lookup() and sysfs_rename_dir()

In reply to Tejun Heo

Quoted 5 lines · Tejun Heo

sd children list walking in sysfs_lookup() and sd renaming in sysfs_rename_dir() were left out during i_mutex -> sysfs_mutex conversion. Fix them.
Signed-off-by Tejun Heo <[email protected]>

Ok, to apply this, it messes with Eric's further patches for the shadow directory stuff. But since it looks like you and Eric seem to have worked something else in that area, I'll drop Eric's patches for now, as that's the safest thing.

Eric, is that ok?

thanks,

greg k-h

Greg KH

2007-08-02 01:08 UTC #4 lore raw

Re: [PATCH 1/7] sysfs: fix locking in sysfs_lookup() and sysfs_rename_dir()

Quoted 5 lines · Greg KH

Quoted 4 lines · Tejun Heo
sd children list walking in sysfs_lookup() and sd renaming in sysfs_rename_dir() were left out during i_mutex -> sysfs_mutex conversion. Fix them.

Quoted 8 lines

Quoted 1 line
Signed-off-by Tejun Heo <[email protected]>
Ok, to apply this, it messes with Eric's further patches for the shadow directory stuff. But since it looks like you and Eric seem to have worked something else in that area, I'll drop Eric's patches for now, as that's the safest thing.
Eric, is that ok?

Oh nevermind, it looks like you already took care of this with the later patches...

thanks,

greg k-h

Eric W. Biederman

2007-08-02 01:11 UTC #5 lore raw

Re: [PATCH 1/7] sysfs: fix locking in sysfs_lookup() and sysfs_rename_dir()

In reply to Greg KH

Quoted 5 lines · Greg KH

Quoted 4 lines · Tejun Heo
sd children list walking in sysfs_lookup() and sd renaming in sysfs_rename_dir() were left out during i_mutex -> sysfs_mutex conversion. Fix them.

Quoted 8 lines

Quoted 1 line
Signed-off-by Tejun Heo <[email protected]>
Ok, to apply this, it messes with Eric's further patches for the shadow directory stuff. But since it looks like you and Eric seem to have worked something else in that area, I'll drop Eric's patches for now, as that's the safest thing.
Eric, is that ok?

Sounds good. With a little luck I should have something working in a couple of hours and then I can see about getting a tree with both my patches and Tejun. So I will probably rebase on top of Tejun's latest patches.

Eric

Tejun Heo

author

2007-07-31 10:15 UTC #6 lore raw

PATCH 3/7 sysfs: cosmetic changes in sysfs_lookup()

In reply to Tejun Heo

* remove space between * and symbol name in variable declaration.

* kill unnecessary new line.

* kill 'found' and test 'sd' instead.

Signed-off-by Tejun Heo <[email protected]>

---
 fs/sysfs/dir.c |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

fs/sysfs/dir.c +5 ~~-10~~

index 1c3a0dd..026ea70 100644
@@ -756,24 +756,19 @@ static struct dentry * sysfs_lookup(struct inode *dir, struct dentry *dentry,
 				struct nameidata *nd)
 {
 	struct dentry *ret = NULL;
-	struct sysfs_dirent * parent_sd = dentry->d_parent->d_fsdata;
-	struct sysfs_dirent * sd;
+	struct sysfs_dirent *parent_sd = dentry->d_parent->d_fsdata;
+	struct sysfs_dirent *sd;
 	struct bin_attribute *bin_attr;
 	struct inode *inode;
-	int found = 0;
 
 	mutex_lock(&sysfs_mutex);
 
-	for (sd = parent_sd->s_children; sd; sd = sd->s_sibling) {
-		if (sysfs_type(sd) &&
-		    !strcmp(sd->s_name, dentry->d_name.name)) {
-			found = 1;
+	for (sd = parent_sd->s_children; sd; sd = sd->s_sibling)
+		if (sysfs_type(sd) && !strcmp(sd->s_name, dentry->d_name.name))
 			break;
-		}
-	}
 
 	/* no such entry */
-	if (!found)
+	if (!sd)
 		goto out_unlock;
 
 	/* attach dentry and inode */

Signature · 1 line

1.5.0.3

Cornelia Huck

Acked-by on v1 3/7

2007-07-31 12:57 UTC #7 lore raw

Re: [PATCH 3/7] sysfs: cosmetic changes in sysfs_lookup()

Quoted 11 lines · Tejun Heo

* remove space between * and symbol name in variable declaration.
* kill unnecessary new line.
* kill 'found' and test 'sd' instead.
Signed-off-by Tejun Heo <[email protected]>
---
 fs/sysfs/dir.c |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

Acked-by Cornelia Huck <[email protected]>

Tejun Heo

author

2007-07-31 10:16 UTC #8 lore raw

PATCH 4/7 sysfs: simplify sysfs_remove_dir()

In reply to Tejun Heo

With the shadow directories gone, sysfs_remove_dir() can be simplified.

* parent doesn't need to be grabbed separately. Just access old_dentry->d_parent.

* parent sd can never change. Remove code to move under the new parent.

* Massage comments a bit.

Signed-off-by Tejun Heo <[email protected]>

---
 fs/sysfs/dir.c |   26 ++++----------------------
 1 files changed, 4 insertions(+), 22 deletions(-)

fs/sysfs/dir.c +4 ~~-22~~

index 026ea70..0fe6aa3 100644
@@ -883,14 +883,10 @@ int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
 	struct sysfs_dirent *sd;
 	struct dentry *parent = NULL;
 	struct dentry *old_dentry = NULL, *new_dentry = NULL;
-	struct sysfs_dirent *parent_sd;
 	const char *dup_name = NULL;
 	int error;
 
-	if (!kobj->parent)
-		return -EINVAL;
-
-	/* get dentries */
+	/* get the original dentry */
 	sd = kobj->sd;
 	old_dentry = sysfs_get_dentry(sd);
 	if (IS_ERR(old_dentry)) {

@@ -898,12 +894,7 @@ int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
 		goto out_dput;
 	}
 
-	parent_sd = kobj->parent->sd;
-	parent = sysfs_get_dentry(parent_sd);
-	if (IS_ERR(parent)) {
-		error = PTR_ERR(parent);
-		goto out_dput;
-	}
+	parent = old_dentry->d_parent;
 
 	/* lock parent and get dentry for new name */
 	mutex_lock(&parent->d_inode->i_mutex);

@@ -933,22 +924,14 @@ int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
 		goto out_drop;
 
 	mutex_lock(&sysfs_mutex);
-
 	dup_name = sd->s_name;
 	sd->s_name = new_name;
+	mutex_unlock(&sysfs_mutex);
 
-	/* move under the new parent */
+	/* rename */
 	d_add(new_dentry, NULL);
 	d_move(sd->s_dentry, new_dentry);
 
-	sysfs_unlink_sibling(sd);
-	sysfs_get(parent_sd);
-	sysfs_put(sd->s_parent);
-	sd->s_parent = parent_sd;
-	sysfs_link_sibling(sd);
-
-	mutex_unlock(&sysfs_mutex);
-
 	error = 0;
 	goto out_unlock;

@@ -958,7 +941,6 @@ int sysfs_rename_dir(struct kobject * kobj, const char *new_name)
 	mutex_unlock(&parent->d_inode->i_mutex);
  out_dput:
 	kfree(dup_name);
-	dput(parent);
 	dput(old_dentry);
 	dput(new_dentry);
 	return error;

Signature · 1 line

1.5.0.3

Cornelia Huck

Acked-by on v1 4/7

2007-07-31 12:59 UTC #9 lore raw

Re: [PATCH 4/7] sysfs: simplify sysfs_remove_dir()

Quoted 8 lines · Tejun Heo

With the shadow directories gone, sysfs_remove_dir() can be simplified.
* parent doesn't need to be grabbed separately. Just access old_dentry->d_parent.
* parent sd can never change. Remove code to move under the new parent.

Quoted 7 lines

* Massage comments a bit.
Signed-off-by Tejun Heo <[email protected]>
---
 fs/sysfs/dir.c |   26 ++++----------------------
 1 files changed, 4 insertions(+), 22 deletions(-)

It's not sysfs_remove_dir(), but sysfs_rename_dir() :) Otherwise this looks good.

Acked-by Cornelia Huck <[email protected]>

Tejun Heo

author

2007-07-31 13:22 UTC #10 lore raw

Re: [PATCH 4/7] sysfs: simplify sysfs_remove_dir()

Quoted 13 lines · Cornelia Huck

Quoted 10 lines · Tejun Heo
With the shadow directories gone, sysfs_remove_dir() can be simplified.
* parent doesn't need to be grabbed separately. Just access old_dentry->d_parent.
* parent sd can never change. Remove code to move under the new parent.
* Massage comments a bit.

Quoted 7 lines

Quoted 5 lines
Signed-off-by Tejun Heo <[email protected]>
---
 fs/sysfs/dir.c |   26 ++++----------------------
 1 files changed, 4 insertions(+), 22 deletions(-)
It's not sysfs_remove_dir(), but sysfs_rename_dir() :) Otherwise this looks good.

Yeah, what was I thinking. :-)

Signature · 1 line

tejun

Tejun Heo

author

2007-07-31 10:17 UTC #11 lore raw

PATCH 5/7 sysfs: make sysfs_add/remove_one() call link/unlink_sibling() implictly

In reply to Tejun Heo

When adding or removing a sysfs_dirent, the user used to be required to call link/unlink separately. It was for two reasons - code looked like that before sysfs_addrm_cxt conversion and to avoid looping through parent_sd->children list twice during removal.

Performance optimization during removal just isn't worth it. Make sysfs_add/remove_one() call sysfs_link/unlink_sibing() implicitly. This makes code simpler albeit slightly less efficient. This change doesn't introduce any noticeable behavior change.

Signed-off-by Tejun Heo <[email protected]>

---
 fs/sysfs/dir.c     |   21 ++++++++++-----------
 fs/sysfs/file.c    |    4 +---
 fs/sysfs/inode.c   |   17 ++++-------------
 fs/sysfs/symlink.c |    4 +---
 fs/sysfs/sysfs.h   |    2 --
 5 files changed, 16 insertions(+), 32 deletions(-)

fs/sysfs/dir.c +10 ~~-11~~

index 0fe6aa3..8ce3ffb 100644
@@ -30,7 +30,7 @@ static DEFINE_IDA(sysfs_ino_ida);
  *	Locking:
  *	mutex_lock(sysfs_mutex)
  */
-void sysfs_link_sibling(struct sysfs_dirent *sd)
+static void sysfs_link_sibling(struct sysfs_dirent *sd)
 {
 	struct sysfs_dirent *parent_sd = sd->s_parent;

@@ -49,7 +49,7 @@ void sysfs_link_sibling(struct sysfs_dirent *sd)
  *	Locking:
  *	mutex_lock(sysfs_mutex)
  */
-void sysfs_unlink_sibling(struct sysfs_dirent *sd)
+static void sysfs_unlink_sibling(struct sysfs_dirent *sd)
 {
 	struct sysfs_dirent **pos;

@@ -500,6 +500,8 @@ void sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 		inc_nlink(acxt->parent_inode);
 
 	acxt->cnt++;
+
+	sysfs_link_sibling(sd);
 }
 
 /**

@@ -521,7 +523,9 @@ void sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
  */
 void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 {
-	BUG_ON(sd->s_sibling || (sd->s_flags & SYSFS_FLAG_REMOVED));
+	BUG_ON(sd->s_flags & SYSFS_FLAG_REMOVED);
+
+	sysfs_unlink_sibling(sd);
 
 	sd->s_flags |= SYSFS_FLAG_REMOVED;
 	sd->s_sibling = acxt->removed;

@@ -697,10 +701,8 @@ static int create_dir(struct kobject *kobj, struct sysfs_dirent *parent_sd,
 	/* link in */
 	sysfs_addrm_start(&acxt, parent_sd);
 
-	if (!sysfs_find_dirent(parent_sd, name)) {
+	if (!sysfs_find_dirent(parent_sd, name))
 		sysfs_add_one(&acxt, sd);
-		sysfs_link_sibling(sd);
-	}
 
 	if (!sysfs_addrm_finish(&acxt)) {
 		sysfs_put(sd);

@@ -821,7 +823,6 @@ static void remove_dir(struct sysfs_dirent *sd)
 	struct sysfs_addrm_cxt acxt;
 
 	sysfs_addrm_start(&acxt, sd->s_parent);
-	sysfs_unlink_sibling(sd);
 	sysfs_remove_one(&acxt, sd);
 	sysfs_addrm_finish(&acxt);
 }

@@ -846,11 +847,9 @@ static void __sysfs_remove_dir(struct sysfs_dirent *dir_sd)
 	while (*pos) {
 		struct sysfs_dirent *sd = *pos;
 
-		if (sysfs_type(sd) && sysfs_type(sd) != SYSFS_DIR) {
-			*pos = sd->s_sibling;
-			sd->s_sibling = NULL;
+		if (sysfs_type(sd) && sysfs_type(sd) != SYSFS_DIR)
 			sysfs_remove_one(&acxt, sd);
-		} else
+		else
 			pos = &(*pos)->s_sibling;
 	}
 	sysfs_addrm_finish(&acxt);

fs/sysfs/file.c +1 -3

index 3e1cc06..fcd065e 100644
@@ -405,10 +405,8 @@ int sysfs_add_file(struct sysfs_dirent *dir_sd, const struct attribute *attr,
 
 	sysfs_addrm_start(&acxt, dir_sd);
 
-	if (!sysfs_find_dirent(dir_sd, attr->name)) {
+	if (!sysfs_find_dirent(dir_sd, attr->name))
 		sysfs_add_one(&acxt, sd);
-		sysfs_link_sibling(sd);
-	}
 
 	if (!sysfs_addrm_finish(&acxt)) {
 		sysfs_put(sd);

fs/sysfs/inode.c +4 ~~-13~~

index 9671164..f05cda9 100644
@@ -190,25 +190,16 @@ void sysfs_instantiate(struct dentry *dentry, struct inode *inode)
 int sysfs_hash_and_remove(struct sysfs_dirent *dir_sd, const char *name)
 {
 	struct sysfs_addrm_cxt acxt;
-	struct sysfs_dirent **pos, *sd;
+	struct sysfs_dirent *sd;
 
 	if (!dir_sd)
 		return -ENOENT;
 
 	sysfs_addrm_start(&acxt, dir_sd);
 
-	for (pos = &dir_sd->s_children; *pos; pos = &(*pos)->s_sibling) {
-		sd = *pos;
-
-		if (!sysfs_type(sd))
-			continue;
-		if (!strcmp(sd->s_name, name)) {
-			*pos = sd->s_sibling;
-			sd->s_sibling = NULL;
-			sysfs_remove_one(&acxt, sd);
-			break;
-		}
-	}
+	sd = sysfs_find_dirent(dir_sd, name);
+	if (sd)
+		sysfs_remove_one(&acxt, sd);
 
 	if (sysfs_addrm_finish(&acxt))
 		return 0;

fs/sysfs/symlink.c +1 -3

index 4ce687f..8f70ca7 100644
@@ -92,10 +92,8 @@ int sysfs_create_link(struct kobject * kobj, struct kobject * target, const char
 
 	sysfs_addrm_start(&acxt, parent_sd);
 
-	if (!sysfs_find_dirent(parent_sd, name)) {
+	if (!sysfs_find_dirent(parent_sd, name))
 		sysfs_add_one(&acxt, sd);
-		sysfs_link_sibling(sd);
-	}
 
 	if (!sysfs_addrm_finish(&acxt)) {
 		error = -EEXIST;

fs/sysfs/sysfs.h +0 -2

index b55e510..32b8b64 100644
@@ -56,8 +56,6 @@ extern struct sysfs_dirent sysfs_root;
 extern struct kmem_cache *sysfs_dir_cachep;
 
 extern struct dentry *sysfs_get_dentry(struct sysfs_dirent *sd);
-extern void sysfs_link_sibling(struct sysfs_dirent *sd);
-extern void sysfs_unlink_sibling(struct sysfs_dirent *sd);
 extern struct sysfs_dirent *sysfs_get_active(struct sysfs_dirent *sd);
 extern void sysfs_put_active(struct sysfs_dirent *sd);
 extern struct sysfs_dirent *sysfs_get_active_two(struct sysfs_dirent *sd);

Signature · 1 line

1.5.0.3

Cornelia Huck

Acked-by on v1 5/7

2007-07-31 13:14 UTC #12 lore raw

Re: [PATCH 5/7] sysfs: make sysfs_add/remove_one() call link/unlink_sibling() implictly

Quoted 10 lines · Tejun Heo

When adding or removing a sysfs_dirent, the user used to be required to call link/unlink separately. It was for two reasons - code looked like that before sysfs_addrm_cxt conversion and to avoid looping through parent_sd->children list twice during removal.
Performance optimization during removal just isn't worth it. Make sysfs_add/remove_one() call sysfs_link/unlink_sibing() implicitly. This makes code simpler albeit slightly less efficient. This change doesn't introduce any noticeable behavior change.

Quoted 9 lines

Signed-off-by Tejun Heo <[email protected]>

---
 fs/sysfs/dir.c     |   21 ++++++++++-----------
 fs/sysfs/file.c    |    4 +---
 fs/sysfs/inode.c   |   17 ++++-------------
 fs/sysfs/symlink.c |    4 +---
 fs/sysfs/sysfs.h   |    2 --
 5 files changed, 16 insertions(+), 32 deletions(-)

I agree. The double loop in the removal path doesn't really hurt, and the code is now much nicer.

Acked-by Cornelia Huck <[email protected]>

Tejun Heo

author

2007-07-31 10:17 UTC #13 lore raw

PATCH 6/7 sysfs: make sysfs_add_one() automatically check for duplicate entry

In reply to Tejun Heo

Make sysfs_add_one() check for duplicate entry and return -EEXIST if such entry exists. This simplifies node addition code a bit.

This patch doesn't introduce any noticeable behavior change.

Signed-off-by Tejun Heo <[email protected]>

---
 fs/sysfs/dir.c     |   25 ++++++++++++++++---------
 fs/sysfs/file.c    |   12 +++++-------
 fs/sysfs/symlink.c |    9 +++------
 fs/sysfs/sysfs.h   |    2 +-
 4 files changed, 25 insertions(+), 23 deletions(-)

fs/sysfs/dir.c +16 -9

index 8ce3ffb..69e57be 100644
@@ -491,9 +491,15 @@ void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,
  *
  *	LOCKING:
  *	Determined by sysfs_addrm_start().
+ *
+ *	RETURNS:
+ *	0 on success, -errno on failure.
  */
-void sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
+int sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 {
+	if (sysfs_find_dirent(acxt->parent_sd, sd->s_name))
+		return -EEXIST;
+
 	sd->s_parent = sysfs_get(acxt->parent_sd);
 
 	if (sysfs_type(sd) == SYSFS_DIR && acxt->parent_inode)

@@ -502,6 +508,8 @@ void sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
 	acxt->cnt++;
 
 	sysfs_link_sibling(sd);
+
+	return 0;
 }
 
 /**

@@ -691,6 +699,7 @@ static int create_dir(struct kobject *kobj, struct sysfs_dirent *parent_sd,
 	umode_t mode = S_IFDIR| S_IRWXU | S_IRUGO | S_IXUGO;
 	struct sysfs_addrm_cxt acxt;
 	struct sysfs_dirent *sd;
+	int rc;
 
 	/* allocate */
 	sd = sysfs_new_dirent(name, mode, SYSFS_DIR);

@@ -700,17 +709,15 @@ static int create_dir(struct kobject *kobj, struct sysfs_dirent *parent_sd,
 
 	/* link in */
 	sysfs_addrm_start(&acxt, parent_sd);
+	rc = sysfs_add_one(&acxt, sd);
+	sysfs_addrm_finish(&acxt);
 
-	if (!sysfs_find_dirent(parent_sd, name))
-		sysfs_add_one(&acxt, sd);
-
-	if (!sysfs_addrm_finish(&acxt)) {
+	if (rc == 0)
+		*p_sd = sd;
+	else
 		sysfs_put(sd);
-		return -EEXIST;
-	}
 
-	*p_sd = sd;
-	return 0;
+	return rc;
 }
 
 int sysfs_create_subdir(struct kobject *kobj, const char *name,

fs/sysfs/file.c +5 -7

index fcd065e..416351a 100644
@@ -397,6 +397,7 @@ int sysfs_add_file(struct sysfs_dirent *dir_sd, const struct attribute *attr,
 	umode_t mode = (attr->mode & S_IALLUGO) | S_IFREG;
 	struct sysfs_addrm_cxt acxt;
 	struct sysfs_dirent *sd;
+	int rc;
 
 	sd = sysfs_new_dirent(attr->name, mode, type);
 	if (!sd)

@@ -404,16 +405,13 @@ int sysfs_add_file(struct sysfs_dirent *dir_sd, const struct attribute *attr,
 	sd->s_elem.attr.attr = (void *)attr;
 
 	sysfs_addrm_start(&acxt, dir_sd);
+	rc = sysfs_add_one(&acxt, sd);
+	sysfs_addrm_finish(&acxt);
 
-	if (!sysfs_find_dirent(dir_sd, attr->name))
-		sysfs_add_one(&acxt, sd);
-
-	if (!sysfs_addrm_finish(&acxt)) {
+	if (rc)
 		sysfs_put(sd);
-		return -EEXIST;
-	}
 
-	return 0;
+	return rc;
 }

fs/sysfs/symlink.c +3 -6

index 8f70ca7..f77ad61 100644
@@ -91,14 +91,11 @@ int sysfs_create_link(struct kobject * kobj, struct kobject * target, const char
 	target_sd = NULL;	/* reference is now owned by the symlink */
 
 	sysfs_addrm_start(&acxt, parent_sd);
+	error = sysfs_add_one(&acxt, sd);
+	sysfs_addrm_finish(&acxt);
 
-	if (!sysfs_find_dirent(parent_sd, name))
-		sysfs_add_one(&acxt, sd);
-
-	if (!sysfs_addrm_finish(&acxt)) {
-		error = -EEXIST;
+	if (error)
 		goto out_put;
-	}
 
 	return 0;

fs/sysfs/sysfs.h +1 -1

index 32b8b64..bb3f0c9 100644
@@ -62,7 +62,7 @@ extern struct sysfs_dirent *sysfs_get_active_two(struct sysfs_dirent *sd);
 extern void sysfs_put_active_two(struct sysfs_dirent *sd);
 extern void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,
 			      struct sysfs_dirent *parent_sd);
-extern void sysfs_add_one(struct sysfs_addrm_cxt *acxt,
+extern int sysfs_add_one(struct sysfs_addrm_cxt *acxt,
 			  struct sysfs_dirent *sd);
 extern void sysfs_remove_one(struct sysfs_addrm_cxt *acxt,
 			     struct sysfs_dirent *sd);

Signature · 1 line

1.5.0.3

Cornelia Huck

Acked-by on v1 6/7

2007-07-31 13:26 UTC #14 lore raw

Re: [PATCH 6/7] sysfs: make sysfs_add_one() automatically check for duplicate entry

Quoted 18 lines · Tejun Heo

Make sysfs_add_one() check for duplicate entry and return -EEXIST if such entry exists. This simplifies node addition code a bit.
This patch doesn't introduce any noticeable behavior change.
Signed-off-by Tejun Heo <[email protected]>
---
 fs/sysfs/dir.c     |   25 ++++++++++++++++---------
 fs/sysfs/file.c    |   12 +++++-------
 fs/sysfs/symlink.c |    9 +++------
 fs/sysfs/sysfs.h   |    2 +-
 4 files changed, 25 insertions(+), 23 deletions(-)
fs/sysfs/dir.c +0 -0
index 8ce3ffb..69e57be 100644

fs/sysfs/dir.c void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt, · line 496

Quoted 6 lines

fs/sysfs/dir.c +3 -0

@@ -491,9 +491,15 @@ void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,
  *
  *	LOCKING:
  *	Determined by sysfs_addrm_start().
+ *
+ *	RETURNS:
+ *	0 on success, -errno on failure.

The only possible failure is -EEXIST, so I'd state that explicitly, especially since you rely on that fact below.

Otherwise

Acked-by Cornelia Huck <[email protected]>

Tejun Heo

author

2007-07-31 10:17 UTC #15 lore raw

PATCH 7/7 sysfs: make sysfs_addrm_finish() return void

In reply to Tejun Heo

With the previous sysfs_add_one() update, there is only one user of the return value of sysfs_addrm_finish() and the user can switch to testing @sd easily. Make sysfs_addrm_finish() return void for cleaner semantics as suggested by Satyam Sharma.

This patch doesn't introduce any noticeable behavior change.

Signed-off-by Tejun Heo <[email protected]>

Cc Satyam Sharma <[email protected]>

---
 fs/sysfs/dir.c   |    7 +------
 fs/sysfs/inode.c |    7 +++++--
 fs/sysfs/sysfs.h |    2 +-
 3 files changed, 7 insertions(+), 9 deletions(-)

fs/sysfs/dir.c +1 -6

index 69e57be..f9523c8 100644
@@ -608,11 +608,8 @@ static void sysfs_drop_dentry(struct sysfs_dirent *sd)
  *
  *	LOCKING:
  *	All mutexes acquired by sysfs_addrm_start() are released.
- *
- *	RETURNS:
- *	Number of added/removed sysfs_dirents since sysfs_addrm_start().
  */
-int sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt)
+void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt)
 {
 	/* release resources acquired by sysfs_addrm_start() */
 	mutex_unlock(&sysfs_mutex);

@@ -638,8 +635,6 @@ int sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt)
 		sysfs_deactivate(sd);
 		sysfs_put(sd);
 	}
-
-	return acxt->cnt;
 }
 
 /**

fs/sysfs/inode.c +5 -2

index f05cda9..e74224e 100644
@@ -201,7 +201,10 @@ int sysfs_hash_and_remove(struct sysfs_dirent *dir_sd, const char *name)
 	if (sd)
 		sysfs_remove_one(&acxt, sd);
 
-	if (sysfs_addrm_finish(&acxt))
+	sysfs_addrm_finish(&acxt);
+
+	if (sd)
 		return 0;
-	return -ENOENT;
+	else
+		return -ENOENT;
 }

fs/sysfs/sysfs.h +1 -1

index bb3f0c9..0436754 100644
@@ -66,7 +66,7 @@ extern int sysfs_add_one(struct sysfs_addrm_cxt *acxt,
 			  struct sysfs_dirent *sd);
 extern void sysfs_remove_one(struct sysfs_addrm_cxt *acxt,
 			     struct sysfs_dirent *sd);
-extern int sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt);
+extern void sysfs_addrm_finish(struct sysfs_addrm_cxt *acxt);
 
 extern struct inode * sysfs_get_inode(struct sysfs_dirent *sd);
 extern void sysfs_instantiate(struct dentry *dentry, struct inode *inode);

Signature · 1 line

1.5.0.3

Satyam Sharma

2007-07-31 12:43 UTC #16 lore raw

Re: [PATCH 7/7] sysfs: make sysfs_addrm_finish() return void

Quoted 9 lines · Tejun Heo

With the previous sysfs_add_one() update, there is only one user of the return value of sysfs_addrm_finish() and the user can switch to testing @sd easily. Make sysfs_addrm_finish() return void for cleaner semantics as suggested by Satyam Sharma.
This patch doesn't introduce any noticeable behavior change.
Signed-off-by Tejun Heo <[email protected]>
Cc Satyam Sharma <[email protected]>

Thanks.

Cornelia Huck

Acked-by on v1 7/7

2007-07-31 13:33 UTC #17 lore raw

Re: [PATCH 7/7] sysfs: make sysfs_addrm_finish() return void

In reply to Tejun Heo

Quoted 7 lines · Tejun Heo

With the previous sysfs_add_one() update, there is only one user of the return value of sysfs_addrm_finish() and the user can switch to testing @sd easily. Make sysfs_addrm_finish() return void for cleaner semantics as suggested by Satyam Sharma.
This patch doesn't introduce any noticeable behavior change.

Quoted 8 lines

Signed-off-by Tejun Heo <[email protected]>
Cc Satyam Sharma <[email protected]>
---
 fs/sysfs/dir.c   |    7 +------
 fs/sysfs/inode.c |    7 +++++--
 fs/sysfs/sysfs.h |    2 +-
 3 files changed, 7 insertions(+), 9 deletions(-)

Acked-by Cornelia Huck <[email protected]>

Cornelia Huck

2007-07-31 12:54 UTC #18 lore raw

In reply to Tejun Heo

Quoted 8 lines · Tejun Heo

Hello, all.
This patchset contains a locking fix and cleanup patches for sysfs.
#01 locking fix
#02 shadow support removal from Eric adapted to apply after #01
#03-07 clean up patches

Quoted 8 lines

#01 should go mainline as it fixes a locking regression introduced by -rc1 merge. The rest should stay in -gregkh till 24 merge window opens up.
Clean up patches don't introduce any noticeable behavior change. They just refactor and simplify codes.
Thanks.

FYI: This seems to work well on s390 so far, when attaching/detaching and moving devices (my standard test cases ;)

Tejun Heo

author

2007-07-31 13:24 UTC #19 lore raw

Quoted 15 lines · Cornelia Huck

Quoted 12 lines · Tejun Heo
Hello, all.
This patchset contains a locking fix and cleanup patches for sysfs.
#01 locking fix
#02 shadow support removal from Eric adapted to apply after #01
#03-07 clean up patches
#01 should go mainline as it fixes a locking regression introduced by -rc1 merge. The rest should stay in -gregkh till 24 merge window opens up.

Quoted 7 lines

Quoted 4 lines
Clean up patches don't introduce any noticeable behavior change. They just refactor and simplify codes.
Thanks.
FYI: This seems to work well on s390 so far, when attaching/detaching and moving devices (my standard test cases ;)

Thanks a lot for testing as always. :-)

Signature · 1 line

tejun