Re: [PATCH v8 05/10] kernfs: Replace global kernfs_open_file_mutex with hashed mutexes.

[Tejun Heo <tj@kernel.org>]

On Sun, Apr 10, 2022 at 12:37:14PM +1000, Imran Khan wrote:
> diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> index 214b48d59148..0946ab341ce4 100644
> --- a/fs/kernfs/file.c
> +++ b/fs/kernfs/file.c
> @@ -19,18 +19,14 @@
>  #include "kernfs-internal.h"
>  
>  /*
> - * There's one kernfs_open_file for each open file and one kernfs_open_node
> - * for each kernfs_node with one or more open files.
> - *
> + * kernfs locks
> + */
> +extern struct kernfs_global_locks *kernfs_locks;

This should be in a header file, right?

> @@ -545,7 +543,7 @@ static int kernfs_get_open_node(struct kernfs_node *kn,
>  	 * need rcu_read_lock to ensure its existence.
>  	 */
>  	on = rcu_dereference_protected(kn->attr.open,
> -				   lockdep_is_held(&kernfs_open_file_mutex));
> +				   lockdep_is_held(mutex));
>  	if (on) {
>  		list_add_tail(&of->list, &on->files);
>  		mutex_unlock(mutex);
> @@ -593,10 +591,10 @@ static void kernfs_put_open_node(struct kernfs_node *kn,
>  	mutex = kernfs_open_file_mutex_lock(kn);
>  
>  	on = rcu_dereference_protected(kn->attr.open,
> -				       lockdep_is_held(&kernfs_open_file_mutex));
> +				       lockdep_is_held(mutex));
>  
>  	if (!on) {
> -		mutex_unlock(&kernfs_open_file_mutex);
> +		mutex_unlock(mutex);
>  		return;
>  	}

Don't above chunks belong to the previous patch?

> @@ -769,6 +767,10 @@ static int kernfs_fop_release(struct inode *inode, struct file *filp)
>  	struct mutex *mutex = NULL;
>  
>  	if (kn->flags & KERNFS_HAS_RELEASE) {
> +		/**
> +		 * Callers of kernfs_fop_release, don't need global
> +		 * exclusion so using hashed mutex here is safe.
> +		 */

I don't think we use /** for in-line comments. Just do balanced wings.

 /*
  * ....
  */

besides, I'm not sure the comment is helping that much. It'd be better to
have a comment describing the locking rules comprehensively around the lock
definition and/or helpers.

>  		mutex = kernfs_open_file_mutex_lock(kn);
>  		kernfs_release_file(kn, of);
>  		mutex_unlock(mutex);
> @@ -796,9 +798,9 @@ void kernfs_drain_open_files(struct kernfs_node *kn)
>  
>  	mutex = kernfs_open_file_mutex_lock(kn);
>  	on = rcu_dereference_check(kn->attr.open,
> -				   lockdep_is_held(&kernfs_open_file_mutex));
> +				   lockdep_is_held(mutex));
>  	if (!on) {
> -		mutex_unlock(&kernfs_open_file_mutex);
> +		mutex_unlock(mutex);

Again, should be in the previous patch.

> +void __init kernfs_lock_init(void)
> +{
> +	int count;
> +
> +	kernfs_locks = kmalloc(sizeof(struct kernfs_global_locks), GFP_KERNEL);
> +	WARN_ON(!kernfs_locks);
> +
> +	for (count = 0; count < NR_KERNFS_LOCKS; count++)
> +		mutex_init(&kernfs_locks->open_file_mutex[count].lock);
> +}

Does this need to be a separate function?

>  void __init kernfs_init(void)
>  {
>  	kernfs_node_cache = kmem_cache_create("kernfs_node_cache",
> @@ -397,4 +409,6 @@ void __init kernfs_init(void)
>  	kernfs_iattrs_cache  = kmem_cache_create("kernfs_iattrs_cache",
>  					      sizeof(struct kernfs_iattrs),
>  					      0, SLAB_PANIC, NULL);
> +
> +	kernfs_lock_init();
>  }
> diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
> index 2dd9c8df0f4f..cc514bda0ae7 100644
> --- a/include/linux/kernfs.h
> +++ b/include/linux/kernfs.h
...
> + * filp->private_data points to seq_file whose ->private points to
> + * kernfs_open_file.
> + * kernfs_open_files are chained at kernfs_open_node->files, which is
> + * protected by kernfs_open_file_mutex.lock.

Can you either flow the sentences together or have a blank line inbetween if
they're supposed to be in separate paragraphs?

> + */
> +struct kernfs_open_file_mutex {
> +	struct mutex lock;
> +} ____cacheline_aligned_in_smp;

Is there a reason to define the outer struct?

> +/*
> + * To reduce possible contention in sysfs access, arising due to single
> + * locks, use an array of locks and use kernfs_node object address as
> + * hash keys to get the index of these locks.
> + */
> +struct kernfs_global_locks {
> +	struct kernfs_open_file_mutex open_file_mutex[NR_KERNFS_LOCKS];
> +};

Ditto, why not just define the array directly?

> +void kernfs_lock_init(void);

Does this function need to be exposed?

Thanks.

-- 
tejun