General sysfs enhancements
Subsystems (2)
cover General sysfs enhancements
This patchset is built on top of 2.6.33-rc3 +
"sysfs: Cache the last sysfs_dirent to improve readdir scalability v2"
Greg I assume you have that patch somewhere in queue.
This patchset contains little cleanups all around sysfs.
- Some basic cleanups.
- Some sysfs scaling work
- Implementation of sysfs_rename
drivers/base/core.c | 18 ++++++------------ fs/sysfs/dir.c | 22 +++++++++++++++++++++- fs/sysfs/inode.c | 28 ++++++++-------------------- fs/sysfs/mount.c | 5 ++--- fs/sysfs/symlink.c | 38 ++++++++++++++++++++++++++++++++++++++ fs/sysfs/sysfs.h | 6 +++--- include/linux/sysfs.h | 9 +++++++++ 7 files changed, 87 insertions(+), 39 deletions(-)
This should be the final patchset before we can seriously look at network namespace support in sysfs, again.
Eric
PATCH 1/7 sysfs: Serialize updates to the vfs inode
From: Eric W. Biederman <[email protected]>
The way the vfs is structured only calls to the filesystem methods actually update the vfs inode. We add to the normal number of places where the vfs inode is updated by also updating the vfs inode in sysfs_refresh_inode.
Grabbing the inode mutex in sysfs_permission and sysfs_getattr causes deadlocks, because somtimes those operations are called with the inode mutex held, but not always. Therefore we can not depend upon the inode mutex to serialize all updates to the vfs inode.
We take the sysfs_mutex in all of those places so we can also use it to protect the vfs inode. To accomplish that we simply requires extending the vfs inode in sysfs_setattr over inode_change_ok (so we have an unchanging inode when we perform the check), and inode_setattr.
--- fs/sysfs/inode.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
fs/sysfs/inode.c +4 -4
@@ -112,20 +112,20 @@ int sysfs_setattr(struct dentry *dentry, struct iattr *iattr)if (!sd)return -EINVAL ; +mutex_lock (&sysfs_mutex); error =inode_change_ok (inode, iattr);if (error) -return error; + goto out; iattr->ia_valid &= ~ATTR_SIZE ;/* ignore size changes */ error =inode_setattr (inode, iattr);if (error) -return error; + goto out; -mutex_lock (&sysfs_mutex); error =sysfs_sd_setattr (sd, iattr); +out:mutex_unlock (&sysfs_mutex); -return error; }
Signature · 1 line
1.6.5.2.143.g8cc62
Re: [PATCH 1/7] sysfs: Serialize updates to the vfs inode
Quoted 13 lines · Eric W. Biederman
From: Eric W. Biederman <[email protected]>
The way the vfs is structured only calls to the filesystem methods actually update the vfs inode. We add to the normal number of places where the vfs inode is updated by also updating the vfs inode in
sysfs_refresh_inode.Grabbing the inode mutex in
sysfs_permissionandsysfs_getattrcauses deadlocks, because somtimes those operations are called with the inode mutex held, but not always. Therefore we can not depend upon the inode mutex to serialize all updates to the vfs inode.
Quoted 7 lines
We take the
sysfs_mutexin all of those places so we can also use it to protect the vfs inode. To accomplish that we simply requires extending the vfs inode insysfs_setattroverinode_change_ok(so we have an unchanging inode when we perform the check), andinode_setattr.Signed-off-by Eric W. Biederman <[email protected]>
I'm a little confused about the patch intro: it makes it sound like this is preparatory for a followup, but in fact it is a bugfix, right? Hard to exploit i think, but should it go to -stable?
Quoted 40 lines
--- fs/sysfs/inode.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)fs/sysfs/inode.c +4
-4@@ -112,20 +112,20 @@ int sysfs_setattr(struct dentry *dentry, struct iattr *iattr)if (!sd)return -EINVAL ; +mutex_lock (&sysfs_mutex); error =inode_change_ok (inode, iattr);if (error) -return error; + goto out; iattr->ia_valid &= ~ATTR_SIZE ;/* ignore size changes */ error =inode_setattr (inode, iattr);if (error) -return error; + goto out; -mutex_lock (&sysfs_mutex); error =sysfs_sd_setattr (sd, iattr); +out:mutex_unlock (&sysfs_mutex); -return error; }Signature · 6 lines
1.6.5.2.143.g8cc62 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 2/7 sysfs: Pack sysfs_dirent more tightly.
In reply to Eric W. BiedermanFrom: Eric W. Biederman <[email protected]>
Placing the 16bit s_mode between a pointer and a long doesn't pack well especailly on 64bit where we wast 48 bits. So move s_mode and declare it as a unsigned short. This is the sysfs backing store after all we don't need fields extra large just in case someday we want userspace to be able to use a larger value.
--- fs/sysfs/sysfs.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
fs/sysfs/sysfs.h +1 -1
@@ -66,8 +66,8 @@ struct sysfs_dirent { };unsigned int s_flags; +unsigned short s_mode;ino_t s_ino; -umode_t s_mode;struct sysfs_inode_attrs *s_iattr; };
Signature · 1 line
1.6.5.2.143.g8cc62
Re: [PATCH 2/7] sysfs: Pack sysfs_dirent more tightly.
Quoted 9 lines · Eric W. Biederman
From: Eric W. Biederman <[email protected]>
Placing the 16bit
s_modebetween a pointer and a long doesn't pack well especailly on 64bit where we wast 48 bits. So moves_modeand declare it as a unsigned short. This is the sysfs backing store after all we don't need fields extra large just in case someday we want userspace to be able to use a larger value.Signed-off-by Eric W. Biederman <[email protected]>
Signature · 1 line
tejun
PATCH 3/7 sysfs: Keep an nlink count on sysfs directories.
In reply to Eric W. BiedermanFrom: Eric W. Biederman <[email protected]>
On large directories sysfs_count_nlinks can be a significant bottleneck, so keep a count in sysfs_dirent. If we exceed the maximum number of directory entries we can store return nlink of 1. An nlink of 1 matches what reiserfs does in this case, and it let's find and similar utlities know that we have a the directory nlink can not be used for optimization purposes.
--- fs/sysfs/dir.c | 20 ++++++++++++++++++++ fs/sysfs/inode.c | 15 +-------------- fs/sysfs/mount.c | 1 + fs/sysfs/sysfs.h | 1 + 4 files changed, 23 insertions(+), 14 deletions(-)
fs/sysfs/dir.c +20 -0
@@ -359,6 +359,7 @@ struct sysfs_dirent *sysfs_new_dirent(const char *name, umode_t mode, int type) sd->s_name = name; sd->s_mode = mode; sd->s_flags = type; + sd->s_nlink = type ==SYSFS_DIR ?2 :1 ;return sd;
@@ -392,6 +393,23 @@ void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,mutex_lock (&sysfs_mutex); } +static void sysfs_dir_inc_nlink (struct sysfs_dirent *sd) +{ +/* If we overflow force nlink to be 1 */ +if (sd->s_nlink >1 ) { + sd->s_nlink++; +if (sd->s_nlink ==0 ) + sd->s_nlink =1 ; + } +} + +static void sysfs_dir_dec_nlink (struct sysfs_dirent *sd) +{ +/* Don't decrement if we overflowed an increment */ +if (sd->s_nlink !=1 ) + sd->s_nlink--; +} +/** * __sysfs_add_one - add sysfs_dirent to parent without warning * @acxt: addrm context to use
@@ -420,6 +438,7 @@ int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)return -EEXIST ; sd->s_parent =sysfs_get (acxt->parent_sd); +sysfs_dir_inc_nlink (sd->s_parent);sysfs_link_sibling (sd);
@@ -512,6 +531,7 @@ void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)BUG_ON (sd->s_flags &SYSFS_FLAG_REMOVED ); +sysfs_dir_dec_nlink (sd->s_parent);sysfs_unlink_sibling (sd);/* Update timestamps on the parent */
fs/sysfs/inode.c +1 -14
@@ -201,18 +201,6 @@ static inline void set_inode_attr(struct inode * inode, struct iattr * iattr) inode->i_ctime = iattr->ia_ctime; } -static int sysfs_count_nlink (struct sysfs_dirent *sd) -{ -struct sysfs_dirent *child; -int nr =0 ; - -for (child = sd->s_dir.children; child; child = child->s_sibling) -if (sysfs_type (child) ==SYSFS_DIR ) - nr++; - -return nr +2 ; -} -static void sysfs_refresh_inode (struct sysfs_dirent *sd,struct inode *inode) {struct sysfs_inode_attrs *iattrs = sd->s_iattr;
@@ -228,8 +216,7 @@ static void sysfs_refresh_inode(struct sysfs_dirent *sd, struct inode *inode) iattrs->ia_secdata_len); } -if (sysfs_type (sd) ==SYSFS_DIR ) - inode->i_nlink =sysfs_count_nlink (sd); + inode->i_nlink = sd->s_nlink; }int sysfs_getattr (struct vfsmount *mnt,struct dentry *dentry,struct kstat *stat)
fs/sysfs/mount.c +1 -0
@@ -37,6 +37,7 @@ struct sysfs_dirent sysfs_root = { .s_count = ATOMIC_INIT(1), .s_flags = SYSFS_DIR, .s_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO, + .s_nlink = 2, .s_ino = 1, };
fs/sysfs/sysfs.h +1 -0
@@ -67,6 +67,7 @@ struct sysfs_dirent {unsigned int s_flags;unsigned short s_mode; +unsigned short s_nlink;ino_t s_ino;struct sysfs_inode_attrs *s_iattr; };
Signature · 1 line
1.6.5.2.143.g8cc62
Re: [PATCH 3/7] sysfs: Keep an nlink count on sysfs directories.
Hello,
Quoted 2 lines · Eric W. Biederman
On large directories
sysfs_count_nlinkscan be a significant bottleneck, so keep a count insysfs_dirent.
I was about to suggest changing s_flags to ushort too. Hmmm... adding a new field to sysfs_dirent somewhat worries me but this doesn't add to the size of the structure. How significant bottlenect are we talking about?
Quoted 4 lines
If we exceed the maximum number of directory entries we can store return nlink of 1. An nlink of 1 matches what reiserfs does in this case, and it let's find and similar utlities know that we have a the directory nlink can not be used for optimization purposes.
Hmmm... what's the limit on reiserfs? Is it 64k too?
Thanks.
Signature · 1 line
tejun
Re: [PATCH 3/7] sysfs: Keep an nlink count on sysfs directories.
Quoted 10 lines · Tejun Heo
Hello,
Quoted 2 lines · Eric W. Biederman
On large directories
sysfs_count_nlinkscan be a significant bottleneck, so keep a count insysfs_dirent.I was about to suggest changing
s_flagsto ushort too. Hmmm... adding a new field tosysfs_direntsomewhat worries me but this doesn't add to the size of the structure. How significant bottlenect are we talking about?
It was seen in measurements of sysfs before my last round of changes, which cause us to refresh the inode, and call sysfs_count_nlink more often.
I am surprised no one has complained about 2.6.33-rcN yet and reported a performance regression.
Ultimately not having a cached nlink count transforms what should be constant time operations to operations that run in time O(N).
Quoted 6 lines
Quoted 4 lines
If we exceed the maximum number of directory entries we can store return nlink of 1. An nlink of 1 matches what reiserfs does in this case, and it let's find and similar utlities know that we have a the directory nlink can not be used for optimization purposes.
Hmmm... what's the limit on reiserfs? Is it 64k too?
The resierfs limit is a bit short of a 32bit number. Ext[234] all have a 16bit nlink field, and they fail the operation when you attempt to increment nlink past their limit.
In this case the comparison with reiserfs is to show that at some point throwing up our hands and not counting and just returning nlink 1 is something userspace can occassionally expect to see. It is common enough that find has handled this idiom for years.
Since we can handle this without increasing the size of the sysfs_dirent I figure we should have a good quality of implementation for the common case and return something userspace can deal with for the extreme cases.
Eric
Re: [PATCH 3/7] sysfs: Keep an nlink count on sysfs directories.
In reply to Tejun HeoQuoted 10 lines · Tejun Heo
Hello,
Quoted 2 lines · Eric W. Biederman
On large directories
sysfs_count_nlinkscan be a significant bottleneck, so keep a count insysfs_dirent.I was about to suggest changing
s_flagsto ushort too. Hmmm... adding a new field tosysfs_direntsomewhat worries me but this doesn't add to the size of the structure. How significant bottlenect are we talking about?
100,000 entries in a sysfs directory is a requirement for network devices.
Quoted 6 lines
Quoted 4 lines
If we exceed the maximum number of directory entries we can store return nlink of 1. An nlink of 1 matches what reiserfs does in this case, and it let's find and similar utlities know that we have a the directory nlink can not be used for optimization purposes.
Hmmm... what's the limit on reiserfs? Is it 64k too?
64k is too small. 10 gig interfaces can currently service 50-100k users, each of which requires their own network device.
-ben
Re: [PATCH 3/7] sysfs: Keep an nlink count on sysfs directories.
Quoted 7 lines · Benjamin LaHaise
Quoted 6 lines · Tejun Heo
Hello,
Quoted 2 lines · Eric W. Biederman
On large directories
sysfs_count_nlinkscan be a significant bottleneck, so keep a count insysfs_dirent.
Quoted 6 lines
Quoted 4 lines
I was about to suggest changing
s_flagsto ushort too. Hmmm... adding a new field tosysfs_direntsomewhat worries me but this doesn't add to the size of the structure. How significant bottlenect are we talking about?100,000 entries in a sysfs directory is a requirement for network devices.
I haven't limited that.
Quoted 9 lines
Quoted 6 lines
Quoted 4 lines
If we exceed the maximum number of directory entries we can store return nlink of 1. An nlink of 1 matches what reiserfs does in this case, and it let's find and similar utlities know that we have a the directory nlink can not be used for optimization purposes.
Hmmm... what's the limit on reiserfs? Is it 64k too?
64k is too small. 10 gig interfaces can currently service 50-100k users, each of which requires their own network device.
64k is the point at which I stop returning a userful nlink number to userspace, sysfs directories can still scale to any size.
I could legitimately always return nlink == 1, and all userspace utilities that I know of would be fine, but I am trying to be a bit more polite than that.
I don't see the link count as interesting enough to store more than 16bits for it. Even with 32bits of storage for nlink sysfs would have to have to handle the rollover case as I am doing now. So I don't see any advantage to storing more bits.
Eric
Re: [PATCH 3/7] sysfs: Keep an nlink count on sysfs directories.
Quoted 4 lines · Eric W. Biederman
I don't see the link count as interesting enough to store more than 16bits for it. Even with 32bits of storage for nlink sysfs would have to have to handle the rollover case as I am doing now. So I don't see any advantage to storing more bits.
I'm not terribly concerned with what value gets returned, but rather about how long is spent calculating it. Ideally, new sysfs directory entries can be inserted in an O(1) or other reasonable order operation. I'll try to find some time to re-run my interface scaling tests with your latest changes.
=ben
Re: [PATCH 3/7] sysfs: Keep an nlink count on sysfs directories.
Quoted 11 lines · Benjamin LaHaise
Quoted 4 lines · Eric W. Biederman
I don't see the link count as interesting enough to store more than 16bits for it. Even with 32bits of storage for nlink sysfs would have to have to handle the rollover case as I am doing now. So I don't see any advantage to storing more bits.
I'm not terribly concerned with what value gets returned, but rather about how long is spent calculating it. Ideally, new sysfs directory entries can be inserted in an O(1) or other reasonable order operation. I'll try to find some time to re-run my interface scaling tests with your latest changes.
Sounds good. My changes so far have been the easy low hanging fruit.
readdir should be at O(N) and stat etc should all be constant time. Lookups by name are still O(N), and I'm not ready to look at going better until after I have network namespace support into sysfs.
Eric
Re: [PATCH 3/7] sysfs: Keep an nlink count on sysfs directories.
In reply to Eric W. BiedermanHello,
Quoted 9 lines · Eric W. Biederman
Quoted 2 lines
64k is too small. 10 gig interfaces can currently service 50-100k users, each of which requires their own network device.
64k is the point at which I stop returning a userful nlink number to userspace, sysfs directories can still scale to any size.
I could legitimately always return nlink == 1, and all userspace utilities that I know of would be fine, but I am trying to be a bit more polite than that.
I don't think it's more polite. It's more error-prone. 64k is a limit where breach would be rare enough to be unexpected but would still happen on corner cases. I don't know what would be the end result of incorrect nlink but it's far better to have something which behaves consistently than to have something which flips its meaning after reaching certain rare but still quite possible limit. If nlink is something we can ignore, just set it to 1 for all sysfs files. If nlink must be kept correct, use 32bit limit and fail to create more.
Quoted 4 lines
I don't see the link count as interesting enough to store more than 16bits for it. Even with 32bits of storage for nlink sysfs would have to have to handle the rollover case as I am doing now. So I don't see any advantage to storing more bits.
I really don't think reiserfs's example is a good one to follow especially with significantly lowered limit.
Thanks.
Signature · 1 line
tejun
Re: [PATCH 3/7] sysfs: Keep an nlink count on sysfs directories.
In reply to Benjamin LaHaiseQuoted 1 line · Benjamin LaHaise
64k is too small. 10 gig interfaces can currently service 50-100k users,
I'll certainly buy that.
Quoted 1 line
each of which requires their own network device.
You throwing each user into their own separate network namespace?
Re: [PATCH 3/7] sysfs: Keep an nlink count on sysfs directories.
In reply to Eric W. BiedermanQuoted 33 lines · Eric W. Biederman
From: Eric W. Biederman <[email protected]>
On large directories
sysfs_count_nlinkscan be a significant bottleneck, so keep a count insysfs_dirent. If we exceed the maximum number of directory entries we can store return nlink of 1. An nlink of 1 matches what reiserfs does in this case, and it let's find and similar utlities know that we have a the directory nlink can not be used for optimization purposes.Signed-off-by Eric W. Biederman <[email protected]>--- fs/sysfs/dir.c | 20 ++++++++++++++++++++ fs/sysfs/inode.c | 15 +-------------- fs/sysfs/mount.c | 1 + fs/sysfs/sysfs.h | 1 + 4 files changed, 23 insertions(+), 14 deletions(-)fs/sysfs/dir.c +1
-0@@ -359,6 +359,7 @@ struct sysfs_dirent *sysfs_new_dirent(const char *name, umode_t mode, int type) sd->s_name = name; sd->s_mode = mode; sd->s_flags = type; + sd->s_nlink = type ==SYSFS_DIR ?2 :1 ;return sd;@@ -392,6 +393,23 @@ void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,mutex_lock (&sysfs_mutex); }
fs/sysfs/dir.c void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt, · line 404
Quoted 10 lines
fs/sysfs/dir.c +9
-0+static void sysfs_dir_inc_nlink (struct sysfs_dirent *sd) +{ +/* If we overflow force nlink to be 1 */ +if (sd->s_nlink >1 ) { + sd->s_nlink++; +if (sd->s_nlink ==0 ) + sd->s_nlink =1 ; + } +}
Now, the original code only ups nlink on parent if child is a dir. IIUC your code will now inc nlink for files as well. Is any userspace going to be confused by that?
Quoted 86 lines
fs/sysfs/dir.c +10
-0+ +static void sysfs_dir_dec_nlink (struct sysfs_dirent *sd) +{ +/* Don't decrement if we overflowed an increment */ +if (sd->s_nlink !=1 ) + sd->s_nlink--; +} +/** * __sysfs_add_one - add sysfs_dirent to parent without warning * @acxt: addrm context to use @@ -420,6 +438,7 @@ int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)return -EEXIST ; sd->s_parent =sysfs_get (acxt->parent_sd); +sysfs_dir_inc_nlink (sd->s_parent);sysfs_link_sibling (sd);@@ -512,6 +531,7 @@ void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)BUG_ON (sd->s_flags &SYSFS_FLAG_REMOVED ); +sysfs_dir_dec_nlink (sd->s_parent);sysfs_unlink_sibling (sd);/* Update timestamps on the parent */ fs/sysfs/inode.c +1
-14@@ -201,18 +201,6 @@ static inline void set_inode_attr(struct inode * inode, struct iattr * iattr) inode->i_ctime = iattr->ia_ctime; } -static int sysfs_count_nlink (struct sysfs_dirent *sd) -{ -struct sysfs_dirent *child; -int nr =0 ; - -for (child = sd->s_dir.children; child; child = child->s_sibling) -if (sysfs_type (child) ==SYSFS_DIR ) - nr++; - -return nr +2 ; -} -static void sysfs_refresh_inode (struct sysfs_dirent *sd,struct inode *inode) {struct sysfs_inode_attrs *iattrs = sd->s_iattr;@@ -228,8 +216,7 @@ static void sysfs_refresh_inode(struct sysfs_dirent *sd, struct inode *inode) iattrs->ia_secdata_len); } -if (sysfs_type (sd) ==SYSFS_DIR ) - inode->i_nlink =sysfs_count_nlink (sd); + inode->i_nlink = sd->s_nlink; }int sysfs_getattr (struct vfsmount *mnt,struct dentry *dentry,struct kstat *stat)fs/sysfs/mount.c +1
-0@@ -37,6 +37,7 @@ struct sysfs_dirent sysfs_root = { .s_count = ATOMIC_INIT(1), .s_flags = SYSFS_DIR, .s_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO, + .s_nlink = 2, .s_ino = 1, };fs/sysfs/sysfs.h +1
-0@@ -67,6 +67,7 @@ struct sysfs_dirent {unsigned int s_flags;unsigned short s_mode; +unsigned short s_nlink;ino_t s_ino;struct sysfs_inode_attrs *s_iattr; };Signature · 1 line
1.6.5.2.143.g8cc62
Re: [PATCH 3/7] sysfs: Keep an nlink count on sysfs directories.
Quoted 35 lines · Serge E. Hallyn
Quoted 34 lines · Eric W. Biederman
From: Eric W. Biederman <[email protected]>
On large directories
sysfs_count_nlinkscan be a significant bottleneck, so keep a count insysfs_dirent. If we exceed the maximum number of directory entries we can store return nlink of 1. An nlink of 1 matches what reiserfs does in this case, and it let's find and similar utlities know that we have a the directory nlink can not be used for optimization purposes.Signed-off-by Eric W. Biederman <[email protected]>--- fs/sysfs/dir.c | 20 ++++++++++++++++++++ fs/sysfs/inode.c | 15 +-------------- fs/sysfs/mount.c | 1 + fs/sysfs/sysfs.h | 1 + 4 files changed, 23 insertions(+), 14 deletions(-)fs/sysfs/dir.c +1
-0@@ -359,6 +359,7 @@ struct sysfs_dirent *sysfs_new_dirent(const char *name, umode_t mode, int type) sd->s_name = name; sd->s_mode = mode; sd->s_flags = type; + sd->s_nlink = type ==SYSFS_DIR ?2 :1 ;return sd;@@ -392,6 +393,23 @@ void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,mutex_lock (&sysfs_mutex); }
Quoted 13 lines
fs/sysfs/dir.c void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt, · line 404
Quoted 9 lines
fs/sysfs/dir.c +9
-0+static void sysfs_dir_inc_nlink (struct sysfs_dirent *sd) +{ +/* If we overflow force nlink to be 1 */ +if (sd->s_nlink >1 ) { + sd->s_nlink++; +if (sd->s_nlink ==0 ) + sd->s_nlink =1 ; + } +}Now, the original code only ups nlink on parent if child is a dir. IIUC your code will now inc nlink for files as well. Is any userspace going to be confused by that?
Good point. nlink should only be upped for directories.
Eric
Re: [PATCH 3/7] sysfs: Keep an nlink count on sysfs directories.
In reply to Eric W. BiedermanQuoted 28 lines · Eric W. Biederman
From: Eric W. Biederman <[email protected]>
On large directories
sysfs_count_nlinkscan be a significant bottleneck, so keep a count insysfs_dirent. If we exceed the maximum number of directory entries we can store return nlink of 1. An nlink of 1 matches what reiserfs does in this case, and it let's find and similar utlities know that we have a the directory nlink can not be used for optimization purposes.Signed-off-by Eric W. Biederman <[email protected]>--- fs/sysfs/dir.c | 20 ++++++++++++++++++++ fs/sysfs/inode.c | 15 +-------------- fs/sysfs/mount.c | 1 + fs/sysfs/sysfs.h | 1 + 4 files changed, 23 insertions(+), 14 deletions(-)fs/sysfs/dir.c +1
-0@@ -420,6 +438,7 @@ int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)return -EEXIST ; sd->s_parent =sysfs_get (acxt->parent_sd); +sysfs_dir_inc_nlink (sd->s_parent);sysfs_link_sibling (sd);
fs/sysfs/dir.c void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd) · line 537
Quoted 7 lines
fs/sysfs/dir.c +1
-0@@ -512,6 +531,7 @@ void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)BUG_ON (sd->s_flags &SYSFS_FLAG_REMOVED ); +sysfs_dir_dec_nlink (sd->s_parent);sysfs_unlink_sibling (sd);/* Update timestamps on the parent */
Shouldn't s_nlink be adjusted in sysfs_rename for the move case as well?
Re: [PATCH 3/7] sysfs: Keep an nlink count on sysfs directories.
Quoted 32 lines · Cornelia Huck
Quoted 29 lines · Eric W. Biederman
From: Eric W. Biederman <[email protected]>
On large directories
sysfs_count_nlinkscan be a significant bottleneck, so keep a count insysfs_dirent. If we exceed the maximum number of directory entries we can store return nlink of 1. An nlink of 1 matches what reiserfs does in this case, and it let's find and similar utlities know that we have a the directory nlink can not be used for optimization purposes.Signed-off-by Eric W. Biederman <[email protected]>--- fs/sysfs/dir.c | 20 ++++++++++++++++++++ fs/sysfs/inode.c | 15 +-------------- fs/sysfs/mount.c | 1 + fs/sysfs/sysfs.h | 1 + 4 files changed, 23 insertions(+), 14 deletions(-)fs/sysfs/dir.c +1
-0@@ -420,6 +438,7 @@ int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)return -EEXIST ; sd->s_parent =sysfs_get (acxt->parent_sd); +sysfs_dir_inc_nlink (sd->s_parent);sysfs_link_sibling (sd);@@ -512,6 +531,7 @@ void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd)
Quoted 8 lines
fs/sysfs/dir.c void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd) · line 537
Quoted 6 lines
fs/sysfs/dir.c +1
-0BUG_ON (sd->s_flags &SYSFS_FLAG_REMOVED ); +sysfs_dir_dec_nlink (sd->s_parent);sysfs_unlink_sibling (sd);/* Update timestamps on the parent */ Shouldn't
s_nlinkbe adjusted insysfs_renamefor the move case as well?
Yes. I hereby nominate this as my most buggy patch ever. Ugh.
I thought I actually cared about nlinks when I bothered to keep a nlink count, but I am starting to think I really don't.
Eric
PATCH 4/7 sysfs: Implement sysfs_rename_link
In reply to Eric W. BiedermanFrom: Eric W. Biederman <[email protected]>
Because of rename ordering problems we occassionally give false warnings about invalid sysfs operations. So using sysfs_rename create a sysfs_rename_link function that doesn't need strange workarounds.
--- fs/sysfs/symlink.c | 38 ++++++++++++++++++++++++++++++++++++++ include/linux/sysfs.h | 9 +++++++++ 2 files changed, 47 insertions(+), 0 deletions(-)
fs/sysfs/symlink.c +38 -0
@@ -123,6 +123,44 @@ void sysfs_remove_link(struct kobject * kobj, const char * name)sysfs_hash_and_remove (parent_sd, name); } +/** +* sysfs_rename_link - rename symlink in object's directory. +* @kobj: object we're acting for. +* @targ: object we're pointing to. +* @old: previous name of the symlink. +* @new: new name of the symlink. +* +* A helper function for the common rename symlink idiom. +*/ +int sysfs_rename_link (struct kobject *kobj,struct kobject *targ, +const char *old,const char *new) +{ +struct sysfs_dirent *parent_sd, *sd =NULL ; +int result; + +if (!kobj) + parent_sd = &sysfs_root; +else + parent_sd = kobj->sd; + + result = -ENOENT ; + sd =sysfs_get_dirent (parent_sd, old); +if (!sd) + goto out; + + result = -EINVAL ; +if (sysfs_type (sd) !=SYSFS_KOBJ_LINK ) + goto out; +if (sd->s_symlink.target_sd->s_dir.kobj != targ) + goto out; + + result =sysfs_rename (sd, parent_sd, new); + +out: +sysfs_put (sd); +return result; +} +static int sysfs_get_target_path (struct sysfs_dirent *parent_sd,struct sysfs_dirent *target_sd,char *path) {
include/linux/sysfs.h +9 -0
@@ -110,6 +110,9 @@ int __must_check sysfs_create_link_nowarn(struct kobject *kobj, const char *name);void sysfs_remove_link (struct kobject *kobj,const char *name); +int sysfs_rename_link (struct kobject *kobj,struct kobject *target, +const char *old_name,const char *new_name); +int __must_check sysfs_create_group (struct kobject *kobj,const struct attribute_group *grp); int sysfs_update_group(struct kobject *kobj,
@@ -203,6 +206,12 @@ static inline void sysfs_remove_link(struct kobject *kobj, const char *name) { } +static inline int sysfs_rename_link (struct kobject *k,struct kobject *t, +const char *old_name,const char *new_name) +{ +return 0 ; +} +static inline int sysfs_create_group (struct kobject *kobj,const struct attribute_group *grp) {
Signature · 1 line
1.6.5.2.143.g8cc62
Re: [PATCH 4/7] sysfs: Implement sysfs_rename_link
Quoted 11 lines · Eric W. Biederman
From: Eric W. Biederman <[email protected]>
Because of rename ordering problems we occassionally give false warnings about invalid sysfs operations. So using
sysfs_renamecreate asysfs_rename_linkfunction that doesn't need strange workarounds.Cc Benjamin Thery <[email protected]>Cc Daniel Lezcano <[email protected]>Cc Tejun Heo <[email protected]>Signed-off-by Eric W. Biederman <[email protected]>
Thanks.
Signature · 1 line
tejun
Re: [PATCH 4/7] sysfs: Implement sysfs_rename_link
In reply to Eric W. BiedermanQuoted 6 lines · Eric W. Biederman
From: Eric W. Biederman <[email protected]>
Because of rename ordering problems we occassionally give false warnings about invalid sysfs operations. So using
sysfs_renamecreate asysfs_rename_linkfunction that doesn't need strange workarounds.
It looks nice and clean esp compared to the code you're replacing. OTOH I do wonder whether your sysfs_type(sd) != SYSFS_KOBJ_LINK check will catch unexpected ways that device_rename() was being called. Shouldn't, I realize, since sysfs_create_link() always sets SYSFS_KOBJ_LINK, but the check wasn't there before, so stranger things have happened.
Quoted 4 lines
Cc Benjamin Thery <[email protected]>Cc Daniel Lezcano <[email protected]>Cc Tejun Heo <[email protected]>Signed-off-by Eric W. Biederman <[email protected]>
Quoted 83 lines
--- fs/sysfs/symlink.c | 38 ++++++++++++++++++++++++++++++++++++++ include/linux/sysfs.h | 9 +++++++++ 2 files changed, 47 insertions(+), 0 deletions(-)fs/sysfs/symlink.c +38
-0@@ -123,6 +123,44 @@ void sysfs_remove_link(struct kobject * kobj, const char * name)sysfs_hash_and_remove (parent_sd, name); } +/** +* sysfs_rename_link - rename symlink in object's directory. +* @kobj: object we're acting for. +* @targ: object we're pointing to. +* @old: previous name of the symlink. +* @new: new name of the symlink. +* +* A helper function for the common rename symlink idiom. +*/ +int sysfs_rename_link (struct kobject *kobj,struct kobject *targ, +const char *old,const char *new) +{ +struct sysfs_dirent *parent_sd, *sd =NULL ; +int result; + +if (!kobj) + parent_sd = &sysfs_root; +else + parent_sd = kobj->sd; + + result = -ENOENT ; + sd =sysfs_get_dirent (parent_sd, old); +if (!sd) + goto out; + + result = -EINVAL ; +if (sysfs_type (sd) !=SYSFS_KOBJ_LINK ) + goto out; +if (sd->s_symlink.target_sd->s_dir.kobj != targ) + goto out; + + result =sysfs_rename (sd, parent_sd, new); + +out: +sysfs_put (sd); +return result; +} +static int sysfs_get_target_path (struct sysfs_dirent *parent_sd,struct sysfs_dirent *target_sd,char *path) {include/linux/sysfs.h +9
-0@@ -110,6 +110,9 @@ int __must_check sysfs_create_link_nowarn(struct kobject *kobj, const char *name);void sysfs_remove_link (struct kobject *kobj,const char *name); +int sysfs_rename_link (struct kobject *kobj,struct kobject *target, +const char *old_name,const char *new_name); +int __must_check sysfs_create_group (struct kobject *kobj,const struct attribute_group *grp); int sysfs_update_group(struct kobject *kobj,@@ -203,6 +206,12 @@ static inline void sysfs_remove_link(struct kobject *kobj, const char *name) { } +static inline int sysfs_rename_link (struct kobject *k,struct kobject *t, +const char *old_name,const char *new_name) +{ +return 0 ; +} +static inline int sysfs_create_group (struct kobject *kobj,const struct attribute_group *grp) {Signature · 1 line
1.6.5.2.143.g8cc62
PATCH 5/7 driver core: Use sysfs_rename_link in device_rename
In reply to Eric W. BiedermanFrom: Eric W. Biederman <[email protected]>
Don't open code the renaming of symlinks in sysfs instead use the new helper function sysfs_rename_link
--- drivers/base/core.c | 18 ++++++------------ 1 files changed, 6 insertions(+), 12 deletions(-)
drivers/base/core.c +6 -12
@@ -1574,22 +1574,16 @@ int device_rename(struct device *dev, char *new_name)if (old_class_name) { new_class_name =make_class_name (dev->class->name, &dev->kobj);if (new_class_name) { - error =sysfs_create_link_nowarn (&dev->parent->kobj, - &dev->kobj, - new_class_name); -if (error) - goto out; -sysfs_remove_link (&dev->parent->kobj, old_class_name); + error =sysfs_rename_link (&dev->parent->kobj, + &dev->kobj, + old_class_name, + new_class_name); } }#else if (dev->class) { - error =sysfs_create_link_nowarn (&dev->class->p->class_subsys.kobj, - &dev->kobj,dev_name (dev)); -if (error) - goto out; -sysfs_remove_link (&dev->class->p->class_subsys.kobj, - old_device_name); + error =sysfs_rename_link (&dev->class->p->class_subsys.kobj, + &dev->kobj, old_device_name, new_name); }#endif
Signature · 1 line
1.6.5.2.143.g8cc62
Re: [PATCH 5/7] driver core: Use sysfs_rename_link in device_rename
Quoted 6 lines · Eric W. Biederman
From: Eric W. Biederman <[email protected]>
Don't open code the renaming of symlinks in sysfs instead use the new helper function
sysfs_rename_linkSigned-off-by Eric W. Biederman <[email protected]>
Thanks.
Signature · 1 line
tejun
Re: [PATCH 5/7] driver core: Use sysfs_rename_link in device_rename
In reply to Eric W. BiedermanQuoted 6 lines · Eric W. Biederman
From: Eric W. Biederman <[email protected]>
Don't open code the renaming of symlinks in sysfs instead use the new helper function
sysfs_rename_linkSigned-off-by Eric W. Biederman <[email protected]>
Quoted 39 lines
--- drivers/base/core.c | 18 ++++++------------ 1 files changed, 6 insertions(+), 12 deletions(-)drivers/base/core.c +6
-12@@ -1574,22 +1574,16 @@ int device_rename(struct device *dev, char *new_name)if (old_class_name) { new_class_name =make_class_name (dev->class->name, &dev->kobj);if (new_class_name) { - error =sysfs_create_link_nowarn (&dev->parent->kobj, - &dev->kobj, - new_class_name); -if (error) - goto out; -sysfs_remove_link (&dev->parent->kobj, old_class_name); + error =sysfs_rename_link (&dev->parent->kobj, + &dev->kobj, + old_class_name, + new_class_name); } }#else if (dev->class) { - error =sysfs_create_link_nowarn (&dev->class->p->class_subsys.kobj, - &dev->kobj,dev_name (dev)); -if (error) - goto out; -sysfs_remove_link (&dev->class->p->class_subsys.kobj, - old_device_name); + error =sysfs_rename_link (&dev->class->p->class_subsys.kobj, + &dev->kobj, old_device_name, new_name); }#endif Signature · 1 line
1.6.5.2.143.g8cc62
PATCH 6/7 sysfs: Pass super_block to sysfs_get_inode
In reply to Eric W. BiedermanFrom: Eric W. Biederman <[email protected]>
Currently sysfs_get_inode magically returns an inode on sysfs_sb. Make the super_block parameter explicit and the code becomes clearer.
--- fs/sysfs/dir.c | 2 +- fs/sysfs/inode.c | 5 +++-- fs/sysfs/mount.c | 2 +- fs/sysfs/sysfs.h | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-)
fs/sysfs/dir.c +1 -1
@@ -701,7 +701,7 @@ static struct dentry * sysfs_lookup(struct inode *dir, struct dentry *dentry, }/* attach dentry and inode */ - inode =sysfs_get_inode (sd); + inode =sysfs_get_inode (dir->i_sb, sd);if (!inode) { ret =ERR_PTR (-ENOMEM ); goto out_unlock;
fs/sysfs/inode.c +3 -2
@@ -271,6 +271,7 @@ static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode)/** * sysfs_get_inode - get inode for sysfs_dirent +* @sb: super block * @sd: sysfs_dirent to allocate inode for * * Get inode for @sd. If such inode doesn't exist, a new inode
@@ -283,11 +284,11 @@ static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode)* RETURNS: * Pointer to allocated inode on success, NULL on failure. */ -struct inode *sysfs_get_inode (struct sysfs_dirent *sd) +struct inode *sysfs_get_inode (struct super_block *sb,struct sysfs_dirent *sd) {struct inode *inode; - inode =iget_locked (sysfs_sb, sd->s_ino); + inode =iget_locked (sb, sd->s_ino);if (inode && (inode->i_state &I_NEW ))sysfs_init_inode (sd, inode);
fs/sysfs/mount.c +1 -1
@@ -55,7 +55,7 @@ static int sysfs_fill_super(struct super_block *sb, void *data, int silent)/* get root inode, initialize and unlock it */ mutex_lock (&sysfs_mutex); - inode =sysfs_get_inode (&sysfs_root); + inode =sysfs_get_inode (sb, &sysfs_root);mutex_unlock (&sysfs_mutex);if (!inode) {pr_debug ("sysfs: could not get root inode\n" );
fs/sysfs/sysfs.h +1 -1
@@ -169,7 +169,7 @@ static inline void __sysfs_put(struct sysfs_dirent *sd)/* * inode.c */ -struct inode *sysfs_get_inode (struct sysfs_dirent *sd); +struct inode *sysfs_get_inode (struct super_block *sb,struct sysfs_dirent *sd);void sysfs_delete_inode (struct inode *inode);int sysfs_sd_setattr (struct sysfs_dirent *sd,struct iattr *iattr);int sysfs_permission (struct inode *inode,int mask);
Signature · 1 line
1.6.5.2.143.g8cc62
Re: [PATCH 6/7] sysfs: Pass super_block to sysfs_get_inode
Quoted 8 lines · Eric W. Biederman
From: Eric W. Biederman <[email protected]>
Currently
sysfs_get_inodemagically returns an inode onsysfs_sb. Make thesuper_blockparameter explicit and the code becomes clearer.Acked-by Tejun Heo <[email protected]>Signed-off-by Eric W. Biederman <[email protected]>
Quoted 74 lines
--- fs/sysfs/dir.c | 2 +- fs/sysfs/inode.c | 5 +++-- fs/sysfs/mount.c | 2 +- fs/sysfs/sysfs.h | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-)fs/sysfs/dir.c +1
-1@@ -701,7 +701,7 @@ static struct dentry * sysfs_lookup(struct inode *dir, struct dentry *dentry, }/* attach dentry and inode */ - inode =sysfs_get_inode (sd); + inode =sysfs_get_inode (dir->i_sb, sd);if (!inode) { ret =ERR_PTR (-ENOMEM ); goto out_unlock;fs/sysfs/inode.c +3
-2@@ -271,6 +271,7 @@ static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode)/** * sysfs_get_inode - get inode for sysfs_dirent +* @sb: super block * @sd: sysfs_dirent to allocate inode for * * Get inode for @sd. If such inode doesn't exist, a new inode @@ -283,11 +284,11 @@ static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode)* RETURNS: * Pointer to allocated inode on success, NULL on failure. */ -struct inode *sysfs_get_inode (struct sysfs_dirent *sd) +struct inode *sysfs_get_inode (struct super_block *sb,struct sysfs_dirent *sd) {struct inode *inode; - inode =iget_locked (sysfs_sb, sd->s_ino); + inode =iget_locked (sb, sd->s_ino);if (inode && (inode->i_state &I_NEW ))sysfs_init_inode (sd, inode);fs/sysfs/mount.c +1
-1@@ -55,7 +55,7 @@ static int sysfs_fill_super(struct super_block *sb, void *data, int silent)/* get root inode, initialize and unlock it */ mutex_lock (&sysfs_mutex); - inode =sysfs_get_inode (&sysfs_root); + inode =sysfs_get_inode (sb, &sysfs_root);mutex_unlock (&sysfs_mutex);if (!inode) {pr_debug ("sysfs: could not get root inode\n" );fs/sysfs/sysfs.h +1
-1@@ -169,7 +169,7 @@ static inline void __sysfs_put(struct sysfs_dirent *sd)/* * inode.c */ -struct inode *sysfs_get_inode (struct sysfs_dirent *sd); +struct inode *sysfs_get_inode (struct super_block *sb,struct sysfs_dirent *sd);void sysfs_delete_inode (struct inode *inode);int sysfs_sd_setattr (struct sysfs_dirent *sd,struct iattr *iattr);int sysfs_permission (struct inode *inode,int mask);Signature · 1 line
1.6.5.2.143.g8cc62
PATCH 7/7 sysfs: Kill unused sysfs_sb variable.
In reply to Eric W. BiedermanFrom: Eric W. Biederman <[email protected]>
Now that there are no more users we can remove
the sysfs_sb variable.
--- fs/sysfs/mount.c | 2 -- fs/sysfs/sysfs.h | 1 - 2 files changed, 0 insertions(+), 3 deletions(-)
fs/sysfs/mount.c +0 -2
@@ -23,7 +23,6 @@static struct vfsmount *sysfs_mount; -struct super_block * sysfs_sb =NULL ;struct kmem_cache *sysfs_dir_cachep;static const struct super_operations sysfs_ops = {
@@ -51,7 +50,6 @@ static int sysfs_fill_super(struct super_block *sb, void *data, int silent) sb->s_magic =SYSFS_MAGIC ; sb->s_op = &sysfs_ops; sb->s_time_gran =1 ; - sysfs_sb = sb;/* get root inode, initialize and unlock it */ mutex_lock (&sysfs_mutex);
fs/sysfs/sysfs.h +0 -1
@@ -112,7 +112,6 @@ struct sysfs_addrm_cxt {* mount.c */ extern struct sysfs_dirent sysfs_root; -extern struct super_block *sysfs_sb;extern struct kmem_cache *sysfs_dir_cachep;/*
Signature · 1 line
1.6.5.2.143.g8cc62
Re: [PATCH 7/7] sysfs: Kill unused sysfs_sb variable.
Quoted 7 lines · Eric W. Biederman
From: Eric W. Biederman <[email protected]>
Now that there are no more users we can remove
thesysfs_sbvariable.Acked-by Tejun Heo <[email protected]>Signed-off-by Eric W. Biederman <[email protected]>
Quoted 44 lines
--- fs/sysfs/mount.c | 2 -- fs/sysfs/sysfs.h | 1 - 2 files changed, 0 insertions(+), 3 deletions(-)fs/sysfs/mount.c +0
-2@@ -23,7 +23,6 @@static struct vfsmount *sysfs_mount; -struct super_block * sysfs_sb =NULL ;struct kmem_cache *sysfs_dir_cachep;static const struct super_operations sysfs_ops = {@@ -51,7 +50,6 @@ static int sysfs_fill_super(struct super_block *sb, void *data, int silent) sb->s_magic =SYSFS_MAGIC ; sb->s_op = &sysfs_ops; sb->s_time_gran =1 ; - sysfs_sb = sb;/* get root inode, initialize and unlock it */ mutex_lock (&sysfs_mutex);fs/sysfs/sysfs.h +0
-1@@ -112,7 +112,6 @@ struct sysfs_addrm_cxt {* mount.c */ extern struct sysfs_dirent sysfs_root; -extern struct super_block *sysfs_sb;extern struct kmem_cache *sysfs_dir_cachep;/* Signature · 6 lines
1.6.5.2.143.g8cc62 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 5 lines · Eric W. Biederman
This patchset is built on top of 2.6.33-rc3 +
"sysfs: Cache the lastsysfs_direntto improve readdir scalability v2"Greg I assume you have that patch somewhere in queue.
It is now applied.
Quoted 4 lines
This patchset contains little cleanups all around sysfs.
- Some basic cleanups.
- Some sysfs scaling work
- Implementation ofsysfs_rename
Care to resend these, as it seems that some needed to be redone.
thanks,
greg k-h
cover General sysfs enhancements
Quoted 7 lines · Greg KH
Quoted 5 lines · Eric W. Biederman
This patchset is built on top of 2.6.33-rc3 +
"sysfs: Cache the lastsysfs_direntto improve readdir scalability v2"Greg I assume you have that patch somewhere in queue.
Quoted 8 lines
It is now applied.
Quoted 4 lines
This patchset contains little cleanups all around sysfs.
- Some basic cleanups.
- Some sysfs scaling work
- Implementation ofsysfs_renameCare to resend these, as it seems that some needed to be redone.
Here goes. I am finally recovered enough from my cold to find the time and energy to do this properly.
For now I am dropping the controversial and buggy nlink changing patch from my patchset, I can come back to that another time.
Eric
PATCH 1/6 sysfs: Serialize updates to the vfs inode
From: Eric W. Biederman <[email protected]>
The vfs depends upon filesystem methods to update the
vfs inode. Sysfs adds to the normal number of places
where the vfs inode is updated by also updatng the vfs inode in sysfs_refresh_inode.
Typically the inode mutex is used to serialize updates to the vfs inode, but grabbing the inode mutex in sysfs_permission and sysfs_getattr causes deadlocks, because sometimes the vfs calls those operations with the inode mutex held. Therefore sysfs can not use the inode mutex to serial updates to the vfs inode.
The sysfs_mutex is acquired in all of the routines where sysfs updates the vfs inode, and with a small change we can consistently protext sysfs vfs inode updates with the sysfs_mutex. To protect the sysfs vfs inode updates with the sysfs_mutex simply requires extending the scope of sysfs_mutex in sysfs_setattr over inode_setattr, and over inode_change_ok (so we have an unchanging inode when we perform the check).
--- fs/sysfs/inode.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
fs/sysfs/inode.c +4 -4
@@ -111,20 +111,20 @@ int sysfs_setattr(struct dentry *dentry, struct iattr *iattr)if (!sd)return -EINVAL ; +mutex_lock (&sysfs_mutex); error =inode_change_ok (inode, iattr);if (error) -return error; + goto out; iattr->ia_valid &= ~ATTR_SIZE ;/* ignore size changes */ error =inode_setattr (inode, iattr);if (error) -return error; + goto out; -mutex_lock (&sysfs_mutex); error =sysfs_sd_setattr (sd, iattr); +out:mutex_unlock (&sysfs_mutex); -return error; }
Signature · 1 line
1.6.5.2.143.g8cc62
PATCH 2/6 sysfs: Pack sysfs_dirent more tightly.
In reply to Eric W. BiedermanFrom: Eric W. Biederman <[email protected]>
Placing the 16bit s_mode between a pointer and a long doesn't pack well especailly on 64bit where we wast 48 bits. So move s_mode and declare it as a unsigned short. This is the sysfs backing store after all we don't need fields extra large just in case someday we want userspace to be able to use a larger value.
--- fs/sysfs/sysfs.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
fs/sysfs/sysfs.h +1 -1
@@ -66,8 +66,8 @@ struct sysfs_dirent { };unsigned int s_flags; +unsigned short s_mode;ino_t s_ino; -umode_t s_mode;struct sysfs_inode_attrs *s_iattr; };
Signature · 1 line
1.6.5.2.143.g8cc62
PATCH 3/6 sysfs: Implement sysfs_rename_link
In reply to Eric W. BiedermanFrom: Eric W. Biederman <[email protected]>
Because of rename ordering problems we occassionally give false warnings about invalid sysfs operations. So using sysfs_rename create a sysfs_rename_link function that doesn't need strange workarounds.
--- fs/sysfs/symlink.c | 38 ++++++++++++++++++++++++++++++++++++++ include/linux/sysfs.h | 9 +++++++++ 2 files changed, 47 insertions(+), 0 deletions(-)
fs/sysfs/symlink.c +38 -0
@@ -123,6 +123,44 @@ void sysfs_remove_link(struct kobject * kobj, const char * name)sysfs_hash_and_remove (parent_sd, name); } +/** +* sysfs_rename_link - rename symlink in object's directory. +* @kobj: object we're acting for. +* @targ: object we're pointing to. +* @old: previous name of the symlink. +* @new: new name of the symlink. +* +* A helper function for the common rename symlink idiom. +*/ +int sysfs_rename_link (struct kobject *kobj,struct kobject *targ, +const char *old,const char *new) +{ +struct sysfs_dirent *parent_sd, *sd =NULL ; +int result; + +if (!kobj) + parent_sd = &sysfs_root; +else + parent_sd = kobj->sd; + + result = -ENOENT ; + sd =sysfs_get_dirent (parent_sd, old); +if (!sd) + goto out; + + result = -EINVAL ; +if (sysfs_type (sd) !=SYSFS_KOBJ_LINK ) + goto out; +if (sd->s_symlink.target_sd->s_dir.kobj != targ) + goto out; + + result =sysfs_rename (sd, parent_sd, new); + +out: +sysfs_put (sd); +return result; +} +static int sysfs_get_target_path (struct sysfs_dirent *parent_sd,struct sysfs_dirent *target_sd,char *path) {
include/linux/sysfs.h +9 -0
@@ -148,6 +148,9 @@ int __must_check sysfs_create_link_nowarn(struct kobject *kobj, const char *name);void sysfs_remove_link (struct kobject *kobj,const char *name); +int sysfs_rename_link (struct kobject *kobj,struct kobject *target, +const char *old_name,const char *new_name); +int __must_check sysfs_create_group (struct kobject *kobj,const struct attribute_group *grp); int sysfs_update_group(struct kobject *kobj,
@@ -241,6 +244,12 @@ static inline void sysfs_remove_link(struct kobject *kobj, const char *name) { } +static inline int sysfs_rename_link (struct kobject *k,struct kobject *t, +const char *old_name,const char *new_name) +{ +return 0 ; +} +static inline int sysfs_create_group (struct kobject *kobj,const struct attribute_group *grp) {
Signature · 1 line
1.6.5.2.143.g8cc62
PATCH 4/6 driver core: Use sysfs_rename_link in device_rename
In reply to Eric W. BiedermanFrom: Eric W. Biederman <[email protected]>
Don't open code the renaming of symlinks in sysfs instead use the new helper function sysfs_rename_link
--- drivers/base/core.c | 18 ++++++------------ 1 files changed, 6 insertions(+), 12 deletions(-)
drivers/base/core.c +6 -12
@@ -1574,22 +1574,16 @@ int device_rename(struct device *dev, char *new_name)if (old_class_name) { new_class_name =make_class_name (dev->class->name, &dev->kobj);if (new_class_name) { - error =sysfs_create_link_nowarn (&dev->parent->kobj, - &dev->kobj, - new_class_name); -if (error) - goto out; -sysfs_remove_link (&dev->parent->kobj, old_class_name); + error =sysfs_rename_link (&dev->parent->kobj, + &dev->kobj, + old_class_name, + new_class_name); } }#else if (dev->class) { - error =sysfs_create_link_nowarn (&dev->class->p->class_subsys.kobj, - &dev->kobj,dev_name (dev)); -if (error) - goto out; -sysfs_remove_link (&dev->class->p->class_subsys.kobj, - old_device_name); + error =sysfs_rename_link (&dev->class->p->class_subsys.kobj, + &dev->kobj, old_device_name, new_name); }#endif
Signature · 1 line
1.6.5.2.143.g8cc62
PATCH 5/6 sysfs: Pass super_block to sysfs_get_inode
In reply to Eric W. BiedermanFrom: Eric W. Biederman <[email protected]>
Currently sysfs_get_inode magically returns an inode on sysfs_sb. Make the super_block parameter explicit and the code becomes clearer.
--- fs/sysfs/dir.c | 2 +- fs/sysfs/inode.c | 5 +++-- fs/sysfs/mount.c | 2 +- fs/sysfs/sysfs.h | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-)
fs/sysfs/dir.c +1 -1
@@ -645,7 +645,7 @@ static struct dentry * sysfs_lookup(struct inode *dir, struct dentry *dentry, }/* attach dentry and inode */ - inode =sysfs_get_inode (sd); + inode =sysfs_get_inode (dir->i_sb, sd);if (!inode) { ret =ERR_PTR (-ENOMEM ); goto out_unlock;
fs/sysfs/inode.c +3 -2
@@ -283,6 +283,7 @@ static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode)/** * sysfs_get_inode - get inode for sysfs_dirent +* @sb: super block * @sd: sysfs_dirent to allocate inode for * * Get inode for @sd. If such inode doesn't exist, a new inode
@@ -295,11 +296,11 @@ static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode)* RETURNS: * Pointer to allocated inode on success, NULL on failure. */ -struct inode *sysfs_get_inode (struct sysfs_dirent *sd) +struct inode *sysfs_get_inode (struct super_block *sb,struct sysfs_dirent *sd) {struct inode *inode; - inode =iget_locked (sysfs_sb, sd->s_ino); + inode =iget_locked (sb, sd->s_ino);if (inode && (inode->i_state &I_NEW ))sysfs_init_inode (sd, inode);
fs/sysfs/mount.c +1 -1
@@ -54,7 +54,7 @@ static int sysfs_fill_super(struct super_block *sb, void *data, int silent)/* get root inode, initialize and unlock it */ mutex_lock (&sysfs_mutex); - inode =sysfs_get_inode (&sysfs_root); + inode =sysfs_get_inode (sb, &sysfs_root);mutex_unlock (&sysfs_mutex);if (!inode) {pr_debug ("sysfs: could not get root inode\n" );
fs/sysfs/sysfs.h +1 -1
@@ -172,7 +172,7 @@ static inline void __sysfs_put(struct sysfs_dirent *sd)/* * inode.c */ -struct inode *sysfs_get_inode (struct sysfs_dirent *sd); +struct inode *sysfs_get_inode (struct super_block *sb,struct sysfs_dirent *sd);void sysfs_delete_inode (struct inode *inode);int sysfs_sd_setattr (struct sysfs_dirent *sd,struct iattr *iattr);int sysfs_permission (struct inode *inode,int mask);
Signature · 1 line
1.6.5.2.143.g8cc62
PATCH 6/6 sysfs: Kill unused sysfs_sb variable.
In reply to Eric W. BiedermanFrom: Eric W. Biederman <[email protected]>
Now that there are no more users we can remove
the sysfs_sb variable.
--- fs/sysfs/mount.c | 2 -- fs/sysfs/sysfs.h | 1 - 2 files changed, 0 insertions(+), 3 deletions(-)
fs/sysfs/mount.c +0 -2
@@ -23,7 +23,6 @@static struct vfsmount *sysfs_mount; -struct super_block * sysfs_sb =NULL ;struct kmem_cache *sysfs_dir_cachep;static const struct super_operations sysfs_ops = {
@@ -50,7 +49,6 @@ static int sysfs_fill_super(struct super_block *sb, void *data, int silent) sb->s_magic =SYSFS_MAGIC ; sb->s_op = &sysfs_ops; sb->s_time_gran =1 ; - sysfs_sb = sb;/* get root inode, initialize and unlock it */ mutex_lock (&sysfs_mutex);
fs/sysfs/sysfs.h +0 -1
@@ -115,7 +115,6 @@ struct sysfs_addrm_cxt {* mount.c */ extern struct sysfs_dirent sysfs_root; -extern struct super_block *sysfs_sb;extern struct kmem_cache *sysfs_dir_cachep;/*
Signature · 1 line
1.6.5.2.143.g8cc62