Re: [patch 04/10] fs: fs_struct rwlock to spinlock

[Valerie Aurora <vaurora@redhat.com>]

On Wed, Aug 18, 2010 at 04:37:33AM +1000, Nick Piggin wrote:
> fs: fs_struct rwlock to spinlock
> 
> struct fs_struct.lock is an rwlock with the read-side used to protect root and
> pwd members while taking references to them. Taking a reference to a path
> typically requires just 2 atomic ops, so the critical section is very small.
> Parallel read-side operations would have cacheline contention on the lock, the
> dentry, and the vfsmount cachelines, so the rwlock is unlikely to ever give a
> real parallelism increase.
> 
> Replace it with a spinlock to avoid one or two atomic operations in typical
> path lookup fastpath.
> 
> Signed-off-by: Nick Piggin <npiggin@kernel.dk>
> 
> ---
>  drivers/staging/pohmelfs/path_entry.c |    8 ++++----
>  fs/exec.c                             |    4 ++--
>  fs/fs_struct.c                        |   32 ++++++++++++++++----------------
>  include/linux/fs_struct.h             |   14 +++++++-------
>  kernel/fork.c                         |   10 +++++-----
>  5 files changed, 34 insertions(+), 34 deletions(-)
> 
> Index: linux-2.6/fs/fs_struct.c
> ===================================================================
> --- linux-2.6.orig/fs/fs_struct.c	2010-08-18 04:04:01.000000000 +1000
> +++ linux-2.6/fs/fs_struct.c	2010-08-18 04:04:29.000000000 +1000
> @@ -13,11 +13,11 @@ void set_fs_root(struct fs_struct *fs, s
>  {
>  	struct path old_root;
>  
> -	write_lock(&fs->lock);
> +	spin_lock(&fs->lock);
>  	old_root = fs->root;
>  	fs->root = *path;
>  	path_get(path);
> -	write_unlock(&fs->lock);
> +	spin_unlock(&fs->lock);
>  	if (old_root.dentry)
>  		path_put(&old_root);
>  }
> @@ -30,11 +30,11 @@ void set_fs_pwd(struct fs_struct *fs, st
>  {
>  	struct path old_pwd;
>  
> -	write_lock(&fs->lock);
> +	spin_lock(&fs->lock);
>  	old_pwd = fs->pwd;
>  	fs->pwd = *path;
>  	path_get(path);
> -	write_unlock(&fs->lock);
> +	spin_unlock(&fs->lock);
>  
>  	if (old_pwd.dentry)
>  		path_put(&old_pwd);
> @@ -51,7 +51,7 @@ void chroot_fs_refs(struct path *old_roo
>  		task_lock(p);
>  		fs = p->fs;
>  		if (fs) {
> -			write_lock(&fs->lock);
> +			spin_lock(&fs->lock);
>  			if (fs->root.dentry == old_root->dentry
>  			    && fs->root.mnt == old_root->mnt) {
>  				path_get(new_root);
> @@ -64,7 +64,7 @@ void chroot_fs_refs(struct path *old_roo
>  				fs->pwd = *new_root;
>  				count++;
>  			}
> -			write_unlock(&fs->lock);
> +			spin_unlock(&fs->lock);
>  		}
>  		task_unlock(p);
>  	} while_each_thread(g, p);
> @@ -87,10 +87,10 @@ void exit_fs(struct task_struct *tsk)
>  	if (fs) {
>  		int kill;
>  		task_lock(tsk);
> -		write_lock(&fs->lock);
> +		spin_lock(&fs->lock);
>  		tsk->fs = NULL;
>  		kill = !--fs->users;
> -		write_unlock(&fs->lock);
> +		spin_unlock(&fs->lock);
>  		task_unlock(tsk);
>  		if (kill)
>  			free_fs_struct(fs);
> @@ -104,7 +104,7 @@ struct fs_struct *copy_fs_struct(struct
>  	if (fs) {
>  		fs->users = 1;
>  		fs->in_exec = 0;
> -		rwlock_init(&fs->lock);
> +		spin_lock_init(&fs->lock);
>  		fs->umask = old->umask;
>  		get_fs_root_and_pwd(old, &fs->root, &fs->pwd);
>  	}
> @@ -121,10 +121,10 @@ int unshare_fs_struct(void)
>  		return -ENOMEM;
>  
>  	task_lock(current);
> -	write_lock(&fs->lock);
> +	spin_lock(&fs->lock);
>  	kill = !--fs->users;
>  	current->fs = new_fs;
> -	write_unlock(&fs->lock);
> +	spin_unlock(&fs->lock);
>  	task_unlock(current);
>  
>  	if (kill)
> @@ -143,7 +143,7 @@ EXPORT_SYMBOL(current_umask);
>  /* to be mentioned only in INIT_TASK */
>  struct fs_struct init_fs = {
>  	.users		= 1,
> -	.lock		= __RW_LOCK_UNLOCKED(init_fs.lock),
> +	.lock		= __SPIN_LOCK_UNLOCKED(init_fs.lock),
>  	.umask		= 0022,
>  };
>  
> @@ -156,14 +156,14 @@ void daemonize_fs_struct(void)
>  
>  		task_lock(current);
>  
> -		write_lock(&init_fs.lock);
> +		spin_lock(&init_fs.lock);
>  		init_fs.users++;
> -		write_unlock(&init_fs.lock);
> +		spin_unlock(&init_fs.lock);
>  
> -		write_lock(&fs->lock);
> +		spin_lock(&fs->lock);
>  		current->fs = &init_fs;
>  		kill = !--fs->users;
> -		write_unlock(&fs->lock);
> +		spin_unlock(&fs->lock);
>  
>  		task_unlock(current);
>  		if (kill)
> Index: linux-2.6/include/linux/fs_struct.h
> ===================================================================
> --- linux-2.6.orig/include/linux/fs_struct.h	2010-08-18 04:04:01.000000000 +1000
> +++ linux-2.6/include/linux/fs_struct.h	2010-08-18 04:04:29.000000000 +1000
> @@ -5,7 +5,7 @@
>  
>  struct fs_struct {
>  	int users;
> -	rwlock_t lock;
> +	spinlock_t lock;
>  	int umask;
>  	int in_exec;
>  	struct path root, pwd;
> @@ -23,29 +23,29 @@ extern int unshare_fs_struct(void);
>  
>  static inline void get_fs_root(struct fs_struct *fs, struct path *root)
>  {
> -	read_lock(&fs->lock);
> +	spin_lock(&fs->lock);
>  	*root = fs->root;
>  	path_get(root);
> -	read_unlock(&fs->lock);
> +	spin_unlock(&fs->lock);
>  }
>  
>  static inline void get_fs_pwd(struct fs_struct *fs, struct path *pwd)
>  {
> -	read_lock(&fs->lock);
> +	spin_lock(&fs->lock);
>  	*pwd = fs->pwd;
>  	path_get(pwd);
> -	read_unlock(&fs->lock);
> +	spin_unlock(&fs->lock);
>  }
>  
>  static inline void get_fs_root_and_pwd(struct fs_struct *fs, struct path *root,
>  				       struct path *pwd)
>  {
> -	read_lock(&fs->lock);
> +	spin_lock(&fs->lock);
>  	*root = fs->root;
>  	path_get(root);
>  	*pwd = fs->pwd;
>  	path_get(pwd);
> -	read_unlock(&fs->lock);
> +	spin_unlock(&fs->lock);
>  }
>  
>  #endif /* _LINUX_FS_STRUCT_H */
> Index: linux-2.6/fs/exec.c
> ===================================================================
> --- linux-2.6.orig/fs/exec.c	2010-08-18 04:04:01.000000000 +1000
> +++ linux-2.6/fs/exec.c	2010-08-18 04:04:29.000000000 +1000
> @@ -1117,7 +1117,7 @@ int check_unsafe_exec(struct linux_binpr
>  	bprm->unsafe = tracehook_unsafe_exec(p);
>  
>  	n_fs = 1;
> -	write_lock(&p->fs->lock);
> +	spin_lock(&p->fs->lock);
>  	rcu_read_lock();
>  	for (t = next_thread(p); t != p; t = next_thread(t)) {
>  		if (t->fs == p->fs)
> @@ -1134,7 +1134,7 @@ int check_unsafe_exec(struct linux_binpr
>  			res = 1;
>  		}
>  	}
> -	write_unlock(&p->fs->lock);
> +	spin_unlock(&p->fs->lock);
>  
>  	return res;
>  }
> Index: linux-2.6/kernel/fork.c
> ===================================================================
> --- linux-2.6.orig/kernel/fork.c	2010-08-18 04:04:01.000000000 +1000
> +++ linux-2.6/kernel/fork.c	2010-08-18 04:04:29.000000000 +1000
> @@ -752,13 +752,13 @@ static int copy_fs(unsigned long clone_f
>  	struct fs_struct *fs = current->fs;
>  	if (clone_flags & CLONE_FS) {
>  		/* tsk->fs is already what we want */
> -		write_lock(&fs->lock);
> +		spin_lock(&fs->lock);
>  		if (fs->in_exec) {
> -			write_unlock(&fs->lock);
> +			spin_unlock(&fs->lock);
>  			return -EAGAIN;
>  		}
>  		fs->users++;
> -		write_unlock(&fs->lock);
> +		spin_unlock(&fs->lock);
>  		return 0;
>  	}
>  	tsk->fs = copy_fs_struct(fs);
> @@ -1676,13 +1676,13 @@ SYSCALL_DEFINE1(unshare, unsigned long,
>  
>  		if (new_fs) {
>  			fs = current->fs;
> -			write_lock(&fs->lock);
> +			spin_lock(&fs->lock);
>  			current->fs = new_fs;
>  			if (--fs->users)
>  				new_fs = NULL;
>  			else
>  				new_fs = fs;
> -			write_unlock(&fs->lock);
> +			spin_unlock(&fs->lock);
>  		}
>  
>  		if (new_mm) {
> Index: linux-2.6/drivers/staging/pohmelfs/path_entry.c
> ===================================================================
> --- linux-2.6.orig/drivers/staging/pohmelfs/path_entry.c	2010-08-18 04:04:01.000000000 +1000
> +++ linux-2.6/drivers/staging/pohmelfs/path_entry.c	2010-08-18 04:04:29.000000000 +1000
> @@ -44,9 +44,9 @@ int pohmelfs_construct_path_string(struc
>  		return -ENOENT;
>  	}
>  
> -	read_lock(&current->fs->lock);
> +	spin_lock(&current->fs->lock);
>  	path.mnt = mntget(current->fs->root.mnt);
> -	read_unlock(&current->fs->lock);
> +	spin_unlock(&current->fs->lock);
>  
>  	path.dentry = d;
>  
> @@ -91,9 +91,9 @@ int pohmelfs_path_length(struct pohmelfs
>  		return -ENOENT;
>  	}
>  
> -	read_lock(&current->fs->lock);
> +	spin_lock(&current->fs->lock);
>  	root = dget(current->fs->root.dentry);
> -	read_unlock(&current->fs->lock);
> +	spin_unlock(&current->fs->lock);
>  
>  	spin_lock(&dcache_lock);

Your reasoning makes sense to me.  Shared reader access seems very
unlikely whereas the cost of taking the lock is certain.

Reviewed-by: Valerie Aurora <vaurora@redhat.com>

-VAL