On Thu, May 13, 2021 at 09:50:19PM +0800, Ian Kent wrote: > On Wed, 2021-05-12 at 08:21 +0200, Greg Kroah-Hartman wrote: > > On Wed, May 12, 2021 at 08:38:35AM +0800, Ian Kent wrote: > > > There have been a few instances of contention on the kernfs_mutex > > > during > > > path walks, a case on very large IBM systems seen by myself, a > > > report by > > > Brice Goglin and followed up by Fox Chen, and I've since seen a > > > couple > > > of other reports by CoreOS users. > > > > > > The common thread is a large number of kernfs path walks leading to > > > slowness of path walks due to kernfs_mutex contention. > > > > > > The problem being that changes to the VFS over some time have > > > increased > > > it's concurrency capabilities to an extent that kernfs's use of a > > > mutex > > > is no longer appropriate. There's also an issue of walks for non- > > > existent > > > paths causing contention if there are quite a few of them which is > > > a less > > > common problem. > > > > > > This patch series is relatively straight forward. > > > > > > All it does is add the ability to take advantage of VFS negative > > > dentry > > > caching to avoid needless dentry alloc/free cycles for lookups of > > > paths > > > that don't exit and change the kernfs_mutex to a read/write > > > semaphore. > > > > > > The patch that tried to stay in VFS rcu-walk mode during path walks > > > has > > > been dropped for two reasons. First, it doesn't actually give very > > > much > > > improvement and, second, if there's a place where mistakes could go > > > unnoticed it would be in that path. This makes the patch series > > > simpler > > > to review and reduces the likelihood of problems going unnoticed > > > and > > > popping up later. > > > > > > The patch to use a revision to identify if a directory has changed > > > has > > > also been dropped. If the directory has changed the dentry revision > > > needs to be updated to avoid subsequent rb tree searches and after > > > changing to use a read/write semaphore the update also requires a > > > lock. > > > But the d_lock is the only lock available at this point which might > > > itself be contended. > > > > > > Changes since v3: > > > - remove unneeded indirection when referencing the super block. > > > - check if inode attribute update is actually needed. > > > > > > Changes since v2: > > > - actually fix the inode attribute update locking. > > > - drop the patch that tried to stay in rcu-walk mode. > > > - drop the use a revision to identify if a directory has changed > > > patch. > > > > > > Changes since v1: > > > - fix locking in .permission() and .getattr() by re-factoring the > > > attribute > > > handling code. > > > --- > > > > > > Ian Kent (5): > > > kernfs: move revalidate to be near lookup > > > kernfs: use VFS negative dentry caching > > > kernfs: switch kernfs to use an rwsem > > > kernfs: use i_lock to protect concurrent inode updates > > > kernfs: add kernfs_need_inode_refresh() > > > > > > > > > fs/kernfs/dir.c | 170 ++++++++++++++++++++------------ > > > ---- > > > fs/kernfs/file.c | 4 +- > > > fs/kernfs/inode.c | 45 ++++++++-- > > > fs/kernfs/kernfs-internal.h | 5 +- > > > fs/kernfs/mount.c | 12 +-- > > > fs/kernfs/symlink.c | 4 +- > > > include/linux/kernfs.h | 2 +- > > > 7 files changed, 147 insertions(+), 95 deletions(-) > > > > > > -- > > > Ian > > > > > > > Any benchmark numbers that you ran that are better/worse with this > > patch > > series? That woul dbe good to know, otherwise you aren't changing > > functionality here, so why would we take these changes? :) > > Hi Greg, > > I'm sorry, I don't have a benchmark. > > My continued work on this has been driven by the report from > Brice Goglin and Fox Chen, and also because I've seen a couple > of other reports of kernfs_mutex contention that is resolved > by the series. > > Unfortunately the two reports I've seen fairly recently are on > kernels that are about as far away from the upstream kernel > as you can get so probably aren't useful in making my case. > > The report I've worked on most recently is on CoreOS/Kunbernetes > (based on RHEL-8.3) where the machine load goes to around 870 > after loading what they call an OpenShift performance profile. > > I looked at some sysreq dumps and they have a seemingly endless > number of processes waiting on the kernfs_mutex. > > I tried to look at the Kubernetes source but it's written in > go so there would need to be a lot of time spent to work out > what's going on, I'm trying to find someone to help with that. > > All I can say from looking at the Kubernetes source is it has > quite a few sysfs paths in it so I assume it uses sysfs fairly > heavily. > > The other problem I saw was also on CoreOS/Kunernetes. > A vmcore analysis showed kernfs_mutex contention but with a > different set of processes and not as significant as the former > problem. > > So, even though this isn't against the current upstream, there > isn't much difference in the kernfs/sysfs source between those > two kernels and given the results of Brice and Fox I fear I'll > be seeing more of this as time goes by. > > I'm fairly confident that the user space applications aren't > optimal (although you may have stronger words for it than that) > I was hoping you would agree that it's sensible for the kernel > to protect itself to the extent that it can provided the change > is straight forward enough. > > I have tried to make the patches as simple as possible without > loosing much of the improvement to minimize any potential ongoing > maintenance burden. > > So, I'm sorry I can't offer you more incentive to consider the > series, but I remain hopeful you will. At the very least, if you could test the series on those "older" systems and say "booting went from X seconds to Y seconds!". Otherwise, while changes are nice, without a real-world test that this actually made any difference at all, why would we take these changes? thanks, greg k-h
