[4/4] xfs: replace mr*() functions with native rwsem calls

Message ID	20200128145528.2093039-5-preichl@redhat.com (mailing list archive)
State	Superseded
Headers	show Return-Path: <SRS0=UJS8=3R=vger.kernel.org=linux-xfs-owner@kernel.org> From: Pavel Reichl <preichl@redhat.com> To: linux-xfs@vger.kernel.org Cc: Pavel Reichl <preichl@redhat.com> Subject: [PATCH 4/4] xfs: replace mr*() functions with native rwsem calls Date: Tue, 28 Jan 2020 15:55:28 +0100 Message-Id: <20200128145528.2093039-5-preichl@redhat.com> In-Reply-To: <20200128145528.2093039-1-preichl@redhat.com> References: <20200128145528.2093039-1-preichl@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-xfs-owner@vger.kernel.org Precedence: bulk
Series	xfs: Remove wrappers for some semaphores \| expand [0/4] xfs: Remove wrappers for some semaphores [1/4] xfs: change xfs_isilocked() to always use lockdep() [2/4] xfs: Remove mr_writer field from mrlock_t [3/4] xfs: Make i_lock and i_mmap native rwsems [4/4] xfs: replace mr*() functions with native rwsem calls

Pavel Reichl Jan. 28, 2020, 2:55 p.m. UTC

Remove mr*() functions as they only wrap the standard kernel functions
for kernel manimulation.

Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 fs/xfs/mrlock.h    | 61 ----------------------------------------------
 fs/xfs/xfs_inode.c | 33 +++++++++++++------------
 fs/xfs/xfs_linux.h |  1 -
 fs/xfs/xfs_super.c |  6 ++---
 4 files changed, 19 insertions(+), 82 deletions(-)
 delete mode 100644 fs/xfs/mrlock.h

Darrick J. Wong Jan. 28, 2020, 4:44 p.m. UTC | #1

On Tue, Jan 28, 2020 at 03:55:28PM +0100, Pavel Reichl wrote:
> Remove mr*() functions as they only wrap the standard kernel functions
> for kernel manimulation.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  fs/xfs/mrlock.h    | 61 ----------------------------------------------
>  fs/xfs/xfs_inode.c | 33 +++++++++++++------------
>  fs/xfs/xfs_linux.h |  1 -
>  fs/xfs/xfs_super.c |  6 ++---
>  4 files changed, 19 insertions(+), 82 deletions(-)
>  delete mode 100644 fs/xfs/mrlock.h
> 
> diff --git a/fs/xfs/mrlock.h b/fs/xfs/mrlock.h
> deleted file mode 100644
> index 245f417a7ffe..000000000000
> --- a/fs/xfs/mrlock.h
> +++ /dev/null
> @@ -1,61 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * Copyright (c) 2000-2006 Silicon Graphics, Inc.
> - * All Rights Reserved.
> - */
> -#ifndef __XFS_SUPPORT_MRLOCK_H__
> -#define __XFS_SUPPORT_MRLOCK_H__
> -
> -#include <linux/rwsem.h>
> -
> -typedef struct {
> -	struct rw_semaphore	mr_lock;
> -} mrlock_t;
> -
> -#if defined(DEBUG) || defined(XFS_WARN)
> -#define mrinit(smp, name)	init_rwsem(smp)
> -#else
> -#define mrinit(smp, name)	init_rwsem(smp)
> -#endif
> -
> -#define mrlock_init(smp, t, n, s)	mrinit(smp, n)
> -#define mrfree(smp)		do { } while (0)
> -
> -static inline void mraccess_nested(struct rw_semaphore *s, int subclass)
> -{
> -	down_read_nested(s, subclass);
> -}
> -
> -static inline void mrupdate_nested(struct rw_semaphore *s, int subclass)
> -{
> -	down_write_nested(s, subclass);
> -}
> -
> -static inline int mrtryaccess(struct rw_semaphore *s)
> -{
> -	return down_read_trylock(s);
> -}
> -
> -static inline int mrtryupdate(struct rw_semaphore *s)
> -{
> -	if (!down_write_trylock(s))
> -		return 0;
> -	return 1;
> -}
> -
> -static inline void mrunlock_excl(struct rw_semaphore *s)
> -{
> -	up_write(s);
> -}
> -
> -static inline void mrunlock_shared(struct rw_semaphore *s)
> -{
> -	up_read(s);
> -}
> -
> -static inline void mrdemote(struct rw_semaphore *s)
> -{
> -	downgrade_write(s);
> -}
> -
> -#endif /* __XFS_SUPPORT_MRLOCK_H__ */
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 567dae69cfac..01bca957e305 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -191,14 +191,15 @@ xfs_ilock(
>  	}
>  
>  	if (lock_flags & XFS_MMAPLOCK_EXCL)
> -		mrupdate_nested(&ip->i_mmaplock, XFS_MMAPLOCK_DEP(lock_flags));
> +		down_write_nested(&ip->i_mmaplock,
> +				XFS_MMAPLOCK_DEP(lock_flags));

FWIW I would have started by converting mrlock_t to rw_semaphore in all
callers and then dropped mrlock_t, but this somewhat more circuitous
route works too.

--D

>  	else if (lock_flags & XFS_MMAPLOCK_SHARED)
> -		mraccess_nested(&ip->i_mmaplock, XFS_MMAPLOCK_DEP(lock_flags));
> +		down_read_nested(&ip->i_mmaplock, XFS_MMAPLOCK_DEP(lock_flags));
>  
>  	if (lock_flags & XFS_ILOCK_EXCL)
> -		mrupdate_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
> +		down_write_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
>  	else if (lock_flags & XFS_ILOCK_SHARED)
> -		mraccess_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
> +		down_read_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
>  }
>  
>  /*
> @@ -242,27 +243,27 @@ xfs_ilock_nowait(
>  	}
>  
>  	if (lock_flags & XFS_MMAPLOCK_EXCL) {
> -		if (!mrtryupdate(&ip->i_mmaplock))
> +		if (!down_write_trylock(&ip->i_mmaplock))
>  			goto out_undo_iolock;
>  	} else if (lock_flags & XFS_MMAPLOCK_SHARED) {
> -		if (!mrtryaccess(&ip->i_mmaplock))
> +		if (!down_read_trylock(&ip->i_mmaplock))
>  			goto out_undo_iolock;
>  	}
>  
>  	if (lock_flags & XFS_ILOCK_EXCL) {
> -		if (!mrtryupdate(&ip->i_lock))
> +		if (!down_write_trylock(&ip->i_lock))
>  			goto out_undo_mmaplock;
>  	} else if (lock_flags & XFS_ILOCK_SHARED) {
> -		if (!mrtryaccess(&ip->i_lock))
> +		if (!down_read_trylock(&ip->i_lock))
>  			goto out_undo_mmaplock;
>  	}
>  	return 1;
>  
>  out_undo_mmaplock:
>  	if (lock_flags & XFS_MMAPLOCK_EXCL)
> -		mrunlock_excl(&ip->i_mmaplock);
> +		up_write(&ip->i_mmaplock);
>  	else if (lock_flags & XFS_MMAPLOCK_SHARED)
> -		mrunlock_shared(&ip->i_mmaplock);
> +		up_read(&ip->i_mmaplock);
>  out_undo_iolock:
>  	if (lock_flags & XFS_IOLOCK_EXCL)
>  		up_write(&VFS_I(ip)->i_rwsem);
> @@ -309,14 +310,14 @@ xfs_iunlock(
>  		up_read(&VFS_I(ip)->i_rwsem);
>  
>  	if (lock_flags & XFS_MMAPLOCK_EXCL)
> -		mrunlock_excl(&ip->i_mmaplock);
> +		up_write(&ip->i_mmaplock);
>  	else if (lock_flags & XFS_MMAPLOCK_SHARED)
> -		mrunlock_shared(&ip->i_mmaplock);
> +		up_read(&ip->i_mmaplock);
>  
>  	if (lock_flags & XFS_ILOCK_EXCL)
> -		mrunlock_excl(&ip->i_lock);
> +		up_write(&ip->i_lock);
>  	else if (lock_flags & XFS_ILOCK_SHARED)
> -		mrunlock_shared(&ip->i_lock);
> +		up_read(&ip->i_lock);
>  
>  	trace_xfs_iunlock(ip, lock_flags, _RET_IP_);
>  }
> @@ -335,9 +336,9 @@ xfs_ilock_demote(
>  		~(XFS_IOLOCK_EXCL|XFS_MMAPLOCK_EXCL|XFS_ILOCK_EXCL)) == 0);
>  
>  	if (lock_flags & XFS_ILOCK_EXCL)
> -		mrdemote(&ip->i_lock);
> +		downgrade_write(&ip->i_lock);
>  	if (lock_flags & XFS_MMAPLOCK_EXCL)
> -		mrdemote(&ip->i_mmaplock);
> +		downgrade_write(&ip->i_mmaplock);
>  	if (lock_flags & XFS_IOLOCK_EXCL)
>  		downgrade_write(&VFS_I(ip)->i_rwsem);
>  
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index 8738bb03f253..921a3eb093ed 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -22,7 +22,6 @@ typedef __u32			xfs_nlink_t;
>  #include "xfs_types.h"
>  
>  #include "kmem.h"
> -#include "mrlock.h"
>  
>  #include <linux/semaphore.h>
>  #include <linux/mm.h>
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 760901783944..1289ce1f4e9e 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -661,10 +661,8 @@ xfs_fs_inode_init_once(
>  	atomic_set(&ip->i_pincount, 0);
>  	spin_lock_init(&ip->i_flags_lock);
>  
> -	mrlock_init(&ip->i_mmaplock, MRLOCK_ALLOW_EQUAL_PRI|MRLOCK_BARRIER,
> -		     "xfsino", ip->i_ino);
> -	mrlock_init(&ip->i_lock, MRLOCK_ALLOW_EQUAL_PRI|MRLOCK_BARRIER,
> -		     "xfsino", ip->i_ino);
> +	init_rwsem(&ip->i_mmaplock);
> +	init_rwsem(&ip->i_lock);
>  }
>  
>  /*
> -- 
> 2.24.1
>

Christoph Hellwig Jan. 30, 2020, 7:45 a.m. UTC | #2

On Tue, Jan 28, 2020 at 03:55:28PM +0100, Pavel Reichl wrote:
> Remove mr*() functions as they only wrap the standard kernel functions
> for kernel manimulation.

I think patches 2, 3 and 4 make more sense if merged into a single
patch to replace the mrlocks with rw_sempahores.

Pavel Reichl Jan. 30, 2020, 8:57 a.m. UTC | #3

On Thu, Jan 30, 2020 at 8:45 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Jan 28, 2020 at 03:55:28PM +0100, Pavel Reichl wrote:
> > Remove mr*() functions as they only wrap the standard kernel functions
> > for kernel manimulation.
>
> I think patches 2, 3 and 4 make more sense if merged into a single
> patch to replace the mrlocks with rw_sempahores.
>

Hello Christoph,

the changes are divided into three patches so the changes are really
obvious and each patch does just one thing...it was actually an extra
effort to separate the changes but if there's an agreement that it
does not add any value then I can squash them into one - no problem
;-)

Bye

Christoph Hellwig Jan. 30, 2020, 1:31 p.m. UTC | #4

On Thu, Jan 30, 2020 at 09:57:02AM +0100, Pavel Reichl wrote:
> the changes are divided into three patches so the changes are really
> obvious and each patch does just one thing...it was actually an extra
> effort to separate the changes but if there's an agreement that it
> does not add any value then I can squash them into one - no problem
> ;-)

Well, they make the change a lot less obvious.  After we have some form
of patch 1, mrlocks are just a pointless wrapper.  Removing it in one
go is completely obvious - splitting it in 3 patches with weird
inbetween states is everything but.

Pavel Reichl Jan. 30, 2020, 1:43 p.m. UTC | #5

OK, thank you for your opinion. :-)

On Thu, Jan 30, 2020 at 2:31 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Thu, Jan 30, 2020 at 09:57:02AM +0100, Pavel Reichl wrote:
> > the changes are divided into three patches so the changes are really
> > obvious and each patch does just one thing...it was actually an extra
> > effort to separate the changes but if there's an agreement that it
> > does not add any value then I can squash them into one - no problem
> > ;-)
>
> Well, they make the change a lot less obvious.  After we have some form
> of patch 1, mrlocks are just a pointless wrapper.  Removing it in one
> go is completely obvious - splitting it in 3 patches with weird
> inbetween states is everything but.
>

[4/4] xfs: replace mr*() functions with native rwsem calls

Commit Message

Comments

Patch