| Message ID | 162218354775.34379.5629941272050849549.stgit@web.messagingengine.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | kernfs: proposed locking and concurrency improvement | expand |
On Fri, May 28, 2021 at 02:33:42PM +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. Fox, can you take some time and test these to verify it all still works properly with your benchmarks? thanks, greg k-h
On Fri, May 28, 2021 at 4:56 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Fri, May 28, 2021 at 02:33:42PM +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. > > Fox, can you take some time and test these to verify it all still works > properly with your benchmarks? Sure, I will take a look. Actually, I've tested it before, but I will test it again to confirm it. > thanks, > > greg k-h thanks, fox
On Fri, May 28, 2021 at 4:56 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > Fox, can you take some time and test these to verify it all still works > properly with your benchmarks? > I've tested it on an AWS C5a (amd, 96 logical cores): Before, mutex_locks in kernfs_iop_permission(), kernfs_dop_revalidate take significant time. With the patchset, there is no mutex_lock issue. (see flamegraph before.png/after.png) On AWS C5 (intel, also 96 logical cores), the benchmark runs slower than on c5a. But I don't think it's related, because running the benchmark on ext4 is slower too, and the perf report, which is no different from running on kernfs with this patchset, shows the pressure is on the VFS side. My conclusion: It works well with my benchmark. I've attached: flame graphs -- before.png/after.png benchmark outputs -- result.before/result.after perf reports -- report.before/report.after perf report on ext4 -- report.baremetal.ext4 for you reference. thanks, fox
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