Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752193Ab0AAXLg (ORCPT ); Fri, 1 Jan 2010 18:11:36 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752130Ab0AAXLe (ORCPT ); Fri, 1 Jan 2010 18:11:34 -0500 Received: from smtp1.linux-foundation.org ([140.211.169.13]:55760 "EHLO smtp1.linux-foundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752054Ab0AAXLd (ORCPT ); Fri, 1 Jan 2010 18:11:33 -0500 Date: Fri, 1 Jan 2010 15:10:49 -0800 (PST) From: Linus Torvalds X-X-Sender: torvalds@localhost.localdomain To: "Eric W. Biederman" cc: KOSAKI Motohiro , Borislav Petkov , David Airlie , Linux Kernel Mailing List , Greg KH , Al Viro Subject: Re: [PATCH] sysfs: Cache the last sysfs_dirent to improve readdir scalability v2 In-Reply-To: Message-ID: References: <20091226094504.GA6214@liondog.tnic> <20091228092712.AA8C.A69D9226@jp.fujitsu.com> User-Agent: Alpine 2.00 (LFD 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1442 Lines: 50 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/