@@ -17,6 +17,8 @@
#include "kernfs-internal.h"
+static DEFINE_MUTEX(attr_mutex);
+
static const struct address_space_operations kernfs_aops = {
.readpage = simple_readpage,
.write_begin = simple_write_begin,
@@ -32,33 +34,33 @@ static const struct inode_operations kernfs_iops = {
static struct kernfs_iattrs *__kernfs_iattrs(struct kernfs_node *kn, int alloc)
{
- static DEFINE_MUTEX(iattr_mutex);
- struct kernfs_iattrs *ret;
-
- mutex_lock(&iattr_mutex);
+ struct kernfs_iattrs *iattr = NULL;
- if (kn->iattr || !alloc)
+ mutex_lock(&attr_mutex);
+ if (kn->iattr || !alloc) {
+ iattr = kn->iattr;
goto out_unlock;
+ }
- kn->iattr = kmem_cache_zalloc(kernfs_iattrs_cache, GFP_KERNEL);
- if (!kn->iattr)
+ iattr = kmem_cache_zalloc(kernfs_iattrs_cache, GFP_KERNEL);
+ if (!iattr)
goto out_unlock;
/* assign default attributes */
- kn->iattr->ia_uid = GLOBAL_ROOT_UID;
- kn->iattr->ia_gid = GLOBAL_ROOT_GID;
+ iattr->ia_uid = GLOBAL_ROOT_UID;
+ iattr->ia_gid = GLOBAL_ROOT_GID;
- ktime_get_real_ts64(&kn->iattr->ia_atime);
- kn->iattr->ia_mtime = kn->iattr->ia_atime;
- kn->iattr->ia_ctime = kn->iattr->ia_atime;
+ ktime_get_real_ts64(&iattr->ia_atime);
+ iattr->ia_mtime = iattr->ia_atime;
+ iattr->ia_ctime = iattr->ia_atime;
- simple_xattrs_init(&kn->iattr->xattrs);
- atomic_set(&kn->iattr->nr_user_xattrs, 0);
- atomic_set(&kn->iattr->user_xattr_size, 0);
+ simple_xattrs_init(&iattr->xattrs);
+ atomic_set(&iattr->nr_user_xattrs, 0);
+ atomic_set(&iattr->user_xattr_size, 0);
+ kn->iattr = iattr;
out_unlock:
- ret = kn->iattr;
- mutex_unlock(&iattr_mutex);
- return ret;
+ mutex_unlock(&attr_mutex);
+ return iattr;
}
static struct kernfs_iattrs *kernfs_iattrs(struct kernfs_node *kn)
@@ -189,9 +191,11 @@ int kernfs_iop_getattr(const struct path *path, struct kstat *stat,
struct inode *inode = d_inode(path->dentry);
struct kernfs_node *kn = inode->i_private;
- down_write(&kernfs_rwsem);
+ down_read(&kernfs_rwsem);
+ mutex_lock(&attr_mutex);
kernfs_refresh_inode(kn, inode);
- up_writeread(&kernfs_rwsem);
+ mutex_unlock(&attr_mutex);
+ up_read(&kernfs_rwsem);
generic_fillattr(inode, stat);
return 0;
@@ -281,9 +285,11 @@ int kernfs_iop_permission(struct inode *inode, int mask)
kn = inode->i_private;
- down_write(&kernfs_rwsem);
+ down_read(&kernfs_rwsem);
+ mutex_lock(&attr_mutex);
kernfs_refresh_inode(kn, inode);
- up_write(&kernfs_rwsem);
+ mutex_unlock(&attr_mutex);
+ up_read(&kernfs_rwsem);
return generic_permission(inode, mask);
}
The inode operations .permission() and .getattr() use the kernfs node write lock but all that's needed is to keep the rb tree stable while copying the node attributes. And .permission() is called frequently during path walks so it can cause quite a bit of contention between kernfs node opertations and path walks when the number of concurrant walks is high. Ideally the inode mutex would protect the inode update but .permission() may be called both with and without holding the inode mutex so there's no way for kernfs .permission() to know if it is the holder of the mutex which means it could be released during the update. So refactor __kernfs_iattrs() by moving the static mutex declaration out of the function and changing the function itself a little. And also use the mutex to protect the inode attribute fields updated by .permission() and .getattr() calls to kernfs_refresh_inode(). Using the attr mutex to protect two different things, the node attributes as well as the copy of them to the inode is not ideal. But the only other choice is to use two locks which seems like excessive ovherhead when the attr mutex is so closely related to the inode fields it's protecting. Signed-off-by: Ian Kent <raven@themaw.net> --- fs/kernfs/inode.c | 50 ++++++++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 22 deletions(-)