[PATCHSET] Sysfs cleanups from Eric W. Biederman

[PATCHSET] Sysfs cleanups from Eric W. Biederman

[Tejun Heo]

Hello, all.

This is subset of Eric W. Biederman's "Sysfs cleanups & tagged
directory support" patchset[1] with the following modifications.

* fix-i_mutex-locking-in-sysfs_get_dentry patch is added at the top
  and #14-Don_t-use-lookup_one_len_kern and
  #15-vfs-Remove-lookup_one_len_kern are dropped.  This is because #14
  contained had a bug where it might created dentry/inode for an
  already deleted sysfs_dirent.  I think it's benefitial to keep
  single lookup path.

* Rewrote simplify-sysfs_get_dentry patch and
  #08-Implement-__sysfs_get_dentry,
  #09-Move-sysfs_get_dentry-below-__sysfs_get_dentry and
  #10-Rewrite-sysfs_get_dentry-in-terms-of-__sysfs_get_dentry are
  omitted as __sysfs_get_dentry() isn't used by anyone.

* #16, 19-25 are omitted as it isn't clear yet how the tagged entry
  support will end up.

* readdir simplification fixed.

* sysfs_mutex double locking fixed.

The patchset is on top of the current -gregkh.

Thanks.

--
tejun

[1] http://thread.gmane.org/gmane.linux.kernel/564287/focus=519

[PATCH 01/14] sysfs: fix i_mutex locking in sysfs_get_dentry()

[Tejun Heo]

lookup_one_len_kern() should be called with the parent's i_mutex
locked.  Fix it.

Spotted by Eric W. Biederman.

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Eric W. Biederman <ebiederm@xmission.com>
---
 fs/sysfs/dir.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index e4fd8a2..b33af5e 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -130,8 +130,10 @@ struct dentry *sysfs_get_dentry(struct sysfs_dirent *sd)
 
 		/* look it up */
 		parent_dentry = dentry;
+		mutex_lock(&parent_dentry->d_inode->i_mutex);
 		dentry = lookup_one_len_kern(cur->s_name, parent_dentry,
 					     strlen(cur->s_name));
+		mutex_unlock(&parent_dentry->d_inode->i_mutex);
 		dput(parent_dentry);
 
 		if (IS_ERR(dentry)) {
-- 
1.5.0.3

[PATCH 12/14] sysfs: kill SYSFS_FLAG_REMOVED

[Tejun Heo]

With sysfs_get_dentry() simplified, there's no user of
SYSFS_FLAG_REMOVED left.  Kill it.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 fs/sysfs/dir.c        |    5 +----
 include/linux/sysfs.h |    1 -
 2 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index c55c811..1752342 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -222,7 +222,7 @@ static void sysfs_deactivate(struct sysfs_dirent *sd)
 	DECLARE_COMPLETION_ONSTACK(wait);
 	int v;
 
-	BUG_ON(sd->s_sibling || !(sd->s_flags & SYSFS_FLAG_REMOVED));
+	BUG_ON(sd->s_sibling);
 	sd->s_sibling = (void *)&wait;
 
 	/* atomic_add_return() is a mb(), put_active() will always see
@@ -462,11 +462,8 @@ int 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_flags & SYSFS_FLAG_REMOVED);
-
 	sysfs_unlink_sibling(sd);
 
-	sd->s_flags |= SYSFS_FLAG_REMOVED;
 	sd->s_sibling = acxt->removed;
 	acxt->removed = sd;
 
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index c16e4c5..82c31b2 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -83,7 +83,6 @@ struct sysfs_ops {
 #define SYSFS_COPY_NAME		(SYSFS_DIR | SYSFS_KOBJ_LINK)
 
 #define SYSFS_FLAG_MASK		~SYSFS_TYPE_MASK
-#define SYSFS_FLAG_REMOVED	0x0100
 
 #ifdef CONFIG_SYSFS
 
-- 
1.5.0.3

[PATCH 10/14] sysfs: simply sysfs_get_dentry

[Tejun Heo]

Now that we know the sysfs tree structure cannot change under us and
sysfs shadow support is dropped, sysfs_get_dentry() can be simplified
greatly.  It can just look up from the root and there's no need to
retry on failure.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 fs/sysfs/dir.c |   91 ++++++++++----------------------------------------------
 1 files changed, 16 insertions(+), 75 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 740d88e..aee24e9 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -78,9 +78,8 @@ static void sysfs_unlink_sibling(struct sysfs_dirent *sd)
  *	@sd: sysfs_dirent of interest
  *
  *	Get dentry for @sd.  Dentry is looked up if currently not
- *	present.  This function climbs sysfs_dirent tree till it
- *	reaches a sysfs_dirent with valid dentry attached and descends
- *	down from there looking up dentry for each step.
+ *	present.  This function descends from the root looking up
+ *	dentry for each step.
  *
  *	LOCKING:
  *	mutex_lock(sysfs_rename_mutex)
@@ -90,86 +89,28 @@ static void sysfs_unlink_sibling(struct sysfs_dirent *sd)
  */
 struct dentry *sysfs_get_dentry(struct sysfs_dirent *sd)
 {
-	struct sysfs_dirent *cur;
-	struct dentry *parent_dentry, *dentry;
-	int i, depth;
+	struct dentry *dentry = dget(sysfs_sb->s_root);
 
-	/* Find the first parent which has valid s_dentry and get the
-	 * dentry.
-	 */
-	mutex_lock(&sysfs_mutex);
- restart0:
-	spin_lock(&sysfs_assoc_lock);
- restart1:
-	spin_lock(&dcache_lock);
-
-	dentry = NULL;
-	depth = 0;
-	cur = sd;
-	while (!cur->s_dentry || !cur->s_dentry->d_inode) {
-		if (cur->s_flags & SYSFS_FLAG_REMOVED) {
-			dentry = ERR_PTR(-ENOENT);
-			depth = 0;
-			break;
-		}
-		cur = cur->s_parent;
-		depth++;
-	}
-	if (!IS_ERR(dentry))
-		dentry = dget_locked(cur->s_dentry);
+	while (dentry->d_fsdata != sd) {
+		struct sysfs_dirent *cur;
+		struct dentry *parent;
 
-	spin_unlock(&dcache_lock);
-	spin_unlock(&sysfs_assoc_lock);
-
-	/* from the found dentry, look up depth times */
-	while (depth--) {
-		/* find and get depth'th ancestor */
-		for (cur = sd, i = 0; cur && i < depth; i++)
+		/* find the first ancestor which hasn't been looked up */
+		cur = sd;
+		while (cur->s_parent != dentry->d_fsdata)
 			cur = cur->s_parent;
 
-		/* This can happen if tree structure was modified due
-		 * to move/rename.  Restart.
-		 */
-		if (i != depth) {
-			dput(dentry);
-			goto restart0;
-		}
-
-		sysfs_get(cur);
-
-		mutex_unlock(&sysfs_mutex);
-
 		/* look it up */
-		parent_dentry = dentry;
-		mutex_lock(&parent_dentry->d_inode->i_mutex);
-		dentry = lookup_one_len_kern(cur->s_name, parent_dentry,
+		parent = dentry;
+		mutex_lock(&parent->d_inode->i_mutex);
+		dentry = lookup_one_len_kern(cur->s_name, parent,
 					     strlen(cur->s_name));
-		mutex_unlock(&parent_dentry->d_inode->i_mutex);
-		dput(parent_dentry);
-
-		if (IS_ERR(dentry)) {
-			sysfs_put(cur);
-			return dentry;
-		}
+		mutex_unlock(&parent->d_inode->i_mutex);
+		dput(parent);
 
-		mutex_lock(&sysfs_mutex);
-		spin_lock(&sysfs_assoc_lock);
-
-		/* This, again, can happen if tree structure has
-		 * changed and we looked up the wrong thing.  Restart.
-		 */
-		if (cur->s_dentry != dentry) {
-			dput(dentry);
-			sysfs_put(cur);
-			goto restart1;
-		}
-
-		spin_unlock(&sysfs_assoc_lock);
-
-		sysfs_put(cur);
+		if (IS_ERR(dentry))
+			break;
 	}
-
-	mutex_unlock(&sysfs_mutex);
 	return dentry;
 }
 
-- 
1.5.0.3

Re: [PATCHSET] Sysfs cleanups from Eric W. Biederman

[Cornelia Huck]

On Mon, 20 Aug 2007 21:36:29 +0900,
Tejun Heo <htejun@gmail.com> wrote:

> Hello, all.
> 
> This is subset of Eric W. Biederman's "Sysfs cleanups & tagged
> directory support" patchset[1] with the following modifications.
> 
> * fix-i_mutex-locking-in-sysfs_get_dentry patch is added at the top
>   and #14-Don_t-use-lookup_one_len_kern and
>   #15-vfs-Remove-lookup_one_len_kern are dropped.  This is because #14
>   contained had a bug where it might created dentry/inode for an
>   already deleted sysfs_dirent.  I think it's benefitial to keep
>   single lookup path.
> 
> * Rewrote simplify-sysfs_get_dentry patch and
>   #08-Implement-__sysfs_get_dentry,
>   #09-Move-sysfs_get_dentry-below-__sysfs_get_dentry and
>   #10-Rewrite-sysfs_get_dentry-in-terms-of-__sysfs_get_dentry are
>   omitted as __sysfs_get_dentry() isn't used by anyone.
> 
> * #16, 19-25 are omitted as it isn't clear yet how the tagged entry
>   support will end up.
> 
> * readdir simplification fixed.
> 
> * sysfs_mutex double locking fixed.
> 
> The patchset is on top of the current -gregkh.

Survives some casual testing (including moving devices). Patches look
sane to me at a cursory glance as well (sorry, don't have much time at
the moment).

Re: [PATCHSET] Sysfs cleanups from Eric W. Biederman

[Tejun Heo]

Cornelia Huck wrote:
> On Mon, 20 Aug 2007 21:36:29 +0900,
> Tejun Heo <htejun@gmail.com> wrote:
> 
>> Hello, all.
>>
>> This is subset of Eric W. Biederman's "Sysfs cleanups & tagged
>> directory support" patchset[1] with the following modifications.
>>
>> * fix-i_mutex-locking-in-sysfs_get_dentry patch is added at the top
>>   and #14-Don_t-use-lookup_one_len_kern and
>>   #15-vfs-Remove-lookup_one_len_kern are dropped.  This is because #14
>>   contained had a bug where it might created dentry/inode for an
>>   already deleted sysfs_dirent.  I think it's benefitial to keep
>>   single lookup path.
>>
>> * Rewrote simplify-sysfs_get_dentry patch and
>>   #08-Implement-__sysfs_get_dentry,
>>   #09-Move-sysfs_get_dentry-below-__sysfs_get_dentry and
>>   #10-Rewrite-sysfs_get_dentry-in-terms-of-__sysfs_get_dentry are
>>   omitted as __sysfs_get_dentry() isn't used by anyone.
>>
>> * #16, 19-25 are omitted as it isn't clear yet how the tagged entry
>>   support will end up.
>>
>> * readdir simplification fixed.
>>
>> * sysfs_mutex double locking fixed.
>>
>> The patchset is on top of the current -gregkh.
> 
> Survives some casual testing (including moving devices). Patches look
> sane to me at a cursory glance as well (sorry, don't have much time at
> the moment).

Thanks.

-- 
tejun

Re: [PATCHSET] Sysfs cleanups from Eric W. Biederman

[Eric W. Biederman]

Tejun Heo <htejun@gmail.com> writes:

> Hello, all.
>
> This is subset of Eric W. Biederman's "Sysfs cleanups & tagged
> directory support" patchset[1] with the following modifications.

As a base: 
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

>
> * fix-i_mutex-locking-in-sysfs_get_dentry patch is added at the top
>   and #14-Don_t-use-lookup_one_len_kern and
>   #15-vfs-Remove-lookup_one_len_kern are dropped.  This is because #14
>   contained had a bug where it might created dentry/inode for an
>   already deleted sysfs_dirent.  I think it's benefitial to keep
>   single lookup path.

I think I disagree with the bug spotting.

At least in net we the sysfs_rename_mutex which keeps parent
directories from disappearing.  Further we have a reference
to the leaf sysfs_dirent and are actively manipulating it, so
the sysfs_dirent should not disappear on us.

> * Rewrote simplify-sysfs_get_dentry patch and
>   #08-Implement-__sysfs_get_dentry,
>   #09-Move-sysfs_get_dentry-below-__sysfs_get_dentry and
>   #10-Rewrite-sysfs_get_dentry-in-terms-of-__sysfs_get_dentry are
>   omitted as __sysfs_get_dentry() isn't used by anyone.

Right.  __sysfs_get_dentry is an optimization that has makes
the best case for sysfs_get_dentry O(1) instead of O(depth).
However this doesn't matter because sysfs_get_dentry is not
on any fast path and the maximum depth of sysfs directories
is fairly shallow and programmer controlled.

The only user other user of __sysfs_get_dentry is in the tagged
directory support, and even that user doesn't strictly need it.
Although it is a bit silly to populate the dcache just so you
can invalidate it a moment later...

Just doing the dget(sysfs_sb->s_root) is a bit clearer in
sysfs_get_dentry then knowing implicitly that is what
__sysfs_get_dentry does in the worst cased.

> * #16, 19-25 are omitted as it isn't clear yet how the tagged entry
>   support will end up.
>
> * readdir simplification fixed.
>
> * sysfs_mutex double locking fixed.
>
> The patchset is on top of the current -gregkh.

Eric

Re: [PATCHSET] Sysfs cleanups from Eric W. Biederman

[Tejun Heo]

Hello,

Eric W. Biederman wrote:
>> * fix-i_mutex-locking-in-sysfs_get_dentry patch is added at the top
>>   and #14-Don_t-use-lookup_one_len_kern and
>>   #15-vfs-Remove-lookup_one_len_kern are dropped.  This is because #14
>>   contained had a bug where it might created dentry/inode for an
>>   already deleted sysfs_dirent.  I think it's benefitial to keep
>>   single lookup path.
> 
> I think I disagree with the bug spotting.
> 
> At least in net we the sysfs_rename_mutex which keeps parent
> directories from disappearing.  Further we have a reference
> to the leaf sysfs_dirent and are actively manipulating it, so
> the sysfs_dirent should not disappear on us.

sysfs_rename_mutex() keeps out renaming and moving not removing.  Also,
reference prevents sysfs_dirent from being released not from being
removed.  The different in lookup path is that it searches the children
list - sd's are unlinked from children list on removal.

>> * Rewrote simplify-sysfs_get_dentry patch and
>>   #08-Implement-__sysfs_get_dentry,
>>   #09-Move-sysfs_get_dentry-below-__sysfs_get_dentry and
>>   #10-Rewrite-sysfs_get_dentry-in-terms-of-__sysfs_get_dentry are
>>   omitted as __sysfs_get_dentry() isn't used by anyone.
> 
> Right.  __sysfs_get_dentry is an optimization that has makes
> the best case for sysfs_get_dentry O(1) instead of O(depth).
> However this doesn't matter because sysfs_get_dentry is not
> on any fast path and the maximum depth of sysfs directories
> is fairly shallow and programmer controlled.

The reason why sysfs_get_dentry() climbed up first then climbed down was
not because of performance.  It was to support the original shadow
implementation.  Because there was no reliable way to reach a leaf node
from the root, dentries of all shadows are pinned such that they can
serve as the starting point for dentry lookup and the climing up was to
reach that starting point.  Now that the dentry-multiplexing shadow
support is gone, there's no need to do the climing up.

> The only user other user of __sysfs_get_dentry is in the tagged
> directory support, and even that user doesn't strictly need it.
> Although it is a bit silly to populate the dcache just so you
> can invalidate it a moment later...

Yeah, actually the only essential user of that kind of look up is
sysfs_drop_dentry() which is in deletion path and can't fail due to
allocation failure and has the logic open-coded.  I have nothing against
__sysfs_get_dentry().  It was just not needed by the patches I forwarded
this time.  Feel free to include it as you see fit.

I'm currently working on kobj/sysfs separation.  As most internal
implementation is sd based already, the changes are mostly confined to
interface functions but it's still a big change.  I think I'll be able
to post the patches in this week or early next week at the latest.

Thanks.

-- 
tejun

Re: [PATCHSET] Sysfs cleanups from Eric W. Biederman

[Eric W. Biederman]

Tejun Heo <htejun@gmail.com> writes:

> Hello,
>
> Eric W. Biederman wrote:
>>> * fix-i_mutex-locking-in-sysfs_get_dentry patch is added at the top
>>>   and #14-Don_t-use-lookup_one_len_kern and
>>>   #15-vfs-Remove-lookup_one_len_kern are dropped.  This is because #14
>>>   contained had a bug where it might created dentry/inode for an
>>>   already deleted sysfs_dirent.  I think it's benefitial to keep
>>>   single lookup path.
>> 
>> I think I disagree with the bug spotting.
>> 
>> At least in net we the sysfs_rename_mutex which keeps parent
>> directories from disappearing.  Further we have a reference
>> to the leaf sysfs_dirent and are actively manipulating it, so
>> the sysfs_dirent should not disappear on us.
>
> sysfs_rename_mutex() keeps out renaming and moving not removing.  Also,
> reference prevents sysfs_dirent from being released not from being
> removed.  The different in lookup path is that it searches the children
> list - sd's are unlinked from children list on removal.

True.   I guess what I was seeing was not a code but a usages
restriction which may not be enforced.

If we are using sysfs_get_dentry the sd is not deleted because
it is alive and being used, likewise the path to it should also be
alive.  Once you add in classic unix delete behaviour allowing for
alive but deleted files this no longer applies.  But I don't think
that makes sense for sysfs, but I may be confused.

>>> * Rewrote simplify-sysfs_get_dentry patch and
>>>   #08-Implement-__sysfs_get_dentry,
>>>   #09-Move-sysfs_get_dentry-below-__sysfs_get_dentry and
>>>   #10-Rewrite-sysfs_get_dentry-in-terms-of-__sysfs_get_dentry are
>>>   omitted as __sysfs_get_dentry() isn't used by anyone.
>> 
>> Right.  __sysfs_get_dentry is an optimization that has makes
>> the best case for sysfs_get_dentry O(1) instead of O(depth).
>> However this doesn't matter because sysfs_get_dentry is not
>> on any fast path and the maximum depth of sysfs directories
>> is fairly shallow and programmer controlled.
>
> The reason why sysfs_get_dentry() climbed up first then climbed down was
> not because of performance.  It was to support the original shadow
> implementation.  Because there was no reliable way to reach a leaf node
> from the root, dentries of all shadows are pinned such that they can
> serve as the starting point for dentry lookup and the climing up was to
> reach that starting point.  Now that the dentry-multiplexing shadow
> support is gone, there's no need to do the climing up.

Reasonable.  The reason I kept the climbing up was the performance
benefit I saw.

>> The only user other user of __sysfs_get_dentry is in the tagged
>> directory support, and even that user doesn't strictly need it.
>> Although it is a bit silly to populate the dcache just so you
>> can invalidate it a moment later...
>
> Yeah, actually the only essential user of that kind of look up is
> sysfs_drop_dentry() which is in deletion path and can't fail due to
> allocation failure and has the logic open-coded.  I have nothing against
> __sysfs_get_dentry().  It was just not needed by the patches I forwarded
> this time.  Feel free to include it as you see fit.

Sure.

> I'm currently working on kobj/sysfs separation.  As most internal
> implementation is sd based already, the changes are mostly confined to
> interface functions but it's still a big change.  I think I'll be able
> to post the patches in this week or early next week at the latest.

Ok.

I'd like to make certain we have a path to tagged support or something
like it if we can.

Eric