[PATCHSET 2.6.23-rc1] sysfs: locking fix and cleanups

[PATCHSET 2.6.23-rc1] sysfs: locking fix and cleanups

[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.

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

Thanks.

--
tejun

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

[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 <htejun@gmail.com>
---
 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(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 8ce3ffb..69e57be 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -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,
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index fcd065e..416351a 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -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;
 }
 
 
diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index 8f70ca7..f77ad61 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -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;
 
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index 32b8b64..bb3f0c9 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -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);
-- 
1.5.0.3

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

[Cornelia Huck]

On Tue, 31 Jul 2007 19:15:08 +0900,
Tejun Heo <htejun@gmail.com> wrote:

> 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 <htejun@gmail.com>
> ---
>  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(-)
> 
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index 8ce3ffb..69e57be 100644
> --- a/fs/sysfs/dir.c
> +++ b/fs/sysfs/dir.c
> @@ -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 <cornelia.huck@de.ibm.com>

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

[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 <htejun@gmail.com>
---
 fs/sysfs/dir.c |   26 ++++----------------------
 1 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 026ea70..0fe6aa3 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -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;
-- 
1.5.0.3

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

[Cornelia Huck]

On Tue, 31 Jul 2007 19:15:08 +0900,
Tejun Heo <htejun@gmail.com> wrote:

> 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 <htejun@gmail.com>
> ---
>  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 <cornelia.huck@de.ibm.com>

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

[Tejun Heo]

Cornelia Huck wrote:
> On Tue, 31 Jul 2007 19:15:08 +0900,
> Tejun Heo <htejun@gmail.com> wrote:
> 
>> 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 <htejun@gmail.com>
>> ---
>>  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.  :-)

-- 
tejun

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

[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 <htejun@gmail.com>
---
 fs/sysfs/dir.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 048e605..83e76b3 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -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);
-- 
1.5.0.3

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

[Cornelia Huck]

On Tue, 31 Jul 2007 19:15:08 +0900,
Tejun Heo <htejun@gmail.com> wrote:

> 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 <htejun@gmail.com>
> ---
>  fs/sysfs/dir.c |   21 ++++++++++++---------
>  1 files changed, 12 insertions(+), 9 deletions(-)

Looks sane.

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

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

[Greg KH]

On Tue, Jul 31, 2007 at 07:15:08PM +0900, Tejun Heo wrote:
> 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 <htejun@gmail.com>

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

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

[Eric W. Biederman]

Greg KH <greg@kroah.com> writes:

> On Tue, Jul 31, 2007 at 07:15:08PM +0900, Tejun Heo wrote:
>> 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 <htejun@gmail.com>
>
> 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

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

[Greg KH]

On Wed, Aug 01, 2007 at 05:29:00PM -0700, Greg KH wrote:
> On Tue, Jul 31, 2007 at 07:15:08PM +0900, Tejun Heo wrote:
> > 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 <htejun@gmail.com>
> 
> 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

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

[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 <htejun@gmail.com>
---
 fs/sysfs/dir.c |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 1c3a0dd..026ea70 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -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 */
-- 
1.5.0.3

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

[Cornelia Huck]

On Tue, 31 Jul 2007 19:15:08 +0900,
Tejun Heo <htejun@gmail.com> wrote:

> * remove space between * and symbol name in variable declaration.
> 
> * kill unnecessary new line.
> 
> * kill 'found' and test 'sd' instead.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  fs/sysfs/dir.c |   15 +++++----------
>  1 files changed, 5 insertions(+), 10 deletions(-)
> 
Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

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

[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 <htejun@gmail.com>
---
 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(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 0fe6aa3..8ce3ffb 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -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);
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 3e1cc06..fcd065e 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -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);
diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index 9671164..f05cda9 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -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;
diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index 4ce687f..8f70ca7 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -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;
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index b55e510..32b8b64 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -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);
-- 
1.5.0.3

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

[Cornelia Huck]

On Tue, 31 Jul 2007 19:15:08 +0900,
Tejun Heo <htejun@gmail.com> wrote:

> 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 <htejun@gmail.com>
> ---
>  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 <cornelia.huck@de.ibm.com>

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

[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 <htejun@gmail.com>
Cc: Satyam Sharma <satyam.sharma@gmail.com>
---
 fs/sysfs/dir.c   |    7 +------
 fs/sysfs/inode.c |    7 +++++--
 fs/sysfs/sysfs.h |    2 +-
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 69e57be..f9523c8 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -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;
 }
 
 /**
diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index f05cda9..e74224e 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -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;
 }
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index bb3f0c9..0436754 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -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);
-- 
1.5.0.3

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

[Satyam Sharma]

On Tue, 31 Jul 2007, Tejun Heo wrote:

> 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 <htejun@gmail.com>
> Cc: Satyam Sharma <satyam.sharma@gmail.com>

Thanks.

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

[Cornelia Huck]

On Tue, 31 Jul 2007 19:15:09 +0900,
Tejun Heo <htejun@gmail.com> wrote:

> 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 <htejun@gmail.com>
> Cc: Satyam Sharma <satyam.sharma@gmail.com>
> ---
>  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 <cornelia.huck@de.ibm.com>

Re: [PATCHSET 2.6.23-rc1] sysfs: locking fix and cleanups

[Cornelia Huck]

On Tue, 31 Jul 2007 19:15:08 +0900,
Tejun Heo <htejun@gmail.com> wrote:

> 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.

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

Re: [PATCHSET 2.6.23-rc1] sysfs: locking fix and cleanups

[Tejun Heo]

Cornelia Huck wrote:
> On Tue, 31 Jul 2007 19:15:08 +0900,
> Tejun Heo <htejun@gmail.com> wrote:
> 
>> 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.
> 
> 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.  :-)

-- 
tejun