Messages in this thread | | | Date | Fri, 1 Jan 2010 15:10:49 -0800 (PST) | From | Linus Torvalds <> | Subject | Re: [PATCH] sysfs: Cache the last sysfs_dirent to improve readdir scalability v2 |
| |
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
| |