sysfs regression: wrong link counts
Subsystems (8)
Hi,
I cannot boot properly with this commit:
commit 524b6c5b39b931311dfe5a2f5abae2f5c9731676
Author: Eric W. Biederman <[email protected]> Date: Sun Dec 18 20:09:31 2011 -0800
sysfs: Kill nlink counting.
1) network systemd rule doesn't start network
2) sensors complain:
sensors_init: Kernel interface error
ad 2) look at what it does:
/* returns !0 if sysfs filesystem was found, 0 otherwise */ int sensors_init_sysfs(void)
{
struct stat statbuf;
snprintf(sensors_sysfs_mount, NAME_MAX, "%s", "/sys");
if (stat(sensors_sysfs_mount, &statbuf) < 0
|| statbuf.st_nlink <= 2) /* Empty directory */
return 0;
return 1;
}So this looks like it became a part of ABI we cannot break...
A revert of this commit on the top of today's -next fixes the problem.
thanks,
Signature · 2 lines
js suse labs
Quoted 7 lines · Jiri Slaby
Hi,
I cannot boot properly with this commit:
commit524b6c5b39b931311dfe5a2f5abae2f5c9731676Author: Eric W. Biederman <[email protected]> Date: Sun Dec 18 20:09:31 2011 -0800
Quoted 6 lines
sysfs: Kill nlink counting.
1) network systemd rule doesn't start network
2) sensors complain:
sensors_init: Kernel interface error
Odd.
What in systemd is causing a reliance on this?
Quoted 13 lines
ad 2) look at what it does:
/* returns !0 if sysfs filesystem was found, 0 otherwise */ intsensors_init_sysfs(void){ struct stat statbuf; snprintf(sensors_sysfs_mount, NAME_MAX, "%s", "/sys"); if (stat(sensors_sysfs_mount, &statbuf) < 0 || statbuf.st_nlink <= 2) /* Empty directory */ return 0; return 1; }
Ah, a hack to see if the directory is empty.
Isn't there some other "proper" way of doing this in userspace, or is this really the correct way?
Quoted 3 lines
So this looks like it became a part of ABI we cannot break...
A revert of this commit on the top of today's -next fixes the problem.
Ick.
Eric, care to fix this up, or do you want me to revert it?
thanks,
greg k-h
Quoted 2 lines
Isn't there some other "proper" way of doing this in userspace, or is this really the correct way?
You can look at the S_IFMT bits and stuff however link count indicating number of subdirectories is a standard Unix thing and used by many quite mundane tools as an optimisation.
Alan
Quoted 6 lines · Alan Cox
Quoted 2 lines
Isn't there some other "proper" way of doing this in userspace, or is this really the correct way?
You can look at the
S_IFMTbits and stuff however link count indicating number of subdirectories is a standard Unix thing and used by many quite mundane tools as an optimisation.
Ah, yeah, that is easier.
Eric, care to fix this or want me to revert it?
thanks,
greg k-h
Al Viro
maintainerQuoted 11 lines · Greg KH
Quoted 6 lines · Alan Cox
Quoted 2 lines
Isn't there some other "proper" way of doing this in userspace, or is this really the correct way?
You can look at the
S_IFMTbits and stuff however link count indicating number of subdirectories is a standard Unix thing and used by many quite mundane tools as an optimisation.Ah, yeah, that is easier.
Eric, care to fix this or want me to revert it?
Fix _what_? Userland shite quoted upthread? Because that's where the bug is - the mundane tools mentioned by Alan treat 1 in st_nlink as "no information about the number of subdirectories". And shite might be too mild a term for the little gem in question, really...
Al Viro
maintainerQuoted 9 lines · Al Viro
Quoted 8 lines · Greg KH
Quoted 6 lines · Alan Cox
Quoted 2 lines
Isn't there some other "proper" way of doing this in userspace, or is this really the correct way?
You can look at the
S_IFMTbits and stuff however link count indicating number of subdirectories is a standard Unix thing and used by many quite mundane tools as an optimisation.
Quoted 8 lines
Quoted 3 lines
Ah, yeah, that is easier.
Eric, care to fix this or want me to revert it?
Fix _what_? Userland shite quoted upthread? Because that's where the bug is - the mundane tools mentioned by Alan treat 1 in
st_nlinkas "no information about the number of subdirectories". And shite might be too mild a term for the little gem in question, really...
To repeat this piece of bogosity for those who might've missed it:
/* returns !0 if sysfs filesystem was found, 0 otherwise */ int sensors_init_sysfs(void)
{
struct stat statbuf;
snprintf(sensors_sysfs_mount, NAME_MAX, "%s", "/sys");
if (stat(sensors_sysfs_mount, &statbuf) < 0
|| statbuf.st_nlink <= 2) /* Empty directory */
return 0;
return 1;
}which is completely bogus - contrary to what it says in comments, it does *not* check anything about sysfs (or directories being empty). Checking that sysfs is mounted on /sys could be done by statfs(2) and checking ->f_type, or, considering what the code in lib/sysfs.c is doing, just checking that /sys/class is readable and failing otherwise.
Sigh...
Quoted 11 lines · Greg KH
Quoted 6 lines · Alan Cox
Quoted 2 lines
Isn't there some other "proper" way of doing this in userspace, or is this really the correct way?
You can look at the
S_IFMTbits and stuff however link count indicating number of subdirectories is a standard Unix thing and used by many quite mundane tools as an optimisation.Ah, yeah, that is easier.
Eric, care to fix this or want me to revert it?
With respect to sensors the conversation has already been had, and I fix is already queued and should come out in the sensors release due out in a week or so. So sensors should be fixed before this code is merged into Linus's kernel.
Now why something minor like sensors that nothing seems to depend strongly on should stop a system from booting is a huge question.
So I am going to have the conversation to triage what is up with systemd. That seems totally unexpected.
Eric
Quoted 10 lines · Eric W. Biederman
Quoted 8 lines · Greg KH
Quoted 6 lines · Alan Cox
Quoted 2 lines
Isn't there some other "proper" way of doing this in userspace, or is this really the correct way?
You can look at the
S_IFMTbits and stuff however link count indicating number of subdirectories is a standard Unix thing and used by many quite mundane tools as an optimisation.
Quoted 8 lines
Quoted 3 lines
Ah, yeah, that is easier.
Eric, care to fix this or want me to revert it?
With respect to sensors the conversation has already been had, and I fix is already queued and should come out in the sensors release due out in a week or so. So sensors should be fixed before this code is merged into Linus's kernel.
Oh, we are not going to break userspace with 3.3, are we? I understand that what sensors do is nothing but sh*t. But this change should wait until everybody has a chance to have fixed sensors package in their distribution.
To be clear, what I'm suggesting is to postpone the change and schedule it for something like 3.7.
Quoted 2 lines
Now why something minor like sensors that nothing seems to depend strongly on should stop a system from booting is a huge question.
sensors do not prevent boot at all, of course. The other cannot-start-network bug does.
Quoted 2 lines
So I am going to have the conversation to triage what is up with systemd. That seems totally unexpected.
Looks like netdev renaming problem, see my other mail.
thanks,
Signature · 3 lines
js suse labs .
Quoted 1 line · Jiri Slaby
Oh, we are not going to break userspace with 3.3, are we?
No. This code is currently scheduled for 3.4.
Quoted 7 lines
I understand
that what sensors do is nothing but sh*t. But this change should wait until everybody has a chance to have fixed sensors package in their distribution.To be clear, what I'm suggesting is to postpone the change and schedule it for something like 3.7.
The sensors update with the fix is scheduled for about a week out, well before 3.3 ships.
The code in next for 3.4 is perhaps 4-6 months out from a kernel release. That seems like plenty of time for distros to get an update package for sensors together.
Eric
Quoted 3 lines · Eric W. Biederman
The sensors update with the fix is scheduled for about a week out, well before 3.3 ships.
That's almost certainly *not* going to help.
Guys, people don't update their user space. Some people run modern kernels on the enterprise distros - and those can be *years* out of date.
If people report problems, we revert things. It's that simple. It really doesn't matter if you call it a user space bug or not - if user space depends on what we used to do, then we continue to do it.
We can *try* the kernel change, but I can already tell you that anybody who thinks that "sensors will be fixed by all users by the time the kernel comes out" is likely totally full of shit. Not to mention that it apparently *already* causes problems for people who want to test -next, and this breaks those peoples setup, and thus means that -next gets less testing.
Really. "It's a bug in user space" is *NOT* an excuse for breaking things. Never was. Never is. There are (other) excuses for breaking things, but "user space did something I didn't expect it to do, and I consider it a bug" is absolutely not one of them.
Linus
Al Viro
maintainerQuoted 11 lines · Linus Torvalds
We can *try* the kernel change, but I can already tell you that anybody who thinks that "sensors will be fixed by all users by the time the kernel comes out" is likely totally full of shit. Not to mention that it apparently *already* causes problems for people who want to test -next, and this breaks those peoples setup, and thus means that -next gets less testing.
Really. "It's a bug in user space" is *NOT* an excuse for breaking things. Never was. Never is. There are (other) excuses for breaking things, but "user space did something I didn't expect it to do, and I consider it a bug" is absolutely not one of them.
It's actually "user space did something utterly weird that happened not to break with the current behaviour by accident", and yes, it's better to revert that change for the reasons you've described. But it's definitely a userland bug - as in "code doesn't do what it intends to do" and no matter what we do kernel-side it ought to be fixed in lm-sensors. Several years later it hopefully will become a non-issue (and we'll need to document the conflict with lm-sensors earlier than $FIXED_VERSION in Documentation/Changes when that sysfs patch finally goes in).
Quoted 16 lines · Linus Torvalds
Quoted 3 lines · Eric W. Biederman
The sensors update with the fix is scheduled for about a week out, well before 3.3 ships.
That's almost certainly *not* going to help.
Guys, people don't update their user space. Some people run modern kernels on the enterprise distros - and those can be *years* out of date.
If people report problems, we revert things. It's that simple. It really doesn't matter if you call it a user space bug or not - if user space depends on what we used to do, then we continue to do it.
Quoted 6 lines
We can *try* the kernel change, but I can already tell you that anybody who thinks that "sensors will be fixed by all users by the time the kernel comes out" is likely totally full of shit. Not to mention that it apparently *already* causes problems for people who want to test -next, and this breaks those peoples setup, and thus means that -next gets less testing.
By and large I am in agreement with you, and this patch was designed to be very focused in case we ran into userspace that had problems to make it easy to revert. The first question I asked when this was reported is how badly do things break. So I could tell if things needed to be fixed.
My current understanding is that it is exactly sensors that breaks. One program that complains loudly and doesn't function.
To the best of my knowledge nothing else fails, or depends on sensors.
I would like to have the code in -next a little longer to see if anything else breaks.
Quoted 4 lines
Really. "It's a bug in user space" is *NOT* an excuse for breaking things. Never was. Never is. There are (other) excuses for breaking things, but "user space did something I didn't expect it to do, and I consider it a bug" is absolutely not one of them.
Part of the reason for the patch is it is a partial regression fix for sysfs using more memory in 3.3 and 3.2 than in previous kernels. struct sysfs_dirent is nowhere near as critical as struct page but the same kind of small in structure size add up.
Thinking about it I should be able to whip up a tiny patch that adds a Kconfig option. Support for programs that are don't interpret nlinks correctly.
That should allow people who want the size reduction to have a smaller sysfs and it should allow people who are conservative to guarantee that the code works, and it should let me find out if anything else out there actually cares.
Eric
PATCH 1/1 sysfs: Optionally count subdirectories to support buggy applications
lm_sensors and possibly other applications get confused if all sysfs directories return nlink == 1. The lm_sensors code that got confused was just wrong and a fixed version of lm_sensors should be released shortly.
There may be other applications that have problems with sysfs return nlink == 1 for directories. To allow people to continue to use old versions of userspace with new kernels add to sysfs a compile time option to maintain mostly precise directory counts for those people who don't mind the cost.
I have moved where we keep nlink in sysfs_dirent as compared to previous versions of subdirectory counting to a location that packs better.
--- fs/sysfs/Kconfig | 15 +++++++++++++++ fs/sysfs/dir.c | 8 ++++++++ fs/sysfs/inode.c | 2 ++ fs/sysfs/sysfs.h | 38 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 0 deletions(-)
fs/sysfs/Kconfig +15 -0
@@ -21,3 +21,18 @@ config SYSFS example, "root=03:01" for /dev/hda1. Designers of embedded systems may wish to say N here to conserve space. + +config SYSFS_COUNT_LINKS + bool "sysfs count subdirectoires to support buggy applications" + default n + help + + Increase the memory and cpu untilization of sysfs by maintaining + by maintaining a count of how hard links from subdirectories a + directory has. This allows sysfs to report the directory nlink + as the number of subdirectories plus two. With out this enabled + sysfs will report the nlink of all directories as 1, which is + the standard way of indicating that the number of subdirectoires + is not tracked. + + Unless you know you need this say N here.
fs/sysfs/dir.c +8 -0
@@ -91,6 +91,9 @@ static int sysfs_link_sibling(struct sysfs_dirent *sd)struct rb_node **node = &sd->s_parent->s_dir.children.rb_node;struct rb_node *parent =NULL ; +if (sysfs_type (sd) ==SYSFS_DIR ) +sysfs_inc_nlink (sd->s_parent); +while (*node) {struct sysfs_dirent *pos;int result;
@@ -123,6 +126,9 @@ static int sysfs_link_sibling(struct sysfs_dirent *sd)*/ static void sysfs_unlink_sibling (struct sysfs_dirent *sd) { +if (sysfs_type (sd) ==SYSFS_DIR ) +sysfs_dec_nlink (sd->s_parent); +rb_erase (&sd->s_rb, &sd->s_parent->s_dir.children); }
@@ -367,6 +373,8 @@ struct sysfs_dirent *sysfs_new_dirent(const char *name, umode_t mode, int type) sd->s_mode = mode; sd->s_flags = type; +sysfs_init_nlink (sd); +return sd; err_out2:
fs/sysfs/inode.c +2 -0
@@ -216,6 +216,8 @@ static void sysfs_refresh_inode(struct sysfs_dirent *sd, struct inode *inode) iattrs->ia_secdata, iattrs->ia_secdata_len); } + +set_nlink (inode,sysfs_read_nlink (sd)); }int sysfs_getattr (struct vfsmount *mnt,struct dentry *dentry,struct kstat *stat)
fs/sysfs/sysfs.h +38 -0
@@ -69,6 +69,9 @@ struct sysfs_dirent {const void *s_ns;/* namespace tag */ unsigned int s_hash;/* ns + name hash */ +#ifdef CONFIG_SYSFS_COUNT_LINKS +unsigned int s_nlink; +#endif union {struct sysfs_elem_dir s_dir;struct sysfs_elem_symlink s_symlink;
@@ -127,6 +130,41 @@ do { \#define sysfs_dirent_init_lockdep (sd) do {} while(0)#endif +#ifdef CONFIG_SYSFS_COUNT_LINKS +static inline void sysfs_init_nlink (struct sysfs_dirent *sd) +{ +if (sysfs_type (sd) ==SYSFS_DIR ) + sd->s_nlink =2 ; +else + sd->s_nlink =1 ; +} +static inline void sysfs_inc_nlink (struct sysfs_dirent *sd) +{ + sd->s_nlink++; +} +static inline void sysfs_dec_nlink (struct sysfs_dirent *sd) +{ + sd->s_nlink++; +} +static inline unsigned int sysfs_read_nlink (struct sysfs_dirent *sd) +{ +return sd->s_nlink; +} +#else +static inline void sysfs_init_nlink (struct sysfs_dirent *sd) +{ +} +static inline void sysfs_inc_nlink (struct sysfs_dirent *sd) +{ +} +static inline void sysfs_dec_nlink (struct sysfs_dirent *sd) +{ +} +static inline unsigned int sysfs_read_nlink (struct sysfs_dirent *sd) +{ +return 1 ; +} +#endif /* * Context structure to be used while adding/removing nodes. */
Signature · 1 line
1.7.2.5
Greg Kroah-Hartman
maintainerRe: [PATCH] sysfs: Optionally count subdirectories to support buggy applications
Quoted 28 lines · Eric W. Biederman
lm_sensorsand possibly other applications get confused if all sysfs directories return nlink == 1. Thelm_sensorscode that got confused was just wrong and a fixed version oflm_sensorsshould be released shortly.There may be other applications that have problems with sysfs return nlink == 1 for directories. To allow people to continue to use old versions of userspace with new kernels add to sysfs a compile time option to maintain mostly precise directory counts for those people who don't mind the cost.
I have moved where we keep nlink in
sysfs_direntas compared to previous versions of subdirectory counting to a location that packs better.Signed-off-by Eric W. Biederman <[email protected]>--- fs/sysfs/Kconfig | 15 +++++++++++++++ fs/sysfs/dir.c | 8 ++++++++ fs/sysfs/inode.c | 2 ++ fs/sysfs/sysfs.h | 38 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 0 deletions(-)fs/sysfs/Kconfig +0
-0
fs/sysfs/Kconfig config SYSFS · line 27
Quoted 7 lines
fs/sysfs/Kconfig +4
-0@@ -21,3 +21,18 @@ config SYSFS example, "root=03:01" for /dev/hda1. Designers of embedded systems may wish to say N here to conserve space. + +config SYSFS_COUNT_LINKS + bool "sysfs count subdirectoires to support buggy applications" + default n
As we don't want to break things, this should be default y, right?
Also, should we list this in the feature_removal list as well so that we can get rid of it in a year or so?
thanks,
greg k-h
Re: [PATCH] sysfs: Optionally count subdirectories to support buggy applications
Quoted 31 lines · Greg Kroah-Hartman
Quoted 30 lines · Eric W. Biederman
lm_sensorsand possibly other applications get confused if all sysfs directories return nlink == 1. Thelm_sensorscode that got confused was just wrong and a fixed version oflm_sensorsshould be released shortly.There may be other applications that have problems with sysfs return nlink == 1 for directories. To allow people to continue to use old versions of userspace with new kernels add to sysfs a compile time option to maintain mostly precise directory counts for those people who don't mind the cost.
I have moved where we keep nlink in
sysfs_direntas compared to previous versions of subdirectory counting to a location that packs better.Signed-off-by Eric W. Biederman <[email protected]>--- fs/sysfs/Kconfig | 15 +++++++++++++++ fs/sysfs/dir.c | 8 ++++++++ fs/sysfs/inode.c | 2 ++ fs/sysfs/sysfs.h | 38 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 0 deletions(-)fs/sysfs/Kconfig +0
-0@@ -21,3 +21,18 @@ config SYSFS example, "root=03:01" for /dev/hda1.
Quoted 7 lines
fs/sysfs/Kconfig config SYSFS · line 27
Quoted 5 lines
fs/sysfs/Kconfig +4
-0Designers of embedded systems may wish to say N here to conserve space. + +config SYSFS_COUNT_LINKS + bool "sysfs count subdirectoires to support buggy applications" + default nAs we don't want to break things, this should be default y, right?
The new behavior is backwards compatible. What the new behavior is not is bug compatible. So nothing *should* break.
Furthermore the breaking we have seen so far is limited to just lm_sensors. That is exactly one program that is not a server failing to start. That seems pretty minor in the worst case.
So I really don't expect anyone who ships 3.4 to enable this option.
I have written the option solely so that in case my assessment turns out to be wrong there is already a tested solution. I have been through the pain of not being able to upgrade/test a new kernel because of a backwards incompatible change and it can be very unpleasant.
Quoted 2 lines
Also, should we list this in the
feature_removallist as well so that we can get rid of it in a year or so?
Good idea. I don't know if anyone actually reads feature removal but it is good to serve notice. I will cook up a patch for that.
Eric
Greg Kroah-Hartman
maintainerRe: [PATCH] sysfs: Optionally count subdirectories to support buggy applications
Quoted 41 lines · Eric W. Biederman
Quoted 31 lines · Greg Kroah-Hartman
Quoted 30 lines · Eric W. Biederman
lm_sensorsand possibly other applications get confused if all sysfs directories return nlink == 1. Thelm_sensorscode that got confused was just wrong and a fixed version oflm_sensorsshould be released shortly.There may be other applications that have problems with sysfs return nlink == 1 for directories. To allow people to continue to use old versions of userspace with new kernels add to sysfs a compile time option to maintain mostly precise directory counts for those people who don't mind the cost.
I have moved where we keep nlink in
sysfs_direntas compared to previous versions of subdirectory counting to a location that packs better.Signed-off-by Eric W. Biederman <[email protected]>--- fs/sysfs/Kconfig | 15 +++++++++++++++ fs/sysfs/dir.c | 8 ++++++++ fs/sysfs/inode.c | 2 ++ fs/sysfs/sysfs.h | 38 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 0 deletions(-)fs/sysfs/Kconfig +0
-0@@ -21,3 +21,18 @@ config SYSFS example, "root=03:01" for /dev/hda1.Quoted 7 lines
fs/sysfs/Kconfig config SYSFS · line 27
Quoted 5 lines
fs/sysfs/Kconfig +4
-0Designers of embedded systems may wish to say N here to conserve space. + +config SYSFS_COUNT_LINKS + bool "sysfs count subdirectoires to support buggy applications" + default nAs we don't want to break things, this should be default y, right?
Quoted 2 lines
The new behavior is backwards compatible. What the new behavior is not is bug compatible. So nothing *should* break.
"should", but you really don't know, as all we have is one report so far.
Quoted 5 lines
Furthermore the breaking we have seen so far is limited to just
lm_sensors. That is exactly one program that is not a server failing to start. That seems pretty minor in the worst case.So I really don't expect anyone who ships 3.4 to enable this option.
What about users who use a new kernel on old userspace, which happens all the time (i.e. all the kernel developers themselves)?
Quoted 4 lines
I have written the option solely so that in case my assessment turns out to be wrong there is already a tested solution. I have been through the pain of not being able to upgrade/test a new kernel because of a backwards incompatible change and it can be very unpleasant.
I'd really prefer this to be default 'y', and if a distro knows it can turn it off to save time/space, it can.
thanks,
greg k-h
Re: [PATCH] sysfs: Optionally count subdirectories to support buggy applications
In reply to Eric W. Biedermanfs/sysfs/Kconfig config SYSFS · line 26
Quoted 2 lines · Eric W. Biederman
fs/sysfs/Kconfig +2
-0+config SYSFS_COUNT_LINKS + bool "sysfs count subdirectoires to support buggy applications"
subdirectories
fs/sysfs/Kconfig config SYSFS · line 30
Quoted 1 line
fs/sysfs/Kconfig +1
-0+ Increase the memory and cpu untilization of sysfs by maintaining
utilization
fs/sysfs/Kconfig config SYSFS · line 33
Quoted 1 line
fs/sysfs/Kconfig +1
-0+ as the number of subdirectories plus two. With out this enabled
Without
fs/sysfs/Kconfig config SYSFS · line 35
Quoted 1 line
fs/sysfs/Kconfig +1
-0+ the standard way of indicating that the number of subdirectoires
subdirectories
Dave
Re: [PATCH] sysfs: Optionally count subdirectories to support buggy applications
In reply to Eric W. BiedermanQuoted 4 lines · Eric W. Biederman
fs/sysfs/Kconfig +0
-0@@ -21,3 +21,18 @@ config SYSFS example, "root=03:01" for /dev/hda1.
fs/sysfs/Kconfig config SYSFS · line 31
Quoted 10 lines
fs/sysfs/Kconfig +8
-0Designers of embedded systems may wish to say N here to conserve space. + +config SYSFS_COUNT_LINKS + bool "sysfs count subdirectoires to support buggy applications" + default n + help + + Increase the memory and cpu untilization of sysfs by maintaining + by maintaining a count of how hard links from subdirectories a
by maintaining only once.
of how "many"?
fs/sysfs/Kconfig config SYSFS · line 38
Quoted 7 lines
fs/sysfs/Kconfig +7
-0+ directory has. This allows sysfs to report the directory nlink + as the number of subdirectories plus two. With out this enabled + sysfs will report the nlink of all directories as 1, which is + the standard way of indicating that the number of subdirectoires + is not tracked. + + Unless you know you need this say N here.
You should put the opposite here. "If you are sure you don't need it, say N."
Quoted 2 lines
fs/sysfs/sysfs.h +0
-0
...
Quoted 10 lines
fs/sysfs/sysfs.h +6
-0@@ -127,6 +130,41 @@ do { \#define sysfs_dirent_init_lockdep (sd) do {} while(0)#endif +#ifdef CONFIG_SYSFS_COUNT_LINKS +static inline void sysfs_init_nlink (struct sysfs_dirent *sd) +{ +if (sysfs_type (sd) ==SYSFS_DIR ) + sd->s_nlink =2 ; +else
fs/sysfs/sysfs.h do { \ · line 148
Quoted 10 lines
fs/sysfs/sysfs.h +10
-0+ sd->s_nlink =1 ; +} +static inline void sysfs_inc_nlink (struct sysfs_dirent *sd) +{ + sd->s_nlink++; +} +static inline void sysfs_dec_nlink (struct sysfs_dirent *sd) +{ + sd->s_nlink++; +}
Minus minus.
thanks,
Signature · 2 lines
js suse labs
Re: [PATCH] sysfs: Optionally count subdirectories to support buggy applications
In reply to Eric W. BiedermanQuoted 5 lines · Eric W. Biederman
lm_sensorsand possibly other applications get confused if all sysfs directories return nlink == 1. Thelm_sensorscode that got confused was just wrong and a fixed version oflm_sensorsshould be released shortly.
I think this patch is horrible, and broken.
And making the feature a config option is just stupid.
The simple approach is to
- implement a inode->i_op->getattr function for sysfs
- make it call generic_fillattr()
- after filling in the generic fields, count the number of sysfs
children it has and update stat->nlink appropriately.
No extra "keep track of inode counts by hand" crap, and no idiotic config options that just make it easy to (conditionally) get things wrong. Just do it right, and do it *unconditionally* right.
The Linux VFS layer is smart, and allows filesystems to do these kinds of fixups.
Linus
Re: [PATCH] sysfs: Optionally count subdirectories to support buggy applications
Quoted 4 lines · Linus Torvalds
No extra "keep track of inode counts by hand" crap, and no idiotic config options that just make it easy to (conditionally) get things wrong. Just do it right, and do it *unconditionally* right.
And btw, "nlink shows number of subdirectories" for a directory entry really *is* right. It's how Unix filesystems work, like it or not.
It's mainly lazy/bad filesystems that set nlink to 1. So the whole "nlink==1" case is meant for crap like FAT etc, not for a filesystem that we control and that could easily just do it right.
Which is why I detest that config option. It's as if you were asking the user
"Do you want to make the sysfs filesystem act like crap filesystems?"
and kernel config time. What kind of inane question is that?
Linus
Al Viro
maintainerRe: [PATCH] sysfs: Optionally count subdirectories to support buggy applications
Quoted 7 lines · Linus Torvalds
Quoted 4 lines · Linus Torvalds
No extra "keep track of inode counts by hand" crap, and no idiotic config options that just make it easy to (conditionally) get things wrong. Just do it right, and do it *unconditionally* right.
Quoted 6 lines
And btw, "nlink shows number of subdirectories" for a directory entry really *is* right. It's how Unix filesystems work, like it or not.
It's mainly lazy/bad filesystems that set nlink to 1. So the whole "nlink==1" case is meant for crap like FAT etc, not for a filesystem that we control and that could easily just do it right.
It's a bit more complicated than that and it had always been a bit of a minefield. v6: link(2) began with
ip = namei(&uchar, 0);
if(ip == NULL)
return;
if(ip->i_nlink >= 127) {
u.u_error = EMLINK;
goto out;
}(and mkdir(), of course, was implemented via mknod+link). Up to 127 links to any object. Fine; v7: EMLINK defined, but _never_ returned. Moreover, mkdir(1) didn't bother to check link counts either. Result: 65535 calls of link(2) and you've got yourself an fs corruption on i_nlink overflow (it was 16bit in on-disk inode).
Linux implementation of link(2) had exactly the same bug as that of v7 until 0.98.3 (for link(2)) and 0.99.1 (for mkdir(2), rename(2) - didn't exist as syscalls on v7, but had the same problem as soon as they made it in kernel at some point in BSD history). Note that limit for minixfs remained ridiculously low until '97 or so; for ext2 it was 32000 from the very beginning but note that e.g. ext had _not_ acquired fixes for link overflows on mkdir() at all - it had that hole all the way until removal in 2.1.26. Of course, there was a (completely useless) POSIX-mandated LINK_MAX, but since the actual limit depends on fs type, well... the checks remain in ->link()/->mkdir()/->rename() instances and IIRC I've caught a bunch of overflows in those circa 2.1.very_late.
As the result, old stat(2) had 16bit st_nlink - same as v7. Nobody needed more, after all. Alpha port got it 32bit (inherited the struct stat layout from OSF, presumably?), but other early ports kept 16bit there.
At some point folks started whining about wanting more subdirectories and that's when the things began to get really convoluted and ugly. By that time we had a variant of stat(2) that used 32bit st_nlink, but on-disk layouts remained a problem. Some filesystems went for more or less reasonable "the limit is circa 2^32, EMLINK if we get more than that and -EOVERFLOW on old_stat(2) if it doesn't fit into 16 bits". However, it was _not_ universally true - e.g. reiserfs does _NOT_ do that for mkdir(). There we get "directory link count grows up to 16bit limit and gets stuck at 1 if it ever grows past that", on the theory that st_nlink == 1 is distinguishable from anything you'd normally get for a directory. I have no idea where that invention has come from, but it's been around for more than a decade. Of course, Hans being Hans, it had been advertised as major reiserfs feature - you could get an unlimited amount of subdirectories, which no other fs on Linux would allow, nevermind that nobody sane would actually make use of that...
At about the same time somebody had done the same trick on ext2 - Daniel, probably, since it had been floating in directory index patchset. It never got merged into mainline; ext3 port _did_, but without those bits actually used (EXT3_DIR_LINK_MAX is defined, but never used). ext4 actually has it hooked in.
I don't see anything else other than ext4 and reiserfs using that convention, but then just grepping for inc_nlink/inode_inc_use_count shows a bloody mess - e.g. jffs2 does not check overflows (32bit there) at all, neither on link() nor on mkdir()/rename(). It _might_ get away with that (with non-standard errno, if so), but I don't remember that code well enough to tell at the quick look. HFS+ doesn't seem to check for overflows either and while it doesn't track link count on directories, it does support link(2) and it does bump i_nlink there. A _lot_ of filesystems (starting with ramfs) assumes that we'll get an OOM before we get to 2^32 links to an inode on purely in-core filesystem; reasonable back in 2001 or so, but not these days...
Al Viro
maintainerRFC 1/1 killing boilerplate checks in ->link/->mkdir/->rename
Quoted 12 lines
Linux implementation of link(2) had exactly the same bug as that of v7 until 0.98.3 (for link(2)) and 0.99.1 (for mkdir(2), rename(2) - didn't exist as syscalls on v7, but had the same problem as soon as they made it in kernel at some point in BSD history). Note that limit for minixfs remained ridiculously low until '97 or so; for ext2 it was 32000 from the very beginning but note that e.g. ext had _not_ acquired fixes for link overflows on
mkdir()at all - it had that hole all the way until removal in 2.1.26. Of course, there was a (completely useless) POSIX-mandatedLINK_MAX, but since the actual limit depends on fs type, well... the checks remain in ->link()/->mkdir()/->rename() instances and IIRC I've caught a bunch of overflows in those circa 2.1.very_late.
FWIW, there's something we really should've done a long time ago: putting that limit into sb->s_max_links. With 0 meaning "leave all checks to ->link/->mkdir/->rename". Something like the following would make a reasonable start - just the conversion of obvious cases. As the next step I'd probably initialize it as ~0U instead of 0 and let the filesystems that want something trickier (reiserfs, ext4, gfs2, ocfs2) explicitly set it to 0 in their foo_fill_super(). That would take care of a bunch of cases where we forgot to do those checks (ubifs, hfsplus, jffs2, ramfs, etc.) and it's probably a saner default anyway.
Comments? Boilerplate removal follows (22 files changed, 45 insertions(+), 120 deletions(-)), but it's *not* for immediate merge; it's really completely untested.
fs/exofs/namei.c +1 -12
@@ -143,9 +143,6 @@ static int exofs_link(struct dentry *old_dentry, struct inode *dir, {struct inode *inode = old_dentry->d_inode; -if (inode->i_nlink >=EXOFS_LINK_MAX ) -return -EMLINK ; - inode->i_ctime =CURRENT_TIME ;inode_inc_link_count (inode);ihold (inode);
@@ -156,10 +153,7 @@ static int exofs_link(struct dentry *old_dentry, struct inode *dir,static int exofs_mkdir (struct inode *dir,struct dentry *dentry,umode_t mode) {struct inode *inode; -int err = -EMLINK ; - -if (dir->i_nlink >=EXOFS_LINK_MAX ) - goto out; +int err;inode_inc_link_count (dir);
@@ -275,11 +269,6 @@ static int exofs_rename(struct inode *old_dir, struct dentry *old_dentry,if (err) goto out_dir; }else { -if (dir_de) { - err = -EMLINK ; -if (new_dir->i_nlink >=EXOFS_LINK_MAX ) - goto out_dir; - } err =exofs_add_link (new_dentry, old_inode);if (err) goto out_dir;
fs/exofs/super.c +1 -0
@@ -754,6 +754,7 @@ static int exofs_fill_super(struct super_block *sb, void *data, int silent) sb->s_blocksize =EXOFS_BLKSIZE ; sb->s_blocksize_bits =EXOFS_BLKSHIFT ; sb->s_maxbytes =MAX_LFS_FILESIZE ; + sb->s_max_links =EXOFS_LINK_MAX ;atomic_set (&sbi->s_curr_pending,0 ); sb->s_bdev =NULL ; sb->s_dev =0 ;
fs/ext2/namei.c +1 -12
@@ -195,9 +195,6 @@ static int ext2_link (struct dentry * old_dentry, struct inode * dir,struct inode *inode = old_dentry->d_inode;int err; -if (inode->i_nlink >=EXT2_LINK_MAX ) -return -EMLINK ; -dquot_initialize (dir); inode->i_ctime =CURRENT_TIME_SEC ;
@@ -217,10 +214,7 @@ static int ext2_link (struct dentry * old_dentry, struct inode * dir,static int ext2_mkdir (struct inode * dir,struct dentry * dentry,umode_t mode) {struct inode * inode; -int err = -EMLINK ; - -if (dir->i_nlink >=EXT2_LINK_MAX ) - goto out; +int err;dquot_initialize (dir);
@@ -346,11 +340,6 @@ static int ext2_rename (struct inode * old_dir, struct dentry * old_dentry,drop_nlink (new_inode);inode_dec_link_count (new_inode); }else { -if (dir_de) { - err = -EMLINK ; -if (new_dir->i_nlink >=EXT2_LINK_MAX ) - goto out_dir; - } err =ext2_add_link (new_dentry, old_inode);if (err) goto out_dir;
fs/ext2/super.c +1 -0
@@ -919,6 +919,7 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent) } sb->s_maxbytes =ext2_max_size (sb->s_blocksize_bits); + sb->s_max_links =EXT2_LINK_MAX ;if (le32_to_cpu (es->s_rev_level) ==EXT2_GOOD_OLD_REV ) { sbi->s_inode_size =EXT2_GOOD_OLD_INODE_SIZE ;
fs/jfs/namei.c +0 -13
@@ -220,12 +220,6 @@ static int jfs_mkdir(struct inode *dip, struct dentry *dentry, umode_t mode)dquot_initialize (dip); -/* link count overflow on parent directory ? */ -if (dip->i_nlink ==JFS_LINK_MAX ) { - rc = -EMLINK ; - goto out1; - } -/* * search parent directory for entry/freespace * (dtSearch() returns parent directory page pinned)
@@ -806,9 +800,6 @@ static int jfs_link(struct dentry *old_dentry,jfs_info ("jfs_link: %s %s" , old_dentry->d_name.name, dentry->d_name.name); -if (ip->i_nlink ==JFS_LINK_MAX ) -return -EMLINK ; -dquot_initialize (dir); tid =txBegin (ip->i_sb,0 );
@@ -1138,10 +1129,6 @@ static int jfs_rename(struct inode *old_dir, struct dentry *old_dentry, rc = -ENOTEMPTY ; goto out3; } - }else if ((new_dir != old_dir) && - (new_dir->i_nlink ==JFS_LINK_MAX )) { - rc = -EMLINK ; - goto out3; } }else if (new_ip) {IWRITE_LOCK (new_ip,RDWRLOCK_NORMAL );
fs/jfs/super.c +1 -0
@@ -441,6 +441,7 @@ static int jfs_fill_super(struct super_block *sb, void *data, int silent)return -ENOMEM ; sb->s_fs_info = sbi; + sb->s_max_links =JFS_LINK_MAX ; sbi->sb = sb; sbi->uid = sbi->gid = sbi->umask =-1 ;
fs/logfs/dir.c +0 -3
@@ -558,9 +558,6 @@ static int logfs_link(struct dentry *old_dentry, struct inode *dir, {struct inode *inode = old_dentry->d_inode; -if (inode->i_nlink >=LOGFS_LINK_MAX ) -return -EMLINK ; - inode->i_ctime = dir->i_ctime = dir->i_mtime =CURRENT_TIME ;ihold (inode);inc_nlink (inode);
fs/logfs/super.c +1 -0
@@ -542,6 +542,7 @@ static struct dentry *logfs_get_sb_device(struct logfs_super *super,* the filesystem incompatible with 32bit systems. */ sb->s_maxbytes = (1ull <<43 ) -1 ; + sb->s_max_links =LOGFS_LINK_MAX ; sb->s_op = &logfs_super_operations; sb->s_flags = flags |MS_NOATIME ;
fs/minix/inode.c +5 -5
@@ -190,24 +190,24 @@ static int minix_fill_super(struct super_block *s, void *data, int silent) sbi->s_version =MINIX_V1 ; sbi->s_dirsize =16 ; sbi->s_namelen =14 ; - sbi->s_link_max =MINIX_LINK_MAX ; + s->s_max_links =MINIX_LINK_MAX ; }else if (s->s_magic ==MINIX_SUPER_MAGIC2 ) { sbi->s_version =MINIX_V1 ; sbi->s_dirsize =32 ; sbi->s_namelen =30 ; - sbi->s_link_max =MINIX_LINK_MAX ; + s->s_max_links =MINIX_LINK_MAX ; }else if (s->s_magic ==MINIX2_SUPER_MAGIC ) { sbi->s_version =MINIX_V2 ; sbi->s_nzones = ms->s_zones; sbi->s_dirsize =16 ; sbi->s_namelen =14 ; - sbi->s_link_max =MINIX2_LINK_MAX ; + s->s_max_links =MINIX2_LINK_MAX ; }else if (s->s_magic ==MINIX2_SUPER_MAGIC2 ) { sbi->s_version =MINIX_V2 ; sbi->s_nzones = ms->s_zones; sbi->s_dirsize =32 ; sbi->s_namelen =30 ; - sbi->s_link_max =MINIX2_LINK_MAX ; + s->s_max_links =MINIX2_LINK_MAX ; }else if ( *(__u16 *)(bh->b_data +24 ) ==MINIX3_SUPER_MAGIC ) { m3s = (struct minix3_super_block *) bh->b_data; s->s_magic = m3s->s_magic;
@@ -221,9 +221,9 @@ static int minix_fill_super(struct super_block *s, void *data, int silent) sbi->s_dirsize =64 ; sbi->s_namelen =60 ; sbi->s_version =MINIX_V3 ; - sbi->s_link_max =MINIX2_LINK_MAX ; sbi->s_mount_state =MINIX_VALID_FS ;sb_set_blocksize (s, m3s->s_blocksize); + s->s_max_links =MINIX2_LINK_MAX ; }else goto out_no_fs;
fs/minix/minix.h +0 -1
@@ -34,7 +34,6 @@ struct minix_sb_info {unsigned long s_max_size;int s_dirsize;int s_namelen; -int s_link_max;struct buffer_head ** s_imap;struct buffer_head ** s_zmap;struct buffer_head * s_sbh;
fs/minix/namei.c +1 -13
@@ -94,9 +94,6 @@ static int minix_link(struct dentry * old_dentry, struct inode * dir, {struct inode *inode = old_dentry->d_inode; -if (inode->i_nlink >=minix_sb (inode->i_sb)->s_link_max) -return -EMLINK ; - inode->i_ctime =CURRENT_TIME_SEC ;inode_inc_link_count (inode);ihold (inode);
@@ -106,10 +103,7 @@ static int minix_link(struct dentry * old_dentry, struct inode * dir,static int minix_mkdir (struct inode * dir,struct dentry *dentry,umode_t mode) {struct inode * inode; -int err = -EMLINK ; - -if (dir->i_nlink >=minix_sb (dir->i_sb)->s_link_max) - goto out; +int err;inode_inc_link_count (dir);
@@ -181,7 +175,6 @@ static int minix_rmdir(struct inode * dir, struct dentry *dentry)static int minix_rename (struct inode * old_dir,struct dentry *old_dentry,struct inode * new_dir,struct dentry *new_dentry) { -struct minix_sb_info * info =minix_sb (old_dir->i_sb);struct inode * old_inode = old_dentry->d_inode;struct inode * new_inode = new_dentry->d_inode;struct page * dir_page =NULL ;
@@ -219,11 +212,6 @@ static int minix_rename(struct inode * old_dir, struct dentry *old_dentry,drop_nlink (new_inode);inode_dec_link_count (new_inode); }else { -if (dir_de) { - err = -EMLINK ; -if (new_dir->i_nlink >= info->s_link_max) - goto out_dir; - } err =minix_add_link (new_dentry, old_inode);if (err) goto out_dir;
fs/namei.c +13 -0
@@ -2545,6 +2545,7 @@ SYSCALL_DEFINE3(mknod, const char __user *, filename, umode_t, mode, unsigned, dint vfs_mkdir (struct inode *dir,struct dentry *dentry,umode_t mode) {int error =may_create (dir, dentry); +unsigned max_links = dir->i_sb->s_max_links;if (error)return error;
@@ -2557,6 +2558,9 @@ int vfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)if (error)return error; +if (max_links && dir->i_nlink >= max_links) +return -EMLINK ; + error = dir->i_op->mkdir (dir, dentry, mode);if (!error)fsnotify_mkdir (dir, dentry);
@@ -2887,6 +2891,7 @@ SYSCALL_DEFINE2(symlink, const char __user *, oldname, const char __user *, newnint vfs_link (struct dentry *old_dentry,struct inode *dir,struct dentry *new_dentry) {struct inode *inode = old_dentry->d_inode; +unsigned max_links = dir->i_sb->s_max_links;int error;if (!inode)
@@ -2917,6 +2922,8 @@ int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_de/* Make sure we don't allow creating hardlink to an unlinked file */ if (inode->i_nlink ==0 ) error = -ENOENT ; +else if (max_links && inode->i_nlink >= max_links) + error = -EMLINK ;else error = dir->i_op->link (old_dentry, dir, new_dentry);mutex_unlock (&inode->i_mutex);
@@ -3026,6 +3033,7 @@ static int vfs_rename_dir(struct inode *old_dir, struct dentry *old_dentry, {int error =0 ;struct inode *target = new_dentry->d_inode; +unsigned max_links = new_dir->i_sb->s_max_links;/* * If we are going to change the parent - check write permissions,
@@ -3049,6 +3057,11 @@ static int vfs_rename_dir(struct inode *old_dir, struct dentry *old_dentry,if (d_mountpoint (old_dentry) ||d_mountpoint (new_dentry)) goto out; + error = -EMLINK ; +if (max_links && !target && new_dir != old_dir && + new_dir->i_nlink >= max_links) + goto out; +if (target)shrink_dcache_parent (new_dentry); error = old_dir->i_op->rename (old_dir, old_dentry, new_dir, new_dentry);
fs/nilfs2/namei.c +0 -11
@@ -193,9 +193,6 @@ static int nilfs_link(struct dentry *old_dentry, struct inode *dir,struct nilfs_transaction_info ti;int err; -if (inode->i_nlink >=NILFS_LINK_MAX ) -return -EMLINK ; - err =nilfs_transaction_begin (dir->i_sb, &ti,1 );if (err)return err;
@@ -219,9 +216,6 @@ static int nilfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)struct nilfs_transaction_info ti;int err; -if (dir->i_nlink >=NILFS_LINK_MAX ) -return -EMLINK ; - err =nilfs_transaction_begin (dir->i_sb, &ti,1 );if (err)return err;
@@ -400,11 +394,6 @@ static int nilfs_rename(struct inode *old_dir, struct dentry *old_dentry,drop_nlink (new_inode);nilfs_mark_inode_dirty (new_inode); }else { -if (dir_de) { - err = -EMLINK ; -if (new_dir->i_nlink >=NILFS_LINK_MAX ) - goto out_dir; - } err =nilfs_add_link (new_dentry, old_inode);if (err) goto out_dir;
fs/nilfs2/super.c +1 -0
@@ -1059,6 +1059,7 @@ nilfs_fill_super(struct super_block *sb, void *data, int silent) sb->s_export_op = &nilfs_export_ops; sb->s_root =NULL ; sb->s_time_gran =1 ; + sb->s_max_links =NILFS_LINK_MAX ; bdi = sb->s_bdev->bd_inode->i_mapping->backing_dev_info; sb->s_bdi = bdi ? : &default_backing_dev_info;
fs/sysv/namei.c +1 -11
@@ -121,9 +121,6 @@ static int sysv_link(struct dentry * old_dentry, struct inode * dir, {struct inode *inode = old_dentry->d_inode; -if (inode->i_nlink >=SYSV_SB (inode->i_sb)->s_link_max) -return -EMLINK ; - inode->i_ctime =CURRENT_TIME_SEC ;inode_inc_link_count (inode);ihold (inode);
@@ -134,10 +131,8 @@ static int sysv_link(struct dentry * old_dentry, struct inode * dir,static int sysv_mkdir (struct inode * dir,struct dentry *dentry,umode_t mode) {struct inode * inode; -int err = -EMLINK ; +int err; -if (dir->i_nlink >=SYSV_SB (dir->i_sb)->s_link_max) - goto out;inode_inc_link_count (dir); inode =sysv_new_inode (dir,S_IFDIR |mode);
@@ -251,11 +246,6 @@ static int sysv_rename(struct inode * old_dir, struct dentry * old_dentry,drop_nlink (new_inode);inode_dec_link_count (new_inode); }else { -if (dir_de) { - err = -EMLINK ; -if (new_dir->i_nlink >=SYSV_SB (new_dir->i_sb)->s_link_max) - goto out_dir; - } err =sysv_add_link (new_dentry, old_inode);if (err) goto out_dir;
fs/sysv/super.c +12 -12
@@ -44,7 +44,7 @@ enum { JAN_1_1980 = (10*365 + 2) * 24 * 60 * 60 }; -static void detected_xenix (struct sysv_sb_info *sbi) +static void detected_xenix (struct sysv_sb_info *sbi,unsigned *max_links) {struct buffer_head *bh1 = sbi->s_bh1;struct buffer_head *bh2 = sbi->s_bh2;
@@ -59,7 +59,7 @@ static void detected_xenix(struct sysv_sb_info *sbi) sbd2 = (struct xenix_super_block *) (bh2->b_data -512 ); } - sbi->s_link_max =XENIX_LINK_MAX ; + *max_links =XENIX_LINK_MAX ; sbi->s_fic_size =XENIX_NICINOD ; sbi->s_flc_size =XENIX_NICFREE ; sbi->s_sbd1 = (char *)sbd1;
@@ -75,7 +75,7 @@ static void detected_xenix(struct sysv_sb_info *sbi) sbi->s_nzones =fs32_to_cpu (sbi, sbd1->s_fsize); } -static void detected_sysv4 (struct sysv_sb_info *sbi) +static void detected_sysv4 (struct sysv_sb_info *sbi,unsigned *max_links) {struct sysv4_super_block * sbd;struct buffer_head *bh1 = sbi->s_bh1;
@@ -86,7 +86,7 @@ static void detected_sysv4(struct sysv_sb_info *sbi)else sbd = (struct sysv4_super_block *) bh2->b_data; - sbi->s_link_max =SYSV_LINK_MAX ; + *max_links =SYSV_LINK_MAX ; sbi->s_fic_size =SYSV_NICINOD ; sbi->s_flc_size =SYSV_NICFREE ; sbi->s_sbd1 = (char *)sbd;
@@ -103,7 +103,7 @@ static void detected_sysv4(struct sysv_sb_info *sbi) sbi->s_nzones =fs32_to_cpu (sbi, sbd->s_fsize); } -static void detected_sysv2 (struct sysv_sb_info *sbi) +static void detected_sysv2 (struct sysv_sb_info *sbi,unsigned *max_links) {struct sysv2_super_block *sbd;struct buffer_head *bh1 = sbi->s_bh1;
@@ -114,7 +114,7 @@ static void detected_sysv2(struct sysv_sb_info *sbi)else sbd = (struct sysv2_super_block *) bh2->b_data; - sbi->s_link_max =SYSV_LINK_MAX ; + *max_links =SYSV_LINK_MAX ; sbi->s_fic_size =SYSV_NICINOD ; sbi->s_flc_size =SYSV_NICFREE ; sbi->s_sbd1 = (char *)sbd;
@@ -131,14 +131,14 @@ static void detected_sysv2(struct sysv_sb_info *sbi) sbi->s_nzones =fs32_to_cpu (sbi, sbd->s_fsize); } -static void detected_coherent (struct sysv_sb_info *sbi) +static void detected_coherent (struct sysv_sb_info *sbi,unsigned *max_links) {struct coh_super_block * sbd;struct buffer_head *bh1 = sbi->s_bh1; sbd = (struct coh_super_block *) bh1->b_data; - sbi->s_link_max =COH_LINK_MAX ; + *max_links =COH_LINK_MAX ; sbi->s_fic_size =COH_NICINOD ; sbi->s_flc_size =COH_NICFREE ; sbi->s_sbd1 = (char *)sbd;
@@ -154,12 +154,12 @@ static void detected_coherent(struct sysv_sb_info *sbi) sbi->s_nzones =fs32_to_cpu (sbi, sbd->s_fsize); } -static void detected_v7 (struct sysv_sb_info *sbi) +static void detected_v7 (struct sysv_sb_info *sbi,unsigned *max_links) {struct buffer_head *bh2 = sbi->s_bh2;struct v7_super_block *sbd = (struct v7_super_block *)bh2->b_data; - sbi->s_link_max =V7_LINK_MAX ; + *max_links =V7_LINK_MAX ; sbi->s_fic_size =V7_NICINOD ; sbi->s_flc_size =V7_NICFREE ; sbi->s_sbd1 = (char *)sbd;
@@ -290,7 +290,7 @@ static char *flavour_names[] = { [FSTYPE_AFS] = "AFS", }; -static void (*flavour_setup[])(struct sysv_sb_info *) = { +static void (*flavour_setup[])(struct sysv_sb_info *,unsigned *) = { [FSTYPE_XENIX ] = detected_xenix, [FSTYPE_SYSV4 ] = detected_sysv4, [FSTYPE_SYSV2 ] = detected_sysv2,
@@ -310,7 +310,7 @@ static int complete_read_super(struct super_block *sb, int silent, int size) sbi->s_firstinodezone =2 ; - flavour_setup[sbi->s_type](sbi); + flavour_setup[sbi->s_type](sbi, &sb->s_max_links); sbi->s_truncate =1 ; sbi->s_ndatazones = sbi->s_nzones - sbi->s_firstdatazone;
fs/sysv/sysv.h +0 -1
@@ -24,7 +24,6 @@ struct sysv_sb_info {char s_bytesex;/* bytesex (le/be/pdp) */ char s_truncate;/* if 1: names > SYSV_NAMELEN chars are truncated */ /* if 0: they are disallowed (ENAMETOOLONG) */ -nlink_t s_link_max;/* max number of hard links to a file */ unsigned int s_inodes_per_block;/* number of inodes per block */ unsigned int s_inodes_per_block_1;/* inodes_per_block - 1 */ unsigned int s_inodes_per_block_bits;/* log2(inodes_per_block) */
fs/udf/namei.c +0 -13
@@ -32,8 +32,6 @@#include <linux/crc-itu-t.h> #include <linux/exportfs.h> -enum {UDF_MAX_LINKS =0xffff }; -static inline int udf_match (int len1,const unsigned char *name1,int len2,const unsigned char *name2) {
@@ -649,10 +647,6 @@ static int udf_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)struct udf_inode_info *dinfo =UDF_I (dir);struct udf_inode_info *iinfo; - err = -EMLINK ; -if (dir->i_nlink >=UDF_MAX_LINKS ) - goto out; - err = -EIO ; inode =udf_new_inode (dir,S_IFDIR | mode, &err);if (!inode)
@@ -1032,9 +1026,6 @@ static int udf_link(struct dentry *old_dentry, struct inode *dir,struct fileIdentDesc cfi, *fi;int err; -if (inode->i_nlink >=UDF_MAX_LINKS ) -return -EMLINK ; - fi =udf_add_entry (dir, dentry, &fibh, &cfi, &err);if (!fi) {return err;
@@ -1126,10 +1117,6 @@ static int udf_rename(struct inode *old_dir, struct dentry *old_dentry,if (udf_get_lb_pblock (old_inode->i_sb, &tloc,0 ) != old_dir->i_ino) goto end_rename; - - retval = -EMLINK ; -if (!new_inode && new_dir->i_nlink >=UDF_MAX_LINKS ) - goto end_rename; }if (!nfi) { nfi = udf_add_entry(new_dir, new_dentry, &nfibh, &ncfi,
fs/udf/super.c +3 -0
@@ -75,6 +75,8 @@#define UDF_DEFAULT_BLOCKSIZE 2048 +enum {UDF_MAX_LINKS =0xffff }; +/* These are the "meat" - everything else is stuffing */ static int udf_fill_super (struct super_block *,void *,int );static void udf_put_super (struct super_block *);
@@ -2042,6 +2044,7 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent) goto error_out; } sb->s_maxbytes =MAX_LFS_FILESIZE ; + sb->s_max_links =UDF_MAX_LINKS ;return 0 ; error_out:
fs/ufs/namei.c +1 -13
@@ -166,10 +166,6 @@ static int ufs_link (struct dentry * old_dentry, struct inode * dir,int error;lock_ufs (dir->i_sb); -if (inode->i_nlink >=UFS_LINK_MAX ) { -unlock_ufs (dir->i_sb); -return -EMLINK ; - } inode->i_ctime =CURRENT_TIME_SEC ;inode_inc_link_count (inode);
@@ -183,10 +179,7 @@ static int ufs_link (struct dentry * old_dentry, struct inode * dir,static int ufs_mkdir (struct inode * dir,struct dentry * dentry,umode_t mode) {struct inode * inode; -int err = -EMLINK ; - -if (dir->i_nlink >=UFS_LINK_MAX ) - goto out; +int err;lock_ufs (dir->i_sb);inode_inc_link_count (dir);
@@ -305,11 +298,6 @@ static int ufs_rename(struct inode *old_dir, struct dentry *old_dentry,drop_nlink (new_inode);inode_dec_link_count (new_inode); }else { -if (dir_de) { - err = -EMLINK ; -if (new_dir->i_nlink >=UFS_LINK_MAX ) - goto out_dir; - } err =ufs_add_link (new_dentry, old_inode);if (err) goto out_dir;
fs/ufs/super.c +1 -0
@@ -1157,6 +1157,7 @@ magic_found: "fast symlink size (%u)\n", uspi->s_maxsymlinklen); uspi->s_maxsymlinklen = maxsymlen; } + sb->s_max_links =UFS_LINK_MAX ; inode =ufs_iget (sb,UFS_ROOTINO );if (IS_ERR (inode)) {
include/linux/fs.h +1 -0
@@ -1459,6 +1459,7 @@ struct super_block {u8 s_uuid[16 ];/* UUID */ void *s_fs_info;/* Filesystem private info */ +unsigned int s_max_links;fmode_t s_mode;/* Granularity of c/m/atime in ns.
Re: [RFC] killing boilerplate checks in ->link/->mkdir/->rename
Quoted 4 lines · Al Viro
Comments? Boilerplate removal follows (22 files changed, 45 insertions(+), 120 deletions(-)), but it's *not* for immediate merge; it's really completely untested.
Looks ok to me. Historically, the more things we can check at the VFS layer, the better.
Linus
Al Viro
maintainerRe: [RFC] killing boilerplate checks in ->link/->mkdir/->rename
Quoted 8 lines · Linus Torvalds
Quoted 4 lines · Al Viro
Comments? ?Boilerplate removal follows (22 files changed, 45 insertions(+), 120 deletions(-)), but it's *not* for immediate merge; it's really completely untested.
Looks ok to me. Historically, the more things we can check at the VFS layer, the better.
After looking a bit more: nlink_t is a f*cking mess. Almost any code using that type kernel-side is broken. Crap galore:
* sometimes it's 32 bits, sometimes 16, sometimes 64. Essentially
at random.
* almost all have it unsigned, except for sparc32, where it's
signed short [inherited from v7 via SunOS? BTW, in v6 it used to be even funnier - char, which is where ridiculous LINK_MAX == 127 comes from]
IOW, nlink_t is an attractive nuisance - it's nearly impossible to use in a portable way and we are lucky that almost nobody tries to. Exceptions:ocfs2_rename() does
nlink_t old_dir_nlink = old_dir->i_nlink; ...
followed later by comparison with old_dir->i_nlink. And no, it's not to handle truncation - it's "what if i_nlink changed while ocfs2_rename() had been grabbing the cluster lock" kind of thing. OCFS2 can have up to 2^32 links to file, so truncation is really possible... AFAICS, that one is a genuine bug - this nlink_t should be u32... Another one is proc_dir_entry ->nlink and it would cause Bad Things(tm) on architecture with 16bit nlink_t if we could end up with 65534 subdirectories in some procfs dir. Might be possible, might be not - doing that under /proc/sys is definitely possible, but that won't be enough; needs to be proc_dir_entry-backed directory. Again, solution is to use explicit u32 anyway.
* compat_nlink_t is even funnier - it's signed in *two* cases; sparc
and ppc. No, nlink_t on ppc32 is unsigned. Not that anyone cared, really, since the _only_ use of that type is in struct compat_stat. For exactly one field. Only used as left-hand side of assignment, which is actually broken since unlike cp_new_stat(), cp_compat_stat() does *not* check if the value fits into st_nlink. Bug, needs to be fixed. Incidentally, just what should we do on sparc32 if we run into a file with 4G-10 links? -EOVERFLOW or silently put 65536-10 in st_nlink and be done with that? Note that filesystems allowing that many links *do* exist...
* when does jfs dtInsert() return -EMLINK? Can it ever get triggered?
* WTF is XFS doing with these checks? Note that we have them
done _twice_ on all paths - explictly from xfs_create(), xfs_link(), xfs_rename() and then from xfs_bumplink() called by exactly the same set of functions.
* what's up with btrfs_insert_inode_ref()? I've tried to trace
the codepaths around there, but... Incidentally, when could fixup_low_keys() return non-zero? I don't see any candidates for that in there... Chris?
* ubifs, hfsplus, jffs2 - definitely broken if you create enough
links. i_nlink wraparound to zero, confused inode eviction logics.
Al Viro
maintainerRe: [RFC] killing boilerplate checks in ->link/->mkdir/->rename
Quoted 10 lines · Al Viro
After looking a bit more:
nlink_tis a f*cking mess. Almost any code using that type kernel-side is broken. Crap galore:
* sometimes it's 32 bits, sometimes 16, sometimes 64. Essentially
at random.
* almost all have it unsigned, except for sparc32, where it's
signed short [inherited from v7 via SunOS? BTW, in v6 it used to be even funnier - char, which is where ridiculousLINK_MAX== 127 comes from]IOW,
nlink_tis an attractive nuisance - it's nearly impossible to use in a portable way and we are lucky that almost nobody tries to.
Incidentally, why the hell do we have
typedef __kernel_nlink_t nlink_t;
anyway? It's *not* exposed to userland and it's different from the userland nlink_t (which is unsigned int on 32bit and unsigned long on 64bit). Why not use __kernel_nlink_t (or explicitly-sized __uNN) in arch/*/include/asm/stat.h and declare nlink_t kernel-side as __u32?
Why do we have daddr_t, while we are at it? There is exactly one user - fs/freevxfs and there we definitely want a fixed-sized type.
Re: [RFC] killing boilerplate checks in ->link/->mkdir/->rename
Quoted 7 lines · Al Viro
Incidentally, why the hell do we have
typedef__kernel_nlink_tnlink_t;
anyway? It's *not* exposed to userland and it's different from the userlandnlink_t(which is unsigned int on 32bit and unsigned long on 64bit). Why not use__kernel_nlink_t(or explicitly-sized__uNN) inarch/*/include/asm/stat.hand declarenlink_tkernel-side as__u32?
Probably hysterical raisins, and just converted to the whole __kernel_nlink_t form together with other, more relevant things.
Feel free to remove it.
Quoted 2 lines
Why do we have
daddr_t, while we are at it? There is exactly one user - fs/freevxfs and there we definitely want a fixed-sized type.
I think it's something that probably came from freevxfs and BSD roots or similar. It's a BSD'ism, afaik.
Linus
Re: [RFC] killing boilerplate checks in ->link/->mkdir/->rename
In reply to Al ViroQuoted 7 lines · Al Viro
Quoted 6 lines · Linus Torvalds
Quoted 4 lines · Al Viro
Comments? ?Boilerplate removal follows (22 files changed, 45 insertions(+), 120 deletions(-)), but it's *not* for immediate merge; it's really completely untested.
Quoted 6 lines
Quoted 2 lines
Looks ok to me. Historically, the more things we can check at the VFS layer, the better.
* what's up with
btrfs_insert_inode_ref()? I've tried to trace
the codepaths around there, but...
Btrfs stores backrefs (the filename, directory inode number) from the inode to the directory.
In the current format that's a pretty low limit on how many of these we can store for hard links to the same file in the same directory, but basically no limit on how many backrefs we can store to the same file from different directories.
Mark Fasheh was working on a patch to change the backrefs to make the links-from-the-same-dir case consistent with the
links-from-different-dir case. With today's code, we'll go -EMLINK at
different times depending on the length of the file name and what links you've already made.
Quoted 2 lines
Incidentally, when could
fixup_low_keys()
return non-zero? I don't see any candidates for that in there... Chris?
A long time ago this one used to cow blocks and so it needed an error return. I think Jeff Mahoney has a patch queued up to make it (among many others) void.
-chris
Al Viro
maintainerRe: [RFC] killing boilerplate checks in ->link/->mkdir/->rename
In reply to Al ViroQuoted 2 lines · Al Viro
* ubifs, hfsplus, jffs2 - definitely broken if you create enough
links.i_nlinkwraparound to zero, confused inode eviction logics.
BTW, ubifs plays funny games with i_nlink - decrements it early in unlink/rmdir/rename and then increments it back on failure. *If* we really want it that way, we need to use set_nlink() there. Frankly, I'd rather deal with drop_nlink() after the last possible failure exit... Unless there are serious reasons why that wouldn't work, that is. Artem?
Re: [RFC] killing boilerplate checks in ->link/->mkdir/->rename
Quoted 10 lines · Al Viro
Quoted 2 lines · Al Viro
* ubifs, hfsplus, jffs2 - definitely broken if you create enough
links.i_nlinkwraparound to zero, confused inode eviction logics.BTW, ubifs plays funny games with
i_nlink- decrements it early in unlink/rmdir/rename and then increments it back on failure. *If* we really want it that way, we need to useset_nlink()there. Frankly, I'd rather deal withdrop_nlink()after the last possible failure exit... Unless there are serious reasons why that wouldn't work, that is.
There was a good reason for those games. I cannot provide a good explanation right now because I need to refresh my memory (but I am sure there must be a big comment in the code about this) and my daughter is getting upset already because I am typing something instead of playing with her. Will try to answer on Monday. Have a nice weekend!
Signature · 2 lines
Best Regards, Artem Bityutskiy
Re: [RFC] killing boilerplate checks in ->link/->mkdir/->rename
In reply to Al ViroQuoted 11 lines · Al Viro
Quoted 2 lines · Al Viro
* ubifs, hfsplus, jffs2 - definitely broken if you create enough
links.i_nlinkwraparound to zero, confused inode eviction logics.BTW, ubifs plays funny games with
i_nlink- decrements it early in unlink/rmdir/rename and then increments it back on failure. *If* we really want it that way, we need to useset_nlink()there. Frankly, I'd rather deal withdrop_nlink()after the last possible failure exit... Unless there are serious reasons why that wouldn't work, that is. Artem?
The way code is organized is that the UBIFS journal subsystem functions accept 'struct inode' and struct direntry' objects and write them out to the media as they are in RAM. So before calling 'ubifs_jnl_update()' we should have all the fields in 'struct inode' to be set correctly. Thus, we have to drop 'i_nlink' before calling 'ubifs_jnl_update()'.
WRT dealing with 'drop_nlink()' after the last possible failure - I can just make own partial copy of 'struct inode' and pass it to 'ubifs_jnl_update()', if there is a need. I would not like to make the journal code more complex than it already is by adding 'i_nlink' usage smartness.
WRT 'sen_nlink()' - I can use it instead of 'drop_nlink()'/'inc_nlink()' of course. But I do not really see why is this better. E.g., 'drop_nlink()' additionally gives me ' WARN_ON()' in case of 'i_nlink' wrapping.
Signature · 2 lines
Best Regards, Artem Bityutskiy
Al Viro
maintainerRe: [RFC] killing boilerplate checks in ->link/->mkdir/->rename
Quoted 4 lines · Artem Bityutskiy
WRT '
sen_nlink()' - I can use it instead of 'drop_nlink()'/'inc_nlink()' of course. But I do not really see why is this better. E.g., 'drop_nlink()' additionally gives me 'WARN_ON()' in case of 'i_nlink' wrapping.
So does inc_nlink() when you are asking to get from nlink=0 to nlink=1. I.e. on failure exit in your unlink()...
Re: [RFC] killing boilerplate checks in ->link/->mkdir/->rename
Quoted 9 lines · Al Viro
Quoted 4 lines · Artem Bityutskiy
WRT '
sen_nlink()' - I can use it instead of 'drop_nlink()'/'inc_nlink()' of course. But I do not really see why is this better. E.g., 'drop_nlink()' additionally gives me 'WARN_ON()' in case of 'i_nlink' wrapping.So does
inc_nlink()when you are asking to get from nlink=0 to nlink=1. I.e. on failure exit in yourunlink()...
Indeed! I'll switch to 'clear_nlink(inode)' and 'set_nlink(inode, 1)' for those inodes which are being unliked.
Signature · 2 lines
Best Regards, Artem Bityutskiy
Al Viro
maintainerRe: [RFC] killing boilerplate checks in ->link/->mkdir/->rename
Quoted 8 lines · Artem Bityutskiy
Quoted 7 lines · Al Viro
Quoted 4 lines · Artem Bityutskiy
WRT '
sen_nlink()' - I can use it instead of 'drop_nlink()'/'inc_nlink()' of course. But I do not really see why is this better. E.g., 'drop_nlink()' additionally gives me 'WARN_ON()' in case of 'i_nlink' wrapping.
Quoted 5 lines
Quoted 2 lines
So does
inc_nlink()when you are asking to get from nlink=0 to nlink=1. I.e. on failure exit in yourunlink()...Indeed! I'll switch to '
clear_nlink(inode)' and 'set_nlink(inode, 1)' for those inodes which are being unliked.
Huh? I thought ubifs allowed link(2)... IOW, i_nlink could've been greater than 1 when you called ubifs_unlink().
Re: [RFC] killing boilerplate checks in ->link/->mkdir/->rename
Quoted 9 lines · Al Viro
Quoted 8 lines · Artem Bityutskiy
Quoted 7 lines · Al Viro
Quoted 4 lines · Artem Bityutskiy
WRT '
sen_nlink()' - I can use it instead of 'drop_nlink()'/'inc_nlink()' of course. But I do not really see why is this better. E.g., 'drop_nlink()' additionally gives me 'WARN_ON()' in case of 'i_nlink' wrapping.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
So does
inc_nlink()when you are asking to get from nlink=0 to nlink=1. I.e. on failure exit in yourunlink()...Indeed! I'll switch to '
clear_nlink(inode)' and 'set_nlink(inode, 1)' for those inodes which are being unliked.Huh? I thought ubifs allowed link(2)... IOW,
i_nlinkcould've been greater than 1 when you calledubifs_unlink().
You are right, I'll take this correctly when I change the code rather than writing an e-mail. Thanks! I think I'll submit a patch this week.
Signature · 2 lines
Best Regards, Artem Bityutskiy
Re: [RFC] killing boilerplate checks in ->link/->mkdir/->rename
In reply to Al ViroQuoted 3 lines · Al Viro
Quoted 1 line · Linus Torvalds
On Thu, Feb 2, 2012 at 1:24 PM, Al Viro <[email protected]> wrote:
* WTF is XFS doing with these checks?
It is validating nlink against the maximum supported by the XFS on-disk format. It was originally limited by what could be reported to pathconf() on Irix - a signed int. We have that same problem on Linux, too, because on 32 bit systems the maximum number of links that can be reported via pathconf is 2^31....
Quoted 4 lines
Note that we have them
done _twice_ on all paths - explictly fromxfs_create(),xfs_link(),xfs_rename()and then fromxfs_bumplink()called by exactly the same set of functions.
Well, that's a bit stupid, isn't it? Trivial to fix, though...
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [RFC] killing boilerplate checks in ->link/->mkdir/->rename
In reply to Al ViroQuoted 9 lines · Al Viro
FWIW, there's something we really should've done a long time ago: putting that limit into
sb->s_max_links. With 0 meaning "leave all checks to ->link/->mkdir/->rename". Something like the following would make a reasonable start - just the conversion of obvious cases. As the next step I'd probably initialize it as ~0U instead of 0 and let the filesystems that want something trickier (reiserfs, ext4, gfs2, ocfs2) explicitly set it to 0 in theirfoo_fill_super(). That would take care of a bunch of cases where we forgot to do those checks (ubifs, hfsplus, jffs2, ramfs, etc.) and it's probably a saner default anyway.
This would also give userspace some hope of pathconf(path, _PC_LINK_MAX) returning the actual value from the filesystem, instead of hard-coding this into glibc itself based on the statfs-returned f_type magic value.
Cheers, Andreas
Al Viro
maintainerRe: [RFC] killing boilerplate checks in ->link/->mkdir/->rename
Quoted 11 lines · Andreas Dilger
Quoted 9 lines · Al Viro
FWIW, there's something we really should've done a long time ago: putting that limit into
sb->s_max_links. With 0 meaning "leave all checks to ->link/->mkdir/->rename". Something like the following would make a reasonable start - just the conversion of obvious cases. As the next step I'd probably initialize it as ~0U instead of 0 and let the filesystems that want something trickier (reiserfs, ext4, gfs2, ocfs2) explicitly set it to 0 in theirfoo_fill_super(). That would take care of a bunch of cases where we forgot to do those checks (ubifs, hfsplus, jffs2, ramfs, etc.) and it's probably a saner default anyway.
Quoted 3 lines
This would also give userspace some hope of
pathconf(path, _PC_LINK_MAX)returning the actual value from the filesystem, instead of hard-coding this into glibc itself based on the statfs-returnedf_typemagic value.
*snort*
Even skipping the standard flame about pathconf() as an API, this will not work.
* we have filesystems that do not allow link creation at all and
do keep track of subdirectories count in i_nlink of directories. What would you have them store? As it is, ~0U works just fine, but pathconf() users won't be happy with it.
* we have filesystems that allow unlimited subdirectories, while
limiting the number of links to non-directories; ->s_max_links == 0 will work just fine, but won't make pathconf() happy.
* we have filesystems that have more complex rules re links to
non-directory (see mail from Chris in this thread). What would you have pathconf() do?
Re: [RFC] killing boilerplate checks in ->link/->mkdir/->rename
Quoted 18 lines · Al Viro
Quoted 11 lines · Andreas Dilger
Quoted 9 lines · Al Viro
FWIW, there's something we really should've done a long time ago: putting that limit into
sb->s_max_links. With 0 meaning "leave all checks to ->link/->mkdir/->rename". Something like the following would make a reasonable start - just the conversion of obvious cases. As the next step I'd probably initialize it as ~0U instead of 0 and let the filesystems that want something trickier (reiserfs, ext4, gfs2, ocfs2) explicitly set it to 0 in theirfoo_fill_super(). That would take care of a bunch of cases where we forgot to do those checks (ubifs, hfsplus, jffs2, ramfs, etc.) and it's probably a saner default anyway.Quoted 3 lines
This would also give userspace some hope of
pathconf(path, _PC_LINK_MAX)returning the actual value from the filesystem, instead of hard-coding this into glibc itself based on the statfs-returnedf_typemagic value.*snort*
Quoted 12 lines
Even skipping the standard flame about
pathconf()as an API, this will not work.
* we have filesystems that do not allow link creation at all and
do keep track of subdirectories count ini_nlinkof directories. What would you have them store? As it is, ~0U works just fine, butpathconf()users won't be happy with it.
* we have filesystems that allow unlimited subdirectories, while
limiting the number of links to non-directories; ->s_max_links == 0 will work just fine, but won't makepathconf()happy.
* we have filesystems that have more complex rules re links to
non-directory (see mail from Chris in this thread). What would you havepathconf()do?
No comment on how good an API pathconf() is, but getting a per-filesystem value from the kernel has to be better than a hard-coded value coded in a library in userspace.
Cheers, Andreas
Re: [PATCH] sysfs: Optionally count subdirectories to support buggy applications
In reply to Linus TorvaldsQuoted 14 lines · Linus Torvalds
Quoted 4 lines · Linus Torvalds
No extra "keep track of inode counts by hand" crap, and no idiotic config options that just make it easy to (conditionally) get things wrong. Just do it right, and do it *unconditionally* right.
And btw, "nlink shows number of subdirectories" for a directory entry really *is* right. It's how Unix filesystems work, like it or not.
It's mainly lazy/bad filesystems that set nlink to 1. So the whole "nlink==1" case is meant for crap like FAT etc, not for a filesystem that we control and that could easily just do it right.
Quoted 5 lines
Which is why I detest that config option. It's as if you were asking the user
"Do you want to make the sysfs filesystem act like crap filesystems?"
and kernel config time. What kind of inane question is that?
<thread resumed...>
What's going on here? I still have to revert "sysfs: Kill nlink counting." with today's -next to have working sensors.
thanks,
Signature · 2 lines
js suse labs
Greg Kroah-Hartman
maintainerRe: [PATCH] sysfs: Optionally count subdirectories to support buggy applications
Quoted 17 lines · Jiri Slaby
Quoted 16 lines · Linus Torvalds
Quoted 4 lines · Linus Torvalds
No extra "keep track of inode counts by hand" crap, and no idiotic config options that just make it easy to (conditionally) get things wrong. Just do it right, and do it *unconditionally* right.
And btw, "nlink shows number of subdirectories" for a directory entry really *is* right. It's how Unix filesystems work, like it or not.
It's mainly lazy/bad filesystems that set nlink to 1. So the whole "nlink==1" case is meant for crap like FAT etc, not for a filesystem that we control and that could easily just do it right.
Which is why I detest that config option. It's as if you were asking the user
Quoted 8 lines
Quoted 3 lines
"Do you want to make the sysfs filesystem act like crap filesystems?"
and kernel config time. What kind of inane question is that?
<thread resumed...>
What's going on here? I still have to revert "sysfs: Kill nlink counting." with today's -next to have working sensors.
I don't remember. I thought there was a proposed patch for this issue from Eric, but I don't see it in my queue anywhere.
Eric, what was the resolution here?
greg k-h
Re: [PATCH] sysfs: Optionally count subdirectories to support buggy applications
Quoted 3 lines · Greg Kroah-Hartman
I don't remember. I thought there was a proposed patch for this issue from Eric, but I don't see it in my queue anywhere.
That patch was an abortion. Adding a config option for behavior like this is totally bogus, and the only reason for that config option was that sysfs did silly things.
It's only in -next, though, I was assuming that the whole "Kill nlink counting" commit never makes it to me. Because I won't take it.
I outlined how the counting could easily be done without actually having to maintain an explicit count in the sysfs.
Or we should just keep doing the counting.
Linus
Greg Kroah-Hartman
maintainerRe: [PATCH] sysfs: Optionally count subdirectories to support buggy applications
Quoted 10 lines · Linus Torvalds
Quoted 3 lines · Greg Kroah-Hartman
I don't remember. I thought there was a proposed patch for this issue from Eric, but I don't see it in my queue anywhere.
That patch was an abortion. Adding a config option for behavior like this is totally bogus, and the only reason for that config option was that sysfs did silly things.
Quoted 7 lines
It's only in -next, though, I was assuming that the whole "Kill nlink counting" commit never makes it to me. Because I won't take it.
I outlined how the counting could easily be done without actually having to maintain an explicit count in the sysfs.
Or we should just keep doing the counting.
Ok, I've now reverted this patch in my tree.
greg k-h
Re: [PATCH] sysfs: Optionally count subdirectories to support buggy applications
In reply to Linus TorvaldsQuoted 9 lines · Linus Torvalds
Quoted 3 lines · Greg Kroah-Hartman
I don't remember. I thought there was a proposed patch for this issue from Eric, but I don't see it in my queue anywhere.
That patch was an abortion. Adding a config option for behavior like this is totally bogus, and the only reason for that config option was that sysfs did silly things.
The biggest reason it is bogus is that it doesn't get properly tested or reviewed. Sigh. My first patch to fix things had a bad typo that everyone missed.
Quoted 5 lines
It's only in -next, though, I was assuming that the whole "Kill nlink counting" commit never makes it to me. Because I won't take it.
I outlined how the counting could easily be done without actually having to maintain an explicit count in the sysfs.
And if you had bothered to look you would have seen how we used to have that code and it was removed because it was a performance bottleneck.
Quoted 1 line
Or we should just keep doing the counting.
The current counting that we do gives the wrong numbers, in the edge cases. To my knowledge a deleted sysfs directory has never returned nlink == 0.
Keeping compatibility is easy enough that it looks like it is worth doing, but maintaining 30+ years of backwards compatibility is what nlink >1 in unix filesystem directories is. I don't see any practical sense in keeping . and .. directories on disk or upping the unix nlink directory count because of them. To me it looks like just one of those things you do. Like hash directory entries so you can have a big directory and still be able to have a 32bit offset you can pass to lseek that is stable across renames and deletes.
Quoted 1 line
From the point of view of maintaining sysfs a 32bit
nlink_tin sysfs is
too small. It is wrong for sysfs to refuse to represent devices that exist and I have heard of machines that have enough memory it possible to create more than 2^32 network devices. So sysfs must handle overflow and sysfs must use the nlink == 1 in some cases. I was just thinking we would get better userspace test coverage if we don't bother to handle the other cases.
Eric
Re: [PATCH] sysfs: Optionally count subdirectories to support buggy applications
Quoted 3 lines · Eric W. Biederman
Keeping compatibility is easy enough that it looks like it is worth doing, but maintaining 30+ years of backwards compatibility
Stop right there.
This is *not* about some arbitrary "30-year backwards compatibility".
This is about your patch BREAKING EXISTING BINARIES.
So stop the f*&^ing around already. The patch was shown to be broken, stop making excuses, and stop blathering.
End of story. Binary compatibility is more important than *any* of your patches. If you continue to argue anything else or making excuses, I'm going to ask people to just ignore your patches entirely.
Seriously. Binary compatibility is *so* important that I do not want to have anything to do with kernel developers who don't understand that importance. If you continue to pooh-pooh the issue, you only show yourself to be unreliable. Don't do it.
Dammit, I'm continually surprised by the *idiots* out there that don't understand that binary compatibility is one of the absolute top priorities. The *only* reason for an OS kernel existing in the first place is to serve user-space. The kernel has no relevance on its own. Breaking existing binaries - and then not acknowledging how horribly bad that was - is just about the *worst* offense any kernel developer can do.
Because that shows that they don't understand what the whole *point* of the kernel was after all. We're not masturbating around with some research project. We never were. Even when Linux was young, the whole and only point was to make a *usable* system. It's why it's not some crazy drug-induced microkernel or other random crazy thing.
Really.
Linus
Re: [PATCH] sysfs: Optionally count subdirectories to support buggy applications
In reply to Greg Kroah-HartmanQuoted 24 lines · Greg Kroah-Hartman
Quoted 23 lines · Jiri Slaby
Quoted 14 lines · Linus Torvalds
Quoted 4 lines · Linus Torvalds
No extra "keep track of inode counts by hand" crap, and no idiotic config options that just make it easy to (conditionally) get things wrong. Just do it right, and do it *unconditionally* right.
And btw, "nlink shows number of subdirectories" for a directory entry really *is* right. It's how Unix filesystems work, like it or not.
It's mainly lazy/bad filesystems that set nlink to 1. So the whole "nlink==1" case is meant for crap like FAT etc, not for a filesystem that we control and that could easily just do it right.
Quoted 5 lines
Which is why I detest that config option. It's as if you were asking the user
"Do you want to make the sysfs filesystem act like crap filesystems?"
and kernel config time. What kind of inane question is that?
<thread resumed...>
Quoted 7 lines
Quoted 2 lines
What's going on here? I still have to revert "sysfs: Kill nlink counting." with today's -next to have working sensors.
I don't remember. I thought there was a proposed patch for this issue from Eric, but I don't see it in my queue anywhere.
Eric, what was the resolution here?
Apologies. Cold/Allergies and distractions have kept it away. sysfs patches in a follow up.
Eric
PATCH 1/3 sysfs: Compact sysfs_dirent s_flags into a byte.
In reply to Greg Kroah-HartmanIn an effort to keep sysfs_dirent small used the smallest basic type I can for sysfs s_flags.
--- fs/sysfs/sysfs.h | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
fs/sysfs/sysfs.h +5 -5
@@ -76,7 +76,7 @@ struct sysfs_dirent {struct sysfs_elem_bin_attr s_bin_attr; }; -unsigned short s_flags; +unsigned char s_flags;umode_t s_mode;unsigned int s_ino;struct sysfs_inode_attrs *s_iattr;
@@ -84,7 +84,7 @@ struct sysfs_dirent {#define SD_DEACTIVATED_BIAS INT_MIN -#define SYSFS_TYPE_MASK 0x00ff +#define SYSFS_TYPE_MASK 0x000f#define SYSFS_DIR 0x0001#define SYSFS_KOBJ_ATTR 0x0002#define SYSFS_KOBJ_BIN_ATTR 0x0004
@@ -93,11 +93,11 @@ struct sysfs_dirent {#define SYSFS_ACTIVE_REF (SYSFS_KOBJ_ATTR | SYSFS_KOBJ_BIN_ATTR)/* identify any namespace tag on sysfs_dirents */ -#define SYSFS_NS_TYPE_MASK 0xf00 -#define SYSFS_NS_TYPE_SHIFT 8 +#define SYSFS_NS_TYPE_MASK 0x70 +#define SYSFS_NS_TYPE_SHIFT 4#define SYSFS_FLAG_MASK ~(SYSFS_NS_TYPE_MASK|SYSFS_TYPE_MASK) -#define SYSFS_FLAG_REMOVED 0x02000 +#define SYSFS_FLAG_REMOVED 0x080static inline unsigned int sysfs_type (struct sysfs_dirent *sd) {
Signature · 1 line
1.7.2.5
PATCH 2/3 sysfs: Maintain usable nlink directory counts.
When it is easy keep fully accurate nlink counts for sysfs directories, when it is not easy set nlink to 1. This maintains as much compatibility with unix programs that expect directories to have a usable nlink count as possible, without trying to do the impossible.
Directory nlink count overflows in sysfs are inevitable by design so only bother to use a byte to store the directory nlink count. A directory with 254 entries is larger than any sysfs directory in a normal configruation but small enough we should see tools that care about large sysfs directories should experience nlink == 1 during their testing.
This fixes libsensors and possibly other applications that get confused if all sysfs directories return nlink == 1. The lm_sensors code that got confused was just wrong is fixed in the lm_sensors trunk and a fixed version should be released sometime soon.
The nlink of all deleted directories is set to 0. Returning for the first time a correct nlink count for deleted sysfs directories.
Once a directory nlink count drops below 2 refuse to increment or decrement it as there is not enough information available. It is important that this works for nlink == 0 as well as nlink == 1 because currently
sysfs supports deleting non-empty directories (the PCI layer requires this behavior).
For tagged directories set the nlink count == 1 because we currently have one sysfs_dirent and multiple logical sysfs directories making pre computing nlink impossible.
--- fs/sysfs/dir.c | 14 ++++++++++++++ fs/sysfs/inode.c | 1 + fs/sysfs/sysfs.h | 16 ++++++++++++++++ 3 files changed, 31 insertions(+), 0 deletions(-)
fs/sysfs/dir.c +14 -0
@@ -91,6 +91,9 @@ static int sysfs_link_sibling(struct sysfs_dirent *sd)struct rb_node **node = &sd->s_parent->s_dir.children.rb_node;struct rb_node *parent =NULL ; +if (sysfs_type (sd) ==SYSFS_DIR ) +sysfs_inc_nlink (sd->s_parent); +while (*node) {struct sysfs_dirent *pos;int result;
@@ -123,6 +126,9 @@ static int sysfs_link_sibling(struct sysfs_dirent *sd)*/ static void sysfs_unlink_sibling (struct sysfs_dirent *sd) { +if (sysfs_type (sd) ==SYSFS_DIR ) +sysfs_dec_nlink (sd->s_parent); +rb_erase (&sd->s_rb, &sd->s_parent->s_dir.children); }
@@ -366,6 +372,9 @@ 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 =1 ; +if (sysfs_type (sd) ==SYSFS_DIR ) + sd->s_nlink =2 ;return sd;
@@ -536,6 +545,7 @@ void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd) ps_iattrs->ia_ctime = ps_iattrs->ia_mtime =CURRENT_TIME ; } + sd->s_nlink =0 ; sd->s_flags |=SYSFS_FLAG_REMOVED ; sd->u.removed_list = acxt->removed; acxt->removed = sd;
@@ -660,6 +670,10 @@ static int create_dir(struct kobject *kobj, struct sysfs_dirent *parent_sd, sd->s_ns = ns; sd->s_dir.kobj = kobj; +/* Accurate nlink count impossible (one field mutiple dirs) */ +if (sysfs_ns_type (sd)) + sd->s_nlink =1 ; +/* link in */ sysfs_addrm_start (&acxt, parent_sd); rc =sysfs_add_one (&acxt, sd);
fs/sysfs/inode.c +1 -0
@@ -216,6 +216,7 @@ static void sysfs_refresh_inode(struct sysfs_dirent *sd, struct inode *inode) iattrs->ia_secdata, iattrs->ia_secdata_len); } +set_nlink (inode, sd->s_nlink); }int sysfs_getattr (struct vfsmount *mnt,struct dentry *dentry,struct kstat *stat)
fs/sysfs/sysfs.h +16 -0
@@ -76,6 +76,7 @@ struct sysfs_dirent {struct sysfs_elem_bin_attr s_bin_attr; }; +unsigned char s_nlink;unsigned char s_flags;umode_t s_mode;unsigned int s_ino;
@@ -127,6 +128,21 @@ do { \#define sysfs_dirent_init_lockdep (sd) do {} while(0)#endif +static inline void sysfs_inc_nlink (struct sysfs_dirent *sd) +{ +if (sd->s_nlink <=1 ) +return ; + sd->s_nlink++; +if (sd->s_nlink ==0 ) + sd->s_nlink =1 ; +} +static inline void sysfs_dec_nlink (struct sysfs_dirent *sd) +{ +if (sd->s_nlink <=1 ) +return ; + sd->s_nlink--; +} +/* * Context structure to be used while adding/removing nodes. */
Signature · 1 line
1.7.2.5
PATCH 3/3 sysfs: Remove SYSFS_FLAG_REMOVED and use sd->s_nlink == 0 instead.
Now that we have a nlink field in sysfs_dirent report deleted files and directories the traditional way with nlink == 0.
--- fs/sysfs/dir.c | 11 +++++------ fs/sysfs/sysfs.h | 1 - 2 files changed, 5 insertions(+), 7 deletions(-)
fs/sysfs/dir.c +5 -6
@@ -202,7 +202,7 @@ static void sysfs_deactivate(struct sysfs_dirent *sd)DECLARE_COMPLETION_ONSTACK (wait);int v; -BUG_ON (!(sd->s_flags &SYSFS_FLAG_REMOVED )); +BUG_ON (sd->s_nlink !=0 );if (!(sysfs_type (sd) &SYSFS_ACTIVE_REF ))return ;
@@ -279,7 +279,7 @@ void release_sysfs_dirent(struct sysfs_dirent * sd)static int sysfs_dentry_delete (const struct dentry *dentry) {struct sysfs_dirent *sd = dentry->d_fsdata; -return !!(sd->s_flags &SYSFS_FLAG_REMOVED ); +return sd->s_nlink ==0 ; }static int sysfs_dentry_revalidate (struct dentry *dentry,struct nameidata *nd)
@@ -294,7 +294,7 @@ static int sysfs_dentry_revalidate(struct dentry *dentry, struct nameidata *nd)mutex_lock (&sysfs_mutex);/* The sysfs dirent has been deleted */ -if (sd->s_flags &SYSFS_FLAG_REMOVED ) +if (sd->s_nlink ==0 ) goto out_bad;/* The sysfs dirent has been moved? */
@@ -534,7 +534,7 @@ void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd) {struct sysfs_inode_attrs *ps_iattr; -BUG_ON (sd->s_flags &SYSFS_FLAG_REMOVED ); +BUG_ON (sd->s_nlink ==0 );sysfs_unlink_sibling (sd);
@@ -546,7 +546,6 @@ void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd) } sd->s_nlink =0 ; - sd->s_flags |=SYSFS_FLAG_REMOVED ; sd->u.removed_list = acxt->removed; acxt->removed = sd; }
@@ -946,7 +945,7 @@ static struct sysfs_dirent *sysfs_dir_pos(const void *ns, struct sysfs_dirent *parent_sd, loff_t hash, struct sysfs_dirent *pos) { if (pos) { - int valid = !(pos->s_flags & SYSFS_FLAG_REMOVED) && + int valid = (pos->s_nlink > 0) && pos->s_parent == parent_sd && hash == pos->s_hash;sysfs_put (pos);
fs/sysfs/sysfs.h +0 -1
@@ -98,7 +98,6 @@ struct sysfs_dirent {#define SYSFS_NS_TYPE_SHIFT 4#define SYSFS_FLAG_MASK ~(SYSFS_NS_TYPE_MASK|SYSFS_TYPE_MASK) -#define SYSFS_FLAG_REMOVED 0x080static inline unsigned int sysfs_type (struct sysfs_dirent *sd) {
Signature · 1 line
1.7.2.5
Re: [PATCH 3/3] sysfs: Remove SYSFS_FLAG_REMOVED and use sd->s_nlink == 0 instead.
Is this safe?
What's the serialization with sysfs_link_sibling(), which you just made do
sd->s_nlink++;
if (!sd->s_nlink)
sd->s_nlink = 1;in your previous patch?
Yes, sysfs_link_sibling() runs under sysfs_mutex, but sysfs_dentry_delete() does not.
So what protects sysfs_dentry_delete() from never seeing that bogus 0 due to the overflow?
Linus
Quoted 82 lines · Eric W. Biederman
Now that we have a nlink field in
sysfs_direntreport deleted files and directories the traditional way with nlink == 0.Signed-off-by Eric W. Biederman <[email protected]>---
fs/sysfs/dir.c | 11 +++++------
fs/sysfs/sysfs.h | 1 -2 files changed, 5 insertions(+), 7 deletions(-)fs/sysfs/dir.c +5
-6@@ -202,7 +202,7 @@ static void sysfs_deactivate(struct sysfs_dirent *sd) DECLARE_COMPLETION_ONSTACK(wait); int v; - BUG_ON(!(sd->s_flags & SYSFS_FLAG_REMOVED)); + BUG_ON(sd->s_nlink != 0); if (!(sysfs_type(sd) & SYSFS_ACTIVE_REF)) return;@@ -279,7 +279,7 @@ void release_sysfs_dirent(struct sysfs_dirent * sd) static int sysfs_dentry_delete(const struct dentry *dentry) { struct sysfs_dirent *sd = dentry->d_fsdata; - return !!(sd->s_flags & SYSFS_FLAG_REMOVED); + return sd->s_nlink == 0; } static int sysfs_dentry_revalidate(struct dentry *dentry, struct nameidata *nd)@@ -294,7 +294,7 @@ static int sysfs_dentry_revalidate(struct dentry *dentry, struct nameidata *nd) mutex_lock(&sysfs_mutex); /* The sysfs dirent has been deleted */ - if (sd->s_flags & SYSFS_FLAG_REMOVED) + if (sd->s_nlink == 0) goto out_bad; /* The sysfs dirent has been moved? */@@ -534,7 +534,7 @@ void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd) { struct sysfs_inode_attrs *ps_iattr; - BUG_ON(sd->s_flags & SYSFS_FLAG_REMOVED); + BUG_ON(sd->s_nlink == 0); sysfs_unlink_sibling(sd);@@ -546,7 +546,6 @@ void sysfs_remove_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd) } sd->s_nlink = 0; - sd->s_flags |= SYSFS_FLAG_REMOVED; sd->u.removed_list = acxt->removed; acxt->removed = sd; }@@ -946,7 +945,7 @@ static struct sysfs_dirent *sysfs_dir_pos(const void *ns, struct sysfs_dirent *parent_sd, loff_t hash, struct sysfs_dirent *pos) { if (pos) { - int valid = !(pos->s_flags & SYSFS_FLAG_REMOVED) && + int valid = (pos->s_nlink > 0) &&pos->s_parent ==
parent_sd&& hash ==pos->s_hash;sysfs_put(pos);fs/sysfs/sysfs.h +0
-2@@ -98,7 +98,6 @@ struct sysfs_dirent { #define SYSFS_NS_TYPE_SHIFT 4 #define SYSFS_FLAG_MASK ~(SYSFS_NS_TYPE_MASK|SYSFS_TYPE_MASK) -#define SYSFS_FLAG_REMOVED 0x080 static inline unsigned int sysfs_type(struct sysfs_dirent *sd) { --1.7.2.5
Greg Kroah-Hartman
maintainerRe: [PATCH 1/3] sysfs: Compact sysfs_dirent s_flags into a byte.
In reply to Eric W. BiedermanQuoted 14 lines · Eric W. Biederman
In an effort to keep
sysfs_direntsmall used the smallest basic type I can for sysfss_flags.Signed-off-by Eric W. Biederman <[email protected]>--- fs/sysfs/sysfs.h | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)fs/sysfs/sysfs.h +0
-0
fs/sysfs/sysfs.h struct sysfs_dirent { · line 82
Quoted 8 lines
fs/sysfs/sysfs.h +1
-1@@ -76,7 +76,7 @@ struct sysfs_dirent {struct sysfs_elem_bin_attr s_bin_attr; }; -unsigned short s_flags; +unsigned char s_flags;umode_t s_mode;unsigned int s_ino;struct sysfs_inode_attrs *s_iattr;
Are you sure this saved you any real space here? Have you use pahole (I think that's the tool name) to determine if there are holes in the structure? Given that you moved this to a smaller variable, yet there are pointers later on, odds are nothing changed at all overall.
Verification would be good to have, to see if this work was really worth it, right?
thanks,
greg k-h
Re: [PATCH 1/3] sysfs: Compact sysfs_dirent s_flags into a byte.
Quoted 29 lines · Greg Kroah-Hartman
Quoted 14 lines · Eric W. Biederman
In an effort to keep
sysfs_direntsmall used the smallest basic type I can for sysfss_flags.Signed-off-by Eric W. Biederman <[email protected]>--- fs/sysfs/sysfs.h | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)fs/sysfs/sysfs.h +0
-0fs/sysfs/sysfs.h struct sysfs_dirent { · line 82
Quoted 8 lines
fs/sysfs/sysfs.h +1
-1@@ -76,7 +76,7 @@ struct sysfs_dirent {struct sysfs_elem_bin_attr s_bin_attr; }; -unsigned short s_flags; +unsigned char s_flags;umode_t s_mode;unsigned int s_ino;struct sysfs_inode_attrs *s_iattr;Are you sure this saved you any real space here? Have you use pahole (I think that's the tool name) to determine if there are holes in the structure? Given that you moved this to a smaller variable, yet there are pointers later on, odds are nothing changed at all overall.
Quoted 2 lines
Verification would be good to have, to see if this work was really worth it, right?
The next patch stuffs an 8bit nlink in the hole. I was making room so we don't have to grow sysfs_dirent.
Eric
Quoted 6 lines · Alan Cox
Quoted 2 lines
Isn't there some other "proper" way of doing this in userspace, or is this really the correct way?
You can look at the
S_IFMTbits and stuff however link count indicating number of subdirectories is a standard Unix thing and used by many quite mundane tools as an optimisation.
Those tools for a decade or better all know to treat nlink == 1 as the case where the optimization does not apply.
On a traditional unix filesytem the most common place you will see nlink == 1 is when the link count overflows. extN with > 65536 subdirectories as I recall.
Eric
Quoted 11 lines · On Mon, 30 Jan 2012 19:45:10 -0800 [email protected] (Eric W. Biederman)…
Quoted 6 lines · Alan Cox
Quoted 2 lines
Isn't there some other "proper" way of doing this in userspace, or is this really the correct way?
You can look at the
S_IFMTbits and stuff however link count indicating number of subdirectories is a standard Unix thing and used by many quite mundane tools as an optimisation.Those tools for a decade or better all know to treat nlink == 1 as the case where the optimization does not apply.
Most of the do. but as has been demonstrated here - not all
2012/1/30 Jiri Slaby <[email protected]>:
Quoted 9 lines
I cannot boot properly with this commit:
commit524b6c5b39b931311dfe5a2f5abae2f5c9731676
Author: Eric W. Biederman <[email protected]>
Date: Sun Dec 18 20:09:31 2011 -0800sysfs: Kill nlink counting.
1) network systemd rule doesn't start network
What does that mean? What's a network systemd rule?
Kay
network regression: cannot rename netdev twice
Quoted 6 lines · Kay Sievers
2012/1/30 Jiri Slaby <[email protected]>:
Quoted 5 lines
I cannot boot properly with this commit:
commit524b6c5b39b931311dfe5a2f5abae2f5c9731676Author: Eric W. Biederman <[email protected]> Date: Sun Dec 18 20:09:31 2011 -0800
Quoted 6 lines
Quoted 4 lines
sysfs: Kill nlink counting.
1) network systemd rule doesn't start network
What does that mean? What's a network systemd rule?
Oh, perhaps you call it a service file, not rule file?
Anyway this is a different bug. Revert of the patch above does not help.
The bug lays in the network layer. udev is unable to perform persistent eth naming:
# ip link set eth0 name eth1 -- this one is OK # ip link set eth1 name eth0 RTNETLINK answers: No such file or directory
thanks,
Signature · 2 lines
js suse labs
Re: network regression: cannot rename netdev twice
Quoted 9 lines · Jiri Slaby
Quoted 8 lines · Kay Sievers
2012/1/30 Jiri Slaby <[email protected]>:
Quoted 7 lines
I cannot boot properly with this commit:
commit524b6c5b39b931311dfe5a2f5abae2f5c9731676
Author: Eric W. Biederman <[email protected]>
Date: Sun Dec 18 20:09:31 2011 -0800sysfs: Kill nlink counting.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
1) network systemd rule doesn't start network
What does that mean? What's a network systemd rule?
Oh, perhaps you call it a service file, not rule file?
Anyway this is a different bug. Revert of the patch above does not help.
Ok, fine. I checked too, and systemd does not play any silly games with link counts.
Quoted 5 lines
The bug lays in the network layer. udev is unable to perform persistent eth naming:
# ip link set eth0 name eth1 -- this one is OK # ip link set eth1 name eth0 RTNETLINK answers: No such file or directory
Please make sure nothing tries to swap netif names in userspace. We have given up that approach, because it is far too fragile to temporary rename devices to be able to swap the names, and race against the loading of new kernel network drivers at the same time.
This might be a new kernel problem here, but in general that approach is just broken, we have have given up fiddling around here. Udev does not do that anymore, and also the code that currently *can* be used to do this, will be removed from udev in the future.
Network devices can only be renamed to a namespace that isn't ethX, and which does not race against kernel names.
Does is work, if you rename the devices to something else than ethX?
Kay
Re: network regression: cannot rename netdev twice
Quoted 32 lines · Kay Sievers
Quoted 9 lines · Jiri Slaby
Quoted 8 lines · Kay Sievers
2012/1/30 Jiri Slaby <[email protected]>:
Quoted 7 lines
I cannot boot properly with this commit:
commit524b6c5b39b931311dfe5a2f5abae2f5c9731676Author: Eric W. Biederman <[email protected]> Date: Sun Dec 18 20:09:31 2011 -0800sysfs: Kill nlink counting.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
1) network systemd rule doesn't start network
What does that mean? What's a network systemd rule?
Oh, perhaps you call it a service file, not rule file?
Anyway this is a different bug. Revert of the patch above does not help.
Ok, fine. I checked too, and systemd does not play any silly games with link counts.
Quoted 5 lines
The bug lays in the network layer. udev is unable to perform persistent eth naming:
# ip link set eth0 name eth1 -- this one is OK # ip link set eth1 name eth0 RTNETLINK answers: No such file or directoryPlease make sure nothing tries to swap netif names in userspace. We have given up that approach, because it is far too fragile to temporary rename devices to be able to swap the names, and race against the loading of new kernel network drivers at the same time.
Quoted 7 lines
This might be a new kernel problem here, but in general that approach is just broken, we have have given up fiddling around here. Udev does not do that anymore, and also the code that currently *can* be used to do this, will be removed from udev in the future.
Network devices can only be renamed to a namespace that isn't ethX, and which does not race against kernel names.
I have two eth interfaces. The one on the motherboard is named eth0, an added PCI card is eth1. But kernel enumerates them in the opposite order.
So udev does this sequence:
eth1 -> rename3
eth0 -> eth1
rename3 -> eth0
How it can do it differently? (This is openSUSE factory.)
Quoted 1 line
Does is work, if you rename the devices to something else than ethX?
Negative:
# ip link set eth0 name krtek # ip link set krtek name jezek RTNETLINK answers: No such file or directory
thanks,
Signature · 2 lines
js suse labs
Re: network regression: cannot rename netdev twice
Quoted 44 lines · Jiri Slaby
Quoted 32 lines · Kay Sievers
Quoted 9 lines · Jiri Slaby
Quoted 8 lines · Kay Sievers
2012/1/30 Jiri Slaby <[email protected]>:
Quoted 7 lines
I cannot boot properly with this commit:
commit524b6c5b39b931311dfe5a2f5abae2f5c9731676
Author: Eric W. Biederman <[email protected]>
Date: Sun Dec 18 20:09:31 2011 -0800sysfs: Kill nlink counting.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
1) network systemd rule doesn't start network
What does that mean? What's a network systemd rule?
Oh, perhaps you call it a service file, not rule file?
Anyway this is a different bug. Revert of the patch above does not help.
Ok, fine. I checked too, and systemd does not play any silly games with link counts.
Quoted 5 lines
The bug lays in the network layer. udev is unable to perform persistent eth naming:
# ip link set eth0 name eth1 -- this one is OK # ip link set eth1 name eth0 RTNETLINK answers: No such file or directoryPlease make sure nothing tries to swap netif names in userspace. We have given up that approach, because it is far too fragile to temporary rename devices to be able to swap the names, and race against the loading of new kernel network drivers at the same time.
Quoted 7 lines
This might be a new kernel problem here, but in general that approach is just broken, we have have given up fiddling around here. Udev does not do that anymore, and also the code that currently *can* be used to do this, will be removed from udev in the future.
Network devices can only be renamed to a namespace that isn't ethX, and which does not race against kernel names.
I have two eth interfaces. The one on the motherboard is named eth0, an added PCI card is eth1. But kernel enumerates them in the opposite order.
Quoted 6 lines
So udev does this sequence:
eth1 -> rename3
eth0 -> eth1
rename3 -> eth0How it can do it differently? (This is openSUSE factory.)
A future udev will not help you doing that. We have given up supporting this approach. Renaming is done during booting, at the same time we load new kernel drivers, and all breaks in non-interesting ways. Apart from all the other unsolvable problems with this model.
Pretending we are able to rename netif names in the same namespace the kernel is allocating new names is just plain wrong. There are races you can't control. The entire approach creates far more problems than it solves. We just have to admit it was wrong to do that. Custom/to-rename netif names can just not be ethX.
Quoted 6 lines
Quoted 1 line
Does is work, if you rename the devices to something else than ethX?
Negative:
# ip link set eth0 name krtek # ip link set krtek name jezek RTNETLINK answers: No such file or directory
This is a command sequence you type manually?
You are sure that userspace is not working in the background, triggered by uevents, and comes into your way here?
Kay
Re: network regression: cannot rename netdev twice
Quoted 8 lines · Kay Sievers
Quoted 6 lines
Quoted 1 line
Does is work, if you rename the devices to something else than ethX?
Negative:
# ip link set eth0 name krtek # ip link set krtek name jezek RTNETLINK answers: No such file or directoryThis is a command sequence you type manually?
Yea, and it is working with 3.3.0-rc1-next-20120124_64+. Not with 3.3.0-rc1-next-20120131_64+.
Quoted 2 lines
You are sure that userspace is not working in the background, triggered by uevents, and comes into your way here?
Note that krtek exists after the first command. But cannot be renamed further.
thanks,
Signature · 2 lines
js suse labs
Re: network regression: cannot rename netdev twice
Quoted 4 lines · Jiri Slaby
Quoted 1 line · Kay Sievers
This is a command sequence you type manually?
Quoted 8 lines
Yea, and it is working with 3.3.0-rc1-next-20120124_64+. Not with 3.3.0-rc1-next-20120131_64+.
Quoted 2 lines
You are sure that userspace is not working in the background, triggered by uevents, and comes into your way here?
Note that krtek exists after the first command. But cannot be renamed further.
Yeah, I can confirm the problem here. I works fine with earlier kernels and fails with the latest -next:
# uname -r 3.3.0-rc1-next-20120131+ # modprobe dummy # ip link set dummy0 name foo0 # ip link set foo0 name bar0 RTNETLINK answers: No such file or directory
Kay
Re: network regression: cannot rename netdev twice
Quoted 17 lines · Kay Sievers
Quoted 4 lines · Jiri Slaby
Quoted 1 line · Kay Sievers
This is a command sequence you type manually?
Quoted 8 lines
Yea, and it is working with 3.3.0-rc1-next-20120124_64+. Not with 3.3.0-rc1-next-20120131_64+.
Quoted 2 lines
You are sure that userspace is not working in the background, triggered by uevents, and comes into your way here?
Note that krtek exists after the first command. But cannot be renamed further.
Yeah, I can confirm the problem here. I works fine with earlier kernels and fails with the latest -next:
Quoted 6 lines
# uname -r 3.3.0-rc1-next-20120131+ # modprobe dummy # ip link set dummy0 name foo0 # ip link set foo0 name bar0 RTNETLINK answers: No such file or directory
There is something weird going on when sysfs directories and symlinks are renamed. My guess is that I fat fingered something with one of my last sysfs patches. I will look more deeply once I have slept some more.
The second network device renames fails because the first rename did not work properly. ls -l /sys/class/net/ /sys/virtual/net/ will let you see what I mean.
Eric
PATCH 1/1 sysfs: Update the name hash when renaming sysfs entries
This fixes a bug introduced with sysfs name hashes where renaming a network device appears to succeed but silently makes the sysfs files for that network device inaccessible.
In at least one configuration this bug has stopped networking from coming up during boot.
--- fs/sysfs/dir.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
fs/sysfs/dir.c +1 -0
@@ -872,6 +872,7 @@ int sysfs_rename(struct sysfs_dirent *sd, dup_name = sd->s_name; sd->s_name = new_name; + sd->s_hash =sysfs_name_hash (sd->s_ns, sd->s_name); }/* Move to the appropriate place in the appropriate directories rbtree. */
Signature · 1 line
1.7.2.5
Re: [PATCH] sysfs: Update the name hash when renaming sysfs entries
Quoted 9 lines · Eric W. Biederman
This fixes a bug introduced with sysfs name hashes where renaming a network device appears to succeed but silently makes the sysfs files for that network device inaccessible.
In at least one configuration this bug has stopped networking from coming up during boot.
Signed-off-by Eric W. Biederman <[email protected]>
It works for me. Thanks.
Quoted 16 lines
--- fs/sysfs/dir.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)fs/sysfs/dir.c +1
-0@@ -872,6 +872,7 @@ int sysfs_rename(struct sysfs_dirent *sd, dup_name = sd->s_name; sd->s_name = new_name; + sd->s_hash =sysfs_name_hash (sd->s_ns, sd->s_name); }/* Move to the appropriate place in the appropriate directories rbtree. */
Signature · 2 lines
js suse labs
Re: [PATCH] sysfs: Update the name hash when renaming sysfs entries
In reply to Eric W. BiedermanQuoted 5 lines · Eric W. Biederman
This fixes a bug introduced with sysfs name hashes where renaming a network device appears to succeed but silently makes the sysfs files for that network device inaccessible.
Quoted 7 lines
In at least one configuration this bug has stopped networking from coming up during boot.
Signed-off-by Eric W. Biederman <[email protected]>--- fs/sysfs/dir.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
Thanks for this, I'll queue it up later today.
greg k-h
Re: network regression: cannot rename netdev twice
In reply to Kay SieversQuoted 4 lines · Kay Sievers
Please make sure nothing tries to swap netif names in userspace. We have given up that approach, because it is far too fragile to temporary rename devices to be able to swap the names, and race against the loading of new kernel network drivers at the same time.
That's a damn fair reason, but the loss of that functionality could cause trouble. In fact, at first glance, to me it looks like this has a large potential for unleashing untold pain and suffering in the sysadmin ranks unless early userspace can emulate it somehow.
Is it possible to configure the kernel to use something other than "eth#" as its initial namespace for netif names? Or is there some other way to get eth1 to be what you need eth1 to be during userland boot?
Signature · 4 lines
"One disk to rule them all, One disk to find them. One disk to bring them all and in the darkness grind them. In the Land of Redmond where the shadows lie." -- The Silicon Valley Tarot Henrique Holschuh
Re: network regression: cannot rename netdev twice
Quoted 6 lines · Henrique de Moraes Holschuh
Quoted 4 lines · Kay Sievers
Please make sure nothing tries to swap netif names in userspace. We have given up that approach, because it is far too fragile to temporary rename devices to be able to swap the names, and race against the loading of new kernel network drivers at the same time.
Quoted 8 lines
That's a damn fair reason, but the loss of that functionality could cause trouble. In fact, at first glance, to me it looks like this has a large potential for unleashing untold pain and suffering in the sysadmin ranks unless early userspace can emulate it somehow.
Is it possible to configure the kernel to use something other than "eth#" as its initial namespace for netif names? Or is there some other way to get eth1 to be what you need eth1 to be during userland boot?
I don't think there is a sane way to do that. Someone could add a kernel command line parameter to switch ethX in the kernel to something else, and create custom udev rules which match on device properties and apply configured names which are ethX again. But for all that, there will be no generally available support in common base system tools, and we absolutely do not recommend anybody doing that.
Udev will not provide any help for that any more, not for automatic device name reservation from a hotplug path, not for device name swaps in the kernel namespace. It will only be allowed to rename devices to a namespace that does not clash with the kernel's one.
People should use biosdevname's pci-slot names, or the on-board labels names like DELL does for configuration-less stable names, or use manually configured names 'internal', 'external' ,'dmz', 'vpn' and so on.
I think we should stop pretending we can solve problems, resulting from simple enumeration depending on device-discovery order. These numbers can never be stable, can never reliably work in the reality we are working with.
It's time to leave these false promises behind us and move on and that means, no stable ethX names anymore.
Kay
Re: network regression: cannot rename netdev twice
Quoted 6 lines · Kay Sievers
Quoted 3 lines · Henrique de Moraes Holschuh
Is it possible to configure the kernel to use something other than "eth#" as its initial namespace for netif names? Or is there some other way to get eth1 to be what you need eth1 to be during userland boot?
Quoted 6 lines
I don't think there is a sane way to do that. Someone could add a kernel command line parameter to switch ethX in the kernel to something else, and create custom udev rules which match on device properties and apply configured names which are ethX again. But for all that, there will be no generally available support in common base system tools, and we absolutely do not recommend anybody doing that.
What sort of impact analysis on userspace was done about this change?
Nobody in his right mind would go back to the dark ages of uncontrolled ifnames. You're effectively forcing everybody with a clue away from the eth# namespace.
Just to be very clear: the impact of this is the need to change the interface names on potentially millions of lines of firewall rules and scripts out there, as well as tracking down stuff (mostly scripts) that special-cases the eth prefix.
Is there a really good reason why we cannot have a way to move the kernel away from the eth# namespace at boot (through a kernel parameter, maybe with the default namespace set at compile time), AND keep the "common base system tools" support to assign ifname based on MAC addresses that we have right now?
Signature · 4 lines
"One disk to rule them all, One disk to find them. One disk to bring them all and in the darkness grind them. In the Land of Redmond where the shadows lie." -- The Silicon Valley Tarot Henrique Holschuh
Re: network regression: cannot rename netdev twice
Quoted 7 lines · Henrique de Moraes Holschuh
Quoted 6 lines · Kay Sievers
Quoted 3 lines · Henrique de Moraes Holschuh
Is it possible to configure the kernel to use something other than "eth#" as its initial namespace for netif names? Or is there some other way to get eth1 to be what you need eth1 to be during userland boot?
Quoted 8 lines
Quoted 6 lines
I don't think there is a sane way to do that. Someone could add a kernel command line parameter to switch ethX in the kernel to something else, and create custom udev rules which match on device properties and apply configured names which are ethX again. But for all that, there will be no generally available support in common base system tools, and we absolutely do not recommend anybody doing that.
What sort of impact analysis on userspace was done about this change?
None. It will just not be supported for new setups. Existing ones will do what they always did.
Quoted 3 lines
Nobody in his right mind would go back to the dark ages of uncontrolled ifnames. You're effectively forcing everybody with a clue away from the eth# namespace.
Yes. It's a game we have lost and we will not win in the future. I gave up, and I warn everybody who think it's simple to manage.
Quoted 4 lines
Just to be very clear: the impact of this is the need to change the interface names on potentially millions of lines of firewall rules and scripts out there, as well as tracking down stuff (mostly scripts) that special-cases the eth prefix.
Yeah, and for good, ethX is a pretty much random kernel name, and I personally will no longer work on conceptually broken infrastructure that can never deliver what it seems to promise. In the longer run, tools need to be fixed to automatically handle changing names, or not care about the names at all, or names need to be explicitly set up outside the ethX namespace to be predictable.
After years of working in that area I will stop to work on these hacks to promise stable ethX names. It was just wrong, like enumerations always are in hotplug setups.
Quoted 3 lines
Is there a really good reason why we cannot have a way to move the kernel away from the eth# namespace at boot (through a kernel parameter, maybe with the default namespace set at compile time),
Could work, but I don't think it is worth. Simple enumeration, and automatic persistent on-disk device name reservation in a flat number-range is just a very flawed concept. I'm not interested in working on that, but that surely should not stop anybody from trying and providing tools that can do that.
Quoted 3 lines
AND keep the
"common base system tools" support to assign ifname based on MAC addresses that we have right now?
Not provided by udev's default setup, which did persistent name reservation in the device hotplug path. It is already disabled and will be entirely removed from the source tree some day. Other tools can still try to provide that. But I declare that model as officially failed and udev will not even try anything like that anymore.
People who need predictable interface names should just manually configure custom/descriptive names, or names which are reliably derived from the hardware, like firmware-provided names or the pci slot number.
Kay
Re: network regression: cannot rename netdev twice
Quoted 3 lines · Kay Sievers
After years of working in that area I will stop to work on these hacks to promise stable ethX names. It was just wrong, like enumerations always are in hotplug setups.
So (real world case) I've got a server that's got a 1G ethernet connected to the public net, a 1G ethernet that's a cluster management network, and a 10G ethernet that connects to our HPC clusters.
And I want to add iptables rules that distinguish based on interface. Currently I can nail the management net to eth0, the public net to eth1, and the 10G to eth2, and then just add "-i eth1" or whatever in the iptables ruleset.
I really don't care if the 0/1/2 move around - but if we're not having nailed-down interface names, what will take the place of '-i ethN' in iptables?
Quoted 4 lines
People who need predictable interface names should just manually configure custom/descriptive names, or names which are reliably derived from the hardware, like firmware-provided names or the pci slot number.
Or is this sort of thing in /etc/udev/rules.d/70-persistent-net.rules
SUBSYSTEM=="net", ACTION=="add", DRIVERS=="?*", ATTR{address}=="00:25:90:0b:f2:80", ATTR{type}=="1", KERNEL=="eth*", NAME="eth0" what you are trying to move to, and my systems are already onboard and
I should just move along, nothing to see here? ;)
Re: network regression: cannot rename netdev twice
Quoted 22 lines · On Wed, Feb 8, 2012 at 07:42, <[email protected]> wrote:
Quoted 3 lines · Kay Sievers
After years of working in that area I will stop to work on these hacks to promise stable ethX names. It was just wrong, like enumerations always are in hotplug setups.
So (real world case) I've got a server that's got a 1G ethernet connected to the public net, a 1G ethernet that's a cluster management network, and a 10G ethernet that connects to our HPC clusters.
And I want to add iptables rules that distinguish based on interface. Currently I can nail the management net to eth0, the public net to eth1, and the 10G to eth2, and then just add "-i eth1" or whatever in the iptables ruleset.
I really don't care if the 0/1/2 move around - but if we're not having nailed-down interface names, what will take the place of '-i ethN' in iptables?
Quoted 4 lines
People who need predictable interface names should just manually configure custom/descriptive names, or names which are reliably derived from the hardware, like firmware-provided names or the pci slot number.
Quoted 4 lines
Or is this sort of thing in
/etc/udev/rules.d/70-persistent-net.rules
SUBSYSTEM=="net", ACTION=="add", DRIVERS=="?*", ATTR{address}=="00:25:90:0b:f2:80", ATTR{type}=="1", KERNEL=="eth*", NAME="eth0" what you are trying to move to, and my systems are already onboard and
I should just move along, nothing to see here? ;)
Yeah, that's what we did in the past. It works fine if you never have to swap names like eth0 and eth1, with need to free one of the the names with a temporary rename.
If another device is added by a different kernel module, or just a USB network device is already plugged-in at bootup time, the parallel loading of drivers might cause the kernel to create a new eth0 or eth1 just in the moment we have the temporary rename active and we want to swap the names.
That model is just entirely flawed and will never work reliably without creating an even bigger mess we already have, by requiring complex retry loops across multiple devices, or having global locks including the kernel's device name allocation logic.
Let's just move on and stop pretending we want or we can solve these problems. Simple device enumerations in hotplug setups can by their very definition not work in a predictable way, we should never have tried to mess around here, and just moved on to something that has at least the potential to work.
Kay
Re: network regression: cannot rename netdev twice
Quoted 10 lines · Kay Sievers
Quoted 4 lines · On Wed, Feb 8, 2012 at 07:42, <[email protected]> wrote:
Or is this sort of thing in
/etc/udev/rules.d/70-persistent-net.rules
SUBSYSTEM=="net", ACTION=="add", DRIVERS=="?*", ATTR{address}=="00:25:90:0b:f2:80", ATTR{type}=="1", KERNEL=="eth*", NAME="eth0" what you are trying to move to, and my systems are already onboard and
I should just move along, nothing to see here? ;)Yeah, that's what we did in the past. It works fine if you never have to swap names like eth0 and eth1, with need to free one of the the names with a temporary rename.
Well, if I had my druthers, I'd stick name="net-mgt", "net-pub", and "net-10g" in the udev rules, and not care about 1/2/3 and race conditions, because meaningful names are easier to not screw up (just last week found a system that had eth1 and eth2 reversed in some iptables rules, wouldn't have happened if they were -mgt and -pub).
Only thing stopping me is getting iptables to accept '-i net-10g', and the distro /etc/sysconfig/network scripts like ifup and ifdown playing nice....
So it sounds like what I want as a sysadmin is the same thing you want as a maintainer...
Re: network regression: cannot rename netdev twice
Our customers would prefer network device names of the form "Ethernet0/0" or "ge-0/0/0" (but I said no...)
Re: network regression: cannot rename netdev twice
In reply to [email protected]Quoted 18 lines · On Wed, Feb 8, 2012 at 21:06, <[email protected]> wrote:
Quoted 10 lines · Kay Sievers
Quoted 4 lines · On Wed, Feb 8, 2012 at 07:42, <[email protected]> wrote:
Or is this sort of thing in
/etc/udev/rules.d/70-persistent-net.rules
SUBSYSTEM=="net", ACTION=="add", DRIVERS=="?*", ATTR{address}=="00:25:90:0b:f2:80", ATTR{type}=="1", KERNEL=="eth*", NAME="eth0" what you are trying to move to, and my systems are already onboard and
I should just move along, nothing to see here? ;)Yeah, that's what we did in the past. It works fine if you never have to swap names like eth0 and eth1, with need to free one of the the names with a temporary rename.
Well, if I had my druthers, I'd stick name="net-mgt", "net-pub", and "net-10g" in the udev rules, and not care about 1/2/3 and race conditions, because meaningful names are easier to not screw up (just last week found a system that had eth1 and eth2 reversed in some iptables rules, wouldn't have happened if they were -mgt and -pub).
Quoted 5 lines
Only thing stopping me is getting iptables to accept '-i net-10g', and the distro
/etc/sysconfig/networkscripts like ifup and ifdown playing nice....So it sounds like what I want as a sysadmin is the same thing you want as a maintainer...
Yeah, that sounds very much like it is.
I want to push some responsibility to the admin, do less automagic, and personally want to be less responsible for all the unintended screw-up the automagic is causing everywhere.
Sure, the intention to keep the names like they always have been was good, but a good intention and a broken model to deliver it, and continue to pretend we can solve it, is the worst things we can do. :)
Kay
Quoted 1 line · Jiri Slaby
1) network systemd rule doesn't start network
What silly thing is the network systemd rule doing?
Eric
Quoted 31 lines · On poniedziałek, 30 stycznia 2012 o 22:56:26 Jiri Slaby wrote:
Hi,
I cannot boot properly with this commit:
commit524b6c5b39b931311dfe5a2f5abae2f5c9731676Author: Eric W. Biederman <[email protected]> Date: Sun Dec 18 20:09:31 2011 -0800sysfs: Kill nlink counting.
1) network systemd rule doesn't start network
2) sensors complain:
sensors_init: Kernel interface errorad 2) look at what it does:
/* returns !0 if sysfs filesystem was found, 0 otherwise */ intsensors_init_sysfs(void){ struct stat statbuf; snprintf(sensors_sysfs_mount, NAME_MAX, "%s", "/sys"); if (stat(sensors_sysfs_mount, &statbuf) < 0 || statbuf.st_nlink <= 2) /* Empty directory */ return 0; return 1; }
Quoted 5 lines
So this looks like it became a part of ABI we cannot break...
A revert of this commit on the top of today's -next fixes the problem.
thanks,
I created a Bugzilla entry at
https://bugzilla.kernel.org/show_bug.cgi?id=42712
for your bug report, please add your address to the CC list in there, thanks!
Signature · 2 lines
Maciej Rutecki http://www.mrutecki.pl