Message ID | 20200128145528.2093039-5-preichl@redhat.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Series | xfs: Remove wrappers for some semaphores | expand |
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 >
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.
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
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.
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. >
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)); 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); } /*
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