Date:   Thu, 2 Mar 2023 16:08:19 +0000
From:   Matthew Wilcox <willy@infradead.org>
To:     Imran Khan <imran.f.khan@oracle.com>
Cc:     tj@kernel.org, gregkh@linuxfoundation.org, viro@zeniv.linux.org.uk,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        joe.jin@oracle.com
Subject: Re: [PATCH 2/3] kernfs: Use a per-fs rwsem to protect per-fs list of
 kernfs_super_info.
Message-ID: <ZADJ85K6KTb6XiR4@casper.infradead.org>
References: <20230302043203.1695051-1-imran.f.khan@oracle.com>
 <20230302043203.1695051-3-imran.f.khan@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230302043203.1695051-3-imran.f.khan@oracle.com>
Precedence: bulk

On Thu, Mar 02, 2023 at 03:32:02PM +1100, Imran Khan wrote:
> Right now per-fs kernfs_rwsem protects list of kernfs_super_info instances
> for a kernfs_root. Since kernfs_rwsem is used to synchronize several other
> operations across kernfs and since most of these operations don't impact
> kernfs_super_info, we can use a separate per-fs rwsem to synchronize access
> to list of kernfs_super_info.
> This helps in reducing contention around kernfs_rwsem and also allows
> operations that change/access list of kernfs_super_info to proceed without
> contending for kernfs_rwsem.
> 
> Signed-off-by: Imran Khan <imran.f.khan@oracle.com>

But you don't remove the acquisition of kernfs_rwsem in
kernfs_notify_workfn(), so I don't see how this helps?

Also, every use of this rwsem is as a writer, so it could/should be a
plain mutex, no?  Or should you be acquiring it for read in
kernfs_notify_workfn()?

>  fs/kernfs/dir.c             | 1 +
>  fs/kernfs/file.c            | 2 ++
>  fs/kernfs/kernfs-internal.h | 1 +
>  fs/kernfs/mount.c           | 8 ++++----
>  4 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> index 953b2717c60e6..2cdb8516e5287 100644
> --- a/fs/kernfs/dir.c
> +++ b/fs/kernfs/dir.c
> @@ -944,6 +944,7 @@ struct kernfs_root *kernfs_create_root(struct kernfs_syscall_ops *scops,
>  	idr_init(&root->ino_idr);
>  	init_rwsem(&root->kernfs_rwsem);
>  	init_rwsem(&root->kernfs_iattr_rwsem);
> +	init_rwsem(&root->kernfs_supers_rwsem);
>  	INIT_LIST_HEAD(&root->supers);
>  
>  	/*
> diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
> index e4a50e4ff0d23..b84cf0cd4bd44 100644
> --- a/fs/kernfs/file.c
> +++ b/fs/kernfs/file.c
> @@ -924,6 +924,7 @@ static void kernfs_notify_workfn(struct work_struct *work)
>  	/* kick fsnotify */
>  	down_write(&root->kernfs_rwsem);
>  
> +	down_write(&root->kernfs_supers_rwsem);
>  	list_for_each_entry(info, &kernfs_root(kn)->supers, node) {
>  		struct kernfs_node *parent;
>  		struct inode *p_inode = NULL;
> @@ -960,6 +961,7 @@ static void kernfs_notify_workfn(struct work_struct *work)
>  		iput(inode);
>  	}
>  
> +	up_write(&root->kernfs_supers_rwsem);
>  	up_write(&root->kernfs_rwsem);
>  	kernfs_put(kn);
>  	goto repeat;
> diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
> index 3297093c920de..a9b854cdfdb5f 100644
> --- a/fs/kernfs/kernfs-internal.h
> +++ b/fs/kernfs/kernfs-internal.h
> @@ -48,6 +48,7 @@ struct kernfs_root {
>  	wait_queue_head_t	deactivate_waitq;
>  	struct rw_semaphore	kernfs_rwsem;
>  	struct rw_semaphore	kernfs_iattr_rwsem;
> +	struct rw_semaphore	kernfs_supers_rwsem;
>  };
>  
>  /* +1 to avoid triggering overflow warning when negating it */
> diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
> index e08e8d9998070..d49606accb07b 100644
> --- a/fs/kernfs/mount.c
> +++ b/fs/kernfs/mount.c
> @@ -351,9 +351,9 @@ int kernfs_get_tree(struct fs_context *fc)
>  		}
>  		sb->s_flags |= SB_ACTIVE;
>  
> -		down_write(&root->kernfs_rwsem);
> +		down_write(&root->kernfs_supers_rwsem);
>  		list_add(&info->node, &info->root->supers);
> -		up_write(&root->kernfs_rwsem);
> +		up_write(&root->kernfs_supers_rwsem);
>  	}
>  
>  	fc->root = dget(sb->s_root);
> @@ -380,9 +380,9 @@ void kernfs_kill_sb(struct super_block *sb)
>  	struct kernfs_super_info *info = kernfs_info(sb);
>  	struct kernfs_root *root = info->root;
>  
> -	down_write(&root->kernfs_rwsem);
> +	down_write(&root->kernfs_supers_rwsem);
>  	list_del(&info->node);
> -	up_write(&root->kernfs_rwsem);
> +	up_write(&root->kernfs_supers_rwsem);
>  
>  	/*
>  	 * Remove the superblock from fs_supers/s_instances
> -- 
> 2.34.1
>