| Message ID | 20240715071324.265879-1-mjguzik@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | vfs: use RCU in ilookup | expand |
On 15-Jul-24 12:43 PM, Mateusz Guzik wrote: > A soft lockup in ilookup was reported when stress-testing a 512-way > system [1] (see [2] for full context) and it was verified that not > taking the lock shifts issues back to mm. > > [1] https://lore.kernel.org/linux-mm/56865e57-c250-44da-9713-cf1404595bcc@amd.com/ Mateusz, Just want to mention explicitly that in addition to the lockless lookup changes that you suggested in [1], the test run also included your other commit https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs.inode.rcu&id=7180f8d91fcbf252de572d9ffacc945effed0060 as you had suggested. Regards, Bharata.
On Mon, Jul 15, 2024 at 10:02 AM Bharata B Rao <bharata@amd.com> wrote: > > On 15-Jul-24 12:43 PM, Mateusz Guzik wrote: > > A soft lockup in ilookup was reported when stress-testing a 512-way > > system [1] (see [2] for full context) and it was verified that not > > taking the lock shifts issues back to mm. > > > > [1] https://lore.kernel.org/linux-mm/56865e57-c250-44da-9713-cf1404595bcc@amd.com/ > > Mateusz, > > Just want to mention explicitly that in addition to the lockless lookup > changes that you suggested in [1], the test run also included your other > commit > https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs.inode.rcu&id=7180f8d91fcbf252de572d9ffacc945effed0060 > as you had suggested. > That commit is needed to have this compile to begin with, so it was kind of implied. :)
On Mon, 15 Jul 2024 09:13:24 +0200, Mateusz Guzik wrote: > A soft lockup in ilookup was reported when stress-testing a 512-way > system [1] (see [2] for full context) and it was verified that not > taking the lock shifts issues back to mm. > > [1] https://lore.kernel.org/linux-mm/56865e57-c250-44da-9713-cf1404595bcc@amd.com/ > [2] https://lore.kernel.org/linux-mm/d2841226-e27b-4d3d-a578-63587a3aa4f3@amd.com/ > > [...] Applied to the vfs.inode.rcu branch of the vfs/vfs.git tree. Patches in the vfs.inode.rcu branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.inode.rcu [1/1] vfs: use RCU in ilookup https://git.kernel.org/vfs/vfs/c/2eae762dece6
On Mon 15-07-24 09:13:24, Mateusz Guzik wrote: > A soft lockup in ilookup was reported when stress-testing a 512-way > system [1] (see [2] for full context) and it was verified that not > taking the lock shifts issues back to mm. > > [1] https://lore.kernel.org/linux-mm/56865e57-c250-44da-9713-cf1404595bcc@amd.com/ > [2] https://lore.kernel.org/linux-mm/d2841226-e27b-4d3d-a578-63587a3aa4f3@amd.com/ > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> Looks good to me. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > > fwiw the originally sent patch to the reporter performs a lockless > lookup first and falls back to the locked variant, but that was me > playing overfly safe. > > I would add tested-by but patches are not the same in the end. > > This is the only spot which can get this fixup, everything else taking > the lock is also using custom callbacks, so filesystems invoking such > code will need to get patched up on case-by-case basis (but > realistically they probably already can do RCU-only operation). > > fs/inode.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/fs/inode.c b/fs/inode.c > index f356fe2ec2b6..52ca063c552c 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -1525,9 +1525,7 @@ 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 (inode) { > if (IS_ERR(inode)) > -- > 2.43.0 >
diff --git a/fs/inode.c b/fs/inode.c index f356fe2ec2b6..52ca063c552c 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -1525,9 +1525,7 @@ 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 (inode) { if (IS_ERR(inode))
A soft lockup in ilookup was reported when stress-testing a 512-way system [1] (see [2] for full context) and it was verified that not taking the lock shifts issues back to mm. [1] https://lore.kernel.org/linux-mm/56865e57-c250-44da-9713-cf1404595bcc@amd.com/ [2] https://lore.kernel.org/linux-mm/d2841226-e27b-4d3d-a578-63587a3aa4f3@amd.com/ Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> --- fwiw the originally sent patch to the reporter performs a lockless lookup first and falls back to the locked variant, but that was me playing overfly safe. I would add tested-by but patches are not the same in the end. This is the only spot which can get this fixup, everything else taking the lock is also using custom callbacks, so filesystems invoking such code will need to get patched up on case-by-case basis (but realistically they probably already can do RCU-only operation). fs/inode.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)