On Thu, 2021-05-13 at 17:19 +0200, Greg Kroah-Hartman wrote: > 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!". The last test I did was done on the system showing high load and it went from around 870 to around 3. It completely resolved the reported problem. I need to have the current patches re-tested and that can take a while and I need to look at Fox's results results, I'm thinking the additional patch in v4 is probably not needed. Ian
