sysfs: Cache the last sysfs_dirent to improve readdir scalability
diff mbox series

Message ID m11viaexlj.fsf_-_@fess.ebiederm.org
State New, archived
Headers show
Series
  • sysfs: Cache the last sysfs_dirent to improve readdir scalability
Related show

Commit Message

Eric W. Biederman Jan. 1, 2010, 1:58 p.m. UTC
When sysfs_readdir stops short we now cache the next sysfs_dirent to
return to user space in filp->private_data.  There is no impact on the
rest of sysfs by doing this and in the common case it allows us to
pick up exactly where we left off with no seeking.

Additionally I drop and regrab the sysfs_mutex around filldir to avoid
a page fault arbitrarily increasing the hold time on the sysfs_mutex.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---

I haven't stressed this patch but it is sound in principle, and is a
general sysfs improvement, regardless of any locking issues.

 fs/sysfs/dir.c |   85 ++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 62 insertions(+), 23 deletions(-)

Comments

Borislav Petkov Jan. 1, 2010, 3:33 p.m. UTC | #1
On Fri, Jan 01, 2010 at 05:58:00AM -0800, Eric W. Biederman wrote:
> 
> When sysfs_readdir stops short we now cache the next sysfs_dirent to
> return to user space in filp->private_data.  There is no impact on the
> rest of sysfs by doing this and in the common case it allows us to
> pick up exactly where we left off with no seeking.
> 
> Additionally I drop and regrab the sysfs_mutex around filldir to avoid
> a page fault arbitrarily increasing the hold time on the sysfs_mutex.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
> ---
> 
> I haven't stressed this patch but it is sound in principle, and is a
> general sysfs improvement, regardless of any locking issues.

I've slapped it ontop of v2.6.33-rc2-249-gcd6e125 here and the circular
locking warning is gone. I'll keep an eye on it in the next couple of
days, just in case.

Tested-by: Borislav Petkov <petkovbb@gmail.com>
Linus Torvalds Jan. 1, 2010, 6:56 p.m. UTC | #2
On Fri, 1 Jan 2010, Eric W. Biederman wrote:
> 
> When sysfs_readdir stops short we now cache the next sysfs_dirent to
> return to user space in filp->private_data.  There is no impact on the
> rest of sysfs by doing this and in the common case it allows us to
> pick up exactly where we left off with no seeking.
> 
> Additionally I drop and regrab the sysfs_mutex around filldir to avoid
> a page fault arbitrarily increasing the hold time on the sysfs_mutex.

Ok, looks mostly sane, but a few things look odd.

>  
> -	if (filp->f_pos == 0) {
> +	if (!pos && filp->f_pos == 0) {
>  		ino = parent_sd->s_ino;
>  		if (filldir(dirent, ".", 1, filp->f_pos, ino, DT_DIR) == 0)
>  			filp->f_pos++;
>  	}
> -	if (filp->f_pos == 1) {
> +	if (!pos && filp->f_pos == 1) {
>  		if (parent_sd->s_parent)
>  			ino = parent_sd->s_parent->s_ino;
>  		else
> @@ -847,29 +879,35 @@ static int sysfs_readdir(struct file * filp, void * dirent, filldir_t filldir)
>  		if (filldir(dirent, "..", 2, filp->f_pos, ino, DT_DIR) == 0)
>  			filp->f_pos++;
>  	}
> -	if ((filp->f_pos > 1) && (filp->f_pos < INT_MAX)) {
> -		mutex_lock(&sysfs_mutex);
> -
> -		/* Skip the dentries we have already reported */
> -		pos = parent_sd->s_dir.children;
> -		while (pos && (filp->f_pos > pos->s_ino))
> -			pos = pos->s_sibling;
> +	/* EOF? */
> +	if (!pos && filp->f_pos > 2)
> +		return 0;

These are all incorrect in the presense of 'lseek'. You can't do that

	if (!pos && "test filp->f_pos")

thing, because you get all the wrong results for both the case of an lseek 
before doing any readdir (which is undefined behavior, so I guess that's 
technically ok) _and_ for the 'lseek back to zero _after_ doing a readdir' 
case (which is _not_ undefined behavior!

It looks like it might be easy to fix by making a sysfs_llseek() function 
that does something like

	.. sysfs_llseek(..)
	{
		mutex_lock(&sysfs_mutex);
		sysfs_release();
		filp->private_data = NULL;
		mutex_unlock(&sysfs_mutex);

		return generic_file_llseek(..);
	}

or similar.  Except themn you'll need to change the EOF condition testing 
and turn it into a re-validation event. Or maybe do the re-validation in 
sysfs_llseek() itself, rather than just dropping the cached data.

Hmm? I haven't thought it through very deeply, so maybe I'm missing 
something.

		Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Linus Torvalds Jan. 1, 2010, 11:10 p.m. UTC | #3
On Fri, 1 Jan 2010, Eric W. Biederman wrote:
>  		mutex_unlock(&sysfs_mutex);
> +		ret = filldir(dirent, name, len, filp->f_pos, ino, type);
> +		mutex_lock(&sysfs_mutex);
> +		if (ret < 0)
> +			break;
> +	}
> +	mutex_unlock(&sysfs_mutex);
> +	if ((filp->f_pos > 1) && !pos) { /* EOF */
> +		filp->f_pos = INT_MAX;
> +		filp->private_data = NULL;
>  	}
>  	return 0;

That

	mutex_lock(&sysfs_mutex);
	if (ret < 0)
		break;

looks just silly. We know 'pos' is non-NULL, so the break will effectively 
just be a "mutex_unlock + return 0", and we just did the mutex_lock, so 
why not instead do

	if (ret < 0)
		return 0;
	mutex_lock(&sysfs_mutex);

there?

Not that it really _matters_, but it seems way clearer, no?

But other than that mindless nit, I can't see anything wrong with your 
logic, and it looks ok to me from just reading the patch itself.

So I guess that's an "Ack", although I'd prefer it to get some more 
testing and perhaps go through Greg's tree as sysfs patches usually go.

And by "testing" I mean both the "yes, this second version also breaks the 
lockdep chain and avoids the warning", but also some kind of actual 
testing of /sysfs itself. If there is any.

		Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Greg KH Jan. 2, 2010, 5:59 a.m. UTC | #4
On Fri, Jan 01, 2010 at 03:10:49PM -0800, Linus Torvalds wrote:
> 
> So I guess that's an "Ack", although I'd prefer it to get some more 
> testing and perhaps go through Greg's tree as sysfs patches usually go.
> 
> And by "testing" I mean both the "yes, this second version also breaks the 
> lockdep chain and avoids the warning", but also some kind of actual 
> testing of /sysfs itself. If there is any.

I'll queue this up in my tree on Monday to get it some testing.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Borislav Petkov Jan. 2, 2010, 3:40 p.m. UTC | #5
On Fri, Jan 01, 2010 at 03:10:49PM -0800, Linus Torvalds wrote:
> And by "testing" I mean both the "yes, this second version also breaks the 
> lockdep chain and avoids the warning",

Yes, it survived a couple of suspend/resume cycles so far.

Patch
diff mbox series

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index f05f230..ad8a57e 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -827,19 +827,51 @@  static inline unsigned char dt_type(struct sysfs_dirent *sd)
 	return (sd->s_mode >> 12) & 15;
 }
 
+static int sysfs_dir_release(struct inode *inode, struct file *filp)
+{
+	sysfs_put(filp->private_data);
+	return 0;
+}
+
+static struct sysfs_dirent *sysfs_dir_pos(struct sysfs_dirent *parent_sd,
+	ino_t ino, struct sysfs_dirent *pos)
+{
+	if (pos) {
+		int valid = !(pos->s_flags & SYSFS_FLAG_REMOVED) &&
+			pos->s_parent == parent_sd &&
+			ino == pos->s_ino;
+		sysfs_put(pos);
+		if (valid)
+			return pos;
+	}
+	pos = parent_sd->s_dir.children;
+	while (pos && (ino > pos->s_ino))
+		pos = pos->s_sibling;
+	return pos;
+}
+
+static struct sysfs_dirent *sysfs_dir_next_pos(struct sysfs_dirent *parent_sd,
+	ino_t ino, struct sysfs_dirent *pos)
+{
+	pos = sysfs_dir_pos(parent_sd, ino, pos);
+	if (pos)
+		pos = pos->s_sibling;
+	return pos;
+}
+
 static int sysfs_readdir(struct file * filp, void * dirent, filldir_t filldir)
 {
 	struct dentry *dentry = filp->f_path.dentry;
 	struct sysfs_dirent * parent_sd = dentry->d_fsdata;
-	struct sysfs_dirent *pos;
+	struct sysfs_dirent *pos = filp->private_data;
 	ino_t ino;
 
-	if (filp->f_pos == 0) {
+	if (!pos && filp->f_pos == 0) {
 		ino = parent_sd->s_ino;
 		if (filldir(dirent, ".", 1, filp->f_pos, ino, DT_DIR) == 0)
 			filp->f_pos++;
 	}
-	if (filp->f_pos == 1) {
+	if (!pos && filp->f_pos == 1) {
 		if (parent_sd->s_parent)
 			ino = parent_sd->s_parent->s_ino;
 		else
@@ -847,29 +879,35 @@  static int sysfs_readdir(struct file * filp, void * dirent, filldir_t filldir)
 		if (filldir(dirent, "..", 2, filp->f_pos, ino, DT_DIR) == 0)
 			filp->f_pos++;
 	}
-	if ((filp->f_pos > 1) && (filp->f_pos < INT_MAX)) {
-		mutex_lock(&sysfs_mutex);
-
-		/* Skip the dentries we have already reported */
-		pos = parent_sd->s_dir.children;
-		while (pos && (filp->f_pos > pos->s_ino))
-			pos = pos->s_sibling;
+	/* EOF? */
+	if (!pos && filp->f_pos > 2)
+		return 0;
 
-		for ( ; pos; pos = pos->s_sibling) {
-			const char * name;
-			int len;
-
-			name = pos->s_name;
-			len = strlen(name);
-			filp->f_pos = ino = pos->s_ino;
+	mutex_lock(&sysfs_mutex);
+	for (pos = sysfs_dir_pos(parent_sd, filp->f_pos, pos);
+	     pos;
+	     pos = sysfs_dir_next_pos(parent_sd, filp->f_pos, pos)) {
+		const char * name;
+		unsigned int type;
+		int len, ret;
+
+		name = pos->s_name;
+		len = strlen(name);
+		ino = pos->s_ino;
+		type = dt_type(pos);
+		filp->f_pos = ino;
+		filp->private_data = sysfs_get(pos);
 
-			if (filldir(dirent, name, len, filp->f_pos, ino,
-					 dt_type(pos)) < 0)
-				break;
-		}
-		if (!pos)
-			filp->f_pos = INT_MAX;
 		mutex_unlock(&sysfs_mutex);
+		ret = filldir(dirent, name, len, filp->f_pos, ino, type);
+		mutex_lock(&sysfs_mutex);
+		if (ret < 0)
+			break;
+	}
+	mutex_unlock(&sysfs_mutex);
+	if (!pos) { /* EOF */
+		filp->f_pos = 3;
+		filp->private_data = NULL;
 	}
 	return 0;
 }
@@ -878,5 +916,6 @@  static int sysfs_readdir(struct file * filp, void * dirent, filldir_t filldir)
 const struct file_operations sysfs_dir_operations = {
 	.read		= generic_read_dir,
 	.readdir	= sysfs_readdir,
+	.release	= sysfs_dir_release,
 	.llseek		= generic_file_llseek,
 };