sysfs: tagged directories not merged completely yet

Hi Greg,

Someone made me notice that Eric's "sysfs tagged directories" patchset you merged recently isn't complete in your "patches" tree. The last two patches that actually uses this new feature aren't in your tree yet.

* [PATCH 7/8] netns: Enable tagging for net_class directories in sysfs
* [PATCH 8/8] sysfs: user namespaces: fix bug with clone(CLONE_NEWUSER) with fairsched

I didn't noticed it sooner because I'm still using my own version of the patchset on top of linux-2.6.

I'm wondering why you left these two patches out? Is it on purpose? Do you expect other maintainers to merged them in their respective tree? Dave Miller acked the patch for network namespace and said "You can send it all through Greg or whoever, don't feel obligated to push it through me since it depends upon the earlier bits in this series".

I only got so far through the patch set and then got interrupted (was at different conferences for 2 weeks), sorry. I'll get the rest of the patchset added later today.

Note, I did talk to Al Viro about these patches last week at the kernel summit, and he advised holding off on sending them to mainline before he could review them. So I'll keep them in my tree for now, but not push them to Linus until he acks them as he raised a few issues in our conversation.

Quoted 10 lines · Benjamin Thery

Hi Greg,

Someone made me notice that Eric's "sysfs tagged directories" patchset you merged recently isn't complete in your "patches" tree. The last two patches that actually uses this new feature aren't in your tree yet.

* [PATCH 7/8] netns: Enable tagging for net_class directories in sysfs
* [PATCH 8/8] sysfs: user namespaces: fix bug with clone(CLONE_NEWUSER) with fairsched

Quoted 8 lines

I didn't noticed it sooner because I'm still using my own version of the patchset on top of linux-2.6.

I'm wondering why you left these two patches out? Is it on purpose? Do you expect other maintainers to merged them in their respective tree? Dave Miller acked the patch for network namespace and said "You can send it all through Greg or whoever, don't feel obligated to push it through me since it depends upon the earlier bits in this series".

I only got so far through the patch set and then got interrupted (was at different conferences for 2 weeks), sorry. I'll get the rest of the patchset added later today.

Note, I did talk to Al Viro about these patches last week at the kernel summit, and he advised holding off on sending them to mainline before he could review them. So I'll keep them in my tree for now, but not push them to Linus until he acks them as he raised a few issues in our conversation.

thanks,

greg k-h

Oh.
It's a pity Al couldn't re-review them before. We've already lost a lot of time with this patchset and it's blocking easier testing of network namespaces (right now, with a mainline kernel, we have to disable sysfs to build network namespaces).

Thanks anyway. It's a good step to have it in a tree.

Quoted 6 lines · Benjamin Thery

Oh.
It's a pity Al couldn't re-review them before. We've already lost a lot of time with this patchset and it's blocking easier testing of network namespaces (right now, with a mainline kernel, we have to disable sysfs to build network namespaces).

I am confident that we have a good base with these patches and the rest of the work can be done incrementally on top of them if any issues turn up.

Al recent rework of sysctl has a very similar structure.

Quoted 8 lines · Eric W. Biederman

Quoted 6 lines · Benjamin Thery

Oh.
It's a pity Al couldn't re-review them before. We've already lost a lot of time with this patchset and it's blocking easier testing of network namespaces (right now, with a mainline kernel, we have to disable sysfs to build network namespaces).

Quoted 4 lines

I am confident that we have a good base with these patches and the rest of the work can be done incrementally on top of them if any issues turn up.

Al recent rework of sysctl has a very similar structure.

No, it does not. My apologies for delay, but here are more printable parts of review.

Quoted 8 lines · Eric W. Biederman

Quoted 6 lines · Benjamin Thery

Oh.
It's a pity Al couldn't re-review them before. We've already lost a lot of time with this patchset and it's blocking easier testing of network namespaces (right now, with a mainline kernel, we have to disable sysfs to build network namespaces).

Quoted 4 lines

I am confident that we have a good base with these patches and the rest of the work can be done incrementally on top of them if any issues turn up.

Al recent rework of sysctl has a very similar structure.

No, it does not. My apologies for delay, but here are more printable parts of review.

From the point of view of the VFS sysfs is a distributed filesystem.

a) You do *not* share struct inode between the superblocks, for fsck sake!

d) You REALLY, REALLY do not unhash busy dentries of directories.

* may I politely suggest that
again:

	mutex_lock(&A);
	if (!mutex_trylock(&B)) {
		mutex_unlock(&A);
		goto again;
	}

is somewhat, er, deficient way to deal with buggered locking hierarchy?

Not to mention anything else, that's obviously FUBAR on UP box - if we have B contended, we've just killed the box dead since we'll never give the CPU back to whoever happens to hold B. See sysfs_mv_dir() for a lovely example.

c) You do *not* change dentry tree topology without s_vfs_rename_mutex on affected superblock. That, BTW, is broken in mainline sysfs as well.

b) You do *not* grab i_mutex on ancestors after having grabbed it on file, as sysfs_chmod_file() does.

In addition to that, there are interesting internal problems:
* inumbers are released by final sysfs_put(); that can happen before the final iput() on corresponding inode. Guess what happens if new entry is created in the meanwhile, reuses the same inumber and lookup gets to sysfs_get_inode() on it?

* sysfs_count_nlink() is called from sysfs_fill_super() without sysfs_mutex; now this sucker can get called at any moment.

* just what is protecting ->s_tag?

* __sysfs_remove_dir() appears to assume that subdirectories are possible; AFAICS, if we *do* get them, we get very screwed after remove_dir().

* everything else aside, the internal locking is extremely heavy. For fsck sake, guys, a single system-wide mutex that can be grabbed for the duration of readdir on any directory and block just about anything in the filesystem? Just mmap() something over NFS on a slow link and do getdents() to such buffer. Watch a *lot* of stuff getting buggered. Hell, you can't even do ifconfig up while that sucker is held...

Seriously, people, it's getting worse than devfs had ever been ;-/

Al, thanks a lot for the review, I really appreciate it.

Eric, I've removed the following patches from my tree so you can rework them, if you want to.

Quoted 10 lines · Eric W. Biederman

Quoted 5 lines · Benjamin Thery

Oh.
It's a pity Al couldn't re-review them before. We've already lost a lot of time with this patchset and it's blocking easier testing of network namespaces (right now, with a mainline kernel, we have to disable sysfs to build network namespaces).

I am confident that we have a good base with these patches and the rest of the work can be done incrementally on top of them if any issues turn up.

Al recent rework of sysctl has a very similar structure.

No, it does not. My apologies for delay, but here are more printable parts of review.

First of all, this stuff breaks just about every damn integrity constraint VFS knows of. It tries to tiptoe through the resulting minefield - without success. So the first group of comments will be of "you *really* don't do $FOO" variety. I'm very far from being convinced that we want to special-case in VFS every kind of weirdness sysfs happens to do; in effect, that would require adding a FS_IS_SYSFS_SO_BEND_OVER fs type flag and making a lot of locking conditional on that.

a) You do *not* share struct inode between the superblocks, for fsck sake!
b) You do *not* grab i_mutex on ancestors after having grabbed it on file, as sysfs_chmod_file() does.
c) You do *not* change dentry tree topology without s_vfs_rename_mutex on affected superblock. That, BTW, is broken in mainline sysfs as well.
d) You REALLY, REALLY do not unhash busy dentries of directories.

In addition to that, there are interesting internal problems:
* inumbers are released by final sysfs_put(); that can happen before the final iput() on corresponding inode. Guess what happens if new entry is created in the meanwhile, reuses the same inumber and lookup gets to sysfs_get_inode() on it?
* may I politely suggest that
again:

	mutex_lock(&A);
	if (!mutex_trylock(&B)) {
		mutex_unlock(&A);
		goto again;
	}

is somewhat, er, deficient way to deal with buggered locking hierarchy? Not to mention anything else, that's obviously FUBAR on UP box - if we have B contended, we've just killed the box dead since we'll never give the CPU back to whoever happens to hold B. See sysfs_mv_dir() for a lovely example.
* sysfs_count_nlink() is called from sysfs_fill_super() without sysfs_mutex; now this sucker can get called at any moment.
* just what is protecting ->s_tag?
* __sysfs_remove_dir() appears to assume that subdirectories are possible; AFAICS, if we *do* get them, we get very screwed after remove_dir().
* everything else aside, the internal locking is extremely heavy. For fsck sake, guys, a single system-wide mutex that can be grabbed for the duration of readdir on any directory and block just about anything in the filesystem? Just mmap() something over NFS on a slow link and do getdents() to such buffer. Watch a *lot* of stuff getting buggered. Hell, you can't even do ifconfig up while that sucker is held...

Seriously, people, it's getting worse than devfs had ever been ;-/

c) You do *not* change dentry tree topology without s_vfs_rename_mutex on affected superblock. That, BTW, is broken in mainline sysfs as well.

In addition to that, there are interesting internal problems:
* inumbers are released by final sysfs_put(); that can happen before the final iput() on corresponding inode. Guess what happens if new entry is created in the meanwhile, reuses the same inumber and lookup gets to sysfs_get_inode() on it?

* may I politely suggest that
again:

	mutex_lock(&A);
	if (!mutex_trylock(&B)) {
		mutex_unlock(&A);
		goto again;
	}

is somewhat, er, deficient way to deal with buggered locking hierarchy? Not to mention anything else, that's obviously FUBAR on UP box - if we have B contended, we've just killed the box dead since we'll never give the CPU back to whoever happens to hold B. See sysfs_mv_dir() for a lovely example.

* sysfs_count_nlink() is called from sysfs_fill_super() without sysfs_mutex; now this sucker can get called at any moment.

* __sysfs_remove_dir() appears to assume that subdirectories are possible; AFAICS, if we *do* get them, we get very screwed after remove_dir().

* everything else aside, the internal locking is extremely heavy. For fsck sake, guys, a single system-wide mutex that can be grabbed for the duration of readdir on any directory and block just about anything in the filesystem? Just mmap() something over NFS on a slow link and do getdents() to such buffer. Watch a *lot* of stuff getting buggered. Hell, you can't even do ifconfig up while that sucker is held...

Seriously, people, it's getting worse than devfs had ever been ;-/

If the filler is a real concern, I think it's better to just decouple it rather than making sysfs locking fine-grained. sysfs metadata might as well be protected by a single spinlock if it can be decoupled from vfs locking and stuff. It's just an in-memory tree which isn't used too often.

Generally, the VFS layer isn't too easy for sysfs which is a bit like distributed filesystem but has more strict here-and-now rule (all changes should be visible instantaneously). At the beginning, sysfs didn't have much metadata itself, it just used the VFS data structures but that was too large so sysfs_dirent got introduced and it tried to update VFS data structures as necessary and (this is when I started working on it) the current code and Eric's patcheset evolved from there.

Maybe it can be done better by taking more traditional distributed filesystem approach - re/invalidation on access. I don't know whether it will fit sysfs's needs but if it can be done, sysfs would be able to ride along with other distributed filesystems and become much more conventional in its interfacing with VFS.

Quoted 5 lines

If the filler is a real concern, I think it's better to just decouple it rather than making sysfs locking fine-grained. sysfs metadata might as well be protected by a single spinlock if it can be decoupled from vfs locking and stuff. It's just an in-memory tree which isn't used too often.

I think with a little care we can make the sysfs read side rcu protected which would remove any real locking from lookup and readdir.

Quoted 9 lines

Generally, the VFS layer isn't too easy for sysfs which is a bit like distributed filesystem but has more strict here-and-now rule (all changes should be visible instantaneously). At the beginning, sysfs didn't have much metadata itself, it just used the VFS data structures but that was too large so sysfs_dirent got introduced and it tried to update VFS data structures as necessary and (this is when I started working on it) the current code and Eric's patcheset evolved from there.

Quoted 5 lines

Maybe it can be done better by taking more traditional distributed filesystem approach - re/invalidation on access. I don't know whether it will fit sysfs's needs but if it can be done, sysfs would be able to ride along with other distributed filesystems and become much more conventional in its interfacing with VFS.

The revalidate on access model doesn't appear to have a way to track remote renames. Something sysfs supports.

I have just spent a little bit of time thinking it through. I had previously thought that we could take advantage of the fact that sysfs only allows VFS reads we could fix our backwards lock ordering by optimizing the read side with rcu. Unfortunately the VFS still takes locks on rename and similar paths despite the fact sysfs does not implement those paths functions. Therefore whatever we do has to be handle all VFS operations even if we don't support them. Weird, but true.

We may need to delay dentry unhashing until revalidate. I think I see some issues if we don't do that.

It is a wee bit subtle but sysfs_get_dentry grabs inode->i_mutex. of potentially all of the parents of sd. So I can not hold the inode mutex of the directory while it is called.

sysfs_mv_dir picks an arbitrary mutex lock ordering and busy waits until it can grab the inode mutexes in that order. This can dead lock on UP systems.

Instead use the generic lock_rename that guarantees we grab parent directories before child directories, so there is not need to spin and no chance of deadlock.

Unless someone will give an example of how having multiple superblocks sharing inodes is a problem in practice for sysfs and call it good for 2.6.28. Certainly it shouldn't be an issue if the network namespace code is compiled out. And it should greatly improve testing of the network namespace to at least have access to sysfs.

Later Tejun or I or possibly someone else who cares can go back and simplify the sysfs locking to remove the need for multiple superblocks sharing inodes, and to address the other big nasties in the current sysfs implementation.

Greg I agree with Al that sysfs isn't perfect but we sure aren't going to fix it if you keep dropping or taking years to merge every patch from the people working on it, and then dropping those patches because someone frowns at them.

Quoted 3 lines · Eric W. Biederman

It is a wee bit subtle but sysfs_get_dentry grabs inode->i_mutex. of potentially all of the parents of sd. So I can not hold the inode mutex of the directory while it is called.

Hi Eric,

This patch looks good to me. In my quest to parse exactly what was going on, I rewrote a description of the patch. Could you look over this and see if I'm on target?

Before this patch, inode->i_mutex is held in order to keep the inode's mode and ctime from changing out underneath us. If we didn't do this, you could potentially get garbage when reading them out of the old inode. We calculated these new permissions once since it is redundant to do it several times.

We also need to perform a sysfs_get_dentry() operation on the sysfs_dirent in order to find all the dentries on each sb.

This needs
to be performed once for each sb in which the inode appears. "[B]ut sysfs_get_dentry grabs inode->i_mutex. of potentially all of the parents of sd. So I can not hold the inode mutex of the directory while it is called."

This patch drops the inode->i_mutex over the entire "for each sysfs_dirent" loop. It, instead, reacquires and drops it each time we need to calculate the new mode/ctime for the target dentries. It does this away from the sysfs_get_dentry() call now.

Quoted 5 lines · Eric W. Biederman

Unless someone will give an example of how having multiple superblocks sharing inodes is a problem in practice for sysfs and call it good for 2.6.28. Certainly it shouldn't be an issue if the network namespace code is compiled out. And it should greatly improve testing of the network namespace to at least have access to sysfs.

But if the network namespace code is in? THen we have problems, right? And that's the whole point here.

The fact that you are trying to limit userspace view of in-kernel data structures, based on that specific user, is, in my opinion, crazy.

Why not just keep all users from seeing sysfs, and then have a user daemon doing something on top of FUSE if you really want to see this kind of stuff.

Eric W. Biederman wrote:
IIRC, the original readdir implementation put a cursor entry to walk through the children list. The implementation was horribly broken in a number of different ways (ISTR problems with locking and multiple and different type of walkers) and I just gutted out all the complexity out and made it simple as getting it correct was far more important and there seemed to be little need for optimization.

Yeah, using RCU sounds like a plan.

Quoted 2 lines

The revalidate on access model doesn't appear to have a way to track remote renames. Something sysfs supports.

Yeap, IIRC, one of the reasons why sysfs wasn't converted over to sysfs was because sysfs guarantees inode doesn't change over rename or move so that notifications keep working over renames.

Quoted 5 lines · Eric W. Biederman

Unless someone will give an example of how having multiple superblocks sharing inodes is a problem in practice for sysfs and call it good for 2.6.28. Certainly it shouldn't be an issue if the network namespace code is compiled out. And it should greatly improve testing of the network namespace to at least have access to sysfs.

But if the network namespace code is in? THen we have problems, right? And that's the whole point here.

The fact that you are trying to limit userspace view of in-kernel data structures, based on that specific user, is, in my opinion, crazy.

Why not just keep all users from seeing sysfs, and then have a user daemon doing something on top of FUSE if you really want to see this kind of stuff.

The "leakage" just seems too hard to stop.

Quoted 4 lines

Later Tejun or I or possibly someone else who cares can go back and simplify the sysfs locking to remove the need for multiple superblocks sharing inodes, and to address the other big nasties in the current sysfs implementation.

I know how the whole "we'll go back later and fix it up" stuff works, I've used that excuse too many times in the past myself. Never happens :)

Quoted 4 lines

Greg I agree with Al that sysfs isn't perfect but we sure aren't going to fix it if you keep dropping or taking years to merge every patch from the people working on it, and then dropping those patches because someone frowns at them.

"years"? Come on, these did take a while due to travel and other stuff. These are core kernel changes, and need time to ensure that they work properly, and get the proper review from people who understand this kind of stuff.

And to call Al a generic "someone", is just rude and disrespectful. I trust his opinion in this area far more than I do yours, to be honest.

This whole series is dropped, if you want to resubmit them, feel free to, _after_ adressing his issues.

Quoted 10 lines · Greg KH

Quoted 5 lines · Eric W. Biederman

Unless someone will give an example of how having multiple superblocks sharing inodes is a problem in practice for sysfs and call it good for 2.6.28. Certainly it shouldn't be an issue if the network namespace code is compiled out. And it should greatly improve testing of the network namespace to at least have access to sysfs.

But if the network namespace code is in? THen we have problems, right? And that's the whole point here.

Quoted 6 lines

The fact that you are trying to limit userspace view of in-kernel data structures, based on that specific user, is, in my opinion, crazy.

Why not just keep all users from seeing sysfs, and then have a user daemon doing something on top of FUSE if you really want to see this kind of stuff.

Well the blocker is really that when you create a new network namespace, it wants to create a new loopback interface, but /sys/devices/virtual/net/lo already exists. That's the same issue with user namespace when the fair scheduler is enabled, which tries to re-create /sys/kernel/uids/0.

Otherwise yeah at least for my own uses, containers wouldn't need to look at /sys at all.

Heck you wouldn't even need FUSE, just mount -t tmpfs /sys/class/net and manually link the right devices from /sys/devices/virtual/net.

Hello, a bit of additions after some sleep.

Heh... it did sound like a plan but I don't think the plan would solve the problem. filldir can't be put in rcu read critical section. :-p

Quoted 6 lines

Quoted 2 lines

The revalidate on access model doesn't appear to have a way to track remote renames. Something sysfs supports.

Yeap, IIRC, one of the reasons why sysfs wasn't converted over to sysfs was because sysfs guarantees inode doesn't change over rename or move so that notifications keep working over renames.

s/over to sysfs/over to revalidation/ and s/inode/dentry/. Maybe we can just ignore dnotify? :-(

Quoted 27 lines · Serge E. Hallyn

Quoted 10 lines · Greg KH

Quoted 5 lines · Eric W. Biederman

Unless someone will give an example of how having multiple superblocks sharing inodes is a problem in practice for sysfs and call it good for 2.6.28. Certainly it shouldn't be an issue if the network namespace code is compiled out. And it should greatly improve testing of the network namespace to at least have access to sysfs.

But if the network namespace code is in? THen we have problems, right? And that's the whole point here.

Quoted 6 lines

The fact that you are trying to limit userspace view of in-kernel data structures, based on that specific user, is, in my opinion, crazy.

Why not just keep all users from seeing sysfs, and then have a user daemon doing something on top of FUSE if you really want to see this kind of stuff.

Well the blocker is really that when you create a new network namespace, it wants to create a new loopback interface, but /sys/devices/virtual/net/lo already exists. That's the same issue with user namespace when the fair scheduler is enabled, which tries to re-create /sys/kernel/uids/0.

Otherwise yeah at least for my own uses, containers wouldn't need to look at /sys at all.

Quoted 2 lines

Heck you wouldn't even need FUSE, just mount -t tmpfs /sys/class/net and manually link the right devices from /sys/devices/virtual/net.

Great, that sounds like a solution.

So tell me again why we need these huge sysfs reworks? :)

Quoted 3 lines

Well the blocker is really that when you create a new network namespace, it wants to create a new loopback interface, but /sys/devices/virtual/net/lo already exists. That's the same issue with

Quoted 4 lines · Tejun Heo

Hello, a bit of additions after some sleep.

Heh... it did sound like a plan but I don't think the plan would solve the problem. filldir can't be put in rcu read critical section. :-p

There is srcu and there is the trick of grabbing the reference count on the current sysfs_dirent over the filldir and dropping the rcu lock (which works for proc).

To cut down on lock overhead from user space accesses that works.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

The revalidate on access model doesn't appear to have a way to track remote renames. Something sysfs supports.

Yeap, IIRC, one of the reasons why sysfs wasn't converted over to sysfs was because sysfs guarantees inode doesn't change over rename or move so that notifications keep working over renames.

s/over to sysfs/over to revalidation/ and s/inode/dentry/. Maybe we can just ignore dnotify? :-(

Well there are more cases than dnotify, there is the renaming of directories in sysfs, although rare that I think get awkward if we use revalidation.

I'm still not certain how we can get the lock ordering so it doesn't cause us problems. I will look at revalidation and what the other distributed filesystems are doing and see if that might work. If it doesn't we need refactor the VFS locking.

Quoted 28 lines · Greg KH

Quoted 27 lines · Serge E. Hallyn

Quoted 10 lines · Greg KH

Quoted 5 lines · Eric W. Biederman

Unless someone will give an example of how having multiple superblocks sharing inodes is a problem in practice for sysfs and call it good for 2.6.28. Certainly it shouldn't be an issue if the network namespace code is compiled out. And it should greatly improve testing of the network namespace to at least have access to sysfs.

But if the network namespace code is in? THen we have problems, right? And that's the whole point here.

Quoted 6 lines

The fact that you are trying to limit userspace view of in-kernel data structures, based on that specific user, is, in my opinion, crazy.

Why not just keep all users from seeing sysfs, and then have a user daemon doing something on top of FUSE if you really want to see this kind of stuff.

Well the blocker is really that when you create a new network namespace, it wants to create a new loopback interface, but /sys/devices/virtual/net/lo already exists. That's the same issue with user namespace when the fair scheduler is enabled, which tries to re-create /sys/kernel/uids/0.

Otherwise yeah at least for my own uses, containers wouldn't need to look at /sys at all.

Quoted 6 lines

Quoted 2 lines

Heck you wouldn't even need FUSE, just mount -t tmpfs /sys/class/net and manually link the right devices from /sys/devices/virtual/net.

Great, that sounds like a solution.

So tell me again why we need these huge sysfs reworks? :)

Because :

Quoted 1 line

Quoted 1 line

Well the blocker is really that when you create a new network namespace,

Quoted 2 lines

Quoted 2 lines

it wants to create a new loopback interface, but /sys/devices/virtual/net/lo already exists. That's the same issue with

So at least we'd have to do something to allow creation of 'duplicate' devices in different namespaces. It might be fine if we just ended up with /sys/devices/virtual/net/lo, if created in a child net namespace, be named /sys/devices/virtual/net/lo.childXYZ. Then userspace can mount -t tmpfs none /sys/class/net and ln -s /sys/devices/virtual/net/lo.childXYZ /sys/class/net/lo.

Quoted 5 lines · Eric W. Biederman

Unless someone will give an example of how having multiple superblocks sharing inodes is a problem in practice for sysfs and call it good for 2.6.28. Certainly it shouldn't be an issue if the network namespace code is compiled out. And it should greatly improve testing of the network namespace to at least have access to sysfs.

But if the network namespace code is in? THen we have problems, right?

And that's the whole point here.

The fact that you are trying to limit userspace view of in-kernel data structures, based on that specific user, is, in my opinion, crazy.

Why not just keep all users from seeing sysfs, and then have a user daemon doing something on top of FUSE if you really want to see this kind of stuff.

The "leakage" just seems too hard to stop.

I know how the whole "we'll go back later and fix it up" stuff works, I've used that excuse too many times in the past myself. Never happens :)

Quoted 4 lines

Greg I agree with Al that sysfs isn't perfect but we sure aren't going to fix it if you keep dropping or taking years to merge every patch from the people working on it, and then dropping those patches because someone frowns at them.

"years"? Come on, these did take a while due to travel and other stuff.

These are core kernel changes, and need time to ensure that they work properly, and get the proper review from people who understand this kind of stuff.

And to call Al a generic "someone", is just rude and disrespectful. I trust his opinion in this area far more than I do yours, to be honest.

This whole series is dropped, if you want to resubmit them, feel free to, _after_ adressing his issues.

bah,

Hmmm... I'm probably missing something (and being lazy) but how does it guarantee the validity of the next pointer after dropping the rcu lock?

Quoted 4 lines

I'm still not certain how we can get the lock ordering so it doesn't cause us problems. I will look at revalidation and what the other distributed filesystems are doing and see if that might work. If it doesn't we need refactor the VFS locking.

Yeah, if we can make sysfs behave like other distributed filesystems, it would be great. :-)

bah,

Quoted 34 lines · Serge E. Hallyn

Quoted 33 lines · Greg KH

Quoted 24 lines · Serge E. Hallyn

Quoted 10 lines · Greg KH

Quoted 5 lines · Eric W. Biederman

Unless someone will give an example of how having multiple superblocks sharing inodes is a problem in practice for sysfs and call it good for 2.6.28. Certainly it shouldn't be an issue if the network namespace code is compiled out. And it should greatly improve testing of the network namespace to at least have access to sysfs.

But if the network namespace code is in? THen we have problems, right? And that's the whole point here.

Quoted 6 lines

The fact that you are trying to limit userspace view of in-kernel data structures, based on that specific user, is, in my opinion, crazy.

Why not just keep all users from seeing sysfs, and then have a user daemon doing something on top of FUSE if you really want to see this kind of stuff.

Well the blocker is really that when you create a new network namespace, it wants to create a new loopback interface, but /sys/devices/virtual/net/lo already exists. That's the same issue with user namespace when the fair scheduler is enabled, which tries to re-create /sys/kernel/uids/0.

Quoted 5 lines

Otherwise yeah at least for my own uses, containers wouldn't need to look at /sys at all.

Heck you wouldn't even need FUSE, just mount -t tmpfs /sys/class/net and manually link the right devices from /sys/devices/virtual/net.

Great, that sounds like a solution.

Quoted 5 lines

Quoted 1 line

So tell me again why we need these huge sysfs reworks? :)

Because :

Quoted 1 line

Quoted 1 line

Well the blocker is really that when you create a new network namespace,

No, wait. Why would you want to do such a thing in the first place?

Quoted 9 lines

Quoted 2 lines

Quoted 2 lines

it wants to create a new loopback interface, but /sys/devices/virtual/net/lo already exists. That's the same issue with

So at least we'd have to do something to allow creation of 'duplicate' devices in different namespaces. It might be fine if we just ended up with /sys/devices/virtual/net/lo, if created in a child net namespace, be named /sys/devices/virtual/net/lo.childXYZ. Then userspace can mount -t tmpfs none /sys/class/net and ln -s /sys/devices/virtual/net/lo.childXYZ /sys/class/net/lo.

ick.

I agree with Tejun here, what's this whole network namespace stuff, what problems is it trying to solve and what are its goals?

thanks,

greg k-h

Hello, Greg.

Quoted 7 lines · Greg KH

Quoted 5 lines · Eric W. Biederman

Unless someone will give an example of how having multiple superblocks sharing inodes is a problem in practice for sysfs and call it good for 2.6.28. Certainly it shouldn't be an issue if the network namespace code is compiled out. And it should greatly improve testing of the network namespace to at least have access to sysfs.

Quoted 5 lines

But if the network namespace code is in? THen we have problems, right? And that's the whole point here.

The fact that you are trying to limit userspace view of in-kernel data structures, based on that specific user, is, in my opinion, crazy.

Well, that's the whole point of all the namespace stuff. If we're gonna do namespaces, view of in-kernel data structures need to be limited and modified one way or the other.

Quoted 3 lines

Why not just keep all users from seeing sysfs, and then have a user daemon doing something on top of FUSE if you really want to see this kind of stuff.

That sounds nice. Out of ignorance, how is the /proc dealt with? Maybe we can have some unified approach for this multiple views of the system stuff.

From a user interface design perspective I don't see a good

From the perspective of the internal sysfs data structures tagged

Can somebody hammer the big picture regarding namespaces into my small head?

Quoted 3 lines

That sounds nice. Out of ignorance, how is the /proc dealt with? Maybe we can have some unified approach for this multiple views of the system stuff.

/proc uses just about every trick in the book to make this work.

/proc/sys uses a magic d_compare method.

/proc/net becomes a symlink to /proc/<pid>/net and we get completely
different directory trees below that. Shortly that code will
use auto mounts of a proc_net filesystem, that has different super blocks one for each different network namespace.

/proc/sysvipc/* simply returns different values from it's files depending
upon which process is reading them.

/proc itself has multiple super blocks one for each different pid namespace.

The long term direction is to be able to see everything at once. If you mount all of the filesystems multiple times in the proper way. That allows monitoring software to watch what is going on inside of a container without a challenge, and it makes it a user space policy decision how much an individual container sees.

For sysfs there isn't have the option of putting things under /proc/<pid>, the directories I am interested in (at least for network devices) are scattered all over sysfs and come and go with device hotplug events so I don't see a realistic way of splitting those directories out into their own filesystem.

From a user interface design perspective I don't see a good alternative to having /sys/class/net/, /sys/virtual/net/, and all of the other directories different based on network namespace. Then have the network namespace be specified by super block. Looking at current and doing the magic d_compare trick almost works, but it runs into problems with sysfs_get_dentry.

From the perspective of the internal sysfs data structures tagged dirents are clean and simple so I don't see a reason to re-architect that.

I have spent the last several days looking deeply at what the vfs can do, and how similar situations are handled. My observations are:

1) exportfs from nfsd is similar to our kobject to sysfs_dirent layer, and solves that set of problems cleanly, including remote rename. So there is no fundamental reason we need inverted twisting locking in sysfs, or otherwise violate existing vfs rules.

2) i_mutex seems to protect very little if anything that we care about. The dcache has it's own set of locks. So we may be able to completely avoid taking i_mutex in sysfs and simplify things enormously. Currently I believe we are very similar to ocfs2 in terms of locking requirements.

3) For i_notify and d_notify that seems to require pinning the inode or the dentry in question, so I see no reason why a d_revalidate style of update will have problems.

4) For finer locking granularity of readdir. All we need to do is do the semi-expensive restart for each dirent, and the problem is trivially solved.

5) Large directories are a potential performance problem in sysfs.

So it appears that the path forward is:
- Cleanup sysfs locking and other issues.
- Return to the network namespace code.

Possibly with an intermediate step of only showing the network devices in the initial network namespace in sysfs.

Quoted 2 lines

Can somebody hammer the big picture regarding namespaces into my small head?

100,000 foot view. A namespace introduces a scope so multiple objects can have the same name. Like network devices.

10,000 foot view. The network namespace looks to user space as if the kernel has multiple independent network stacks.

1000 foot view. I have two network devices named lo, and sysfs does not currently have a place for me to put them.

Leakage and being able to fool an application that it has the entire kernel to itself are not concerns. The goal is simply to get the entire object name to object translation boundary and the namespace work is done. We have largely achieved, and the code to do so once complete is reasonable enough that it should be no worse than dealing with any other kernel bug.

Aieeeee... I wanna run screaming and crying. Any chance these can be done using FUSE? FUSE is pretty flexible and should be able to emulate most of proc files w/o too much difficulty.

And can we do the same thing for sysfs using FUSE? So that not only the policy but also the implementation is in userland? The changes are quite pervasive and makes the whole thing pretty difficult to follow.

Quoted 3 lines

From the perspective of the internal sysfs data structures tagged dirents are clean and simple so I don't see a reason to re-architect that.

Heh... you know I have some reservations on that one too. :-)

Quoted 3 lines

I have spent the last several days looking deeply at what the vfs can do, and how similar situations are handled. My observations are:

Thanks. Much appreciated.

Quoted 4 lines

1) exportfs from nfsd is similar to our kobject to sysfs_dirent layer, and solves that set of problems cleanly, including remote rename. So there is no fundamental reason we need inverted twisting locking in sysfs, or otherwise violate existing vfs rules.

eGreat. IIRC, it does it by not moving the existing dentry but invalidating it, right?

The current twisted state is more of what's left from the original tree-of-dentries-and-inodes implementation. It would be great to do proper distributed fs instead.

Quoted 5 lines

2) i_mutex seems to protect very little if anything that we care about. The dcache has it's own set of locks. So we may be able to completely avoid taking i_mutex in sysfs and simplify things enormously. Currently I believe we are very similar to ocfs2 in terms of locking requirements.

I think the timestamps are one of the things it protects.

Quoted 3 lines

3) For i_notify and d_notify that seems to require pinning the inode or the dentry in question, so I see no reason why a d_revalidate style of update will have problems.

Because the existing notifications won't be moved over to the new dentry. dnotify wouldn't work the same way. ISTR that was the reason why I didn't do the d_revalidate thing, but I don't think it really matters. dnotify on sysfs nodes doesn't work properly anyway.

Quoted 3 lines

4) For finer locking granularity of readdir. All we need to do is do the semi-expensive restart for each dirent, and the problem is trivially solved.

That can show the same entry multiple times or skip existing entries. I think it's better to put fake entries and implement iterators.

Quoted 1 line

5) Large directories are a potential performance problem in sysfs.

Yes, it is. It hasn't been an issue till now. You're worrying about look up performance, right?

If that's a real concern we can link sd's
into a hash table, but I'm not sure tho. For listing, O(n) is the best we can do anyway and after the initial lookup, the result would be cached via dcache anyway, so I'm not really sure how much adding a hashtable will buy us.

Quoted 19 lines

So it appears that the path forward is:
- Cleanup sysfs locking and other issues.
- Return to the network namespace code.

Possibly with an intermediate step of only showing the network devices in the initial network namespace in sysfs.

Quoted 2 lines

Can somebody hammer the big picture regarding namespaces into my small head?

100,000 foot view. A namespace introduces a scope so multiple objects can have the same name. Like network devices.

10,000 foot view. The network namespace looks to user space as if the kernel has multiple independent network stacks.

1000 foot view. I have two network devices named lo, and sysfs does not currently have a place for me to put them.

Quoted 6 lines

Leakage and being able to fool an application that it has the entire kernel to itself are not concerns. The goal is simply to get the entire object name to object translation boundary and the namespace work is done. We have largely achieved, and the code to do so once complete is reasonable enough that it should be no worse than dealing with any other kernel bug.

Yes, I'm aware of the goals. What I'm curious about is the consensus regarding network namespace and all its implications. It adds a lot of complexities over a lot of places.

e.g. following the sysfs code
becomes quite a bit more difficult after the namespace changes (maybe it's just me but still).

So, I was asking whether people generally
agree that having the namespace thing is worth the added complexities.

I think it serves pretty small group of users. Hosting service providers and people trying to migrate processes from one machine to another, both of which can be served pretty well with virtualization. It does have higher overhead both processing power and memory wise but IIUC the former is being actively worked on w/ new processor features like nested paging tables and all and memory is really cheap these days, so I'm a bit skeptical how much this is needed and how much we should pay for it.

Another venue to explore is whether the partial view of proc and sysfs can be implemented in less pervasive way. Implementing it via FUSE might not be easier per-se but I think it would be better to do it that way if we can instead of adding complexities to both proc and sysfs.

One last thing that came to mind is, how would uevents be handled? ie. what happens if a network card which is presented as ethN in the namespace goes away? How does the system deal with it?

Quoted 12 lines

Quoted 2 lines

Can somebody hammer the big picture regarding namespaces into my small head?

100,000 foot view. A namespace introduces a scope so multiple objects can have the same name. Like network devices.

10,000 foot view. The network namespace looks to user space as if the kernel has multiple independent network stacks.

1000 foot view. I have two network devices named lo, and sysfs does not currently have a place for me to put them.

Quoted 6 lines

Leakage and being able to fool an application that it has the entire kernel to itself are not concerns. The goal is simply to get the entire object name to object translation boundary and the namespace work is done. We have largely achieved, and the code to do so once complete is reasonable enough that it should be no worse than dealing with any other kernel bug.

Yes, I'm aware of the goals. What I'm curious about is the consensus regarding network namespace and all its implications. It adds a lot of complexities over a lot of places. e.g. following the sysfs code becomes quite a bit more difficult after the namespace changes (maybe it's just me but still). So, I was asking whether people generally agree that having the namespace thing is worth the added complexities.

I think it serves pretty small group of users. Hosting service

providers and people trying to migrate processes from one machine to another, both of which can be served pretty well with virtualization. It does have higher overhead both processing power and memory wise but IIUC the former is being actively worked on w/ new processor features like nested paging tables and all and memory is really cheap these days, so I'm a bit skeptical how much this is needed and how much we should pay for it.

Another venue to explore is whether the partial view of proc and sysfs can be implemented in less pervasive way. Implementing it via FUSE might not be easier per-se but I think it would be better to do it

that way if we can instead of adding complexities to both proc and sysfs.

One last thing that came to mind is, how would uevents be handled? ie. what happens if a network card which is presented as ethN in the namespace goes away? How does the system deal with it?

Thanks.

Signature · 1 line

tejun

Again fuse doesn't address the *core* issue (sysfs needing a way to create files for multiple devicenames with same name). But I believe Benjamin was looking into a minimal patch to fix that. Benjamin, have you gotten anywhere with that?

Quoted 3 lines · Tejun Heo

Aieeeee... I wanna run screaming and crying. Any chance these can be done using FUSE? FUSE is pretty flexible and should be able to emulate most of proc files w/o too much difficulty.

I don't see how FUSE can help. The problem is getting the information out of the kernel, and not breaking backwards compatiblity while we do it. As I understand FUSE it just allows for user space filesystems. Which is great if I want to hide information.

Quoted 4 lines

And can we do the same thing for sysfs using FUSE? So that not only the policy but also the implementation is in userland? The changes are quite pervasive and makes the whole thing pretty difficult to follow.

I don't see how. If userspace doesn't have the information I don't see how placing a filter will allow it to show up there.

The challenge is to not conflict on network device names. If someone can think of where we can put the network devices that are in different network namespaces in sysfs so they don't conflict when they have the same name I have no problem with that. But where can we put them?

Quoted 5 lines

Quoted 5 lines

2) i_mutex seems to protect very little if anything that we care about. The dcache has it's own set of locks. So we may be able to completely avoid taking i_mutex in sysfs and simplify things enormously. Currently I believe we are very similar to ocfs2 in terms of locking requirements.

Quoted 1 line

I think the timestamps are one of the things it protects.

Yes. I think parts of the page cache and anything in the inode itself is protected by i_mutex. As for timestamsp or anything else that we really care about we can and should put them in sysfs_dirent and we can have the stat method recreate it, and possibly have d_revalidate refresh it.

Quoted 7 lines

Quoted 3 lines

3) For i_notify and d_notify that seems to require pinning the inode or the dentry in question, so I see no reason why a d_revalidate style of update will have problems.

Because the existing notifications won't be moved over to the new dentry. dnotify wouldn't work the same way. ISTR that was the reason why I didn't do the d_revalidate thing, but I don't think it really matters. dnotify on sysfs nodes doesn't work properly anyway.

Reasonable. I have seen two ways of handling rename properly. Some weird variant d_splice_alias or some cleaner variant of what we are doing today.

Quoted 5 lines

Quoted 3 lines

4) For finer locking granularity of readdir. All we need to do is do the semi-expensive restart for each dirent, and the problem is trivially solved.

That can show the same entry multiple times or skip existing entries. I think it's better to put fake entries and implement iterators.

The guarantee is that we will see all entries that are there for the duration of readdir, we order the directory by inode, and stick the inode number in f_pos. So now we don't have the problem of returning the same entry multiple times or skipping existing entries.

Quoted 3 lines

Quoted 1 line

5) Large directories are a potential performance problem in sysfs.

Yes, it is. It hasn't been an issue till now. You're worrying about look up performance, right?

Lookup, create, unlink and if we drop the lock during readdir, readdir restart. The all require a linear scan.

Quoted 5 lines

If that's a real concern we can link sd's
into a hash table, but I'm not sure tho. For listing, O(n) is the best we can do anyway and after the initial lookup, the result would be cached via dcache anyway, so I'm not really sure how much adding a hashtable will buy us.

Depends on how many devices people are adding and removing dynamically I guess. sysctl has had that issue so I am thinking about it. I figure we need to make things work properly first.

Quoted 9 lines

Quoted 6 lines

Leakage and being able to fool an application that it has the entire kernel to itself are not concerns. The goal is simply to get the entire object name to object translation boundary and the namespace work is done. We have largely achieved, and the code to do so once complete is reasonable enough that it should be no worse than dealing with any other kernel bug.

Yes, I'm aware of the goals. What I'm curious about is the consensus regarding network namespace and all its implications. It adds a lot of complexities over a lot of places.

Not really. It is really very straight forward. 99% of the modified code simply has an extra pointer dereference.

Except for sysfs the network namespace code that has merged is in a very usable state. There are a few little things like iptables support that still needs some work. From a practical standpoint sysfs was one of the first things I started working on and it is one of the last things to be done.

Quoted 3 lines

e.g. following the sysfs code
becomes quite a bit more difficult after the namespace changes (maybe it's just me but still).

Some of it yes. Which asks for a more comprehensive solution. Part of the challenge is that there has been insistence on an especially generic solution, in sysfs and I'm not certain that has helped.

Quoted 2 lines

So, I was asking whether people generally agree that having the namespace thing is worth the added complexities.

To my knowledge yes. Most of the cost is trivial, and it makes a darn good excuse to clean up problem code.

Quoted 8 lines

I think it serves pretty small group of users. Hosting service providers and people trying to migrate processes from one machine to another, both of which can be served pretty well with virtualization. It does have higher overhead both processing power and memory wise but IIUC the former is being actively worked on w/ new processor features like nested paging tables and all and memory is really cheap these days, so I'm a bit skeptical how much this is needed and how much we should pay for it.

So far sysfs is the most costly and the hardest part. Most of the cost is in the noise and in the design.

One thing the namespaces fundamentally get you is scaling. You can run probably 10x more environments on a single server. Which makes then cheaper and available, on all hardware.

Beyond that there are people who actually just want to use a single namespace for what you can do. They are general tools and are useful in more ways than just checkpoint restart and virtualization.

Think what happens if you are a switch/router and you switch two different networks both using overlaping addresses in the 10.x segment.

Or think how much easier it is to test routing with just a single machine.

All kinds of interesting uses.

Quoted 5 lines

Another venue to explore is whether the partial view of proc and sysfs can be implemented in less pervasive way. Implementing it via FUSE might not be easier per-se but I think it would be better to do it that way if we can instead of adding complexities to both proc and sysfs.

This isn't a partial view thing really. This is how do I put it all in there not have conflicts and preserve backwards compatibility.

In proc. I have work as hard as I can to build a design that will let us see it all without sacrificing backwards compatibility. With /proc/<pid> I have a natural place to put data in a per process view. I don't have that in sysfs, and sysfs at some point stopped being about just the hardware. So the only way I have found to have places for everything is to do multiple mounts.

Quoted 3 lines

One last thing that came to mind is, how would uevents be handled? ie. what happens if a network card which is presented as ethN in the namespace goes away? How does the system deal with it?

It is probably worth a double check. Coming in all physical network devices happen in the initial network namespace so that direction isn't a problem. Worse case I expect we figure out how to add a field that specifies enough about the network namespace so the events can be relayed to appropriate part of user space.

Quoted 4 lines · Serge E. Hallyn

Again fuse doesn't address the *core* issue (sysfs needing a way to create files for multiple devicenames with same name). But I believe Benjamin was looking into a minimal patch to fix that. Benjamin, have you gotten anywhere with that?

I would love to hear a minimal strategy for that.

The only minimal strategy user space wise is to create multiple superblocks. Anything else I an think of violates backwards compatibility.

Quoted 6 lines · Eric W. Biederman

Quoted 4 lines · Serge E. Hallyn

Again fuse doesn't address the *core* issue (sysfs needing a way to create files for multiple devicenames with same name). But I believe Benjamin was looking into a minimal patch to fix that. Benjamin, have you gotten anywhere with that?

Quoted 1 line

I would love to hear a minimal strategy for that.

Oh I just meant for kernel-space. So if a container is creating lo, it will create a device named lo, but the sysfs file will be called lo_1 or something.

Quoted 3 lines

The only minimal strategy user space wise is to create multiple superblocks. Anything else I an think of violates backwards compatibility.

Yes, the above would require that the container either not mount sysfs, ignore sysfs, or tweak sysfs using

	mount -t tmpfs none /sys/class/net
	mount --bind /sys/devices/virtual/net/lo_1 /sys/class/net/lo

or using fuse.

I'd definately prefer the sysfs tagging approach. But I'd prefer the above over never being able to use network namespaces on a standard distro (with sysfs enabled).

-serge

Quoted 9 lines · Eric W. Biederman

Quoted 5 lines · Tejun Heo

Quoted 5 lines

2) i_mutex seems to protect very little if anything that we care about. The dcache has it's own set of locks. So we may be able to completely avoid taking i_mutex in sysfs and simplify things enormously. Currently I believe we are very similar to ocfs2 in terms of locking requirements.

Quoted 7 lines

Quoted 1 line

I think the timestamps are one of the things it protects.

Yes. I think parts of the page cache and anything in the inode itself is protected by i_mutex. As for timestamsp or anything else that we really care about we can and should put them in sysfs_dirent and we can have the stat method recreate it, and possibly have d_revalidate refresh it.

Some of the timestamps are not on the sysfs_dirent because 1. nobody cared (the original sd implementation didn't preserve it) and
2. of memory overhead.

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

3) For i_notify and d_notify that seems to require pinning the inode or the dentry in question, so I see no reason why a d_revalidate style of update will have problems.

Because the existing notifications won't be moved over to the new dentry. dnotify wouldn't work the same way. ISTR that was the reason why I didn't do the d_revalidate thing, but I don't think it really matters. dnotify on sysfs nodes doesn't work properly anyway.

Quoted 3 lines

Reasonable. I have seen two ways of handling rename properly. Some weird variant d_splice_alias or some cleaner variant of what we are doing today.

FWIW, I think it would be just fine to invalidate the renamed dentry.

Quoted 3 lines

Depends on how many devices people are adding and removing dynamically I guess. sysctl has had that issue so I am thinking about it. I figure we need to make things work properly first.

Yeap, let's think about optimization later. The problem hasn't come up yet even on machines where the memory footprint of sysfs dentries and inodes posed serious problems, so I don't think optimizing it is a high priority at this point.

Well, I suppose most of that blame falls on me but I still can't bring myself to agree with the current implementation. The biggest problem I have is that the implementation doesn't really show in straight forward manner what it tries to achieve (showing partial tree depending on sb).

Getting the clean up part in usually isn't a problem, right? But getting in the actual namespace part is (and should be).