[PATCH v5 1/4] xfs: Refactor xfs_isilocked()

[PATCH v5 1/4] xfs: Refactor xfs_isilocked()

[Pavel Reichl]

Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked().
__xfs_rwsem_islocked() is a helper function which encapsulates checking
state of rw_semaphores hold by inode.

Signed-off-by: Pavel Reichl <preichl@redhat.com>
Suggested-by: Dave Chinner <dchinner@redhat.com>
Suggested-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/xfs_inode.c | 54 ++++++++++++++++++++++++++++++++--------------
 fs/xfs/xfs_inode.h |  2 +-
 2 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index c5077e6326c7..3d28c4790231 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -345,32 +345,54 @@ xfs_ilock_demote(
 }
 
 #if defined(DEBUG) || defined(XFS_WARN)
-int
+static inline bool
+__xfs_rwsem_islocked(
+	struct rw_semaphore	*rwsem,
+	bool			shared,
+	bool			excl)
+{
+	bool locked = false;
+
+	if (!rwsem_is_locked(rwsem))
+		return false;
+
+	if (!debug_locks)
+		return true;
+
+	if (shared)
+		locked = lockdep_is_held_type(rwsem, 0);
+
+	if (excl)
+		locked |= lockdep_is_held_type(rwsem, 1);
+
+	return locked;
+}
+
+bool
 xfs_isilocked(
-	xfs_inode_t		*ip,
+	struct xfs_inode	*ip,
 	uint			lock_flags)
 {
-	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
-		if (!(lock_flags & XFS_ILOCK_SHARED))
-			return !!ip->i_lock.mr_writer;
-		return rwsem_is_locked(&ip->i_lock.mr_lock);
+	if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) {
+		return __xfs_rwsem_islocked(&ip->i_lock.mr_lock,
+				(lock_flags & XFS_ILOCK_SHARED),
+				(lock_flags & XFS_ILOCK_EXCL));
 	}
 
-	if (lock_flags & (XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) {
-		if (!(lock_flags & XFS_MMAPLOCK_SHARED))
-			return !!ip->i_mmaplock.mr_writer;
-		return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
+	if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) {
+		return __xfs_rwsem_islocked(&ip->i_mmaplock.mr_lock,
+				(lock_flags & XFS_MMAPLOCK_SHARED),
+				(lock_flags & XFS_MMAPLOCK_EXCL));
 	}
 
-	if (lock_flags & (XFS_IOLOCK_EXCL|XFS_IOLOCK_SHARED)) {
-		if (!(lock_flags & XFS_IOLOCK_SHARED))
-			return !debug_locks ||
-				lockdep_is_held_type(&VFS_I(ip)->i_rwsem, 0);
-		return rwsem_is_locked(&VFS_I(ip)->i_rwsem);
+	if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) {
+		return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem,
+				(lock_flags & XFS_IOLOCK_SHARED),
+				(lock_flags & XFS_IOLOCK_EXCL));
 	}
 
 	ASSERT(0);
-	return 0;
+	return false;
 }
 #endif
 
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 492e53992fa9..3d7ce355407d 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -416,7 +416,7 @@ void		xfs_ilock(xfs_inode_t *, uint);
 int		xfs_ilock_nowait(xfs_inode_t *, uint);
 void		xfs_iunlock(xfs_inode_t *, uint);
 void		xfs_ilock_demote(xfs_inode_t *, uint);
-int		xfs_isilocked(xfs_inode_t *, uint);
+bool		xfs_isilocked(xfs_inode_t *, uint);
 uint		xfs_ilock_data_map_shared(struct xfs_inode *);
 uint		xfs_ilock_attr_map_shared(struct xfs_inode *);
 
-- 
2.24.1

[PATCH v5 2/4] xfs: clean up whitespace in xfs_isilocked() calls

[Pavel Reichl]

Make whitespace follow the same pattern in all xfs_isilocked() calls.

Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 fs/xfs/libxfs/xfs_bmap.c | 2 +-
 fs/xfs/xfs_file.c        | 3 ++-
 fs/xfs/xfs_inode.c       | 6 +++---
 fs/xfs/xfs_qm.c          | 2 +-
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 9a6d7a84689a..bc2be29193aa 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3906,7 +3906,7 @@ xfs_bmapi_read(
 	ASSERT(*nmap >= 1);
 	ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK|XFS_BMAPI_ENTIRE|
 			   XFS_BMAPI_COWFORK)));
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
 
 	if (XFS_IS_CORRUPT(mp, !xfs_ifork_has_extents(ip, whichfork)) ||
 	    XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BMAPIFORMAT)) {
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index b8a4a3f29b36..6b65572bdb21 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -770,7 +770,8 @@ xfs_break_layouts(
 	bool			retry;
 	int			error;
 
-	ASSERT(xfs_isilocked(XFS_I(inode), XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL));
+	ASSERT(xfs_isilocked(XFS_I(inode),
+			XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL));
 
 	do {
 		retry = false;
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 3d28c4790231..14f46f099470 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2828,7 +2828,7 @@ static void
 xfs_iunpin(
 	struct xfs_inode	*ip)
 {
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
 
 	trace_xfs_inode_unpin_nowait(ip, _RET_IP_);
 
@@ -3692,7 +3692,7 @@ xfs_iflush(
 
 	XFS_STATS_INC(mp, xs_iflush_count);
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
 	ASSERT(xfs_isiflocked(ip));
 	ASSERT(ip->i_d.di_format != XFS_DINODE_FMT_BTREE ||
 	       ip->i_d.di_nextents > XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK));
@@ -3824,7 +3824,7 @@ xfs_iflush_int(
 	struct xfs_dinode	*dip;
 	struct xfs_mount	*mp = ip->i_mount;
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
 	ASSERT(xfs_isiflocked(ip));
 	ASSERT(ip->i_d.di_format != XFS_DINODE_FMT_BTREE ||
 	       ip->i_d.di_nextents > XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK));
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 0b0909657bad..d2896925b5ca 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1802,7 +1802,7 @@ xfs_qm_vop_chown_reserve(
 	int			error;
 
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
 	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
 
 	delblks = ip->i_delayed_blks;
-- 
2.24.1

Re: [PATCH v5 2/4] xfs: clean up whitespace in xfs_isilocked() calls

[Dave Chinner]

On Fri, Feb 14, 2020 at 07:59:40PM +0100, Pavel Reichl wrote:
> Make whitespace follow the same pattern in all xfs_isilocked() calls.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 2 +-
>  fs/xfs/xfs_file.c        | 3 ++-
>  fs/xfs/xfs_inode.c       | 6 +++---
>  fs/xfs/xfs_qm.c          | 2 +-
>  4 files changed, 7 insertions(+), 6 deletions(-)

Simple enough.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v5 2/4] xfs: clean up whitespace in xfs_isilocked() calls

[Christoph Hellwig]

On Fri, Feb 14, 2020 at 07:59:40PM +0100, Pavel Reichl wrote:
> Make whitespace follow the same pattern in all xfs_isilocked() calls.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v5 3/4] xfs: xfs_isilocked() can only check a single lock type

[Pavel Reichl]

In its current form, xfs_isilocked() is only able to test one lock type
at a time - ilock, iolock, or mmap lock, but combinations are not
properly handled. The intent here is to check that both XFS_IOLOCK_EXCL
and XFS_ILOCK_EXCL are held, so test them each separately.

The commit ecfea3f0c8c6 ("xfs: split xfs_bmap_shift_extents") ORed the
flags together which was an error, so this patch reverts that part of
the change and check the locks independently.

Fixes: ecfea3f0c8c6 ("xfs: split xfs_bmap_shift_extents")

Reviewed-by: Eric Sandeen <sandeen@redhat.com>
Suggested-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 fs/xfs/libxfs/xfs_bmap.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index bc2be29193aa..c9dc94f114ed 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5829,7 +5829,8 @@ xfs_bmap_collapse_extents(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -EIO;
 
-	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));
+	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 
 	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
 		error = xfs_iread_extents(tp, ip, whichfork);
@@ -5946,7 +5947,8 @@ xfs_bmap_insert_extents(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -EIO;
 
-	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));
+	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 
 	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
 		error = xfs_iread_extents(tp, ip, whichfork);
-- 
2.24.1

Re: [PATCH v5 3/4] xfs: xfs_isilocked() can only check a single lock type

[Dave Chinner]

On Fri, Feb 14, 2020 at 07:59:41PM +0100, Pavel Reichl wrote:
> In its current form, xfs_isilocked() is only able to test one lock type
> at a time - ilock, iolock, or mmap lock, but combinations are not
> properly handled. The intent here is to check that both XFS_IOLOCK_EXCL
> and XFS_ILOCK_EXCL are held, so test them each separately.
> 
> The commit ecfea3f0c8c6 ("xfs: split xfs_bmap_shift_extents") ORed the
> flags together which was an error, so this patch reverts that part of
> the change and check the locks independently.
> 
> Fixes: ecfea3f0c8c6 ("xfs: split xfs_bmap_shift_extents")
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Pavel Reichl <preichl@redhat.com>

looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v5 3/4] xfs: xfs_isilocked() can only check a single lock type

[Christoph Hellwig]

Looks good,

although as a standalone fix this should really be first in the series.

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v5 4/4] xfs: replace mrlock_t with rw_semaphores

[Pavel Reichl]

Remove mrlock_t as it does not provide any extra value over
rw_semaphores. Make i_lock and i_mmaplock native rw_semaphores and
replace mr*() functions with native rwsem calls.

Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 fs/xfs/mrlock.h    | 78 ----------------------------------------------
 fs/xfs/xfs_inode.c | 40 +++++++++++++-----------
 fs/xfs/xfs_inode.h |  4 +--
 fs/xfs/xfs_iops.c  |  4 +--
 fs/xfs/xfs_linux.h |  2 +-
 fs/xfs/xfs_super.c |  6 ++--
 6 files changed, 29 insertions(+), 105 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 79155eec341b..000000000000
--- a/fs/xfs/mrlock.h
+++ /dev/null
@@ -1,78 +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;
-#if defined(DEBUG) || defined(XFS_WARN)
-	int			mr_writer;
-#endif
-} mrlock_t;
-
-#if defined(DEBUG) || defined(XFS_WARN)
-#define mrinit(mrp, name)	\
-	do { (mrp)->mr_writer = 0; init_rwsem(&(mrp)->mr_lock); } while (0)
-#else
-#define mrinit(mrp, name)	\
-	do { init_rwsem(&(mrp)->mr_lock); } while (0)
-#endif
-
-#define mrlock_init(mrp, t,n,s)	mrinit(mrp, n)
-#define mrfree(mrp)		do { } while (0)
-
-static inline void mraccess_nested(mrlock_t *mrp, int subclass)
-{
-	down_read_nested(&mrp->mr_lock, subclass);
-}
-
-static inline void mrupdate_nested(mrlock_t *mrp, int subclass)
-{
-	down_write_nested(&mrp->mr_lock, subclass);
-#if defined(DEBUG) || defined(XFS_WARN)
-	mrp->mr_writer = 1;
-#endif
-}
-
-static inline int mrtryaccess(mrlock_t *mrp)
-{
-	return down_read_trylock(&mrp->mr_lock);
-}
-
-static inline int mrtryupdate(mrlock_t *mrp)
-{
-	if (!down_write_trylock(&mrp->mr_lock))
-		return 0;
-#if defined(DEBUG) || defined(XFS_WARN)
-	mrp->mr_writer = 1;
-#endif
-	return 1;
-}
-
-static inline void mrunlock_excl(mrlock_t *mrp)
-{
-#if defined(DEBUG) || defined(XFS_WARN)
-	mrp->mr_writer = 0;
-#endif
-	up_write(&mrp->mr_lock);
-}
-
-static inline void mrunlock_shared(mrlock_t *mrp)
-{
-	up_read(&mrp->mr_lock);
-}
-
-static inline void mrdemote(mrlock_t *mrp)
-{
-#if defined(DEBUG) || defined(XFS_WARN)
-	mrp->mr_writer = 0;
-#endif
-	downgrade_write(&mrp->mr_lock);
-}
-
-#endif /* __XFS_SUPPORT_MRLOCK_H__ */
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 14f46f099470..1f979e46c6bf 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);
 
@@ -374,13 +375,16 @@ xfs_isilocked(
 	uint			lock_flags)
 {
 	if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) {
-		return __xfs_rwsem_islocked(&ip->i_lock.mr_lock,
+		ASSERT(!(lock_flags & ~(XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)));
+		return __xfs_rwsem_islocked(&ip->i_lock,
 				(lock_flags & XFS_ILOCK_SHARED),
 				(lock_flags & XFS_ILOCK_EXCL));
 	}
 
 	if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) {
-		return __xfs_rwsem_islocked(&ip->i_mmaplock.mr_lock,
+		ASSERT(!(lock_flags &
+			~(XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)));
+		return __xfs_rwsem_islocked(&ip->i_mmaplock,
 				(lock_flags & XFS_MMAPLOCK_SHARED),
 				(lock_flags & XFS_MMAPLOCK_EXCL));
 	}
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 3d7ce355407d..a7cf112a9e4f 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -39,8 +39,8 @@ typedef struct xfs_inode {
 
 	/* Transaction and locking information. */
 	struct xfs_inode_log_item *i_itemp;	/* logging information */
-	mrlock_t		i_lock;		/* inode lock */
-	mrlock_t		i_mmaplock;	/* inode mmap IO lock */
+	struct rw_semaphore	i_lock;		/* inode lock */
+	struct rw_semaphore	i_mmaplock;	/* inode mmap IO lock */
 	atomic_t		i_pincount;	/* inode pin count */
 
 	/*
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 81f2f93caec0..7c3df574cf4c 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -1319,9 +1319,9 @@ xfs_setup_inode(
 		 */
 		lockdep_set_class(&inode->i_rwsem,
 				  &inode->i_sb->s_type->i_mutex_dir_key);
-		lockdep_set_class(&ip->i_lock.mr_lock, &xfs_dir_ilock_class);
+		lockdep_set_class(&ip->i_lock, &xfs_dir_ilock_class);
 	} else {
-		lockdep_set_class(&ip->i_lock.mr_lock, &xfs_nondir_ilock_class);
+		lockdep_set_class(&ip->i_lock, &xfs_nondir_ilock_class);
 	}
 
 	/*
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index 8738bb03f253..8248744f1245 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>
@@ -60,6 +59,7 @@ typedef __u32			xfs_nlink_t;
 #include <linux/list_sort.h>
 #include <linux/ratelimit.h>
 #include <linux/rhashtable.h>
+#include <linux/rwsem.h>
 
 #include <asm/page.h>
 #include <asm/div64.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

Re: [PATCH v5 4/4] xfs: replace mrlock_t with rw_semaphores

[Dave Chinner]

On Fri, Feb 14, 2020 at 07:59:42PM +0100, Pavel Reichl wrote:
> Remove mrlock_t as it does not provide any extra value over
> rw_semaphores. Make i_lock and i_mmaplock native rw_semaphores and
> replace mr*() functions with native rwsem calls.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v5 4/4] xfs: replace mrlock_t with rw_semaphores

[Christoph Hellwig]

On Fri, Feb 14, 2020 at 07:59:42PM +0100, Pavel Reichl wrote:
> Remove mrlock_t as it does not provide any extra value over
> rw_semaphores. Make i_lock and i_mmaplock native rw_semaphores and
> replace mr*() functions with native rwsem calls.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v5 1/4] xfs: Refactor xfs_isilocked()

[Chaitanya Kulkarni]

Since it has more than one patch and version 5,
I couldn't find the cover-letter and a change log for this
series, is there a particular reason why it is not present or I
missed it?

On 02/14/2020 11:00 AM, Pavel Reichl wrote:
> Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked().
> __xfs_rwsem_islocked() is a helper function which encapsulates checking
> state of rw_semaphores hold by inode.
>
>

Re: [PATCH v5 1/4] xfs: Refactor xfs_isilocked()

[Pavel Reichl]

On Sat, Feb 15, 2020 at 2:38 AM Chaitanya Kulkarni
<Chaitanya.Kulkarni@wdc.com> wrote:
>
> Since it has more than one patch and version 5,
> I couldn't find the cover-letter and a change log for this
> series, is there a particular reason why it is not present or I
> missed it?
>
> On 02/14/2020 11:00 AM, Pavel Reichl wrote:
> > Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked().
> > __xfs_rwsem_islocked() is a helper function which encapsulates checking
> > state of rw_semaphores hold by inode.
> >
> >
>
>

Hi Chaitanya,

sorry for the absence of the changelog I forgot to add it - that was
not on purpose.

To summarize the changes: I moved the asserts from the first patch to
the third as suggested by Eric and changed the commit messages as
suggested by Dave.

Regarding the missing cover-letter it was same since version one and I
was not sure I should resend it with every new version, should I?

 Thanks you for your comments.

Best regards

Pavel Reichl

Re: [PATCH v5 1/4] xfs: Refactor xfs_isilocked()

[Chaitanya Kulkarni]

On 02/17/2020 02:56 AM, Pavel Reichl wrote:
> On Sat, Feb 15, 2020 at 2:38 AM Chaitanya Kulkarni
> <Chaitanya.Kulkarni@wdc.com>  wrote:
>> >
>> >Since it has more than one patch and version 5,
>> >I couldn't find the cover-letter and a change log for this
>> >series, is there a particular reason why it is not present or I
>> >missed it?
>> >
>> >On 02/14/2020 11:00 AM, Pavel Reichl wrote:
>>> > >Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked().
>>> > >__xfs_rwsem_islocked() is a helper function which encapsulates checking
>>> > >state of rw_semaphores hold by inode.
>>> > >
>>> > >
>> >
>> >
> Hi Chaitanya,
>
> sorry for the absence of the changelog I forgot to add it - that was
> not on purpose.
>
> To summarize the changes: I moved the asserts from the first patch to
> the third as suggested by Eric and changed the commit messages as
> suggested by Dave.
Thanks.
>
> Regarding the missing cover-letter it was same since version one and I
> was not sure I should resend it with every new version, should I?
It's okay, it just makes re-viewer's life easier to look for the change.
>
>   Thanks you for your comments.
>
> Best regards
>
> Pavel Reichl
>
>

Re: [PATCH v5 1/4] xfs: Refactor xfs_isilocked()

[Dave Chinner]

On Fri, Feb 14, 2020 at 07:59:39PM +0100, Pavel Reichl wrote:
> Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked().
> __xfs_rwsem_islocked() is a helper function which encapsulates checking
> state of rw_semaphores hold by inode.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>
> Suggested-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 54 ++++++++++++++++++++++++++++++++--------------
>  fs/xfs/xfs_inode.h |  2 +-
>  2 files changed, 39 insertions(+), 17 deletions(-)

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v5 1/4] xfs: Refactor xfs_isilocked()

[Christoph Hellwig]

On Fri, Feb 14, 2020 at 07:59:39PM +0100, Pavel Reichl wrote:
> Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked().
> __xfs_rwsem_islocked() is a helper function which encapsulates checking
> state of rw_semaphores hold by inode.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>
> Suggested-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 54 ++++++++++++++++++++++++++++++++--------------
>  fs/xfs/xfs_inode.h |  2 +-
>  2 files changed, 39 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c5077e6326c7..3d28c4790231 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -345,32 +345,54 @@ xfs_ilock_demote(
>  }
>  
>  #if defined(DEBUG) || defined(XFS_WARN)
> -int
> +static inline bool
> +__xfs_rwsem_islocked(
> +	struct rw_semaphore	*rwsem,
> +	bool			shared,
> +	bool			excl)
> +{
> +	bool locked = false;
> +
> +	if (!rwsem_is_locked(rwsem))
> +		return false;
> +
> +	if (!debug_locks)
> +		return true;
> +
> +	if (shared)
> +		locked = lockdep_is_held_type(rwsem, 0);
> +
> +	if (excl)
> +		locked |= lockdep_is_held_type(rwsem, 1);
> +
> +	return locked;

This could use some comments explaining the logic, especially why we
need the shared and excl flags, which seems very confusing given that
a lock can be held either shared or exclusive, but not neither and not
both.

Re: [PATCH v5 1/4] xfs: Refactor xfs_isilocked()

[Darrick J. Wong]

On Mon, Feb 17, 2020 at 05:35:21AM -0800, Christoph Hellwig wrote:
> On Fri, Feb 14, 2020 at 07:59:39PM +0100, Pavel Reichl wrote:
> > Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked().
> > __xfs_rwsem_islocked() is a helper function which encapsulates checking
> > state of rw_semaphores hold by inode.
> > 
> > Signed-off-by: Pavel Reichl <preichl@redhat.com>
> > Suggested-by: Dave Chinner <dchinner@redhat.com>
> > Suggested-by: Eric Sandeen <sandeen@redhat.com>
> > ---
> >  fs/xfs/xfs_inode.c | 54 ++++++++++++++++++++++++++++++++--------------
> >  fs/xfs/xfs_inode.h |  2 +-
> >  2 files changed, 39 insertions(+), 17 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index c5077e6326c7..3d28c4790231 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -345,32 +345,54 @@ xfs_ilock_demote(
> >  }
> >  
> >  #if defined(DEBUG) || defined(XFS_WARN)
> > -int
> > +static inline bool
> > +__xfs_rwsem_islocked(
> > +	struct rw_semaphore	*rwsem,
> > +	bool			shared,
> > +	bool			excl)
> > +{
> > +	bool locked = false;
> > +
> > +	if (!rwsem_is_locked(rwsem))
> > +		return false;
> > +
> > +	if (!debug_locks)
> > +		return true;
> > +
> > +	if (shared)
> > +		locked = lockdep_is_held_type(rwsem, 0);
> > +
> > +	if (excl)
> > +		locked |= lockdep_is_held_type(rwsem, 1);
> > +
> > +	return locked;
> 
> This could use some comments explaining the logic, especially why we
> need the shared and excl flags, which seems very confusing given that
> a lock can be held either shared or exclusive, but not neither and not
> both.

Yes, this predicate should document that callers are allowed to pass in
shared==true and excl==true when the caller wants to assert that either
lock type (shared or excl) of a given lock class (e.g. iolock) are held.

--D

Re: [PATCH v5 1/4] xfs: Refactor xfs_isilocked()

[Pavel Reichl]

On Wed, Feb 19, 2020 at 5:48 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> On Mon, Feb 17, 2020 at 05:35:21AM -0800, Christoph Hellwig wrote:
> > On Fri, Feb 14, 2020 at 07:59:39PM +0100, Pavel Reichl wrote:
> > > Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked().
> > > __xfs_rwsem_islocked() is a helper function which encapsulates checking
> > > state of rw_semaphores hold by inode.
> > >
> > > Signed-off-by: Pavel Reichl <preichl@redhat.com>
> > > Suggested-by: Dave Chinner <dchinner@redhat.com>
> > > Suggested-by: Eric Sandeen <sandeen@redhat.com>
> > > ---
> > >  fs/xfs/xfs_inode.c | 54 ++++++++++++++++++++++++++++++++--------------
> > >  fs/xfs/xfs_inode.h |  2 +-
> > >  2 files changed, 39 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > index c5077e6326c7..3d28c4790231 100644
> > > --- a/fs/xfs/xfs_inode.c
> > > +++ b/fs/xfs/xfs_inode.c
> > > @@ -345,32 +345,54 @@ xfs_ilock_demote(
> > >  }
> > >
> > >  #if defined(DEBUG) || defined(XFS_WARN)
> > > -int
> > > +static inline bool
> > > +__xfs_rwsem_islocked(
> > > +   struct rw_semaphore     *rwsem,
> > > +   bool                    shared,
> > > +   bool                    excl)
> > > +{
> > > +   bool locked = false;
> > > +
> > > +   if (!rwsem_is_locked(rwsem))
> > > +           return false;
> > > +
> > > +   if (!debug_locks)
> > > +           return true;
> > > +
> > > +   if (shared)
> > > +           locked = lockdep_is_held_type(rwsem, 0);
> > > +
> > > +   if (excl)
> > > +           locked |= lockdep_is_held_type(rwsem, 1);
> > > +
> > > +   return locked;
> >
> > This could use some comments explaining the logic, especially why we
> > need the shared and excl flags, which seems very confusing given that
> > a lock can be held either shared or exclusive, but not neither and not
> > both.
>
> Yes, this predicate should document that callers are allowed to pass in
> shared==true and excl==true when the caller wants to assert that either
> lock type (shared or excl) of a given lock class (e.g. iolock) are held.
>
> --D
>

Hello,

thanks for the comments.

Would code comment preceding the definition of __xfs_rwsem_islocked()
work for you?

Something like:

/* This is a helper function that encapsulates checking the state of
 * rw semaphores.
 *
 * if shared == true AND excl == true then function returns true if either
 * lock type (shared or excl) of a given semaphore are held.
 */

Re: [PATCH v5 1/4] xfs: Refactor xfs_isilocked()

[Christoph Hellwig]

On Tue, Feb 18, 2020 at 08:48:21PM -0800, Darrick J. Wong wrote:
> > > +static inline bool
> > > +__xfs_rwsem_islocked(
> > > +	struct rw_semaphore	*rwsem,
> > > +	bool			shared,
> > > +	bool			excl)
> > > +{
> > > +	bool locked = false;
> > > +
> > > +	if (!rwsem_is_locked(rwsem))
> > > +		return false;
> > > +
> > > +	if (!debug_locks)
> > > +		return true;
> > > +
> > > +	if (shared)
> > > +		locked = lockdep_is_held_type(rwsem, 0);
> > > +
> > > +	if (excl)
> > > +		locked |= lockdep_is_held_type(rwsem, 1);
> > > +
> > > +	return locked;
> > 
> > This could use some comments explaining the logic, especially why we
> > need the shared and excl flags, which seems very confusing given that
> > a lock can be held either shared or exclusive, but not neither and not
> > both.
> 
> Yes, this predicate should document that callers are allowed to pass in
> shared==true and excl==true when the caller wants to assert that either
> lock type (shared or excl) of a given lock class (e.g. iolock) are held.

Looking at the lockdep_is_held_type implementation, and our existing
code for i_rwsem I really don't see the point of the extra shared
check.  Something like:

static inline bool
__xfs_rwsem_islocked(
	struct rw_semaphore	*rwsem,
	bool			excl)
{
	if (rwsem_is_locked(rwsem)) {
		if (debug_locks && excl)
			return lockdep_is_held_type(rwsem, 1);
		return true;
	}

	return false;
}

should be all that we really need.

Re: [PATCH v5 1/4] xfs: Refactor xfs_isilocked()

[Eric Sandeen]

On 2/19/20 12:40 PM, Christoph Hellwig wrote:
> On Tue, Feb 18, 2020 at 08:48:21PM -0800, Darrick J. Wong wrote:
>>>> +static inline bool
>>>> +__xfs_rwsem_islocked(
>>>> +	struct rw_semaphore	*rwsem,
>>>> +	bool			shared,
>>>> +	bool			excl)
>>>> +{
>>>> +	bool locked = false;
>>>> +
>>>> +	if (!rwsem_is_locked(rwsem))
>>>> +		return false;
>>>> +
>>>> +	if (!debug_locks)
>>>> +		return true;
>>>> +
>>>> +	if (shared)
>>>> +		locked = lockdep_is_held_type(rwsem, 0);
>>>> +
>>>> +	if (excl)
>>>> +		locked |= lockdep_is_held_type(rwsem, 1);
>>>> +
>>>> +	return locked;
>>>
>>> This could use some comments explaining the logic, especially why we
>>> need the shared and excl flags, which seems very confusing given that
>>> a lock can be held either shared or exclusive, but not neither and not
>>> both.
>>
>> Yes, this predicate should document that callers are allowed to pass in
>> shared==true and excl==true when the caller wants to assert that either
>> lock type (shared or excl) of a given lock class (e.g. iolock) are held.
> 
> Looking at the lockdep_is_held_type implementation, and our existing
> code for i_rwsem I really don't see the point of the extra shared
> check.  Something like:
> 
> static inline bool
> __xfs_rwsem_islocked(
> 	struct rw_semaphore	*rwsem,
> 	bool			excl)
> {
> 	if (rwsem_is_locked(rwsem)) {
> 		if (debug_locks && excl)
> 			return lockdep_is_held_type(rwsem, 1);
> 		return true;
> 	}
> 
> 	return false;
> }
> 
> should be all that we really need.

I think that's a lot more clear.  In addition to the slight confusion over a (true, true)
set of args, the current proposal also has the extra confusion of what happens if we pass
(false, false), for example.

One other thought, since debug_locks getting turned off by lockdep means that
an exclusive test reverts to a shared|exclusive test, would it be worth adding
a WARN_ON_ONCE to make it clear when xfs rwsem lock testing coverage has been
reduced?

-Eric

Re: [PATCH v5 1/4] xfs: Refactor xfs_isilocked()

[Pavel Reichl]

On Wed, Feb 19, 2020 at 9:16 PM Eric Sandeen <sandeen@sandeen.net> wrote:
>
>
>
> On 2/19/20 12:40 PM, Christoph Hellwig wrote:
> > On Tue, Feb 18, 2020 at 08:48:21PM -0800, Darrick J. Wong wrote:
> >>>> +static inline bool
> >>>> +__xfs_rwsem_islocked(
> >>>> +  struct rw_semaphore     *rwsem,
> >>>> +  bool                    shared,
> >>>> +  bool                    excl)
> >>>> +{
> >>>> +  bool locked = false;
> >>>> +
> >>>> +  if (!rwsem_is_locked(rwsem))
> >>>> +          return false;
> >>>> +
> >>>> +  if (!debug_locks)
> >>>> +          return true;
> >>>> +
> >>>> +  if (shared)
> >>>> +          locked = lockdep_is_held_type(rwsem, 0);
> >>>> +
> >>>> +  if (excl)
> >>>> +          locked |= lockdep_is_held_type(rwsem, 1);
> >>>> +
> >>>> +  return locked;
> >>>
> >>> This could use some comments explaining the logic, especially why we
> >>> need the shared and excl flags, which seems very confusing given that
> >>> a lock can be held either shared or exclusive, but not neither and not
> >>> both.
> >>
> >> Yes, this predicate should document that callers are allowed to pass in
> >> shared==true and excl==true when the caller wants to assert that either
> >> lock type (shared or excl) of a given lock class (e.g. iolock) are held.
> >
> > Looking at the lockdep_is_held_type implementation, and our existing
> > code for i_rwsem I really don't see the point of the extra shared
> > check.  Something like:
> >
> > static inline bool
> > __xfs_rwsem_islocked(
> >       struct rw_semaphore     *rwsem,
> >       bool                    excl)
> > {
> >       if (rwsem_is_locked(rwsem)) {
> >               if (debug_locks && excl)
> >                       return lockdep_is_held_type(rwsem, 1);
> >               return true;
> >       }
> >
> >       return false;
> > }
> >
> > should be all that we really need.
>
> I think that's a lot more clear.  In addition to the slight confusion over a (true, true)
> set of args, the current proposal also has the extra confusion of what happens if we pass
> (false, false), for example.
>
> One other thought, since debug_locks getting turned off by lockdep means that
> an exclusive test reverts to a shared|exclusive test, would it be worth adding
> a WARN_ON_ONCE to make it clear when xfs rwsem lock testing coverage has been
> reduced?
>
> -Eric
>

OK, thanks for the comments.

Eric in the following code is WARN_ONCE() used as you suggested or did
you have something else in mind?

static inline bool
__xfs_rwsem_islocked(
        struct rw_semaphore     *rwsem,
        bool                    excl)
{
        if (!rwsem_is_locked(rwsem)) {
                return false;
        }

        if (excl) {
                if (debug_locks) {
                        return lockdep_is_held_type(rwsem, 1);
                }
                WARN_ONCE(1,
                        "xfs rwsem lock testing coverage has been reduced\n");
        }
        return true;
}

Re: [PATCH v5 1/4] xfs: Refactor xfs_isilocked()

[Christoph Hellwig]

On Thu, Feb 20, 2020 at 05:30:35PM +0100, Pavel Reichl wrote:
> OK, thanks for the comments.
> 
> Eric in the following code is WARN_ONCE() used as you suggested or did
> you have something else in mind?
> 
> static inline bool
> __xfs_rwsem_islocked(
>         struct rw_semaphore     *rwsem,
>         bool                    excl)
> {
>         if (!rwsem_is_locked(rwsem)) {
>                 return false;
>         }
> 
>         if (excl) {
>                 if (debug_locks) {
>                         return lockdep_is_held_type(rwsem, 1);
>                 }
>                 WARN_ONCE(1,
>                         "xfs rwsem lock testing coverage has been reduced\n");
>         }

Yikes, hell no.  This means every debug xfs build without lockdep
will be full of warnings all the time.

Re: [PATCH v5 1/4] xfs: Refactor xfs_isilocked()

[Eric Sandeen]

On 2/20/20 10:32 AM, Christoph Hellwig wrote:
> On Thu, Feb 20, 2020 at 05:30:35PM +0100, Pavel Reichl wrote:
>> OK, thanks for the comments.
>>
>> Eric in the following code is WARN_ONCE() used as you suggested or did
>> you have something else in mind?
>>
>> static inline bool
>> __xfs_rwsem_islocked(
>>         struct rw_semaphore     *rwsem,
>>         bool                    excl)
>> {
>>         if (!rwsem_is_locked(rwsem)) {
>>                 return false;
>>         }
>>
>>         if (excl) {
>>                 if (debug_locks) {
>>                         return lockdep_is_held_type(rwsem, 1);
>>                 }
>>                 WARN_ONCE(1,
>>                         "xfs rwsem lock testing coverage has been reduced\n");
>>         }
> 
> Yikes, hell no.  This means every debug xfs build without lockdep
> will be full of warnings all the time.

Let's just drop my suggestion for now, no need to further complicate the series
at this time.

-Eric

Re: [PATCH v5 1/4] xfs: Refactor xfs_isilocked()

[Darrick J. Wong]

On Thu, Feb 20, 2020 at 08:32:32AM -0800, Christoph Hellwig wrote:
> On Thu, Feb 20, 2020 at 05:30:35PM +0100, Pavel Reichl wrote:
> > OK, thanks for the comments.
> > 
> > Eric in the following code is WARN_ONCE() used as you suggested or did
> > you have something else in mind?
> > 
> > static inline bool
> > __xfs_rwsem_islocked(
> >         struct rw_semaphore     *rwsem,
> >         bool                    excl)
> > {
> >         if (!rwsem_is_locked(rwsem)) {
> >                 return false;
> >         }
> > 
> >         if (excl) {
> >                 if (debug_locks) {
> >                         return lockdep_is_held_type(rwsem, 1);
> >                 }
> >                 WARN_ONCE(1,
> >                         "xfs rwsem lock testing coverage has been reduced\n");
> >         }
> 
> Yikes, hell no.  This means every debug xfs build without lockdep
> will be full of warnings all the time.

Well... once per module load, but if you /were/ going to go down this
route I'd at least gate the warning on IS_ENABLED(CONFIG_LOCKDEP) so
that we only get this one-time warning when lockdep is enabled but dies
anyway, so that we'll know that we're running with half a brain.

--D

Re: [PATCH v5 1/4] xfs: Refactor xfs_isilocked()

[Pavel Reichl]

On Wed, Feb 19, 2020 at 7:40 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Feb 18, 2020 at 08:48:21PM -0800, Darrick J. Wong wrote:
> > > > +static inline bool
> > > > +__xfs_rwsem_islocked(
> > > > + struct rw_semaphore     *rwsem,
> > > > + bool                    shared,
> > > > + bool                    excl)
> > > > +{
> > > > + bool locked = false;
> > > > +
> > > > + if (!rwsem_is_locked(rwsem))
> > > > +         return false;
> > > > +
> > > > + if (!debug_locks)
> > > > +         return true;
> > > > +
> > > > + if (shared)
> > > > +         locked = lockdep_is_held_type(rwsem, 0);
> > > > +
> > > > + if (excl)
> > > > +         locked |= lockdep_is_held_type(rwsem, 1);
> > > > +
> > > > + return locked;
> > >
> > > This could use some comments explaining the logic, especially why we
> > > need the shared and excl flags, which seems very confusing given that
> > > a lock can be held either shared or exclusive, but not neither and not
> > > both.
> >
> > Yes, this predicate should document that callers are allowed to pass in
> > shared==true and excl==true when the caller wants to assert that either
> > lock type (shared or excl) of a given lock class (e.g. iolock) are held.
>
> Looking at the lockdep_is_held_type implementation, and our existing
> code for i_rwsem I really don't see the point of the extra shared
> check.  Something like:
>
> static inline bool
> __xfs_rwsem_islocked(
>         struct rw_semaphore     *rwsem,
>         bool                    excl)
> {
>         if (rwsem_is_locked(rwsem)) {
>                 if (debug_locks && excl)
>                         return lockdep_is_held_type(rwsem, 1);
>                 return true;
>         }
>
>         return false;
> }
>
> should be all that we really need.
>

You don't see the point of extra shared check, but if we want to check
that the semaphore is locked for reading and not writing? Having the
semaphore locked for writing would make the code safe from race
condition but could be a performance hit, right?

Thanks for comments.

Re: [PATCH v5 1/4] xfs: Refactor xfs_isilocked()

[Eric Sandeen]

On 2/21/20 11:49 AM, Pavel Reichl wrote:
> On Wed, Feb 19, 2020 at 7:40 PM Christoph Hellwig <hch@infradead.org> wrote:
>>
>> On Tue, Feb 18, 2020 at 08:48:21PM -0800, Darrick J. Wong wrote:
>>>>> +static inline bool
>>>>> +__xfs_rwsem_islocked(
>>>>> + struct rw_semaphore     *rwsem,
>>>>> + bool                    shared,
>>>>> + bool                    excl)
>>>>> +{
>>>>> + bool locked = false;
>>>>> +
>>>>> + if (!rwsem_is_locked(rwsem))
>>>>> +         return false;
>>>>> +
>>>>> + if (!debug_locks)
>>>>> +         return true;
>>>>> +
>>>>> + if (shared)
>>>>> +         locked = lockdep_is_held_type(rwsem, 0);
>>>>> +
>>>>> + if (excl)
>>>>> +         locked |= lockdep_is_held_type(rwsem, 1);
>>>>> +
>>>>> + return locked;
>>>>
>>>> This could use some comments explaining the logic, especially why we
>>>> need the shared and excl flags, which seems very confusing given that
>>>> a lock can be held either shared or exclusive, but not neither and not
>>>> both.
>>>
>>> Yes, this predicate should document that callers are allowed to pass in
>>> shared==true and excl==true when the caller wants to assert that either
>>> lock type (shared or excl) of a given lock class (e.g. iolock) are held.
>>
>> Looking at the lockdep_is_held_type implementation, and our existing
>> code for i_rwsem I really don't see the point of the extra shared
>> check.  Something like:
>>
>> static inline bool
>> __xfs_rwsem_islocked(
>>         struct rw_semaphore     *rwsem,
>>         bool                    excl)
>> {
>>         if (rwsem_is_locked(rwsem)) {
>>                 if (debug_locks && excl)
>>                         return lockdep_is_held_type(rwsem, 1);
>>                 return true;
>>         }
>>
>>         return false;
>> }
>>
>> should be all that we really need.
>>
> 
> You don't see the point of extra shared check, but if we want to check
> that the semaphore is locked for reading and not writing? Having the
> semaphore locked for writing would make the code safe from race
> condition but could be a performance hit, right?

So, I raised this question with Pavel but I think maybe it was borne
of my misunderstanding.

Ok let me think this through.  Today we have:

int
xfs_isilocked(
        xfs_inode_t             *ip,
        uint                    lock_flags)
{
        if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
                if (!(lock_flags & XFS_ILOCK_SHARED))
                        return !!ip->i_lock.mr_writer;
                return rwsem_is_locked(&ip->i_lock.mr_lock);
        }
        ....

If we assert xfs_isilocked(ip, XFS_ILOCK_SHARED) I guess we /already/ get a positive
result if the inode is actually locked XFS_ILOCK_EXCL.  So perhaps Christoph's
suggestion really just keeps implementing what we already have today.

It might be a reasonable question re: whether we ever want to know that we are locked
shared and NOT locked exclusive, but we can't do that today, so I guess it shouldn't
complicate this patchset.

... do I have this right?

Thanks,
-Eric