Messages in this thread | | | Date | Fri, 22 Mar 2013 12:43:50 -0700 | Subject | Re: [CFT] Re: VFS deadlock ? | From | Linus Torvalds <> |
| |
On Thu, Mar 21, 2013 at 10:33 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > And I wonder how big of a deal the aggressive dentry deletion is. > Maybe it's even ok to allocate/free the inodes all the time. The whole > "get the inode hash lock and look it up there" can't be all that > wonderful either. It takes the inode->i_lock for each entry it finds > on the hash list, which looks horrible. I suspect our slab allocator > isn't much worse than that, although the RCU freeing of the inodes > could end up being problematic.
I tested this out by just having a process that keeps stat'ing the same /proc inode over and over and over again, which should be pretty much the worst-case situation (because we will delete the dentry after each stat, and so we'll repopulate it for each stat)
The allocation overhead seems to be in the noise. The real costs are all in proc_lookup_de() just finding the entry, with strlen/strcmp being much hotter. So if we actually care about performance for these cases, what we would actually want to do is to just cache the dentries and have some kind of timeout instead of getting rid of them immediately. That would get rid of the lookup costs, and in the process also get rid of the constant inode allocation/deallocation costs.
There was also some locking overhead, but that's also mainly dentry-related, with the inode side being visible but not dominant. Again, not immediately expiring the dentries would make all that just go away.
Regardless, the patch seems to be the right thing to do. It actually simplifies /proc, seems to fix the problem for Dave, and is small and should be trivial to backport. I've committed it and marked it for stable. Let's hope there won't be any nasty surprises, but it does seem to be the simplest non-hacky patch.
Linus
| |