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

[PATCH 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 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: change xfs_isilocked() to always use lockdep()
  xfs: Remove mr_writer field from mrlock_t
  xfs: Make i_lock and i_mmap native rwsems
  xfs: replace mr*() functions with native rwsem calls

 fs/xfs/mrlock.h    | 78 ----------------------------------------------
 fs/xfs/xfs_inode.c | 43 ++++++++++++++-----------
 fs/xfs/xfs_inode.h |  6 ++--
 fs/xfs/xfs_iops.c  |  4 +--
 fs/xfs/xfs_linux.h |  1 -
 fs/xfs/xfs_super.c |  6 ++--
 6 files changed, 32 insertions(+), 106 deletions(-)
 delete mode 100644 fs/xfs/mrlock.h

-- 
2.24.1

[PATCH 1/4] xfs: change xfs_isilocked() to always use lockdep()

[Pavel Reichl]

mr_writer is obsolete and the information it contains is accesible
from mr_lock.

Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 fs/xfs/xfs_inode.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index c5077e6326c7..32fac6152dc3 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -352,13 +352,17 @@ xfs_isilocked(
 {
 	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
 		if (!(lock_flags & XFS_ILOCK_SHARED))
-			return !!ip->i_lock.mr_writer;
+			return !debug_locks ||
+				lockdep_is_held_type(&ip->i_lock.mr_lock, 0);
 		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 !debug_locks ||
+				lockdep_is_held_type(
+					&ip->i_mmaplock.mr_lock,
+					0);
 		return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
 	}
 
-- 
2.24.1

[PATCH 2/4] xfs: Remove mr_writer field from mrlock_t

[Pavel Reichl]

mr_writer field is no longer used by any code.

Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 fs/xfs/mrlock.h | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/fs/xfs/mrlock.h b/fs/xfs/mrlock.h
index 79155eec341b..1923be92a832 100644
--- a/fs/xfs/mrlock.h
+++ b/fs/xfs/mrlock.h
@@ -10,14 +10,11 @@
 
 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)
+	do { init_rwsem(&(mrp)->mr_lock); } while (0)
 #else
 #define mrinit(mrp, name)	\
 	do { init_rwsem(&(mrp)->mr_lock); } while (0)
@@ -34,9 +31,6 @@ static inline void mraccess_nested(mrlock_t *mrp, int 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)
@@ -48,17 +42,11 @@ 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);
 }
 
@@ -69,9 +57,6 @@ static inline void mrunlock_shared(mrlock_t *mrp)
 
 static inline void mrdemote(mrlock_t *mrp)
 {
-#if defined(DEBUG) || defined(XFS_WARN)
-	mrp->mr_writer = 0;
-#endif
 	downgrade_write(&mrp->mr_lock);
 }
 
-- 
2.24.1

[PATCH 3/4] xfs: Make i_lock and i_mmap native rwsems

[Pavel Reichl]

mr*() functions need to take struct rw_semaphore as parameter.

Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 fs/xfs/mrlock.h    | 38 ++++++++++++++++++--------------------
 fs/xfs/xfs_inode.c |  8 ++++----
 fs/xfs/xfs_inode.h |  6 ++++--
 fs/xfs/xfs_iops.c  |  4 ++--
 4 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/fs/xfs/mrlock.h b/fs/xfs/mrlock.h
index 1923be92a832..245f417a7ffe 100644
--- a/fs/xfs/mrlock.h
+++ b/fs/xfs/mrlock.h
@@ -13,51 +13,49 @@ typedef struct {
 } mrlock_t;
 
 #if defined(DEBUG) || defined(XFS_WARN)
-#define mrinit(mrp, name)	\
-	do { init_rwsem(&(mrp)->mr_lock); } while (0)
+#define mrinit(smp, name)	init_rwsem(smp)
 #else
-#define mrinit(mrp, name)	\
-	do { init_rwsem(&(mrp)->mr_lock); } while (0)
+#define mrinit(smp, name)	init_rwsem(smp)
 #endif
 
-#define mrlock_init(mrp, t,n,s)	mrinit(mrp, n)
-#define mrfree(mrp)		do { } while (0)
+#define mrlock_init(smp, t, n, s)	mrinit(smp, n)
+#define mrfree(smp)		do { } while (0)
 
-static inline void mraccess_nested(mrlock_t *mrp, int subclass)
+static inline void mraccess_nested(struct rw_semaphore *s, int subclass)
 {
-	down_read_nested(&mrp->mr_lock, subclass);
+	down_read_nested(s, subclass);
 }
 
-static inline void mrupdate_nested(mrlock_t *mrp, int subclass)
+static inline void mrupdate_nested(struct rw_semaphore *s, int subclass)
 {
-	down_write_nested(&mrp->mr_lock, subclass);
+	down_write_nested(s, subclass);
 }
 
-static inline int mrtryaccess(mrlock_t *mrp)
+static inline int mrtryaccess(struct rw_semaphore *s)
 {
-	return down_read_trylock(&mrp->mr_lock);
+	return down_read_trylock(s);
 }
 
-static inline int mrtryupdate(mrlock_t *mrp)
+static inline int mrtryupdate(struct rw_semaphore *s)
 {
-	if (!down_write_trylock(&mrp->mr_lock))
+	if (!down_write_trylock(s))
 		return 0;
 	return 1;
 }
 
-static inline void mrunlock_excl(mrlock_t *mrp)
+static inline void mrunlock_excl(struct rw_semaphore *s)
 {
-	up_write(&mrp->mr_lock);
+	up_write(s);
 }
 
-static inline void mrunlock_shared(mrlock_t *mrp)
+static inline void mrunlock_shared(struct rw_semaphore *s)
 {
-	up_read(&mrp->mr_lock);
+	up_read(s);
 }
 
-static inline void mrdemote(mrlock_t *mrp)
+static inline void mrdemote(struct rw_semaphore *s)
 {
-	downgrade_write(&mrp->mr_lock);
+	downgrade_write(s);
 }
 
 #endif /* __XFS_SUPPORT_MRLOCK_H__ */
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 32fac6152dc3..567dae69cfac 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -353,17 +353,17 @@ xfs_isilocked(
 	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
 		if (!(lock_flags & XFS_ILOCK_SHARED))
 			return !debug_locks ||
-				lockdep_is_held_type(&ip->i_lock.mr_lock, 0);
-		return rwsem_is_locked(&ip->i_lock.mr_lock);
+				lockdep_is_held_type(&ip->i_lock, 0);
+		return rwsem_is_locked(&ip->i_lock);
 	}
 
 	if (lock_flags & (XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) {
 		if (!(lock_flags & XFS_MMAPLOCK_SHARED))
 			return !debug_locks ||
 				lockdep_is_held_type(
-					&ip->i_mmaplock.mr_lock,
+					&ip->i_mmaplock,
 					0);
-		return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
+		return rwsem_is_locked(&ip->i_mmaplock);
 	}
 
 	if (lock_flags & (XFS_IOLOCK_EXCL|XFS_IOLOCK_SHARED)) {
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 492e53992fa9..0ea811587cec 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 */
 
 	/*
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);
 	}
 
 	/*
-- 
2.24.1

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

[Pavel Reichl]

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

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

diff --git a/fs/xfs/mrlock.h b/fs/xfs/mrlock.h
deleted file mode 100644
index 245f417a7ffe..000000000000
--- a/fs/xfs/mrlock.h
+++ /dev/null
@@ -1,61 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Copyright (c) 2000-2006 Silicon Graphics, Inc.
- * All Rights Reserved.
- */
-#ifndef __XFS_SUPPORT_MRLOCK_H__
-#define __XFS_SUPPORT_MRLOCK_H__
-
-#include <linux/rwsem.h>
-
-typedef struct {
-	struct rw_semaphore	mr_lock;
-} mrlock_t;
-
-#if defined(DEBUG) || defined(XFS_WARN)
-#define mrinit(smp, name)	init_rwsem(smp)
-#else
-#define mrinit(smp, name)	init_rwsem(smp)
-#endif
-
-#define mrlock_init(smp, t, n, s)	mrinit(smp, n)
-#define mrfree(smp)		do { } while (0)
-
-static inline void mraccess_nested(struct rw_semaphore *s, int subclass)
-{
-	down_read_nested(s, subclass);
-}
-
-static inline void mrupdate_nested(struct rw_semaphore *s, int subclass)
-{
-	down_write_nested(s, subclass);
-}
-
-static inline int mrtryaccess(struct rw_semaphore *s)
-{
-	return down_read_trylock(s);
-}
-
-static inline int mrtryupdate(struct rw_semaphore *s)
-{
-	if (!down_write_trylock(s))
-		return 0;
-	return 1;
-}
-
-static inline void mrunlock_excl(struct rw_semaphore *s)
-{
-	up_write(s);
-}
-
-static inline void mrunlock_shared(struct rw_semaphore *s)
-{
-	up_read(s);
-}
-
-static inline void mrdemote(struct rw_semaphore *s)
-{
-	downgrade_write(s);
-}
-
-#endif /* __XFS_SUPPORT_MRLOCK_H__ */
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 567dae69cfac..01bca957e305 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -191,14 +191,15 @@ xfs_ilock(
 	}
 
 	if (lock_flags & XFS_MMAPLOCK_EXCL)
-		mrupdate_nested(&ip->i_mmaplock, XFS_MMAPLOCK_DEP(lock_flags));
+		down_write_nested(&ip->i_mmaplock,
+				XFS_MMAPLOCK_DEP(lock_flags));
 	else if (lock_flags & XFS_MMAPLOCK_SHARED)
-		mraccess_nested(&ip->i_mmaplock, XFS_MMAPLOCK_DEP(lock_flags));
+		down_read_nested(&ip->i_mmaplock, XFS_MMAPLOCK_DEP(lock_flags));
 
 	if (lock_flags & XFS_ILOCK_EXCL)
-		mrupdate_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
+		down_write_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
 	else if (lock_flags & XFS_ILOCK_SHARED)
-		mraccess_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
+		down_read_nested(&ip->i_lock, XFS_ILOCK_DEP(lock_flags));
 }
 
 /*
@@ -242,27 +243,27 @@ xfs_ilock_nowait(
 	}
 
 	if (lock_flags & XFS_MMAPLOCK_EXCL) {
-		if (!mrtryupdate(&ip->i_mmaplock))
+		if (!down_write_trylock(&ip->i_mmaplock))
 			goto out_undo_iolock;
 	} else if (lock_flags & XFS_MMAPLOCK_SHARED) {
-		if (!mrtryaccess(&ip->i_mmaplock))
+		if (!down_read_trylock(&ip->i_mmaplock))
 			goto out_undo_iolock;
 	}
 
 	if (lock_flags & XFS_ILOCK_EXCL) {
-		if (!mrtryupdate(&ip->i_lock))
+		if (!down_write_trylock(&ip->i_lock))
 			goto out_undo_mmaplock;
 	} else if (lock_flags & XFS_ILOCK_SHARED) {
-		if (!mrtryaccess(&ip->i_lock))
+		if (!down_read_trylock(&ip->i_lock))
 			goto out_undo_mmaplock;
 	}
 	return 1;
 
 out_undo_mmaplock:
 	if (lock_flags & XFS_MMAPLOCK_EXCL)
-		mrunlock_excl(&ip->i_mmaplock);
+		up_write(&ip->i_mmaplock);
 	else if (lock_flags & XFS_MMAPLOCK_SHARED)
-		mrunlock_shared(&ip->i_mmaplock);
+		up_read(&ip->i_mmaplock);
 out_undo_iolock:
 	if (lock_flags & XFS_IOLOCK_EXCL)
 		up_write(&VFS_I(ip)->i_rwsem);
@@ -309,14 +310,14 @@ xfs_iunlock(
 		up_read(&VFS_I(ip)->i_rwsem);
 
 	if (lock_flags & XFS_MMAPLOCK_EXCL)
-		mrunlock_excl(&ip->i_mmaplock);
+		up_write(&ip->i_mmaplock);
 	else if (lock_flags & XFS_MMAPLOCK_SHARED)
-		mrunlock_shared(&ip->i_mmaplock);
+		up_read(&ip->i_mmaplock);
 
 	if (lock_flags & XFS_ILOCK_EXCL)
-		mrunlock_excl(&ip->i_lock);
+		up_write(&ip->i_lock);
 	else if (lock_flags & XFS_ILOCK_SHARED)
-		mrunlock_shared(&ip->i_lock);
+		up_read(&ip->i_lock);
 
 	trace_xfs_iunlock(ip, lock_flags, _RET_IP_);
 }
@@ -335,9 +336,9 @@ xfs_ilock_demote(
 		~(XFS_IOLOCK_EXCL|XFS_MMAPLOCK_EXCL|XFS_ILOCK_EXCL)) == 0);
 
 	if (lock_flags & XFS_ILOCK_EXCL)
-		mrdemote(&ip->i_lock);
+		downgrade_write(&ip->i_lock);
 	if (lock_flags & XFS_MMAPLOCK_EXCL)
-		mrdemote(&ip->i_mmaplock);
+		downgrade_write(&ip->i_mmaplock);
 	if (lock_flags & XFS_IOLOCK_EXCL)
 		downgrade_write(&VFS_I(ip)->i_rwsem);
 
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index 8738bb03f253..921a3eb093ed 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -22,7 +22,6 @@ typedef __u32			xfs_nlink_t;
 #include "xfs_types.h"
 
 #include "kmem.h"
-#include "mrlock.h"
 
 #include <linux/semaphore.h>
 #include <linux/mm.h>
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 760901783944..1289ce1f4e9e 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -661,10 +661,8 @@ xfs_fs_inode_init_once(
 	atomic_set(&ip->i_pincount, 0);
 	spin_lock_init(&ip->i_flags_lock);
 
-	mrlock_init(&ip->i_mmaplock, MRLOCK_ALLOW_EQUAL_PRI|MRLOCK_BARRIER,
-		     "xfsino", ip->i_ino);
-	mrlock_init(&ip->i_lock, MRLOCK_ALLOW_EQUAL_PRI|MRLOCK_BARRIER,
-		     "xfsino", ip->i_ino);
+	init_rwsem(&ip->i_mmaplock);
+	init_rwsem(&ip->i_lock);
 }
 
 /*
-- 
2.24.1

Re: [PATCH 1/4] xfs: change xfs_isilocked() to always use lockdep()

[Darrick J. Wong]

On Tue, Jan 28, 2020 at 03:55:25PM +0100, Pavel Reichl wrote:
> mr_writer is obsolete and the information it contains is accesible
> from mr_lock.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c5077e6326c7..32fac6152dc3 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -352,13 +352,17 @@ xfs_isilocked(
>  {
>  	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
>  		if (!(lock_flags & XFS_ILOCK_SHARED))
> -			return !!ip->i_lock.mr_writer;
> +			return !debug_locks ||
> +				lockdep_is_held_type(&ip->i_lock.mr_lock, 0);

Why do we reference debug_locks here directly?  It looks as though that
variable exists to shut up lockdep assertions WARN_ONs, but
xfs_isilocked is a predicate (and not itself an assertion), so why can't
we 'return lockdep_is_held_type(...);' directly?

(He says scowling at his own RVB in 6552321831dce).

--D

>  		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 !debug_locks ||
> +				lockdep_is_held_type(
> +					&ip->i_mmaplock.mr_lock,
> +					0);
>  		return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
>  	}
>  
> -- 
> 2.24.1
> 

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

[Darrick J. Wong]

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

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

--D

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

Re: [PATCH 1/4] xfs: change xfs_isilocked() to always use lockdep()

[Pavel Reichl]

Hi, maybe performance optimalizaton? If debug_locks is false then
there's no need to call lockdep_is_held_type()? But I'm not sure :-)

On Tue, Jan 28, 2020 at 5:42 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> On Tue, Jan 28, 2020 at 03:55:25PM +0100, Pavel Reichl wrote:
> > mr_writer is obsolete and the information it contains is accesible
> > from mr_lock.
> >
> > Signed-off-by: Pavel Reichl <preichl@redhat.com>
> > ---
> >  fs/xfs/xfs_inode.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index c5077e6326c7..32fac6152dc3 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -352,13 +352,17 @@ xfs_isilocked(
> >  {
> >       if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> >               if (!(lock_flags & XFS_ILOCK_SHARED))
> > -                     return !!ip->i_lock.mr_writer;
> > +                     return !debug_locks ||
> > +                             lockdep_is_held_type(&ip->i_lock.mr_lock, 0);
>
> Why do we reference debug_locks here directly?  It looks as though that
> variable exists to shut up lockdep assertions WARN_ONs, but
> xfs_isilocked is a predicate (and not itself an assertion), so why can't
> we 'return lockdep_is_held_type(...);' directly?
>
> (He says scowling at his own RVB in 6552321831dce).
>
> --D
>
> >               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 !debug_locks ||
> > +                             lockdep_is_held_type(
> > +                                     &ip->i_mmaplock.mr_lock,
> > +                                     0);
> >               return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
> >       }
> >
> > --
> > 2.24.1
> >
>

Re: [PATCH 1/4] xfs: change xfs_isilocked() to always use lockdep()

[Eric Sandeen]

On 1/28/20 10:42 AM, Darrick J. Wong wrote:
> On Tue, Jan 28, 2020 at 03:55:25PM +0100, Pavel Reichl wrote:
>> mr_writer is obsolete and the information it contains is accesible
>> from mr_lock.
>>
>> Signed-off-by: Pavel Reichl <preichl@redhat.com>
>> ---
>>   fs/xfs/xfs_inode.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>> index c5077e6326c7..32fac6152dc3 100644
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -352,13 +352,17 @@ xfs_isilocked(
>>   {
>>   	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
>>   		if (!(lock_flags & XFS_ILOCK_SHARED))
>> -			return !!ip->i_lock.mr_writer;
>> +			return !debug_locks ||
>> +				lockdep_is_held_type(&ip->i_lock.mr_lock, 0);
> 
> Why do we reference debug_locks here directly?  It looks as though that
> variable exists to shut up lockdep assertions WARN_ONs, but
> xfs_isilocked is a predicate (and not itself an assertion), so why can't
> we 'return lockdep_is_held_type(...);' directly?
> 
> (He says scowling at his own RVB in 6552321831dce).

yes that's the answer to why /this/ patch does that ;)

We seem to not be the only ones but who knows if this is cargo cult or?

8 i915/gem/i915_gem_object. i915_gem_object_lookup_rc   66 
WARN_ON(debug_locks && !lock_is_held(&rcu_lock_map));

l include/net/sock.h        sock_owned_by_me          1573 
WARN_ON_ONCE(!lockdep_sock_is_held(sk) && debug_locks);

I'd be inclined to accept this as the current way xfs_isilocked handles
the lockdep tests, and if it's wrong or unnecessary that's another patch.

-Eric

> --D
> 
>>   		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 !debug_locks ||
>> +				lockdep_is_held_type(
>> +					&ip->i_mmaplock.mr_lock,
>> +					0);
>>   		return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
>>   	}
>>   
>> -- 
>> 2.24.1
>>
> 

Re: [PATCH 1/4] xfs: change xfs_isilocked() to always use lockdep()

[Dave Chinner]

On Tue, Jan 28, 2020 at 08:42:00AM -0800, Darrick J. Wong wrote:
> On Tue, Jan 28, 2020 at 03:55:25PM +0100, Pavel Reichl wrote:
> > mr_writer is obsolete and the information it contains is accesible
> > from mr_lock.
> > 
> > Signed-off-by: Pavel Reichl <preichl@redhat.com>
> > ---
> >  fs/xfs/xfs_inode.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index c5077e6326c7..32fac6152dc3 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -352,13 +352,17 @@ xfs_isilocked(
> >  {
> >  	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> >  		if (!(lock_flags & XFS_ILOCK_SHARED))
> > -			return !!ip->i_lock.mr_writer;
> > +			return !debug_locks ||
> > +				lockdep_is_held_type(&ip->i_lock.mr_lock, 0);
> 
> Why do we reference debug_locks here directly?  It looks as though that
> variable exists to shut up lockdep assertions WARN_ONs, but
> xfs_isilocked is a predicate (and not itself an assertion), so why can't
> we 'return lockdep_is_held_type(...);' directly?

It's because that's the way lockdep is structured. That is, lockdep
turns off when the first error is reported, and debug_locks is the
variable used to turn lockdep warnings/checking off once an error
has occurred.

It is normally wrapped in lockdep_assert...() macros so you don't
see it, but it is not referenced at all inside the lockdep functions
that do the actual lock state checking. Hence to replicate lockdep
behaviour, we have to check it, too.

The lockdep code now has these wrappers for rwsems:

#define lockdep_assert_held_write(l)    do {                    \
                WARN_ON(debug_locks && !lockdep_is_held_type(l, 0));    \
        } while (0)

#define lockdep_assert_held_read(l)     do {                            \
                WARN_ON(debug_locks && !lockdep_is_held_type(l, 1));    \
        } while (0)

But xfs_isilocked() is called from within ASSERT() calls, so we
don't want WARN_ON() calls within the ASSERT() calls which provide
their own WARN/BUG handling.

IOWs, we essentially open coded lockdep_assert_held_read (and now
_write) to fit into our own framework of lock checking.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/4] xfs: change xfs_isilocked() to always use lockdep()

[Dave Chinner]

On Tue, Jan 28, 2020 at 03:55:25PM +0100, Pavel Reichl wrote:
> mr_writer is obsolete and the information it contains is accesible
> from mr_lock.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  fs/xfs/xfs_inode.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c5077e6326c7..32fac6152dc3 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -352,13 +352,17 @@ xfs_isilocked(
>  {
>  	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
>  		if (!(lock_flags & XFS_ILOCK_SHARED))
> -			return !!ip->i_lock.mr_writer;
> +			return !debug_locks ||
> +				lockdep_is_held_type(&ip->i_lock.mr_lock, 0);
>  		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 !debug_locks ||
> +				lockdep_is_held_type(
> +					&ip->i_mmaplock.mr_lock,
> +					0);
>  		return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
>  	}

Ok, so this code is only called from ASSERT() statements, which
means this turns off write lock checking for XFS debug kernels if
lockdep is not enabled. Hence I think these checks need to be
restructured to be based around rwsem_is_locked() first and lockdep
second.

That is:

/* In all implementations count != 0 means locked */
static inline int rwsem_is_locked(struct rw_semaphore *sem)
{
        return atomic_long_read(&sem->count) != 0;
}

This captures both read and write locks on the rwsem, and doesn't
discriminate at all. Now we don't have explicit writer lock checking
in CONFIG_XFS_DEBUG=y kernels, I think we need to at least check
that the rwsem is locked in all cases to catch cases where we are
calling a function without the lock held. That will ctach most
programming mistakes, and then lockdep will provide the
read-vs-write discrimination to catch the "hold the wrong lock type"
mistakes.

Hence I think this code should end up looking like this:

	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
		bool locked = false;

		if (!rwsem_is_locked(&ip->i_lock))
			return false;
		if (!debug_locks)
			return true;
		if (lock_flags & XFS_ILOCK_EXCL)
			locked = lockdep_is_held_type(&ip->i_lock, 0);
		if (lock_flags & XFS_ILOCK_SHARED)
			locked |= lockdep_is_held_type(&ip->i_lock, 1);
		return locked;
	}

Thoughts?

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/4] xfs: change xfs_isilocked() to always use lockdep()

[Darrick J. Wong]

On Thu, Jan 30, 2020 at 09:18:19AM +1100, Dave Chinner wrote:
> On Tue, Jan 28, 2020 at 03:55:25PM +0100, Pavel Reichl wrote:
> > mr_writer is obsolete and the information it contains is accesible
> > from mr_lock.
> > 
> > Signed-off-by: Pavel Reichl <preichl@redhat.com>
> > ---
> >  fs/xfs/xfs_inode.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index c5077e6326c7..32fac6152dc3 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -352,13 +352,17 @@ xfs_isilocked(
> >  {
> >  	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> >  		if (!(lock_flags & XFS_ILOCK_SHARED))
> > -			return !!ip->i_lock.mr_writer;
> > +			return !debug_locks ||
> > +				lockdep_is_held_type(&ip->i_lock.mr_lock, 0);
> >  		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 !debug_locks ||
> > +				lockdep_is_held_type(
> > +					&ip->i_mmaplock.mr_lock,
> > +					0);
> >  		return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
> >  	}
> 
> Ok, so this code is only called from ASSERT() statements, which
> means this turns off write lock checking for XFS debug kernels if
> lockdep is not enabled. Hence I think these checks need to be
> restructured to be based around rwsem_is_locked() first and lockdep
> second.
> 
> That is:
> 
> /* In all implementations count != 0 means locked */
> static inline int rwsem_is_locked(struct rw_semaphore *sem)
> {
>         return atomic_long_read(&sem->count) != 0;
> }
> 
> This captures both read and write locks on the rwsem, and doesn't
> discriminate at all. Now we don't have explicit writer lock checking
> in CONFIG_XFS_DEBUG=y kernels, I think we need to at least check
> that the rwsem is locked in all cases to catch cases where we are
> calling a function without the lock held. That will ctach most
> programming mistakes, and then lockdep will provide the
> read-vs-write discrimination to catch the "hold the wrong lock type"
> mistakes.
> 
> Hence I think this code should end up looking like this:
> 
> 	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> 		bool locked = false;
> 
> 		if (!rwsem_is_locked(&ip->i_lock))
> 			return false;
> 		if (!debug_locks)
> 			return true;
> 		if (lock_flags & XFS_ILOCK_EXCL)
> 			locked = lockdep_is_held_type(&ip->i_lock, 0);
> 		if (lock_flags & XFS_ILOCK_SHARED)
> 			locked |= lockdep_is_held_type(&ip->i_lock, 1);
> 		return locked;
> 	}
> 
> Thoughts?

I like that a lot better, though perhaps the if body should be factored
into a separate static inline so we don't repeat that 3x.

--D

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 1/4] xfs: change xfs_isilocked() to always use lockdep()

[Dave Chinner]

On Wed, Jan 29, 2020 at 02:25:32PM -0800, Darrick J. Wong wrote:
> On Thu, Jan 30, 2020 at 09:18:19AM +1100, Dave Chinner wrote:
> > On Tue, Jan 28, 2020 at 03:55:25PM +0100, Pavel Reichl wrote:
> > > mr_writer is obsolete and the information it contains is accesible
> > > from mr_lock.
> > > 
> > > Signed-off-by: Pavel Reichl <preichl@redhat.com>
> > > ---
> > >  fs/xfs/xfs_inode.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > index c5077e6326c7..32fac6152dc3 100644
> > > --- a/fs/xfs/xfs_inode.c
> > > +++ b/fs/xfs/xfs_inode.c
> > > @@ -352,13 +352,17 @@ xfs_isilocked(
> > >  {
> > >  	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> > >  		if (!(lock_flags & XFS_ILOCK_SHARED))
> > > -			return !!ip->i_lock.mr_writer;
> > > +			return !debug_locks ||
> > > +				lockdep_is_held_type(&ip->i_lock.mr_lock, 0);
> > >  		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 !debug_locks ||
> > > +				lockdep_is_held_type(
> > > +					&ip->i_mmaplock.mr_lock,
> > > +					0);
> > >  		return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
> > >  	}
> > 
> > Ok, so this code is only called from ASSERT() statements, which
> > means this turns off write lock checking for XFS debug kernels if
> > lockdep is not enabled. Hence I think these checks need to be
> > restructured to be based around rwsem_is_locked() first and lockdep
> > second.
> > 
> > That is:
> > 
> > /* In all implementations count != 0 means locked */
> > static inline int rwsem_is_locked(struct rw_semaphore *sem)
> > {
> >         return atomic_long_read(&sem->count) != 0;
> > }
> > 
> > This captures both read and write locks on the rwsem, and doesn't
> > discriminate at all. Now we don't have explicit writer lock checking
> > in CONFIG_XFS_DEBUG=y kernels, I think we need to at least check
> > that the rwsem is locked in all cases to catch cases where we are
> > calling a function without the lock held. That will ctach most
> > programming mistakes, and then lockdep will provide the
> > read-vs-write discrimination to catch the "hold the wrong lock type"
> > mistakes.
> > 
> > Hence I think this code should end up looking like this:
> > 
> > 	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> > 		bool locked = false;
> > 
> > 		if (!rwsem_is_locked(&ip->i_lock))
> > 			return false;
> > 		if (!debug_locks)
> > 			return true;
> > 		if (lock_flags & XFS_ILOCK_EXCL)
> > 			locked = lockdep_is_held_type(&ip->i_lock, 0);
> > 		if (lock_flags & XFS_ILOCK_SHARED)
> > 			locked |= lockdep_is_held_type(&ip->i_lock, 1);
> > 		return locked;
> > 	}
> > 
> > Thoughts?
> 
> I like that a lot better, though perhaps the if body should be factored
> into a separate static inline so we don't repeat that 3x.

Yup, I had thoughts along those lines, too, but each lock type uses
different flags and that makes it more verbose than it could be.
Maybe something like this?

static inline bool
__xfs_is_ilocked(
	struct rwsem	*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(&ip->i_lock, 0);
	if (excl)
		locked |= lockdep_is_held_type(&ip->i_lock, 1);
	return locked;
}

bool
xfs_isilocked(
	struct xfs_inode	*ip,
	int			lock_flags)
{
	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED))
		return __xfs_is_ilocked(&ip->i_lock,
				(lock_flags & XFS_ILOCK_SHARED),
				(lock_flags & XFS_ILOCK_EXCL));

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

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

	ASSERT(0);
	return false;
}

At which point I wonder if it would simply be better to have:

bool
xfs_is_ilocked(
	struct xfs_inode	*ip,
	int			lock_flags)
{
	return __xfs_is_ilocked(&ip->i_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,
				(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));
}

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/4] xfs: change xfs_isilocked() to always use lockdep()

[Christoph Hellwig]

On Thu, Jan 30, 2020 at 09:18:19AM +1100, Dave Chinner wrote:
> This captures both read and write locks on the rwsem, and doesn't
> discriminate at all. Now we don't have explicit writer lock checking
> in CONFIG_XFS_DEBUG=y kernels, I think we need to at least check
> that the rwsem is locked in all cases to catch cases where we are
> calling a function without the lock held. That will ctach most
> programming mistakes, and then lockdep will provide the
> read-vs-write discrimination to catch the "hold the wrong lock type"
> mistakes.
> 
> Hence I think this code should end up looking like this:
> 
> 	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> 		bool locked = false;
> 
> 		if (!rwsem_is_locked(&ip->i_lock))
> 			return false;
> 		if (!debug_locks)
> 			return true;
> 		if (lock_flags & XFS_ILOCK_EXCL)
> 			locked = lockdep_is_held_type(&ip->i_lock, 0);
> 		if (lock_flags & XFS_ILOCK_SHARED)
> 			locked |= lockdep_is_held_type(&ip->i_lock, 1);
> 		return locked;
> 	}
> 
> Thoughts?

I like the idea, but I really think that this does not belong into XFS,
but into the core rwsem code.  That means replacing the lock_flags with
a bool exclusive, picking a good name for it (can't think of one right
now, except for re-using rwsem_is_locked), and adding a kerneldoc
comment explaining the semantics and use cases in detail.

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

[Christoph Hellwig]

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

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

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

[Pavel Reichl]

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

Hello Christoph,

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

Bye

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

[Christoph Hellwig]

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

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

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

[Pavel Reichl]

OK, thank you for your opinion. :-)

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

Re: [PATCH 1/4] xfs: change xfs_isilocked() to always use lockdep()

[Dave Chinner]

On Wed, Jan 29, 2020 at 11:44:24PM -0800, Christoph Hellwig wrote:
> On Thu, Jan 30, 2020 at 09:18:19AM +1100, Dave Chinner wrote:
> > This captures both read and write locks on the rwsem, and doesn't
> > discriminate at all. Now we don't have explicit writer lock checking
> > in CONFIG_XFS_DEBUG=y kernels, I think we need to at least check
> > that the rwsem is locked in all cases to catch cases where we are
> > calling a function without the lock held. That will ctach most
> > programming mistakes, and then lockdep will provide the
> > read-vs-write discrimination to catch the "hold the wrong lock type"
> > mistakes.
> > 
> > Hence I think this code should end up looking like this:
> > 
> > 	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> > 		bool locked = false;
> > 
> > 		if (!rwsem_is_locked(&ip->i_lock))
> > 			return false;
> > 		if (!debug_locks)
> > 			return true;
> > 		if (lock_flags & XFS_ILOCK_EXCL)
> > 			locked = lockdep_is_held_type(&ip->i_lock, 0);
> > 		if (lock_flags & XFS_ILOCK_SHARED)
> > 			locked |= lockdep_is_held_type(&ip->i_lock, 1);
> > 		return locked;
> > 	}
> > 
> > Thoughts?
> 
> I like the idea, but I really think that this does not belong into XFS,
> but into the core rwsem code.  That means replacing the lock_flags with
> a bool exclusive, picking a good name for it (can't think of one right
> now, except for re-using rwsem_is_locked), and adding a kerneldoc
> comment explaining the semantics and use cases in detail.

I'd say that's the step after removing mrlocks in XFS. Get this
patchset sorted, then lift the rwsem checking function to the core
code as a separate patchset that can be handled indepedently to the
changes we need to make to XFS...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/4] xfs: change xfs_isilocked() to always use lockdep()

[Bill O'Donnell]

On Fri, Jan 31, 2020 at 07:14:47AM +1100, Dave Chinner wrote:
> On Wed, Jan 29, 2020 at 11:44:24PM -0800, Christoph Hellwig wrote:
> > On Thu, Jan 30, 2020 at 09:18:19AM +1100, Dave Chinner wrote:
> > > This captures both read and write locks on the rwsem, and doesn't
> > > discriminate at all. Now we don't have explicit writer lock checking
> > > in CONFIG_XFS_DEBUG=y kernels, I think we need to at least check
> > > that the rwsem is locked in all cases to catch cases where we are
> > > calling a function without the lock held. That will ctach most
> > > programming mistakes, and then lockdep will provide the
> > > read-vs-write discrimination to catch the "hold the wrong lock type"
> > > mistakes.
> > > 
> > > Hence I think this code should end up looking like this:
> > > 
> > > 	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> > > 		bool locked = false;
> > > 
> > > 		if (!rwsem_is_locked(&ip->i_lock))
> > > 			return false;
> > > 		if (!debug_locks)
> > > 			return true;
> > > 		if (lock_flags & XFS_ILOCK_EXCL)
> > > 			locked = lockdep_is_held_type(&ip->i_lock, 0);
> > > 		if (lock_flags & XFS_ILOCK_SHARED)
> > > 			locked |= lockdep_is_held_type(&ip->i_lock, 1);
> > > 		return locked;
> > > 	}
> > > 
> > > Thoughts?
> > 
> > I like the idea, but I really think that this does not belong into XFS,
> > but into the core rwsem code.  That means replacing the lock_flags with
> > a bool exclusive, picking a good name for it (can't think of one right
> > now, except for re-using rwsem_is_locked), and adding a kerneldoc
> > comment explaining the semantics and use cases in detail.
> 
> I'd say that's the step after removing mrlocks in XFS. Get this
> patchset sorted, then lift the rwsem checking function to the core
> code as a separate patchset that can be handled indepedently to the
> changes we need to make to XFS...

I agree with this approach, with modification of rwsem checking code as
as separate follow-on patchset.
Thanks-
Bill

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>