From: ebiederm@xmission.com (Eric W. Biederman) Subject: Re: [PATCH 08/20] sysfs: Optimize just changing the sysfs file mode. Date: Thu, 21 May 2009 00:54:18 -0700 Message-ID: References: <1242865694-2100-1-git-send-email-ebiederm@xmission.com> <1242865694-2100-2-git-send-email-ebiederm@xmission.com> <1242865694-2100-3-git-send-email-ebiederm@xmission.com> <1242865694-2100-4-git-send-email-ebiederm@xmission.com> <1242865694-2100-5-git-send-email-ebiederm@xmission.com> <1242865694-2100-6-git-send-email-ebiederm@xmission.com> <1242865694-2100-7-git-send-email-ebiederm@xmission.com> <1242865694-2100-8-git-send-email-ebiederm@xmission.com> <4A1502C6.5020103@kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Andrew Morton , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, Cornelia Huck , linux-fsdevel@vger.kernel.org, "Eric W. Biederman" To: Tejun Heo Return-path: Received: from out01.mta.xmission.com ([166.70.13.231]:58939 "EHLO out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751437AbZEUHyW (ORCPT ); Thu, 21 May 2009 03:54:22 -0400 In-Reply-To: <4A1502C6.5020103@kernel.org> (Tejun Heo's message of "Thu\, 21 May 2009 16\:29\:10 +0900") Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Tejun Heo writes: > Eric W. Biederman wrote: >> From: Eric W. Biederman >> >> Don't allocate a struct iattr for the sysfs dentry if just >> the mode changes because we have a field for that on the >> sysfs_dirent, and we can trigger that case with sysfs_chmod_file. >> >> Signed-off-by: Eric W. Biederman >> --- >> fs/sysfs/inode.c | 22 ++++++++++++++-------- >> 1 files changed, 14 insertions(+), 8 deletions(-) >> >> diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c >> index 555f0ff..70ff2a2 100644 >> --- a/fs/sysfs/inode.c >> +++ b/fs/sysfs/inode.c >> @@ -60,12 +60,16 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr) >> return error; >> >> iattr->ia_valid &= ~ATTR_SIZE; /* ignore size changes */ >> + if (iattr->ia_valid & ATTR_MODE) { >> + if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID)) >> + iattr->ia_mode &= ~S_ISGID; >> + } >> >> error = inode_setattr(inode, iattr); >> if (error) >> return error; >> >> - if (!sd_iattr) { >> + if (!sd_iattr && (ia_valid & ~ATTR_MODE)) { >> /* setting attributes for the first time, allocate now */ >> sd_iattr = kzalloc(sizeof(struct iattr), GFP_KERNEL); >> if (!sd_iattr) >> @@ -78,6 +82,13 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr) >> sd->s_iattr = sd_iattr; >> } >> >> + if (ia_valid & ATTR_MODE) >> + sd->s_mode = iattr->ia_mode; >> + >> + /* If we don't need the extra attributes leave */ >> + if (!sd_iattr) >> + return 0; > > One visible difference is lack of timestamp update. Is there any use > case where sysfs file mode changing needs to be fast?\ Not really. If the time changes we set something besides ATTR_MODE like ATTR_MTIME or ATTR_CTIME. If we come in through any of the user space entry points ATTR_CTIME appears to be set so this optimization will not trigger. I think there are cases where we only opportunistically track time changes, when the structure is allocated that this changes but it is a very small percentage of the time. The practical effect of my changes should be that we only track timestamps when user space actually performs an explicit change to the file. If someone was depending on some weird indirect side effect like that on one of the 5-6 files that calls sysfs_chmod let's make it explicit. For me this isn't about making this go faster. This is about keeping the sysfs data structures small when we can. It doesn't really complicate the code and we wind up doing the obvious thing. Eric