[PATCH v6 0/4] xfs: Remove wrappers for some semaphores

[PATCH v6 0/4] xfs: Remove wrappers for some semaphores

[Pavel Reichl]

Remove some wrappers that we have in XFS around the read-write semaphore
locks.

The goal of this cleanup is to remove mrlock_t structure and its mr*()
wrapper functions and replace it with native rw_semaphore type and its
native calls.

Pavel Reichl (4):
  xfs: Refactor xfs_isilocked()
  xfs: clean up whitespace in xfs_isilocked() calls
  xfs: xfs_isilocked() can only check a single lock type
  xfs: replace mrlock_t with rw_semaphores

 fs/xfs/libxfs/xfs_bmap.c |  8 ++--
 fs/xfs/mrlock.h          | 78 -------------------------------------
 fs/xfs/xfs_file.c        |  3 +-
 fs/xfs/xfs_inode.c       | 84 +++++++++++++++++++++++-----------------
 fs/xfs/xfs_inode.h       |  6 +--
 fs/xfs/xfs_iops.c        |  4 +-
 fs/xfs/xfs_linux.h       |  2 +-
 fs/xfs/xfs_qm.c          |  2 +-
 fs/xfs/xfs_super.c       |  6 +--
 9 files changed, 65 insertions(+), 128 deletions(-)
 delete mode 100644 fs/xfs/mrlock.h

-- 
2.24.1

[PATCH v6 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>
---
Changes from V5:
	Drop shared flag from __xfs_rwsem_islocked()


 fs/xfs/xfs_inode.c | 42 ++++++++++++++++++++++++++----------------
 fs/xfs/xfs_inode.h |  2 +-
 2 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index c5077e6326c7..4faf7827717b 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -345,32 +345,42 @@ xfs_ilock_demote(
 }
 
 #if defined(DEBUG) || defined(XFS_WARN)
-int
+static inline bool
+__xfs_rwsem_islocked(
+	struct rw_semaphore	*rwsem,
+	bool			excl)
+{
+	if (!rwsem_is_locked(rwsem))
+		return false;
+
+	if (debug_locks && excl)
+		return lockdep_is_held_type(rwsem, 1);
+
+	return true;
+}
+
+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_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_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_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 v6 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>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
Changes from V5: None


 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 4faf7827717b..e778739adbf8 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2816,7 +2816,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_);
 
@@ -3680,7 +3680,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));
@@ -3812,7 +3812,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

[PATCH v6 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>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Suggested-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
Changes from V5: None

 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

[PATCH v6 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>
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
Changes from V5: None

 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 e778739adbf8..c07323e05db9 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);
 
@@ -365,12 +366,15 @@ 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_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_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 v6 1/4] xfs: Refactor xfs_isilocked()

[Darrick J. Wong]

On Thu, Feb 27, 2020 at 09:36:33PM +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>
> ---
> Changes from V5:
> 	Drop shared flag from __xfs_rwsem_islocked()
> 
> 
>  fs/xfs/xfs_inode.c | 42 ++++++++++++++++++++++++++----------------
>  fs/xfs/xfs_inode.h |  2 +-
>  2 files changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c5077e6326c7..4faf7827717b 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -345,32 +345,42 @@ xfs_ilock_demote(
>  }
>  
>  #if defined(DEBUG) || defined(XFS_WARN)
> -int
> +static inline bool
> +__xfs_rwsem_islocked(
> +	struct rw_semaphore	*rwsem,
> +	bool			excl)
> +{
> +	if (!rwsem_is_locked(rwsem))
> +		return false;

So, uh, I finally made the time to dig through what exactly lockdep
provides as far as testing functions, and came up with the following
truth table for the old xfs_isilocked behavior w.r.t. IOLOCK:

(nolockdep corresponds to debug_locks == 0)

RWSEM STATE		PARAMETERS TO XFS_ISILOCKED:
			SHARED	EXCL	SHARED | EXCL
readlocked		y	n	y
writelocked		y	y	y
unlocked		n	n	n
nolockdep readlocked	y	y	y
nolockdep writelocked	y	y	y
nolockdep unlocked	n	y	n

Note that EXCL × nolockdep_unlocked returns an incorrect result, but
because we only use it with ASSERTs there haven't been any failures
reported.

And here's your new version:

readlocked		y	y	y
writelocked		y	n	n
unlocked		n	n	n
nolockdep readlocked	y	y	y
nolockdep writelocked	y	y	y
nolockdep unlocked	n	n	n

Thanks for fixing the false positive that I mentioned above.

> +
> +	if (debug_locks && excl)
> +		return lockdep_is_held_type(rwsem, 1);

This is wrong, the second parameter of lockdep_is_held_type is 0 to test
if the rwsem is write-locked; 1 to test if it is read-locked; or -1 to
test if the rwsem is read or write-locked.

So, this function's call signature should change so that callers can
communicate both _SHARED and _EXCL; and then you can pick the correct
"r" parameter value for the lockdep_is_held_type() call.  Then all of
this becomes:

	if !debug_locks:
		return rwsem_is_locked(rwsem)

	if shared and excl:
		r = -1
	elif shared:
		r = 1
	else:
		r = 0
	return lockdep_is_held_type(rwsem, r)

Note also that you don't necessarily need to pass shared and excl as
separate parameters (as you did in v3); the XFS_*LOCK_{EXCL,SHARED}
definitions enable you to take care of all that with some clever bit
shifting and masking.

--D

> +
> +	return true;
> +}
> +
> +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_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_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_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
> 

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

[Pavel Reichl]

On Fri, Feb 28, 2020 at 6:10 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> On Thu, Feb 27, 2020 at 09:36:33PM +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>
> > ---
> > Changes from V5:
> >       Drop shared flag from __xfs_rwsem_islocked()
> >
> >
> >  fs/xfs/xfs_inode.c | 42 ++++++++++++++++++++++++++----------------
> >  fs/xfs/xfs_inode.h |  2 +-
> >  2 files changed, 27 insertions(+), 17 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index c5077e6326c7..4faf7827717b 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -345,32 +345,42 @@ xfs_ilock_demote(
> >  }
> >
> >  #if defined(DEBUG) || defined(XFS_WARN)
> > -int
> > +static inline bool
> > +__xfs_rwsem_islocked(
> > +     struct rw_semaphore     *rwsem,
> > +     bool                    excl)
> > +{
> > +     if (!rwsem_is_locked(rwsem))
> > +             return false;
>
> So, uh, I finally made the time to dig through what exactly lockdep
> provides as far as testing functions, and came up with the following
> truth table for the old xfs_isilocked behavior w.r.t. IOLOCK:

Thank you for doing that and I'm sorry that I'm replying with such a delay.

>
> (nolockdep corresponds to debug_locks == 0)
>
> RWSEM STATE             PARAMETERS TO XFS_ISILOCKED:
>                         SHARED  EXCL    SHARED | EXCL
> readlocked              y       n       y
> writelocked             y       y       y
> unlocked                n       n       n
> nolockdep readlocked    y       y       y
> nolockdep writelocked   y       y       y
> nolockdep unlocked      n       y       n
>
> Note that EXCL × nolockdep_unlocked returns an incorrect result, but
> because we only use it with ASSERTs there haven't been any failures
> reported.
>
> And here's your new version:
>
> readlocked              y       y       y
> writelocked             y       n       n
> unlocked                n       n       n
> nolockdep readlocked    y       y       y
> nolockdep writelocked   y       y       y
> nolockdep unlocked      n       n       n
>
> Thanks for fixing the false positive that I mentioned above.
>
> > +
> > +     if (debug_locks && excl)
> > +             return lockdep_is_held_type(rwsem, 1);
>
> This is wrong, the second parameter of lockdep_is_held_type is 0 to test
> if the rwsem is write-locked; 1 to test if it is read-locked; or -1 to
> test if the rwsem is read or write-locked.

Thank you for noticing.


>
> So, this function's call signature should change so that callers can
> communicate both _SHARED and _EXCL; and then you can pick the correct

Thanks for the suggestion...but that's how v5 signature looked like
before Christoph and Eric requested change...on the grounds that
there're:
*  confusion over a (true, true) set of args
*  confusion of what happens if we pass (false, false).

> "r" parameter value for the lockdep_is_held_type() call.  Then all of
> this becomes:
>
>         if !debug_locks:
>                 return rwsem_is_locked(rwsem)
>
>         if shared and excl:
>                 r = -1
>         elif shared:
>                 r = 1
>         else:
>                 r = 0
>         return lockdep_is_held_type(rwsem, r)

I tried to create a table for this code as well:

readlocked              y       n       y
writelocked             *n*       y       y
unlocked                n       n       n
nolockdep readlocked    y       y       y
nolockdep writelocked   y       y       y
nolockdep unlocked      n       n       n

I think that when we query writelocked lock for being shared having
'no' for an answer may not be expected...or at least this is how I
read the code.

int __lock_is_held(const struct lockdep_map *lock, int read)
{
        struct task_struct *curr = current;
        int i;

        for (i = 0; i < curr->lockdep_depth; i++) {
                struct held_lock *hlock = curr->held_locks + i;

                if (match_held_lock(hlock, lock)) {
                        if (read == -1 || hlock->read == read)
                                return 1;

                        return 0;
                }
        }

        return 0;
}

Thanks for any comments

>
> Note also that you don't necessarily need to pass shared and excl as
> separate parameters (as you did in v3); the XFS_*LOCK_{EXCL,SHARED}
> definitions enable you to take care of all that with some clever bit
> shifting and masking.
>
> --D
>
> > +
> > +     return true;
> > +}
> > +
> > +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_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_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_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
> >
>

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

[Eric Sandeen]

On 3/18/20 12:13 PM, Pavel Reichl wrote:
> On Fri, Feb 28, 2020 at 6:10 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:

...

>> So, this function's call signature should change so that callers can
>> communicate both _SHARED and _EXCL; and then you can pick the correct
> 
> Thanks for the suggestion...but that's how v5 signature looked like
> before Christoph and Eric requested change...on the grounds that
> there're:
> *  confusion over a (true, true) set of args
> *  confusion of what happens if we pass (false, false).
> 
>> "r" parameter value for the lockdep_is_held_type() call.  Then all of
>> this becomes:
>>
>>         if !debug_locks:
>>                 return rwsem_is_locked(rwsem)
>>
>>         if shared and excl:
>>                 r = -1
>>         elif shared:
>>                 r = 1
>>         else:
>>                 r = 0
>>         return lockdep_is_held_type(rwsem, r)
> 
> I tried to create a table for this code as well:

<adding back the table headers>

> (nolockdep corresponds to debug_locks == 0)
>
> RWSEM STATE             PARAMETERS TO XFS_ISILOCKED:
>                         SHARED  EXCL    SHARED | EXCL
> readlocked              y       n       y
> writelocked             *n*     y       y
> unlocked                n       n       n
> nolockdep readlocked    y       y       y
> nolockdep writelocked   y       y       y
> nolockdep unlocked      n       n       n
> 
> I think that when we query writelocked lock for being shared having
> 'no' for an answer may not be expected...or at least this is how I
> read the code.

This might be ok, because
a) it is technically correct (is it shared? /no/ it is exclusive), and
b) in the XFS code today we never call:

	xfs_isilocked(ip, XFS_ILOCK_SHARED);

it's always:

	xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL);

So I think that if we document the behavior clearly, the truth table above
would be ok.

Thoughts?

-Eric

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

[Darrick J. Wong]

On Wed, Mar 18, 2020 at 12:46:37PM -0500, Eric Sandeen wrote:
> On 3/18/20 12:13 PM, Pavel Reichl wrote:
> > On Fri, Feb 28, 2020 at 6:10 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> 
> ...
> 
> >> So, this function's call signature should change so that callers can
> >> communicate both _SHARED and _EXCL; and then you can pick the correct
> > 
> > Thanks for the suggestion...but that's how v5 signature looked like
> > before Christoph and Eric requested change...on the grounds that
> > there're:
> > *  confusion over a (true, true) set of args
> > *  confusion of what happens if we pass (false, false).

Yeah.  I don't mean adding back the dual booleans, I meant refactoring
the way we define the lock constants so that you can use bit shifting
and masking:

#define XFS_IOLOCK_SHIFT	0
#define XFS_ILOCK_SHIFT		2
#define XFS_MMAPLOCK_SHIFT	4

#define XFS_SHARED_LOCK_SHIFT	1

#define XFS_IOLOCK_EXCL	    (1 << (XFS_IOLOCK_SHIFT))
#define XFS_IOLOCK_SHARED   (1 << (XFS_IOLOCK_SHIFT + XFS_SHARED_LOCK_SHIFT))
#define XFS_ILOCK_EXCL	    (1 << (XFS_ILOCK_SHIFT))
#define XFS_ILOCK_SHARED    (1 << (XFS_ILOCK_SHIFT + XFS_SHARED_LOCK_SHIFT))
#define XFS_MMAPLOCK_EXCL   (1 << (XFS_MMAPLOCK_SHIFT))
#define XFS_MMAPLOCK_SHARED (1 << (XFS_MMAPLOCK_SHIFT + XFS_SHARED_LOCK_SHIFT))

Because then in the outer xfs_isilocked function you can do:

if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED))
	return __isilocked(&ip->i_lock, lock_flags >> XFS_ILOCK_SHIFT);

if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED))
	return __isilocked(&ip->i_mmaplock, lock_flags >> XFS_MMAPLOCK_SHIFT);

if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED))
	return __isilocked(&VFS_I(ip)->i_rwsem, lock_flags >> XFS_IOLOCK_SHIFT);

And finally in __isilocked you can do:

static inline bool __isilocked(rwsem, lock_flags)
{
	int	arg;

	if (!debug_locks)
		return rwsem_is_locked(rwsem);

	if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) {
		/*
		 * The caller could be asking if we have (shared | excl)
		 * access to the lock.  Ask lockdep if the rwsem is
		 * locked either for read or write access.
		 *
		 * The caller could also be asking if we have only
		 * shared access to the lock.  Holding a rwsem
		 * write-locked implies read access as well, so the
		 * request to lockdep is the same for this case.
		 */
		arg = -1;
	} else {
		/*
		 * The caller is asking if we have only exclusive access
		 * to the lock.  Ask lockdep if the rwsem is locked for
		 * write access.
		 */
		arg = 0;
	}
	return lockdep_is_held_type(rwsem, arg);
}

> >> "r" parameter value for the lockdep_is_held_type() call.  Then all of
> >> this becomes:
> >>
> >>         if !debug_locks:
> >>                 return rwsem_is_locked(rwsem)
> >>
> >>         if shared and excl:
> >>                 r = -1
> >>         elif shared:
> >>                 r = 1
> >>         else:
> >>                 r = 0
> >>         return lockdep_is_held_type(rwsem, r)
> > 
> > I tried to create a table for this code as well:
> 
> <adding back the table headers>
> 
> > (nolockdep corresponds to debug_locks == 0)
> >
> > RWSEM STATE             PARAMETERS TO XFS_ISILOCKED:
> >                         SHARED  EXCL    SHARED | EXCL
> > readlocked              y       n       y
> > writelocked             *n*     y       y
> > unlocked                n       n       n
> > nolockdep readlocked    y       y       y
> > nolockdep writelocked   y       y       y
> > nolockdep unlocked      n       n       n
> > 
> > I think that when we query writelocked lock for being shared having
> > 'no' for an answer may not be expected...or at least this is how I
> > read the code.
> 
> This might be ok, because
> a) it is technically correct (is it shared? /no/ it is exclusive), and
> b) in the XFS code today we never call:
> 
> 	xfs_isilocked(ip, XFS_ILOCK_SHARED);
> 
> it's always:
> 
> 	xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL);
> 
> So I think that if we document the behavior clearly, the truth table above
> would be ok.
> 
> Thoughts?

No, Pavel's right, I got the pseudocode wrong, because holding a write
lock means you also hold the read lock.

	if !debug_locks:
		return rwsem_is_locked(rwsem)

	if shared:
		r = -1
	else:
		r = 0
	return lockdep_is_held_type(rwsem, r)

--D

> -Eric

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

[Eric Sandeen]

On 3/18/20 1:49 PM, Darrick J. Wong wrote:
>>> I think that when we query writelocked lock for being shared having
>>> 'no' for an answer may not be expected...or at least this is how I
>>> read the code.
>> This might be ok, because
>> a) it is technically correct (is it shared? /no/ it is exclusive), and
>> b) in the XFS code today we never call:
>>
>> 	xfs_isilocked(ip, XFS_ILOCK_SHARED);
>>
>> it's always:
>>
>> 	xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL);
>>
>> So I think that if we document the behavior clearly, the truth table above
>> would be ok.
>>
>> Thoughts?
> No, Pavel's right, I got the pseudocode wrong, because holding a write
> lock means you also hold the read lock.
> 
> 	if !debug_locks:
> 		return rwsem_is_locked(rwsem)
> 
> 	if shared:
> 		r = -1
> 	else:
> 		r = 0
> 	return lockdep_is_held_type(rwsem, r)

I think it comes down to semantics, and either way is fine as long as we
don't change the behavior of existing callers, and document exactly what
the islocked function does now.

-Eric

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

[Pavel Reichl]

On Wed, Mar 18, 2020 at 7:50 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> On Wed, Mar 18, 2020 at 12:46:37PM -0500, Eric Sandeen wrote:
> > On 3/18/20 12:13 PM, Pavel Reichl wrote:
> > > On Fri, Feb 28, 2020 at 6:10 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >
> > ...
> >
> > >> So, this function's call signature should change so that callers can
> > >> communicate both _SHARED and _EXCL; and then you can pick the correct
> > >
> > > Thanks for the suggestion...but that's how v5 signature looked like
> > > before Christoph and Eric requested change...on the grounds that
> > > there're:
> > > *  confusion over a (true, true) set of args
> > > *  confusion of what happens if we pass (false, false).
>
> Yeah.  I don't mean adding back the dual booleans, I meant refactoring
> the way we define the lock constants so that you can use bit shifting
> and masking:
>
> #define XFS_IOLOCK_SHIFT        0
> #define XFS_ILOCK_SHIFT         2
> #define XFS_MMAPLOCK_SHIFT      4
>
> #define XFS_SHARED_LOCK_SHIFT   1
>
> #define XFS_IOLOCK_EXCL     (1 << (XFS_IOLOCK_SHIFT))
> #define XFS_IOLOCK_SHARED   (1 << (XFS_IOLOCK_SHIFT + XFS_SHARED_LOCK_SHIFT))
> #define XFS_ILOCK_EXCL      (1 << (XFS_ILOCK_SHIFT))
> #define XFS_ILOCK_SHARED    (1 << (XFS_ILOCK_SHIFT + XFS_SHARED_LOCK_SHIFT))
> #define XFS_MMAPLOCK_EXCL   (1 << (XFS_MMAPLOCK_SHIFT))
> #define XFS_MMAPLOCK_SHARED (1 << (XFS_MMAPLOCK_SHIFT + XFS_SHARED_LOCK_SHIFT))

Thank you for the code - now I see what you meant and I like it,
however allow me a question:
Are you aware that XFS_IOLOCK_SHIFT, XFS_MMAPLOCK_SHIFT,
XFS_ILOCK_SHIFT are already defined with different values and used in
xfs_lock_inumorder()?

I have no trouble to investigate the code and see if it is OK i.g.
XFS_IOLOCK_EXCL to be 21 (I guess I should check that no bit arrays
are used to store the value, etc)

Or maybe I should just rewrite  the '#define XFS_IOLOCK_SHIFT
0' to something like '#define XFS_IOLOCK_TYPE_SHIFT        0' ?

Do you have any thoughts about that?

Thanks!


>
> Because then in the outer xfs_isilocked function you can do:
>
> if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED))
>         return __isilocked(&ip->i_lock, lock_flags >> XFS_ILOCK_SHIFT);
>
> if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED))
>         return __isilocked(&ip->i_mmaplock, lock_flags >> XFS_MMAPLOCK_SHIFT);
>
> if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED))
>         return __isilocked(&VFS_I(ip)->i_rwsem, lock_flags >> XFS_IOLOCK_SHIFT);
>
> And finally in __isilocked you can do:
>
> static inline bool __isilocked(rwsem, lock_flags)
> {
>         int     arg;
>
>         if (!debug_locks)
>                 return rwsem_is_locked(rwsem);
>
>         if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) {
>                 /*
>                  * The caller could be asking if we have (shared | excl)
>                  * access to the lock.  Ask lockdep if the rwsem is
>                  * locked either for read or write access.
>                  *
>                  * The caller could also be asking if we have only
>                  * shared access to the lock.  Holding a rwsem
>                  * write-locked implies read access as well, so the
>                  * request to lockdep is the same for this case.
>                  */
>                 arg = -1;
>         } else {
>                 /*
>                  * The caller is asking if we have only exclusive access
>                  * to the lock.  Ask lockdep if the rwsem is locked for
>                  * write access.
>                  */
>                 arg = 0;
>         }
>         return lockdep_is_held_type(rwsem, arg);
> }
>
> > >> "r" parameter value for the lockdep_is_held_type() call.  Then all of
> > >> this becomes:
> > >>
> > >>         if !debug_locks:
> > >>                 return rwsem_is_locked(rwsem)
> > >>
> > >>         if shared and excl:
> > >>                 r = -1
> > >>         elif shared:
> > >>                 r = 1
> > >>         else:
> > >>                 r = 0
> > >>         return lockdep_is_held_type(rwsem, r)
> > >
> > > I tried to create a table for this code as well:
> >
> > <adding back the table headers>
> >
> > > (nolockdep corresponds to debug_locks == 0)
> > >
> > > RWSEM STATE             PARAMETERS TO XFS_ISILOCKED:
> > >                         SHARED  EXCL    SHARED | EXCL
> > > readlocked              y       n       y
> > > writelocked             *n*     y       y
> > > unlocked                n       n       n
> > > nolockdep readlocked    y       y       y
> > > nolockdep writelocked   y       y       y
> > > nolockdep unlocked      n       n       n
> > >
> > > I think that when we query writelocked lock for being shared having
> > > 'no' for an answer may not be expected...or at least this is how I
> > > read the code.
> >
> > This might be ok, because
> > a) it is technically correct (is it shared? /no/ it is exclusive), and
> > b) in the XFS code today we never call:
> >
> >       xfs_isilocked(ip, XFS_ILOCK_SHARED);
> >
> > it's always:
> >
> >       xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL);
> >
> > So I think that if we document the behavior clearly, the truth table above
> > would be ok.
> >
> > Thoughts?
>
> No, Pavel's right, I got the pseudocode wrong, because holding a write
> lock means you also hold the read lock.
>
>         if !debug_locks:
>                 return rwsem_is_locked(rwsem)
>
>         if shared:
>                 r = -1
>         else:
>                 r = 0
>         return lockdep_is_held_type(rwsem, r)
>
> --D
>
> > -Eric
>

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

[Darrick J. Wong]

On Fri, Mar 20, 2020 at 03:41:08PM +0100, Pavel Reichl wrote:
> On Wed, Mar 18, 2020 at 7:50 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >
> > On Wed, Mar 18, 2020 at 12:46:37PM -0500, Eric Sandeen wrote:
> > > On 3/18/20 12:13 PM, Pavel Reichl wrote:
> > > > On Fri, Feb 28, 2020 at 6:10 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > >
> > > ...
> > >
> > > >> So, this function's call signature should change so that callers can
> > > >> communicate both _SHARED and _EXCL; and then you can pick the correct
> > > >
> > > > Thanks for the suggestion...but that's how v5 signature looked like
> > > > before Christoph and Eric requested change...on the grounds that
> > > > there're:
> > > > *  confusion over a (true, true) set of args
> > > > *  confusion of what happens if we pass (false, false).
> >
> > Yeah.  I don't mean adding back the dual booleans, I meant refactoring
> > the way we define the lock constants so that you can use bit shifting
> > and masking:
> >
> > #define XFS_IOLOCK_SHIFT        0
> > #define XFS_ILOCK_SHIFT         2
> > #define XFS_MMAPLOCK_SHIFT      4
> >
> > #define XFS_SHARED_LOCK_SHIFT   1
> >
> > #define XFS_IOLOCK_EXCL     (1 << (XFS_IOLOCK_SHIFT))
> > #define XFS_IOLOCK_SHARED   (1 << (XFS_IOLOCK_SHIFT + XFS_SHARED_LOCK_SHIFT))
> > #define XFS_ILOCK_EXCL      (1 << (XFS_ILOCK_SHIFT))
> > #define XFS_ILOCK_SHARED    (1 << (XFS_ILOCK_SHIFT + XFS_SHARED_LOCK_SHIFT))
> > #define XFS_MMAPLOCK_EXCL   (1 << (XFS_MMAPLOCK_SHIFT))
> > #define XFS_MMAPLOCK_SHARED (1 << (XFS_MMAPLOCK_SHIFT + XFS_SHARED_LOCK_SHIFT))
> 
> Thank you for the code - now I see what you meant and I like it,
> however allow me a question:
> Are you aware that XFS_IOLOCK_SHIFT, XFS_MMAPLOCK_SHIFT,
> XFS_ILOCK_SHIFT are already defined with different values and used in
> xfs_lock_inumorder()?
> 
> I have no trouble to investigate the code and see if it is OK i.g.
> XFS_IOLOCK_EXCL to be 21 (I guess I should check that no bit arrays
> are used to store the value, etc)
> 
> Or maybe I should just rewrite  the '#define XFS_IOLOCK_SHIFT
> 0' to something like '#define XFS_IOLOCK_TYPE_SHIFT        0' ?
> 
> Do you have any thoughts about that?

XFS_IOLOCK_TYPE_SHIFT seems fine to me to avoid clashing with lockdep. :)

(perhaps XFS_IOLOCK_FLAG_SHIFT?)

--D

> 
> Thanks!
> 
> 
> >
> > Because then in the outer xfs_isilocked function you can do:
> >
> > if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED))
> >         return __isilocked(&ip->i_lock, lock_flags >> XFS_ILOCK_SHIFT);
> >
> > if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED))
> >         return __isilocked(&ip->i_mmaplock, lock_flags >> XFS_MMAPLOCK_SHIFT);
> >
> > if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED))
> >         return __isilocked(&VFS_I(ip)->i_rwsem, lock_flags >> XFS_IOLOCK_SHIFT);
> >
> > And finally in __isilocked you can do:
> >
> > static inline bool __isilocked(rwsem, lock_flags)
> > {
> >         int     arg;
> >
> >         if (!debug_locks)
> >                 return rwsem_is_locked(rwsem);
> >
> >         if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) {
> >                 /*
> >                  * The caller could be asking if we have (shared | excl)
> >                  * access to the lock.  Ask lockdep if the rwsem is
> >                  * locked either for read or write access.
> >                  *
> >                  * The caller could also be asking if we have only
> >                  * shared access to the lock.  Holding a rwsem
> >                  * write-locked implies read access as well, so the
> >                  * request to lockdep is the same for this case.
> >                  */
> >                 arg = -1;
> >         } else {
> >                 /*
> >                  * The caller is asking if we have only exclusive access
> >                  * to the lock.  Ask lockdep if the rwsem is locked for
> >                  * write access.
> >                  */
> >                 arg = 0;
> >         }
> >         return lockdep_is_held_type(rwsem, arg);
> > }
> >
> > > >> "r" parameter value for the lockdep_is_held_type() call.  Then all of
> > > >> this becomes:
> > > >>
> > > >>         if !debug_locks:
> > > >>                 return rwsem_is_locked(rwsem)
> > > >>
> > > >>         if shared and excl:
> > > >>                 r = -1
> > > >>         elif shared:
> > > >>                 r = 1
> > > >>         else:
> > > >>                 r = 0
> > > >>         return lockdep_is_held_type(rwsem, r)
> > > >
> > > > I tried to create a table for this code as well:
> > >
> > > <adding back the table headers>
> > >
> > > > (nolockdep corresponds to debug_locks == 0)
> > > >
> > > > RWSEM STATE             PARAMETERS TO XFS_ISILOCKED:
> > > >                         SHARED  EXCL    SHARED | EXCL
> > > > readlocked              y       n       y
> > > > writelocked             *n*     y       y
> > > > unlocked                n       n       n
> > > > nolockdep readlocked    y       y       y
> > > > nolockdep writelocked   y       y       y
> > > > nolockdep unlocked      n       n       n
> > > >
> > > > I think that when we query writelocked lock for being shared having
> > > > 'no' for an answer may not be expected...or at least this is how I
> > > > read the code.
> > >
> > > This might be ok, because
> > > a) it is technically correct (is it shared? /no/ it is exclusive), and
> > > b) in the XFS code today we never call:
> > >
> > >       xfs_isilocked(ip, XFS_ILOCK_SHARED);
> > >
> > > it's always:
> > >
> > >       xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL);
> > >
> > > So I think that if we document the behavior clearly, the truth table above
> > > would be ok.
> > >
> > > Thoughts?
> >
> > No, Pavel's right, I got the pseudocode wrong, because holding a write
> > lock means you also hold the read lock.
> >
> >         if !debug_locks:
> >                 return rwsem_is_locked(rwsem)
> >
> >         if shared:
> >                 r = -1
> >         else:
> >                 r = 0
> >         return lockdep_is_held_type(rwsem, r)
> >
> > --D
> >
> > > -Eric
> >
>