Re: [PATCH 0/6] kernfs: proposed locking and concurrency improvement

[Ian Kent <raven@themaw.net>]

On Wed, 2020-12-23 at 15:30 +0800, Fox Chen wrote:
> Hi Ian,
> 
> On Tue, Dec 22, 2020 at 3:47 PM Ian Kent <raven@themaw.net> wrote:
> > Here is what I currently have for the patch series we were
> > discussing
> > recently.
> > 
> > I'm interested to see how this goes with the problem you are
> > seeing.
> > 
> > The last patch in the series (the attributes update patch) has seen
> > no more than compile testing, I hope I haven't messed up on that.
> 
> OK, I just ignored the last patch (kernfs: add a spinlock to kernfs
> iattrs for inode updates)
> because I can not boot the kernel after applying it.

Right, clearly I haven't got that last patch right yet.

It looks like the problem there is that the iattrs structure might
not be allocated at the time so clearly I can't just use the spin
lock inside.

It would be simple enough to resolve except for the need to set
the inode link count too.

Ian

> 
> Applying the first five patches, my benchmark shows no improvement
> this time,
> one open+read+close cycle spends 500us. :(
> 
> perf suggests that down_write in kernfs_iop_permission drags the most
> performance
> (full report see attachment) which makes sense to me, since
> down_write
> has no difference
> here compared to the mutex, they all allow only one thread running
> simultaneously.
> 
> sigh... It's really hard to fix.
> 
>    |
>    |--51.81%--inode_permission
>    |          |
>    |           --51.36%--kernfs_iop_permission
>    |                     |
>    |                     |--50.41%--down_write
>    |                     |          |
>    |                     |           --50.32%
> --rwsem_down_write_slowpath
>    |                     |                     |
>    |                     |                      --49.67%
> --rwsem_optimistic_spin
>    |                     |                                |
>    |                     |                                 --49.31%
> --osq_lock
>    |                     |
>    |                      --0.74%--up_write
>    |                                rwsem_wake.isra.0
>    |                                |
>    |                                 --0.69%--wake_up_q
>    |                                           |
>    |                                            --0.62%
> --try_to_wake_up
>    |                                                      |
>    |
> --0.54%--_raw_spin_lock_irqsave
>    |                                                                 
> |
>    |
> --0.52%--native_queued
>    |
>     --29.90%--walk_component
>               |
>                --29.72%--lookup_fast
>                          |
>                           --29.51%--kernfs_dop_revalidate
>                                     |
>                                     |--28.88%--down_read
>                                     |          |
>                                     |
> --28.75%--rwsem_down_read_slowpath
>                                     |                     |
>                                     |
> --28.50%--rwsem_optimistic_spin
>                                     |                                
> |
>                                     |
> --28.38%--osq_lock
>                                     |
>                                      --0.59%--up_read
>                                                rwsem_wake.isra.0
>                                                |
>                                                 --0.54%--wake_up_q
>                                                           |
> 
> --0.53%--try_to_wake_up
> 
> > Please keep in mind that Greg's continued silence on the question
> > of whether the series might be re-considered indicates to me that
> > he has not changed his position on this.
> 
> Since the problem doesn't break the system, it doesn't need immediate
> attention.
> So I bet it must be labeled with a low priority in Greg's to-do list.
> And it's merging
> window & holiday season. Let's just give him more time. :)
> 
> 
> > ---
> > 
> > Ian Kent (6):
> >       kernfs: move revalidate to be near lookup
> >       kernfs: use VFS negative dentry caching
> >       kernfs: use revision to identify directory node changes
> >       kernfs: switch kernfs to use an rwsem
> >       kernfs: stay in rcu-walk mode if possible
> >       kernfs: add a spinlock to kernfs iattrs for inode updates
> > 
> > 
> >  fs/kernfs/dir.c             |  285 ++++++++++++++++++++++++++++---
> > ------------
> >  fs/kernfs/file.c            |    4 -
> >  fs/kernfs/inode.c           |   19 ++-
> >  fs/kernfs/kernfs-internal.h |   30 ++++-
> >  fs/kernfs/mount.c           |   12 +-
> >  fs/kernfs/symlink.c         |    4 -
> >  include/linux/kernfs.h      |    5 +
> >  7 files changed, 238 insertions(+), 121 deletions(-)
> > 
> > --
> > Ian
> > 
> 
> thanks,
> fox