Re: [PATCH 08/20] sysfs: Optimize just changing the sysfs file mode.

[ebiederm@xmission.com (Eric W. Biederman)]

Tejun Heo <tj@kernel.org> writes:

> Eric W. Biederman wrote:
>> From: Eric W. Biederman <ebiederm@xmission.com>
>> 
>> 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 <ebiederm@aristanetworks.com>
>> ---
>>  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