Re: [PATCH kernfs 1/3] kernfs: switch global kernfs_idr_lock to per-fs lock

[Jinliang Zheng]

> On Sat, Apr 12, 2025 at 07:50:54PM +0800, alexjlzheng@xxxxxxxxx wrote:
> > On Sat, 12 Apr 2025 08:12:22 +0200, gregkh@xxxxxxxxxxxxxxxxxxx wrote:
> > > On Sat, Apr 13, 2025 at 02:31:07AM +0800, alexjlzheng@xxxxxxxxx wrote:
> > > > From: Jinliang Zheng <alexjlzheng@xxxxxxxxxxx>
> > > > 
> > > > The kernfs implementation has big lock granularity(kernfs_idr_lock) so
> > > > every kernfs-based(e.g., sysfs, cgroup) fs are able to compete the lock.
> > > > 
> > > > This patch switches the global kernfs_idr_lock to per-fs lock, which
> > > > put the spinlock into kernfs_root.
> > > > 
> > > > Signed-off-by: Jinliang Zheng <alexjlzheng@xxxxxxxxxxx>
> > > > ---
> > > >  fs/kernfs/dir.c             | 14 +++++++-------
> > > >  fs/kernfs/kernfs-internal.h |  1 +
> > > >  2 files changed, 8 insertions(+), 7 deletions(-)
> > > 
> > > What kind of testing / benchmark did you do for this series that shows
> > > that this works, AND that this actually is measureable?  What workload
> > > are you doing that causes these changes to be needed?
> > 
> > Thank you for your reply. :)
> > 
> > We are trying to implement a kernfs-based filesystem that will have
> > multiple instances running at the same time, i.e., multiple kernfs_roots.
> 
> I don't think that kernfs is meant for that very well, what is that
> filesystem going to be for?

Thank you for your reply. :)

Similar to cgroupfs and sysfs, it is used to export the status and configurations
of some kernel variables in hierarchical modes of the kernel. The only difference
is that it may have many instances, that is, many kernfs_roots.

> 
> > While investigating the kernfs implementation, we found some global locks
> > that would cause noticeable lock contention when there are many filesystem
> > instances.
> > 
> > Fortunately, we found that some optimizations have been made in [1], which
> > moved kernfs_rwsem into kernfs_root. But there are still some global locks
> > left.
> > 
> > We think it is also necessary to switch the remaining global locks to
> > per-fs. Moreover, we strongly agree with Tejun Heo's point in [1]:
> > 
> >   "... this is the right thing to do even if there is no concrete
> >    performance argument (not saying there isn't). It's just weird to
> >    entangle these completely unrelated users in a single rwsem."
> > 
> > We think kernfs will be widely used to build other filesystems, so we
> > strongly recommend switching global locks to per-fs.
> 
> I don't strongly object, but I would like to see some real-world numbers first.

Haha, we are still evaluating whether to implement it based on kernfs, so it
has not been implemented and tested yet. However, for these global locks, I
think it is more reasonable to switch to per-fs, regardless of whether there
is a significant performance improvement (in fact, when the number of kernfs-based
filesystem instances increases, the lock contention will definitely be reduced.
At least, it will not get worse, hahaha).

Haha, but if you think it is not necessary to do this, just ignore this patchset.

Thank you,
Jinliang Zheng. :)

> 
> thanks,
> 
> greg k-h