[PATCH v2 0/7] xfs: Remove wrappers for some semaphores

[PATCH v2 0/7] 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 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 (7):
  xfs: Add xfs_is_{i,io,mmap}locked functions
  xfs: Update checking excl. locks for ilock
  xfs: Update checking read or write locks for ilock
  xfs: Update checking for iolock
  xfs: Update checking for mmaplock
  xfs: update excl. lock check for IOLOCK and ILOCK
  xfs: Replace mrlock_t by rw_semaphore

 fs/xfs/libxfs/xfs_attr.c        |   2 +-
 fs/xfs/libxfs/xfs_attr_remote.c |   2 +-
 fs/xfs/libxfs/xfs_bmap.c        |  22 +++---
 fs/xfs/libxfs/xfs_inode_fork.c  |   2 +-
 fs/xfs/libxfs/xfs_rtbitmap.c    |   2 +-
 fs/xfs/libxfs/xfs_trans_inode.c |   6 +-
 fs/xfs/mrlock.h                 |  78 ----------------------
 fs/xfs/xfs_attr_list.c          |   2 +-
 fs/xfs/xfs_bmap_util.c          |   8 +--
 fs/xfs/xfs_dquot.c              |   4 +-
 fs/xfs/xfs_file.c               |   5 +-
 fs/xfs/xfs_inode.c              | 114 ++++++++++++++++++++------------
 fs/xfs/xfs_inode.h              |  10 ++-
 fs/xfs/xfs_inode_item.c         |   4 +-
 fs/xfs/xfs_iops.c               |  12 ++--
 fs/xfs/xfs_linux.h              |   1 -
 fs/xfs/xfs_qm.c                 |  12 ++--
 fs/xfs/xfs_reflink.c            |   2 +-
 fs/xfs/xfs_rtalloc.c            |   4 +-
 fs/xfs/xfs_super.c              |   6 +-
 fs/xfs/xfs_symlink.c            |   2 +-
 fs/xfs/xfs_trans_dquot.c        |   2 +-
 22 files changed, 127 insertions(+), 175 deletions(-)
 delete mode 100644 fs/xfs/mrlock.h

-- 
2.24.1

[PATCH v2 1/7] xfs: Add xfs_is_{i,io,mmap}locked functions

[Pavel Reichl]

Add xfs_is_ilocked(), xfs_is_iolocked() and xfs_is_mmaplocked()

Signed-off-by: Pavel Reichl <preichl@redhat.com>
Suggested-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_inode.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_inode.h |  3 +++
 2 files changed, 56 insertions(+)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index c5077e6326c7..80874c80df6d 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -372,6 +372,59 @@ xfs_isilocked(
 	ASSERT(0);
 	return 0;
 }
+
+static inline bool
+__xfs_is_ilocked(
+	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_is_ilocked(
+	struct xfs_inode	*ip,
+	int			lock_flags)
+{
+	return __xfs_is_ilocked(&ip->i_lock.mr_lock,
+			(lock_flags & XFS_ILOCK_SHARED),
+			(lock_flags & XFS_ILOCK_EXCL));
+}
+
+bool
+xfs_is_mmaplocked(
+	struct xfs_inode	*ip,
+	int			lock_flags)
+{
+	return __xfs_is_ilocked(&ip->i_mmaplock.mr_lock,
+			(lock_flags & XFS_MMAPLOCK_SHARED),
+			(lock_flags & XFS_MMAPLOCK_EXCL));
+}
+
+bool
+xfs_is_iolocked(
+	struct xfs_inode	*ip,
+	int			lock_flags)
+{
+	return __xfs_is_ilocked(&VFS_I(ip)->i_rwsem,
+			(lock_flags & XFS_IOLOCK_SHARED),
+			(lock_flags & XFS_IOLOCK_EXCL));
+}
 #endif
 
 /*
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 492e53992fa9..6ba575f35c1f 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -417,6 +417,9 @@ 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_is_ilocked(struct xfs_inode *, int);
+bool		xfs_is_mmaplocked(struct xfs_inode *, int);
+bool		xfs_is_iolocked(struct xfs_inode *, int);
 uint		xfs_ilock_data_map_shared(struct xfs_inode *);
 uint		xfs_ilock_attr_map_shared(struct xfs_inode *);
 
-- 
2.24.1

[PATCH v2 2/7] xfs: Update checking excl. locks for ilock

[Pavel Reichl]

Signed-off-by: Pavel Reichl <preichl@redhat.com>
Suggested-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_attr_remote.c |  2 +-
 fs/xfs/libxfs/xfs_bmap.c        | 10 +++++-----
 fs/xfs/libxfs/xfs_rtbitmap.c    |  2 +-
 fs/xfs/libxfs/xfs_trans_inode.c |  6 +++---
 fs/xfs/xfs_dquot.c              |  4 ++--
 fs/xfs/xfs_inode.c              |  4 ++--
 fs/xfs/xfs_inode_item.c         |  4 ++--
 fs/xfs/xfs_iops.c               |  4 ++--
 fs/xfs/xfs_qm.c                 | 10 +++++-----
 fs/xfs/xfs_reflink.c            |  2 +-
 fs/xfs/xfs_rtalloc.c            |  4 ++--
 fs/xfs/xfs_trans_dquot.c        |  2 +-
 12 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index 8b7f74b3bea2..5607c5551095 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -577,7 +577,7 @@ xfs_attr_rmtval_stale(
 	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_buf		*bp;
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 
 	if (XFS_IS_CORRUPT(mp, map->br_startblock == DELAYSTARTBLOCK) ||
 	    XFS_IS_CORRUPT(mp, map->br_startblock == HOLESTARTBLOCK))
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 9a6d7a84689a..318c006b4b50 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -1238,7 +1238,7 @@ xfs_iread_extents(
 	struct xfs_btree_cur	*cur;
 	int			error;
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 
 	if (XFS_IS_CORRUPT(mp,
 			   XFS_IFORK_FORMAT(ip, whichfork) !=
@@ -4402,7 +4402,7 @@ xfs_bmapi_write(
 	ASSERT(tp != NULL);
 	ASSERT(len > 0);
 	ASSERT(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL);
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT(!(flags & XFS_BMAPI_REMAP));
 
 	/* zeroing is for currently only for data extents, not metadata */
@@ -4678,7 +4678,7 @@ xfs_bmapi_remap(
 	ifp = XFS_IFORK_PTR(ip, whichfork);
 	ASSERT(len > 0);
 	ASSERT(len <= (xfs_filblks_t)MAXEXTLEN);
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_PREALLOC |
 			   XFS_BMAPI_NORMAP)));
 	ASSERT((flags & (XFS_BMAPI_ATTRFORK | XFS_BMAPI_PREALLOC)) !=
@@ -5329,7 +5329,7 @@ __xfs_bunmapi(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -EIO;
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT(len > 0);
 	ASSERT(nexts >= 0);
 
@@ -5700,7 +5700,7 @@ xfs_bmse_merge(
 	blockcount = left->br_blockcount + got->br_blockcount;
 
 	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT(xfs_bmse_can_merge(left, got, shift));
 
 	new = *left;
diff --git a/fs/xfs/libxfs/xfs_rtbitmap.c b/fs/xfs/libxfs/xfs_rtbitmap.c
index f42c74cb8be5..4ba9dea9bdf0 100644
--- a/fs/xfs/libxfs/xfs_rtbitmap.c
+++ b/fs/xfs/libxfs/xfs_rtbitmap.c
@@ -974,7 +974,7 @@ xfs_rtfree_extent(
 	mp = tp->t_mountp;
 
 	ASSERT(mp->m_rbmip->i_itemp != NULL);
-	ASSERT(xfs_isilocked(mp->m_rbmip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(mp->m_rbmip, XFS_ILOCK_EXCL));
 
 	error = xfs_rtcheck_alloc_range(mp, tp, bno, len);
 	if (error)
diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
index 2b8ccb5b975d..bac474d97574 100644
--- a/fs/xfs/libxfs/xfs_trans_inode.c
+++ b/fs/xfs/libxfs/xfs_trans_inode.c
@@ -29,7 +29,7 @@ xfs_trans_ijoin(
 {
 	xfs_inode_log_item_t	*iip;
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 	if (ip->i_itemp == NULL)
 		xfs_inode_item_init(ip, ip->i_mount);
 	iip = ip->i_itemp;
@@ -58,7 +58,7 @@ xfs_trans_ichgtime(
 	struct timespec64	tv;
 
 	ASSERT(tp);
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 
 	tv = current_time(inode);
 
@@ -88,7 +88,7 @@ xfs_trans_log_inode(
 	struct inode	*inode = VFS_I(ip);
 
 	ASSERT(ip->i_itemp != NULL);
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 
 	/*
 	 * Don't bother with i_lock for the I_DIRTY_TIME check here, as races
diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
index d223e1ae90a6..74d9d00d45ef 100644
--- a/fs/xfs/xfs_dquot.c
+++ b/fs/xfs/xfs_dquot.c
@@ -862,7 +862,7 @@ xfs_qm_dqget_inode(
 	if (error)
 		return error;
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT(xfs_inode_dquot(ip, type) == NULL);
 
 	id = xfs_qm_id_for_quotatype(ip, type);
@@ -919,7 +919,7 @@ xfs_qm_dqget_inode(
 	}
 
 dqret:
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 	trace_xfs_dqget_miss(dqp);
 	*O_dqpp = dqp;
 	return 0;
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 80874c80df6d..a19c6ddaea6f 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1574,7 +1574,7 @@ xfs_itruncate_extents_flags(
 	xfs_filblks_t		unmap_len;
 	int			error = 0;
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT(!atomic_read(&VFS_I(ip)->i_count) ||
 	       xfs_isilocked(ip, XFS_IOLOCK_EXCL));
 	ASSERT(new_size <= XFS_ISIZE(ip));
@@ -2805,7 +2805,7 @@ xfs_ifree(
 	int			error;
 	struct xfs_icluster	xic = { 0 };
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT(VFS_I(ip)->i_nlink == 0);
 	ASSERT(ip->i_d.di_nextents == 0);
 	ASSERT(ip->i_d.di_anextents == 0);
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 8bd5d0de6321..6396d7b2038c 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -440,7 +440,7 @@ xfs_inode_item_pin(
 {
 	struct xfs_inode	*ip = INODE_ITEM(lip)->ili_inode;
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 
 	trace_xfs_inode_pin(ip, _RET_IP_);
 	atomic_inc(&ip->i_pincount);
@@ -574,7 +574,7 @@ xfs_inode_item_release(
 	unsigned short		lock_flags;
 
 	ASSERT(ip->i_itemp != NULL);
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 
 	lock_flags = iip->ili_lock_flags;
 	iip->ili_lock_flags = 0;
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 81f2f93caec0..eba2ec2a59f1 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -598,7 +598,7 @@ xfs_setattr_mode(
 	struct inode		*inode = VFS_I(ip);
 	umode_t			mode = iattr->ia_mode;
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 
 	inode->i_mode &= S_IFMT;
 	inode->i_mode |= mode & ~S_IFMT;
@@ -611,7 +611,7 @@ xfs_setattr_time(
 {
 	struct inode		*inode = VFS_I(ip);
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 
 	if (iattr->ia_valid & ATTR_ATIME)
 		inode->i_atime = iattr->ia_atime;
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 0b0909657bad..dfe155cbaa55 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -253,7 +253,7 @@ xfs_qm_dqattach_one(
 	struct xfs_dquot	*dqp;
 	int			error;
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 	error = 0;
 
 	/*
@@ -323,7 +323,7 @@ xfs_qm_dqattach_locked(
 	if (!xfs_qm_need_dqattach(ip))
 		return 0;
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 
 	if (XFS_IS_UQUOTA_ON(mp) && !ip->i_udquot) {
 		error = xfs_qm_dqattach_one(ip, ip->i_d.di_uid, XFS_DQ_USER,
@@ -354,7 +354,7 @@ xfs_qm_dqattach_locked(
 	 * Don't worry about the dquots that we may have attached before any
 	 * error - they'll get detached later if it has not already been done.
 	 */
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 	return error;
 }
 
@@ -1754,7 +1754,7 @@ xfs_qm_vop_chown(
 				 XFS_TRANS_DQ_RTBCOUNT : XFS_TRANS_DQ_BCOUNT;
 
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT(XFS_IS_QUOTA_RUNNING(ip->i_mount));
 
 	/* old dquot */
@@ -1915,7 +1915,7 @@ xfs_qm_vop_create_dqattach(
 	if (!XFS_IS_QUOTA_RUNNING(mp) || !XFS_IS_QUOTA_ON(mp))
 		return;
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
 
 	if (udqp && XFS_IS_UQUOTA_ON(mp)) {
diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index b0ce04ffd3cd..1d570ad1cfc9 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -359,7 +359,7 @@ xfs_reflink_allocate_cow(
 	xfs_filblks_t		resaligned;
 	xfs_extlen_t		resblks = 0;
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 	if (!ip->i_cowfp) {
 		ASSERT(!xfs_is_reflink_inode(ip));
 		xfs_ifork_init_cow(ip);
diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 6209e7b6b895..35906b3c2825 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -1121,7 +1121,7 @@ xfs_rtallocate_extent(
 	xfs_fsblock_t	sb;		/* summary file block number */
 	xfs_buf_t	*sumbp;		/* summary file block buffer */
 
-	ASSERT(xfs_isilocked(mp->m_rbmip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(mp->m_rbmip, XFS_ILOCK_EXCL));
 	ASSERT(minlen > 0 && minlen <= maxlen);
 
 	/*
@@ -1278,7 +1278,7 @@ xfs_rtpick_extent(
 	uint64_t	seq;		/* sequence number of file creation */
 	uint64_t	*seqp;		/* pointer to seqno in inode */
 
-	ASSERT(xfs_isilocked(mp->m_rbmip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(mp->m_rbmip, XFS_ILOCK_EXCL));
 
 	seqp = (uint64_t *)&VFS_I(mp->m_rbmip)->i_atime;
 	if (!(mp->m_rbmip->i_d.di_flags & XFS_DIFLAG_NEWRTBM)) {
diff --git a/fs/xfs/xfs_trans_dquot.c b/fs/xfs/xfs_trans_dquot.c
index d1b9869bc5fa..ed3a78e6a295 100644
--- a/fs/xfs/xfs_trans_dquot.c
+++ b/fs/xfs/xfs_trans_dquot.c
@@ -808,7 +808,7 @@ xfs_trans_reserve_quota_nblks(
 
 	ASSERT(!xfs_is_quota_inode(&mp->m_sb, ip->i_ino));
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT((flags & ~(XFS_QMOPT_FORCE_RES | XFS_QMOPT_ENOSPC)) ==
 				XFS_TRANS_DQ_RES_RTBLKS ||
 	       (flags & ~(XFS_QMOPT_FORCE_RES | XFS_QMOPT_ENOSPC)) ==
-- 
2.24.1

[PATCH v2 3/7] xfs: Update checking read or write locks for ilock

[Pavel Reichl]

Signed-off-by: Pavel Reichl <preichl@redhat.com>
Suggested-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_attr.c       | 2 +-
 fs/xfs/libxfs/xfs_bmap.c       | 2 +-
 fs/xfs/libxfs/xfs_inode_fork.c | 2 +-
 fs/xfs/xfs_attr_list.c         | 2 +-
 fs/xfs/xfs_inode.c             | 6 +++---
 fs/xfs/xfs_qm.c                | 2 +-
 fs/xfs/xfs_symlink.c           | 2 +-
 7 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index e6149720ce02..e692225d2e64 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -107,7 +107,7 @@ xfs_attr_get_ilocked(
 	struct xfs_inode	*ip,
 	struct xfs_da_args	*args)
 {
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
 
 	if (!xfs_inode_hasattr(ip))
 		return -ENOATTR;
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 318c006b4b50..86a9fe2a7629 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_is_ilocked(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/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index ad2b9c313fd2..ffb5731a73da 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -560,7 +560,7 @@ xfs_iextents_copy(
 	struct xfs_bmbt_irec	rec;
 	int64_t			copied = 0;
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
 	ASSERT(ifp->if_bytes > 0);
 
 	for_each_xfs_iext(ifp, &icur, &rec) {
diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
index d37743bdf274..a52539a0fd63 100644
--- a/fs/xfs/xfs_attr_list.c
+++ b/fs/xfs/xfs_attr_list.c
@@ -511,7 +511,7 @@ xfs_attr_list_int_ilocked(
 {
 	struct xfs_inode		*dp = context->dp;
 
-	ASSERT(xfs_isilocked(dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(dp, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
 
 	/*
 	 * Decide on what work routines to call based on the inode size.
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index a19c6ddaea6f..d7cb2886ca81 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2859,7 +2859,7 @@ static void
 xfs_iunpin(
 	struct xfs_inode	*ip)
 {
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
 
 	trace_xfs_inode_unpin_nowait(ip, _RET_IP_);
 
@@ -3723,7 +3723,7 @@ xfs_iflush(
 
 	XFS_STATS_INC(mp, xs_iflush_count);
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
 	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));
@@ -3855,7 +3855,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_is_ilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
 	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 dfe155cbaa55..757c8cc00e39 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_is_ilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
 	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
 
 	delblks = ip->i_delayed_blks;
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index d762d42ed0ff..20179f173688 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -41,7 +41,7 @@ xfs_readlink_bmap_ilocked(
 	int			fsblocks = 0;
 	int			offset;
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
+	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
 
 	fsblocks = xfs_symlink_blocks(mp, pathlen);
 	error = xfs_bmapi_read(ip, 0, fsblocks, mval, &nmaps, 0);
-- 
2.24.1

[PATCH v2 4/7] xfs: Update checking for iolock

[Pavel Reichl]

Signed-off-by: Pavel Reichl <preichl@redhat.com>
Suggested-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/libxfs/xfs_bmap.c | 4 ++--
 fs/xfs/xfs_bmap_util.c   | 4 ++--
 fs/xfs/xfs_file.c        | 3 ++-
 fs/xfs/xfs_inode.c       | 2 +-
 fs/xfs/xfs_iops.c        | 2 +-
 5 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 86a9fe2a7629..c3638552b3c0 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5699,7 +5699,7 @@ xfs_bmse_merge(
 
 	blockcount = left->br_blockcount + got->br_blockcount;
 
-	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
+	ASSERT(xfs_is_iolocked(ip, XFS_IOLOCK_EXCL));
 	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT(xfs_bmse_can_merge(left, got, shift));
 
@@ -5904,7 +5904,7 @@ xfs_bmap_can_insert_extents(
 	int			is_empty;
 	int			error = 0;
 
-	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
+	ASSERT(xfs_is_iolocked(ip, XFS_IOLOCK_EXCL));
 
 	if (XFS_FORCED_SHUTDOWN(ip->i_mount))
 		return -EIO;
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index e62fb5216341..ae0bccb2288f 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1065,7 +1065,7 @@ xfs_collapse_file_space(
 	uint			resblks = XFS_DIOSTRAT_SPACE_RES(mp, 0);
 	bool			done = false;
 
-	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
+	ASSERT(xfs_is_iolocked(ip, XFS_IOLOCK_EXCL));
 	ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
 
 	trace_xfs_collapse_file_space(ip);
@@ -1133,7 +1133,7 @@ xfs_insert_file_space(
 	xfs_fileoff_t		shift_fsb = XFS_B_TO_FSB(mp, len);
 	bool			done = false;
 
-	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
+	ASSERT(xfs_is_iolocked(ip, XFS_IOLOCK_EXCL));
 	ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
 
 	trace_xfs_insert_file_space(ip);
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index b8a4a3f29b36..9b3958ca73d9 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_is_iolocked(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 d7cb2886ca81..328a3b4ffbd2 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1576,7 +1576,7 @@ xfs_itruncate_extents_flags(
 
 	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 	ASSERT(!atomic_read(&VFS_I(ip)->i_count) ||
-	       xfs_isilocked(ip, XFS_IOLOCK_EXCL));
+	       xfs_is_iolocked(ip, XFS_IOLOCK_EXCL));
 	ASSERT(new_size <= XFS_ISIZE(ip));
 	ASSERT(tp->t_flags & XFS_TRANS_PERM_LOG_RES);
 	ASSERT(ip->i_itemp != NULL);
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index eba2ec2a59f1..aad255521514 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -865,7 +865,7 @@ xfs_setattr_size(
 	uint			lock_flags = 0;
 	bool			did_zeroing = false;
 
-	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
+	ASSERT(xfs_is_iolocked(ip, XFS_IOLOCK_EXCL));
 	ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
 	ASSERT(S_ISREG(inode->i_mode));
 	ASSERT((iattr->ia_valid & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
-- 
2.24.1

[PATCH v2 5/7] xfs: Update checking for mmaplock

[Pavel Reichl]

Signed-off-by: Pavel Reichl <preichl@redhat.com>
Suggested-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_bmap_util.c | 4 ++--
 fs/xfs/xfs_file.c      | 2 +-
 fs/xfs/xfs_iops.c      | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index ae0bccb2288f..377389fadf5a 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1066,7 +1066,7 @@ xfs_collapse_file_space(
 	bool			done = false;
 
 	ASSERT(xfs_is_iolocked(ip, XFS_IOLOCK_EXCL));
-	ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
+	ASSERT(xfs_is_mmaplocked(ip, XFS_MMAPLOCK_EXCL));
 
 	trace_xfs_collapse_file_space(ip);
 
@@ -1134,7 +1134,7 @@ xfs_insert_file_space(
 	bool			done = false;
 
 	ASSERT(xfs_is_iolocked(ip, XFS_IOLOCK_EXCL));
-	ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
+	ASSERT(xfs_is_mmaplocked(ip, XFS_MMAPLOCK_EXCL));
 
 	trace_xfs_insert_file_space(ip);
 
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 9b3958ca73d9..a4dbd9a6f45a 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -749,7 +749,7 @@ xfs_break_dax_layouts(
 {
 	struct page		*page;
 
-	ASSERT(xfs_isilocked(XFS_I(inode), XFS_MMAPLOCK_EXCL));
+	ASSERT(xfs_is_mmaplocked(XFS_I(inode), XFS_MMAPLOCK_EXCL));
 
 	page = dax_layout_busy_page(inode->i_mapping);
 	if (!page)
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index aad255521514..67a0f940b30e 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -866,7 +866,7 @@ xfs_setattr_size(
 	bool			did_zeroing = false;
 
 	ASSERT(xfs_is_iolocked(ip, XFS_IOLOCK_EXCL));
-	ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
+	ASSERT(xfs_is_mmaplocked(ip, XFS_MMAPLOCK_EXCL));
 	ASSERT(S_ISREG(inode->i_mode));
 	ASSERT((iattr->ia_valid & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
 		ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
-- 
2.24.1

[PATCH v2 6/7] xfs: update excl. lock check for IOLOCK and ILOCK

[Pavel Reichl]

Signed-off-by: Pavel Reichl <preichl@redhat.com>
Suggested-by: Dave Chinner <dchinner@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 c3638552b3c0..2d371f87e890 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_is_iolocked(ip, XFS_IOLOCK_EXCL) ||
+		xfs_is_ilocked(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_is_iolocked(ip, XFS_IOLOCK_EXCL) ||
+		xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
 
 	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
 		error = xfs_iread_extents(tp, ip, whichfork);
-- 
2.24.1

[PATCH v2 7/7] xfs: Replace mrlock_t by rw_semaphore

[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>
Suggested-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/mrlock.h    | 78 ----------------------------------------------
 fs/xfs/xfs_inode.c | 65 +++++++++++---------------------------
 fs/xfs/xfs_inode.h |  7 +++--
 fs/xfs/xfs_iops.c  |  4 +--
 fs/xfs/xfs_linux.h |  1 -
 fs/xfs/xfs_super.c |  6 ++--
 6 files changed, 27 insertions(+), 134 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 328a3b4ffbd2..ca54e2e151c0 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);
 
@@ -345,34 +346,6 @@ xfs_ilock_demote(
 }
 
 #if defined(DEBUG) || defined(XFS_WARN)
-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 (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_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);
-	}
-
-	ASSERT(0);
-	return 0;
-}
-
 static inline bool
 __xfs_is_ilocked(
 	struct rw_semaphore	*rwsem,
@@ -401,7 +374,7 @@ xfs_is_ilocked(
 	struct xfs_inode	*ip,
 	int			lock_flags)
 {
-	return __xfs_is_ilocked(&ip->i_lock.mr_lock,
+	return __xfs_is_ilocked(&ip->i_lock,
 			(lock_flags & XFS_ILOCK_SHARED),
 			(lock_flags & XFS_ILOCK_EXCL));
 }
@@ -411,7 +384,7 @@ xfs_is_mmaplocked(
 	struct xfs_inode	*ip,
 	int			lock_flags)
 {
-	return __xfs_is_ilocked(&ip->i_mmaplock.mr_lock,
+	return __xfs_is_ilocked(&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 6ba575f35c1f..d7c77d1d7930 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -9,6 +9,8 @@
 #include "xfs_inode_buf.h"
 #include "xfs_inode_fork.h"
 
+#include <linux/rwsem.h>
+
 /*
  * Kernel only inode definitions
  */
@@ -39,8 +41,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 */
 
 	/*
@@ -416,7 +418,6 @@ 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_is_ilocked(struct xfs_inode *, int);
 bool		xfs_is_mmaplocked(struct xfs_inode *, int);
 bool		xfs_is_iolocked(struct xfs_inode *, int);
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 67a0f940b30e..bb3a2a57f04a 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..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

Re: [PATCH v2 1/7] xfs: Add xfs_is_{i,io,mmap}locked functions

[Chaitanya Kulkarni]

Hi Pavel,

Thanks for the patch. Please see inline-comment.
On 02/03/2020 09:59 AM, Pavel Reichl wrote:
> Add xfs_is_ilocked(), xfs_is_iolocked() and xfs_is_mmaplocked()
>
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>
> ---
>   fs/xfs/xfs_inode.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++
>   fs/xfs/xfs_inode.h |  3 +++
>   2 files changed, 56 insertions(+)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c5077e6326c7..80874c80df6d 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -372,6 +372,59 @@ xfs_isilocked(
>   	ASSERT(0);
>   	return 0;
>   }
> +
> +static inline bool
> +__xfs_is_ilocked(
> +	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_is_ilocked(
> +	struct xfs_inode	*ip,
> +	int			lock_flags)
nit:- isn't above lock_flag variable should be uint ?
Is there a particular reason that it has int type ?
> +{
> +	return __xfs_is_ilocked(&ip->i_lock.mr_lock,
> +			(lock_flags & XFS_ILOCK_SHARED),
> +			(lock_flags & XFS_ILOCK_EXCL));
> +}
> +
> +bool
> +xfs_is_mmaplocked(
> +	struct xfs_inode	*ip,
> +	int			lock_flags)
Same here.
> +{
> +	return __xfs_is_ilocked(&ip->i_mmaplock.mr_lock,
> +			(lock_flags & XFS_MMAPLOCK_SHARED),
> +			(lock_flags & XFS_MMAPLOCK_EXCL));
> +}
> +
> +bool
> +xfs_is_iolocked(
> +	struct xfs_inode	*ip,
> +	int			lock_flags)
Same here.
> +{
> +	return __xfs_is_ilocked(&VFS_I(ip)->i_rwsem,
> +			(lock_flags & XFS_IOLOCK_SHARED),
> +			(lock_flags & XFS_IOLOCK_EXCL));
> +}
>   #endif
>
>   /*
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 492e53992fa9..6ba575f35c1f 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -417,6 +417,9 @@ 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_is_ilocked(struct xfs_inode *, int);
> +bool		xfs_is_mmaplocked(struct xfs_inode *, int);
> +bool		xfs_is_iolocked(struct xfs_inode *, int);
>   uint		xfs_ilock_data_map_shared(struct xfs_inode *);
>   uint		xfs_ilock_attr_map_shared(struct xfs_inode *);
>
>

Re: [PATCH v2 1/7] xfs: Add xfs_is_{i,io,mmap}locked functions

[Dave Chinner]

On Mon, Feb 03, 2020 at 06:58:44PM +0100, Pavel Reichl wrote:
> Add xfs_is_ilocked(), xfs_is_iolocked() and xfs_is_mmaplocked()

The commit description is supposed to explain "Why?" rather than
describe what the code does.

So why are we adding these interfaces?

> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_inode.h |  3 +++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c5077e6326c7..80874c80df6d 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -372,6 +372,59 @@ xfs_isilocked(
>  	ASSERT(0);
>  	return 0;
>  }
> +
> +static inline bool
> +__xfs_is_ilocked(
> +	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 needs a comment explaining the reason why it is structured this
way. I can see quite clearly what it is doing, but why it is done
this way is not immediately apparent from the code.

In a few months, I'm not going to remember the reasons for this
code, and if the neither the code nor the commit description
explains the reasons why the code is like this, then it's really
quite difficult and time consuming to try to discover the reason for
the code being this way.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 2/7] xfs: Update checking excl. locks for ilock

[Dave Chinner]

On Mon, Feb 03, 2020 at 06:58:45PM +0100, Pavel Reichl wrote:
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_attr_remote.c |  2 +-
>  fs/xfs/libxfs/xfs_bmap.c        | 10 +++++-----
>  fs/xfs/libxfs/xfs_rtbitmap.c    |  2 +-
>  fs/xfs/libxfs/xfs_trans_inode.c |  6 +++---
>  fs/xfs/xfs_dquot.c              |  4 ++--
>  fs/xfs/xfs_inode.c              |  4 ++--
>  fs/xfs/xfs_inode_item.c         |  4 ++--
>  fs/xfs/xfs_iops.c               |  4 ++--
>  fs/xfs/xfs_qm.c                 | 10 +++++-----
>  fs/xfs/xfs_reflink.c            |  2 +-
>  fs/xfs/xfs_rtalloc.c            |  4 ++--
>  fs/xfs/xfs_trans_dquot.c        |  2 +-
>  12 files changed, 27 insertions(+), 27 deletions(-)

Looks fine, though you could combine this with the next patch.

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

Re: [PATCH v2 3/7] xfs: Update checking read or write locks for ilock

[Dave Chinner]

On Mon, Feb 03, 2020 at 06:58:46PM +0100, Pavel Reichl wrote:
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>

BTW, for these aPI conversions, you don't really need a suggested-by
tag on them. The first patch taht introduces the API, yes, but the
mechanical conversions don't really need it.

Otherwise, looks fine.

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

Re: [PATCH v2 4/7] xfs: Update checking for iolock

[Dave Chinner]

On Mon, Feb 03, 2020 at 06:58:47PM +0100, Pavel Reichl wrote:
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_bmap.c | 4 ++--
>  fs/xfs/xfs_bmap_util.c   | 4 ++--
>  fs/xfs/xfs_file.c        | 3 ++-
>  fs/xfs/xfs_inode.c       | 2 +-
>  fs/xfs/xfs_iops.c        | 2 +-
>  5 files changed, 8 insertions(+), 7 deletions(-)

....

> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index b8a4a3f29b36..9b3958ca73d9 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_is_iolocked(XFS_I(inode),
> +		XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL));

Whitespace: XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL

Otherwise looks ok.

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

Re: [PATCH v2 5/7] xfs: Update checking for mmaplock

[Dave Chinner]

On Mon, Feb 03, 2020 at 06:58:48PM +0100, Pavel Reichl wrote:
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_bmap_util.c | 4 ++--
>  fs/xfs/xfs_file.c      | 2 +-
>  fs/xfs/xfs_iops.c      | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)

looks fine.

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

Re: [PATCH v2 6/7] xfs: update excl. lock check for IOLOCK and ILOCK

[Dave Chinner]

On Mon, Feb 03, 2020 at 06:58:49PM +0100, Pavel Reichl wrote:
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> Suggested-by: Dave Chinner <dchinner@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 c3638552b3c0..2d371f87e890 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_is_iolocked(ip, XFS_IOLOCK_EXCL) ||
> +		xfs_is_ilocked(ip, XFS_ILOCK_EXCL));

Hmmm. I think this is incorrect - a bug in the original code in that
xfs_isilocked() will only check one lock type and so this never
worked as intended.

That is, we should have both the IOLOCK and the ILOCK held here.
The IOLOCK is taken by the high level xfs_file_fallocate() code to
lock out IO, while ILOCK is taken cwinside the transaction to make
the metadata changes atomic.

Hence I think this should actually end up as:

	ASSERT(xfs_is_iolocked(ip, XFS_IOLOCK_EXCL));
	ASSERT(xfs_is_ilocked(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_is_iolocked(ip, XFS_IOLOCK_EXCL) ||
> +		xfs_is_ilocked(ip, XFS_ILOCK_EXCL));

Same here.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 1/7] xfs: Add xfs_is_{i,io,mmap}locked functions

[Eric Sandeen]

On 2/3/20 11:58 AM, Pavel Reichl wrote:
> Add xfs_is_ilocked(), xfs_is_iolocked() and xfs_is_mmaplocked()
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_inode.h |  3 +++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c5077e6326c7..80874c80df6d 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -372,6 +372,59 @@ xfs_isilocked(
>  	ASSERT(0);
>  	return 0;
>  }
> +
> +static inline bool
> +__xfs_is_ilocked(
> +	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_is_ilocked(
> +	struct xfs_inode	*ip,
> +	int			lock_flags)
> +{
> +	return __xfs_is_ilocked(&ip->i_lock.mr_lock,
> +			(lock_flags & XFS_ILOCK_SHARED),
> +			(lock_flags & XFS_ILOCK_EXCL));
> +}

Apologies for not following or chiming in sooner on the prior discussion.

It seems a little odd that we must specify the lock type to test to
each of these 3 functions which are already named after a specific lock type.

i.e.:

xfs_is_mmaplocked(XFS_MMAPLOCK_EXCL);

seems a little redundant, but more problematic:

xfs_is_mmaplocked(XFS_ILOCK_SHARED);

would be accepted and silently return false.  I think that at least an
ASSERT() in each of the 3 functions to be sure we didn't pass in nonsense
flags would be wise.

-Eric

Re: [PATCH v2 1/7] xfs: Add xfs_is_{i,io,mmap}locked functions

[Christoph Hellwig]

> +static inline bool
> +__xfs_is_ilocked(
> +	struct rw_semaphore	*rwsem,
> +	bool			shared,
> +	bool			excl)

This calling conventions seems odd.  In other places like
lockdep we just have a bool excl.  This means we might get a false
positive when the lock is held exclusive but only shared is asserted,
but given that the low-level helpers can't give better information
that isn't going to hurt.

Also I'd name this function xfs_rwsem_is_locked, as there is nothing
inode specific here.

I also agree that this function needs good comments explaining the
rationale.

> +bool
> +xfs_is_ilocked(
> +	struct xfs_inode	*ip,
> +	int			lock_flags)
> +{
> +	return __xfs_is_ilocked(&ip->i_lock.mr_lock,
> +			(lock_flags & XFS_ILOCK_SHARED),
> +			(lock_flags & XFS_ILOCK_EXCL));
> +}
> +
> +bool
> +xfs_is_mmaplocked(
> +	struct xfs_inode	*ip,
> +	int			lock_flags)
> +{
> +	return __xfs_is_ilocked(&ip->i_mmaplock.mr_lock,
> +			(lock_flags & XFS_MMAPLOCK_SHARED),
> +			(lock_flags & XFS_MMAPLOCK_EXCL));
> +}
> +
> +bool
> +xfs_is_iolocked(
> +	struct xfs_inode	*ip,
> +	int			lock_flags)
> +{
> +	return __xfs_is_ilocked(&VFS_I(ip)->i_rwsem,
> +			(lock_flags & XFS_IOLOCK_SHARED),
> +			(lock_flags & XFS_IOLOCK_EXCL));
> +}
>  #endif

What is the benefit of these helpers?  xfs_isilocked can just
call __xfs_is_ilocked / xfs_rwsem_is_locked directly.

Re: [PATCH v2 2/7] xfs: Update checking excl. locks for ilock

[Christoph Hellwig]

> -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> +	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));

I think this is a very bad interface.  Either we keep our good old
xfs_isilocked that just operates on the inode and lock flags, or
we use something that gets the actual lock passed.  But an interface
that encodes the lock in both the function called and the flags, and
one that doesn't follow neither the XFS lock flags conventions nor
the core kernel convention is just not very useful.

Re: [PATCH v2 2/7] xfs: Update checking excl. locks for ilock

[Eric Sandeen]

On 2/4/20 12:21 AM, Christoph Hellwig wrote:
>> -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>> +	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
> 
> I think this is a very bad interface.  Either we keep our good old
> xfs_isilocked that just operates on the inode and lock flags, or
> we use something that gets the actual lock passed.  But an interface
> that encodes the lock in both the function called and the flags, and
> one that doesn't follow neither the XFS lock flags conventions nor
> the core kernel convention is just not very useful.

I think this came out of Dave's suggestion on the previous patchset,
but I agree with you Chrisoph.  Even if there is a future reason to
split it out into a function for each type, I don't see a reason to
do it now, and this interface is awkward.

I'd prefer to keep xfs_isilocked() with the current calling convention and
just change its internals to use lockdep.  Dave spotted a bug in the
current implementation, but I think that can be fixed.

Splitting out the 3 lock testing functions seems to me like complexity
creep that doesn't need to be in this series.

Dave, thoughts?

-Eric

Re: [PATCH v2 2/7] xfs: Update checking excl. locks for ilock

[Dave Chinner]

On Tue, Feb 04, 2020 at 10:08:45AM -0600, Eric Sandeen wrote:
> On 2/4/20 12:21 AM, Christoph Hellwig wrote:
> >> -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> >> +	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
> > 
> > I think this is a very bad interface.  Either we keep our good old
> > xfs_isilocked that just operates on the inode and lock flags, or
> > we use something that gets the actual lock passed.  But an interface
> > that encodes the lock in both the function called and the flags, and
> > one that doesn't follow neither the XFS lock flags conventions nor
> > the core kernel convention is just not very useful.
> 
> I think this came out of Dave's suggestion on the previous patchset,
> but I agree with you Chrisoph.  Even if there is a future reason to
> split it out into a function for each type, I don't see a reason to
> do it now, and this interface is awkward.
> 
> I'd prefer to keep xfs_isilocked() with the current calling convention and
> just change its internals to use lockdep.  Dave spotted a bug in the
> current implementation, but I think that can be fixed.
> 
> Splitting out the 3 lock testing functions seems to me like complexity
> creep that doesn't need to be in this series.
> 
> Dave, thoughts?

All I care about is that we get rid of the mrlock_t. I'm not
interested in bikeshedding the details to death. I've put my 2c
worth in, if you don't like it, then that's fine and I'm not going
to get upset about that.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 2/7] xfs: Update checking excl. locks for ilock

[Eric Sandeen]

On 2/4/20 3:06 PM, Dave Chinner wrote:
> On Tue, Feb 04, 2020 at 10:08:45AM -0600, Eric Sandeen wrote:
>> On 2/4/20 12:21 AM, Christoph Hellwig wrote:
>>>> -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>>>> +	ASSERT(xfs_is_ilocked(ip, XFS_ILOCK_EXCL));
>>>
>>> I think this is a very bad interface.  Either we keep our good old
>>> xfs_isilocked that just operates on the inode and lock flags, or
>>> we use something that gets the actual lock passed.  But an interface
>>> that encodes the lock in both the function called and the flags, and
>>> one that doesn't follow neither the XFS lock flags conventions nor
>>> the core kernel convention is just not very useful.
>>
>> I think this came out of Dave's suggestion on the previous patchset,
>> but I agree with you Chrisoph.  Even if there is a future reason to
>> split it out into a function for each type, I don't see a reason to
>> do it now, and this interface is awkward.
>>
>> I'd prefer to keep xfs_isilocked() with the current calling convention and
>> just change its internals to use lockdep.  Dave spotted a bug in the
>> current implementation, but I think that can be fixed.
>>
>> Splitting out the 3 lock testing functions seems to me like complexity
>> creep that doesn't need to be in this series.
>>
>> Dave, thoughts?
> 
> All I care about is that we get rid of the mrlock_t. I'm not
> interested in bikeshedding the details to death. I've put my 2c
> worth in, if you don't like it, then that's fine and I'm not going
> to get upset about that.

In the short term I'd suggest something like this. It keeps the helper,
but we don't have to change the callsites, and breaking out separate types
etc can be done after the mrlock removal patchset is done - in a separate
series if/when needed.

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(
        struct xfs_inode        *ip,
        uint                    lock_flags)
{       
        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)) {
                return __xfs_rwsem_islocked(&ip->i_mmlock.mr_lock,
                                (lock_flags & XFS_MMAPLOCK_SHARED),
                                (lock_flags & XFS_MMAPLOCK_EXCL));
        }
        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));
        }
}

That still has the problem/bug with the existing

	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));

callsite since it doesn't actually test both types but again that is a separate
bugfix - it could be changed to accumulate a true/false state for all the flags
in the lock_flags argument in another bugfix patch.

-Eric