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

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

[Pavel Reichl]

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

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

Changes in version 8:
* Patchset was rebased so it applies cleanly.
* The patch 'xfs: replace mrlock_t with rw_semaphores' contains change in
xfs_btree.c which transfers ownership of lock so lockdep won't assert
(This was reported by Darrick and proposed change fixes this issue).

Changes in version 9:
*Fixed white space in patch 'xfs: Refactor xfs_isilocked()'
*Updated code comments as suggested by djwong (thanks!) in patch: 'xfs: replace mrlock_t with rw_semaphores'

Changes in version 10:
* Fixed use-after-free in 'xfs: replace mrlock_t with rw_semaphores' (thanks Darrick)
* Moved part of refactor of xfs_isilocked() from patch 'xfs: Refactor xfs_isilocked()' to patch 'xfs: replace mrlock_t with rw_semaphores' - to fix compilation error
* Typo in comment in 'xfs: replace mrlock_t with rw_semaphores'

Changes in version 11:
* Dropped typedef xfs_node_t from xfs_isilocked in 'xfs: Refactor xfs_isilocked()'

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

 fs/xfs/libxfs/xfs_bmap.c  |   8 +--
 fs/xfs/libxfs/xfs_btree.c |  24 +++++++++
 fs/xfs/mrlock.h           |  78 -----------------------------
 fs/xfs/xfs_file.c         |   3 +-
 fs/xfs/xfs_inode.c        | 102 +++++++++++++++++++++++++-------------
 fs/xfs/xfs_inode.h        |  25 ++++++----
 fs/xfs/xfs_iops.c         |   4 +-
 fs/xfs/xfs_linux.h        |   2 +-
 fs/xfs/xfs_qm.c           |   2 +-
 fs/xfs/xfs_super.c        |   6 +--
 10 files changed, 121 insertions(+), 133 deletions(-)
 delete mode 100644 fs/xfs/mrlock.h

-- 
2.26.2

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

[Pavel Reichl]

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

Signed-off-by: Pavel Reichl <preichl@redhat.com>
Suggested-by: Dave Chinner <dchinner@redhat.com>
Suggested-by: Eric Sandeen <sandeen@redhat.com>
Suggested-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/xfs_inode.c | 48 ++++++++++++++++++++++++++++++++++++++--------
 fs/xfs/xfs_inode.h | 21 +++++++++++++-------
 2 files changed, 54 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index c06129cffba9..7c1ceb4df4ec 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -345,9 +345,43 @@ xfs_ilock_demote(
 }
 
 #if defined(DEBUG) || defined(XFS_WARN)
-int
+static inline bool
+__xfs_rwsem_islocked(
+	struct rw_semaphore	*rwsem,
+	int			lock_flags)
+{
+	int			arg;
+
+	if (!debug_locks)
+		return rwsem_is_locked(rwsem);
+
+	if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) {
+		/*
+		 * The caller could be asking if we have (shared | excl)
+		 * access to the lock. Ask lockdep if the rwsem is
+		 * locked either for read or write access.
+		 *
+		 * The caller could also be asking if we have only
+		 * shared access to the lock. Holding a rwsem
+		 * write-locked implies read access as well, so the
+		 * request to lockdep is the same for this case.
+		 */
+		arg = -1;
+	} else {
+		/*
+		 * The caller is asking if we have only exclusive access
+		 * to the lock. Ask lockdep if the rwsem is locked for
+		 * write access.
+		 */
+		arg = 0;
+	}
+
+	return lockdep_is_held_type(rwsem, arg);
+}
+
+bool
 xfs_isilocked(
-	xfs_inode_t		*ip,
+	struct xfs_inode	*ip,
 	uint			lock_flags)
 {
 	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
@@ -362,15 +396,13 @@ xfs_isilocked(
 		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);
+	if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) {
+		return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem,
+				(lock_flags >> XFS_IOLOCK_FLAG_SHIFT));
 	}
 
 	ASSERT(0);
-	return 0;
+	return false;
 }
 #endif
 
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index e9a8bb184d1f..77776af75c77 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -268,12 +268,19 @@ static inline void xfs_ifunlock(struct xfs_inode *ip)
  * Bit ranges:	1<<1  - 1<<16-1 -- iolock/ilock modes (bitfield)
  *		1<<16 - 1<<32-1 -- lockdep annotation (integers)
  */
-#define	XFS_IOLOCK_EXCL		(1<<0)
-#define	XFS_IOLOCK_SHARED	(1<<1)
-#define	XFS_ILOCK_EXCL		(1<<2)
-#define	XFS_ILOCK_SHARED	(1<<3)
-#define	XFS_MMAPLOCK_EXCL	(1<<4)
-#define	XFS_MMAPLOCK_SHARED	(1<<5)
+
+#define XFS_IOLOCK_FLAG_SHIFT	0
+#define XFS_ILOCK_FLAG_SHIFT	2
+#define XFS_MMAPLOCK_FLAG_SHIFT	4
+
+#define XFS_SHARED_LOCK_SHIFT	1
+
+#define XFS_IOLOCK_EXCL		(1 << (XFS_IOLOCK_FLAG_SHIFT))
+#define XFS_IOLOCK_SHARED	(XFS_IOLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT))
+#define XFS_ILOCK_EXCL		(1 << (XFS_ILOCK_FLAG_SHIFT))
+#define XFS_ILOCK_SHARED	(XFS_ILOCK_EXCL << (XFS_SHARED_LOCK_SHIFT))
+#define XFS_MMAPLOCK_EXCL	(1 << (XFS_MMAPLOCK_FLAG_SHIFT))
+#define XFS_MMAPLOCK_SHARED	(XFS_MMAPLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT))
 
 #define XFS_LOCK_MASK		(XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED \
 				| XFS_ILOCK_EXCL | XFS_ILOCK_SHARED \
@@ -412,7 +419,7 @@ void		xfs_ilock(xfs_inode_t *, uint);
 int		xfs_ilock_nowait(xfs_inode_t *, uint);
 void		xfs_iunlock(xfs_inode_t *, uint);
 void		xfs_ilock_demote(xfs_inode_t *, uint);
-int		xfs_isilocked(xfs_inode_t *, uint);
+bool		xfs_isilocked(struct xfs_inode *, uint);
 uint		xfs_ilock_data_map_shared(struct xfs_inode *);
 uint		xfs_ilock_attr_map_shared(struct xfs_inode *);
 
-- 
2.26.2

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

[Brian Foster]

On Fri, Oct 09, 2020 at 09:55:12PM +0200, Pavel Reichl wrote:
> Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked().
> __xfs_rwsem_islocked() is a helper function which encapsulates checking
> state of rw_semaphores hold by inode.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>
> Suggested-by: Eric Sandeen <sandeen@redhat.com>
> Suggested-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_inode.c | 48 ++++++++++++++++++++++++++++++++++++++--------
>  fs/xfs/xfs_inode.h | 21 +++++++++++++-------
>  2 files changed, 54 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c06129cffba9..7c1ceb4df4ec 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -345,9 +345,43 @@ xfs_ilock_demote(
>  }
>  
>  #if defined(DEBUG) || defined(XFS_WARN)
> -int
> +static inline bool
> +__xfs_rwsem_islocked(
> +	struct rw_semaphore	*rwsem,
> +	int			lock_flags)
> +{
> +	int			arg;
> +
> +	if (!debug_locks)
> +		return rwsem_is_locked(rwsem);
> +
> +	if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) {
> +		/*
> +		 * The caller could be asking if we have (shared | excl)
> +		 * access to the lock. Ask lockdep if the rwsem is
> +		 * locked either for read or write access.
> +		 *
> +		 * The caller could also be asking if we have only
> +		 * shared access to the lock. Holding a rwsem
> +		 * write-locked implies read access as well, so the
> +		 * request to lockdep is the same for this case.
> +		 */
> +		arg = -1;
> +	} else {
> +		/*
> +		 * The caller is asking if we have only exclusive access
> +		 * to the lock. Ask lockdep if the rwsem is locked for
> +		 * write access.
> +		 */
> +		arg = 0;
> +	}

Are these arg values documented somewhere? A quick look at the function
below didn't show anything..

Also, I find the pattern of shifting in the caller slightly confusing,
particularly with the 'lock_flags' name being passed down through the
caller. Any reason we couldn't pass the shift value as a parameter and
do the shift at the top of the function so the logic is clear and in one
place?

> +
> +	return lockdep_is_held_type(rwsem, arg);
> +}
> +
> +bool
>  xfs_isilocked(
> -	xfs_inode_t		*ip,
> +	struct xfs_inode	*ip,
>  	uint			lock_flags)
>  {
>  	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
...
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index e9a8bb184d1f..77776af75c77 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -268,12 +268,19 @@ static inline void xfs_ifunlock(struct xfs_inode *ip)
>   * Bit ranges:	1<<1  - 1<<16-1 -- iolock/ilock modes (bitfield)
>   *		1<<16 - 1<<32-1 -- lockdep annotation (integers)
>   */
> -#define	XFS_IOLOCK_EXCL		(1<<0)
> -#define	XFS_IOLOCK_SHARED	(1<<1)
> -#define	XFS_ILOCK_EXCL		(1<<2)
> -#define	XFS_ILOCK_SHARED	(1<<3)
> -#define	XFS_MMAPLOCK_EXCL	(1<<4)
> -#define	XFS_MMAPLOCK_SHARED	(1<<5)
> +
> +#define XFS_IOLOCK_FLAG_SHIFT	0
> +#define XFS_ILOCK_FLAG_SHIFT	2
> +#define XFS_MMAPLOCK_FLAG_SHIFT	4
> +
> +#define XFS_SHARED_LOCK_SHIFT	1
> +
> +#define XFS_IOLOCK_EXCL		(1 << (XFS_IOLOCK_FLAG_SHIFT))
> +#define XFS_IOLOCK_SHARED	(XFS_IOLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT))
> +#define XFS_ILOCK_EXCL		(1 << (XFS_ILOCK_FLAG_SHIFT))
> +#define XFS_ILOCK_SHARED	(XFS_ILOCK_EXCL << (XFS_SHARED_LOCK_SHIFT))
> +#define XFS_MMAPLOCK_EXCL	(1 << (XFS_MMAPLOCK_FLAG_SHIFT))
> +#define XFS_MMAPLOCK_SHARED	(XFS_MMAPLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT))
>  

Any reason for the extra params around the shift values?

Brian

>  #define XFS_LOCK_MASK		(XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED \
>  				| XFS_ILOCK_EXCL | XFS_ILOCK_SHARED \
> @@ -412,7 +419,7 @@ void		xfs_ilock(xfs_inode_t *, uint);
>  int		xfs_ilock_nowait(xfs_inode_t *, uint);
>  void		xfs_iunlock(xfs_inode_t *, uint);
>  void		xfs_ilock_demote(xfs_inode_t *, uint);
> -int		xfs_isilocked(xfs_inode_t *, uint);
> +bool		xfs_isilocked(struct xfs_inode *, uint);
>  uint		xfs_ilock_data_map_shared(struct xfs_inode *);
>  uint		xfs_ilock_attr_map_shared(struct xfs_inode *);
>  
> -- 
> 2.26.2
> 

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

[Darrick J. Wong]

On Mon, Oct 12, 2020 at 12:03:08PM -0400, Brian Foster wrote:
> On Fri, Oct 09, 2020 at 09:55:12PM +0200, Pavel Reichl wrote:
> > Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked().
> > __xfs_rwsem_islocked() is a helper function which encapsulates checking
> > state of rw_semaphores hold by inode.
> > 
> > Signed-off-by: Pavel Reichl <preichl@redhat.com>
> > Suggested-by: Dave Chinner <dchinner@redhat.com>
> > Suggested-by: Eric Sandeen <sandeen@redhat.com>
> > Suggested-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/xfs_inode.c | 48 ++++++++++++++++++++++++++++++++++++++--------
> >  fs/xfs/xfs_inode.h | 21 +++++++++++++-------
> >  2 files changed, 54 insertions(+), 15 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index c06129cffba9..7c1ceb4df4ec 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -345,9 +345,43 @@ xfs_ilock_demote(
> >  }
> >  
> >  #if defined(DEBUG) || defined(XFS_WARN)
> > -int
> > +static inline bool
> > +__xfs_rwsem_islocked(
> > +	struct rw_semaphore	*rwsem,
> > +	int			lock_flags)
> > +{
> > +	int			arg;
> > +
> > +	if (!debug_locks)
> > +		return rwsem_is_locked(rwsem);
> > +
> > +	if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) {
> > +		/*
> > +		 * The caller could be asking if we have (shared | excl)
> > +		 * access to the lock. Ask lockdep if the rwsem is
> > +		 * locked either for read or write access.
> > +		 *
> > +		 * The caller could also be asking if we have only
> > +		 * shared access to the lock. Holding a rwsem
> > +		 * write-locked implies read access as well, so the
> > +		 * request to lockdep is the same for this case.
> > +		 */
> > +		arg = -1;
> > +	} else {
> > +		/*
> > +		 * The caller is asking if we have only exclusive access
> > +		 * to the lock. Ask lockdep if the rwsem is locked for
> > +		 * write access.
> > +		 */
> > +		arg = 0;
> > +	}
> 
> Are these arg values documented somewhere? A quick look at the function
> below didn't show anything..

Alas, no. :(

If you trace lockdep_is_held_type -> lock_is_held_type -> __lock_is_held
then you'll notice that "if (read == -1" bit, but none of those
functions are documented.

So I have no if that's /really/ permanent, other than to say that it
exists because Dave and Christoph and I requested it years ago and
commit f8319483f57f1 has been unchanged since 2016.

--D

> Also, I find the pattern of shifting in the caller slightly confusing,
> particularly with the 'lock_flags' name being passed down through the
> caller. Any reason we couldn't pass the shift value as a parameter and
> do the shift at the top of the function so the logic is clear and in one
> place?
> 
> > +
> > +	return lockdep_is_held_type(rwsem, arg);
> > +}
> > +
> > +bool
> >  xfs_isilocked(
> > -	xfs_inode_t		*ip,
> > +	struct xfs_inode	*ip,
> >  	uint			lock_flags)
> >  {
> >  	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> ...
> > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> > index e9a8bb184d1f..77776af75c77 100644
> > --- a/fs/xfs/xfs_inode.h
> > +++ b/fs/xfs/xfs_inode.h
> > @@ -268,12 +268,19 @@ static inline void xfs_ifunlock(struct xfs_inode *ip)
> >   * Bit ranges:	1<<1  - 1<<16-1 -- iolock/ilock modes (bitfield)
> >   *		1<<16 - 1<<32-1 -- lockdep annotation (integers)
> >   */
> > -#define	XFS_IOLOCK_EXCL		(1<<0)
> > -#define	XFS_IOLOCK_SHARED	(1<<1)
> > -#define	XFS_ILOCK_EXCL		(1<<2)
> > -#define	XFS_ILOCK_SHARED	(1<<3)
> > -#define	XFS_MMAPLOCK_EXCL	(1<<4)
> > -#define	XFS_MMAPLOCK_SHARED	(1<<5)
> > +
> > +#define XFS_IOLOCK_FLAG_SHIFT	0
> > +#define XFS_ILOCK_FLAG_SHIFT	2
> > +#define XFS_MMAPLOCK_FLAG_SHIFT	4
> > +
> > +#define XFS_SHARED_LOCK_SHIFT	1
> > +
> > +#define XFS_IOLOCK_EXCL		(1 << (XFS_IOLOCK_FLAG_SHIFT))
> > +#define XFS_IOLOCK_SHARED	(XFS_IOLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT))
> > +#define XFS_ILOCK_EXCL		(1 << (XFS_ILOCK_FLAG_SHIFT))
> > +#define XFS_ILOCK_SHARED	(XFS_ILOCK_EXCL << (XFS_SHARED_LOCK_SHIFT))
> > +#define XFS_MMAPLOCK_EXCL	(1 << (XFS_MMAPLOCK_FLAG_SHIFT))
> > +#define XFS_MMAPLOCK_SHARED	(XFS_MMAPLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT))
> >  
> 
> Any reason for the extra params around the shift values?
> 
> Brian
> 
> >  #define XFS_LOCK_MASK		(XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED \
> >  				| XFS_ILOCK_EXCL | XFS_ILOCK_SHARED \
> > @@ -412,7 +419,7 @@ void		xfs_ilock(xfs_inode_t *, uint);
> >  int		xfs_ilock_nowait(xfs_inode_t *, uint);
> >  void		xfs_iunlock(xfs_inode_t *, uint);
> >  void		xfs_ilock_demote(xfs_inode_t *, uint);
> > -int		xfs_isilocked(xfs_inode_t *, uint);
> > +bool		xfs_isilocked(struct xfs_inode *, uint);
> >  uint		xfs_ilock_data_map_shared(struct xfs_inode *);
> >  uint		xfs_ilock_attr_map_shared(struct xfs_inode *);
> >  
> > -- 
> > 2.26.2
> > 
> 

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

[Brian Foster]

On Mon, Oct 12, 2020 at 02:28:18PM -0700, Darrick J. Wong wrote:
> On Mon, Oct 12, 2020 at 12:03:08PM -0400, Brian Foster wrote:
> > On Fri, Oct 09, 2020 at 09:55:12PM +0200, Pavel Reichl wrote:
> > > Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked().
> > > __xfs_rwsem_islocked() is a helper function which encapsulates checking
> > > state of rw_semaphores hold by inode.
> > > 
> > > Signed-off-by: Pavel Reichl <preichl@redhat.com>
> > > Suggested-by: Dave Chinner <dchinner@redhat.com>
> > > Suggested-by: Eric Sandeen <sandeen@redhat.com>
> > > Suggested-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  fs/xfs/xfs_inode.c | 48 ++++++++++++++++++++++++++++++++++++++--------
> > >  fs/xfs/xfs_inode.h | 21 +++++++++++++-------
> > >  2 files changed, 54 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > index c06129cffba9..7c1ceb4df4ec 100644
> > > --- a/fs/xfs/xfs_inode.c
> > > +++ b/fs/xfs/xfs_inode.c
> > > @@ -345,9 +345,43 @@ xfs_ilock_demote(
> > >  }
> > >  
> > >  #if defined(DEBUG) || defined(XFS_WARN)
> > > -int
> > > +static inline bool
> > > +__xfs_rwsem_islocked(
> > > +	struct rw_semaphore	*rwsem,
> > > +	int			lock_flags)
> > > +{
> > > +	int			arg;
> > > +
> > > +	if (!debug_locks)
> > > +		return rwsem_is_locked(rwsem);
> > > +
> > > +	if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) {
> > > +		/*
> > > +		 * The caller could be asking if we have (shared | excl)
> > > +		 * access to the lock. Ask lockdep if the rwsem is
> > > +		 * locked either for read or write access.
> > > +		 *
> > > +		 * The caller could also be asking if we have only
> > > +		 * shared access to the lock. Holding a rwsem
> > > +		 * write-locked implies read access as well, so the
> > > +		 * request to lockdep is the same for this case.
> > > +		 */
> > > +		arg = -1;
> > > +	} else {
> > > +		/*
> > > +		 * The caller is asking if we have only exclusive access
> > > +		 * to the lock. Ask lockdep if the rwsem is locked for
> > > +		 * write access.
> > > +		 */
> > > +		arg = 0;
> > > +	}
> > 
> > Are these arg values documented somewhere? A quick look at the function
> > below didn't show anything..
> 
> Alas, no. :(
> 
> If you trace lockdep_is_held_type -> lock_is_held_type -> __lock_is_held
> then you'll notice that "if (read == -1" bit, but none of those
> functions are documented.
> 

Ok, so -1 is basically a catchall that causes lockdep to tell us whether
the lock is held or not (in any mode). Any other value is presumably
defined by the lockdep tracking infrastructure since it directly
compares to ->read.

Hmm.. lockdep.h has this:

#define lock_acquire_exclusive(l, s, t, n, i)           lock_acquire(l, s, t, 0, 1, n, i)
#define lock_acquire_shared(l, s, t, n, i)              lock_acquire(l, s, t, 1, 1, n, i)
#define lock_acquire_shared_recursive(l, s, t, n, i)    lock_acquire(l, s, t, 2, 1, n, i)

... which at least seems to correlate the values with modes (0 ==
exclusive, 1 || 2 == shared, etc.). I think an additional sentence or
two on that in our comments would be helpful for the next person that
needs to grok this code. For example, something like the following
(which factors in the above two comments and IMO is slightly more
clear):

"If the shared flag is not set, pass 0 to explicitly check for exclusive
access to the lock. If the shared flag is set, we typically want to make
sure the lock is at least held in shared mode (i.e., shared | excl) but
we don't necessarily care that it might actually be held exclusive.
Therefore, pass -1 to check whether the lock is held in any mode rather
than one of the explicit shared mode values (1 or 2)."

Brian

> So I have no if that's /really/ permanent, other than to say that it
> exists because Dave and Christoph and I requested it years ago and
> commit f8319483f57f1 has been unchanged since 2016.
> 
> --D
> 
> > Also, I find the pattern of shifting in the caller slightly confusing,
> > particularly with the 'lock_flags' name being passed down through the
> > caller. Any reason we couldn't pass the shift value as a parameter and
> > do the shift at the top of the function so the logic is clear and in one
> > place?
> > 
> > > +
> > > +	return lockdep_is_held_type(rwsem, arg);
> > > +}
> > > +
> > > +bool
> > >  xfs_isilocked(
> > > -	xfs_inode_t		*ip,
> > > +	struct xfs_inode	*ip,
> > >  	uint			lock_flags)
> > >  {
> > >  	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> > ...
> > > diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> > > index e9a8bb184d1f..77776af75c77 100644
> > > --- a/fs/xfs/xfs_inode.h
> > > +++ b/fs/xfs/xfs_inode.h
> > > @@ -268,12 +268,19 @@ static inline void xfs_ifunlock(struct xfs_inode *ip)
> > >   * Bit ranges:	1<<1  - 1<<16-1 -- iolock/ilock modes (bitfield)
> > >   *		1<<16 - 1<<32-1 -- lockdep annotation (integers)
> > >   */
> > > -#define	XFS_IOLOCK_EXCL		(1<<0)
> > > -#define	XFS_IOLOCK_SHARED	(1<<1)
> > > -#define	XFS_ILOCK_EXCL		(1<<2)
> > > -#define	XFS_ILOCK_SHARED	(1<<3)
> > > -#define	XFS_MMAPLOCK_EXCL	(1<<4)
> > > -#define	XFS_MMAPLOCK_SHARED	(1<<5)
> > > +
> > > +#define XFS_IOLOCK_FLAG_SHIFT	0
> > > +#define XFS_ILOCK_FLAG_SHIFT	2
> > > +#define XFS_MMAPLOCK_FLAG_SHIFT	4
> > > +
> > > +#define XFS_SHARED_LOCK_SHIFT	1
> > > +
> > > +#define XFS_IOLOCK_EXCL		(1 << (XFS_IOLOCK_FLAG_SHIFT))
> > > +#define XFS_IOLOCK_SHARED	(XFS_IOLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT))
> > > +#define XFS_ILOCK_EXCL		(1 << (XFS_ILOCK_FLAG_SHIFT))
> > > +#define XFS_ILOCK_SHARED	(XFS_ILOCK_EXCL << (XFS_SHARED_LOCK_SHIFT))
> > > +#define XFS_MMAPLOCK_EXCL	(1 << (XFS_MMAPLOCK_FLAG_SHIFT))
> > > +#define XFS_MMAPLOCK_SHARED	(XFS_MMAPLOCK_EXCL << (XFS_SHARED_LOCK_SHIFT))
> > >  
> > 
> > Any reason for the extra params around the shift values?
> > 
> > Brian
> > 
> > >  #define XFS_LOCK_MASK		(XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED \
> > >  				| XFS_ILOCK_EXCL | XFS_ILOCK_SHARED \
> > > @@ -412,7 +419,7 @@ void		xfs_ilock(xfs_inode_t *, uint);
> > >  int		xfs_ilock_nowait(xfs_inode_t *, uint);
> > >  void		xfs_iunlock(xfs_inode_t *, uint);
> > >  void		xfs_ilock_demote(xfs_inode_t *, uint);
> > > -int		xfs_isilocked(xfs_inode_t *, uint);
> > > +bool		xfs_isilocked(struct xfs_inode *, uint);
> > >  uint		xfs_ilock_data_map_shared(struct xfs_inode *);
> > >  uint		xfs_ilock_attr_map_shared(struct xfs_inode *);
> > >  
> > > -- 
> > > 2.26.2
> > > 
> > 
> 

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

[Pavel Reichl]

On 10/12/20 6:03 PM, Brian Foster wrote:
> On Fri, Oct 09, 2020 at 09:55:12PM +0200, Pavel Reichl wrote:
>> Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked().
>> __xfs_rwsem_islocked() is a helper function which encapsulates checking
>> state of rw_semaphores hold by inode.
>>
>> Signed-off-by: Pavel Reichl <preichl@redhat.com>
>> Suggested-by: Dave Chinner <dchinner@redhat.com>
>> Suggested-by: Eric Sandeen <sandeen@redhat.com>
>> Suggested-by: Darrick J. Wong <darrick.wong@oracle.com>
>> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>> ---
>>  fs/xfs/xfs_inode.c | 48 ++++++++++++++++++++++++++++++++++++++--------
>>  fs/xfs/xfs_inode.h | 21 +++++++++++++-------
>>  2 files changed, 54 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>> index c06129cffba9..7c1ceb4df4ec 100644
>> --- a/fs/xfs/xfs_inode.c
>> +++ b/fs/xfs/xfs_inode.c
>> @@ -345,9 +345,43 @@ xfs_ilock_demote(
>>  }
>>  
>>  #if defined(DEBUG) || defined(XFS_WARN)
>> -int
>> +static inline bool
>> +__xfs_rwsem_islocked(
>> +	struct rw_semaphore	*rwsem,
>> +	int			lock_flags)
>> +{
>> +	int			arg;
>> +
>> +	if (!debug_locks)
>> +		return rwsem_is_locked(rwsem);
>> +
>> +	if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) {
>> +		/*
>> +		 * The caller could be asking if we have (shared | excl)
>> +		 * access to the lock. Ask lockdep if the rwsem is
>> +		 * locked either for read or write access.
>> +		 *
>> +		 * The caller could also be asking if we have only
>> +		 * shared access to the lock. Holding a rwsem
>> +		 * write-locked implies read access as well, so the
>> +		 * request to lockdep is the same for this case.
>> +		 */
>> +		arg = -1;
>> +	} else {
>> +		/*
>> +		 * The caller is asking if we have only exclusive access
>> +		 * to the lock. Ask lockdep if the rwsem is locked for
>> +		 * write access.
>> +		 */
>> +		arg = 0;
>> +	}
...
> 
> Also, I find the pattern of shifting in the caller slightly confusing,
> particularly with the 'lock_flags' name being passed down through the
> caller. Any reason we couldn't pass the shift value as a parameter and
> do the shift at the top of the function so the logic is clear and in one
> place?
> 

Hi Brian, is following change what you had in mind? Thanks!


>> @@ -349,14 +349,16 @@ xfs_ilock_demote(
 static inline bool
 __xfs_rwsem_islocked(
 	struct rw_semaphore	*rwsem,
-	int			lock_flags)
+	int			lock_flags,
+	int			shift)
 {
 	int			arg;
+	const int		shifted_lock_flags = lock_flags >> shift;
 
 	if (!debug_locks)
 		return rwsem_is_locked(rwsem);
 
-	if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) {
+	if (shifted_lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) {
 		/*
 		 * The caller could be asking if we have (shared | excl)
 		 * access to the lock. Ask lockdep if the rwsem is
@@ -387,20 +389,20 @@ xfs_isilocked(
 {
 	if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) {
 		ASSERT(!(lock_flags & ~(XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)));
-		return __xfs_rwsem_islocked(&ip->i_lock,
-				(lock_flags >> XFS_ILOCK_FLAG_SHIFT));
+		return __xfs_rwsem_islocked(&ip->i_lock, lock_flags,
+				XFS_ILOCK_FLAG_SHIFT);
 	}
 
 	if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) {
 		ASSERT(!(lock_flags &
 			~(XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)));
-		return __xfs_rwsem_islocked(&ip->i_mmaplock,
-				(lock_flags >> XFS_MMAPLOCK_FLAG_SHIFT));
+		return __xfs_rwsem_islocked(&ip->i_mmaplock, lock_flags,
+				XFS_MMAPLOCK_FLAG_SHIFT);
 	}
 
 	if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) {
-		return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem,
-				(lock_flags >> XFS_IOLOCK_FLAG_SHIFT));
+		return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem, lock_flags,
+				XFS_IOLOCK_FLAG_SHIFT);
 	}
 
 	ASSERT(0);

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

[Brian Foster]

On Wed, Oct 14, 2020 at 11:04:31PM +0200, Pavel Reichl wrote:
> 
> 
> On 10/12/20 6:03 PM, Brian Foster wrote:
> > On Fri, Oct 09, 2020 at 09:55:12PM +0200, Pavel Reichl wrote:
> >> Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked().
> >> __xfs_rwsem_islocked() is a helper function which encapsulates checking
> >> state of rw_semaphores hold by inode.
> >>
> >> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> >> Suggested-by: Dave Chinner <dchinner@redhat.com>
> >> Suggested-by: Eric Sandeen <sandeen@redhat.com>
> >> Suggested-by: Darrick J. Wong <darrick.wong@oracle.com>
> >> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> >> ---
> >>  fs/xfs/xfs_inode.c | 48 ++++++++++++++++++++++++++++++++++++++--------
> >>  fs/xfs/xfs_inode.h | 21 +++++++++++++-------
> >>  2 files changed, 54 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> >> index c06129cffba9..7c1ceb4df4ec 100644
> >> --- a/fs/xfs/xfs_inode.c
> >> +++ b/fs/xfs/xfs_inode.c
> >> @@ -345,9 +345,43 @@ xfs_ilock_demote(
> >>  }
> >>  
> >>  #if defined(DEBUG) || defined(XFS_WARN)
> >> -int
> >> +static inline bool
> >> +__xfs_rwsem_islocked(
> >> +	struct rw_semaphore	*rwsem,
> >> +	int			lock_flags)
> >> +{
> >> +	int			arg;
> >> +
> >> +	if (!debug_locks)
> >> +		return rwsem_is_locked(rwsem);
> >> +
> >> +	if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) {
> >> +		/*
> >> +		 * The caller could be asking if we have (shared | excl)
> >> +		 * access to the lock. Ask lockdep if the rwsem is
> >> +		 * locked either for read or write access.
> >> +		 *
> >> +		 * The caller could also be asking if we have only
> >> +		 * shared access to the lock. Holding a rwsem
> >> +		 * write-locked implies read access as well, so the
> >> +		 * request to lockdep is the same for this case.
> >> +		 */
> >> +		arg = -1;
> >> +	} else {
> >> +		/*
> >> +		 * The caller is asking if we have only exclusive access
> >> +		 * to the lock. Ask lockdep if the rwsem is locked for
> >> +		 * write access.
> >> +		 */
> >> +		arg = 0;
> >> +	}
> ...
> > 
> > Also, I find the pattern of shifting in the caller slightly confusing,
> > particularly with the 'lock_flags' name being passed down through the
> > caller. Any reason we couldn't pass the shift value as a parameter and
> > do the shift at the top of the function so the logic is clear and in one
> > place?
> > 
> 
> Hi Brian, is following change what you had in mind? Thanks!
> 

Yep, pretty much. I find shifted_lock_flags to be a little verbose as a
name. I'd be fine with just doing something like 'lock_flags >>= shift'
near the top of the function, but that's more of a personal nit. I also
like Christoph's suggestion to avoid the arg variable (along with the
comment update suggested in the discussion with Darrick).

Brian

> 
> >> @@ -349,14 +349,16 @@ xfs_ilock_demote(
>  static inline bool
>  __xfs_rwsem_islocked(
>  	struct rw_semaphore	*rwsem,
> -	int			lock_flags)
> +	int			lock_flags,
> +	int			shift)
>  {
>  	int			arg;
> +	const int		shifted_lock_flags = lock_flags >> shift;
>  
>  	if (!debug_locks)
>  		return rwsem_is_locked(rwsem);
>  
> -	if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) {
> +	if (shifted_lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) {
>  		/*
>  		 * The caller could be asking if we have (shared | excl)
>  		 * access to the lock. Ask lockdep if the rwsem is
> @@ -387,20 +389,20 @@ xfs_isilocked(
>  {
>  	if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) {
>  		ASSERT(!(lock_flags & ~(XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)));
> -		return __xfs_rwsem_islocked(&ip->i_lock,
> -				(lock_flags >> XFS_ILOCK_FLAG_SHIFT));
> +		return __xfs_rwsem_islocked(&ip->i_lock, lock_flags,
> +				XFS_ILOCK_FLAG_SHIFT);
>  	}
>  
>  	if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) {
>  		ASSERT(!(lock_flags &
>  			~(XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)));
> -		return __xfs_rwsem_islocked(&ip->i_mmaplock,
> -				(lock_flags >> XFS_MMAPLOCK_FLAG_SHIFT));
> +		return __xfs_rwsem_islocked(&ip->i_mmaplock, lock_flags,
> +				XFS_MMAPLOCK_FLAG_SHIFT);
>  	}
>  
>  	if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) {
> -		return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem,
> -				(lock_flags >> XFS_IOLOCK_FLAG_SHIFT));
> +		return __xfs_rwsem_islocked(&VFS_I(ip)->i_rwsem, lock_flags,
> +				XFS_IOLOCK_FLAG_SHIFT);
>  	}
>  
>  	ASSERT(0);
> 

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

[Christoph Hellwig]

On Fri, Oct 09, 2020 at 09:55:12PM +0200, Pavel Reichl wrote:
> Refactor xfs_isilocked() to use newly introduced __xfs_rwsem_islocked().
> __xfs_rwsem_islocked() is a helper function which encapsulates checking
> state of rw_semaphores hold by inode.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> Suggested-by: Dave Chinner <dchinner@redhat.com>
> Suggested-by: Eric Sandeen <sandeen@redhat.com>
> Suggested-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_inode.c | 48 ++++++++++++++++++++++++++++++++++++++--------
>  fs/xfs/xfs_inode.h | 21 +++++++++++++-------
>  2 files changed, 54 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c06129cffba9..7c1ceb4df4ec 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -345,9 +345,43 @@ xfs_ilock_demote(
>  }
>  
>  #if defined(DEBUG) || defined(XFS_WARN)
> -int
> +static inline bool
> +__xfs_rwsem_islocked(
> +	struct rw_semaphore	*rwsem,
> +	int			lock_flags)
> +{
> +	int			arg;
> +
> +	if (!debug_locks)
> +		return rwsem_is_locked(rwsem);
> +
> +	if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) {
> +		/*
> +		 * The caller could be asking if we have (shared | excl)
> +		 * access to the lock. Ask lockdep if the rwsem is
> +		 * locked either for read or write access.
> +		 *
> +		 * The caller could also be asking if we have only
> +		 * shared access to the lock. Holding a rwsem
> +		 * write-locked implies read access as well, so the
> +		 * request to lockdep is the same for this case.
> +		 */
> +		arg = -1;
> +	} else {
> +		/*
> +		 * The caller is asking if we have only exclusive access
> +		 * to the lock. Ask lockdep if the rwsem is locked for
> +		 * write access.
> +		 */
> +		arg = 0;
> +	}
> +
> +	return lockdep_is_held_type(rwsem, arg);

Why not write this as:

	if (lock_flags & (1 << XFS_SHARED_LOCK_SHIFT)) {
		...
		return lockdep_is_held_type(rwsem, 1);
	}

	...
	return lockdep_is_held_type(rwsem, 0);
}

which seems a lot easier to read compare to the strange arg variable.

Otherwise this looks good to me:

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

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

[Pavel Reichl]

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

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

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 1b0a01b06a05..ced3b996cd8a 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3883,7 +3883,7 @@ xfs_bmapi_read(
 
 	ASSERT(*nmap >= 1);
 	ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_ENTIRE)));
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
 
 	if (WARN_ON_ONCE(!ifp))
 		return -EFSCORRUPTED;
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index a29f78a663ca..c8b1d4e4199a 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -763,7 +763,8 @@ xfs_break_layouts(
 	bool			retry;
 	int			error;
 
-	ASSERT(xfs_isilocked(XFS_I(inode), XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL));
+	ASSERT(xfs_isilocked(XFS_I(inode),
+			XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL));
 
 	do {
 		retry = false;
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 7c1ceb4df4ec..a3baec1c5bcf 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2781,7 +2781,7 @@ static void
 xfs_iunpin(
 	struct xfs_inode	*ip)
 {
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
 
 	trace_xfs_inode_unpin_nowait(ip, _RET_IP_);
 
@@ -3474,7 +3474,7 @@ xfs_iflush(
 	struct xfs_mount	*mp = ip->i_mount;
 	int			error;
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
 	ASSERT(xfs_isiflocked(ip));
 	ASSERT(ip->i_df.if_format != XFS_DINODE_FMT_BTREE ||
 	       ip->i_df.if_nextents > XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK));
diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index be67570badf8..57bfa5266c47 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -1803,7 +1803,7 @@ xfs_qm_vop_chown_reserve(
 	int			error;
 
 
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
 	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
 
 	delblks = ip->i_delayed_blks;
-- 
2.26.2

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

[Brian Foster]

On Fri, Oct 09, 2020 at 09:55:13PM +0200, Pavel Reichl wrote:
> Make whitespace follow the same pattern in all xfs_isilocked() calls.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_bmap.c | 2 +-
>  fs/xfs/xfs_file.c        | 3 ++-
>  fs/xfs/xfs_inode.c       | 4 ++--
>  fs/xfs/xfs_qm.c          | 2 +-
>  4 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 1b0a01b06a05..ced3b996cd8a 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3883,7 +3883,7 @@ xfs_bmapi_read(
>  
>  	ASSERT(*nmap >= 1);
>  	ASSERT(!(flags & ~(XFS_BMAPI_ATTRFORK | XFS_BMAPI_ENTIRE)));
> -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED|XFS_ILOCK_EXCL));
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
>  
>  	if (WARN_ON_ONCE(!ifp))
>  		return -EFSCORRUPTED;
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index a29f78a663ca..c8b1d4e4199a 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -763,7 +763,8 @@ xfs_break_layouts(
>  	bool			retry;
>  	int			error;
>  
> -	ASSERT(xfs_isilocked(XFS_I(inode), XFS_IOLOCK_SHARED|XFS_IOLOCK_EXCL));
> +	ASSERT(xfs_isilocked(XFS_I(inode),
> +			XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL));
>  
>  	do {
>  		retry = false;
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 7c1ceb4df4ec..a3baec1c5bcf 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2781,7 +2781,7 @@ static void
>  xfs_iunpin(
>  	struct xfs_inode	*ip)
>  {
> -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
>  
>  	trace_xfs_inode_unpin_nowait(ip, _RET_IP_);
>  
> @@ -3474,7 +3474,7 @@ xfs_iflush(
>  	struct xfs_mount	*mp = ip->i_mount;
>  	int			error;
>  
> -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
>  	ASSERT(xfs_isiflocked(ip));
>  	ASSERT(ip->i_df.if_format != XFS_DINODE_FMT_BTREE ||
>  	       ip->i_df.if_nextents > XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK));
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index be67570badf8..57bfa5266c47 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -1803,7 +1803,7 @@ xfs_qm_vop_chown_reserve(
>  	int			error;
>  
>  
> -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL | XFS_ILOCK_SHARED));
>  	ASSERT(XFS_IS_QUOTA_RUNNING(mp));
>  
>  	delblks = ip->i_delayed_blks;
> -- 
> 2.26.2
> 

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

[Christoph Hellwig]

Looks good,

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

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

[Pavel Reichl]

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

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

Fixes: ecfea3f0c8c6 ("xfs: split xfs_bmap_shift_extents")

Suggested-by: Dave Chinner <dchinner@redhat.com>
Signed-off-by: Pavel Reichl <preichl@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.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 ced3b996cd8a..ff5cc8a5d476 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5787,7 +5787,8 @@ xfs_bmap_collapse_extents(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -EIO;
 
-	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));
+	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 
 	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
 		error = xfs_iread_extents(tp, ip, whichfork);
@@ -5904,7 +5905,8 @@ xfs_bmap_insert_extents(
 	if (XFS_FORCED_SHUTDOWN(mp))
 		return -EIO;
 
-	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));
+	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
+	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
 
 	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
 		error = xfs_iread_extents(tp, ip, whichfork);
-- 
2.26.2

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

[Brian Foster]

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

Reviewed-by: Brian Foster <bfoster@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 ced3b996cd8a..ff5cc8a5d476 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5787,7 +5787,8 @@ xfs_bmap_collapse_extents(
>  	if (XFS_FORCED_SHUTDOWN(mp))
>  		return -EIO;
>  
> -	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));
> +	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  
>  	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
>  		error = xfs_iread_extents(tp, ip, whichfork);
> @@ -5904,7 +5905,8 @@ xfs_bmap_insert_extents(
>  	if (XFS_FORCED_SHUTDOWN(mp))
>  		return -EIO;
>  
> -	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL));
> +	ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> +	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
>  
>  	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
>  		error = xfs_iread_extents(tp, ip, whichfork);
> -- 
> 2.26.2
> 

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

[Christoph Hellwig]

Looks good,

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

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

[Pavel Reichl]

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

Release the lock in xfs_btree_split() just before the work-queue
executing xfs_btree_split_worker() is scheduled and make
xfs_btree_split_worker() to acquire the lock as a first thing and
release it just before returning from the function. This it done so the
ownership of the lock is transfered between kernel threads and thus
lockdep won't complain about lock being held by a different kernel
thread.

Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 fs/xfs/libxfs/xfs_btree.c | 24 ++++++++++++
 fs/xfs/mrlock.h           | 78 ---------------------------------------
 fs/xfs/xfs_inode.c        | 50 +++++++++++++------------
 fs/xfs/xfs_inode.h        |  4 +-
 fs/xfs/xfs_iops.c         |  4 +-
 fs/xfs/xfs_linux.h        |  2 +-
 fs/xfs/xfs_super.c        |  6 +--
 7 files changed, 57 insertions(+), 111 deletions(-)
 delete mode 100644 fs/xfs/mrlock.h

diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index 2d25bab68764..d1fdb180f7ae 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -2816,6 +2816,12 @@ xfs_btree_split_worker(
 	unsigned long		pflags;
 	unsigned long		new_pflags = PF_MEMALLOC_NOFS;
 
+	/*
+	 * Update lockdep's lock ownership information to point to
+	 * this thread as the thread that scheduled this worker is waiting
+	 * for its completion.
+	 */
+	rwsem_acquire(&args->cur->bc_ino.ip->i_lock.dep_map, 0, 0, _RET_IP_);
 	/*
 	 * we are in a transaction context here, but may also be doing work
 	 * in kswapd context, and hence we may need to inherit that state
@@ -2829,6 +2835,12 @@ xfs_btree_split_worker(
 
 	args->result = __xfs_btree_split(args->cur, args->level, args->ptrp,
 					 args->key, args->curp, args->stat);
+	/*
+	 * Update lockdep's lock ownership information to reflect that we will
+	 * be transferring the ilock from this worker back to the scheduling
+	 * thread.
+	 */
+	rwsem_release(&args->cur->bc_ino.ip->i_lock.dep_map, _THIS_IP_);
 	complete(args->done);
 
 	current_restore_flags_nested(&pflags, new_pflags);
@@ -2863,8 +2875,20 @@ xfs_btree_split(
 	args.done = &done;
 	args.kswapd = current_is_kswapd();
 	INIT_WORK_ONSTACK(&args.work, xfs_btree_split_worker);
+	/*
+	 * Update lockdep's ownership information to reflect that we
+	 * will be transferring the ilock from this thread to the
+	 * worker.
+	 */
+	rwsem_release(&cur->bc_ino.ip->i_lock.dep_map, _THIS_IP_);
 	queue_work(xfs_alloc_wq, &args.work);
 	wait_for_completion(&done);
+	/*
+	 * Update lockdep's lock ownership information to point to
+	 * this thread as the lock owner now that the worker item is
+	 * done.
+	 */
+	rwsem_acquire(&cur->bc_ino.ip->i_lock.dep_map, 0, 0, _RET_IP_);
 	destroy_work_on_stack(&args.work);
 	return args.result;
 }
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 a3baec1c5bcf..213a4a947854 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);
 
@@ -384,16 +385,17 @@ xfs_isilocked(
 	struct xfs_inode	*ip,
 	uint			lock_flags)
 {
-	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
-		if (!(lock_flags & XFS_ILOCK_SHARED))
-			return !!ip->i_lock.mr_writer;
-		return rwsem_is_locked(&ip->i_lock.mr_lock);
+	if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) {
+		ASSERT(!(lock_flags & ~(XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)));
+		return __xfs_rwsem_islocked(&ip->i_lock,
+				(lock_flags >> XFS_ILOCK_FLAG_SHIFT));
 	}
 
-	if (lock_flags & (XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) {
-		if (!(lock_flags & XFS_MMAPLOCK_SHARED))
-			return !!ip->i_mmaplock.mr_writer;
-		return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
+	if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) {
+		ASSERT(!(lock_flags &
+			~(XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)));
+		return __xfs_rwsem_islocked(&ip->i_mmaplock,
+				(lock_flags >> XFS_MMAPLOCK_FLAG_SHIFT));
 	}
 
 	if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) {
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index 77776af75c77..896a088989f2 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -39,8 +39,8 @@ typedef struct xfs_inode {
 
 	/* Transaction and locking information. */
 	struct xfs_inode_log_item *i_itemp;	/* logging information */
-	mrlock_t		i_lock;		/* inode lock */
-	mrlock_t		i_mmaplock;	/* inode mmap IO lock */
+	struct rw_semaphore	i_lock;		/* inode lock */
+	struct rw_semaphore	i_mmaplock;	/* inode mmap IO lock */
 	atomic_t		i_pincount;	/* inode pin count */
 
 	/*
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 80a13c8561d8..66cca3e599c7 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -1336,9 +1336,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 ab737fed7b12..ba37217f86d2 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>
@@ -61,6 +60,7 @@ typedef __u32			xfs_nlink_t;
 #include <linux/ratelimit.h>
 #include <linux/rhashtable.h>
 #include <linux/xattr.h>
+#include <linux/rwsem.h>
 
 #include <asm/page.h>
 #include <asm/div64.h>
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 71ac6c1cdc36..00be9cfa29fa 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -708,10 +708,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.26.2

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

[Brian Foster]

On Fri, Oct 09, 2020 at 09:55:15PM +0200, Pavel Reichl wrote:
> Remove mrlock_t as it does not provide any extra value over
> rw_semaphores. Make i_lock and i_mmaplock native rw_semaphores and
> replace mr*() functions with native rwsem calls.
> 
> Release the lock in xfs_btree_split() just before the work-queue
> executing xfs_btree_split_worker() is scheduled and make
> xfs_btree_split_worker() to acquire the lock as a first thing and
> release it just before returning from the function. This it done so the
> ownership of the lock is transfered between kernel threads and thus
> lockdep won't complain about lock being held by a different kernel
> thread.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_btree.c | 24 ++++++++++++
>  fs/xfs/mrlock.h           | 78 ---------------------------------------
>  fs/xfs/xfs_inode.c        | 50 +++++++++++++------------
>  fs/xfs/xfs_inode.h        |  4 +-
>  fs/xfs/xfs_iops.c         |  4 +-
>  fs/xfs/xfs_linux.h        |  2 +-
>  fs/xfs/xfs_super.c        |  6 +--
>  7 files changed, 57 insertions(+), 111 deletions(-)
>  delete mode 100644 fs/xfs/mrlock.h
> 
> diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
> index 2d25bab68764..d1fdb180f7ae 100644
> --- a/fs/xfs/libxfs/xfs_btree.c
> +++ b/fs/xfs/libxfs/xfs_btree.c
...
> @@ -2863,8 +2875,20 @@ xfs_btree_split(
>  	args.done = &done;
>  	args.kswapd = current_is_kswapd();
>  	INIT_WORK_ONSTACK(&args.work, xfs_btree_split_worker);
> +	/*
> +	 * Update lockdep's ownership information to reflect that we
> +	 * will be transferring the ilock from this thread to the
> +	 * worker.
> +	 */

Can we update this comment to explain why we need to do this? E.g., I'm
assuming there's a lockdep splat somewhere down in the split worker
without it, but it's not immediately clear where and so it might not be
obvious if we're ever able to remove this.

> +	rwsem_release(&cur->bc_ino.ip->i_lock.dep_map, _THIS_IP_);
>  	queue_work(xfs_alloc_wq, &args.work);
>  	wait_for_completion(&done);
> +	/*
> +	 * Update lockdep's lock ownership information to point to
> +	 * this thread as the lock owner now that the worker item is
> +	 * done.
> +	 */
> +	rwsem_acquire(&cur->bc_ino.ip->i_lock.dep_map, 0, 0, _RET_IP_);
>  	destroy_work_on_stack(&args.work);
>  	return args.result;
>  }
...
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index a3baec1c5bcf..213a4a947854 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
...
> @@ -384,16 +385,17 @@ xfs_isilocked(
>  	struct xfs_inode	*ip,
>  	uint			lock_flags)
>  {
> -	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> -		if (!(lock_flags & XFS_ILOCK_SHARED))
> -			return !!ip->i_lock.mr_writer;
> -		return rwsem_is_locked(&ip->i_lock.mr_lock);
> +	if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) {
> +		ASSERT(!(lock_flags & ~(XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)));
> +		return __xfs_rwsem_islocked(&ip->i_lock,
> +				(lock_flags >> XFS_ILOCK_FLAG_SHIFT));
>  	}
>  
> -	if (lock_flags & (XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) {
> -		if (!(lock_flags & XFS_MMAPLOCK_SHARED))
> -			return !!ip->i_mmaplock.mr_writer;
> -		return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
> +	if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) {
> +		ASSERT(!(lock_flags &
> +			~(XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)));
> +		return __xfs_rwsem_islocked(&ip->i_mmaplock,
> +				(lock_flags >> XFS_MMAPLOCK_FLAG_SHIFT));
>  	}
>  
>  	if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) {

Can we add a similar assert for this case as we have for the others?
Otherwise the rest looks fairly straightforward to me.

Brian

> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index 77776af75c77..896a088989f2 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -39,8 +39,8 @@ typedef struct xfs_inode {
>  
>  	/* Transaction and locking information. */
>  	struct xfs_inode_log_item *i_itemp;	/* logging information */
> -	mrlock_t		i_lock;		/* inode lock */
> -	mrlock_t		i_mmaplock;	/* inode mmap IO lock */
> +	struct rw_semaphore	i_lock;		/* inode lock */
> +	struct rw_semaphore	i_mmaplock;	/* inode mmap IO lock */
>  	atomic_t		i_pincount;	/* inode pin count */
>  
>  	/*
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 80a13c8561d8..66cca3e599c7 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -1336,9 +1336,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 ab737fed7b12..ba37217f86d2 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>
> @@ -61,6 +60,7 @@ typedef __u32			xfs_nlink_t;
>  #include <linux/ratelimit.h>
>  #include <linux/rhashtable.h>
>  #include <linux/xattr.h>
> +#include <linux/rwsem.h>
>  
>  #include <asm/page.h>
>  #include <asm/div64.h>
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 71ac6c1cdc36..00be9cfa29fa 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -708,10 +708,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.26.2
> 

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

[Pavel Reichl]

On 10/12/20 6:04 PM, Brian Foster wrote:
> ...
>> @@ -2863,8 +2875,20 @@ xfs_btree_split(
>>  	args.done = &done;
>>  	args.kswapd = current_is_kswapd();
>>  	INIT_WORK_ONSTACK(&args.work, xfs_btree_split_worker);
>> +	/*
>> +	 * Update lockdep's ownership information to reflect that we
>> +	 * will be transferring the ilock from this thread to the
>> +	 * worker.
>> +	 */
> 
> Can we update this comment to explain why we need to do this? E.g., I'm
> assuming there's a lockdep splat somewhere down in the split worker
> without it, but it's not immediately clear where and so it might not be
> obvious if we're ever able to remove this.

Hi, would something like this work for you?

	/*
+	 * Update lockdep's ownership information to reflect that we
+	 * will be transferring the ilock from this thread to the
+	 * worker (xfs_btree_split_worker() run via queue_work()).
+	 * If the ownership transfer would not happen lockdep would
+	 * assert in the worker thread because the ilock would be owned
+	 * by the original thread.
+	 */

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

[Brian Foster]

On Mon, Oct 12, 2020 at 10:44:38PM +0200, Pavel Reichl wrote:
> 
> 
> On 10/12/20 6:04 PM, Brian Foster wrote:
> > ...
> >> @@ -2863,8 +2875,20 @@ xfs_btree_split(
> >>  	args.done = &done;
> >>  	args.kswapd = current_is_kswapd();
> >>  	INIT_WORK_ONSTACK(&args.work, xfs_btree_split_worker);
> >> +	/*
> >> +	 * Update lockdep's ownership information to reflect that we
> >> +	 * will be transferring the ilock from this thread to the
> >> +	 * worker.
> >> +	 */
> > 
> > Can we update this comment to explain why we need to do this? E.g., I'm
> > assuming there's a lockdep splat somewhere down in the split worker
> > without it, but it's not immediately clear where and so it might not be
> > obvious if we're ever able to remove this.
> 
> Hi, would something like this work for you?
> 
> 	/*
> +	 * Update lockdep's ownership information to reflect that we
> +	 * will be transferring the ilock from this thread to the
> +	 * worker (xfs_btree_split_worker() run via queue_work()).
> +	 * If the ownership transfer would not happen lockdep would
> +	 * assert in the worker thread because the ilock would be owned
> +	 * by the original thread.
> +	 */
> 

That doesn't really answer the question. Do you have a record of the
lockdep error message that occurs without this state transfer, by
chance?

Brian

> 

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

[Pavel Reichl]

On 10/13/20 1:04 PM, Brian Foster wrote:
> On Mon, Oct 12, 2020 at 10:44:38PM +0200, Pavel Reichl wrote:
>>
>>
>> On 10/12/20 6:04 PM, Brian Foster wrote:
>>> ...
>>>> @@ -2863,8 +2875,20 @@ xfs_btree_split(
>>>>  	args.done = &done;
>>>>  	args.kswapd = current_is_kswapd();
>>>>  	INIT_WORK_ONSTACK(&args.work, xfs_btree_split_worker);
>>>> +	/*
>>>> +	 * Update lockdep's ownership information to reflect that we
>>>> +	 * will be transferring the ilock from this thread to the
>>>> +	 * worker.
>>>> +	 */
>>>
>>> Can we update this comment to explain why we need to do this? E.g., I'm
>>> assuming there's a lockdep splat somewhere down in the split worker
>>> without it, but it's not immediately clear where and so it might not be
>>> obvious if we're ever able to remove this.
>>
>> Hi, would something like this work for you?
>>
>> 	/*
>> +	 * Update lockdep's ownership information to reflect that we
>> +	 * will be transferring the ilock from this thread to the
>> +	 * worker (xfs_btree_split_worker() run via queue_work()).
>> +	 * If the ownership transfer would not happen lockdep would
>> +	 * assert in the worker thread because the ilock would be owned
>> +	 * by the original thread.
>> +	 */
>>
> 
> That doesn't really answer the question. Do you have a record of the
> lockdep error message that occurs without this state transfer, by
> chance?
> 
> Brian

Hi, please see this mail from Darrick - he hit the issue first - http://mail.spinics.net/lists/linux-xfs/msg38967.html

> 
>>
> 

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

[Brian Foster]

On Tue, Oct 13, 2020 at 03:39:03PM +0200, Pavel Reichl wrote:
> 
> 
> On 10/13/20 1:04 PM, Brian Foster wrote:
> > On Mon, Oct 12, 2020 at 10:44:38PM +0200, Pavel Reichl wrote:
> >>
> >>
> >> On 10/12/20 6:04 PM, Brian Foster wrote:
> >>> ...
> >>>> @@ -2863,8 +2875,20 @@ xfs_btree_split(
> >>>>  	args.done = &done;
> >>>>  	args.kswapd = current_is_kswapd();
> >>>>  	INIT_WORK_ONSTACK(&args.work, xfs_btree_split_worker);
> >>>> +	/*
> >>>> +	 * Update lockdep's ownership information to reflect that we
> >>>> +	 * will be transferring the ilock from this thread to the
> >>>> +	 * worker.
> >>>> +	 */
> >>>
> >>> Can we update this comment to explain why we need to do this? E.g., I'm
> >>> assuming there's a lockdep splat somewhere down in the split worker
> >>> without it, but it's not immediately clear where and so it might not be
> >>> obvious if we're ever able to remove this.
> >>
> >> Hi, would something like this work for you?
> >>
> >> 	/*
> >> +	 * Update lockdep's ownership information to reflect that we
> >> +	 * will be transferring the ilock from this thread to the
> >> +	 * worker (xfs_btree_split_worker() run via queue_work()).
> >> +	 * If the ownership transfer would not happen lockdep would
> >> +	 * assert in the worker thread because the ilock would be owned
> >> +	 * by the original thread.
> >> +	 */
> >>
> > 
> > That doesn't really answer the question. Do you have a record of the
> > lockdep error message that occurs without this state transfer, by
> > chance?
> > 
> > Brian
> 
> Hi, please see this mail from Darrick - he hit the issue first - http://mail.spinics.net/lists/linux-xfs/msg38967.html
> 

Ah, I see.. thanks. I was thinking there was some kind of lock imbalance
warning or something, but in reality I think something like the
following is sufficient:

"Update lockdep's ownership information to reflect transfer of the ilock
from the current task to the worker. Otherwise assertions that the lock
is held (such as when logging the inode) might fail due to incorrect
task owner state."

Brian

> > 
> >>
> > 
> 

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

[Pavel Reichl]

> ...
>> @@ -384,16 +385,17 @@ xfs_isilocked(
>>  	struct xfs_inode	*ip,
>>  	uint			lock_flags)
>>  {
>> -	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
>> -		if (!(lock_flags & XFS_ILOCK_SHARED))
>> -			return !!ip->i_lock.mr_writer;
>> -		return rwsem_is_locked(&ip->i_lock.mr_lock);
>> +	if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) {
>> +		ASSERT(!(lock_flags & ~(XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)));
>> +		return __xfs_rwsem_islocked(&ip->i_lock,
>> +				(lock_flags >> XFS_ILOCK_FLAG_SHIFT));
>>  	}
>>  
>> -	if (lock_flags & (XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) {
>> -		if (!(lock_flags & XFS_MMAPLOCK_SHARED))
>> -			return !!ip->i_mmaplock.mr_writer;
>> -		return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
>> +	if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) {
>> +		ASSERT(!(lock_flags &
>> +			~(XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)));
>> +		return __xfs_rwsem_islocked(&ip->i_mmaplock,
>> +				(lock_flags >> XFS_MMAPLOCK_FLAG_SHIFT));
>>  	}
>>  
>>  	if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) {
> 
> Can we add a similar assert for this case as we have for the others?
> Otherwise the rest looks fairly straightforward to me.
> 

Sure we can! But do we want to?

I think that these asserts are supposed to make sure that only flags for one of the inode's locks are used eg. ILOCK, MMAPLOCK or IOLOCK but no combination! So if we reach this 3rd condition we already know that the flags for ILOCK and MMAPLOCK were not set. However if there's possibility for more locks to be added in the future or just for the 'code symmetry' purposes - I have no problem to update the code.
 

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

[Darrick J. Wong]

On Mon, Oct 12, 2020 at 11:02:51PM +0200, Pavel Reichl wrote:
> 
> > ...
> >> @@ -384,16 +385,17 @@ xfs_isilocked(
> >>  	struct xfs_inode	*ip,
> >>  	uint			lock_flags)
> >>  {
> >> -	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> >> -		if (!(lock_flags & XFS_ILOCK_SHARED))
> >> -			return !!ip->i_lock.mr_writer;
> >> -		return rwsem_is_locked(&ip->i_lock.mr_lock);
> >> +	if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) {
> >> +		ASSERT(!(lock_flags & ~(XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)));
> >> +		return __xfs_rwsem_islocked(&ip->i_lock,
> >> +				(lock_flags >> XFS_ILOCK_FLAG_SHIFT));
> >>  	}
> >>  
> >> -	if (lock_flags & (XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) {
> >> -		if (!(lock_flags & XFS_MMAPLOCK_SHARED))
> >> -			return !!ip->i_mmaplock.mr_writer;
> >> -		return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
> >> +	if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) {
> >> +		ASSERT(!(lock_flags &
> >> +			~(XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)));
> >> +		return __xfs_rwsem_islocked(&ip->i_mmaplock,
> >> +				(lock_flags >> XFS_MMAPLOCK_FLAG_SHIFT));
> >>  	}
> >>  
> >>  	if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) {
> > 
> > Can we add a similar assert for this case as we have for the others?
> > Otherwise the rest looks fairly straightforward to me.
> > 
> 
> Sure we can! But do we want to?
> 
> I think that these asserts are supposed to make sure that only flags
> for one of the inode's locks are used eg. ILOCK, MMAPLOCK or IOLOCK
> but no combination! So if we reach this 3rd condition we already know
> that the flags for ILOCK and MMAPLOCK were not set. However if there's
> possibility for more locks to be added in the future or just for the
> 'code symmetry' purposes - I have no problem to update the code.

It's generally a good idea not to leave logic bombs of the sort where
where the debugging code can bitrot into incorrectness if someone
unwittingly adds another level of locking later.

(That said, I really hope we don't; I already consider it a little
strange to have separate io and mmap locks...)

--D

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

[Brian Foster]

On Mon, Oct 12, 2020 at 11:02:51PM +0200, Pavel Reichl wrote:
> 
> > ...
> >> @@ -384,16 +385,17 @@ xfs_isilocked(
> >>  	struct xfs_inode	*ip,
> >>  	uint			lock_flags)
> >>  {
> >> -	if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> >> -		if (!(lock_flags & XFS_ILOCK_SHARED))
> >> -			return !!ip->i_lock.mr_writer;
> >> -		return rwsem_is_locked(&ip->i_lock.mr_lock);
> >> +	if (lock_flags & (XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)) {
> >> +		ASSERT(!(lock_flags & ~(XFS_ILOCK_EXCL | XFS_ILOCK_SHARED)));
> >> +		return __xfs_rwsem_islocked(&ip->i_lock,
> >> +				(lock_flags >> XFS_ILOCK_FLAG_SHIFT));
> >>  	}
> >>  
> >> -	if (lock_flags & (XFS_MMAPLOCK_EXCL|XFS_MMAPLOCK_SHARED)) {
> >> -		if (!(lock_flags & XFS_MMAPLOCK_SHARED))
> >> -			return !!ip->i_mmaplock.mr_writer;
> >> -		return rwsem_is_locked(&ip->i_mmaplock.mr_lock);
> >> +	if (lock_flags & (XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)) {
> >> +		ASSERT(!(lock_flags &
> >> +			~(XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)));
> >> +		return __xfs_rwsem_islocked(&ip->i_mmaplock,
> >> +				(lock_flags >> XFS_MMAPLOCK_FLAG_SHIFT));
> >>  	}
> >>  
> >>  	if (lock_flags & (XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED)) {
> > 
> > Can we add a similar assert for this case as we have for the others?
> > Otherwise the rest looks fairly straightforward to me.
> > 
> 
> Sure we can! But do we want to?
> 
> I think that these asserts are supposed to make sure that only flags for one of the inode's locks are used eg. ILOCK, MMAPLOCK or IOLOCK but no combination! So if we reach this 3rd condition we already know that the flags for ILOCK and MMAPLOCK were not set. However if there's possibility for more locks to be added in the future or just for the 'code symmetry' purposes - I have no problem to update the code.
>  
> 

Fair point. I do tend to agree with Darrick that I'd rather not rely on
even mildly tricky/subtle logic to optimize away things like asserts
(that are already optimized away on production kernels). Somebody could
come along, refactor this function and very easily gloss over the fact
that particular checks are missing. That said, I suppose it's
technically dead code with the current logic so it's not that big of a
deal IMO.

Brian

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

[Christoph Hellwig]

Looks good,

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