Re: [RESEND PATCH 4/5] kernfs: Replace per-fs rwsem with hashed rwsems.

[Imran Khan]

Hello Tejun,

I am very sorry for replying late, I was away from work for most of
last couple of months. Also apologies for this lengthy reply but
I want to share some data here from recent experiments to see if
some alternative approach can be used here.

> * I find the returning-with-rwsem-held interface and usage odd. We return
>   with locks held all the time, so that part in itself is fine but how it's
>   used in the proposed patch is pretty alien.
> 

The interface for acquiring and releasing hashed rwsem has been
kept similar to interface for acquiring and releasing hashed mutex (which
was done in part-1 of this work at [1].
Could you please clarify which aspect of its usage you find alien here?
Probably there is some scope of improvement there.

> * I don't understand why the topo_mutex is needed. What is its relationship
>   with rename_lock?
> 

topo_mutex is providing synchronization between path walk/lookup and remove
operations.
While looking for a node we will not have address of corresponding node
so we can't acquire node's rwsem right from the beginning.
On the other hand a parallel remove operation for the same node can acquire
corresponding rwsem and go ahead with node removal. So it may happen that
search operation for the node finds and returns it but before it can be
pinned or used, the remove operation, that was going on in parallel, removes
the node and hence makes its any future use wrong.
topo_mutex ensures that for competing search and remove operations only
one proceeds at a time and since object returned by search is pinned before
releasing the topo_mutex, it will be available for subsequent usage.

I understand that kernfs_rename_lock protects against parent change but it does
not prevent against node getting delinked from its sibling list or getting removed.

Further during path walks we need to acquire and release rwsems corresponding to
directories so that these directories don't move and their children RB tree
do not change. Since these rwsems can't be taken under a spinlock,
kernfs_rename_lock can't be used and needed protection against topology change
is provided by topo_mutex.

In current scheme of things global kernfs_rwsem automatically ensures that
amongst move, removal and addition of kernfs_node, only one proceeds at time. So
operations like kernfs_walk_ns can complete without any node getting
added/moved/removed in between.
With the approach of hashed kernfs_rwsem, topo_mutex is being used for similar
use cases where we need to ensure that no node gets added/moved/removed during
an operation.

Ideally I would like to avoid topo_mutex because this too is a global lock but I
am not sure if for all kernfs operations we will always be able to get and hold
hashed kernfs_rwsem for all involved nodes in a consistent manner.

These were my reasons for using topo_mutex with hashed kernfs_rwsem. Please let
me know if you still find use of topo_mutex problematic

> * Can't the double/triple lock helpers loop over the sorted list instead of
>   if'ing each case?
>
Could you please elaborate this query? Do you mean list of nodes in a path or
list of sibling nodes ?As per my understanding even with triple locks there is
no way to ensure all involved nodes remain consistent.


Below is reasoning and data for my experiments with other approaches.

Since hashed kernfs_rwsem approach has been encountering problems in addressing
some corner cases, I am thinking if some alternative approach can be taken here
to keep kernfs_rwsem global, but replace its usage at some places with
alternative global/hashed rwsems.

For example from the current kernfs code we can see that most of the contention
towards kernfs_rwsem is observed in down_read/up_read emanating from
kernfs_iop_permission and kernfs_dop_revalidate:

	-   39.16%    38.98%  showgids     [kernel.kallsyms]      [k] down_read
             38.98% __libc_start_main
                __open_nocancel
                entry_SYSCALL_64_after_hwframe
                do_syscall_64
                sys_open
                do_sys_open
                do_filp_open
              - path_openat
                 - 36.54% link_path_walk
                    - 20.23% inode_permission
                       - __inode_permission
                          - 20.22% kernfs_iop_permission
                               down_read
                    - 15.06% walk_component
                         lookup_fast
                         d_revalidate.part.24
                         kernfs_dop_revalidate
                         down_read
                    - 1.25% kernfs_iop_get_link
                         down_read
                 - 1.25% may_open
                      inode_permission
                      __inode_permission
                      kernfs_iop_permission
                      down_read
                 - 1.20% lookup_fast
                      d_revalidate.part.24
                      kernfs_dop_revalidate
                      down_read
	-   28.96%    28.83%  showgids     [kernel.kallsyms]       [k] up_read
             28.83% __libc_start_main
                __open_nocancel
                entry_SYSCALL_64_after_hwframe
                do_syscall_64
                sys_open
                do_sys_open
                do_filp_open
              - path_openat
                 - 28.42% link_path_walk
                    - 18.09% inode_permission
                       - __inode_permission
                          - 18.07% kernfs_iop_permission
                               up_read
                    - 9.08% walk_component
                         lookup_fast
                       - d_revalidate.part.24
                          - 9.08% kernfs_dop_revalidate
                               up_read
                    - 1.25% kernfs_iop_get_link

In the above snippet down_read/up_read of kernfs_rwsem is taking ~68% of CPU. We
also know that cache line bouncing for kernfs_rwsem is major contributor towards
this overhead because as per [2], changing kernfs_rwsem to a per-cpu
kernfs_rwsem reduced this to a large extent.

Now kernfs_iop_permission is taking kernfs_rwsem to access inode attributes
which are not accessed in kernfs_dop_revalidate (the other path contending for
kernfs_rwsem). So if we use a separate rwsem for protecting inode attributes we
can see that contention towards kernfs_rwsem greatly reduces. For example using
a global rwsem (kernfs_iattr_rwsem) to protect inode attributes as per following
patch:

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 6acd9c3d4cff..f185427131f9 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -757,11 +757,13 @@ int kernfs_add_one(struct kernfs_node *kn)
                goto out_unlock;

        /* Update timestamps on the parent */
+       down_write(&root->kernfs_iattr_rwsem);
        ps_iattr = parent->iattr;
        if (ps_iattr) {
                ktime_get_real_ts64(&ps_iattr->ia_ctime);
                ps_iattr->ia_mtime = ps_iattr->ia_ctime;
        }
+       up_write(&root->kernfs_iattr_rwsem);

        up_write(&root->kernfs_rwsem);

@@ -1442,10 +1444,12 @@ static void __kernfs_remove(struct kernfs_node *kn)
                                pos->parent ? pos->parent->iattr : NULL;

                        /* update timestamps on the parent */
+                       down_write(&root->kernfs_iattr_rwsem);
                        if (ps_iattr) {
                                ktime_get_real_ts64(&ps_iattr->ia_ctime);
                                ps_iattr->ia_mtime = ps_iattr->ia_ctime;
                        }
+                       up_write(&root->kernfs_iattr_rwsem);

                        kernfs_put(pos);
                }
diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index 74f3453f4639..1b8bffc6d2d3 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -101,9 +101,9 @@ int kernfs_setattr(struct kernfs_node *kn, const struct
iattr *iattr)
        int ret;
        struct kernfs_root *root = kernfs_root(kn);

-       down_write(&root->kernfs_rwsem);
+       down_write(&root->kernfs_iattr_rwsem);
        ret = __kernfs_setattr(kn, iattr);
-       up_write(&root->kernfs_rwsem);
+       up_write(&root->kernfs_iattr_rwsem);
        return ret;
 }

@@ -119,7 +119,7 @@ int kernfs_iop_setattr(struct user_namespace *mnt_userns,
struct dentry *dentry,
                return -EINVAL;

        root = kernfs_root(kn);
-       down_write(&root->kernfs_rwsem);
+       down_write(&root->kernfs_iattr_rwsem);
        error = setattr_prepare(&init_user_ns, dentry, iattr);
        if (error)
                goto out;
@@ -132,7 +132,7 @@ int kernfs_iop_setattr(struct user_namespace *mnt_userns,
struct dentry *dentry,
        setattr_copy(&init_user_ns, inode, iattr);

 out:
-       up_write(&root->kernfs_rwsem);
+       up_write(&root->kernfs_iattr_rwsem);
        return error;
 }
@@ -189,10 +189,10 @@ int kernfs_iop_getattr(struct user_namespace *mnt_userns,
        struct kernfs_node *kn = inode->i_private;
        struct kernfs_root *root = kernfs_root(kn);

-       down_read(&root->kernfs_rwsem);
+       down_read(&root->kernfs_iattr_rwsem);
        kernfs_refresh_inode(kn, inode);
        generic_fillattr(&init_user_ns, inode, stat);
-       up_read(&root->kernfs_rwsem);
+       up_read(&root->kernfs_iattr_rwsem);

        return 0;
 }

@@ -285,10 +285,10 @@ int kernfs_iop_permission(struct user_namespace *mnt_userns,
        kn = inode->i_private;
        root = kernfs_root(kn);

-       down_read(&root->kernfs_rwsem);
+       down_read(&root->kernfs_iattr_rwsem);
        kernfs_refresh_inode(kn, inode);
        ret = generic_permission(&init_user_ns, inode, mask);
-       up_read(&root->kernfs_rwsem);
+       up_read(&root->kernfs_iattr_rwsem);

        return ret;
 }

diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
index fc5821effd97..4620b74f44b0 100644
--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h
@@ -47,6 +47,7 @@ struct kernfs_root {

        wait_queue_head_t       deactivate_waitq;
        struct rw_semaphore     kernfs_rwsem;
+       struct rw_semaphore     kernfs_iattr_rwsem;
 };



greatly reduces the CPU usage seen earlier in down_read/up_read:


 -   13.08%    13.02%  showgids       [kernel.kallsyms]       [k] down_read
             13.02% __libc_start_main
                __open_nocancel
                entry_SYSCALL_64_after_hwframe
                do_syscall_64
                sys_open
                do_sys_open
                do_filp_open
              - path_openat
                 - 12.18% link_path_walk
                    - 9.44% inode_permission
                       - __inode_permission
                          - 9.43% kernfs_iop_permission
                               down_read
                    - 2.53% walk_component
                         lookup_fast
                         d_revalidate.part.24
                         kernfs_dop_revalidate
                         down_read
                 + 0.62% may_open
        -   13.03%    12.97%  showgids       [kernel.kallsyms]      [k] up_read
             12.97% __libc_start_main
                __open_nocancel
                entry_SYSCALL_64_after_hwframe
                do_syscall_64
                sys_open
                do_sys_open
                do_filp_open
              - path_openat
                 - 12.86% link_path_walk
                    - 11.55% inode_permission
                       - __inode_permission
                          - 11.54% kernfs_iop_permission
                               up_read
                    - 1.06% walk_component
                         lookup_fast
                       - d_revalidate.part.24
                          - 1.06% kernfs_dop_revalidate

As can be seen down_read/up_read CPU usage is ~26% (compared to ~68% of default
case).

Further using a hashed rwsem for protecting inode attributes as per following patch:

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 6acd9c3d4cff..dfc0d2167d86 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -734,6 +734,7 @@ int kernfs_add_one(struct kernfs_node *kn)
        struct kernfs_iattrs *ps_iattr;
        bool has_ns;
        int ret;
+       struct rw_semaphore *rwsem;

        down_write(&root->kernfs_rwsem);

@@ -757,11 +758,13 @@ int kernfs_add_one(struct kernfs_node *kn)
                goto out_unlock;

        /* Update timestamps on the parent */
+       rwsem = kernfs_iattr_down_write(kn);
        ps_iattr = parent->iattr;
        if (ps_iattr) {
                ktime_get_real_ts64(&ps_iattr->ia_ctime);
                ps_iattr->ia_mtime = ps_iattr->ia_ctime;
        }
+       kernfs_iattr_up_write(rwsem, kn);

        up_write(&root->kernfs_rwsem);

@@ -1443,8 +1446,10 @@ static void __kernfs_remove(struct kernfs_node *kn)

                        /* update timestamps on the parent */
                        if (ps_iattr) {
+                               struct rw_semaphore *rwsem =
kernfs_iattr_down_write(pos->parent);
                                ktime_get_real_ts64(&ps_iattr->ia_ctime);
                                ps_iattr->ia_mtime = ps_iattr->ia_ctime;
+                               kernfs_iattr_up_write(rwsem, kn);
                        }

                        kernfs_put(pos);
diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index 74f3453f4639..2b3cd5a9464f 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -99,11 +99,12 @@ int __kernfs_setattr(struct kernfs_node *kn, const struct
iattr *iattr)
 int kernfs_setattr(struct kernfs_node *kn, const struct iattr *iattr)
 {
        int ret;
+       struct rw_semaphore *rwsem;
        struct kernfs_root *root = kernfs_root(kn);

-       down_write(&root->kernfs_rwsem);
+       rwsem = kernfs_iattr_down_write(kn);
        ret = __kernfs_setattr(kn, iattr);
-       up_write(&root->kernfs_rwsem);
+       kernfs_iattr_up_write(rwsem, kn);
        return ret;
 }

@@ -114,12 +115,13 @@ int kernfs_iop_setattr(struct user_namespace *mnt_userns,
struct dentry *dentry,
        struct kernfs_node *kn = inode->i_private;
        struct kernfs_root *root;
        int error;
+       struct rw_semaphore *rwsem;

        if (!kn)
                return -EINVAL;

        root = kernfs_root(kn);
-       down_write(&root->kernfs_rwsem);
+       rwsem = kernfs_iattr_down_write(kn);
        error = setattr_prepare(&init_user_ns, dentry, iattr);
        if (error)
                goto out;
@@ -132,7 +134,7 @@ int kernfs_iop_setattr(struct user_namespace *mnt_userns,
struct dentry *dentry,
        setattr_copy(&init_user_ns, inode, iattr);

 out:
-       up_write(&root->kernfs_rwsem);
+       kernfs_iattr_up_write(rwsem, kn);
        return error;
 }

@@ -188,11 +190,12 @@ int kernfs_iop_getattr(struct user_namespace *mnt_userns,
        struct inode *inode = d_inode(path->dentry);
        struct kernfs_node *kn = inode->i_private;
        struct kernfs_root *root = kernfs_root(kn);
+       struct rw_semaphore *rwsem;

-       down_read(&root->kernfs_rwsem);
+       rwsem = kernfs_iattr_down_read(kn);
        kernfs_refresh_inode(kn, inode);
        generic_fillattr(&init_user_ns, inode, stat);
-       up_read(&root->kernfs_rwsem);
+       kernfs_iattr_up_read(rwsem, kn);

        return 0;
 }
@@ -278,6 +281,7 @@ int kernfs_iop_permission(struct user_namespace *mnt_userns,
        struct kernfs_node *kn;
        struct kernfs_root *root;
        int ret;
+       struct rw_semaphore *rwsem;

        if (mask & MAY_NOT_BLOCK)
                return -ECHILD;
@@ -285,10 +289,10 @@ int kernfs_iop_permission(struct user_namespace *mnt_userns,
        kn = inode->i_private;
        root = kernfs_root(kn);

-       down_read(&root->kernfs_rwsem);
+       rwsem = kernfs_iattr_down_read(kn);
        kernfs_refresh_inode(kn, inode);
        ret = generic_permission(&init_user_ns, inode, mask);
-       up_read(&root->kernfs_rwsem);
+       kernfs_iattr_up_read(rwsem, kn);

        return ret;
 }
diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
index fc5821effd97..bd1ecd126395 100644
--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h
@@ -169,4 +169,53 @@ extern const struct inode_operations kernfs_symlink_iops;
  * kernfs locks
  */
 extern struct kernfs_global_locks *kernfs_locks;
+
+static inline struct rw_semaphore *kernfs_iattr_rwsem_ptr(struct kernfs_node *kn)
+{
+       int idx = hash_ptr(kn, NR_KERNFS_LOCK_BITS);
+
+       return &kernfs_locks->iattr_rwsem[idx];
+}
+
+static inline struct rw_semaphore *kernfs_iattr_down_write(struct kernfs_node *kn)
+{
+       struct rw_semaphore *rwsem;
+
+       kernfs_get(kn);
+
+       rwsem = kernfs_iattr_rwsem_ptr(kn);
+
+       down_write(rwsem);
+
+       return rwsem;
+}
+
+static inline void kernfs_iattr_up_write(struct rw_semaphore *rwsem,
+                                        struct kernfs_node *kn)
+{
+       up_write(rwsem);
+
+       kernfs_put(kn);
+}
+
+
+static inline struct rw_semaphore *kernfs_iattr_down_read(struct kernfs_node *kn)
+{
+       struct rw_semaphore *rwsem;
+
+       kernfs_get(kn);
+
+       rwsem = kernfs_iattr_rwsem_ptr(kn);
+
+       down_read(rwsem);
+
+       return rwsem;
+}
+
+static inline void kernfs_iattr_up_read(struct rw_semaphore *rwsem,
+                                       struct kernfs_node *kn)
+{
+       up_read(rwsem);
+
+       kernfs_put(kn);
+}
 #endif /* __KERNFS_INTERNAL_H */
diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
index d0859f72d2d6..f282e5d65d04 100644
--- a/fs/kernfs/mount.c
+++ b/fs/kernfs/mount.c
@@ -392,8 +392,10 @@ static void __init kernfs_mutex_init(void)
 {
        int count;

-       for (count = 0; count < NR_KERNFS_LOCKS; count++)
+       for (count = 0; count < NR_KERNFS_LOCKS; count++) {
                mutex_init(&kernfs_locks->open_file_mutex[count]);
+               init_rwsem(&kernfs_locks->iattr_rwsem[count]);
+       }
 }

 static void __init kernfs_lock_init(void)
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index 73f5c120def8..fcbf5e7c921c 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -89,6 +89,7 @@ struct kernfs_iattrs;
  */
 struct kernfs_global_locks {
        struct mutex open_file_mutex[NR_KERNFS_LOCKS];
+       struct rw_semaphore iattr_rwsem[NR_KERNFS_LOCKS];
 };

 enum kernfs_node_type {


gives further improvement in CPU usage:

-    8.26%     8.22%  showgids         [kernel.kallsyms]       [k] down_read
             8.19% __libc_start_main
                __open_nocancel
                entry_SYSCALL_64_after_hwframe
                do_syscall_64
                sys_open
                do_sys_open
                do_filp_open
              - path_openat
                 - 7.59% link_path_walk
                    - 6.66% walk_component
                         lookup_fast
                       - d_revalidate.part.24
                          - 6.66% kernfs_dop_revalidate
                               down_read
                    - 0.71% kernfs_iop_get_link
                         down_read
                 - 0.58% lookup_fast
                      d_revalidate.part.24
                      kernfs_dop_revalidate
                      down_read
        -    7.44%     7.41%  showgids         [kernel.kallsyms]       [k] up_read
             7.39% __libc_start_main
                __open_nocancel
                entry_SYSCALL_64_after_hwframe
                do_syscall_64
                sys_open
                do_sys_open
                do_filp_open
              - path_openat
                 - 7.36% link_path_walk
                    - 6.45% walk_component
                         lookup_fast
                         d_revalidate.part.24
                         kernfs_dop_revalidate
                         up_read

In above snippet CPU usage in down_read/up_read has gone down to ~16%

So do you think that rather than replacing global kernfs_rwsem with a hashed one
, any of the above mentioned 2 patches (1. Use a global rwsem for protecting
inode attributes or 2. Use a hashed rwsem for protecting inode attributes)
can be used. These patches are not breaking code paths involving multiple nodes
that currently use global kernfs_rwsem.
With hashed kernfs_rwsem I guess there will always be a risk of some corner case
getting missed.

If any of these approaches are acceptable, I can send the patch for review along
with other changes of this series

Thanks again for reviews,
  -- Imran

[1]: https://lore.kernel.org/all/20220428055431.3826852-5-imran.f.khan@xxxxxxxxxx/
[2]: https://lore.kernel.org/all/20220620032634.1103822-1-imran.f.khan@xxxxxxxxxx/