On Mon, Jul 15, 2024 at 7:22 AM Bharata B Rao <bharata@xxxxxxx> wrote: > > On 10-Jul-24 6:34 PM, Mateusz Guzik wrote: > >>> However the contention now has shifted to inode_hash_lock. Around 55 > >>> softlockups in ilookup() were observed: > >>> > >>> # tracer: preemptirqsoff > >>> # > >>> # preemptirqsoff latency trace v1.1.5 on 6.10.0-rc3-trnmglru > >>> # -------------------------------------------------------------------- > >>> # latency: 10620430 us, #4/4, CPU#260 | (M:desktop VP:0, KP:0, SP:0 HP:0 > >>> #P:512) > >>> # ----------------- > >>> # | task: fio-3244715 (uid:0 nice:0 policy:0 rt_prio:0) > >>> # ----------------- > >>> # => started at: ilookup > >>> # => ended at: ilookup > >>> # > >>> # > >>> # _------=> CPU# > >>> # / _-----=> irqs-off/BH-disabled > >>> # | / _----=> need-resched > >>> # || / _---=> hardirq/softirq > >>> # ||| / _--=> preempt-depth > >>> # |||| / _-=> migrate-disable > >>> # ||||| / delay > >>> # cmd pid |||||| time | caller > >>> # \ / |||||| \ | / > >>> fio-3244715 260...1. 0us$: _raw_spin_lock <-ilookup > >>> fio-3244715 260.N.1. 10620429us : _raw_spin_unlock <-ilookup > >>> fio-3244715 260.N.1. 10620430us : tracer_preempt_on <-ilookup > >>> fio-3244715 260.N.1. 10620440us : <stack trace> > >>> => _raw_spin_unlock > >>> => ilookup > >>> => blkdev_get_no_open > >>> => blkdev_open > >>> => do_dentry_open > >>> => vfs_open > >>> => path_openat > >>> => do_filp_open > >>> => do_sys_openat2 > >>> => __x64_sys_openat > >>> => x64_sys_call > >>> => do_syscall_64 > >>> => entry_SYSCALL_64_after_hwframe > >>> > >>> It appears that scalability issues with inode_hash_lock has been brought > >>> up multiple times in the past and there were patches to address the same. > >>> > >>> https://lore.kernel.org/all/20231206060629.2827226-9-david@xxxxxxxxxxxxx/ > >>> https://lore.kernel.org/lkml/20240611173824.535995-2-mjguzik@xxxxxxxxx/ > >>> > >>> CC'ing FS folks/list for awareness/comments. > >> > >> Note my patch does not enable RCU usage in ilookup, but this can be > >> trivially added. > >> > >> I can't even compile-test at the moment, but the diff below should do > >> it. Also note the patches are present here > >> https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs.inode.rcu > >> , not yet integrated anywhere. > >> > >> That said, if fio you are operating on the same target inode every > >> time then this is merely going to shift contention to the inode > >> spinlock usage in find_inode_fast. > >> > >> diff --git a/fs/inode.c b/fs/inode.c > >> index ad7844ca92f9..70b0e6383341 100644 > >> --- a/fs/inode.c > >> +++ b/fs/inode.c > >> @@ -1524,10 +1524,14 @@ struct inode *ilookup(struct super_block *sb, > >> unsigned long ino) > >> { > >> struct hlist_head *head = inode_hashtable + hash(sb, ino); > >> struct inode *inode; > >> + > >> again: > >> - spin_lock(&inode_hash_lock); > >> - inode = find_inode_fast(sb, head, ino, true); > >> - spin_unlock(&inode_hash_lock); > >> + inode = find_inode_fast(sb, head, ino, false); > >> + if (IS_ERR_OR_NULL_PTR(inode)) { > >> + spin_lock(&inode_hash_lock); > >> + inode = find_inode_fast(sb, head, ino, true); > >> + spin_unlock(&inode_hash_lock); > >> + } > >> > >> if (inode) { > >> if (IS_ERR(inode)) > >> > > > > I think I expressed myself poorly, so here is take two: > > 1. inode hash soft lookup should get resolved if you apply > > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs.inode.rcu&id=7180f8d91fcbf252de572d9ffacc945effed0060 > > and the above pasted fix (not compile tested tho, but it should be > > obvious what the intended fix looks like) > > 2. find_inode_hash spinlocks the target inode. if your bench only > > operates on one, then contention is going to shift there and you may > > still be getting soft lockups. not taking the spinlock in this > > codepath is hackable, but I don't want to do it without a good > > justification. > > Thanks Mateusz for the fix. With this patch applied, the above mentioned > contention in ilookup() has not been observed for a test run during the > weekend. > Ok, I'll do some clean ups and send a proper patch to the vfs folks later today. Thanks for testing. -- Mateusz Guzik <mjguzik gmail.com>
